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

Jakub Hrozek jhrozek at redhat.com
Mon Sep 21 14:50:22 UTC 2015


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.
-------------- next part --------------
>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

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.
---
 src/providers/ldap/sdap_async_groups.c | 10 ----------
 src/providers/ldap/sdap_async_users.c  |  3 ---
 2 files changed, 13 deletions(-)

diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
index 525c6fa09553d8c0232ce2317751184f83632d86..57a53af3f4eb46e6f31af9ee7c4d4625239d2a54 100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -1874,8 +1874,6 @@ static errno_t sdap_get_groups_next_base(struct tevent_req *req)
 
     switch (state->lookup_type) {
     case SDAP_LOOKUP_SINGLE:
-        sizelimit = 1;
-        need_paging = false;
         break;
     /* Only requests that can return multiple entries should require
      * the paging control
@@ -1885,7 +1883,6 @@ static errno_t sdap_get_groups_next_base(struct tevent_req *req)
         need_paging = true;
         break;
     case SDAP_LOOKUP_ENUMERATE:
-        sizelimit = 0;  /* unlimited */
         need_paging = true;
         break;
     }
@@ -1934,13 +1931,6 @@ static void sdap_get_groups_process(struct tevent_req *subreq)
     DEBUG(SSSDBG_TRACE_FUNC,
           "Search for groups, returned %zu results.\n", count);
 
-    if (state->lookup_type == SDAP_LOOKUP_SINGLE && count > 1) {
-        DEBUG(SSSDBG_MINOR_FAILURE,
-              "Individual group search returned multiple results\n");
-        tevent_req_error(req, EINVAL);
-        return;
-    }
-
     if (state->lookup_type == SDAP_LOOKUP_WILDCARD || \
             state->lookup_type == SDAP_LOOKUP_ENUMERATE || \
         count == 0) {
diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c
index a864a8b2187de7972aa963b355856e97f7c692a9..e38f4cd1610e62aa2cf9f4add3a5f7ad5290e748 100644
--- a/src/providers/ldap/sdap_async_users.c
+++ b/src/providers/ldap/sdap_async_users.c
@@ -692,8 +692,6 @@ static errno_t sdap_search_user_next_base(struct tevent_req *req)
 
     switch (state->lookup_type) {
     case SDAP_LOOKUP_SINGLE:
-        sizelimit = 1;
-        need_paging = false;
         break;
     /* Only requests that can return multiple entries should require
      * the paging control
@@ -703,7 +701,6 @@ static errno_t sdap_search_user_next_base(struct tevent_req *req)
         need_paging = true;
         break;
     case SDAP_LOOKUP_ENUMERATE:
-        sizelimit = 0;  /* unlimited */
         need_paging = true;
         break;
     }
-- 
2.4.3

-------------- next part --------------
>From 680da7fc195ae3e31c53d39ced26186b32562d92 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at 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.
---
 src/tests/cmocka/test_utils.c | 17 -----------------
 src/tests/common.h            |  4 ++++
 src/tests/common_dom.c        | 22 ++++++++++++++++++++++
 3 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c
index 2e413c87d069549ddd0ca0ee5a8dec0b663f481f..2bb1dfef599089dab99a46c6620338dd28457e8c 100644
--- a/src/tests/cmocka/test_utils.c
+++ b/src/tests/cmocka/test_utils.c
@@ -454,23 +454,6 @@ void test_find_domain_by_sid_disabled(void **state)
     }
 }
 
-static struct sss_domain_info *named_domain(TALLOC_CTX *mem_ctx,
-                                            const char *name,
-                                            struct sss_domain_info *parent)
-{
-    struct sss_domain_info *dom = NULL;
-
-    dom = talloc_zero(mem_ctx, struct sss_domain_info);
-    assert_non_null(dom);
-
-    dom->name = talloc_strdup(dom, name);
-    assert_non_null(dom->name);
-
-    dom->parent = parent;
-
-    return dom;
-}
-
 /*
  * dom1 -> sub1a
  *  |
diff --git a/src/tests/common.h b/src/tests/common.h
index 1c6de2c3d6eb924ba7306bd350f8546d61f30751..c0ceebfbf5d9277a422065fbe4ddfdd65c758de8 100644
--- a/src/tests/common.h
+++ b/src/tests/common.h
@@ -134,4 +134,8 @@ test_dbus_call_sync(DBusConnection *conn,
                     int first_arg_type,
                     ...);
 
+struct sss_domain_info *named_domain(TALLOC_CTX *mem_ctx,
+                                     const char *name,
+                                     struct sss_domain_info *parent);
+
 #endif /* !__TESTS_COMMON_H__ */
diff --git a/src/tests/common_dom.c b/src/tests/common_dom.c
index 46ea0b7826c5a2dd025a2faaeb46f614ddd2f281..24d1f5a8ad6dfade016a76de83fa182809f52702 100644
--- a/src/tests/common_dom.c
+++ b/src/tests/common_dom.c
@@ -371,3 +371,25 @@ void test_dom_suite_cleanup(const char *tests_path,
 
     test_multidom_suite_cleanup(tests_path, cdb_file, domains);
 }
+
+struct sss_domain_info *named_domain(TALLOC_CTX *mem_ctx,
+                                     const char *name,
+                                     struct sss_domain_info *parent)
+{
+    struct sss_domain_info *dom = NULL;
+
+    dom = talloc_zero(mem_ctx, struct sss_domain_info);
+    if (dom == NULL) {
+        return NULL;
+    }
+
+    dom->name = talloc_strdup(dom, name);
+    if (dom->name == NULL) {
+        talloc_free(dom);
+        return NULL;
+    }
+
+    dom->parent = parent;
+
+    return dom;
+}
-- 
2.4.3

-------------- next part --------------
>From 9490a26be3b2dfd807ac7d8070eb3d0d1716c93a 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

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         | 61 +++++++++++++++++++++++++++++++++++++
 src/providers/ldap/sdap.h         |  7 +++++
 4 files changed, 68 insertions(+), 70 deletions(-)

diff --git a/src/providers/ldap/ldap_common.h b/src/providers/ldap/ldap_common.h
index 716aaa08ef83b8fca3779b830fdef20bbdb14937..f552520a0503908f82b845f8e813cf67306ec954 100644
--- a/src/providers/ldap/ldap_common.h
+++ b/src/providers/ldap/ldap_common.h
@@ -294,13 +294,6 @@ struct sdap_domain *sdap_domain_get(struct sdap_options *opts,
 struct sdap_domain *sdap_domain_get_by_dn(struct sdap_options *opts,
                                           const char *dn);
 
-errno_t
-sdap_create_search_base(TALLOC_CTX *mem_ctx,
-                        const char *unparsed_base,
-                        int scope,
-                        const char *filter,
-                        struct sdap_search_base **_base);
-
 errno_t sdap_parse_search_base(TALLOC_CTX *mem_ctx,
                                struct dp_option *opts, int class,
                                struct sdap_search_base ***_search_bases);
diff --git a/src/providers/ldap/ldap_options.c b/src/providers/ldap/ldap_options.c
index eb00aab32a55c6841ebbf32d30f3a48722e8b165..7ad6071508d0abbb33984c697b833cf12f9e4df9 100644
--- a/src/providers/ldap/ldap_options.c
+++ b/src/providers/ldap/ldap_options.c
@@ -532,69 +532,6 @@ errno_t sdap_parse_search_base(TALLOC_CTX *mem_ctx,
                                     _search_bases);
 }
 
-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);
-        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;
-}
-
 errno_t common_parse_search_base(TALLOC_CTX *mem_ctx,
                                  const char *unparsed_base,
                                  const char *class_name,
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index 97bc14b8730565b6d0d2c41e51f910f042357bc3..5aa7ff7cae8b52eb358c806f7500e1386c2bc948 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -1031,6 +1031,67 @@ 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);
+        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,
diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
index b3321be48e1b24124d59fbfe88096a2109c920a6..0dc6f751a116f4e32579646e4d9a85470cb9d686 100644
--- a/src/providers/ldap/sdap.h
+++ b/src/providers/ldap/sdap.h
@@ -373,6 +373,13 @@ struct sdap_search_base {
     const char *filter;
 };
 
+errno_t
+sdap_create_search_base(TALLOC_CTX *mem_ctx,
+                        const char *unparsed_base,
+                        int scope,
+                        const char *filter,
+                        struct sdap_search_base **_base);
+
 /* Values from
  * http://msdn.microsoft.com/en-us/library/cc223272%28v=prot.13%29.aspx
  */
-- 
2.4.3

-------------- next part --------------
>From b6270001bd41d791d4d731e643cc570260c36fba 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

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_users.c  |  35 +++++--
 src/tests/cmocka/test_sdap.c           | 186 +++++++++++++++++++++++++++++++++
 6 files changed, 306 insertions(+), 20 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 3eaf578a8c7c3b1e13a9516f96886c7a8f18c4d3..d3f363ad1084c7b8dbd60086a73e2f1d3598a5bf 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1633,6 +1633,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 = \
@@ -1641,6 +1642,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
@@ -2224,6 +2226,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 \
@@ -2245,6 +2248,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 5aa7ff7cae8b52eb358c806f7500e1386c2bc948..fcdc4028efe97bba13f265a8cfd7c75fa6b7a07c 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -1619,3 +1619,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_steal_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)
+{
+    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..edfbf229b4c4396592020de931eba5f83a8f06ed 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_steal_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..653187b3add172d37f234850ccab8ddf109e229c 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_steal_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_users.c b/src/providers/ldap/sdap_async_users.c
index e38f4cd1610e62aa2cf9f4add3a5f7ad5290e748..865439cadeb4f9f9452b1549663691c29e52f27b 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_steal_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..86c940c89b1a1010e0d33e1b89441c09d8b2e9dd 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);
+    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_ptr_equal(talloc_parent(test_ctx->ldap_objects[0]),
+                     test_ctx->ldap_objects);
+    assert_ptr_equal(talloc_parent(test_ctx->ldap_objects[1]),
+                     test_ctx->ldap_objects);
+
+    assert_null(test_ctx->dom_objects[0]);
+    assert_null(test_ctx->dom_objects[1]);
+
+    count = sdap_steal_objects_in_dom(test_ctx->opts,
+                                      test_ctx->dom_objects,
+                                      0,
+                                      test_ctx->parent,
+                                      test_ctx->ldap_objects,
+                                      2, true);
+    assert_int_equal(count, 1);
+
+    assert_non_null(test_ctx->dom_objects[0]);
+    assert_non_null(test_ctx->dom_objects[0] == test_ctx->ldap_objects[0]);
+    assert_null(test_ctx->dom_objects[1]);
+
+    assert_ptr_equal(talloc_parent(test_ctx->ldap_objects[0]),
+                     test_ctx->dom_objects);
+
+    count = sdap_steal_objects_in_dom(test_ctx->opts,
+                                      test_ctx->dom_objects,
+                                      1,
+                                      test_ctx->child,
+                                      test_ctx->ldap_objects,
+                                      2, true);
+    assert_int_equal(count, 1);
+
+    assert_non_null(test_ctx->dom_objects[1]);
+    assert_non_null(test_ctx->dom_objects[1] == test_ctx->ldap_objects[1]);
+    assert_ptr_equal(talloc_parent(test_ctx->ldap_objects[1]),
+                     test_ctx->dom_objects);
+}
+
+static void test_sdap_copy_objects_in_dom_nofilter(void **state)
+{
+    struct copy_dom_obj_test_ctx *test_ctx = talloc_get_type_abort(*state,
+                                                 struct copy_dom_obj_test_ctx);
+    size_t count;
+
+    count = sdap_steal_objects_in_dom(test_ctx->opts,
+                                      test_ctx->dom_objects,
+                                      0,
+                                      test_ctx->parent,
+                                      test_ctx->ldap_objects,
+                                      2, false);
+    assert_int_equal(count, 2);
+
+    assert_ptr_equal(talloc_parent(test_ctx->ldap_objects[0]),
+                     test_ctx->dom_objects);
+    assert_ptr_equal(talloc_parent(test_ctx->ldap_objects[1]),
+                     test_ctx->dom_objects);
+}
+
 int main(int argc, const char *argv[])
 {
     poptContext pc;
@@ -1008,6 +1186,14 @@ int main(int argc, const char *argv[])
         cmocka_unit_test_setup_teardown(test_sdap_inherit_option_user,
                                         test_sdap_inherit_option_setup,
                                         test_sdap_inherit_option_teardown),
+
+        /* Per-domain object filter tests */
+        cmocka_unit_test_setup_teardown(test_sdap_copy_objects_in_dom,
+                                        sdap_copy_objects_in_dom_setup,
+                                        sdap_copy_objects_in_dom_teardown),
+        cmocka_unit_test_setup_teardown(test_sdap_copy_objects_in_dom_nofilter,
+                                        sdap_copy_objects_in_dom_setup,
+                                        sdap_copy_objects_in_dom_teardown),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
2.4.3



More information about the sssd-devel mailing list