[Freeipa-devel] [PATCH 0034] Deny LDAP binds for user accounts with expired principal
Tomas Babej
tbabej at redhat.com
Wed Apr 30 15:07:48 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
This version is rebased on top of OTP patches, addresses Simo's comments
and brings unit tests to cover the functionality (however, they need to
be applied on top of my patches 183-185).
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0034-8-ipa-pwd-extop-Deny-LDAP-binds-for-accounts-with-expi.patch
Type: text/x-patch
Size: 3350 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140430/9d6f22c0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0186-ipatests-Add-test-for-denying-expired-principals.patch
Type: text/x-patch
Size: 2841 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140430/9d6f22c0/attachment-0001.bin>
More information about the Freeipa-devel
mailing list