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

[Simo Sorce]

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 ?

> +            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.
  
-- 
Simo Sorce * Red Hat, Inc * New York