On Thu, Feb 25, 2016 at 12:50:55PM +0100, Jakub Hrozek wrote: > On Tue, Feb 23, 2016 at 12:53:25PM +0100, Sumit Bose wrote: > > Hi, > > > > this patch fixes and issue during initgroups in AD forests. Please see > > the commit message for details. > > > > To reproduce this you can create a new user outside of CN=Users on the > > forest root. The new user can be created in an existing container or in > > a new OU container. Most important is that it is not a child of > > CN=Users. In a child domain (it must be a child, domains with a > > different base won't trigger the issue) create a user with the same > > name. With this setup 'id [email protected]' will not return the complete > > list of group the user is a member of and the patch should fix this. > > > > bye, > > Sumit > > > > Hi, > > the patch works fine, tested with a user named the same in the same OU > in different domains. Coverity found some warnings: > > Error: COMPILER_WARNING: > sssd-1.13.90/src/providers/ldap/sdap_async_initgroups.c:2835:12: warning: > unused variable 'dn_len' [-Wunused-variable] > # size_t dn_len; > # ^ > # 2833| const char *cname; > # 2834| bool in_transaction = false; > # 2835|-> size_t dn_len; > # 2836| size_t c = 0; > # 2837| > > Error: COMPILER_WARNING: > sssd-1.13.90/src/providers/ldap/sdap_async_initgroups.c: scope_hint: In > function 'sdap_get_initgr_user' > sssd-1.13.90/src/providers/ldap/sdap_async_initgroups.c:2836:12: warning: > unused variable 'c' [-Wunused-variable] > # size_t c = 0; > # ^ > # 2834| bool in_transaction = false; > # 2835| size_t dn_len; > # 2836|-> size_t c = 0; > # 2837| > # 2838| DEBUG(SSSDBG_TRACE_ALL, "Receiving info for the user\n"); > > Error: COMPILER_WARNING: > sssd-1.13.90/src/db/sysdb_subdomains.c:23: included_from: Included from here. > sssd-1.13.90/src/db/sysdb_subdomains.c: scope_hint: In function > 'try_to_find_expected_dn' > sssd-1.13.90/src/util/util.h:144:9: warning: 'result_dn_str' may be used > uninitialized in this function [-Wmaybe-uninitialized] > # sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \ > # ^ > sssd-1.13.90/src/db/sysdb_subdomains.c:1075:17: note: 'result_dn_str' was > declared here > # const char *result_dn_str; > # ^ > # 142| int __debug_macro_level = level; \ > # 143| if (DEBUG_IS_SET(__debug_macro_level)) { \ > # 144|-> sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \ > # 145| __debug_macro_level, \ > # 146| format, ##__VA_ARGS__); \ > > Additionally, I wonder if we should namespace the function?
Thank you for the review, new version attached. bye, Sumit > _______________________________________________ > sssd-devel mailing list > [email protected] > https://lists.fedorahosted.org/admin/lists/[email protected]
From 58e44cfa33eefe44d0c0deda94453cdf2b00b4a9 Mon Sep 17 00:00:00 2001 From: Sumit Bose <[email protected]> Date: Fri, 22 Jan 2016 18:14:45 +0100 Subject: [PATCH] sdap: improve filtering of multiple results in GC lookups The Global Catalog of AD contains some information about all users and groups in an AD forest. Users from different domain in the forest can have the same name. The most obvious example is the Administrator user which is present in all domains. Although SSSD uses a domain specific search base for looking up users in the GC the search might still return multiple results if there is a user with the same name in one of the child (or grand-child ...) domains because of the hierarchic nature of the LDAP tree. Limiting the search depth would not help because users can be created in deeply nested OUs. Currently SSSD expects in this case that the user object is store in CN=Users or below. This works for all default users like Administrator but in general users can be created anywhere in the directory tree. If a user is created outside of CN=Users and there is a user with the same name in a child domain the initgroups command to look up the group-memberships of the user fails because it is not clear which of the two results should be used (initgroups for the child domain user works fine). This patch adds an additional scheme to select the right result based on the domain component attribute name 'dc'. This attribute indicates an additional component in the domain name and hence a child domain. So as long as the result contains a dc component following out search base it cannot be the object we are looking for. This scheme includes the old CN=Users based one but since it is more expensive I kept the old scheme which so far worked all the time and only use the new one if the old one fails. Resolves https://fedorahosted.org/sssd/ticket/2961 --- src/db/sysdb.h | 6 ++ src/db/sysdb_subdomains.c | 153 +++++++++++++++++++++++++++++ src/providers/ldap/sdap_async_initgroups.c | 48 ++------- src/tests/cmocka/test_sysdb_subdomains.c | 73 ++++++++++++++ 4 files changed, 238 insertions(+), 42 deletions(-) diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 95a9086766228a6c36c56d3a68a0bb0e493c0cbe..6dd9665f5b73c9996013d767a47fc02c445783f1 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -1227,4 +1227,10 @@ errno_t sysdb_handle_original_uuid(const char *orig_name, const char *src_name, struct sysdb_attrs *dest_attrs, const char *dest_name); + +errno_t sysdb_try_to_find_expected_dn(struct sss_domain_info *dom, + const char *domain_component_name, + struct sysdb_attrs **usr_attrs, + size_t count, + struct sysdb_attrs **exp_usr); #endif /* __SYS_DB_H__ */ diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index b2bf1a0742171b7beccb44fa915c8adba51fefa3..456e6621b3434a9dbf2e611ad880facbc171c174 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -1049,3 +1049,156 @@ done: talloc_free(tmp_ctx); return ret; } + +errno_t sysdb_try_to_find_expected_dn(struct sss_domain_info *dom, + const char *domain_component_name, + struct sysdb_attrs **usr_attrs, + size_t count, + struct sysdb_attrs **exp_usr) +{ + char *dom_basedn; + size_t dom_basedn_len; + char *expected_basedn; + size_t expected_basedn_len; + size_t dn_len; + const char *orig_dn; + size_t c = 0; + int ret; + TALLOC_CTX *tmp_ctx; + struct ldb_context *ldb_ctx; + struct ldb_dn *ldb_dom_basedn; + int dom_basedn_comp_num; + struct ldb_dn *ldb_dn; + int dn_comp_num; + const char *component_name; + struct sysdb_attrs *result = NULL; + const char *result_dn_str = NULL; + + if (dom == NULL || domain_component_name == NULL || usr_attrs == NULL + || count == 0) { + return EINVAL; + } + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n"); + return ENOMEM; + } + + ret = domain_to_basedn(tmp_ctx, dom->name, &dom_basedn); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "domain_to_basedn failed.\n"); + goto done; + } + expected_basedn = talloc_asprintf(tmp_ctx, "%s%s", "cn=users,", dom_basedn); + if (expected_basedn == NULL) { + ret = ENOMEM; + goto done; + } + + ldb_ctx = sysdb_ctx_get_ldb(dom->sysdb); + if (ldb_ctx == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "Missing ldb context.\n"); + ret = EINVAL; + goto done; + } + + ldb_dom_basedn = ldb_dn_new(tmp_ctx, ldb_ctx, dom_basedn); + if (ldb_dom_basedn == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_new failed.\n"); + ret = ENOMEM; + goto done; + } + + dom_basedn_comp_num = ldb_dn_get_comp_num(ldb_dom_basedn); + dom_basedn_comp_num++; + + DEBUG(SSSDBG_TRACE_ALL, "Expected BaseDN is [%s].\n", expected_basedn); + expected_basedn_len = strlen(expected_basedn); + dom_basedn_len = strlen(dom_basedn); + + for (c = 0; c < count; c++) { + ret = sysdb_attrs_get_string(usr_attrs[c], SYSDB_ORIG_DN, &orig_dn); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_string failed.\n"); + goto done; + } + dn_len = strlen(orig_dn); + + if (dn_len > expected_basedn_len + && strcasecmp(orig_dn + (dn_len - expected_basedn_len), + expected_basedn) == 0) { + DEBUG(SSSDBG_TRACE_ALL, + "Found matching dn [%s].\n", orig_dn); + if (result != NULL) { + DEBUG(SSSDBG_OP_FAILURE, + "Found 2 matching DN [%s] and [%s], expecting only 1.\n", + result_dn_str, orig_dn); + ret = EINVAL; + goto done; + } + result = usr_attrs[c]; + result_dn_str = orig_dn; + } + } + + if (result == NULL) { + for (c = 0; c < count; c++) { + ret = sysdb_attrs_get_string(usr_attrs[c], SYSDB_ORIG_DN, &orig_dn); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_string failed.\n"); + goto done; + } + dn_len = strlen(orig_dn); + + if (dn_len > dom_basedn_len + && strcasecmp(orig_dn + (dn_len - dom_basedn_len), + dom_basedn) == 0) { + ldb_dn = ldb_dn_new(tmp_ctx, ldb_ctx, orig_dn); + if (ldb_dn == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_new failed"); + ret = ENOMEM; + goto done; + } + + dn_comp_num = ldb_dn_get_comp_num(ldb_dn); + if (dn_comp_num > dom_basedn_comp_num) { + component_name = ldb_dn_get_component_name(ldb_dn, + (dn_comp_num - dom_basedn_comp_num)); + DEBUG(SSSDBG_TRACE_ALL, "Comparing [%s] and [%s].\n", + component_name, + domain_component_name); + if (component_name != NULL + && strcasecmp(component_name, + domain_component_name) != 0) { + DEBUG(SSSDBG_TRACE_ALL, + "Found matching dn [%s].\n", orig_dn); + if (result != NULL) { + DEBUG(SSSDBG_OP_FAILURE, + "Found 2 matching DN [%s] and [%s], " + "expecting only 1.\n", result_dn_str, orig_dn); + ret = EINVAL; + goto done; + } + result = usr_attrs[c]; + result_dn_str = orig_dn; + } + } + } + } + } + + if (result == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "No matching DN found.\n"); + ret = ENOENT; + goto done; + } + + *exp_usr = result; + + ret = EOK; +done: + talloc_free(tmp_ctx); + + return ret; +} diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c index 39b310a4e2f6b4a0cef79b9c32f6e286e57ffde0..fa19876a4a60e1396d3b5940a828cfababe886fe 100644 --- a/src/providers/ldap/sdap_async_initgroups.c +++ b/src/providers/ldap/sdap_async_initgroups.c @@ -2832,10 +2832,6 @@ static void sdap_get_initgr_user(struct tevent_req *subreq) const char *orig_dn; const char *cname; bool in_transaction = false; - char *expected_basedn; - size_t expected_basedn_len; - size_t dn_len; - size_t c = 0; DEBUG(SSSDBG_TRACE_ALL, "Receiving info for the user\n"); @@ -2871,54 +2867,22 @@ static void sdap_get_initgr_user(struct tevent_req *subreq) tevent_req_error(req, ret); return; } + } else if (count == 1) { + state->orig_user = usr_attrs[0]; } else if (count != 1) { DEBUG(SSSDBG_OP_FAILURE, "Expected one user entry and got %zu\n", count); - ret = domain_to_basedn(state, state->dom->name, &expected_basedn); + ret = sysdb_try_to_find_expected_dn(state->dom, "dc", usr_attrs, count, + &state->orig_user); if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, "domain_to_basedn failed.\n"); - tevent_req_error(req, ret); - return; - } - expected_basedn = talloc_asprintf(state, "%s%s", - "cn=users,", expected_basedn); - if (expected_basedn == NULL) { - DEBUG(SSSDBG_OP_FAILURE, "talloc_append failed.\n"); - tevent_req_error(req, ENOMEM); - return; - } - - DEBUG(SSSDBG_TRACE_ALL, "Expected BaseDN is [%s].\n", expected_basedn); - expected_basedn_len = strlen(expected_basedn); - - for (c = 0; c < count; c++) { - ret = sysdb_attrs_get_string(usr_attrs[c], SYSDB_ORIG_DN, &orig_dn); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_string failed.\n"); - tevent_req_error(req, ret); - return; - } - dn_len = strlen(orig_dn); - - if (dn_len > expected_basedn_len - && strcasecmp(orig_dn + (dn_len - expected_basedn_len), - expected_basedn) == 0) { - DEBUG(SSSDBG_TRACE_ALL, - "Found matching dn [%s].\n", orig_dn); - break; - } - } - - if (c == count) { - DEBUG(SSSDBG_OP_FAILURE, "No matching DN found.\n"); + DEBUG(SSSDBG_OP_FAILURE, + "try_to_find_expected_dn failed. No matching DN found.\n"); tevent_req_error(req, EINVAL); return; } } - state->orig_user = usr_attrs[c]; - ret = sysdb_transaction_start(state->sysdb); if (ret) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to start transaction\n"); diff --git a/src/tests/cmocka/test_sysdb_subdomains.c b/src/tests/cmocka/test_sysdb_subdomains.c index 701bfb726ff7e950d4439b3dc1a3bee437c9e7ed..f55c2918015900351483e3471bf946ea60872dae 100644 --- a/src/tests/cmocka/test_sysdb_subdomains.c +++ b/src/tests/cmocka/test_sysdb_subdomains.c @@ -509,6 +509,76 @@ static void test_sysdb_link_ad_multidom(void **state) } +static void test_try_to_find_expected_dn(void **state) +{ + int ret; + struct sysdb_attrs *result; + struct sysdb_attrs *usr_attrs[10] = { NULL }; + struct sss_domain_info *dom; + struct subdom_test_ctx *test_ctx = + talloc_get_type(*state, struct subdom_test_ctx); + + dom = find_domain_by_name(test_ctx->tctx->dom, + "child2.test_sysdb_subdomains_2", true); + assert_non_null(dom); + + usr_attrs[0] = sysdb_new_attrs(test_ctx); + assert_non_null(usr_attrs[0]); + + ret = sysdb_attrs_add_string(usr_attrs[0], SYSDB_ORIG_DN, + "uid=user,cn=abc,dc=c2,dc=child2,dc=test_sysdb_subdomains_2"); + assert_int_equal(ret, EOK); + + ret = sysdb_try_to_find_expected_dn(NULL, NULL, NULL, 0, NULL); + assert_int_equal(ret, EINVAL); + + ret = sysdb_try_to_find_expected_dn(dom, "dc", usr_attrs, 1, &result); + assert_int_equal(ret, ENOENT); + + ret = sysdb_try_to_find_expected_dn(dom, "xy", usr_attrs, 1, &result); + assert_int_equal(ret, EOK); + assert_ptr_equal(result, usr_attrs[0]); + + usr_attrs[1] = sysdb_new_attrs(test_ctx); + assert_non_null(usr_attrs[1]); + + ret = sysdb_attrs_add_string(usr_attrs[1], SYSDB_ORIG_DN, + "uid=user1,cn=abc,dc=child2,dc=test_sysdb_subdomains_2"); + assert_int_equal(ret, EOK); + + usr_attrs[2] = sysdb_new_attrs(test_ctx); + assert_non_null(usr_attrs[2]); + + ret = sysdb_attrs_add_string(usr_attrs[2], SYSDB_ORIG_DN, + "uid=user2,cn=abc,dc=c2,dc=child2,dc=test_sysdb_subdomains_2"); + assert_int_equal(ret, EOK); + + ret = sysdb_try_to_find_expected_dn(dom, "dc", usr_attrs, 3, &result); + assert_int_equal(ret, EOK); + assert_ptr_equal(result, usr_attrs[1]); + + ret = sysdb_try_to_find_expected_dn(dom, "xy", usr_attrs, 3, &result); + assert_int_equal(ret, EINVAL); + + /* Make sure cn=users match is preferred */ + talloc_free(usr_attrs[2]); + usr_attrs[2] = sysdb_new_attrs(test_ctx); + assert_non_null(usr_attrs[2]); + + ret = sysdb_attrs_add_string(usr_attrs[2], SYSDB_ORIG_DN, + "uid=user2,cn=abc,cn=users,dc=child2,dc=test_sysdb_subdomains_2"); + assert_int_equal(ret, EOK); + + ret = sysdb_try_to_find_expected_dn(dom, "dc", usr_attrs, 3, &result); + assert_int_equal(ret, EOK); + assert_ptr_equal(result, usr_attrs[2]); + + + talloc_free(usr_attrs[0]); + talloc_free(usr_attrs[1]); + talloc_free(usr_attrs[2]); +} + int main(int argc, const char *argv[]) { int rv; @@ -542,6 +612,9 @@ int main(int argc, const char *argv[]) cmocka_unit_test_setup_teardown(test_sysdb_link_ad_multidom, test_sysdb_subdom_setup, test_sysdb_subdom_teardown), + cmocka_unit_test_setup_teardown(test_try_to_find_expected_dn, + test_sysdb_subdom_setup, + test_sysdb_subdom_teardown), }; /* Set debug level to invalid value so we can deside if -d 0 was used. */ -- 2.1.0
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
