On (17/08/16 15:39), Lukas Slebodnik wrote: >On (16/08/16 15:22), Lukas Slebodnik wrote: >>On (16/08/16 11:50), Lukas Slebodnik wrote: >>>On (12/08/16 16:30), Lukas Slebodnik wrote: >>>>On (12/08/16 16:14), Jakub Hrozek wrote: >>>>>On Fri, Aug 12, 2016 at 04:05:22PM +0200, Lukas Slebodnik wrote: >>>>>> On (10/08/16 20:59), Michal Židek wrote: >>>>>> >On 08/10/2016 08:36 PM, Lukas Slebodnik wrote: >>>>>> >> On (10/08/16 17:41), Michal Židek wrote: >>>>>> >> > Hi, >>>>>> >> > >>>>>> >> > see the attached patch. >>>>>> >> > >>>>>> >> > I modified the detection of duplicates when >>>>>> >> > extending the maps (sysdb_attr:ldap_attr). >>>>>> >> > >>>>>> >> > When we try to add entry to the map >>>>>> >> > that already exists in the map, then >>>>>> >> > without this patch we will fail. >>>>>> >> > >>>>>> >> > With this patch, we only fail if the >>>>>> >> > newly added extension would redefine >>>>>> >> > already existing entry in the map. >>>>>> >> > >>>>>> >> > Otherwise it is just skipped without >>>>>> >> > a failure (we just skip adding what >>>>>> >> > is already there). >>>>>> >> > >>>>>> >> > I created simple CI test for this (first >>>>>> >> > patch). >>>>>> >> > >>>>>> >> > Michal >>>>>> >> >>>>>> >> > From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 >>>>>> >> > 2001 >>>>>> >> > From: =?UTF-8?q?Michal=20=C5=BDidek?= <[email protected]> >>>>>> >> > Date: Wed, 10 Aug 2016 15:41:34 +0200 >>>>>> >> > Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps >>>>>> >> > >>>>>> >> > When extending map with entry that already >>>>>> >> > exists in the map in the exacty same form, >>>>>> >> > then there is no need to fail. >>>>>> >> > >>>>>> >> > We should only fail if we try to >>>>>> >> > change purpose of already used sysdb >>>>>> >> > attribute. >>>>>> >> > >>>>>> >> > Resolves: >>>>>> >> > https://fedorahosted.org/sssd/ticket/3120 >>>>>> >> > --- >>>>>> >> > src/providers/ldap/sdap.c | 41 >>>>>> >> > +++++++++++++++++++++++++++++++++++------ >>>>>> >> > 1 file changed, 35 insertions(+), 6 deletions(-) >>>>>> >> > >>>>>> >> > diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c >>>>>> >> > index 97b8f12..e1cf70f 100644 >>>>>> >> > --- a/src/providers/ldap/sdap.c >>>>>> >> > +++ b/src/providers/ldap/sdap.c >>>>>> >> > @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX >>>>>> >> > *mem_ctx, >>>>>> >> > return EOK; >>>>>> >> > } >>>>>> >> > >>>>>> >> > -static bool is_sysdb_duplicate(struct sdap_attr_map *map, >>>>>> >> > - int num_entries, >>>>>> >> > - const char *sysdb_attr) >>>>>> >> > +/* _already_in_map is set to true if the attribute >>>>>> >> > + * already exists in the map and is used for the same >>>>>> >> > + * LDAP attribute. >>>>>> >> > + * >>>>>> >> > + * _conflicts_with_map is set to true if the attribute >>>>>> >> > + * already exists in map, but is used for different >>>>>> >> > + * LDAP attribute. >>>>>> >> > + * */ >>>>>> >> > +static void check_duplicate(struct sdap_attr_map *map, >>>>>> >> > + int num_entries, >>>>>> >> > + const char *sysdb_attr, >>>>>> >> > + const char *ldap_attr, >>>>>> >> > + bool *_already_in_map, >>>>>> >> > + bool *_conflicts_with_map) >>>>>> >> > { >>>>>> >> This function has 3 output boolean argumets: >>>>>> >> It would be better to return enum instead of >>>>>> >> adding new parametrs. >>>>>> >> >>>>>> >> LS >>>>>> > >>>>>> >Ok, attached is version with enum. >>>>>> > >>>>>> >Michal >>>>>> > >>>>>> >>>>>> I tried to rest use-case from ticket #3120 >>>>>> http://www.freeipa.org/page/Web_App_Authentication/Example_setup >>>>>> >>>>>> but sssd_be crashed >>>>>> (gdb) bt >>>>>> #0 0x00007fc29afb8961 in __strncasecmp_l_avx () from /lib64/libc.so.6 >>>>>> #1 0x00007fc29f199ea0 in sysdb_attrs_get_el_ext >>>>>> (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, >>>>>> alloc=alloc@entry=true, el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:290 >>>>>> #2 0x00007fc29f199fad in sysdb_attrs_get_el >>>>>> (attrs=attrs@entry=0x7fc2a1adc740, name=name@entry=0x0, >>>>>> el=el@entry=0x7ffd0d466810) at src/db/sysdb.c:323 >>>>>> #3 0x00007fc28fe41400 in sdap_attrs_add_ldap_attr >>>>>> (ldap_attrs=ldap_attrs@entry=0x7fc2a1adc740, attr_name=0x0, >>>>>> attr_desc=attr_desc@entry=0x0, multivalued=multivalued@entry=true, >>>>>> name=<optimized out>, attrs=attrs@entry=0x7fc2a1ac4860) at >>>>>> src/providers/ldap/sdap_utils.c:40 >>>>>> #4 0x00007fc28fe1a2c7 in sdap_save_user >>>>>> (memctx=memctx@entry=0x7fc2a1adf600, opts=0x7fc2a1a7eae0, >>>>>> dom=0x7fc2a1a54ae0, attrs=<optimized out>, >>>>>> _usn_value=_usn_value@entry=0x0, >>>>>> now=now@entry=0) at src/providers/ldap/sdap_async_users.c:482 >>>>>> #5 0x00007fc28fe2b667 in sdap_get_initgr_user (subreq=0x0) at >>>>>> src/providers/ldap/sdap_async_initgroups.c:2961 >>>>>> #6 0x00007fc28fe13d99 in generic_ext_search_handler (subreq=0x0, >>>>>> opts=<optimized out>) at src/providers/ldap/sdap_async.c:1688 >>>>>> #7 0x00007fc28fe16407 in sdap_get_generic_op_finished (op=<optimized >>>>>> out>, reply=<optimized out>, error=<optimized out>, pvt=<optimized out>) >>>>>> at src/providers/ldap/sdap_async.c:1578 >>>>>> #8 0x00007fc28fe14ded in sdap_process_message (ev=<optimized out>, >>>>>> sh=<optimized out>, msg=0x7fc2a1aba9f0) at >>>>>> src/providers/ldap/sdap_async.c:353 >>>>>> #9 sdap_process_result (ev=<optimized out>, pvt=<optimized out>) at >>>>>> src/providers/ldap/sdap_async.c:197 >>>>>> #10 0x00007fc29b85fb4f in tevent_common_loop_timer_delay () from >>>>>> /lib64/libtevent.so.0 >>>>>> #11 0x00007fc29b860b5a in epoll_event_loop_once () from >>>>>> /lib64/libtevent.so.0 >>>>>> #12 0x00007fc29b85f257 in std_event_context_init () from >>>>>> /lib64/libtevent.so.0 >>>>>> #13 0x00007fc29b85b40d in _tevent_loop_until () from >>>>>> /lib64/libtevent.so.0 >>>>>> #14 0x00007fc2a1a4bd20 in ?? () >>>>>> #15 0x00007fc29f1e7c47 in ?? () from /usr/lib64/sssd/libsss_util.so >>>>>> #16 0x00007fc29b85f1f7 in std_event_loop_once () from >>>>>> /lib64/libtevent.so.0 >>>>>> #17 0x00007fc29f1cb7f3 in server_loop (main_ctx=0x7fc2a1a4d080) at >>>>>> src/util/server.c:702 >>>>>> #18 0x00007fc29fa45952 in main (argc=8, argv=<optimized out>) at >>>>>> src/providers/data_provider_be.c:587 >>>>>> >>>>>> >>>>>> it crashed because one agruments from strcasecmp was NULL >>>>>> (dereference of NULL pointer) >>>>>> >>>>>> I guess that we hit the last value in user_map (zeroed structure) >>>>>> In other words, opts->user_map_cnt does not match reallity. >>>>>> >>>>>> (gdb) l 482 >>>>>> 477 } >>>>>> 478 } >>>>>> 479 } >>>>>> 480 >>>>>> 481 for (i = SDAP_FIRST_EXTRA_USER_AT; i < opts->user_map_cnt; >>>>>> i++) { >>>>>> 482 ret = sdap_attrs_add_list(attrs, >>>>>> opts->user_map[i].sys_name, >>>>>> 483 NULL, user_name, user_attrs); >>>>>> 484 if (ret) { >>>>>> 485 goto done; >>>>>> 486 } >>>>>> >>>>>> NACK >>>>> >>>>>Do you think you can fix the patch with additional one given that this is >>>>>a pretty bad regression and Michal is out for a couple of weeks? >>>>I will try to look >>>> >>>Here you are >>> >>>LS >> >>>From be20aed9f7d49714543237d27851a81821798e6d Mon Sep 17 00:00:00 2001 >>>From: =?UTF-8?q?Michal=20=C5=BDidek?= <[email protected]> >>>Date: Wed, 10 Aug 2016 15:41:34 +0200 >>>Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps >>> >>>When extending map with entry that already >>>exists in the map in the exacty same form, >>>then there is no need to fail. >>> >>>We should only fail if we try to >>>change purpose of already used sysdb >>>attribute. >>> >>>Resolves: >>>https://fedorahosted.org/sssd/ticket/3120 >>> >>>Signed-off-by: Lukas Slebodnik <[email protected]> >>>--- >>> src/providers/ldap/sdap.c | 30 ++++++++++++++++++++++++------ >>> 1 file changed, 24 insertions(+), 6 deletions(-) >>> >>>diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c >>>index >>>97b8f126d4ed6bc59c510d5763789a458bd4863a..0c36e376f58bdcf541425f31e90fdd2e388b43cd >>> 100644 >>>--- a/src/providers/ldap/sdap.c >>>+++ b/src/providers/ldap/sdap.c >>>@@ -122,19 +122,30 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, >>> return EOK; >>> } >>> >>>-static bool is_sysdb_duplicate(struct sdap_attr_map *map, >>>- int num_entries, >>>- const char *sysdb_attr) >>>+enum duplicate_t { >>>+ NOT_FOUND = 0, >>>+ ALREADY_IN_MAP, /* nothing to add */ >>>+ CONFLICT_WITH_MAP /* attempt to redefine attribute */ >>>+}; >>>+ >>>+static enum duplicate_t check_duplicate(struct sdap_attr_map *map, >>>+ int num_entries, >>>+ const char *sysdb_attr, >>>+ const char *ldap_attr) >>> { >>> int i; >>> >>> for (i = 0; i < num_entries; i++) { >>> if (strcmp(map[i].sys_name, sysdb_attr) == 0) { >>>- return true; >>>+ if (strcmp(map[i].name, ldap_attr) == 0) { >>>+ return ALREADY_IN_MAP; >>>+ } else { >>>+ return CONFLICT_WITH_MAP; >>>+ } >>> } >>> } >>> >>>- return false; >>>+ return NOT_FOUND; >>> } >>> >>> int sdap_extend_map(TALLOC_CTX *memctx, >>>@@ -174,7 +185,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, >>> continue; >>> } >>> >>>- if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) { >>>+ ret = check_duplicate(map, num_entries, sysdb_attr, ldap_attr); >>>+ if (ret == ALREADY_IN_MAP) { >>>+ DEBUG(SSSDBG_TRACE_FUNC, >>>+ "Attribute %s (%s in LDAP) is already in map.\n", >>>+ sysdb_attr, ldap_attr); >>>+ --nextra; >>Hmm, it crahsed even after adding this line. >> >>Self-NACK >> >BTW the crash was caused by empty entry in map which was caused by >skipped existing mapping. > >If you do not like interrrating over array with pointers >then I can use two indeces. > >Updated patch is attched. > and one more time with modified test. The test also check values of extra attributes in cache
The integration tests depens on patches from thread "intg: test nested membership" LS
>From d221c60070d310d82fd5753bab5c14ec0ed64c93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <[email protected]> Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 1/2] sdap: Skip exact duplicates when extending maps When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail. We should only fail if we try to change purpose of already used sysdb attribute. Resolves: https://fedorahosted.org/sssd/ticket/3120 Signed-off-by: Lukas Slebodnik <[email protected]> --- src/providers/ldap/sdap.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f126d4ed6bc59c510d5763789a458bd4863a..59530664bed4cc62a121037e37573e963a4703f3 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,30 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; } -static bool is_sysdb_duplicate(struct sdap_attr_map *map, - int num_entries, - const char *sysdb_attr) +enum duplicate_t { + NOT_FOUND = 0, + ALREADY_IN_MAP, /* nothing to add */ + CONFLICT_WITH_MAP /* attempt to redefine attribute */ +}; + +static enum duplicate_t check_duplicate(struct sdap_attr_map *map, + int num_entries, + const char *sysdb_attr, + const char *ldap_attr) { int i; for (i = 0; i < num_entries; i++) { if (strcmp(map[i].sys_name, sysdb_attr) == 0) { - return true; + if (strcmp(map[i].name, ldap_attr) == 0) { + return ALREADY_IN_MAP; + } else { + return CONFLICT_WITH_MAP; + } } } - return false; + return NOT_FOUND; } int sdap_extend_map(TALLOC_CTX *memctx, @@ -167,14 +178,20 @@ int sdap_extend_map(TALLOC_CTX *memctx, return ENOMEM; } - for (i = 0; extra_attrs[i]; i++) { - ret = split_extra_attr(map, extra_attrs[i], &sysdb_attr, &ldap_attr); + for (i = 0; *extra_attrs != NULL; extra_attrs++) { + ret = split_extra_attr(map, *extra_attrs, &sysdb_attr, &ldap_attr); if (ret != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, "Cannot split %s\n", extra_attrs[i]); + DEBUG(SSSDBG_MINOR_FAILURE, "Cannot split %s\n", *extra_attrs); continue; } - if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) { + ret = check_duplicate(map, num_entries, sysdb_attr, ldap_attr); + if (ret == ALREADY_IN_MAP) { + DEBUG(SSSDBG_TRACE_FUNC, + "Attribute %s (%s in LDAP) is already in map.\n", + sysdb_attr, ldap_attr); + continue; + } else if (ret == CONFLICT_WITH_MAP) { DEBUG(SSSDBG_FATAL_FAILURE, "Attribute %s (%s in LDAP) is already used by SSSD, please " "choose a different cache name\n", sysdb_attr, ldap_attr); @@ -193,9 +210,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, map[num_entries+i].def_name == NULL) { return ENOMEM; } - DEBUG(SSSDBG_TRACE_FUNC, "Extending map with %s\n", extra_attrs[i]); + DEBUG(SSSDBG_TRACE_FUNC, "Extending map with %s\n", *extra_attrs); + + /* index must be incremented only for appended entry. */ + ++i; } + nextra = i; + /* Sentinel */ memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map)); -- 2.9.3
>From 4d949fa04599f84d822160197b8b9d0399307a1b Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik <[email protected]> Date: Tue, 16 Aug 2016 11:42:08 +0200 Subject: [PATCH 2/2] intg: Test extra attributes duplicate Regresion test for ticket #3120 Resolves: https://fedorahosted.org/sssd/ticket/3120 --- src/tests/intg/test_ldap.py | 50 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/tests/intg/test_ldap.py b/src/tests/intg/test_ldap.py index c40ce09579755d8e952e990283e93284811f1020..b8fef7afc3658c1985694da6a219d4bb1c608a78 100644 --- a/src/tests/intg/test_ldap.py +++ b/src/tests/intg/test_ldap.py @@ -24,6 +24,7 @@ import signal import subprocess import time import ldap +import ldap.modlist import pytest import config @@ -31,6 +32,7 @@ import ds_openldap import ent import ldap_ent import sssd_id +import sssd_ldb from util import unindent LDAP_BASE_DN = "dc=example,dc=com" @@ -734,3 +736,51 @@ def test_special_characters_in_names(ldap_conn, sanity_rfc2307): "group(_u)ser1", dict(name="group(_u)ser1", passwd="*", gid=5001, mem=ent.contains_only("t(u)ser"))) + + [email protected] +def extra_attributes(request, ldap_conn): + ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) + ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) + ent_list.add_user("user", 2001, 2000) + ent_list.add_group("group", 2000) + create_ldap_fixture(request, ldap_conn, ent_list) + conf = \ + format_basic_conf(ldap_conn, SCHEMA_RFC2307) + \ + unindent("""\ + [domain/LDAP] + ldap_user_extra_attrs = mail, name:uid, givenName + """).format(**locals()) + create_conf_fixture(request, conf) + create_sssd_fixture(request) + + +def test_extra_attribute_already_exists(ldap_conn, extra_attributes): + """Test the effect of the "vetoed_shells" option""" + + user = 'user' + extra_attribute = 'givenName' + given_name = 'unix_user' + + user_dn = "uid=" + user + ",ou=Users," + ldap_conn.ds_inst.base_dn + + old = {'objectClass': ['top', 'inetOrgPerson', 'posixAccount']} + new = {'objectClass': ['top', 'inetOrgPerson', 'posixAccount', + 'extensibleObject']} + ldif = ldap.modlist.modifyModlist(old, new) + + ldap_conn.modify_s(user_dn, ldif) + ldap_conn.modify_s(user_dn, [(ldap.MOD_ADD, extra_attribute, given_name)]) + + ent.assert_passwd_by_name( + user, + dict(name="user", uid=2001, gid=2000, shell="/bin/bash"), + ) + + domain = 'LDAP' + ldb_conn = sssd_ldb.SssdLdb('LDAP') + val = ldb_conn.get_entry_attr(sssd_ldb.CacheType.sysdb, + sssd_ldb.TsCacheEntry.user, + user, domain, extra_attribute) + + assert val == given_name -- 2.9.3
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
