[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