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@ad.domain > > sss_cache -E > > getent group group_with_posix_gid@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.
>From bee1f66b2cf6e06821a2da2db8c93712acbdb13f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@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
>From a1a20bb1c630db85490fd68191d1021c75a43cf6 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@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
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel