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.
bye,
Sumit
>
> and error handling after sss_parse_name will be simpler
> if (ret == EOK) {
>
> } else if (ret == ERR_REGEX_NOMATCH) {
>
> } else {
>
> }
>
> or you can use switch/case depends what do you prefer.
>
> Other wise ACK.
> Integration test passed but we will need to change
> order of patches. Firstly apply patch which fix issues
> and then appply patch with test.
>
> But at least, we can see that patch realy fixes bug :-)
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [email protected]
> https://lists.fedorahosted.org/admin/lists/[email protected]
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
