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