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 d4aa2dfa38515f084f16e20ad8d2b007dae1ff8d 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. Resolves: https://pagure.io/SSSD/sssd/issue/3382 --- src/providers/ipa/ipa_hbac_common.c | 111 +++++++++++++++++++++++++++--------- 1 file changed, 83 insertions(+), 28 deletions(-) diff --git a/src/providers/ipa/ipa_hbac_common.c b/src/providers/ipa/ipa_hbac_common.c index b99b75d..6efff3e 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,86 @@ 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; + } - ret = get_ipa_groupname(users->groups, domain->sysdb, member_dn, - &users->groups[num_groups]); - if (ret != EOK && ret != ERR_UNEXPECTED_ENTRY_TYPE) { + /* 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.. + */ + 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; + } + + /* ..then remove the RDN which is the group name.. */ + 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++; + } + + /* ..the rest should be a DN in the form of: + * cn=groups,cn=ipa_domain_name,cn=sysdb + * If not, just skip this DN. Skipping is safe here, because the rules + * only allow access, never deny, so at worst the user would be denied + * legitimate access + */ + if (ldb_dn_compare(ipa_groups_basedn, member_group_container) != 0) { + DEBUG(SSSDBG_FUNC_DATA, "Skipping non-IPA group %s\n", fqgroupname); + continue; + } + + ret = sss_parse_internal_fqname(tmp_ctx, fqgroupname, + &shortname, NULL); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, "Malformed name %s, skipping!\n", fqgroupname); continue; } - /* Skip entries that are not groups */ - DEBUG(SSSDBG_TRACE_INTERNAL, - "Skipping non-group memberOf [%s]\n", member_dn); + + 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]
