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

[Tomas Babej]

On 01/06/2014 12:16 PM, Tomas Babej wrote:
> 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
>

I updated the commit message to reflect better what the current version
of the patch implements.

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0034-6-ipa-pwd-extop-Deny-LDAP-binds-for-user-accounts-with.patch
Type: text/x-patch
Size: 4233 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140106/3ae46a11/attachment.bin>