URL: https://github.com/SSSD/sssd/pull/234 Author: jhrozek Title: #234: HBAC: Use memberof ASQ search instead of originalMemberOf Action: opened
PR body: """ This PR should fix the bug we were seeing in the HBAC evaluation of users from a trusted AD domain where the originalMemberOf didn't match the memberOf attributes. Because maintaining the originalMemberOf attributes is fragile, let's instead dereference the memberOf attribute and look at the names of the groups this way. There is one unresolved issue in the patch - how to filter the groups from a single domain. The most error-prone method would be to just do a search by name with a domain set, but that would mean N searches for N groups. Alternatively, if other developers don't think that is too much of a hack, we could just construct a base DN of the IPA domain sysdb group container and pop the RDN from the DN of the object examined and compare the two. That would be reasonably fast. """ 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 6abeb465f78348b27fe62e11da88b0b6aa2c8df2 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 domain part of the fully qualified group name. --- src/providers/ipa/ipa_hbac_common.c | 86 ++++++++++++++++++++++++------------- 1 file changed, 57 insertions(+), 29 deletions(-) diff --git a/src/providers/ipa/ipa_hbac_common.c b/src/providers/ipa/ipa_hbac_common.c index b99b75d..6470698 100644 --- a/src/providers/ipa/ipa_hbac_common.c +++ b/src/providers/ipa/ipa_hbac_common.c @@ -507,15 +507,17 @@ 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; + char *group_dom_name; + struct sss_domain_info *ipa_domain; tmp_ctx = talloc_new(mem_ctx); if (tmp_ctx == NULL) return ENOMEM; @@ -533,12 +535,14 @@ 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; + } + 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 +550,67 @@ 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; - - ret = get_ipa_groupname(users->groups, domain->sysdb, member_dn, - &users->groups[num_groups]); - if (ret != EOK && ret != ERR_UNEXPECTED_ENTRY_TYPE) { + 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", member_dn); + "Skipping malformed entry [%s]\n", + ldb_dn_get_linearized(members[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++; + } + + ret = sss_parse_internal_fqname(tmp_ctx, fqgroupname, + &shortname, &group_dom_name); + if (ret != EOK) { + ret = ERR_WRONG_NAME_FORMAT; continue; } - /* Skip entries that are not groups */ - DEBUG(SSSDBG_TRACE_INTERNAL, - "Skipping non-group memberOf [%s]\n", member_dn); + + /* FIXME - alternatively, look at each group's DN and check if it matches + * the IPA DN rather than parsing the domain from the FQNAME? + */ + if (strcasecmp(ipa_domain->name, group_dom_name) != 0) { + DEBUG(SSSDBG_TRACE_LIBS, + "Skipping non-IPA group %s\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]
