[SSSD] [PATCHES] SDAP: Lock out ssh keys when account naturally expires

Jakub Hrozek jhrozek at redhat.com
Wed Mar 4 15:30:51 UTC 2015


On Wed, Mar 04, 2015 at 12:02:03PM +0100, Pavel Reichl wrote:
> Patches needed to be rebased.
> 
> ci passed:
> 
> http://sssd-ci.duckdns.org/logs/job/8/74/summary.html
> 
> 
> 

> From f2801324e2d184e1bf7092f22e8509ffc78a5bfc 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
> 
> New utility function *sss_utc_to_time_t* to convert GeneralizedTime to
> unix time.

Almost ack, see the attached patch for suggestions. It's mostly renaming
the new error code (sorry, the name was my fault, I hope sending the
correcting patch makes that up)

I also moved changing the TZ environment variable into the test function
to avoid side-effects.

Can you also (in another patch) use sss_utc_to_time_t on other
places in the code where pretty much the same code is used? I
guess check_pwexpire_kerberos(), nds_check_expired() and maybe even
sysdb_sudo_convert_time() but I'm not sure about the last one.


> From 5be740031c33ea76aa2c79fd859591ab347ab521 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
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2534

Here I also have a small fix for the Makefile and man page attached. But
more importantly, we shouldn't copy the whole lockout request, but
rename it to ppolicy and add an enum:

enum spap_pwpolicy_mode {
    PWP_LOCKOUT_ONLY,
    PWP_LOCKOUT_EXPIRE,
    PWP_SENTINEL,
};

to the _send function. If the value would be >= than SENTINEL, error
out. Then just modify the is_account_locked() function. 

The request is too big to be duplicated, sorry. We also duplicated some
bugs, see inline.
    
> +static struct tevent_req *
> +sdap_access_ppolicy_send(TALLOC_CTX *mem_ctx,
> +                      struct tevent_context *ev,
> +                      struct be_ctx *be_ctx,
> +                      struct sss_domain_info *domain,
> +                      struct sdap_access_ctx *access_ctx,
> +                      struct sdap_id_conn_ctx *conn,
> +                      const char *username,
> +                      struct ldb_message *user_entry)
> +{
> +    struct sdap_access_ppolicy_req_ctx *state;
> +    struct tevent_req *req;
> +    errno_t ret;
> +
> +    req = tevent_req_create(mem_ctx,
> +                            &state, struct sdap_access_ppolicy_req_ctx);
> +    if (req == NULL) {
> +        return NULL;
> +    }
> +
> +    state->filter = NULL;
> +    state->username = username;
> +    state->opts = access_ctx->id_ctx->opts;
> +    state->conn = conn;
> +    state->ev = ev;
> +    state->access_ctx = access_ctx;
> +    state->domain = domain;
> +    state->ppolicy_dns_index = 0;
> +
> +    DEBUG(SSSDBG_TRACE_FUNC,
> +          "Performing access ppolicy check for user [%s]\n", username);
> +
> +    state->cached_access = ldb_msg_find_attr_as_bool(
> +        user_entry, SYSDB_LDAP_ACCESS_CACHED_LOCKOUT, false);
> +
> +    /* Ok, we have one result, check if we are online or offline */
> +    if (be_is_offline(be_ctx)) {
> +        /* Ok, we're offline. Return from the cache */
> +        ret = sdap_access_decide_offline(state->cached_access);
> +        goto done;
> +    }
> +
> +    ret = sdap_get_basedn_user_entry(user_entry, state->username,
> +                                     &state->basedn);
> +    if (ret != EOK) {
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "Checking ppolicy against LDAP\n");
> +
> +    state->sdap_op = sdap_id_op_create(state,
> +                                       state->conn->conn_cache);
> +    if (!state->sdap_op) {
> +        DEBUG(SSSDBG_OP_FAILURE, "sdap_id_op_create failed\n");
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    ret = sdap_access_ppolicy_retry(req);
> +    if (ret != EOK) {
> +        goto done;
> +    }
> +
> +    return req;
> +
> +done:
> +    if (ret == EOK) {
> +        tevent_req_done(req);
> +    } else {
> +        tevent_req_error(req, ret);
> +    }
> +    tevent_req_post(req, ev);
> +    return req;
> +}
> +
> +static int sdap_access_ppolicy_retry(struct tevent_req *req)
> +{
> +    struct sdap_access_ppolicy_req_ctx *state;
> +    struct tevent_req *subreq;
> +    int ret;
> +
> +    state = tevent_req_data(req, struct sdap_access_ppolicy_req_ctx);
> +    subreq = sdap_id_op_connect_send(state->sdap_op, state, &ret);
> +    if (!subreq) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sdap_id_op_connect_send failed: %d (%s)\n", ret, strerror(ret));
> +        return ret;
> +    }
> +
> +    tevent_req_set_callback(subreq, sdap_access_ppolicy_connect_done, req);
> +    return EOK;
> +}
> +
> +static void sdap_access_ppolicy_connect_done(struct tevent_req *subreq)
> +{
> +    struct tevent_req *req;
> +    struct sdap_access_ppolicy_req_ctx *state;
> +    int ret, dp_error;
> +    const char *ppolicy_dn;
> +
> +    req = tevent_req_callback_data(subreq, struct tevent_req);
> +    state = tevent_req_data(req, struct sdap_access_ppolicy_req_ctx);
> +
> +    ret = sdap_id_op_connect_recv(subreq, &dp_error);
> +    talloc_zfree(subreq);
> +
> +    if (ret != EOK) {
> +        if (dp_error == DP_ERR_OFFLINE) {
> +            ret = sdap_access_decide_offline(state->cached_access);
> +            if (ret == EOK) {
> +                tevent_req_done(req);
> +                return;
> +            }
> +        }
> +
> +        tevent_req_error(req, ret);
> +        return;
> +    }
> +
> +    ppolicy_dn = dp_opt_get_string(state->opts->basic,
> +                                   SDAP_PWDLOCKOUT_DN);
> +
> +    /* option was configured */
> +    if (ppolicy_dn != NULL) {
> +        state->ppolicy_dns = talloc_array(state, const char*, 2);
> +        if (state->ppolicy_dns == NULL) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate ppolicy_dns.\n");
> +            tevent_req_error(req, ERR_ACCESS_DENIED);

Why access denied and not internal error? Wouldn't the user be tricked
into thinking everything worked fine but he was denied access?

> +            return;
> +        }
> +
> +        state->ppolicy_dns[0] = ppolicy_dn;
> +        state->ppolicy_dns[1] = NULL;
> +
> +    } else {
> +        /* try to determine default value */
> +        DEBUG(SSSDBG_CONF_SETTINGS,
> +              "ldap_pwdlockout_dn was not defined in configuration file.\n");
> +
> +        state->ppolicy_dns = get_default_ppolicy_dns(state, state->opts->sdom);
> +        if (state->ppolicy_dns == NULL) {
> +            tevent_req_error(req, ERR_ACCESS_DENIED);

Same comment about access denied.

> +            return;
> +        }
> +    }
> +
> +    /* Connection to LDAP succeeded
> +     * Send 'pwdLockout' request
> +     */
> +    ret = sdap_access_ppolicy_get_lockout_step(req);
> +    if (ret != EOK && ret != EAGAIN) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              "sdap_access_ppolicy_get_lockout_step failed: [%d][%s]\n",
> +              ret, strerror(ret));
> +        tevent_req_error(req, ERR_ACCESS_DENIED);

Same comment about access denied.

> +        return;
> +    }

You don't handle EOK here, so the request would never finish.

> +}
> +
> +static errno_t
> +sdap_access_ppolicy_get_lockout_step(struct tevent_req *req)
> +{
> +    const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKOUT, NULL };
> +    struct sdap_access_ppolicy_req_ctx *state;
> +    struct tevent_req *subreq;
> +    errno_t ret;
> +
> +    state = tevent_req_data(req, struct sdap_access_ppolicy_req_ctx);
> +
> +    /* no more DNs to try */
> +    if (state->ppolicy_dns[state->ppolicy_dns_index] == NULL) {

Please add a debug message here.

> +        ret = EOK;
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_CONF_SETTINGS,
> +          "Trying to find out if ppolicy is enabled using the DN: %s\n",
> +          state->ppolicy_dns[state->ppolicy_dns_index]);
> +
> +    subreq = sdap_get_generic_send(state,
> +                                   state->ev,
> +                                   state->opts,
> +                                   sdap_id_op_handle(state->sdap_op),
> +                                   state->ppolicy_dns[state->ppolicy_dns_index],
> +                                   LDAP_SCOPE_BASE,
> +                                   NULL, attrs,
> +                                   NULL, 0,
> +                                   dp_opt_get_int(state->opts->basic,
> +                                                  SDAP_SEARCH_TIMEOUT),
> +                                   false);
> +    if (subreq == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not start LDAP communication\n");
> +        tevent_req_error(req, EIO);

The request is marked as faile by the caller in other cases, which would
result in a double-free.

> +        ret = EIO;
> +        goto done;
> +    }
> +
> +    /* try next basedn */
> +    state->ppolicy_dns_index++;
> +    tevent_req_set_callback(subreq, sdap_access_ppolicy_get_lockout_done, req);
> +
> +    ret = EAGAIN;
> +
> +done:
> +    return ret;
> +}
> +

[...]

> +static errno_t
> +is_account_locked(const char *pwdAccountLockedTime,
> +                  const char *pwdAccountLockedDurationTime,
> +                  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;
> +    }

Could we add code like:
    if (lockout_only) {
        DEBUG(SSSDBG_TRACE_FUNC,
              "Account of: %s is beeing blocked by password policy, "
              "but value: [%s] value is ignored by SSSD.\n",
              state->username, pwdAccountLockedTime);
        ret = EOK;
        locked = false;
        goto done;
    }


> +
> +    /* 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);

Strange indent



More information about the sssd-devel mailing list