On Fri, Jan 09, 2015 at 02:46:27PM +0100, Jakub Hrozek wrote:
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.
Thank you for the review. This should happen if your AD domain name (not
the real name) contains upper-case characters. This spelling is kept in
sysdb but get lower-cased e.g. in the getgrnam() output. I've add a 8th
patch which should fix it.
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.
fixed
> 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?
yes, I've changed it to short_name to be more explicit becase there are
so many names.
> 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?
about the problems see comment above. I think at this stage all nested
group memberships should be already unrolled and no special handleing
needed.
> + 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
New version attached.
bye,
Sumit