[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