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

[Martin Kosek]

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