[PATCHES] Improve group handling for subdomain users

[Sumit Bose]

Hi,

this series of patches fixes https://fedorahosted.org/sssd/ticket/1604 .
To be precise only 0005 is the fix for the ticket, but the others add
some necessary improvements, especially the removal of a group
membership.

bye,
Sumit

[Simo Sorce]

On Wed, 2012-11-07 at 13:13 +0100, Sumit Bose wrote:
> This patch adds a new call which compares a list of current GIDs with
> a
> list of new GIDs and return a list of GIDs which are currently missing
> and must be added and another list of GIDs which are not used anymore
> and must be deleted. The method is the same as used by
> diff_string_lists().
>
> src/responder/pac/pacsrv.h       |   10 +++
>  src/responder/pac/pacsrv_utils.c |  155
> ++++++++++++++++++++++++++++++++++++++
>  src/tests/pac_responder-tests.c  |  104 +++++++++++++++++++++++++-
>  3 files changed, 267 insertions(+), 2 deletions(-)
>
> diff --git a/src/responder/pac/pacsrv.h b/src/responder/pac/pacsrv.h
> index e088e21..08172f8 100644
> --- a/src/responder/pac/pacsrv.h
> +++ b/src/responder/pac/pacsrv.h
> @@ -106,4 +106,14 @@ errno_t get_pwd_from_pac(TALLOC_CTX *mem_ctx,
>                           struct PAC_LOGON_INFO *logon_info,
>                           struct passwd **_pwd,
>                           struct sysdb_attrs **_attrs);
>
> +errno_t get_gids_to_add_and_remove(TALLOC_CTX *mem_ctx,
>                               size_t current_gid_count,
>                               gid_t *current_gids,
>                               size_t gid_count,
>                               gid_t *gids,
>                               size_t *_add_gid_count,
>                               gid_t **_add_gids,
>                               size_t *_del_gid_count,
>                               gid_t **_del_gids); 

Can you change the function name to diff_gid_lists() ?

Also may be nice to change the following arg names:
current_gid_count -> cur_gid_num
current_gids -> cur_gid_list
gid_count -> new_gid_num
gids -> new_gid_list
_add_gid_count -> _add_gid_num
_add_gids -> _add_gid_list
_del_gid_count -> _del_gid_num
_del_gids -> _del_gid_list

Simo.

[Simo Sorce]

On Wed, 2012-11-07 at 13:13 +0100, Sumit Bose wrote:
>
> if (current_gid_count == 0 && gid_count == 0) {
>    ret = EOK;
>    goto done;
> }

Also noticed that for this condition you probably want to just assign
and return EOK directly, otherwise ...
> +done:
> if (ret == EOK) {
>    *_add_gid_count = add_gid_count;
>    *_add_gids = talloc_steal(mem_ctx, add_gids);
>    *_del_gid_count = del_gid_count;
>    *_del_gids = talloc_steal(mem_ctx, del_gids);
> }
> talloc_free(tmp_ctx);
> return ret;
>
> +}

... here you try to steal NULL pointers and free a NULL context.

talloc_steal and talloc_free do not crash if you pass NULL, but it is
strange to see all this done when clearly you never set anything in
there.

Not fatal, if you feel strongly that jumping to done is better I am ok
with it too.

Simo.

[Simo Sorce]

On Wed, 2012-11-07 at 13:13 +0100, Sumit Bose wrote:
> +static errno_t
> +pac_save_memberships_delete(struct pac_save_memberships_state *state)
> +{

[...]
> for (c = 0; c < pr_ctx->del_gid_count; c++) {
>    ret = sysdb_search_group_by_gid(state,
>
> state->group_dom->sysdb,
>                                    pr_ctx->del_gids[c], NULL,
>
> &group);

Wouldn't it be more efficient to create a search filter that lists all
GIDs we are interested in and do a single search rather than looping
over and over ?

Simo.

[Simo Sorce]

On Wed, 2012-11-07 at 13:13 +0100, Sumit Bose wrote:
> From 9bfb75134ede4d6fe335e2492416e974f239008d Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Wed, 7 Nov 2012 11:53:13 +0100
> Subject: [PATCH 3/5] Use sysdb_initgroups() instead of
>  sysdb_get_user_by_name()
>
> To be able to efficiently store group memberships we need to know the
> current memberships of a user. Instead of just looking up the user in
> the cache sysdb_initgroups() is used which returns the user entry
> together with all groups the user is a member of.
>
> src/responder/pac/pacsrv_cmd.c |   44
> +++++++++++++++++++++++++++++++++------
>  1 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/src/responder/pac/pacsrv_cmd.c
> b/src/responder/pac/pacsrv_cmd.c
> index 7777983..051f697 100644
> --- a/src/responder/pac/pacsrv_cmd.c
> +++ b/src/responder/pac/pacsrv_cmd.c
> @@ -60,11 +60,15 @@ struct pac_req_ctx {
>  size_t gid_count;
>  gid_t *gids;
> size_t current_gid_count;
> gid_t *current_gids;
>
> };
>
> static errno_t pac_add_user_next(struct pac_req_ctx *pr_ctx);
>  static void pac_get_domains_done(struct tevent_req *req);
> -static errno_t save_pac_user(struct pac_req_ctx *pr_ctx);
> +static errno_t save_pac_user(TALLOC_CTX *mem_ctx, struct pac_req_ctx
> *pr_ctx,
>                         size_t *_current_gid_count, gid_t
>
> **_current_gids);
>  static void pac_get_group_done(struct tevent_req *subreq);
>  static errno_t pac_save_memberships_next(struct tevent_req *req);
>  static errno_t pac_store_membership(struct pac_req_ctx *pr_ctx,
> @@ -188,7 +192,8 @@ static errno_t pac_add_user_next(struct
> pac_req_ctx *pr_ctx)
>      struct dom_sid *my_dom_sid;
>      struct local_mapping_ranges *my_range_map;
> ret = save_pac_user(pr_ctx);
> ret = save_pac_user(pr_ctx, pr_ctx, &pr_ctx->current_gid_count,
>                    &pr_ctx->current_gids);
> if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE, ("save_pac_user failed.\n"));
>        goto done;
>
> @@ -223,15 +228,18 @@ done:
>      return ret;
>  }
>
> -static errno_t save_pac_user(struct pac_req_ctx *pr_ctx)
> +static errno_t save_pac_user(TALLOC_CTX *mem_ctx, struct pac_req_ctx
> *pr_ctx,
>                         size_t *_current_gid_count, gid_t
>
> **_current_gids)
>  {
>      struct sysdb_ctx *sysdb;
>      int ret;
> const char *attrs[] = {SYSDB_NAME, SYSDB_UIDNUM, SYSDB_GIDNUM,
>
> NULL};
> struct ldb_message *msg;
> struct passwd *pwd = NULL;
> TALLOC_CTX *tmp_ctx = NULL;
> struct sysdb_attrs *user_attrs = NULL;
>
> struct ldb_result *res = NULL;
>
> gid_t *current_gids = NULL;
>
> size_t current_gid_count = 0;
>
> size_t c;
>
> sysdb = pr_ctx->dom->sysdb;
> if (sysdb == NULL) {
>
> @@ -247,9 +255,29 @@ static errno_t save_pac_user(struct pac_req_ctx
> *pr_ctx)
>          goto done;
>      }
> ret = sysdb_search_user_by_name(tmp_ctx, sysdb,
>
> pr_ctx->user_name, attrs,
>                                &msg);
> ret = sysdb_initgroups(tmp_ctx, sysdb, pr_ctx->user_name, &res);
> if (ret == EOK) {
>    /* First result is the user entry then the groups follow */
>    if (res->count > 1) {
>        current_gid_count = res->count - 1;
>        current_gids = talloc_array(mem_ctx, gid_t,
>
> current_gid_count);
>        if (current_gids == NULL) {
>            DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n"));
>            ret = ENOMEM;
>            goto done;
>        }
>        for (c = 0; c < current_gid_count; c++) {
>            current_gids[c] =
>
> ldb_msg_find_attr_as_uint64(res->msgs[c + 1],
>
> SYSDB_GIDNUM, 0);
>            if (current_gids[c] == 0) {
>                DEBUG(SSSDBG_OP_FAILURE, ("Missing GID.\n"));
>                ret = EINVAL;
>                goto done;
>            }
>        }
>    }
>    /* TODO: check id uid and gid are equal. */
> } else if (ret == ENOENT) {
>        ret = get_pwd_from_pac(tmp_ctx, pr_ctx->pac_ctx, pr_ctx->dom,
>
> @@ -274,6 +302,8 @@ static errno_t save_pac_user(struct pac_req_ctx
> *pr_ctx)
>          goto done;
>      }
> *_current_gid_count = current_gid_count;
> *_current_gids = current_gids;
> ret = EOK;
> done:
>
> 1.7.7.6

Here it seems like you are overloading save_pac_user() with also
returning the current group memberships, but I do not see any real
advantage in doing it as part of pac_sve_user, there is no optimization
involved.

I think it would be more readable if you created a pac_user_get_gids()
that does the initgroups call and just called it before/after
save_pac_user()

Also this patch seem to make save_pac_user() skip storing the user if
sysdb_initgroups() is successful. This means that if the PAC has new
information (like different fullname) that information is simply
discarded and not updated.

I thik keeping these 2 separate makes it more readable and avoids this
bug.

Simo.

[Simo Sorce]

On Wed, 2012-11-07 at 13:13 +0100, Sumit Bose wrote:
> From 64502919dd9dd75b82e2a16808d2c3ed8250a72d Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Wed, 7 Nov 2012 12:09:55 +0100
> Subject: [PATCH 5/5] Store the original group DN in the subdomain user
> object
>
> For user of the local domain the server-side DN of the groups the user
> is a member of is stored with the user object in the cache and used to
> improve performance e.g. by the HBAC code. Since subdomain users
> should
> be handled by HBAC as well the group DN is stored in the same way as
> for
> users of the local domain.

Can you please ad a comment on why you are also removing code that sets
group_attrs ?

I can't see why at a glance.

Simo.

[Sumit Bose]

On Thu, Nov 08, 2012 at 09:34:19AM -0500, Simo Sorce wrote:
> On Wed, 2012-11-07 at 13:13 +0100, Sumit Bose wrote:
> >
>
> Can you change the function name to diff_gid_lists() ?
>
> Also may be nice to change the following arg names:
> current_gid_count -> cur_gid_num
> current_gids -> cur_gid_list
> gid_count -> new_gid_num
> gids -> new_gid_list
> _add_gid_count -> _add_gid_num
> _add_gids -> _add_gid_list
> _del_gid_count -> _del_gid_num
> _del_gids -> _del_gid_list

done
> >
> > if (current_gid_count == 0 && gid_count == 0) {
> >    ret = EOK;
> >    goto done;
> > }
>
> Also noticed that for this condition you probably want to just assign
> and return EOK directly, otherwise ...
> > +done:
> > if (ret == EOK) {
> >    *_add_gid_count = add_gid_count;
> >    *_add_gids = talloc_steal(mem_ctx, add_gids);
> >    *_del_gid_count = del_gid_count;
> >    *_del_gids = talloc_steal(mem_ctx, del_gids);
> > }
> > talloc_free(tmp_ctx);
> > return ret;
> >
> > +}
>
> ... here you try to steal NULL pointers and free a NULL context.
>
> talloc_steal and talloc_free do not crash if you pass NULL, but it is
> strange to see all this done when clearly you never set anything in
> there.
>
> Not fatal, if you feel strongly that jumping to done is better I am ok
> with it too.

Having NULLs for the group lists here would be a typical case if the
group memberships of does not change too often. I would prefer to have a
single exit point where the returned data is set.
> > +static errno_t
> > +pac_save_memberships_delete(struct pac_save_memberships_state *state)
> > +{
>
> [...]
> > for (c = 0; c < pr_ctx->del_gid_count; c++) {
> >    ret = sysdb_search_group_by_gid(state,
> >
> > state->group_dom->sysdb,
> >                                    pr_ctx->del_gids[c], NULL,
> >
> > &group);
>
> Wouldn't it be more efficient to create a search filter that lists all
> GIDs we are interested in and do a single search rather than looping
> over and over ?

I skipped the search her completely and kept the needed attributes after
the sysdb_initgroups() call.
> Here it seems like you are overloading save_pac_user() with also
> returning the current group memberships, but I do not see any real
> advantage in doing it as part of pac_sve_user, there is no optimization
> involved.
>
> I think it would be more readable if you created a pac_user_get_gids()
> that does the initgroups call and just called it before/after
> save_pac_user()
>
> Also this patch seem to make save_pac_user() skip storing the user if
> sysdb_initgroups() is successful. This means that if the PAC has new
> information (like different fullname) that information is simply
> discarded and not updated.
>
> I thik keeping these 2 separate makes it more readable and avoids this
> bug.

done
> Can you please ad a comment on why you are also removing code that sets
> group_attrs ?
>
> I can't see why at a glance.

The old version added SYSDB_MEMBER manually with the help of
group_attrs, the new version just calls sysdb_mod_group_member()
instead.
> Simo.

New series attached.

bye,
Sumit

[Simo Sorce]

On Fri, 2012-11-09 at 12:09 +0100, Sumit Bose wrote:
> New series attached.

ack and pushed to master and 1-9

Simo.