[Freeipa-devel] [PATCH 0034] Deny LDAP binds for user accounts with expired principal
Martin Kosek
mkosek at redhat.com
Thu Mar 27 13:40:53 UTC 2014
On 01/07/2014 01:47 PM, Tomas Babej wrote:
>
> 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
>
Simo, Alexander - are you know OK with Tomas' patch? The patch review is now
taking more than a year, so I think it would be great to finish it :)
Martin
More information about the Freeipa-devel
mailing list