[Freeipa-devel] [PATCH 0034] Deny LDAP binds for user accounts with expired principal
Martin Kosek
mkosek at redhat.com
Tue Apr 8 14:16:08 UTC 2014
On 03/27/2014 02:40 PM, Martin Kosek wrote:
> 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
Resending, it seems the message fell between cracks.
Martin
More information about the Freeipa-devel
mailing list