On (13/04/16 12:43), Sumit Bose wrote: >On Wed, Apr 13, 2016 at 10:39:44AM +0200, Lukas Slebodnik wrote: >> On (12/04/16 11:03), Sumit Bose wrote: >> >Hi, >> > >> >I'm working on adding certificates to overrides and came across some >> >issues with the local overrides. The main cause was that instead you >> >searching the original object in the cache, the DN of the object was >> >constructed. This failed for users form sub-domain which have the >> >fully-qualified name in the DN and not only the short name. Additionally >> >users with mixed-case names where not found in case-insensitive domains >> >which can e.g. with the AD provider and the Administrator user from AD. >> > >> >The first patch contains a new integration test which tests the >> >mixed-case user name. Unfortunately it is currently not possible to test >> >the sub-domain case. >> > >> >The second patch replaces the DN based lookup with a proper search. Imo >> >calls like sysdb_user_dn() to construct a DN should only be used when >> >adding new entries but newer to look up entries. Here always proper >> >searches should be used. And as long as the search uses index attributes >> >it will not be much more expensive than the DN based lookup because >> >afaik the ldb index tables are kept in memory. >> > >> >The third patch makes sure that the tools are aware of the flat name and >> >the SID of the master domain if this information is available in the >> >cache. This might currently not be needed explicitly but helps to avoid >> >some irritating debug messages about a missing flat name when generating >> >fully qualified names. >> > >> >Finally the last patch fixes the output of sub-domain users which >> >already have a fully-qualified name where no additional domain component >> >must be added. >> > >> >bye, >> >Sumit >> >> >From 4065b421e725118f3832d6f8ae71808aa57887c5 Mon Sep 17 00:00:00 2001 >> >From: Sumit Bose <[email protected]> >> >Date: Mon, 11 Apr 2016 14:25:18 +0200 >> >Subject: [PATCH 1/4] intg: local override for user with mixed case name >> > >> >Test for users with fully-qualified and mixed-cased names are added. >> >--- >> > src/tests/intg/ldap_local_override_test.py | 66 >> > +++++++++++++++++++++++++++++- >> > 1 file changed, 65 insertions(+), 1 deletion(-) >> > >> >diff --git a/src/tests/intg/ldap_local_override_test.py >> >b/src/tests/intg/ldap_local_override_test.py >> >index >> >d5f58fc7a5207413df70b2f103887c0a72a47aa7..542527180cf2c7ec5a2984f6b42655fb0c75f5d1 >> > 100644 >> >--- a/src/tests/intg/ldap_local_override_test.py >> >+++ b/src/tests/intg/ldap_local_override_test.py >> >@@ -139,7 +139,8 @@ def create_sssd_fixture(request): >> > OVERRIDE_FILENAME = "export_file" >> > >> > >> >-def prepare_sssd(request, ldap_conn, use_fully_qualified_names=False): >> >+def prepare_sssd(request, ldap_conn, use_fully_qualified_names=False, >> >+ case_sensitive=True): >> > """Prepare SSSD with defaults""" >> > conf = unindent("""\ >> > [sssd] >> >@@ -158,6 +159,7 @@ def prepare_sssd(request, ldap_conn, >> >use_fully_qualified_names=False): >> > ldap_uri = {ldap_conn.ds_inst.ldap_url} >> > ldap_search_base = {ldap_conn.ds_inst.base_dn} >> > use_fully_qualified_names = {use_fully_qualified_names} >> >+ case_sensitive = {case_sensitive} >> > """).format(**locals()) >> > create_conf_fixture(request, conf) >> > create_sssd_fixture(request) >> >@@ -951,3 +953,65 @@ def test_regr_2790_override(ldap_conn, >> >env_regr_2790_override): >> > assert res == sssd_id.NssReturnCode.SUCCESS, \ >> > "Could not find groups for user2 %d" % errno >> > assert sorted(grp_list) == sorted(["group1", "group2"]) >> >+ >> >+# Test fully qualified and case-insensitive names >> >+ >> >[email protected] >> >+def env_mix_cased_name_override(request, ldap_conn): >> >+ """Setup test for mixed case names""" >> >+ >> >+ prepare_sssd(request, ldap_conn, True, False) >> >+ >> >+ # Add entries >> >+ ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >> >+ ent_list.add_user("user1", 10001, 20001) >> >+ ent_list.add_user("uSeR2", 10002, 20002) >> >+ >> >+ create_ldap_fixture(request, ldap_conn, ent_list) >> >+ >> >+ >> >+ pwd.getpwnam('user1@LDAP') >> >+ pwd.getpwnam('user2@LDAP') >> >+ with pytest.raises(KeyError): >> >+ pwd.getpwnam('ov_user1@LDAP') >> >+ with pytest.raises(KeyError): >> >+ pwd.getpwnam('ov_user2@LDAP') >> >+ >> >+ # Override >> >+ subprocess.check_call(["sss_override", "user-add", "user1@LDAP", >> >+ "-u", "10010", >> >+ "-g", "20010", >> >+ "-n", "ov_user1", >> >+ "-c", "Overriden User 1", >> >+ "-h", "/home/ov/user1", >> >+ "-s", "/bin/ov_user1_shell"]) >> >+ >> >+ subprocess.check_call(["sss_override", "user-add", "user2@LDAP", >> >+ "-u", "10020", >> >+ "-g", "20020", >> >+ "-n", "ov_user2", >> >+ "-c", "Overriden User 2", >> >+ "-h", "/home/ov/user2", >> >+ "-s", "/bin/ov_user2_shell"]) >> >+ >> >+ restart_sssd() >> >+ >> >+def test_mix_cased_name_override(ldap_conn, env_mix_cased_name_override): >> >+ """Test if names with upper and lower case letter are overridden""" >> >+ >> >+ # Assert entries are overridden >> >+ user1 = dict(name='ov_user1@LDAP', passwd='*', uid=10010, gid=20010, >> >+ gecos='Overriden User 1', >> >+ dir='/home/ov/user1', >> >+ shell='/bin/ov_user1_shell') >> >+ >> >+ user2 = dict(name='ov_user2@LDAP', passwd='*', uid=10020, gid=20020, >> >+ gecos='Overriden User 2', >> >+ dir='/home/ov/user2', >> >+ shell='/bin/ov_user2_shell') >> >+ >> >+ ent.assert_passwd_by_name('user1@LDAP', user1) >> >+ ent.assert_passwd_by_name('ov_user1@LDAP', user1) >> >+ >> >+ ent.assert_passwd_by_name('user2@LDAP', user2) >> >+ ent.assert_passwd_by_name('ov_user2@LDAP', user2) >> >-- >> >2.1.0 >> > >> >> >From 4574149abea319b41cb6178fb5490339dc550527 Mon Sep 17 00:00:00 2001 >> >From: Sumit Bose <[email protected]> >> >Date: Fri, 8 Apr 2016 14:43:22 +0200 >> >Subject: [PATCH 2/4] sss_override: do not generate DN, search object >> > >> >DNs of existing objects can not be generate reliable because the use of >> >fully qualified names and upper and lower cases in names has to be >> >considered. The most reliable way to get the DN is to search the object >> >and take the DN from the result. >> >--- >> > src/tools/sss_override.c | 34 +++++++++++++++++++++++++++------- >> > 1 file changed, 27 insertions(+), 7 deletions(-) >> > >> >diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c >> >index >> >3eb1191959653c6e4ad9b981c045956572fdc556..c8d3e55c11a19a21dea723dd634103eda5901f60 >> > 100644 >> >--- a/src/tools/sss_override.c >> >+++ b/src/tools/sss_override.c >> >@@ -584,6 +584,7 @@ static errno_t get_object_dn(TALLOC_CTX *mem_ctx, >> > struct ldb_dn *ldb_dn; >> > const char *str_dn; >> > errno_t ret; >> >+ struct ldb_result *res; >> > >> > tmp_ctx = talloc_new(NULL); >> > if (tmp_ctx == NULL) { >> >@@ -593,17 +594,36 @@ static errno_t get_object_dn(TALLOC_CTX *mem_ctx, >> > >> > switch (type) { >> > case SYSDB_MEMBER_USER: >> >- ldb_dn = sysdb_user_dn(tmp_ctx, domain, name); >> >- break; >> >+ ret = sysdb_getpwnam(tmp_ctx, domain, name, &res); >> >+ break; >> > case SYSDB_MEMBER_GROUP: >> >- ldb_dn = sysdb_group_dn(tmp_ctx, domain, name); >> >- break; >> >+ ret = sysdb_getgrnam(tmp_ctx, domain, name, &res); >> >+ break; >> > default: >> >- DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type); >> >- ret = ERR_INTERNAL; >> >- goto done; >> >+ DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type); >> >+ ret = ERR_INTERNAL; >> >+ goto done; >> > } >> > >> >+ if (ret != EOK) { >> >+ DEBUG(SSSDBG_CRIT_FAILURE, >> >+ "Failed to look up original object in cache.\n"); >> >+ goto done; >> >+ } >> >+ >> >+ if (res->count == 0) { >> >+ DEBUG(SSSDBG_MINOR_FAILURE, "Original object not found in >> >cache.\n"); >> >+ ret = ENOENT; >> >+ goto done; >> >+ } else if (res->count > 1) { >> >+ DEBUG(SSSDBG_CRIT_FAILURE, >> >+ "There are multiple object with name [%s] in the cache.\n", >> >name); >> >+ ret = EINVAL; >> >+ goto done; >> >+ } >> >+ >> >+ ldb_dn = res->msgs[0]->dn; >> >+ >> > if (ldb_dn == NULL) { >> > ret = ENOMEM; >> > goto done; >> >-- >> >2.1.0 >> > >> >> >From 08bcb4120c340078ec560d95f6ace7bd09bd8012 Mon Sep 17 00:00:00 2001 >> >From: Sumit Bose <[email protected]> >> >Date: Fri, 8 Apr 2016 18:40:48 +0200 >> >Subject: [PATCH 3/4] tools: read additional data of the master domain >> > >> >--- >> > src/tools/common/sss_tools.c | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> > >> >diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c >> >index >> >d3073a2e534fefa7a82a2e5c162eb22e1d433bc1..b916f772b7333e23119aa417a74a6897ba1f8e4d >> > 100644 >> >--- a/src/tools/common/sss_tools.c >> >+++ b/src/tools/common/sss_tools.c >> >@@ -137,6 +137,14 @@ static errno_t sss_tool_domains_init(TALLOC_CTX >> >*mem_ctx, >> > for (dom = domains; dom != NULL; >> > dom = get_next_domain(dom, SSS_GND_DESCEND)) { >> > if (!IS_SUBDOMAIN(dom)) { >> >+ /* Get flat name and domain ID (SID) from the cache >> >+ * if available */ >> >+ ret = sysdb_master_domain_update(dom); >> >+ if (ret != EOK) { >> >+ DEBUG(SSSDBG_MINOR_FAILURE, "Failed to update domain >> >%s.\n", >> >+ dom->name); >> >+ } >> >+ >> > /* Update list of subdomains for this domain */ >> > ret = sysdb_update_subdomains(dom); >> > if (ret != EOK) { >> >-- >> >2.1.0 >> > >> >> >From 8e5d0bec231b85af75566a6f6fc13bae1623440d Mon Sep 17 00:00:00 2001 >> >From: Sumit Bose <[email protected]> >> >Date: Fri, 8 Apr 2016 18:43:57 +0200 >> >Subject: [PATCH 4/4] sss_override: only add domain if name is not fully >> > qualified >> > >> >--- >> > src/tools/sss_override.c | 28 +++++++++++++++++++++++++++- >> > 1 file changed, 27 insertions(+), 1 deletion(-) >> > >> >diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c >> >index >> >c8d3e55c11a19a21dea723dd634103eda5901f60..7e63bdf6ecb3bb968f631e79f691942237f61793 >> > 100644 >> >--- a/src/tools/sss_override.c >> >+++ b/src/tools/sss_override.c >> >@@ -379,11 +379,37 @@ static char *get_fqname(TALLOC_CTX *mem_ctx, >> > char *fqname; >> > int fqlen; >> > int check; >> >+ char *dummy_domain = NULL; >> >+ int ret; >> > >> >- if (domain == NULL) { >> >+ if (domain == NULL || domain->names == NULL) { >> > return NULL; >> > } >> > >> >+ /* check if the name already contains domain part */ >> >+ ret = sss_parse_name(mem_ctx, domain->names, name, &dummy_domain, >> >NULL); >> >+ if (ret == ERR_REGEX_NOMATCH) { >> >+ DEBUG(SSSDBG_TRACE_FUNC, >> >+ "sss_parse_name could not parse domain from [%s]. " >> >+ "Assuming it is not FQDN.\n", name); >> >+ } else if (ret != EOK) { >> >+ DEBUG(SSSDBG_TRACE_FUNC, >> >+ "sss_parse_name failed [%d]: %s\n", ret, sss_strerror(ret)); >> >+ return NULL; >> >+ } >> >+ >> >+ if (dummy_domain != NULL) { >> >+ talloc_free(dummy_domain); >> >+ DEBUG(SSSDBG_TRACE_FUNC, "Name is already fully qualified.\n"); >> >+ fqname = talloc_strdup(mem_ctx, name); >> >+ if (fqname == NULL) { >> >+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed.\n"); >> >+ return NULL; >> >+ } >> >+ >> >+ return fqname; >> >+ } >> I think it would be better to check ret for EOK >> rather then checking dummy_domain. The variable dummy_domain >> will be everytime set if ret is EOK. > >Are you sure, I thought if the name does not contain a domain part >sss_parse_name() returns EOK and set the returned domain name to NULL? >At least this is how I understand the parse_name_plain() name test. > Ahh, you are right. I was looking into unit tests for sss_parse_name and I didn't notice different re_expression. sss_override should work with any regex.
ACK http://sssd-ci.duckdns.org/logs/job/41/16/summary.html LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
