[SSSD] [PATCHES] SDAP: Lock out ssh keys when account naturally expires
Pavel Reichl
preichl at redhat.com
Thu Mar 5 13:15:15 UTC 2015
On 03/04/2015 10:38 PM, Jakub Hrozek wrote:
> 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.
Thanks, for yours. Patch with duplication was no good.
>
> 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.
hopefully fixed now:
ci:
http://sssd-ci.duckdns.org/logs/job/8/86/summary.html
>
>> From a8826633a944e92b7248e446fd37c349c1c72292 Mon Sep 17 00:00:00 2001
>> From: Pavel Reichl <preichl at 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 <preichl at 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.
done
>
>> @@ -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.
done
>
> 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.
nice but later
>
>> -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.
done, but difficult to parse - please check
>
> Thanks again for the patience. I know there's been several respins, but
> this is access control code and should be reviewed really carefully.
OK, I agree, I just hate to do this things in time press, but I know
it's my mistake.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-UTIL-convert-GeneralizedTime-to-unix-time.patch
Type: text/x-patch
Size: 7202 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/5263ec65/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SDAP-Lock-out-ssh-keys-when-account-naturally-expire.patch
Type: text/x-patch
Size: 28999 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/5263ec65/attachment-0001.bin>
More information about the sssd-devel
mailing list