[SSSD] [PATCH] IPA: Check connection validity explicitly

Jakub Hrozek jhrozek at redhat.com
Mon Oct 5 14:46:55 UTC 2015


On Mon, Oct 05, 2015 at 12:33:48PM +0200, Sumit Bose wrote:
> On Mon, Oct 05, 2015 at 12:08:26PM +0200, Jakub Hrozek wrote:
> > On Fri, Oct 02, 2015 at 10:32:27AM +0200, Jakub Hrozek wrote:
> > > On Thu, Oct 01, 2015 at 01:41:07PM +0200, Jakub Hrozek wrote:
> > > > I don't have a good idea how to reproduce except simulate the failure in
> > > > gdb, sorry... at least I verified that after setting the context to NULL:
> > > >         (gdb) set clist[1] = 0
> > > > the request runs to completion and SSSD doesn't crash anymore. So I think
> > > > we should just add a check..
> > > 
> > > Turns out this is easier to reproduce and hence more embarassing. Just
> > > add POSIX attributes to AD but don't replicate them to GC..
> > 
> > Any takers for review?
> 
> (I already took it some time ago in patchworks :-)
> 
> While the patch fixes the issue and is also a right way to fix the issue
> I would recommend to change it a bit to make the reason more clear.
> 
> As you said to reproduce the issue you need trust to AD configured which
> POSIX attributes but not replicate them to the Global Catalog. If you
> new start with an empty cache a sequence like
> 
> getent group group_with_posix_gid at ad.domain
> sss_cache -E
> getent group group_with_posix_gid at ad.domain
> 
> will trigger the crash.
> 
> What happens is that during the first lookup SSSD sees that the GC does
> not have the POSIX attributes and disable the GC lookup. As a
> consequence ad_gc_conn_list() will return only a single entry for the
> second lookup and unconditionally accessing the second entry will fail.
> While the fix is correct, I would recommend to add a boolean option to 
> ad_gc_conn_list() and set ignore_mark_offline in ad_gc_conn_list().
> Otherwise you have to add extra logic to ipa_get_ad_acct_send() to make
> sure ignore_mark_offline is set even it ad_gc_conn_list() only returns
> one element.

You're right that the logic was scattered all over the place. The
attached patches consolidate it into ad_common.c so all callers use the
same code. It's a larger change than before, but hopefully it's still
digestable and as a bonus we can have tests for different combinations
of contexts, GC status and master/sub domains.

The second patch is not really needed, the only reason I included it is
that I think it's better to have similar code grouped together in small
functions and again, tests.

If you think these are too invasive, we can only apply them to master
and do exactly what you proposed for sssd-1-13.

btw I had a bit of trouble with downstream tests, the seem to fail in
unrelated way, so I'm sending the patches to the list after some manual
testing and will resume the downstream tests tomorrow..
-------------- next part --------------
>From ade75795f8bf7a99a1438c1daf39b26f30473133 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 1 Oct 2015 13:13:05 +0200
Subject: [PATCH 1/2] AD: Provide common connection list construction functions

https://fedorahosted.org/sssd/ticket/2810

Provides a new AD common function ad_ldap_conn_list() that creates a
list of AD connection to use along with properties to avoid mistakes
when manually constructing these lists.
---
 src/providers/ad/ad_common.c          | 32 ++++++++++++++++++++++++
 src/providers/ad/ad_common.h          |  5 ++++
 src/providers/ad/ad_id.c              | 17 +------------
 src/providers/ipa/ipa_subdomains_id.c | 21 ++++++----------
 src/tests/cmocka/test_ad_common.c     | 47 ++++++++++++++++++++++++++++++-----
 5 files changed, 87 insertions(+), 35 deletions(-)

diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 88f36f8ead64443e07b2393aa22ad4d9708d0a0e..a40cdac935d88ea0ad5a2b7d713d026c5cd3110f 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -1240,6 +1240,18 @@ ad_get_dom_ldap_conn(struct ad_id_ctx *ad_ctx, struct sss_domain_info *dom)
     return conn;
 }
 
+static inline void set_con_properties(struct sdap_id_conn_ctx *conn,
+                                      struct sss_domain_info *dom)
+{
+    if (IS_SUBDOMAIN(dom) == true && conn != NULL) {
+        /* Regardless of connection types, a subdomain error must not be
+         * allowed to set the whole back end offline, rather report an error
+         * and let the caller deal with it (normally disable the subdomain
+         */
+        conn->ignore_mark_offline = true;
+    }
+}
+
 struct sdap_id_conn_ctx **
 ad_gc_conn_list(TALLOC_CTX *mem_ctx, struct ad_id_ctx *ad_ctx,
                 struct sss_domain_info *dom)
@@ -1258,6 +1270,26 @@ ad_gc_conn_list(TALLOC_CTX *mem_ctx, struct ad_id_ctx *ad_ctx,
     }
 
     clist[cindex] = ad_get_dom_ldap_conn(ad_ctx, dom);
+    set_con_properties(clist[cindex], dom);
 
     return clist;
 }
+
+struct sdap_id_conn_ctx **
+ad_ldap_conn_list(TALLOC_CTX *mem_ctx,
+                  struct ad_id_ctx *ad_ctx,
+                  struct sss_domain_info *dom)
+{
+    struct sdap_id_conn_ctx **clist;
+
+    clist = talloc_zero_array(mem_ctx, struct sdap_id_conn_ctx *, 2);
+    if (clist == NULL) {
+        return NULL;
+    }
+
+    clist[0] = ad_get_dom_ldap_conn(ad_ctx, dom);
+    set_con_properties(clist[0], dom);
+
+    clist[1] = NULL;
+    return clist;
+}
diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
index 817f5b42cad7cad6a88244fd43bd91a4358d56c0..701e461987cb286ca7add2766ffb4dc496bde01e 100644
--- a/src/providers/ad/ad_common.h
+++ b/src/providers/ad/ad_common.h
@@ -148,6 +148,11 @@ struct sdap_id_conn_ctx **
 ad_gc_conn_list(TALLOC_CTX *mem_ctx, struct ad_id_ctx *ad_ctx,
                struct sss_domain_info *dom);
 
+struct sdap_id_conn_ctx **
+ad_ldap_conn_list(TALLOC_CTX *mem_ctx,
+                  struct ad_id_ctx *ad_ctx,
+                  struct sss_domain_info *dom);
+
 struct sdap_id_conn_ctx *
 ad_get_dom_ldap_conn(struct ad_id_ctx *ad_ctx, struct sss_domain_info *dom);
 
diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
index ecaf6c993bf7ddb7ba565d40ef0ad250114f5536..be0cb3b12f2e3a2b53d740ecf3befc07fd853f8b 100644
--- a/src/providers/ad/ad_id.c
+++ b/src/providers/ad/ad_id.c
@@ -269,29 +269,14 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx *ad_ctx,
     case BE_REQ_GROUP: /* group */
     case BE_REQ_INITGROUPS: /* init groups for user */
         clist = ad_gc_conn_list(breq, ad_ctx, dom);
-        if (clist == NULL) return NULL;
         break;
 
     default:
         /* Requests for other object should only contact LDAP by default */
-        clist = talloc_zero_array(breq, struct sdap_id_conn_ctx *, 2);
-        if (clist == NULL) return NULL;
-
-        clist[0] = ad_ctx->ldap_ctx;
-        clist[1] = NULL;
+        clist = ad_ldap_conn_list(breq, ad_ctx, dom);
         break;
     }
 
-    /* Regardless of connection types, a subdomain error must not be allowed
-     * to set the whole back end offline, rather report an error and let the
-     * caller deal with it (normally disable the subdomain
-     */
-    if (IS_SUBDOMAIN(dom)) {
-        for (cindex = 0; clist[cindex] != NULL; cindex++) {
-            clist[cindex]->ignore_mark_offline = true;
-        }
-    }
-
     return clist;
 }
 
diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
index 8f13608bcfd2f17c27fcba7f087e1a27086a2a1c..472985d4ab4f785aa9c4af94bf8021829ca1c3c8 100644
--- a/src/providers/ipa/ipa_subdomains_id.c
+++ b/src/providers/ipa/ipa_subdomains_id.c
@@ -641,21 +641,16 @@ ipa_get_ad_acct_send(TALLOC_CTX *mem_ctx,
     case BE_REQ_BY_SECID:
     case BE_REQ_GROUP:
         clist = ad_gc_conn_list(req, ad_id_ctx, state->obj_dom);
-        if (clist == NULL) {
-            ret = ENOMEM;
-            goto fail;
-        }
-        clist[1]->ignore_mark_offline = true;
         break;
     default:
-        clist = talloc_zero_array(req, struct sdap_id_conn_ctx *, 2);
-        if (clist == NULL) {
-            ret = ENOMEM;
-            goto fail;
-        }
-        clist[0] = ad_id_ctx->ldap_ctx;
-        clist[0]->ignore_mark_offline = true;
-        clist[1] = NULL;
+        clist = ad_ldap_conn_list(req, ad_id_ctx, state->obj_dom);
+        break;
+    }
+
+    if (clist == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, "Cannot generate AD connection list!\n");
+        ret = ENOMEM;
+        goto fail;
     }
 
     /* Now we already need ad_id_ctx in particular sdap_id_conn_ctx */
diff --git a/src/tests/cmocka/test_ad_common.c b/src/tests/cmocka/test_ad_common.c
index bc9d0940bb22cc4b11f5a5b012ac4ded338714a0..d2b59a23dfbff0bfda8ec7a52a71aec99f56baf3 100644
--- a/src/tests/cmocka/test_ad_common.c
+++ b/src/tests/cmocka/test_ad_common.c
@@ -350,7 +350,7 @@ __wrap_sdap_set_sasl_options(struct sdap_options *id_opts,
     return EOK;
 }
 
-void test_ldap_conn_list(void **state)
+void test_ad_get_dom_ldap_conn(void **state)
 {
     struct sdap_id_conn_ctx *conn;
 
@@ -365,7 +365,7 @@ void test_ldap_conn_list(void **state)
     assert_true(conn == test_ctx->subdom_ad_ctx->ldap_ctx);
 }
 
-void test_conn_list(void **state)
+void test_gc_conn_list(void **state)
 {
     struct sdap_id_conn_ctx **conn_list;
 
@@ -392,7 +392,8 @@ void test_conn_list(void **state)
     assert_true(conn_list[0] == test_ctx->ad_ctx->gc_ctx);
     assert_true(conn_list[0]->ignore_mark_offline);
     assert_true(conn_list[1] == test_ctx->subdom_ad_ctx->ldap_ctx);
-    assert_false(conn_list[1]->ignore_mark_offline);
+    /* Subdomain error should not set the backend offline! */
+    assert_true(conn_list[1]->ignore_mark_offline);
     talloc_free(conn_list);
 
     dp_opt_set_bool(test_ctx->ad_ctx->ad_options->basic, AD_ENABLE_GC, false);
@@ -411,6 +412,37 @@ void test_conn_list(void **state)
     assert_non_null(conn_list);
 
     assert_true(conn_list[0] == test_ctx->subdom_ad_ctx->ldap_ctx);
+    assert_true(conn_list[0]->ignore_mark_offline);
+    assert_null(conn_list[1]);
+    talloc_free(conn_list);
+}
+
+void test_ldap_conn_list(void **state)
+{
+    struct sdap_id_conn_ctx **conn_list;
+
+    struct ad_common_test_ctx *test_ctx = talloc_get_type(*state,
+                                                     struct ad_common_test_ctx);
+    assert_non_null(test_ctx);
+
+    conn_list = ad_ldap_conn_list(test_ctx,
+                                  test_ctx->ad_ctx,
+                                  test_ctx->dom);
+    assert_non_null(conn_list);
+
+    assert_true(conn_list[0] == test_ctx->ad_ctx->ldap_ctx);
+    assert_false(conn_list[0]->ignore_mark_offline);
+    assert_null(conn_list[1]);
+    talloc_free(conn_list);
+
+    conn_list = ad_ldap_conn_list(test_ctx,
+                                  test_ctx->ad_ctx,
+                                  test_ctx->subdom);
+    assert_non_null(conn_list);
+
+    assert_true(conn_list[0] == test_ctx->subdom_ad_ctx->ldap_ctx);
+    assert_true(conn_list[0]->ignore_mark_offline);
+    assert_null(conn_list[1]);
     talloc_free(conn_list);
 }
 
@@ -432,12 +464,15 @@ int main(int argc, const char *argv[])
         cmocka_unit_test_setup_teardown(test_ad_create_2way_trust_options,
                                         test_ad_common_setup,
                                         test_ad_common_teardown),
+        cmocka_unit_test_setup_teardown(test_ad_get_dom_ldap_conn,
+                                        test_ldap_conn_setup,
+                                        test_ldap_conn_teardown),
+        cmocka_unit_test_setup_teardown(test_gc_conn_list,
+                                        test_ldap_conn_setup,
+                                        test_ldap_conn_teardown),
         cmocka_unit_test_setup_teardown(test_ldap_conn_list,
                                         test_ldap_conn_setup,
                                         test_ldap_conn_teardown),
-        cmocka_unit_test_setup_teardown(test_conn_list,
-                                        test_ldap_conn_setup,
-                                        test_ldap_conn_teardown),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
2.4.3

-------------- next part --------------
>From 199f814dea03a68918d333380879e00d99973414 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 5 Oct 2015 16:11:14 +0200
Subject: [PATCH 2/2] AD: Consolidate connection list construction on
 ad_common.c

---
 src/providers/ad/ad_common.c      | 32 ++++++++++++++++++++++++++++++++
 src/providers/ad/ad_common.h      |  5 +++++
 src/providers/ad/ad_id.c          | 18 +-----------------
 src/tests/cmocka/test_ad_common.c | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 17 deletions(-)

diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index a40cdac935d88ea0ad5a2b7d713d026c5cd3110f..16a646ee2b6e01c90edf45569dccb4ae2c160d6b 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -1293,3 +1293,35 @@ ad_ldap_conn_list(TALLOC_CTX *mem_ctx,
     clist[1] = NULL;
     return clist;
 }
+
+struct sdap_id_conn_ctx **
+ad_user_conn_list(TALLOC_CTX *mem_ctx,
+                  struct ad_id_ctx *ad_ctx,
+                  struct sss_domain_info *dom)
+{
+    struct sdap_id_conn_ctx **clist;
+    int cindex = 0;
+
+    clist = talloc_zero_array(ad_ctx, struct sdap_id_conn_ctx *, 3);
+    if (clist == NULL) {
+        return NULL;
+    }
+
+    /* Try GC first for users from trusted domains, but go to LDAP
+     * for users from non-trusted domains to get all POSIX attrs
+     */
+    if (dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC)
+            && IS_SUBDOMAIN(dom)) {
+        clist[cindex] = ad_ctx->gc_ctx;
+        clist[cindex]->ignore_mark_offline = true;
+        cindex++;
+    }
+
+    /* Users from primary domain can be just downloaded from LDAP.
+     * The domain's LDAP connection also works as a fallback
+     */
+    clist[cindex] = ad_get_dom_ldap_conn(ad_ctx, dom);
+    set_con_properties(clist[cindex], dom);
+
+    return clist;
+}
diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
index 701e461987cb286ca7add2766ffb4dc496bde01e..0cefa1859aaa75731267917e66ab9a1905528e91 100644
--- a/src/providers/ad/ad_common.h
+++ b/src/providers/ad/ad_common.h
@@ -153,6 +153,11 @@ ad_ldap_conn_list(TALLOC_CTX *mem_ctx,
                   struct ad_id_ctx *ad_ctx,
                   struct sss_domain_info *dom);
 
+struct sdap_id_conn_ctx **
+ad_user_conn_list(TALLOC_CTX *mem_ctx,
+                  struct ad_id_ctx *ad_ctx,
+                  struct sss_domain_info *dom);
+
 struct sdap_id_conn_ctx *
 ad_get_dom_ldap_conn(struct ad_id_ctx *ad_ctx, struct sss_domain_info *dom);
 
diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
index be0cb3b12f2e3a2b53d740ecf3befc07fd853f8b..51d378863a5c7394ca3a2b8bd72f8c131a2b02b1 100644
--- a/src/providers/ad/ad_id.c
+++ b/src/providers/ad/ad_id.c
@@ -244,25 +244,10 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx *ad_ctx,
               struct sss_domain_info *dom, struct be_acct_req *ar)
 {
     struct sdap_id_conn_ctx **clist;
-    int cindex = 0;
 
     switch (ar->entry_type & BE_REQ_TYPE_MASK) {
     case BE_REQ_USER: /* user */
-        clist = talloc_zero_array(ad_ctx, struct sdap_id_conn_ctx *, 3);
-        if (clist == NULL) return NULL;
-
-        /* Try GC first for users from trusted domains */
-        if (dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC)
-                && IS_SUBDOMAIN(dom)) {
-            clist[cindex] = ad_ctx->gc_ctx;
-            clist[cindex]->ignore_mark_offline = true;
-            cindex++;
-        }
-
-        /* Users from primary domain can be just downloaded from LDAP.
-         * The domain's LDAP connection also works as a fallback
-         */
-        clist[cindex] = ad_get_dom_ldap_conn(ad_ctx, dom);
+        clist = ad_user_conn_list(breq, ad_ctx, dom);
         break;
     case BE_REQ_BY_SECID:   /* by SID */
     case BE_REQ_USER_AND_GROUP: /* get SID */
@@ -270,7 +255,6 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx *ad_ctx,
     case BE_REQ_INITGROUPS: /* init groups for user */
         clist = ad_gc_conn_list(breq, ad_ctx, dom);
         break;
-
     default:
         /* Requests for other object should only contact LDAP by default */
         clist = ad_ldap_conn_list(breq, ad_ctx, dom);
diff --git a/src/tests/cmocka/test_ad_common.c b/src/tests/cmocka/test_ad_common.c
index d2b59a23dfbff0bfda8ec7a52a71aec99f56baf3..b0cf4b5e6b0559c2896273bfcfb1af99cad195a3 100644
--- a/src/tests/cmocka/test_ad_common.c
+++ b/src/tests/cmocka/test_ad_common.c
@@ -446,6 +446,37 @@ void test_ldap_conn_list(void **state)
     talloc_free(conn_list);
 }
 
+void test_user_conn_list(void **state)
+{
+    struct sdap_id_conn_ctx **conn_list;
+
+    struct ad_common_test_ctx *test_ctx = talloc_get_type(*state,
+                                                     struct ad_common_test_ctx);
+    assert_non_null(test_ctx);
+
+    conn_list = ad_user_conn_list(test_ctx,
+                                  test_ctx->ad_ctx,
+                                  test_ctx->dom);
+    assert_non_null(conn_list);
+
+    assert_true(conn_list[0] == test_ctx->ad_ctx->ldap_ctx);
+    assert_false(conn_list[0]->ignore_mark_offline);
+    assert_null(conn_list[1]);
+    talloc_free(conn_list);
+
+    conn_list = ad_user_conn_list(test_ctx,
+                                  test_ctx->ad_ctx,
+                                  test_ctx->subdom);
+    assert_non_null(conn_list);
+
+    assert_true(conn_list[0] == test_ctx->ad_ctx->gc_ctx);
+    assert_true(conn_list[0]->ignore_mark_offline);
+    assert_true(conn_list[1] == test_ctx->subdom_ad_ctx->ldap_ctx);
+    /* Subdomain error should not set the backend offline! */
+    assert_true(conn_list[1]->ignore_mark_offline);
+    talloc_free(conn_list);
+}
+
 int main(int argc, const char *argv[])
 {
     poptContext pc;
@@ -473,6 +504,9 @@ int main(int argc, const char *argv[])
         cmocka_unit_test_setup_teardown(test_ldap_conn_list,
                                         test_ldap_conn_setup,
                                         test_ldap_conn_teardown),
+        cmocka_unit_test_setup_teardown(test_user_conn_list,
+                                        test_ldap_conn_setup,
+                                        test_ldap_conn_teardown),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
2.4.3



More information about the sssd-devel mailing list