Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
The first two patches are for the IPA provider and make sure that all ghost members in a group get resolved because otherwise we cannot determine if the name is overridden or not. This adds an overhead to group lookups, especially for larger groups but I think it is an requirement which cannot be skipped.
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.
Finally the forth patch applies the code-path which is already used for non-default views to the default case as well and adds a new list of members, with correctly overridden names (hopefully :-) which is then used in fill_grmem(). This adds some overhead to the overall group processing in the NSS responder (as can be seen in the test changes because the members are returned in different order in some cases). But I think because the of memory cache this is acceptable and might even help to remove the memberuid attribute in future and make the memberof plugin simpler.
I worked on an alternative approach as well which tried to make the memberof plugin aware of the defaultOverrideName attribute. My wip tree is at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=memberof_defa... but so far it does not work properly. Additionally I would prefer to not touch the memberof plugin.
bye, Sumit
On Thu, Jul 21, 2016 at 02:13:40PM +0200, Sumit Bose wrote:
Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
So far I read them to get a grasp of what they do, but didn't do a full review. See some comments below.
The first two patches are for the IPA provider and make sure that all ghost members in a group get resolved because otherwise we cannot determine if the name is overridden or not. This adds an overhead to group lookups, especially for larger groups but I think it is an requirement which cannot be skipped.
Right. But I wonder if we could skip it on-demand. Could we maybe check if any views exist in the IPA domain at all and not resolve the members if there are no idview data? I would guess that since this is SSSD on the IPA server, then an additiona LDAP lookup might be cheaper than resolving a full large group.
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?
Finally the forth patch applies the code-path which is already used for non-default views to the default case as well and adds a new list of members, with correctly overridden names (hopefully :-) which is then used in fill_grmem(). This adds some overhead to the overall group processing in the NSS responder (as can be seen in the test changes because the members are returned in different order in some cases). But I think because the of memory cache this is acceptable and might even help to remove the memberuid attribute in future and make the memberof plugin simpler.
I worked on an alternative approach as well which tried to make the memberof plugin aware of the defaultOverrideName attribute. My wip tree is at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=memberof_defa... but so far it does not work properly. Additionally I would prefer to not touch the memberof plugin.
bye, Sumit
From 3a4ce5d188c2c97372aa92bb1e1596f8eca8c6e5 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Tue, 12 Jul 2016 17:09:00 +0200 Subject: [PATCH 1/4] IPA: make ipa_resolve_user_list_{send|recv} public and allow AD users
src/providers/ipa/ipa_id.c | 20 ++++++++++++++++---- src/providers/ipa/ipa_id.h | 8 ++++++++ 2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c index cfc25b9b322d78bbc64de96227a03ed0604d3391..7f0cd5689dbfde390c162eef340e1120247b703c 100644 --- a/src/providers/ipa/ipa_id.c +++ b/src/providers/ipa/ipa_id.c @@ -71,7 +71,7 @@ struct ipa_resolve_user_list_state { static errno_t ipa_resolve_user_list_get_user_step(struct tevent_req *req); static void ipa_resolve_user_list_get_user_done(struct tevent_req *subreq);
-static struct tevent_req * +struct tevent_req * ipa_resolve_user_list_send(TALLOC_CTX *memctx, struct tevent_context *ev, struct ipa_id_ctx *ipa_ctx, const char *domain_name, @@ -132,7 +132,14 @@ static errno_t ipa_resolve_user_list_get_user_step(struct tevent_req *req)
DEBUG(SSSDBG_TRACE_ALL, "Trying to resolve user [%s].\n", ar->filter_value);
- subreq = ipa_id_get_account_info_send(state, state->ev, state->ipa_ctx, ar);
- if (strcasecmp(state->domain_name,
state->ipa_ctx->sdap_id_ctx->be->domain->name) != 0) {subreq = ipa_subdomain_account_send(state, state->ev, state->ipa_ctx,ar);- } else {
subreq = ipa_id_get_account_info_send(state, state->ev, state->ipa_ctx,ar);- } if (subreq == NULL) { DEBUG(SSSDBG_OP_FAILURE, "sdap_handle_acct_req_send failed.\n"); return ENOMEM;
@@ -151,7 +158,12 @@ static void ipa_resolve_user_list_get_user_done(struct tevent_req *subreq) struct ipa_resolve_user_list_state); int ret;
- ret = ipa_id_get_account_info_recv(subreq, &state->dp_error);
- if (strcasecmp(state->domain_name,
state->ipa_ctx->sdap_id_ctx->be->domain->name) != 0) {ret = ipa_subdomain_account_recv(subreq, &state->dp_error);- } else {
ret = ipa_id_get_account_info_recv(subreq, &state->dp_error);- } talloc_zfree(subreq); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "sdap_handle_acct request failed: %d\n", ret);
@@ -182,7 +194,7 @@ done: return; }
-static int ipa_resolve_user_list_recv(struct tevent_req *req, int *dp_error) +int ipa_resolve_user_list_recv(struct tevent_req *req, int *dp_error) { struct ipa_resolve_user_list_state *state = tevent_req_data(req, struct ipa_resolve_user_list_state); diff --git a/src/providers/ipa/ipa_id.h b/src/providers/ipa/ipa_id.h index 410c2086833d811571195e05a7cab0e2c82fa5b9..4b25498821c6cd0d574c4ce78e4ece29f60459f7 100644 --- a/src/providers/ipa/ipa_id.h +++ b/src/providers/ipa/ipa_id.h @@ -135,4 +135,12 @@ struct tevent_req *ipa_get_subdom_acct_process_pac_send(TALLOC_CTX *mem_ctx, struct ldb_message *user_msg);
errno_t ipa_get_subdom_acct_process_pac_recv(struct tevent_req *req);
+struct tevent_req * +ipa_resolve_user_list_send(TALLOC_CTX *memctx, struct tevent_context *ev,
struct ipa_id_ctx *ipa_ctx,const char *domain_name,struct ldb_message_element *users);+int ipa_resolve_user_list_recv(struct tevent_req *req, int *dp_error);
#endif
2.1.0
From a47bc1d1acd0e8eae2ccdab0cb285c965cd1189d Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Tue, 12 Jul 2016 17:09:40 +0200 Subject: [PATCH 2/4] IPA: expand ghost members of AD groups in server-mode
src/providers/ipa/ipa_subdomains_id.c | 81 ++++++++++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c index 0a89ef5a68802fb7d5f9faeb9ef1d9f9cf8d14f2..a6cf3a9072c4cc6995a5813dd52397b90dc245c8 100644 --- a/src/providers/ipa/ipa_subdomains_id.c +++ b/src/providers/ipa/ipa_subdomains_id.c @@ -1201,6 +1201,69 @@ fail: return; }
+static void ipa_check_ghost_members_done(struct tevent_req *subreq); +static errno_t ipa_check_ghost_members(struct tevent_req *req) +{
- struct ipa_get_ad_acct_state *state = tevent_req_data(req,
struct ipa_get_ad_acct_state);- errno_t ret;
- struct tevent_req *subreq;
- struct ldb_message_element *ghosts = NULL;
- if (state->obj_msg == NULL) {
ret = get_object_from_cache(state, state->obj_dom, state->ar,&state->obj_msg);if (ret == ENOENT) {DEBUG(SSSDBG_MINOR_FAILURE,"Object not found, ending request\n");return EOK;} else if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE, "get_object_from_cache failed.\n");return ret;}- }
- ghosts = ldb_msg_find_element(state->obj_msg, SYSDB_GHOST);
- if (ghosts != NULL) {
/* Resolve ghost members */subreq = ipa_resolve_user_list_send(state, state->ev,state->ipa_ctx,state->obj_dom->name,ghosts);if (subreq == NULL) {DEBUG(SSSDBG_OP_FAILURE, "ipa_resolve_user_list_send failed.\n");return ENOMEM;}tevent_req_set_callback(subreq, ipa_check_ghost_members_done, req);return EAGAIN;- }
- return EOK;
+}
+static void ipa_check_ghost_members_done(struct tevent_req *subreq) +{
- struct tevent_req *req = tevent_req_callback_data(subreq,
struct tevent_req);- struct ipa_get_ad_acct_state *state = tevent_req_data(req,
struct ipa_get_ad_acct_state);- int ret;
- ret = ipa_resolve_user_list_recv(subreq, NULL);
- talloc_zfree(subreq);
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "ipa_resolve_user_list request failed [%d]\n",ret);tevent_req_error(req, ret);return;- }
- tevent_req_done(req);
- return;
+}
static errno_t ipa_get_ad_apply_override_step(struct tevent_req *req) { struct ipa_get_ad_acct_state *state = tevent_req_data(req, @@ -1228,11 +1291,27 @@ static errno_t ipa_get_ad_apply_override_step(struct tevent_req *req) entry_type = (state->ar->entry_type & BE_REQ_TYPE_MASK); if (entry_type != BE_REQ_INITGROUPS && entry_type != BE_REQ_USER
&& entry_type != BE_REQ_BY_SECID) {
&& entry_type != BE_REQ_BY_SECID&& entry_type != BE_REQ_GROUP) { tevent_req_done(req); return EOK;}
/* expand ghost members, if any, to get group members with overrides
* right. */if (entry_type == BE_REQ_GROUP) {
ret = ipa_check_ghost_members(req);if (ret == EOK) {tevent_req_done(req);return EOK;} else if (ret == EAGAIN) {return EOK;} else {DEBUG(SSSDBG_OP_FAILURE, "ipa_check_ghost_members failed.\n");return ret;}}
/* Replace ID with name in search filter */ if ((entry_type == BE_REQ_USER && state->ar->filter_type == BE_FILTER_IDNUM) || (entry_type == BE_REQ_INITGROUPS
-- 2.1.0
From a2f8440cf9848488353b320cf355a90529840879 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 20 Jul 2016 18:42:27 +0200 Subject: [PATCH 3/4] sysdb: add sysdb_get_user_members_recursively()
src/db/sysdb.h | 5 ++ src/db/sysdb_ops.c | 205 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 407ce3c18a7077e8fe45c3c9c7576ae626105122..489cb4d1b1af81711d93ed1eb0cb734955c3f8c0 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -1260,6 +1260,11 @@ errno_t sysdb_get_sids_of_members(TALLOC_CTX *mem_ctx, const char ***_dns, size_t *_n);
+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);errno_t sysdb_handle_original_uuid(const char *orig_name, struct sysdb_attrs *src_attrs, const char *src_name, diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 4755ea3427b99a51d73b7b9134e357cf2b987613..358e0bb6d493d313ff2e2280b92506f266d3e722 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -4706,6 +4706,211 @@ 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;
- struct ldb_result *res;
- hash_value_t *values;
- 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");goto done;- }
- ret = sss_hash_create(tmp_ctx, 10, &groups);
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "sss_hash_create failed.\n");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;- }
- res = talloc_zero(tmp_ctx, struct ldb_result);
- if (res == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_zero failed.\n");ret = ENOMEM;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 [%llu] 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);- }
- ret = EOK;
+done:
- if (ret == EOK) {
*members = talloc_steal(mem_ctx, res);- } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_FUNC, "No such entry\n");- } else {
DEBUG(SSSDBG_OP_FAILURE, "Error: %d (%s)\n", ret, strerror(ret));- }
- talloc_free(tmp_ctx);
- return ret;
+}
errno_t sysdb_handle_original_uuid(const char *orig_name, struct sysdb_attrs *src_attrs, const char *src_name, -- 2.1.0
From 1ebdd37552bac2ce05519efaf932946f79640a04 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Thu, 7 Jul 2016 18:54:02 +0200 Subject: [PATCH 4/4] views: properly override group member names
Resolves https://fedorahosted.org/sssd/ticket/2948
src/db/sysdb.h | 3 +- src/db/sysdb_search.c | 99 ++++++++++++++++------------- src/db/sysdb_views.c | 136 ++++++++++++++++++---------------------- src/responder/nss/nsssrv_cmd.c | 7 ++- src/tests/cmocka/test_nss_srv.c | 26 ++++---- 5 files changed, 138 insertions(+), 133 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 489cb4d1b1af81711d93ed1eb0cb734955c3f8c0..e10cf1bb26d31a0bb598a85ac38a931017c52600 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -575,7 +575,8 @@ errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain, const char **req_attrs);
errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
struct ldb_message *obj);
struct ldb_message *obj,bool expect_override_dn);errno_t sysdb_getpwnam_with_views(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index e40b36c38e28992e185447497d1bf69cabc09821..cfee5784dbadd692f30d0758e7e5c3c9fb2814cb 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -771,28 +771,33 @@ int sysdb_getgrnam_with_views(TALLOC_CTX *mem_ctx,
/* If there are views we have to check if override values must be added to * the original object. */
- if (DOM_HAS_VIEWS(domain) && orig_obj->count == 1) {
if (!is_local_view(domain->view_name)) {el = ldb_msg_find_element(orig_obj->msgs[0], SYSDB_GHOST);if (el != NULL && el->num_values != 0) {DEBUG(SSSDBG_TRACE_ALL, "Group object [%s], contains ghost ""entries which must be resolved before overrides can be ""applied.\n",ldb_dn_get_linearized(orig_obj->msgs[0]->dn));ret = ENOENT;
- if (orig_obj->count == 1) {
if (DOM_HAS_VIEWS(domain)) {if (!is_local_view(domain->view_name)) {el = ldb_msg_find_element(orig_obj->msgs[0], SYSDB_GHOST);if (el != NULL && el->num_values != 0) {DEBUG(SSSDBG_TRACE_ALL, "Group object [%s], contains ghost ""entries which must be resolved before overrides can be ""applied.\n",ldb_dn_get_linearized(orig_obj->msgs[0]->dn));ret = ENOENT;goto done;}}ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],override_obj == NULL ? NULL : override_obj ->msgs[0],NULL);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n"); goto done; } }
ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],override_obj == NULL ? NULL : override_obj ->msgs[0],NULL);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n");goto done;}ret = sysdb_add_group_member_overrides(domain, orig_obj->msgs[0]);
/* Must be called even without views to check to* SYSDB_DEFAULT_OVERRIDE_NAME */ret = sysdb_add_group_member_overrides(domain, orig_obj->msgs[0],DOM_HAS_VIEWS(domain)); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_group_member_overrides failed.\n");@@ -922,28 +927,33 @@ int sysdb_getgrgid_with_views(TALLOC_CTX *mem_ctx,
/* If there are views we have to check if override values must be added to * the original object. */
- if (DOM_HAS_VIEWS(domain) && orig_obj->count == 1) {
if (!is_local_view(domain->view_name)) {el = ldb_msg_find_element(orig_obj->msgs[0], SYSDB_GHOST);if (el != NULL && el->num_values != 0) {DEBUG(SSSDBG_TRACE_ALL, "Group object [%s], contains ghost ""entries which must be resolved before overrides can be ""applied.\n",ldb_dn_get_linearized(orig_obj->msgs[0]->dn));ret = ENOENT;
- if (orig_obj->count == 1) {
if (DOM_HAS_VIEWS(domain)) {if (!is_local_view(domain->view_name)) {el = ldb_msg_find_element(orig_obj->msgs[0], SYSDB_GHOST);if (el != NULL && el->num_values != 0) {DEBUG(SSSDBG_TRACE_ALL, "Group object [%s], contains ghost ""entries which must be resolved before overrides can be ""applied.\n",ldb_dn_get_linearized(orig_obj->msgs[0]->dn));ret = ENOENT;goto done;}}ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],override_obj == NULL ? NULL : override_obj ->msgs[0],NULL);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n"); goto done; } }
ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],override_obj == NULL ? NULL : override_obj ->msgs[0],NULL);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n");goto done;}ret = sysdb_add_group_member_overrides(domain, orig_obj->msgs[0]);
/* Must be called even without views to check to* SYSDB_DEFAULT_OVERRIDE_NAME */ret = sysdb_add_group_member_overrides(domain, orig_obj->msgs[0],DOM_HAS_VIEWS(domain)); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_group_member_overrides failed.\n");@@ -1157,8 +1167,8 @@ int sysdb_enumgrent_filter_with_views(TALLOC_CTX *mem_ctx, goto done; }
- if (DOM_HAS_VIEWS(domain)) {
for (c = 0; c < res->count; c++) {
- for (c = 0; c < res->count; c++) {
if (DOM_HAS_VIEWS(domain)) { ret = sysdb_add_overrides_to_object(domain, res->msgs[c], NULL, NULL); /* enumeration assumes that the cache is up-to-date, hence we do not@@ -1167,13 +1177,14 @@ int sysdb_enumgrent_filter_with_views(TALLOC_CTX *mem_ctx, DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n"); goto done; }
}
ret = sysdb_add_group_member_overrides(domain, res->msgs[c]);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE,"sysdb_add_group_member_overrides failed.\n");goto done;}
ret = sysdb_add_group_member_overrides(domain, res->msgs[c],DOM_HAS_VIEWS(domain));if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE,"sysdb_add_group_member_overrides failed.\n");goto done; }diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index 2b89e5ca41f719e1217ef3b9e0fd683656e05d42..79f513d13ba41212a6cd84e1d9e609df6acba29c 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -1348,14 +1348,13 @@ done: }
errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
struct ldb_message *obj)
struct ldb_message *obj,bool expect_override_dn){ int ret; size_t c;
- struct ldb_message_element *members;
- struct ldb_result *res_members; TALLOC_CTX *tmp_ctx;
- struct ldb_dn *member_dn;
- struct ldb_result *member_obj; struct ldb_result *override_obj; static const char *member_attrs[] = SYSDB_PW_ATTRS; const char *override_dn_str;
@@ -1366,12 +1365,6 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, char *val; struct sss_domain_info *orig_dom;
- members = ldb_msg_find_element(obj, SYSDB_MEMBER);
- if (members == NULL || members->num_values == 0) {
DEBUG(SSSDBG_TRACE_ALL, "Group has no members.\n");return EOK;- }
- tmp_ctx = talloc_new(NULL); if (tmp_ctx == NULL) { DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
@@ -1379,38 +1372,30 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, goto done; }
- for (c = 0; c < members->num_values; c++) {
member_dn = ldb_dn_from_ldb_val(tmp_ctx, domain->sysdb->ldb,&members->values[c]);if (member_dn == NULL) {DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_from_ldb_val failed.\n");ret = ENOMEM;goto done;}
- ret = sysdb_get_user_members_recursively(tmp_ctx, domain, obj->dn,
&res_members);- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,"sysdb_get_user_members_recursively failed.\n");goto done;- }
ret = ldb_search(domain->sysdb->ldb, member_dn, &member_obj, member_dn,LDB_SCOPE_BASE, member_attrs, NULL);if (ret != LDB_SUCCESS) {ret = sysdb_error_to_errno(ret);goto done;}
- for (c = 0; c < res_members->count; c++) {
if (member_obj->count != 1) {DEBUG(SSSDBG_CRIT_FAILURE,"Base search for member object returned [%d] results.\n",member_obj->count);ret = EINVAL;goto done;}if (ldb_msg_find_attr_as_uint64(member_obj->msgs[0],
if (ldb_msg_find_attr_as_uint64(res_members->msgs[c], SYSDB_UIDNUM, 0) == 0) { /* Skip non-POSIX-user members i.e. groups and non-POSIX users */ continue; }
override_dn_str = ldb_msg_find_attr_as_string(member_obj->msgs[0],SYSDB_OVERRIDE_DN, NULL);
if (expect_override_dn) {override_dn_str = ldb_msg_find_attr_as_string(res_members->msgs[c],SYSDB_OVERRIDE_DN,NULL);} else {override_dn_str = ldb_dn_get_linearized(res_members->msgs[c]->dn);}if (override_dn_str == NULL) { if (is_local_view(domain->view_name)) { /* LOCAL view doesn't have to have overrideDN specified. */@@ -1420,12 +1405,12 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
DEBUG(SSSDBG_CRIT_FAILURE, "Missing override DN for object [%s].\n",
ldb_dn_get_linearized(member_obj->msgs[0]->dn));
ldb_dn_get_linearized(res_members->msgs[c]->dn)); ret = ENOENT; goto done; }
override_dn = ldb_dn_new(member_obj, domain->sysdb->ldb,
override_dn = ldb_dn_new(res_members, domain->sysdb->ldb, override_dn_str); if (override_dn == NULL) { DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_new failed.\n");@@ -1433,22 +1418,27 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, goto done; }
orig_name = ldb_msg_find_attr_as_string(member_obj->msgs[0],
orig_name = ldb_msg_find_attr_as_string(res_members->msgs[c], SYSDB_NAME, NULL); if (orig_name == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Object [%s] has no name.\n",
ldb_dn_get_linearized(member_obj->msgs[0]->dn));
ldb_dn_get_linearized(res_members->msgs[c]->dn)); ret = EINVAL; goto done; }
memberuid = NULL;if (ldb_dn_compare(member_obj->msgs[0]->dn, override_dn) != 0) {
/* start with default view name, if it exists or use NULL */memberuid = ldb_msg_find_attr_as_string(res_members->msgs[c],SYSDB_DEFAULT_OVERRIDE_NAME,NULL);/* If there is an override object, check if the name is overridden */if (ldb_dn_compare(res_members->msgs[c]->dn, override_dn) != 0) { DEBUG(SSSDBG_TRACE_ALL, "Checking override for object [%s].\n",
ldb_dn_get_linearized(member_obj->msgs[0]->dn));
ldb_dn_get_linearized(res_members->msgs[c]->dn));
ret = ldb_search(domain->sysdb->ldb, member_obj, &override_obj,
ret = ldb_search(domain->sysdb->ldb, res_members, &override_obj, override_dn, LDB_SCOPE_BASE, member_attrs, NULL); if (ret != LDB_SUCCESS) { ret = sysdb_error_to_errno(ret);@@ -1458,43 +1448,44 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, if (override_obj->count != 1) { DEBUG(SSSDBG_CRIT_FAILURE, "Base search for override object returned [%d] results.\n",
member_obj->count);
override_obj->count); ret = EINVAL; goto done; } memberuid = ldb_msg_find_attr_as_string(override_obj->msgs[0], SYSDB_NAME,
NULL);
memberuid);}
if (memberuid != NULL) {ret = sss_parse_internal_fqname(tmp_ctx, orig_name,NULL, &orig_domain);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE,"sss_parse_internal_fqname failed to split [%s].\n",orig_name);
/* add domain name if memberuid is a short name */if (memberuid != NULL && strchr(memberuid, '@') == NULL) {ret = sss_parse_internal_fqname(tmp_ctx, orig_name,NULL, &orig_domain);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE,"sss_parse_internal_fqname failed to split [%s].\n",orig_name);goto done;}if (orig_domain != NULL) {orig_dom = find_domain_by_name(get_domains_head(domain),orig_domain, true);if (orig_dom == NULL) {DEBUG(SSSDBG_CRIT_FAILURE,"Cannot find domain with name [%s].\n",orig_domain);ret = ERR_DOMAIN_NOT_FOUND; goto done; }
if (orig_domain != NULL) {orig_dom = find_domain_by_name(get_domains_head(domain),orig_domain, true);if (orig_dom == NULL) {DEBUG(SSSDBG_CRIT_FAILURE,"Cannot find domain with name [%s].\n",orig_domain);ret = ERR_DOMAIN_NOT_FOUND;goto done;}memberuid = sss_create_internal_fqname(tmp_ctx, memberuid,orig_dom->name);if (memberuid == NULL) {DEBUG(SSSDBG_OP_FAILURE,"sss_create_internal_fqname failed.\n");ret = ENOMEM;goto done;}
memberuid = sss_create_internal_fqname(tmp_ctx, memberuid,orig_dom->name);if (memberuid == NULL) {DEBUG(SSSDBG_OP_FAILURE,"sss_create_internal_fqname failed.\n");ret = ENOMEM;goto done; } } }@@ -1521,9 +1512,6 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, DEBUG(SSSDBG_TRACE_ALL, "Added [%s] to [%s].\n", memberuid, OVERRIDE_PREFIX SYSDB_MEMBERUID);
/* Free all temporary data of the current member to avoid memory usage* spikes. All temporary data should be allocated below member_dn. */talloc_free(member_dn);}
ret = EOK;
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 1ae17969688fa29734ca14fd2b152decef1fdbca..4e84b3202cbf367e70a47a3c7edb06e357657538 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -2976,7 +2976,12 @@ static int fill_grent(struct sss_packet *packet,
memnum = 0; if (!dom->ignore_group_members) {
el = sss_view_ldb_msg_find_element(dom, msg, SYSDB_MEMBERUID);
/* unconditionally prefer OVERRIDE_PREFIX SYSDB_MEMBERUID, it* might contain override names from the default view */el = ldb_msg_find_element(msg, OVERRIDE_PREFIX SYSDB_MEMBERUID);if (el == NULL) {el = ldb_msg_find_element(msg, SYSDB_MEMBERUID);} if (el) { ret = fill_members(packet, nctx->rctx, dom, nctx, el, &rzero, &rsize, &memnum);diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c index 82a304feed864b09168d0f3e06a4e1bb120df7e4..892bebe63816a0dac0317dd1fd0135523f0e64d7 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[] = { testmember1.pw_name,
testmember2.pw_name };
- const char *exp_members[] = { testmember2.pw_name,
testmember1.pw_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, testmember1.pw_name);
nss_test_ctx->tctx->dom, testmember2.pw_name);
nss_test_ctx->tctx->dom, testmember2.pw_name);
nss_test_ctx->tctx->dom, testmember1.pw_name);assert_non_null(exp_members[1]);
expected.gr_name = sss_tc_fqname(tmp_ctx,
@@ -1619,8 +1619,8 @@ 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] = testmember1.pw_name;
- exp_members[1] = testmember2.pw_name;
- exp_members[0] = testmember2.pw_name;
- 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]);
@@ -1683,10 +1683,10 @@ static int test_nss_getgrnam_check_mix_dom_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, testmember1.pw_name);
nss_test_ctx->tctx->dom, testmember2.pw_name);
nss_test_ctx->tctx->dom, testmember2.pw_name);
nss_test_ctx->tctx->dom, testmember1.pw_name);@@ -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[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->subdom->names,
nss_test_ctx->subdom, submember2.pw_name);
nss_test_ctx->subdom, submember1.pw_name);- exp_members[2] = 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[2] = testmember1.pw_name;
exp_members[0] = 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
On Mon, Jul 25, 2016 at 01:45:13PM +0200, Jakub Hrozek wrote:
On Thu, Jul 21, 2016 at 02:13:40PM +0200, Sumit Bose wrote:
Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
So far I read them to get a grasp of what they do, but didn't do a full review. See some comments below.
I also submitted the patches to Coverity which found a warning: Error: COMPILER_WARNING: sssd-1.14.1/src/util/util.h:54: included_from: Included from here. sssd-1.14.1/src/db/sysdb_ops.c:22: included_from: Included from here. sssd-1.14.1/src/db/sysdb_ops.c: scope_hint: In function 'sysdb_get_user_members_recursively' sssd-1.14.1/src/db/sysdb_ops.c:4879:36: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 6 has type 'long unsigned int' [-Wformat=] # DEBUG(SSSDBG_CRIT_FAILURE, "More then [%llu] results.\n", UINTMAX_MAX); # ^ sssd-1.14.1/src/util/debug.h:108:22: note: in definition of macro 'DEBUG' # format, ##__VA_ARGS__); \ # ^ # 4877| res->count = count; # 4878| } else { # 4879|-> DEBUG(SSSDBG_CRIT_FAILURE, "More then [%llu] results.\n", UINTMAX_MAX); # 4880| ret = EINVAL; # 4881| goto done;
Error: COMPILER_WARNING: sssd-1.14.1/src/providers/ipa/ipa_subdomains_id.c: scope_hint: In function 'ipa_check_ghost_members_done' sssd-1.14.1/src/providers/ipa/ipa_subdomains_id.c:1250:35: warning: unused variable 'state' [-Wunused-variable] # struct ipa_get_ad_acct_state *state = tevent_req_data(req, # ^ # 1248| struct tevent_req *req = tevent_req_callback_data(subreq, # 1249| struct tevent_req); # 1250|-> struct ipa_get_ad_acct_state *state = tevent_req_data(req, # 1251| struct ipa_get_ad_acct_state); # 1252| int ret;
On (25/07/16 19:26), Jakub Hrozek wrote:
On Mon, Jul 25, 2016 at 01:45:13PM +0200, Jakub Hrozek wrote:
On Thu, Jul 21, 2016 at 02:13:40PM +0200, Sumit Bose wrote:
Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
So far I read them to get a grasp of what they do, but didn't do a full review. See some comments below.
I also submitted the patches to Coverity which found a warning: Error: COMPILER_WARNING:
Jakub, As you can see it's not a coverity warning but COMPILER_WARNING(gcc)
sssd-1.14.1/src/util/util.h:54: included_from: Included from here. sssd-1.14.1/src/db/sysdb_ops.c:22: included_from: Included from here. sssd-1.14.1/src/db/sysdb_ops.c: scope_hint: In function 'sysdb_get_user_members_recursively' sssd-1.14.1/src/db/sysdb_ops.c:4879:36: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 6 has type 'long unsigned int' [-Wformat=] # DEBUG(SSSDBG_CRIT_FAILURE, "More then [%llu] results.\n", UINTMAX_MAX);
^^^^ According to inttypes.h there should be PRIuMAX
But Do we really need UINTMAX_MAX? or should we rather us UINT64_MAX
LS
On Mon, Jul 25, 2016 at 09:28:04PM +0200, Lukas Slebodnik wrote:
On (25/07/16 19:26), Jakub Hrozek wrote:
On Mon, Jul 25, 2016 at 01:45:13PM +0200, Jakub Hrozek wrote:
On Thu, Jul 21, 2016 at 02:13:40PM +0200, Sumit Bose wrote:
Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
So far I read them to get a grasp of what they do, but didn't do a full review. See some comments below.
I also submitted the patches to Coverity which found a warning: Error: COMPILER_WARNING:
Jakub, As you can see it's not a coverity warning but COMPILER_WARNING(gcc)
sssd-1.14.1/src/util/util.h:54: included_from: Included from here. sssd-1.14.1/src/db/sysdb_ops.c:22: included_from: Included from here. sssd-1.14.1/src/db/sysdb_ops.c: scope_hint: In function 'sysdb_get_user_members_recursively' sssd-1.14.1/src/db/sysdb_ops.c:4879:36: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 6 has type 'long unsigned int' [-Wformat=] # DEBUG(SSSDBG_CRIT_FAILURE, "More then [%llu] results.\n", UINTMAX_MAX);
^^^^ According to inttypes.h there should be PRIuMAX
Thanks for the right specifier.
But Do we really need UINTMAX_MAX? or should we rather us UINT64_MAX
I think UINTMAX_MAX is right here because I want to assign an unsigned long (return value of hash_values()) to an unsigned int (struct member of ldb_result). To make sure it fits I compare with UINTMAX_MAX.
bye, Sumit
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On Mon, Jul 25, 2016 at 01:45:13PM +0200, Jakub Hrozek wrote:
On Thu, Jul 21, 2016 at 02:13:40PM +0200, Sumit Bose wrote:
Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
So far I read them to get a grasp of what they do, but didn't do a full review. See some comments below.
The first two patches are for the IPA provider and make sure that all ghost members in a group get resolved because otherwise we cannot determine if the name is overridden or not. This adds an overhead to group lookups, especially for larger groups but I think it is an requirement which cannot be skipped.
Right. But I wonder if we could skip it on-demand. Could we maybe check if any views exist in the IPA domain at all and not resolve the members if there are no idview data? I would guess that since this is SSSD on the IPA server, then an additiona LDAP lookup might be cheaper than resolving a full large group.
Yes, that would be possible. But since this is about AD users I would prefer a different change.
Since AD used RFC2307bis (member attribute with the DN of the member) we already read the user object from the AD GC when resolving the group but only ask for objectclass and samAccountName to be able to fill the ghost entry. If we would switch to the LDAP port, read the whole entry and save it to the chance there wouldn't be any ghost entries in the first place. But I wanted to avoid regressions in the common SDAP code at this time so I resolved the ghosts in a second step.
But I guess it is worth to open a ticket to add either or both kinds of optimization.
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.
bye, Sumit
Finally the forth patch applies the code-path which is already used for non-default views to the default case as well and adds a new list of members, with correctly overridden names (hopefully :-) which is then used in fill_grmem(). This adds some overhead to the overall group processing in the NSS responder (as can be seen in the test changes because the members are returned in different order in some cases). But I think because the of memory cache this is acceptable and might even help to remove the memberuid attribute in future and make the memberof plugin simpler.
I worked on an alternative approach as well which tried to make the memberof plugin aware of the defaultOverrideName attribute. My wip tree is at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=memberof_defa... but so far it does not work properly. Additionally I would prefer to not touch the memberof plugin.
bye, Sumit
On Tue, Jul 26, 2016 at 01:51:56PM +0200, Sumit Bose wrote:
On Mon, Jul 25, 2016 at 01:45:13PM +0200, Jakub Hrozek wrote:
On Thu, Jul 21, 2016 at 02:13:40PM +0200, Sumit Bose wrote:
Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
So far I read them to get a grasp of what they do, but didn't do a full review. See some comments below.
The first two patches are for the IPA provider and make sure that all ghost members in a group get resolved because otherwise we cannot determine if the name is overridden or not. This adds an overhead to group lookups, especially for larger groups but I think it is an requirement which cannot be skipped.
Right. But I wonder if we could skip it on-demand. Could we maybe check if any views exist in the IPA domain at all and not resolve the members if there are no idview data? I would guess that since this is SSSD on the IPA server, then an additiona LDAP lookup might be cheaper than resolving a full large group.
Yes, that would be possible. But since this is about AD users I would prefer a different change.
Since AD used RFC2307bis (member attribute with the DN of the member) we already read the user object from the AD GC when resolving the group but only ask for objectclass and samAccountName to be able to fill the ghost entry. If we would switch to the LDAP port, read the whole entry and save it to the chance there wouldn't be any ghost entries in the first place. But I wanted to avoid regressions in the common SDAP code at this time so I resolved the ghosts in a second step.
But I guess it is worth to open a ticket to add either or both kinds of optimization.
OK, I hope with the timestamp cache, the performance impact would only be during the first lookup.
Should I open that ticket or would you?
On Tue, Jul 26, 2016 at 01:56:38PM +0200, Jakub Hrozek wrote:
On Tue, Jul 26, 2016 at 01:51:56PM +0200, Sumit Bose wrote:
On Mon, Jul 25, 2016 at 01:45:13PM +0200, Jakub Hrozek wrote:
On Thu, Jul 21, 2016 at 02:13:40PM +0200, Sumit Bose wrote:
Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
So far I read them to get a grasp of what they do, but didn't do a full review. See some comments below.
The first two patches are for the IPA provider and make sure that all ghost members in a group get resolved because otherwise we cannot determine if the name is overridden or not. This adds an overhead to group lookups, especially for larger groups but I think it is an requirement which cannot be skipped.
Right. But I wonder if we could skip it on-demand. Could we maybe check if any views exist in the IPA domain at all and not resolve the members if there are no idview data? I would guess that since this is SSSD on the IPA server, then an additiona LDAP lookup might be cheaper than resolving a full large group.
Yes, that would be possible. But since this is about AD users I would prefer a different change.
Since AD used RFC2307bis (member attribute with the DN of the member) we already read the user object from the AD GC when resolving the group but only ask for objectclass and samAccountName to be able to fill the ghost entry. If we would switch to the LDAP port, read the whole entry and save it to the chance there wouldn't be any ghost entries in the first place. But I wanted to avoid regressions in the common SDAP code at this time so I resolved the ghosts in a second step.
But I guess it is worth to open a ticket to add either or both kinds of optimization.
OK, I hope with the timestamp cache, the performance impact would only be during the first lookup.
Should I open that ticket or would you?
Please open a ticket.
New version of the patches attached.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
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.
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.
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
On Tue, Jul 26, 2016 at 10:05:21PM +0200, Sumit Bose wrote:
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.
Thank you, this patch works for me as well and Coverity is clean as well. If you agree, I would squash the last patch into the one that adds sysdb_get_user_members_recursively() and then push once CI finishes.
(Also, ACK)
On Wed, Jul 27, 2016 at 11:11:48AM +0200, Jakub Hrozek wrote:
On Tue, Jul 26, 2016 at 10:05:21PM +0200, Sumit Bose wrote:
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.
Thank you, this patch works for me as well and Coverity is clean as well. If you agree, I would squash the last patch into the one that adds sysdb_get_user_members_recursively() and then push once CI finishes.
(Also, ACK)
Actually, tests fail after adding the memberof patch: [ FAILED ] test_nss_getgrnam_mix_subdom
On Wed, Jul 27, 2016 at 11:36:09AM +0200, Jakub Hrozek wrote:
On Wed, Jul 27, 2016 at 11:11:48AM +0200, Jakub Hrozek wrote:
On Tue, Jul 26, 2016 at 10:05:21PM +0200, Sumit Bose wrote:
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.
Thank you, this patch works for me as well and Coverity is clean as well. If you agree, I would squash the last patch into the one that adds sysdb_get_user_members_recursively() and then push once CI finishes.
(Also, ACK)
Actually, tests fail after adding the memberof patch: [ FAILED ] test_nss_getgrnam_mix_subdom
This turned out to be an off-by-one bug in the test. I'll fix it before pushing. Otherwise ACK, waiting for CI to finish before I push the patches.
On Wed, Jul 27, 2016 at 06:56:15PM +0200, Jakub Hrozek wrote:
On Wed, Jul 27, 2016 at 11:36:09AM +0200, Jakub Hrozek wrote:
On Wed, Jul 27, 2016 at 11:11:48AM +0200, Jakub Hrozek wrote:
On Tue, Jul 26, 2016 at 10:05:21PM +0200, Sumit Bose wrote:
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.
Thank you, this patch works for me as well and Coverity is clean as well. If you agree, I would squash the last patch into the one that adds sysdb_get_user_members_recursively() and then push once CI finishes.
(Also, ACK)
Actually, tests fail after adding the memberof patch: [ FAILED ] test_nss_getgrnam_mix_subdom
This turned out to be an off-by-one bug in the test. I'll fix it before pushing. Otherwise ACK, waiting for CI to finish before I push the patches.
On Thu, Jul 28, 2016 at 09:41:18AM +0200, Jakub Hrozek wrote:
On Wed, Jul 27, 2016 at 06:56:15PM +0200, Jakub Hrozek wrote:
On Wed, Jul 27, 2016 at 11:36:09AM +0200, Jakub Hrozek wrote:
On Wed, Jul 27, 2016 at 11:11:48AM +0200, Jakub Hrozek wrote:
On Tue, Jul 26, 2016 at 10:05:21PM +0200, Sumit Bose wrote:
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.
Thank you, this patch works for me as well and Coverity is clean as well. If you agree, I would squash the last patch into the one that adds sysdb_get_user_members_recursively() and then push once CI finishes.
(Also, ACK)
Actually, tests fail after adding the memberof patch: [ FAILED ] test_nss_getgrnam_mix_subdom
This turned out to be an off-by-one bug in the test. I'll fix it before pushing. Otherwise ACK, waiting for CI to finish before I push the patches.
* master: * f2e8a7c3230fac11175c0bd17c14c66a8e9b25ad * 160ba891ec483c5b7d2a3fcca5bd992fc790efe0 * 17bfd9f69251781140e4b2b55ffeb649d7a79e86 * 1594701fbdc341069e11cff9a85e7a795e52db3d
On (26/07/16 22:05), Sumit Bose wrote:
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
From ee8ebcec8062d75e98da796b291973fd96d45b1c Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@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(-)
Jakub,
is there a reason why this patch has not been pushed to master yet?
LS
On Tue, Nov 08, 2016 at 08:14:20AM +0100, Lukas Slebodnik wrote:
On (26/07/16 22:05), Sumit Bose wrote:
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
From ee8ebcec8062d75e98da796b291973fd96d45b1c Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@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(-)
Jakub,
is there a reason why this patch has not been pushed to master yet?
Please see the earlier replies in this thread. We agreed with Sumit to squash this patch into the previous ones, so if you take a look at 17bfd9f69251781140e4b2b55ffeb649d7a79e86 it already uses memberof search.
Hi,
please find attached a backport to 1.13 of the given patch set including fixes from Lukas.
It's intention is to fix https://fedorahosted.org/sssd/ticket/3118 / https://bugzilla.redhat.com/show_bug.cgi?id=1367802 without introducing a new patch. As a side-effect the issue with missing overriden names from group members is fixed as well. But since the patches touches various parts of the group lookup code it should be carefully tested for regressions.
bye, Sumit
On Thu, Jul 21, 2016 at 02:13:40PM +0200, Sumit Bose wrote:
Hi,
this is my suggestion to solve https://fedorahosted.org/sssd/ticket/2948 "Handle overriden name of members in the memberUid attribute".
The first two patches are for the IPA provider and make sure that all ghost members in a group get resolved because otherwise we cannot determine if the name is overridden or not. This adds an overhead to group lookups, especially for larger groups but I think it is an requirement which cannot be skipped.
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.
Finally the forth patch applies the code-path which is already used for non-default views to the default case as well and adds a new list of members, with correctly overridden names (hopefully :-) which is then used in fill_grmem(). This adds some overhead to the overall group processing in the NSS responder (as can be seen in the test changes because the members are returned in different order in some cases). But I think because the of memory cache this is acceptable and might even help to remove the memberuid attribute in future and make the memberof plugin simpler.
I worked on an alternative approach as well which tried to make the memberof plugin aware of the defaultOverrideName attribute. My wip tree is at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=memberof_defa... but so far it does not work properly. Additionally I would prefer to not touch the memberof plugin.
bye, Sumit
On (24/10/16 16:59), Sumit Bose wrote:
Hi,
please find attached a backport to 1.13 of the given patch set including fixes from Lukas.
It's intention is to fix https://fedorahosted.org/sssd/ticket/3118 / https://bugzilla.redhat.com/show_bug.cgi?id=1367802 without introducing a new patch. As a side-effect the issue with missing overriden names from group members is fixed as well. But since the patches touches various parts of the group lookup code it should be carefully tested for regressions.
ACK
http://sssd-ci.duckdns.org/logs/job/56/38/summary.html
LS
On (08/11/16 10:19), Lukas Slebodnik wrote:
On (24/10/16 16:59), Sumit Bose wrote:
Hi,
please find attached a backport to 1.13 of the given patch set including fixes from Lukas.
It's intention is to fix https://fedorahosted.org/sssd/ticket/3118 / https://bugzilla.redhat.com/show_bug.cgi?id=1367802 without introducing a new patch. As a side-effect the issue with missing overriden names from group members is fixed as well. But since the patches touches various parts of the group lookup code it should be carefully tested for regressions.
ACK
sssd-1-13: * 55fc0bb19e6205af13828a98592b283d3b6d24e0 * 19ba10fcc7dbdfdd7a238fa94f57605cf16fc28e * 8e19dce22b286f1f815cba7150149ab249a62854 * 5d64343d5ffed9cb42184eb30e5bf1871d8196d5 * ce714745ad28dfb6dcfd9f8f8983e492661a6e2f * 3bea6818a3432a349a9901a84fd517c052b19f69
LS
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Sumit Bose