[Freeipa-devel] [PATCH 0034] Deny LDAP binds for user accounts with expired principal
Tomas Babej
tbabej at redhat.com
Tue Jan 7 12:47:43 UTC 2014
On 01/07/2014 07:23 AM, Alexander Bokovoy wrote:
> On Mon, 06 Jan 2014, Tomas Babej wrote:
>>
>> 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
>
>>> From a93d1ec3ca8c7b6e8e754b474257695ba9018e07 Mon Sep 17 00:00:00 2001
>> From: Tomas Babej <tbabej at redhat.com>
>> Date: Mon, 6 Jan 2014 12:11:24 +0100
>> Subject: [PATCH] ipa-pwd-extop: Deny LDAP binds for user accounts
>> with expired
>> principal
>>
>> 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_UNWILLING_TO_PERFORM 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
>> ---
>> daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 45
>> ++++++++++++++++++++++-
>> 1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
>> b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
>> index
>> ef37b5e173359f9404b241c12d8a6dc6e77da128..568cba7189d1575d030b043daee61f713e9bac5f
>> 100644
>> --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
>> +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
>> @@ -1389,13 +1389,17 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
>> Slapi_Value *value = NULL;
>> Slapi_Attr *attr = NULL;
>> struct tm expire_tm;
>> + time_t current_time;
>> + time_t expire_time;
>> char *errMesg = "Internal operations error\n"; /* error message */
>> char *expire = NULL; /* passwordExpirationTime attribute value */
>> + char *principal_expire = NULL; /* krbPrincipalExpiration
>> attribute value */
>> char *dn = NULL; /* bind DN */
>> Slapi_Value *objectclass;
>> int method; /* authentication method */
>> int ret = 0;
>> char *principal = NULL;
>> + bool auth_failed = false;
>>
>> LOG_TRACE("=>\n");
>>
>> @@ -1422,7 +1426,7 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
>> const char *attrs_list[] = {SLAPI_USERPWD_ATTR,
>> "krbprincipalkey", "uid",
>> "krbprincipalname", "objectclass",
>> "passwordexpirationtime",
>> "passwordhistory",
>> - NULL};
>> + "krbprincipalexpiration", NULL};
>>
>> /* retrieve user entry */
>> ret = ipapwd_getEntry(dn, &entry, (char **) attrs_list);
>> @@ -1438,6 +1442,39 @@ 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 == 0) {
>> + ret = slapi_attr_first_value(attr, &value);
>> +
>> + if (ret == 0) {
>> + /* if it is, check whether the principal has not expired */
>> + principal_expire = slapi_value_get_string(value);
>> + memset(&expire_tm, 0, sizeof (expire_tm));
>> +
>> + if (strptime(principal_expire, "%Y%m%d%H%M%SZ",
>> &expire_tm)) {
>> + expire_time = mktime(&expire_tm);
>> +
>> + /* get current time */
>> + current_time = time(NULL);
>> +
>> + /* mktime returns -1 if the tm struct cannot be
>> represented as
>> + * as calendar time (seconds since the Epoch). This
>> might
>> + * happen with tm structs that are ill-formated or
>> on 32-bit
>> + * platforms with dates that would cause overflow
>> + * (year 2038 and later).
>> + * In such cases, skip the expiration check. */
>> +
>> + if (current_time > expire_time && expire_time > 0) {
>> + LOG_FATAL("kerberos principal in %s is
>> expired\n", dn);
>> + errMesg = "Kerberos principal is expired.";
>> + auth_failed = true;
>> + goto done;
>> + }
>> + }
> I think indenting is broken for these two brackets.
>
>
Thanks Alexander, fixed.
Updated version attached.
Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0034-7-ipa-pwd-extop-Deny-LDAP-binds-for-user-accounts-with.patch
Type: text/x-patch
Size: 4225 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140107/307539c9/attachment.bin>
More information about the Freeipa-devel
mailing list