On Wed, Dec 17, 2014 at 11:47:59AM +0100, Sumit Bose wrote:
On Wed, Dec 17, 2014 at 09:33:32AM +0100, Lukas Slebodnik wrote:
> On (17/12/14 09:31), Lukas Slebodnik wrote:
> >On (11/12/14 11:10), Sumit Bose wrote:
> >>Hi,
> >>
> >>this patch set contains some improvements for the view related code on
> >>IPA client. Most of them are performance related, patches 4,5 and 6 make
> >>sure that during group requests all members are properly resovled if a
> >>non-default view is applied to be able to handle user name overrides.
> >>
> >>bye,
> >>Sumit
> >
> >>From c6dcdb109a1d6daa8f91b197a29f4551d9de8d8a Mon Sep 17 00:00:00 2001
> >>From: Sumit Bose <sbose(a)redhat.com>
> >>Date: Fri, 5 Dec 2014 11:11:49 +0100
> >>Subject: [PATCH 4/7] IPA: process_members() optionally return missing
members
> >> list
> >>
> >>---
> >> src/providers/ipa/ipa_s2n_exop.c | 85
+++++++++++++++++++++++++++++++---------
> >> 1 file changed, 67 insertions(+), 18 deletions(-)
> >>
> >There is a warning from static analyzer. Currently it is false possitive and
> >it isn't big chance to do such mistake, but small change in code can fix
it.
> >See html report: left_operand_is_garbage.html.gz
> >
> And one more time with attachment :-)
Thank you, fixed version attached.
bye,
Sumit
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
The patches are mostly good, but I see some problems even without them,
so maybe it makes sense to discuss them anyway.
When the cache is empty and I run "id -G $user", the groups are saved to
the cache with DNs looking like this:
name=unigroup(a)AD.EXAMPLE.COM,cn=groups,cn=AD.EXAMPLE.COM,cn=sysdb
But later when adding the user to groups, the member-add function looks
for:
name=unigroup(a)ad.example.com,cn=groups,cn=AD.EXAMPLE.COM,cn=sysdb
Looks like there is a casing mismatch...I know we made originalDN
case-insensitive but most probably not the sysdb DN. The full log
message is:
(Fri Jan 9 14:25:19 2015) [sssd[be[ipa.example.com]]] [sysdb_update_members_ex]
(0x0020): Could not add member [psuser(a)AD.EXAMPLE.COM] to group
[name=unigroup(a)ad.example.com,cn=groups,cn=AD.EXAMPLE.COM,cn=sysdb]. Skipping.
Some comments on the patches inline..
From 3911cf48ca4f0788476ce8fb50c175dbd0d83e4f Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Thu, 4 Dec 2014 13:26:32 +0100
Subject: [PATCH 1/7] IPA: do not look up overrides on client with default view
The IPA extdom plugin returns the data with the default view already
applied hence it is on needed to look up the override data if the client
has the default view assigned.
ACK
From affa294d04ed7fef4c1d9cfd92098f22f91600ea Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Fri, 5 Dec 2014 11:03:48 +0100
Subject: [PATCH 2/7] IPA: make version check more precise
The call protected by the check does not only expect the version 1 of
the extdom plugin is used but a specific response type as well. Since
version 1 can return older response types as well we want to be on the
safe side.
ACK
From 37cc08b7d2a678f31191794e653aa89202148962 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Fri, 5 Dec 2014 11:06:26 +0100
Subject: [PATCH 3/7] IPA: add missing break
The current request already returned the SID, we do not need to request
it separately.
ACK
From 6f0f58448b86052c848b78151dbc39b0d3c99737 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Fri, 5 Dec 2014 11:11:49 +0100
Subject: [PATCH 4/7] IPA: process_members() optionally return missing members
list
---
src/providers/ipa/ipa_s2n_exop.c | 84 +++++++++++++++++++++++++++++++---------
1 file changed, 66 insertions(+), 18 deletions(-)
diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c
index 6bf492623f638a9900f997a937e05cd66fca09a2..3b02e67e36caa8ee05a702dc3434c965ecee4682
100644
--- a/src/providers/ipa/ipa_s2n_exop.c
+++ b/src/providers/ipa/ipa_s2n_exop.c
@@ -1222,7 +1222,8 @@ fail:
static errno_t process_members(struct sss_domain_info *domain,
struct sysdb_attrs *group_attrs,
- char **members)
+ char **members,
+ TALLOC_CTX *mem_ctx, char ***_missing_members)
{
int ret;
size_t c;
@@ -1231,6 +1232,8 @@ static errno_t process_members(struct sss_domain_info *domain,
const char *dn_str;
struct sss_domain_info *obj_domain;
struct sss_domain_info *parent_domain;
+ char **missing_members = NULL;
+ size_t miss_count = 0;
if (members == NULL) {
DEBUG(SSSDBG_TRACE_INTERNAL, "No members\n");
I think you should also set *_missing_members to NULL if there are no
members.
From c111a183e3ceda85781bf3383a869bf8d8e555d9 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 9 Dec 2014 17:04:30 +0100
Subject: [PATCH 5/7] IPA: rename ipa_s2n_get_groups_send() to
ipa_s2n_get_fqlist_send()
---
src/providers/ipa/ipa_s2n_exop.c | 100 +++++++++++++++++++--------------------
1 file changed, 50 insertions(+), 50 deletions(-)
diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c
index 3b02e67e36caa8ee05a702dc3434c965ecee4682..f3a6aa99408d2df3448a9642ef99b5f2b7082248
100644
--- a/src/providers/ipa/ipa_s2n_exop.c
+++ b/src/providers/ipa/ipa_s2n_exop.c
@@ -877,38 +877,38 @@ done:
return ret;
}
-struct ipa_s2n_get_groups_state {
+struct ipa_s2n_get_fqlist_state {
struct tevent_context *ev;
struct ipa_id_ctx *ipa_ctx;
struct sss_domain_info *dom;
struct sdap_handle *sh;
struct req_input req_input;
- char **group_list;
- size_t group_idx;
+ char **fqname_list;
+ size_t fqname_idx;
int exop_timeout;
struct resp_attrs *attrs;
struct sss_domain_info *obj_domain;
struct sysdb_attrs *override_attrs;
};
-static errno_t ipa_s2n_get_groups_step(struct tevent_req *req);
-static void ipa_s2n_get_groups_get_override_done(struct tevent_req *subreq);
-static void ipa_s2n_get_groups_next(struct tevent_req *subreq);
-static errno_t ipa_s2n_get_groups_save_step(struct tevent_req *req);
+static errno_t ipa_s2n_get_fqlist_step(struct tevent_req *req);
+static void ipa_s2n_get_fqlist_get_override_done(struct tevent_req *subreq);
+static void ipa_s2n_get_fqlist_next(struct tevent_req *subreq);
+static errno_t ipa_s2n_get_fqlist_save_step(struct tevent_req *req);
-static struct tevent_req *ipa_s2n_get_groups_send(TALLOC_CTX *mem_ctx,
+static struct tevent_req *ipa_s2n_get_fqlist_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
struct ipa_id_ctx *ipa_ctx,
struct sss_domain_info *dom,
struct sdap_handle *sh,
int exop_timeout,
- char **group_list)
+ char **fqname_list)
{
int ret;
- struct ipa_s2n_get_groups_state *state;
+ struct ipa_s2n_get_fqlist_state *state;
struct tevent_req *req;
- req = tevent_req_create(mem_ctx, &state, struct ipa_s2n_get_groups_state);
+ req = tevent_req_create(mem_ctx, &state, struct ipa_s2n_get_fqlist_state);
if (req == NULL) {
return NULL;
}
@@ -917,17 +917,17 @@ static struct tevent_req *ipa_s2n_get_groups_send(TALLOC_CTX
*mem_ctx,
state->ipa_ctx = ipa_ctx;
state->dom = dom;
state->sh = sh;
- state->group_list = group_list;
- state->group_idx = 0;
+ state->fqname_list = fqname_list;
+ state->fqname_idx = 0;
state->req_input.type = REQ_INP_NAME;
state->req_input.inp.name = NULL;
state->exop_timeout = exop_timeout;
state->attrs = NULL;
state->override_attrs = NULL;
- ret = ipa_s2n_get_groups_step(req);
+ ret = ipa_s2n_get_fqlist_step(req);
if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_get_groups_step failed.\n");
+ DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_get_fqlist_step failed.\n");
goto done;
}
@@ -940,25 +940,25 @@ done:
return req;
}
-static errno_t ipa_s2n_get_groups_step(struct tevent_req *req)
+static errno_t ipa_s2n_get_fqlist_step(struct tevent_req *req)
{
int ret;
- struct ipa_s2n_get_groups_state *state = tevent_req_data(req,
- struct ipa_s2n_get_groups_state);
+ struct ipa_s2n_get_fqlist_state *state = tevent_req_data(req,
+ struct ipa_s2n_get_fqlist_state);
struct berval *bv_req;
struct tevent_req *subreq;
struct sss_domain_info *parent_domain;
- char *group_name = NULL;
+ char *fqname = NULL;
char *domain_name = NULL;
parent_domain = get_domains_head(state->dom);
ret = sss_parse_name(state, parent_domain->names,
- state->group_list[state->group_idx],
- &domain_name, &group_name);
+ state->fqname_list[state->fqname_idx],
+ &domain_name, &fqname);
Shouldn't this variable be named just 'name'? The fqname is just being
parsed into (name,domain), right?
From 0d742401943698789deb2dd56159713717d56a1d Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Fri, 5 Dec 2014 11:12:42 +0100
Subject: [PATCH 6/7] IPA: resolve missing members
I couldn't do much testing b/c of the problems described earlier, but is
it possible there would be some nested group members that would require
BE_RE_GROUP also?
+ if (missing_list != NULL) {
+ subreq = ipa_s2n_get_fqlist_send(state, state->ev,
+ state->ipa_ctx, state->dom,
+ state->sh, state->exop_timeout,
+ BE_REQ_USER,
+ REQ_FULL_WITH_MEMBERS,
+ missing_list);
if (subreq == NULL) {
DEBUG(SSSDBG_OP_FAILURE,
"ipa_s2n_get_fqlist_send failed.\n");
From 6b7ab672002e3c7f60b2284f679415acc0924388 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 9 Dec 2014 17:48:46 +0100
Subject: [PATCH 7/7] IPA: set SYSDB_INITGR_EXPIRE for RESP_USER_GROUPLIST
Since RESP_USER_GROUPLIST contains all group memberships it is
effectively an initgroups request hence SYSDB_INITGR_EXPIRE will be set.
ACK