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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
