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; + } + } + } + } + /* we aren't interested in host principals */ objectclass = slapi_value_new_string("ipaHost"); if ((slapi_entry_attr_has_syntax_value(entry, SLAPI_ATTR_OBJECTCLASS, objectclass)) == 1) { @@ -1560,6 +1597,12 @@ done: slapi_entry_free(entry); free_ipapwd_krbcfg(&krbcfg); + if (auth_failed){ + slapi_send_ldap_result(pb, LDAP_UNWILLING_TO_PERFORM, NULL, errMesg, + 0, NULL); + return -1; + } + return 0; } -- 1.8.4.2
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
