[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