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

[Tomas Babej]

On 04/08/2013 03:55 PM, Martin Kosek wrote:
> On 04/01/2013 09:52 PM, Rob Crittenden wrote:
>> Tomas Babej wrote:
>>> 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 :)
>> Small nit, the expiration error should probably use 'in' not 'on'.
>>
>> I'm not sure LDAP_INVALID_CREDENTIALS is the right return code. This implies
>> that the password is wrong. I think that LDAP_UNWILLING_TO_PERFORM is probably
>> more correct here. It is what we return on other policy failures.
>>
>> rob
>>
> Additional findings:
>
> 1) Lets not try to get current_time every time, but only when its needed (i.e.
> krbPrincipalExpiration is set) - AFAIK, it is not so cheap operation:
>
> +    /* get current time*/
> +    current_time = time(NULL);
>
> 2) Why using slapi_entry_attr_get_charptr and thus let 389-ds find the
> attribute again when we already have a pointer for the attribute? Would
> slapi_attr_first_value following slapi_entry_attr_find be faster approach?
>
> +    ret = slapi_entry_attr_find(entry, "krbPrincipalExpiration", &attr);
> +    if (ret != 0) {
> +        /* if it is, check whether the principal has not expired */
> +
> +        principal_expire = slapi_entry_attr_get_charptr(entry,
> +                               "krbprincipalexpiration");
>
> This way, we would not create a copy of this attribute value - we do not need a
> copy, we just do check against it.
>
>
> 3) Shouldn't we return -1 in the end when the auth fails? We do that in other
> pre_callbacks. Otherwise we just return 0 (success):
>
> +    if (auth_failed){
> +        slapi_send_ldap_result(pb, LDAP_INVALID_CREDENTIALS, NULL, errMesg,
> +                               0, NULL);
> +    }
> +
>       return 0;
>
> Martin

Thank you both for the review. I updated and retested the patch, with 
your suggestions incorporated.

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