[SSSD] [PATCH] Improvements for LDAP Password Policy support

Ralf Haferkamp rhafer at suse.de
Thu Mar 18 15:00:02 UTC 2010


Am Donnerstag 18 März 2010 15:25:49 schrieb Dmitri Pal:
> Ralf Haferkamp wrote:
> > Am Donnerstag 18 März 2010 12:42:23 schrieb Simo Sorce:
> >> On Wed, 17 Mar 2010 15:33:38 +0100
> >> 
> >> Ralf Haferkamp <rhafer at suse.de> wrote:
> >>> Hi,
> >>> 
> >>> here's another set of enhancements to the LDAP Password Policy
> >>> support in the PAM module and the LDAP backend. The PAM module now
> >>> issues warning when either the grace counter or the expire counter
> >>> of the LDAP Ppolicy Control in > 0.
> >>> 
> >>> Addtionally I changed the detection for Ppolicy support of the
> >>> LDAP Server a bit. If the Server returned the Ppolicy Control in
> >>> the Bind Response  ppolicy support is assumed.
> >>> 
> >>> I left the original check for "pwdAttribute" intact, though I
> >>> think it doesn't make a lot of sense. The "pwdAttribute" LDAP
> >>> Attribute is usually not part of the user's entry, it's part of
> >>> the Entry that contains the Policy. Addtionally it might be
> >>> protected by ACLs and not be returned for anonymous (without
> >>> losing any functionality).
> >> 
> >> Ralf,
> >> patch looks mostly good, but there are some heavy coding style
> >> violations.
> >> 
> >> if ( condition ){ is not good, it should be: is (condition) {
> >> 
> >> same for some functions foo( ccc ); is not good, use foo(ccc);
> >> 
> >> Ie, no space in parenthesis, a space only after keywords, and
> >> always before the opening {
> >> 
> >> Don't use ++x, but x++ if possible.
> >> 
> >> Also there is at least one place where the return of talloc is not
> >> checked.
> >> 
> >> Finally please try to keep the 80 columns limit where possible.
> > 
> > Updated patch attached. I think I fixed the coding style issues.
> > Additionally I just noticed that my orginal patch broke password
> > resets via LDAP password policies. This should be fixed now as
> > well.
> > 
> > regards,
> > 
> > 	Ralf
> 
> +        data = talloc_size(pd, 2* sizeof(uint32_t));
> +        if (*data == NULL) {
> +            DEBUG(1, ("talloc_size failed.\n"));
> +            return ENOMEM;
> +        }
> 
> 
> The returned value check does not look right.
> I do not know if there are other places with similar logic.
There weren't (The one above was embarrasing enough. I must have 
overlooked the compiler warning :| ). Updated patch attached.

> My eye just caught it when I was scrolling through the patch...

Ralf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Improvements-for-LDAP-Password-Policy-support.patch
Type: text/x-patch
Size: 16279 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100318/9c8435f9/attachment.bin>


More information about the sssd-devel mailing list