[Freeipa-devel] [PATCH 0034] Deny LDAP binds for user accounts with expired principal
Rob Crittenden
rcritten at redhat.com
Mon Apr 1 19:52:00 UTC 2013
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
More information about the Freeipa-devel
mailing list