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

Ralf Haferkamp rhafer at suse.de
Thu Mar 18 16:29:37 UTC 2010


Am Donnerstag 18 März 2010 17:15:39 schrieb Dmitri Pal:
> Ralf Haferkamp wrote:
> > 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.
> 
> Shouldn't it be freed somewhere down within the same function?
As Sumit already pointed, that will happen when the associated
struct pam_data is talloc_free'd after the response was sent to the 
client.

Ralf



More information about the sssd-devel mailing list