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

Dmitri Pal dpal at redhat.com
Thu Mar 18 14:25:49 UTC 2010


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.
My eye just caught it when I was scrolling through the patch... 





> ------------------------------------------------------------------------
>
> _______________________________________________
> 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/




More information about the sssd-devel mailing list