[SSSD] [PATCH] LDAP: Do not impose sizelimit=1 for single-entry searches

Lukas Slebodnik lslebodn at redhat.com
Mon Sep 21 17:46:55 UTC 2015


On (21/09/15 16:50), Jakub Hrozek wrote:
>On Mon, Sep 21, 2015 at 03:29:20PM +0200, Lukas Slebodnik wrote:
>> On (17/09/15 09:38), Jakub Hrozek wrote:
>> >On Thu, Sep 17, 2015 at 09:35:11AM +0200, Jakub Hrozek wrote:
>> >> > After just only a month since the nack, attached are updated patches. I
>> >> > would prefer to only apply the first one to downstream, since the others
>> >> > are just optimizations and tests.
>> >> 
>> >> CI runs revealed the tests didn't link correctly on Debian. New patches
>> >> are attached.
>> >
>> >Ugh, those were from a wrong branch. Sorry about the spam.
>> 
>> Attached patches fixed the issue and just one user was stored to
>> sysdb cache
>
>> >From e07541b9b61778fcd3377388fe105de15898cb16 Mon Sep 17 00:00:00 2001
>> >From: Jakub Hrozek <jhrozek at redhat.com>
>> >Date: Wed, 16 Sep 2015 15:28:54 +0200
>> >Subject: [PATCH 3/4] LDAP: Move sdap_create_search_base from ldap to sdap code
>
>[...]
>
>> >+    if (!ldb_dn_validate(ldn)) {
>> >+        DEBUG(SSSDBG_CRIT_FAILURE,
>> >+                "Invalid base DN [%s]\n",
>> >+                 unparsed_base);
>>                 ^^ 
>> I know that this was also in old file,
>> But You can fix extra spaces as part of this change.
>
>The message actually fits to one line.
>
>> 
>> >+        ret = EINVAL;
>> >+        goto done;
>> >+    }
>> >+
>> >+    base->scope = scope;
>> >+    base->filter = filter;
>> >+
>> >+    *_base = talloc_steal(mem_ctx, base);
>> >+    ret = EOK;
>> >+done:
>> >+    talloc_free(tmp_ctx);
>> >+    return ret;
>> >+}
>> >+
>> > static errno_t sdap_set_search_base(struct sdap_options *opts,
>> >                                     struct sdap_domain *sdom,
>> >                                     enum sdap_basic_opt class,
>> 
>> >From 07b62ed6210f25a6b826896c97c8e75e9a6a9867 Mon Sep 17 00:00:00 2001
>> >From: Jakub Hrozek <jhrozek at redhat.com>
>> >Date: Fri, 4 Sep 2015 18:45:45 +0200
>> >Subject: [PATCH 4/4] LDAP: Filter out multiple entries when searching
>> > overlapping domains
>
>[...]
>
>> >+size_t sdap_copy_objects_in_dom(struct sdap_options *opts,
>> >+                                struct sysdb_attrs **dom_objects,
>> >+                                size_t offset,
>> >+                                struct sss_domain_info *dom,
>> >+                                struct sysdb_attrs **all_objects,
>> >+                                size_t count,
>> >+                                bool filter)
>> The name of the function can be confusing
>> It says "copy objects in domain" but objects are not just copied
>> but also stolen from previous talloc context.
>
>Renamed, the name now says steal and not copy.
>
>> >diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c
>> >index ffb8f7e1f5db5564f45c4f5b7aff46139e64537d..ed0d029f03e5299420ce98423c5abc2fee9b48de 100644
>> >--- a/src/providers/ldap/sdap_async_initgroups.c
>> >+++ b/src/providers/ldap/sdap_async_initgroups.c
>> >@@ -2900,6 +2900,7 @@ static void sdap_get_initgr_user(struct tevent_req *subreq)
>> >         expected_basedn_len = strlen(expected_basedn);
>> > 
>> >         for (c = 0; c < count; c++) {
>> >+            /* FIXME - convert to the new stuff?? */
>> Please file also a ticket.
>
>https://fedorahosted.org/sssd/ticket/2798
>
>Sorry about the fixme, it wasn't supposed to be sent to the list :-)
>
>> 
>> >             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");
>> >diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c
>> >index e38f4cd1610e62aa2cf9f4add3a5f7ad5290e748..1d8008731fb2cc8252e3f18d83e05e79c6b3e9bd 100644
>> >--- a/src/providers/ldap/sdap_async_users.c
>> >+++ b/src/providers/ldap/sdap_async_users.c
>> >@@ -617,6 +617,9 @@ struct sdap_search_user_state {
>> > };
>> > 
>> > static errno_t sdap_search_user_next_base(struct tevent_req *req);
>> >+static void sdap_search_user_copy_batch(struct sdap_search_user_state *state,
>> >+                                        struct sysdb_attrs **users,
>> >+                                        size_t count);
>> > static void sdap_search_user_process(struct tevent_req *subreq);
>> > 
>> > struct tevent_req *sdap_search_user_send(TALLOC_CTX *memctx,
>> >@@ -728,7 +731,7 @@ static void sdap_search_user_process(struct tevent_req *subreq)
>> >     struct sdap_search_user_state *state = tevent_req_data(req,
>> >                                             struct sdap_search_user_state);
>> >     int ret;
>> >-    size_t count, i;
>> >+    size_t count;
>> >     struct sysdb_attrs **users;
>> >     bool next_base = false;
>> > 
>> >@@ -762,16 +765,7 @@ static void sdap_search_user_process(struct tevent_req *subreq)
>> >             return;
>> >         }
>> > 
>> >-        /* Copy the new users into the list
>> >-         * They're already allocated on 'state'
>> >-         */
>> >-        for (i = 0; i < count; i++) {
>> >-            state->users[state->count + i] =
>> >-                talloc_steal(state->users, users[i]);
>> >-        }
>> >-
>> >-        state->count += count;
>> >-        state->users[state->count] = NULL;
>> >+        sdap_search_user_copy_batch(state, users, count);
>> >     }
>> > 
>> >     if (next_base) {
>> >@@ -798,6 +792,25 @@ static void sdap_search_user_process(struct tevent_req *subreq)
>> >     tevent_req_done(req);
>> > }
>> > 
>> >+static void sdap_search_user_copy_batch(struct sdap_search_user_state *state,
>> >+                                        struct sysdb_attrs **users,
>> >+                                        size_t count)
>> >+{
>> >+    size_t copied;
>> >+    bool filter;
>> >+
>> >+    /* Always copy all objects for wildcard lookups. */
>> >+    filter = state->lookup_type == SDAP_LOOKUP_SINGLE ? true : false;
>> >+
>> >+    copied = sdap_copy_objects_in_dom(state->opts,
>> >+                                      state->users,
>> >+                                      state->count,
>> >+                                      state->dom,
>> >+                                      users, count, filter);
>> >+
>> >+    state->count += copied;
>> >+    state->users[state->count] = NULL;
>> >+}
>> > 
>> > int sdap_search_user_recv(TALLOC_CTX *memctx, struct tevent_req *req,
>> >                           char **higher_usn, struct sysdb_attrs ***users,
>> >diff --git a/src/tests/cmocka/test_sdap.c b/src/tests/cmocka/test_sdap.c
>> >index 75fc34504bc35e4e4a5fc11bc9d645e78bc2da72..2e11085df29769f970573ec47351951430cec1a8 100644
>> >--- a/src/tests/cmocka/test_sdap.c
>> >+++ b/src/tests/cmocka/test_sdap.c
>> >@@ -941,6 +941,184 @@ static void test_sdap_inherit_option_user(void **state)
>> >     talloc_free(test_ctx->child_sdap_opts->user_map[SDAP_AT_USER_PRINC].name);
>> > }
>> > 
>> >+struct copy_dom_obj_test_ctx {
>> >+    struct sdap_options *opts;
>> >+
>> >+    struct sss_domain_info *parent;
>> >+    struct sss_domain_info *child;
>> >+
>> >+    struct sdap_domain *parent_sd;
>> >+    struct sdap_domain *child_sd;
>> >+
>> >+    struct sysdb_attrs **ldap_objects;
>> >+    struct sysdb_attrs **dom_objects;
>> >+};
>> >+
>> >+static struct sysdb_attrs *test_obj(TALLOC_CTX *mem_ctx,
>> >+                                    const char *name,
>> >+                                    const char *basedn)
>> >+{
>> >+    errno_t ret;
>> >+    const char *orig_dn;
>> >+    struct sysdb_attrs *obj;
>> >+
>> >+    obj = sysdb_new_attrs(mem_ctx);
>> >+    assert_non_null(obj);
>> >+
>> >+    orig_dn = talloc_asprintf(obj, "CN=%s,%s", name, basedn);
>> >+    assert_non_null(orig_dn);
>> >+
>> >+    ret = sysdb_attrs_add_string(obj, SYSDB_ORIG_DN, orig_dn);
>> >+    assert_int_equal(ret, EOK);
>> >+
>> >+    ret = sysdb_attrs_add_string(obj, SYSDB_NAME, name);
>> >+    assert_int_equal(ret, EOK);
>> >+
>> >+    return obj;
>> >+}
>> >+
>> >+static struct sdap_domain *create_sdap_domain(struct sdap_options *opts,
>> >+                                              struct sss_domain_info *dom)
>> >+{
>> >+    errno_t ret;
>> >+    struct sdap_domain *sdom;
>> >+
>> >+    ret = sdap_domain_add(opts, dom, &sdom);
>> >+    assert_int_equal(ret, EOK);
>> >+
>> >+    sdom->search_bases = talloc_array(sdom, struct sdap_search_base *, 2);
>> >+    assert_non_null(sdom->search_bases);
>> >+    sdom->search_bases[1] = NULL;
>> >+
>> >+    ret = sdap_create_search_base(sdom, sdom->basedn,
>> >+                                  LDAP_SCOPE_SUBTREE,
>> >+                                  NULL,
>> >+                                  &sdom->search_bases[0]);
>> >+    assert_int_equal(ret, EOK);
>> >+
>> >+    return sdom;
>> >+}
>> >+
>> >+static int sdap_copy_objects_in_dom_setup(void **state)
>> >+{
>> >+    struct copy_dom_obj_test_ctx *test_ctx;
>> >+
>> >+    test_ctx = talloc_zero(NULL,
>> >+                           struct copy_dom_obj_test_ctx);
>> >+    assert_non_null(test_ctx);
>> >+
>> >+    test_ctx->opts = talloc_zero(test_ctx, struct sdap_options);
>> >+    assert_non_null(test_ctx->opts);
>> >+
>> >+    test_ctx->parent = named_domain(test_ctx, "win.trust.test", NULL);
>> >+    assert_non_null(test_ctx->parent);
>> >+
>> >+    test_ctx->child = named_domain(test_ctx, "child.win.trust.test",
>> >+                                   test_ctx->parent);
>> >+    assert_non_null(test_ctx->child);
>> >+
>> >+    test_ctx->parent_sd = create_sdap_domain(test_ctx->opts,
>> >+                                             test_ctx->parent);
>> >+    assert_non_null(test_ctx->parent_sd);
>> >+
>> >+    test_ctx->child_sd = create_sdap_domain(test_ctx->opts,
>> >+                                             test_ctx->child);
>>                                               ^
>>                                             copy&paste issue :-)
>
>Fixed
>
>> 
>> >+    assert_non_null(test_ctx->child_sd);
>> >+
>> >+    /* These two objects were 'returned by LDAP' */
>> >+    test_ctx->ldap_objects = talloc_zero_array(test_ctx,
>> >+                                               struct sysdb_attrs *, 2);
>> >+    assert_non_null(test_ctx->ldap_objects);
>> >+
>> >+    test_ctx->ldap_objects[0] = test_obj(test_ctx->ldap_objects, "parent",
>> >+                                         test_ctx->parent_sd->basedn);
>> >+    assert_non_null(test_ctx->ldap_objects[0]);
>> >+
>> >+    test_ctx->ldap_objects[1] = test_obj(test_ctx->ldap_objects, "child",
>> >+                                         test_ctx->child_sd->basedn);
>> >+    assert_non_null(test_ctx->ldap_objects[1]);
>> >+
>> >+    /* This is the array we'll filter to */
>> >+    test_ctx->dom_objects = talloc_zero_array(test_ctx,
>> >+                                              struct sysdb_attrs *, 2);
>> >+    assert_non_null(test_ctx->dom_objects);
>> >+
>> >+    *state = test_ctx;
>> >+    return 0;
>> >+}
>> >+
>> >+static int sdap_copy_objects_in_dom_teardown(void **state)
>> >+{
>> >+    struct copy_dom_obj_test_ctx *test_ctx = talloc_get_type_abort(*state,
>> >+                                                 struct copy_dom_obj_test_ctx);
>> >+
>> >+    talloc_free(test_ctx);
>> >+    return 0;
>> >+}
>> >+
>> >+static void test_sdap_copy_objects_in_dom(void **state)
>> >+{
>> >+    struct copy_dom_obj_test_ctx *test_ctx = talloc_get_type_abort(*state,
>> >+                                                 struct copy_dom_obj_test_ctx);
>> >+    size_t count;
>> >+
>> >+    assert_true(talloc_parent(test_ctx->ldap_objects[0]) == \
>> >+                test_ctx->ldap_objects);
>> >+    assert_true(talloc_parent(test_ctx->ldap_objects[1]) == \
>> >+                test_ctx->ldap_objects);
>> >+
>> It's better to use assert_ptr_equal for this purpose
>> So in cese of error the pointers will be printed to standart error.
>
>Fixed.
>
>> 
>> The same pattern is also on other places.
>
>I didn't realize we have this function in cmocka. Feel free to send a
>patch, so far I only fixed the calls these patches add.

>From 571b46e3a9fe477ce3948f06906993c9d9c028be Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek at redhat.com>
>Date: Tue, 21 Jul 2015 21:00:27 +0200
>Subject: [PATCH 1/4] LDAP: imposing sizelimit=1 for single-entry searches
> breaks overlapping domains
>
4x ACK

LS


More information about the sssd-devel mailing list