From ef6dc31d457c672272af05307b3a9a19119f5d77 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 21 May 2018 22:35:48 +0200 Subject: [PATCH] DP/LDAP: Only increase the initgrTimestamp when the full initgroups DP request finishes An initgroups request for an AD user consists of two parts - resolving the AD user, which internally calls an LDAP request and adding the IPA external group memberships. For (probably?) historical reasons from the time before we had any notion of subdomains, the initgrTimestamp attribute is written down at the LDAP request level when it finishes -- which means the initgrTimestamp is written before the IPA external group membership is evaluated. When two requests for initgroups arrive semi-concurrently, it can happen that the first request will trigger the whole machinery while the other one would evaluate the initgrTimestamp attribute that was just bumped, but the IPA group memberships were not yet written to the cache. The result is that the second racing request only returns AD groups. This fix removes writing the timestamp from the generic LDAP code and instead writes the timestamp only when the Data Provider request fully returns. Resolves: https://pagure.io/SSSD/sssd/issue/3744 --- src/providers/data_provider/dp_target_id.c | 67 +++++++++++++++++++++++++++ src/providers/ldap/ldap_id.c | 72 +++++++----------------------- 2 files changed, 83 insertions(+), 56 deletions(-) diff --git a/src/providers/data_provider/dp_target_id.c b/src/providers/data_provider/dp_target_id.c index 1142d5b4e..d123638eb 100644 --- a/src/providers/data_provider/dp_target_id.c +++ b/src/providers/data_provider/dp_target_id.c @@ -372,12 +372,79 @@ static void dp_req_initgr_pp_sr_overlay(struct data_provider *provider, talloc_free(tmp_ctx); } +static errno_t set_initgroups_expire_attribute(struct sss_domain_info *domain, + const char *name) +{ + errno_t ret; + time_t cache_timeout; + struct sysdb_attrs *attrs; + + attrs = sysdb_new_attrs(NULL); + if (attrs == NULL) { + return ENOMEM; + } + + cache_timeout = domain->user_timeout + ? time(NULL) + domain->user_timeout + : 0; + + ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE, cache_timeout); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, "Could not set up attrs\n"); + goto done; + } + + ret = sysdb_set_user_attr(domain, name, attrs, SYSDB_MOD_REP); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to set initgroups expire attribute\n"); + goto done; + } + +done: + talloc_zfree(attrs); + return ret; +} + +static void dp_req_initgr_pp_set_initgr_timestamp(struct dp_initgr_ctx *ctx, + struct dp_reply_std *reply) +{ + errno_t ret; + const char *cname; + + if (reply->dp_error != DP_ERR_OK || reply->error != EOK) { + /* Only bump the timestamp on successful lookups */ + return; + } + + ret = sysdb_get_real_name(ctx, + ctx->domain_info, + ctx->filter_value, + &cname); + if (ret == ENOENT) { + /* No point trying to bump timestamp of an entry that does not exist..*/ + return; + } else if (ret != EOK) { + cname = ctx->filter_value; + DEBUG(SSSDBG_MINOR_FAILURE, + "Failed to canonicalize name, using [%s]\n", cname); + } + + ret = set_initgroups_expire_attribute(ctx->domain_info, cname); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Cannot set the initgroups expire attribute [%d]: %s\n", + ret, sss_strerror(ret)); + } +} + static void dp_req_initgr_pp(const char *req_name, struct data_provider *provider, struct dp_initgr_ctx *ctx, struct dp_reply_std *reply) { (void)reply; + dp_req_initgr_pp_set_initgr_timestamp(ctx, reply); dp_req_initgr_pp_nss_notify(req_name, provider, ctx); dp_req_initgr_pp_sr_overlay(provider, ctx); } diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c index 3bd68780a..000eff93a 100644 --- a/src/providers/ldap/ldap_id.c +++ b/src/providers/ldap/ldap_id.c @@ -1245,40 +1245,6 @@ static int groups_by_user_retry(struct tevent_req *req); static void groups_by_user_connect_done(struct tevent_req *subreq); static void groups_by_user_done(struct tevent_req *subreq); -static errno_t set_initgroups_expire_attribute(struct sss_domain_info *domain, - const char *name) -{ - errno_t ret; - time_t cache_timeout; - struct sysdb_attrs *attrs; - - attrs = sysdb_new_attrs(NULL); - if (attrs == NULL) { - return ENOMEM; - } - - cache_timeout = domain->user_timeout - ? time(NULL) + domain->user_timeout - : 0; - - ret = sysdb_attrs_add_time_t(attrs, SYSDB_INITGR_EXPIRE, cache_timeout); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, "Could not set up attrs\n"); - goto done; - } - - ret = sysdb_set_user_attr(domain, name, attrs, SYSDB_MOD_REP); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Failed to set initgroups expire attribute\n"); - goto done; - } - -done: - talloc_zfree(attrs); - return ret; -} - static struct tevent_req *groups_by_user_send(TALLOC_CTX *memctx, struct tevent_context *ev, struct sdap_id_ctx *ctx, @@ -1396,7 +1362,6 @@ static void groups_by_user_done(struct tevent_req *subreq) struct groups_by_user_state); int dp_error = DP_ERR_FATAL; int ret; - const char *cname; ret = sdap_get_initgr_recv(subreq); talloc_zfree(subreq); @@ -1414,24 +1379,25 @@ static void groups_by_user_done(struct tevent_req *subreq) } state->sdap_ret = ret; - if (ret == EOK || ret == ENOENT) { - /* state->filter_value is still the name used for the original req. The cached - * object might have a different name, e.g. a fully-qualified name. */ - ret = sysdb_get_real_name(state, - state->domain, - state->filter_value, - &cname); - if (ret != EOK) { - cname = state->filter_value; - DEBUG(SSSDBG_TRACE_INTERNAL, - "Failed to canonicalize name, using [%s] [%d]: %s.\n", - cname, ret, sss_strerror(ret)); - } - } - switch (state->sdap_ret) { case ENOENT: if (state->noexist_delete == true) { + const char *cname; + + /* state->filter_value is still the name used for the original + * req. The cached object might have a different name, e.g. a + * fully-qualified name. */ + ret = sysdb_get_real_name(state, + state->domain, + state->filter_value, + &cname); + if (ret != EOK) { + cname = state->filter_value; + DEBUG(SSSDBG_TRACE_INTERNAL, + "Failed to canonicalize name, using [%s] [%d]: %s.\n", + cname, ret, sss_strerror(ret)); + } + ret = sysdb_delete_user(state->domain, cname, 0); if (ret != EOK && ret != ENOENT) { tevent_req_error(req, ret); @@ -1440,12 +1406,6 @@ static void groups_by_user_done(struct tevent_req *subreq) } break; case EOK: - ret = set_initgroups_expire_attribute(state->domain, cname); - if (ret != EOK) { - state->dp_error = DP_ERR_FATAL; - tevent_req_error(req, ret); - return; - } break; default: state->dp_error = dp_error;