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 42e0c9459818d51fa203aa9449d69834c439007e Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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

https://fedorahosted.org/sssd/ticket/2723

In case there are overlapping sdap domains, a search for a single user might match and return multiple entries. For instance, with AD domains represented by search bases: DC=win,DC=trust,DC=test DC=child,DC=win,DC=trust,DC=test

A search for user from win.trust.test would be based at: DC=win,DC=trust,DC=test but would match both search bases and return both users.

Instead of performing complex filtering, just save both users. The responder would select the entry that matches the user's search.

---

ACK

From 9fa269ee45fb37b2c8946668aaf07a4343ce654f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 16 Sep 2015 15:31:02 +0200 Subject: [PATCH 2/4] tests: Move named_domain from test_utils to common test code

This handy function should be reused by other parts of the code.

ACK

From e07541b9b61778fcd3377388fe105de15898cb16 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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

The function shouldn't be placed in the LDAP tree, but in the SDAP tree to make it usable from tests without linking to libraries that are normally linked from LDAP provider (such as confdb)

---

src/providers/ldap/ldap_common.h | 7 ----- src/providers/ldap/ldap_options.c | 63 --------------------------------------- src/providers/ldap/sdap.c | 63 +++++++++++++++++++++++++++++++++++++++ src/providers/ldap/sdap.h | 7 +++++ 4 files changed, 70 insertions(+), 70 deletions(-)

diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97bc14b8730565b6d0d2c41e51f910f042357bc3..ca03bde67eded5955e865194dc41252941e0a8e5 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -1031,6 +1031,69 @@ static char *get_naming_context(TALLOC_CTX *mem_ctx, return naming_context; }

+errno_t +sdap_create_search_base(TALLOC_CTX *mem_ctx,

•                    const char *unparsed_base,
  

•                    int scope,
  

•                    const char *filter,
  

•                    struct sdap_search_base **_base)
  

+{

• struct sdap_search_base *base;
• TALLOC_CTX *tmp_ctx;
• errno_t ret;
• struct ldb_dn *ldn;
• struct ldb_context *ldb;
• tmp_ctx = talloc_new(NULL);
• if (!tmp_ctx) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• /* Create a throwaway LDB context for validating the DN */
• ldb = ldb_init(tmp_ctx, NULL);
• if (!ldb) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• base = talloc_zero(tmp_ctx, struct sdap_search_base);
• if (base == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• base->basedn = talloc_strdup(base, unparsed_base);
• if (base->basedn == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• /* Validate the basedn */
• ldn = ldb_dn_new(tmp_ctx, ldb, unparsed_base);
• if (!ldn) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• 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.

•    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@redhat.com Date: Fri, 4 Sep 2015 18:45:45 +0200 Subject: [PATCH 4/4] LDAP: Filter out multiple entries when searching overlapping domains

In case domain overlap, we might download multiple objects. To avoid saving them all, we attempt to filter out the objects from foreign domains.

We can only do this optimization for non-wildcard lookups.

Makefile.am | 4 + src/providers/ldap/sdap.c | 59 +++++++++ src/providers/ldap/sdap.h | 9 ++ src/providers/ldap/sdap_async_groups.c | 33 +++-- src/providers/ldap/sdap_async_initgroups.c | 1 + src/providers/ldap/sdap_async_users.c | 35 ++++-- src/tests/cmocka/test_sdap.c | 186 +++++++++++++++++++++++++++++ 7 files changed, 307 insertions(+), 20 deletions(-)

diff --git a/Makefile.am b/Makefile.am index dc0670a5c720ab58a47e7da356578256b4659695..3e69e145975349bc5e3e7b5180e2cd68dc537081 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1631,6 +1631,7 @@ ipa_ldap_opt_tests_SOURCES = \ src/providers/data_provider_opts.c \ src/providers/ldap/sdap.c \ src/providers/ldap/sdap_range.c \

• src/providers/ldap/sdap_domain.c \ src/util/sss_ldap.c \ src/tests/ipa_ldap_opt-tests.c

ipa_ldap_opt_tests_CFLAGS = \ @@ -1639,6 +1640,7 @@ ipa_ldap_opt_tests_CFLAGS = \ ipa_ldap_opt_tests_LDADD = \ $(CHECK_LIBS) \ $(TALLOC_LIBS) \

• $(LDB_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ $(OPENLDAP_LIBS) \ libsss_test_common.la

@@ -2222,6 +2224,7 @@ dp_opt_tests_LDADD = \

sdap_tests_SOURCES = \ src/providers/data_provider_opts.c \

• src/providers/ldap/sdap_domain.c \ src/providers/ldap/sdap.c \ src/providers/ldap/sdap_range.c \ src/util/sss_ldap.c \

@@ -2243,6 +2246,7 @@ sdap_tests_LDFLAGS = \ sdap_tests_LDADD = \ $(CMOCKA_LIBS) \ $(TALLOC_LIBS) \

• $(LDB_LIBS) \ $(POPT_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ $(SSS_CRYPT_LIBS) \

diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index ca03bde67eded5955e865194dc41252941e0a8e5..b007347c6d2de5e8d82ddad1874f4d0209ce31f8 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -1621,3 +1621,62 @@ char *sdap_make_oc_list(TALLOC_CTX *mem_ctx, struct sdap_attr_map *map) map[SDAP_OC_GROUP_ALT].name); } }

+static bool sdap_object_in_domain(struct sdap_options *opts,

•                              struct sysdb_attrs *obj,
  

•                              struct sss_domain_info *dom)
  

+{

• errno_t ret;
• const char *original_dn = NULL;
• struct sdap_domain *sdmatch = NULL;
• ret = sysdb_attrs_get_string(obj, SYSDB_ORIG_DN, &original_dn);
• if (ret) {

•    DEBUG(SSSDBG_FUNC_DATA,
  

•          "The group has no original DN, assuming our domain\n");
  

•    return true;
  

• }
• sdmatch = sdap_domain_get_by_dn(opts, original_dn);
• if (sdmatch == NULL) {

•    DEBUG(SSSDBG_FUNC_DATA,
  

•          "The group has no original DN, assuming our domain\n");
  

•    return true;
  

• }
• return (sdmatch->dom == dom);

+}

+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.

+{

• size_t copied = 0;
• /* Own objects from all_objects by dom_objects in case they belong

• * to domain dom.
  

• *
  

• * Don't copy objects from other domains in case
  

• * the search was for parent domain but a child domain would match,
  

• * too, such as:
  

• *  dc=example,dc=com
  

• *  dc=child,dc=example,dc=com
  

• * while searching for an object from dc=example.
  

• */
  

• for (size_t i = 0; i < count; i++) {

•    if (filter &&
  

•            sdap_object_in_domain(opts, all_objects[i], dom) == false) {
  

•        continue;
  

•    }
  

•    dom_objects[offset + copied] =
  

•        talloc_steal(dom_objects, all_objects[i]);
  

•    copied++;
  

• }
• return copied;

+} diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h index 0dc6f751a116f4e32579646e4d9a85470cb9d686..e15c6f2002a3f62e019a290e49e6465bfa460b9f 100644 --- a/src/providers/ldap/sdap.h +++ b/src/providers/ldap/sdap.h @@ -580,4 +580,13 @@ void sdap_steal_server_opts(struct sdap_id_ctx *id_ctx, struct sdap_server_opts **srv_opts);

char *sdap_make_oc_list(TALLOC_CTX *mem_ctx, struct sdap_attr_map *map);

+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);
  

#endif /* _SDAP_H_ */ diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 57a53af3f4eb46e6f31af9ee7c4d4625239d2a54..ecadf147c3c85a35db7b19788f3e0171e1ea030f 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -1905,6 +1905,9 @@ static errno_t sdap_get_groups_next_base(struct tevent_req *req) }

static void sdap_nested_done(struct tevent_req *req); +static void sdap_search_group_copy_batch(struct sdap_get_groups_state *state,

•                                     struct sysdb_attrs **groups,
  

•                                     size_t count);
  

static void sdap_ad_match_rule_members_process(struct tevent_req *subreq);

static void sdap_get_groups_process(struct tevent_req *subreq) @@ -1950,15 +1953,7 @@ static void sdap_get_groups_process(struct tevent_req *subreq) return; }

•    /* Copy the new groups into the list
  

•     */
  

•    for (i = 0; i < count; i++) {
  

•        state->groups[state->count + i] =
  

•            talloc_steal(state->groups, groups[i]);
  

•    }
  

•    state->count += count;
  

•    state->groups[state->count] = NULL;
  

•    sdap_search_group_copy_batch(state, groups, count);
  

  }

  if (next_base) {

@@ -2093,6 +2088,26 @@ static void sdap_get_groups_process(struct tevent_req *subreq) } }

+static void sdap_search_group_copy_batch(struct sdap_get_groups_state *state,

•                                     struct sysdb_attrs **groups,
  

•                                     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->groups,
  

•                                  state->count,
  

•                                  state->dom,
  

•                                  groups, count, filter);
  

• state->count += copied;
• state->groups[state->count] = NULL;

+}

static void sdap_get_groups_done(struct tevent_req *subreq) { struct tevent_req *req = 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.

        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 :-)

• 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.

The same pattern is also on other places.

LS

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@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@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@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