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 <tba...@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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
