URL: https://github.com/SSSD/sssd/pull/309 Author: jhrozek Title: #309: HBAC: Do not rely on originalMemberOf, use the sysdb memberof links instead (sssd-1-13 backprot) Action: opened
PR body: """ This is a backport of the same fix we merged earlier. The IPA HBAC code used to read the group members from the originalMemberOf attribute value for performance reasons. However, especially on IPA clients trusting an AD domain, the originalMemberOf attribute value is often not synchronized correctly. Instead of going through the work of maintaining both member/memberOf and originalMemberOf, let's just do an ASQ search for the group names of the groups the user is a member of in the cache and read their SYSBD_NAME attribute. To avoid clashing between similarly-named groups in IPA and in AD, we look at the container of the group. Resolves: https://pagure.io/SSSD/sssd/issue/3382 (cherry picked from commit c92e49144978ad3b6c9fffa8803ebdad8f6f5b18) """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/309/head:pr309 git checkout pr309
From 1c20ae2bfc7436b9e124136e9b07f95d599c8659 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Sun, 9 Apr 2017 20:50:47 +0200 Subject: [PATCH] HBAC: Do not rely on originalMemberOf, use the sysdb memberof links instead The IPA HBAC code used to read the group members from the originalMemberOf attribute value for performance reasons. However, especially on IPA clients trusting an AD domain, the originalMemberOf attribute value is often not synchronized correctly. Instead of going through the work of maintaining both member/memberOf and originalMemberOf, let's just do an ASQ search for the group names of the groups the user is a member of in the cache and read their SYSBD_NAME attribute. To avoid clashing between similarly-named groups in IPA and in AD, we look at the container of the group. Resolves: https://pagure.io/SSSD/sssd/issue/3382 (cherry picked from commit c92e49144978ad3b6c9fffa8803ebdad8f6f5b18) --- src/providers/ipa/ipa_hbac_common.c | 94 +++++++++++++++++++++++++------------ 1 file changed, 64 insertions(+), 30 deletions(-) diff --git a/src/providers/ipa/ipa_hbac_common.c b/src/providers/ipa/ipa_hbac_common.c index 72a620ef0..4afe44ea1 100644 --- a/src/providers/ipa/ipa_hbac_common.c +++ b/src/providers/ipa/ipa_hbac_common.c @@ -510,14 +510,14 @@ hbac_eval_user_element(TALLOC_CTX *mem_ctx, struct hbac_request_element **user_element) { errno_t ret; - unsigned int i; unsigned int num_groups = 0; TALLOC_CTX *tmp_ctx; - const char *member_dn; struct hbac_request_element *users; - struct ldb_message *msg; - struct ldb_message_element *el; - const char *attrs[] = { SYSDB_ORIG_MEMBEROF, NULL }; + const char *groupname; + struct sss_domain_info *ipa_domain; + struct ldb_dn *ipa_groups_basedn; + struct ldb_result *res; + int exp_comp; tmp_ctx = talloc_new(mem_ctx); if (tmp_ctx == NULL) return ENOMEM; @@ -530,56 +530,90 @@ hbac_eval_user_element(TALLOC_CTX *mem_ctx, users->name = username; - /* Read the originalMemberOf attribute - * This will give us the list of both POSIX and - * non-POSIX groups that this user belongs to. + ipa_domain = get_domains_head(domain); + if (ipa_domain == NULL) { + ret = EINVAL; + goto done; + } + + ipa_groups_basedn = ldb_dn_new_fmt(tmp_ctx, sysdb_ctx_get_ldb(domain->sysdb), + SYSDB_TMPL_GROUP_BASE, ipa_domain->name); + if (ipa_groups_basedn == NULL) { + ret = ENOMEM; + goto done; + } + + /* +1 because there will be a RDN preceding the base DN */ + exp_comp = ldb_dn_get_comp_num(ipa_groups_basedn) + 1; + + /* + * Get all the groups the user is a member of. + * This includes both POSIX and non-POSIX groups. */ - ret = sysdb_search_user_by_name(tmp_ctx, domain, users->name, - attrs, &msg); + ret = sysdb_initgroups(tmp_ctx, domain, username, &res); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, - "Could not determine user memberships for [%s]\n", - users->name); + "sysdb_asq_search failed [%d]: %s\n", ret, sss_strerror(ret)); goto done; } - el = ldb_msg_find_element(msg, SYSDB_ORIG_MEMBEROF); - if (el == NULL || el->num_values == 0) { + if (res->count == 0) { + /* This should not happen at this point */ + DEBUG(SSSDBG_MINOR_FAILURE, + "User [%s] not found in cache.\n", username); + ret = ENOENT; + goto done; + } else if (res->count == 1) { + /* The first item is the user entry */ DEBUG(SSSDBG_TRACE_LIBS, "No groups for [%s]\n", users->name); ret = create_empty_grouplist(users); goto done; } DEBUG(SSSDBG_TRACE_LIBS, - "[%d] groups for [%s]\n", el->num_values, users->name); + "[%u] groups for [%s]\n", res->count - 1, username); - users->groups = talloc_array(users, const char *, el->num_values + 1); + /* This also includes the sentinel, b/c we'll skip the user entry below */ + users->groups = talloc_array(users, const char *, res->count); if (users->groups == NULL) { ret = ENOMEM; goto done; } - for (i = 0; i < el->num_values; i++) { - member_dn = (const char *)el->values[i].data; + /* Start counting from 1 to exclude the user entry */ + for (size_t i = 1; i < res->count; i++) { + /* Only groups from the IPA domain can be referenced from HBAC rules. To + * avoid evaluating groups which might even have the same name, but come + * from a trusted domain, we first copy the DN to a temporary one.. + */ + if (ldb_dn_get_comp_num(res->msgs[i]->dn) != exp_comp + || ldb_dn_compare_base(ipa_groups_basedn, + res->msgs[i]->dn) != 0) { + DEBUG(SSSDBG_FUNC_DATA, + "Skipping non-IPA group %s\n", + ldb_dn_get_linearized(res->msgs[i]->dn)); + continue; + } - ret = get_ipa_groupname(users->groups, domain->sysdb, member_dn, - &users->groups[num_groups]); - if (ret != EOK && ret != ERR_UNEXPECTED_ENTRY_TYPE) { + groupname = ldb_msg_find_attr_as_string(res->msgs[i], SYSDB_NAME, NULL); + if (groupname == NULL) { DEBUG(SSSDBG_MINOR_FAILURE, - "Skipping malformed entry [%s]\n", member_dn); + "Skipping malformed entry [%s]\n", + ldb_dn_get_linearized(res->msgs[i]->dn)); continue; - } else if (ret == EOK) { - DEBUG(SSSDBG_TRACE_LIBS, "Added group [%s] for user [%s]\n", - users->groups[num_groups], users->name); - num_groups++; + } + + users->groups[num_groups] = talloc_strdup(users->groups, groupname); + if (users->groups[num_groups] == NULL) { continue; } - /* Skip entries that are not groups */ - DEBUG(SSSDBG_TRACE_INTERNAL, - "Skipping non-group memberOf [%s]\n", member_dn); + + DEBUG(SSSDBG_TRACE_LIBS, "Added group [%s] for user [%s]\n", + users->groups[num_groups], users->name); + num_groups++; } users->groups[num_groups] = NULL; - if (num_groups < el->num_values) { + if (num_groups < (res->count - 1)) { /* Shrink the array memory */ users->groups = talloc_realloc(users, users->groups, const char *, num_groups+1);
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
