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

[Tomas Babej]

On 04/15/2013 12:43 PM, Tomas Babej wrote:
> 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
>

I rebased the patch on top of current master.

Bumping, since this is planned as part of 3.4 (and there were some
requests for this feature).

Tomas

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