From 7a08d1dea8cb9148ba1afe13f4d4567229c9b381 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Jul 05 2019 10:34:07 +0000 Subject: DP/SYSDB: Move the code to set initgrExpireTimestamp to a reusable function Related: https://pagure.io/SSSD/sssd/issue/4012 Because the initgroups request can, especially in the case of IPA provider with trusts, contain several sub-requests that run some provider-specific initgroups internally and then run post-processing AND because at the same time concurrent requests in the responder need to be sure that the initgrExpireTimestamp is only increased when the initgroups request is really done, we only set the initgrExpireTimestamp in the DP when the request finishes. This means, the background refresh task needs to also set the initgrExpireTimestamp attribute on its own as well. This patch so far splits the helper function into a reusable one so it can later be used by the background refresh. For examples of the bugs caused by the initgrTimestamp being set before the whole multi-step operation finishes, please see tickets #3744 or #2634. Reviewed-by: Sumit Bose --- diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 28801e0..56fd770 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -1113,6 +1113,17 @@ errno_t sysdb_store_override(struct sss_domain_info *domain, enum sysdb_member_type type, struct sysdb_attrs *attrs, struct ldb_dn *obj_dn); +/* + * Cache the time of last initgroups invocation. Typically this is not done when + * the provider-specific request itself finishes, because currently the request + * might hand over to other requests from a different provider (e.g. an AD user + * from a trusted domain might need to also call an IPA request to fetch the + * external groups). Instead, the caller of the initgroups request, typically + * the DP or the periodical refresh task sets the timestamp. + */ +errno_t sysdb_set_initgr_expire_timestamp(struct sss_domain_info *domain, + const char *name_or_upn_or_sid); + /* Password caching function. * If you are in a transaction ignore sysdb and pass in the handle. * If you are not in a transaction pass NULL in handle and provide sysdb, diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 55ba621..c57a13b 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -3277,6 +3277,76 @@ int sysdb_cache_password(struct sss_domain_info *domain, SSS_AUTHTOK_TYPE_PASSWORD, 0); } +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; +} + +errno_t sysdb_set_initgr_expire_timestamp(struct sss_domain_info *domain, + const char *name_or_upn_or_sid) +{ + const char *cname; + errno_t ret; + TALLOC_CTX *tmp_ctx; + + tmp_ctx = talloc_new(NULL); + if (!tmp_ctx) { + return ENOMEM; + } + + ret = sysdb_get_real_name(tmp_ctx, domain, name_or_upn_or_sid, &cname); + if (ret == ENOENT) { + /* No point trying to bump timestamp of an entry that does not exist..*/ + ret = EOK; + goto done; + } else if (ret != EOK) { + cname = name_or_upn_or_sid; + DEBUG(SSSDBG_MINOR_FAILURE, + "Failed to canonicalize name, using [%s]\n", name_or_upn_or_sid); + } + + ret = set_initgroups_expire_attribute(domain, cname); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Cannot set the initgroups expire attribute [%d]: %s\n", + ret, sss_strerror(ret)); + } + + ret = EOK; +done: + talloc_free(tmp_ctx); + return ret; +} + /* =Custom Search================== */ int sysdb_search_custom(TALLOC_CTX *mem_ctx, diff --git a/src/providers/data_provider/dp_target_id.c b/src/providers/data_provider/dp_target_id.c index 748d886..d5b3823 100644 --- a/src/providers/data_provider/dp_target_id.c +++ b/src/providers/data_provider/dp_target_id.c @@ -390,69 +390,22 @@ done: return ret; } -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); + ret = sysdb_set_initgr_expire_timestamp(ctx->domain_info, + ctx->filter_value); if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, - "Cannot set the initgroups expire attribute [%d]: %s\n", - ret, sss_strerror(ret)); + "Failed to set initgroups expiration for [%s]\n", + ctx->filter_value); } }