On Wed, Mar 04, 2015 at 12:02:03PM +0100, Pavel Reichl wrote:
From f2801324e2d184e1bf7092f22e8509ffc78a5bfc Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)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(a)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