[Freeipa-devel] [PATCH 0034] Deny LDAP binds for user accounts with expired principal

[Tomas Babej]

On 02/12/2013 06:23 PM, Simo Sorce wrote:
> On Tue, 2013-02-12 at 18:03 +0100, Tomas Babej wrote:
>> On 02/12/2013 05:50 PM, Tomas Babej wrote:
>>> Hi,
>>>
>>> This patch adds a check for krbprincipalexpiration attribute to
>>> pre_bind operation
>>> in ipa-pwd-extop dirsrv plugin. If the principal is expired, auth is
>>> denied and LDAP_INVALID_CREDENTIALS along with the error message is
>>> sent back to the client. Since krbprincipalexpiration attribute is
>> not
>>> mandatory, if there is no value set, the check is passed.
>>>
>>> https://fedorahosted.org/freeipa/ticket/3305
>
> Comments inline.
>> @@ -1166,6 +1173,29 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
>>           goto done;
>>       }
>>   
>> +    /* check the krbPrincipalExpiration attribute is present */
>> +    ret = slapi_entry_attr_find(entry, "krbPrincipalExpiration",
>> &attr);
>> +    if (!ret) {
> ret is not a boolean so probably better ti use (ret != 0)
>
>> +        /* if it is, check whether the principal has not expired */
>> +
>> +        principal_expire = slapi_entry_attr_get_charptr(entry,
>> +                               "krbprincipalexpiration");
>> +        memset(&expire_tm, 0, sizeof (expire_tm));
>> +
>> +        if (strptime(principal_expire, "%Y%m%d%H%M%SZ", &expire_tm)){
> style issue missing space between ) and {
>
>> +            expire_time = mktime(&expire_tm);
>> +            /* this might have overflown on 32-bit system */
> This comment does not help to point out what you want to put in
> evidence.
> if there is an overflow what is the consequence ? Or does it mean the
> next condition is taking that into account ?
Yeah, this was rather ill-worded. Replaced by a rather verbose
comment that hopefully clears things out.
>
>> +            if (current_time > expire_time && expire_time > 0){
> style issue missing space between ) and {
>
>> +                LOG_FATAL("kerberos principal has expired in user
>> entry: %s\n",
>> +                          dn);
> I think a better phrasing would be: "The kerberos principal on %s is
> expired\n"
>
>> +                errMesg = "Kerberos principal has expired.";
> s/has/is/
>
> The rest looks good to me.
>
> Simo.
>    
Styling issues fixed and updated patch attached :)

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0034-3-Deny-LDAP-binds-for-user-accounts-with-expired-princ.patch
Type: text/x-patch
Size: 4419 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130213/1731fb80/attachment.bin>