On Wed, Mar 04, 2015 at 09:45:19PM +0100, Pavel Reichl wrote:
>
>
> On 03/04/2015 04:37 PM, Jakub Hrozek wrote:
> >On Wed, Mar 04, 2015 at 12:02:03PM +0100, Pavel Reichl wrote:
> >>Patches needed to be rebased.
> >here are the fixups I mentioned in the other mail
> Thanks, new patch set attached.
Thank you for the patience.
Almost ack :-) The code now reads good to me, thanks for removing the
duplication. I couldn't figure out what's up with the tests, my brain is
too tired, we might need to ask nick what could be the issue.
> From a8826633a944e92b7248e446fd37c349c1c72292 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <prei...@redhat.com>
> Date: Tue, 20 Jan 2015 16:27:41 -0500
> Subject: [PATCH 1/2] UTIL: convert GeneralizedTime to unix time
ack sans the test failure in CI. We need to solve it before pushing, but
the patch looks good to me.
> From 4b3a7b2e196c6c19639b30e6be3fe7cb57ec7b7b Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <prei...@redhat.com>
> Date: Tue, 20 Jan 2015 18:34:44 -0500
> Subject: [PATCH 2/2] SDAP: Lock out ssh keys when account naturally expires
>
Mostly good, I only have one place where we might not have
understood each other on IRC and one request.
If you are going to respin the patch once again, can you chance
strerror() for sss_strerror()? It's not a big deal since we'd see the
error value either way, it would just make the error output nicer since
we're using custom codes.
> @@ -1462,28 +1493,30 @@ static void sdap_access_lock_connect_done(struct
> tevent_req *subreq)
> /* Connection to LDAP succeeded
> * Send 'pwdLockout' request
> */
> - ret = sdap_access_lock_get_lockout_step(req);
> + ret = sdap_access_ppolicy_get_lockout_step(req);
> if (ret != EOK && ret != EAGAIN) {
> DEBUG(SSSDBG_CRIT_FAILURE,
> - "sdap_access_lock_get_lockout_step failed: [%d][%s]\n",
> + "sdap_access_ppolicy_get_lockout_step failed: [%d][%s]\n",
> ret, strerror(ret));
> - tevent_req_error(req, ERR_ACCESS_DENIED);
> + tevent_req_error(req, ERR_INTERNAL);
> return;
> }
Here is the place where we might have not understand one another. The
code continues with either EOK or EAGAIN. I think it's correct to
continue with EAGAIN, but do you see issues with marking the request as
done if the function returns EOK? I think EOK should actually never
happen here, but we should handle it nonetheless, because the way I read
the code, the request would never finish otherwise.
btw -- this might be a good place to start using custom return codes
also in future patches. The code that iterates over search bases and
returns EAGAIN on search or EOK when it's done is quite common and I
wonder if using ERR_SEARCH_IN_PROGRESS and ERR_NO_MORE_SEARCH_BASES
would make the callers more readable. Just a thought.
> -static void sdap_access_lock_step_done(struct tevent_req *subreq)
> +static errno_t
> +is_account_locked(const char *pwdAccountLockedTime,
> + const char *pwdAccountLockedDurationTime,
> + enum sdap_pwpolicy_mode pwpol_mode,
> + const char *username,
> + bool *_locked)
> +{
> + errno_t ret;
> + time_t lock_time;
> + time_t duration;
> + time_t now;
> + bool locked;
> +
> + /* Default action is to consider account to be locked. */
> + locked = true;
> +
> + /* account is permanently locked */
> + if (strcasecmp(pwdAccountLockedTime,
> + PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> + ret = EOK;
> + goto done;
> + }
> +
> + if (pwpol_mode == PWP_LOCKOUT_ONLY) {
> + /* We do *not* care about exact value of account locked time, we
> + * only *do* care if the value is equal to
> + * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
> + * permanently.
> + */
> + DEBUG(SSSDBG_TRACE_FUNC,
> + "Account of: %s is beeing blocked by password policy, "
> + "but value: [%s] value is ignored by SSSD.\n",
> + username, pwdAccountLockedTime);
> + locked = false;
> + } else {
> + /* Account may be locked out from natural reasons (too many attempts,
> + * expired password). In this case, pwdAccountLockedTime is also set,
> + * to the time of lock out.
> + */
> + ret = sss_utc_to_time_t(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
> + &lock_time);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_TRACE_FUNC, "sss_utc_to_time_t failed with
> %d:%s.\n",
> + ret, sss_strerror(ret));
> + goto done;
> + }
> +
> + now = time(NULL);
> +
> + /* Account was NOT locked in past. */
> + if (difftime(lock_time, now) > 0.0) {
> + locked = false;
> + } else if (pwdAccountLockedDurationTime != NULL) {
> + errno = 0;
> + duration = strtouint32(pwdAccountLockedDurationTime, NULL, 0);
> + if (errno) {
> + ret = errno;
> + goto done;
> + }
> + /* Lockout has expired */
> + if (duration != 0 && difftime(now, lock_time) > duration) {
> + locked = false;
> + }
> + }
> + }
I would prefer a switch-case here, for one reason - we would need to
have all values of the enum and we would either get a compilation
warning if all values are not handled or an error if we use a default
handled. I prefer the first option, but IIRC Sumit prefers the second
and I'm not too sold on either of them.
Thanks again for the patience. I know there's been several respins, but
this is access control code and should be reviewed really carefully.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
