Merge ~orion-cora/ubuntu/+source/sssd:xenial-sssd-hbac-rule-1722936 into ~usd-import-team/ubuntu/+source/sssd:ubuntu/xenial-devel

[Orion-cora]

Add upstream HBAC patch. Closes LP: #1722936.

[Andreas Hasenack (ahasenack)]

Thanks! Could you please also add a review slot for "canonical-server"? Then it will show up in our queue more easily.

[Andreas Hasenack (ahasenack)]

- lint: OK
- source build: OK
- binary build: OK
- packaging: OK

Do you have a simple way that we can use to test this patch? Or do we need to setup AD and IPA?

review: Needs Information

[Orion-cora (orion-cora)]

I don't have a simple test. The issue we see with AD users trusted by IPA is that sometimes the supplemental group list is not correct. This then can lead to logins not being allowed due to HBAC rules not applying any more. I've been running with this patch applied on one system with no troubles for a month or so. It's highly recommended by upstream, they just have never released a 1.13.5 version with it. But this patch is from the 1.13 branch.

[Andreas Hasenack (ahasenack)]

I'll have to come up with a test plan for this change if we want to SRU it. I should be able to start work on that this week.

Unmerged commits

4241de7... by Orion-cora on 2017-11-27

Add upstream HBAC patch. Closes LP: #1722936.

diff --git a/debian/changelog b/debian/changelog
index d89041d..8882cb4 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+sssd (1.13.4-1ubuntu1.10) xenial; urgency=medium
+
+  * Add upstream HBAC patch.  Closes LP: #1722936.
+
+ -- Orion Poplawski <[e-mail]>  Mon, 27 Nov 2017 09:38:29 -0700
+
 sssd (1.13.4-1ubuntu1.9) xenial; urgency=medium
 
   * debian/patches/bad-initgroups-results-3045.patch: sdap: Fix
diff --git a/debian/patches/hbac.patch b/debian/patches/hbac.patch
new file mode 100644
index 0000000..b65daff
--- /dev/null
+++ b/debian/patches/hbac.patch
@@ -0,0 +1,170 @@
+From 88f6d8ad4eef4b4fa032fd451ad732cf8201b0bf Mon Sep 17 00:00:00 2001
+From: Jakub Hrozek <[e-mail]>
+Date: Jul 27 2017 07:39:58 +0000
+Subject: 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 <[e-mail]>
+Reviewed-by: Fabiano Fidêncio <[e-mail]>
+
+---
+
+diff --git a/src/providers/ipa/ipa_hbac_common.c b/src/providers/ipa/ipa_hbac_common.c
+index 72a620e..4afe44e 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);
+
diff --git a/debian/patches/series b/debian/patches/series
index d556fe1..1ea2a07 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -5,3 +5,4 @@ pidfile-creation.diff
 sanitize_newline.diff
 attempt_ptr_update_on_nonzero_return.diff
 bad-initgroups-results-3045.patch
+hbac.patch