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

Jakub Hrozek jhrozek at redhat.com
Tue Oct 6 10:54:41 UTC 2015


On Mon, Oct 05, 2015 at 04:46:55PM +0200, Jakub Hrozek wrote:
> 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..

The attached patches fix some more Sumit's review comments. With Lukas'
help I was also able to run downstream ad_forest tests and didn't see
any issues even with the patches.
-------------- next part --------------
>From bee1f66b2cf6e06821a2da2db8c93712acbdb13f 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          | 26 +++++++++++++++++++
 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, 81 insertions(+), 35 deletions(-)

diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 88f36f8ead64443e07b2393aa22ad4d9708d0a0e..7d46af4a4bad7d32cac33c731659bc0ac3441fb0 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -1237,6 +1237,14 @@ ad_get_dom_ldap_conn(struct ad_id_ctx *ad_ctx, struct sss_domain_info *dom)
     subdom_id_ctx = talloc_get_type(sdom->pvt, struct ad_id_ctx);
     conn = subdom_id_ctx->ldap_ctx;
 
+    if (IS_SUBDOMAIN(sdom->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;
+    }
+
     return conn;
 }
 
@@ -1261,3 +1269,21 @@ ad_gc_conn_list(TALLOC_CTX *mem_ctx, struct ad_id_ctx *ad_ctx,
 
     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);
+
+    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 a1a20bb1c630db85490fd68191d1021c75a43cf6 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      | 31 +++++++++++++++++++++++++++++++
 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, 71 insertions(+), 17 deletions(-)

diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 7d46af4a4bad7d32cac33c731659bc0ac3441fb0..ffc1351241e1874ebd37fb4a865ad5822cdea479 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -1287,3 +1287,34 @@ 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);
+
+    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