From 58e0e132d291234f0bf6305e5410a68481fe50be Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Thu, 11 Dec 2014 10:49:39 +0100 Subject: [PATCH 1/2] IPA: verify group memberships of trusted domain users Depending on the state of the cache group object a freshly created or updates user entry for a trusted domain user might already be a member of the group or not. This cache makes sure the requested user is a member of all groups returned from the extdom request. Special care has to be taken to cover cross-domain group-memberships properly. Resolves https://fedorahosted.org/sssd/ticket/2529 --- src/providers/ipa/ipa_s2n_exop.c | 145 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 144 insertions(+), 1 deletion(-) diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c index 6fc9b8502b8704634ac629abc0ee925998e5b227..292056af11ec23f52402859221964d35b9bbd4ad 100644 --- a/src/providers/ipa/ipa_s2n_exop.c +++ b/src/providers/ipa/ipa_s2n_exop.c @@ -568,7 +568,7 @@ static errno_t add_v1_user_data(BerElement *ber, struct resp_attrs *attrs) attrs->ngroups++); if (attrs->ngroups > 0) { - attrs->groups = talloc_array(attrs, char *, attrs->ngroups); + attrs->groups = talloc_zero_array(attrs, char *, attrs->ngroups + 1); if (attrs->groups == NULL) { DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n"); ret = ENOMEM; @@ -1645,6 +1645,81 @@ done: return; } +static errno_t get_groups_dns(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom, + char **name_list, char ***_dn_list) +{ + int ret; + TALLOC_CTX *tmp_ctx; + int c; + struct sss_domain_info *root_domain; + char **dn_list; + + if (name_list == NULL) { + *_dn_list = NULL; + return EOK; + } + + /* To handle cross-domain memberships we have to check the domain for + * each group the member should be added or deleted. Since sub-domains + * use fully-qualified names by default any short name can only belong + * to the root/head domain. find_domain_by_object_name() will return + * the domain given in the first argument if the second argument is a + * a short name hence we always use root_domain as first argument. */ + root_domain = get_domains_head(dom); + if (root_domain->fqnames) { + DEBUG(SSSDBG_TRACE_FUNC, + "Root domain uses fully-qualified names, " \ + "objects might not be correctly added to groups with " \ + "short names.\n"); + } + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n"); + return ENOMEM; + } + + for (c = 0; name_list[c] != NULL; c++); + + dn_list = talloc_zero_array(tmp_ctx, char *, c + 1); + if (dn_list == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_zero_array failed.\n"); + ret = ENOMEM; + goto done; + } + + for (c = 0; name_list[c] != NULL; c++) { + dom = find_domain_by_object_name(root_domain, name_list[c]); + if (dom == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Cannot find domain for [%s].\n", name_list[c]); + ret = ENOENT; + goto done; + } + + /* This might fail if some unexpected cases are used. But current + * sysdb code which handles group membership constructs DNs this way + * as well, IPA names are lowercased and AD names by default will be + * lowercased as well. If there are really use-cases which cause an + * issue here, sysdb_group_strdn() has to be replaced by a proper + * search. */ + dn_list[c] = sysdb_group_strdn(dn_list, dom->name, name_list[c]); + if (dn_list[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "sysdb_group_strdn failed.\n"); + ret = ENOMEM; + goto done; + } + } + + *_dn_list = talloc_steal(mem_ctx, dn_list); + ret = EOK; + +done: + talloc_free(tmp_ctx); + + return ret; +} + static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom, struct req_input *req_input, struct resp_attrs *attrs, @@ -1666,6 +1741,13 @@ static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom, const char *tmp_str; struct ldb_result *res; enum sysdb_member_type type; + char **sysdb_grouplist; + char **add_groups; + char **add_groups_dns; + char **del_groups; + char **del_groups_dns; + bool in_transaction = false; + int tret; tmp_ctx = talloc_new(NULL); if (tmp_ctx == NULL) { @@ -1873,6 +1955,13 @@ static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom, } } + ret = sysdb_transaction_start(dom->sysdb); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, "Failed to start transaction\n"); + goto done; + } + in_transaction = true; + ret = sysdb_store_user(dom, name, NULL, attrs->a.user.pw_uid, gid, attrs->a.user.pw_gecos, @@ -1883,6 +1972,53 @@ static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom, DEBUG(SSSDBG_OP_FAILURE, "sysdb_store_user failed.\n"); goto done; } + + if (attrs->response_type == RESP_USER_GROUPLIST) { + ret = get_sysdb_grouplist(tmp_ctx, dom->sysdb, dom, name, + &sysdb_grouplist); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "get_sysdb_grouplist failed.\n"); + goto done; + } + + ret = diff_string_lists(tmp_ctx, attrs->groups, sysdb_grouplist, + &add_groups, &del_groups, NULL); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "diff_string_lists failed.\n"); + goto done; + } + + ret = get_groups_dns(tmp_ctx, dom, add_groups, &add_groups_dns); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "get_groups_dns failed.\n"); + goto done; + } + + ret = get_groups_dns(tmp_ctx, dom, del_groups, &del_groups_dns); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "get_groups_dns failed.\n"); + goto done; + } + + DEBUG(SSSDBG_TRACE_INTERNAL, "Updating memberships for %s\n", + name); + ret = sysdb_update_members_dn(dom, name, SYSDB_MEMBER_USER, + (const char *const *) add_groups_dns, + (const char *const *) del_groups_dns); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, "Membership update failed [%d]: %s\n", + ret, sss_strerror(ret)); + goto done; + } + } + + ret = sysdb_transaction_commit(dom->sysdb); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, "Failed to commit transaction\n"); + goto done; + } + in_transaction = false; + break; case RESP_GROUP: case RESP_GROUP_MEMBERS: @@ -1975,6 +2111,13 @@ static errno_t ipa_s2n_save_objects(struct sss_domain_info *dom, } done: + if (in_transaction) { + tret = sysdb_transaction_cancel(dom->sysdb); + if (tret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, "Failed to cancel transaction\n"); + } + } + talloc_free(tmp_ctx); return ret; -- 2.1.0