From c7c087b5485d50e8689d31fd9d52af935ae398be Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Sun, 9 Apr 2017 20:50:47 +0200
Subject: [PATCH 142/142] 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
Reviewed-by: Sumit Bose <sbose@redhat.com>
(cherry picked from commit c92e49144978ad3b6c9fffa8803ebdad8f6f5b18)
---
src/providers/ipa/ipa_hbac_common.c | 97 +++++++++++++++++++++++++------------
1 file changed, 67 insertions(+), 30 deletions(-)
diff --git a/src/providers/ipa/ipa_hbac_common.c b/src/providers/ipa/ipa_hbac_common.c
index b99b75d322930f16412f6abd4cdf0d7e0b59c32c..ba677965a3eb68a54baf99b1875bca2acbb76c99 100644
--- a/src/providers/ipa/ipa_hbac_common.c
+++ b/src/providers/ipa/ipa_hbac_common.c
@@ -507,15 +507,15 @@ 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 };
char *shortname;
+ const char *fqgroupname = NULL;
+ 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;
@@ -533,56 +533,93 @@ 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;
+ }
+
+ /* +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, username,
- 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) {
+ fqgroupname = ldb_msg_find_attr_as_string(res->msgs[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(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++;
+ }
+
+ 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 < (res->count - 1)) {
/* Shrink the array memory */
users->groups = talloc_realloc(users, users->groups, const char *,
num_groups+1);
--
2.9.4