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

Sumit Bose sbose at redhat.com
Thu Mar 18 16:24:34 UTC 2010


On Thu, Mar 18, 2010 at 12:15:39PM -0400, Dmitri Pal wrote:
> 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?
> 

No, this data will be send to a client in a later call and freed by a
talloc_free at the end to the client request.

bye,
Sumit

> >   
> >> My eye just caught it when I was scrolling through the patch...
> >>     
> >
> > Ralf
> >   
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
> 
> 
> -- 
> Thank you,
> Dmitri Pal
> 
> Engineering Manager IPA project,
> Red Hat Inc.
> 
> 
> -------------------------------
> Looking to carve out IT costs?
> www.redhat.com/carveoutcosts/
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list