On Tue, Jul 26, 2016 at 06:06:48PM +0200, Jakub Hrozek wrote:
> On Tue, Jul 26, 2016 at 05:25:11PM +0200, Jakub Hrozek wrote:
> > On Tue, Jul 26, 2016 at 01:51:56PM +0200, Sumit Bose wrote:
> > > > > The third patch adds a sysdb call to recursively resolve all
> > > > > user-members of a group. Since the groups in SSSD's cache are
> > > > > hierarchically organized the member attribute only contains direct
> > > > > user and group members. To get all users the group members must be
> > > > > resolved recursively.
> > > >
> > > > Would dereferencing memberof:top-level-group yield different results?
> > >
> > > It worked in my testing but I have to admit that I'm not sure if it can
> > > be used reliable all the time, i.e. is independent of all the different
> > > lookup sequences you can have with nested groups. If you are sure it is
> > > reliable, the call can be simplified.
> >
> > This is how memberof is supposed to work. I haven't tested all
> > scenarios either (if there are some corner cases you'd like me to test,
> > just let me know), but if there are differences, I would say these would
> > be bugs in the memberof plugin and should be fixed.
>
> btw the patches seem to work fine, I tested getent passwd on an
> overriden user, getent group on a group this user is a memberof (both an
> AD group and an IPA group with an external group in it) and id of this
> user.
>
> All lookups show the expected data. Coverity is clean and CI passed.
>
> Therefore provisional ACK - the only remaining point remains the recursive
> member vs. memberof part. I don't mind accepting the patch as-is now,
> if we agree to open a ticket and switch to memberof later in 1.14.
Please have a look at attached patch, it replaces the recursive member
based lookup by a (memberof=group_dn) search. It works well for me in
some basic tests.
bye,
Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
From ee8ebcec8062d75e98da796b291973fd96d45b1c Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Tue, 26 Jul 2016 21:30:41 +0200
Subject: [PATCH] sysdb_get_user_members_recursively: use memberof search
---
src/db/sysdb_ops.c | 181 ++++------------------------------------
src/tests/cmocka/test_nss_srv.c | 30 +++----
2 files changed, 33 insertions(+), 178 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index
b7c04cb4ab64c2767e211d6ead1463a3a3024e68..9a8a55ed8aa69e1638d0ab6f636e43baa3d0bfea
100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -4711,159 +4711,42 @@ done:
return ret;
}
-static
-errno_t sysdb_get_user_members_recursively_int(TALLOC_CTX *mem_ctx,
- struct sss_domain_info *dom,
- struct ldb_dn *group_dn,
- hash_table_t *users,
- hash_table_t *groups)
-{
- errno_t ret;
- size_t c;
- size_t m_count;
- struct ldb_message **members;
- const char *oc;
- hash_key_t key;
- hash_value_t value;
- int hret;
-
- /* Get all elemets pointed to by group members */
- ret = sysdb_asq_search(mem_ctx, dom, group_dn, NULL, SYSDB_MEMBER,
- NULL /* requesting all attributes */,
- &m_count, &members);
- if (ret != EOK) {
- if (ret != ENOENT) {
- DEBUG(SSSDBG_OP_FAILURE, "sysdb_asq_search failed.\n");
- }
- goto done;
- }
-
- key.type = HASH_KEY_STRING;
-
- for (c = 0; c < m_count; c++) {
-
- oc = ldb_msg_find_attr_as_string(members[c], SYSDB_OBJECTCLASS, NULL);
- if (oc == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Missing objectclass in object [%s].\n",
- ldb_dn_get_linearized(members[c]->dn));
- ret = EINVAL;
- goto done;
- }
-
- if (strcmp(oc, SYSDB_USER_CLASS) == 0) {
- key.str = discard_const(ldb_dn_get_linearized(members[c]->dn));
- if (key.str == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, "ldb_dn_get_linearized failed.\n");
- ret = EFAULT;
- goto done;
- }
-
- value.type = HASH_VALUE_PTR;
- value.ptr = members[c];
-
- hret = hash_enter(users, &key, &value);
- if (hret != HASH_SUCCESS) {
- DEBUG(SSSDBG_OP_FAILURE, "hash_enter failed.\n");
- ret = ENOMEM;
- goto done;
- }
-
- } else if (strcmp(oc, SYSDB_GROUP_CLASS) == 0) {
- key.str = discard_const(ldb_dn_get_linearized(members[c]->dn));
- if (key.str == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, "ldb_dn_get_linearized failed.\n");
- ret = EFAULT;
- goto done;
- }
-
- hret = hash_lookup(groups, &key, &value);
- if (hret == HASH_SUCCESS) {
- /* Already seen this group, skipping */
- continue;
- } else if (hret != HASH_ERROR_KEY_NOT_FOUND) {
- DEBUG(SSSDBG_OP_FAILURE, "hash_lookup failed.\n");
- ret = EFAULT;
- goto done;
- }
-
- /* group was not found in the hash, adding and processing it */
-
- value.type = HASH_VALUE_PTR;
- value.ptr = members[c];
-
- hret = hash_enter(groups, &key, &value);
- if (hret != HASH_SUCCESS) {
- DEBUG(SSSDBG_OP_FAILURE, "hash_enter failed.\n");
- ret = ENOMEM;
- goto done;
- }
-
- ret = sysdb_get_user_members_recursively_int(mem_ctx, dom,
- members[c]->dn,
- users, groups);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE,
- "sysdb_get_user_members_recursively_int failed.\n");
- goto done;
- }
-
- } else {
- DEBUG(SSSDBG_CRIT_FAILURE,
- "Unexpected objectclass [%s] in object [%s].\n", oc,
- ldb_dn_get_linearized(members[c]->dn));
- ret = EINVAL;
- goto done;
- }
- }
-
- ret = EOK;
-
-done:
-
- return ret;
-}
-
errno_t sysdb_get_user_members_recursively(TALLOC_CTX *mem_ctx,
struct sss_domain_info *dom,
struct ldb_dn *group_dn,
struct ldb_result **members)
{
TALLOC_CTX *tmp_ctx;
- hash_table_t *users;
- hash_table_t *groups;
int ret;
- int hret;
- size_t c;
- unsigned long count;
+ size_t count;
struct ldb_result *res;
- hash_value_t *values;
+ struct ldb_dn *base_dn;
+ char *filter;
+ const char *attrs[] = SYSDB_PW_ATTRS;
+ struct ldb_message **msgs;
tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) {
return ENOMEM;
}
- ret = sss_hash_create(tmp_ctx, 10, &users);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "sss_hash_create failed.\n");
+ base_dn = sysdb_base_dn(dom->sysdb, tmp_ctx);
+ if (base_dn == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "sysdb_base_dn failed.\n");
+ ret = ENOMEM;
goto done;
}
- ret = sss_hash_create(tmp_ctx, 10, &groups);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "sss_hash_create failed.\n");
+ filter = talloc_asprintf(tmp_ctx, "(&("SYSDB_UC")("SYSDB_MEMBEROF"=%s))",
+ ldb_dn_get_linearized(group_dn));
+ if (filter == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");
+ ret = ENOMEM;
goto done;
}
- ret = sysdb_get_user_members_recursively_int(tmp_ctx, dom, group_dn, users,
- groups);
- if (ret != EOK) {
- if (ret != ENOENT) {
- DEBUG(SSSDBG_OP_FAILURE,
- "sysdb_get_user_members_recursively_int failed.\n");
- }
- goto done;
- }
+ ret = sysdb_search_entry(tmp_ctx, dom->sysdb, base_dn, LDB_SCOPE_SUBTREE,
+ filter, attrs, &count, &msgs);
res = talloc_zero(tmp_ctx, struct ldb_result);
if (res == NULL) {
@@ -4872,36 +4755,8 @@ errno_t sysdb_get_user_members_recursively(TALLOC_CTX
*mem_ctx,
goto done;
}
- hret = hash_values(users, &count, &values);
- if (hret != HASH_SUCCESS) {
- DEBUG(SSSDBG_OP_FAILURE, "hash_values failed.\n");
- ret = ENOMEM;
- goto done;
- }
- if (count <= UINTMAX_MAX) {
- res->count = count;
- } else {
- DEBUG(SSSDBG_CRIT_FAILURE, "More then [%"PRIuMAX"] results.\n",
- UINTMAX_MAX);
- ret = EINVAL;
- goto done;
- }
- if (res->count == 0) {
- ret = EOK;
- goto done;
- }
-
- res->msgs = talloc_array(res, struct ldb_message *, res->count);
- if (res->msgs == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
- ret = ENOMEM;
- goto done;
- }
-
- for (c = 0; c < res->count; c++) {
- res->msgs[c] = (struct ldb_message *) talloc_steal(res->msgs,
- values[c].ptr);
- }
+ res->count = count;
+ res->msgs = talloc_steal(res, msgs);
ret = EOK;
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index
892bebe63816a0dac0317dd1fd0135523f0e64d7..6fa167aa464d5685c5fe6d11e792006cc1b0075c
100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -1342,8 +1342,8 @@ static int test_nss_getgrnam_members_check(uint32_t
status,
int ret;
uint32_t nmem;
struct group gr;
- const char *exp_members[] = { testmember2.pw_name,
- testmember1.pw_name };
+ const char *exp_members[] = { testmember1.pw_name,
+ testmember2.pw_name };
struct group expected = {
.gr_gid = testgroup_members.gr_gid,
.gr_name = testgroup_members.gr_name,
@@ -1429,10 +1429,10 @@ static int
test_nss_getgrnam_members_check_fqdn(uint32_t status,
assert_non_null(tmp_ctx);
exp_members[0] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
- nss_test_ctx->tctx->dom,
testmember2.pw_name);
+ nss_test_ctx->tctx->dom,
testmember1.pw_name);
assert_non_null(exp_members[0]);
exp_members[1] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
- nss_test_ctx->tctx->dom,
testmember1.pw_name);
+ nss_test_ctx->tctx->dom,
testmember2.pw_name);
assert_non_null(exp_members[1]);
expected.gr_name = sss_tc_fqname(tmp_ctx,
@@ -1619,11 +1619,11 @@ static int test_nss_getgrnam_check_mix_dom(uint32_t
status,
tmp_ctx = talloc_new(nss_test_ctx);
assert_non_null(tmp_ctx);
- exp_members[0] = testmember2.pw_name;
+ exp_members[0] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
+ nss_test_ctx->subdom, submember1.pw_name);
+ assert_non_null(exp_members[0]);
exp_members[1] = testmember1.pw_name;
- exp_members[2] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
- nss_test_ctx->subdom, submember1.pw_name);
- assert_non_null(exp_members[2]);
+ exp_members[2] = testmember2.pw_name;
assert_int_equal(status, EOK);
@@ -1682,14 +1682,14 @@ static int
test_nss_getgrnam_check_mix_dom_fqdn(uint32_t status,
tmp_ctx = talloc_new(nss_test_ctx);
assert_non_null(tmp_ctx);
- exp_members[0] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
- nss_test_ctx->tctx->dom,
testmember2.pw_name);
+ exp_members[0] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
+ nss_test_ctx->subdom, submember1.pw_name);
assert_non_null(exp_members[0]);
exp_members[1] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
nss_test_ctx->tctx->dom,
testmember1.pw_name);
assert_non_null(exp_members[1]);
- exp_members[2] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
- nss_test_ctx->subdom, submember1.pw_name);
+ exp_members[2] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
+ nss_test_ctx->tctx->dom,
testmember2.pw_name);
assert_non_null(exp_members[2]);
expected.gr_name = sss_tc_fqname(tmp_ctx,
@@ -1752,16 +1752,16 @@ static int test_nss_getgrnam_check_mix_subdom(uint32_t
status,
tmp_ctx = talloc_new(nss_test_ctx);
assert_non_null(tmp_ctx);
- exp_members[1] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
+ exp_members[0] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
nss_test_ctx->subdom, submember1.pw_name);
assert_non_null(exp_members[1]);
- exp_members[2] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
+ exp_members[1] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
nss_test_ctx->subdom, submember2.pw_name);
assert_non_null(exp_members[2]);
/* Important: this member is from a non-qualified domain, so his name will
* not be qualified either
*/
- exp_members[0] = testmember1.pw_name;
+ exp_members[2] = testmember1.pw_name;
expected.gr_name = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
nss_test_ctx->subdom,
testsubdomgroup.gr_name);
--
2.1.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
