[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