[SSSD] [PATCHES] Additional AD provider enhancements

Simo Sorce simo at redhat.com
Fri Jul 6 12:09:33 UTC 2012


On Thu, 2012-07-05 at 20:53 -0400, Stephen Gallagher wrote:
> 
> 
> These patches should finish off the AD provider functionality for
> 1.9.0.
> All of these patches depend on those in the thread "Add Active
> Directory
> identity, auth and chpass providers to SSSD"
> 
> Patch 0001: Add another option 'ad_keytab' that unifies the
> krb5_keytab
> and ldap_krb5_keytab options.

Nack, please just use krb5_keytab, let's not proliferate options names
when unnecessary.

> Patch 0002: Add manpages and SSSDConfig API references for new AD
> provider options. This patch depends on "MAN: Unify "SEE ALSO"
> sections"

Here you also need to remove ad_keytab, but in it's place you may
specify that for the ad provider krb5_keytab is used both for validation
and for protecting the LDAP connections.

> Patch 0003: Add a function to the confdb to allow us to write a
> boolean
> value into the configuration database. (Used in patch 0004)

Why didn't you use ldb_msg_add_string() to set the bool ?

ldb_msg_add_empty(msg, attribute, LDB_FLAG_MOD_REPLACE, NULL);
If (val) {
    ldb_msg_add_string(msg, attribute, "True");
} else {
    ldb_msg_add_string(msg, attribute, "False");
}

would make the code  a lot more compact.

> Patch 0004: During initial configuration of the AD provider, force the
> option case_sensitive to false, since all AD deployments are
> case-insensitive. We do this here (and set it in the confdb) so that
> the
> responders will pick it up. Responders all start after the providers
> have registered with the monitor (or if the monitor gives up waiting
> after five seconds), so there should be no race here.

in confdb_set_bool() please use dom->case_sensitive that you just set,
and not 'false' directly, this way if we ever need a change (or someone
copy/paste the code elsewhere) we are consistent and only one line needs
to be changed to change behavior.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list