URL: https://github.com/SSSD/sssd/pull/234 Author: jhrozek Title: #234: HBAC: Use memberof ASQ search instead of originalMemberOf Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/234/head:pr234 git checkout pr234
From 429c4ad65a711235049ee23d306b2b69372f696d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> 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 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. --- src/providers/ipa/ipa_hbac_common.c | 100 ++++++++++++++++++++++++++---------- 1 file changed, 72 insertions(+), 28 deletions(-) diff --git a/src/providers/ipa/ipa_hbac_common.c b/src/providers/ipa/ipa_hbac_common.c index b99b75d..3efe268 100644 --- a/src/providers/ipa/ipa_hbac_common.c +++ b/src/providers/ipa/ipa_hbac_common.c @@ -507,15 +507,18 @@ 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 *attrs[] = { SYSDB_NAME, NULL }; char *shortname; + struct ldb_message **members; + size_t m_count; + const char *fqgroupname; + struct sss_domain_info *ipa_domain; + struct ldb_dn *ipa_groups_basedn; + struct ldb_dn *member_group_container; tmp_ctx = talloc_new(mem_ctx); if (tmp_ctx == NULL) return ENOMEM; @@ -533,12 +536,21 @@ hbac_eval_user_element(TALLOC_CTX *mem_ctx, } users->name = talloc_steal(users, shortname); - /* 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; + } + ret = sysdb_search_user_by_name(tmp_ctx, domain, username, - attrs, &msg); + NULL, &msg); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Could not determine user memberships for [%s]\n", @@ -546,43 +558,75 @@ hbac_eval_user_element(TALLOC_CTX *mem_ctx, goto done; } - el = ldb_msg_find_element(msg, SYSDB_ORIG_MEMBEROF); - if (el == NULL || el->num_values == 0) { + /* + * Get the name attribute of all groups pointed to by the memberof + * attribute. This includes both POSIX and non-POSIX groups. + */ + ret = sysdb_asq_search(tmp_ctx, domain, msg->dn, + "("SYSDB_OBJECTCLASS"="SYSDB_GROUP_CLASS")", + SYSDB_MEMBEROF, + attrs, + &m_count, &members); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "sysdb_asq_search failed [%d]: %s\n", ret, sss_strerror(ret)); + goto done; + } + + if (m_count == 0) { 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); + DEBUG(SSSDBG_TRACE_LIBS, "[%zu] groups for [%s]\n", m_count, username); - users->groups = talloc_array(users, const char *, el->num_values + 1); + users->groups = talloc_array(users, const char *, m_count + 1); if (users->groups == NULL) { ret = ENOMEM; goto done; } - for (i = 0; i < el->num_values; i++) { - member_dn = (const char *)el->values[i].data; + for (size_t i = 0; i < m_count; i++) { + fqgroupname = ldb_msg_find_attr_as_string(members[i], SYSDB_NAME, NULL); + if (fqgroupname == NULL) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Skipping malformed entry [%s]\n", + ldb_dn_get_linearized(members[i]->dn)); + continue; + } + + member_group_container = ldb_dn_copy(tmp_ctx, members[i]->dn); + if (member_group_container == NULL) { + DEBUG(SSSDBG_MINOR_FAILURE, "ldb_dn_copy failed, skipping!\n"); + continue; + } - ret = get_ipa_groupname(users->groups, domain->sysdb, member_dn, - &users->groups[num_groups]); - if (ret != EOK && ret != ERR_UNEXPECTED_ENTRY_TYPE) { + if (!ldb_dn_remove_child_components(member_group_container, 1)) { DEBUG(SSSDBG_MINOR_FAILURE, - "Skipping malformed entry [%s]\n", member_dn); + "ldb_dn_remove_child_components failed, skipping!\n"); continue; - } else if (ret == EOK) { - DEBUG(SSSDBG_TRACE_LIBS, "Added group [%s] for user [%s]\n", - users->groups[num_groups], users->name); - num_groups++; + } + + if (ldb_dn_compare(ipa_groups_basedn, member_group_container) != 0) { + DEBUG(SSSDBG_FUNC_DATA, "Skipping non-IPA group %s\n", fqgroupname); continue; } - /* Skip entries that are not groups */ - DEBUG(SSSDBG_TRACE_INTERNAL, - "Skipping non-group memberOf [%s]\n", member_dn); + + ret = sss_parse_internal_fqname(tmp_ctx, fqgroupname, + &shortname, NULL); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, "Malformed name %s, skipping!\n", fqgroupname); + continue; + } + + users->groups[num_groups] = talloc_steal(users->groups, shortname); + 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 < m_count) { /* Shrink the array memory */ users->groups = talloc_realloc(users, users->groups, const char *, num_groups+1);
_______________________________________________ sssd-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
