[Freeipa-devel] [PATCH 0034] Deny LDAP binds for user accounts with expired principal

[Simo Sorce]

this need rebasing due to OTP patches, however comments inline.

On Tue, 2014-01-07 at 13:47 +0100, Tomas Babej wrote:
> diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
> b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
> index
> ef37b5e173359f9404b241c12d8a6dc6e77da128..df74a671c219f9b4aaa407f2ebd68f41669817b4 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);

Why not just use a simpler condition from the start of this very nested
sequence of checks ?
Like:

    /* check the krbPrincipalExpiration attribute is present */
    principal_expire = slapi_entry_attr_get_charptr(entry, "krbPrincipalExpiration")
    if (principal_expire) {
        memset(...

I do not see the value of digging through the attr with that many calls.

> +            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.";

This message is sent back to the client, perhaps we should say: "Account
is expired", it seem to me a more understandable error for a casual
user/admin.

> +                    auth_failed = true;

Technically no auth was performed also I am not sure why we need a
boolean ?
Just set rc = LDAP_UNWILLING_TO_PERFORM ?


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

and here:
    if (rc != LDAP_SUCCESS) {
        slapi_send_ldap_result(pb, rc, NULL, errMesg, 0, NULL);

> +                               0, NULL);
> +        return -1;
> +    }
> +
>      return 0;
>  } 

HTH,
Simo.

-- 
Simo Sorce * Red Hat, Inc * New York