[Freeipa-devel] [PATCH 0034] Deny LDAP binds for user accounts with expired principal
Simo Sorce
simo at redhat.com
Wed Apr 30 18:47:53 UTC 2014
On Wed, 2014-04-30 at 17:07 +0200, Tomas Babej 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
>
> 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).
>
LGTM, but I haven't tested the code live.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
More information about the Freeipa-devel
mailing list