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)

Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>

1 files 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);