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:

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
mandatory, if there is no value set, the check is passed.

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",
+    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
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

+                errMesg = "Kerberos principal has expired.";

The rest looks good to me.

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.


Freeipa-devel mailing list

Reply via email to