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.


--
/ Alexander Bokovoy

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to