[SSSD] [PATCH] simple-access-provider: make user grp res more robust

Jakub Hrozek jhrozek at redhat.com
Mon Apr 27 08:59:19 UTC 2015


On Tue, Apr 21, 2015 at 12:42:52PM +0200, Pavel Reichl wrote:
> Hello,
> 
> Kaushik found a problem while verifying
> https://bugzilla.redhat.com/show_bug.cgi?id=1170910 .
> The root of the problem was that we were denying access for user of which at
> least one group failed to be resolved. This is too strict and can be relaxed
> to denying access only if the simple_deny_groups is not empty at the same
> time.
> 
> Please see attached patch.
> 
> Thanks!

> From 0f1ee428e87012323c1446c3d63fe59d12331c01 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Mon, 20 Apr 2015 11:33:29 -0400
> Subject: [PATCH] simple-access-provider: make user grp res more robust
> 
> Not all user groups need to be resolved if group deny list is empty.
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2519
> ---
>  src/providers/simple/simple_access_check.c | 34 +++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/src/providers/simple/simple_access_check.c b/src/providers/simple/simple_access_check.c
> index c8217f6d4ef2560931d3151276085eb2a6028be5..f06bfa48ef09e4a5075f8f7360476beae387de00 100644
> --- a/src/providers/simple/simple_access_check.c
> +++ b/src/providers/simple/simple_access_check.c
> @@ -395,6 +395,8 @@ struct simple_check_groups_state {
>  
>      const char **group_names;
>      size_t num_names;
> +
> +    int failed_to_resolve_groups;

Why do we have an int here but both return bool and never use the int as
an int?

>  };
>  
>  static void simple_check_get_groups_next(struct tevent_req *subreq);
> @@ -430,6 +432,7 @@ simple_check_get_groups_send(TALLOC_CTX *mem_ctx,
>  
>      state->ev = ev;
>      state->ctx = ctx;
> +    state->failed_to_resolve_groups = 0;
>  
>      DEBUG(SSSDBG_TRACE_LIBS, "Looking up groups for user %s\n", username);
>  
> @@ -548,14 +551,13 @@ static void simple_check_get_groups_next(struct tevent_req *subreq)
>          DEBUG(SSSDBG_OP_FAILURE,
>                "Could not resolve name of group with GID %"SPRIgid"\n",
>                state->lookup_groups[state->giter].gid);
> -        tevent_req_error(req, ret);
> -        return;
> +        state->failed_to_resolve_groups++;
> +    } else {
> +        state->num_names++;
> +        state->giter++;

I wonder if we could keep giter++ outside the if-else block, because..

>      }
>  
> -    state->num_names++;
> -    state->giter++;
> -
> -    if (state->giter < state->num_groups) {
> +    if (state->giter + state->failed_to_resolve_groups < state->num_groups) {

..then we wouldn't have to change the condition here and would be able
to keep it simpler.

The rest looks good to me.



More information about the sssd-devel mailing list