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!
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@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.
On 04/27/2015 10:59 AM, Jakub Hrozek wrote:
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@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?
I used it as int here:
if (state->giter + state->failed_to_resolve_groups < state->num_groups)
but I don't any more.
};
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.
Sure, please see updated patch.
The rest looks good to me. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Apr 27, 2015 at 03:05:32PM +0200, Pavel Reichl wrote:
@@ -772,9 +776,11 @@ static void simple_access_check_done(struct tevent_req *subreq) struct simple_access_check_state *state = tevent_req_data(req, struct simple_access_check_state); errno_t ret;
bool failed_to_resolve_group;
/* We know the names now. Run the check. */
- ret = simple_check_get_groups_recv(subreq, state, &state->group_names);
- ret = simple_check_get_groups_recv(subreq, state, &state->group_names,
talloc_zfree(subreq); if (ret == ENOENT) { /* If the user wasn't found, just shortcut */&failed_to_resolve_group);
@@ -786,6 +792,18 @@ static void simple_access_check_done(struct tevent_req *subreq) "Could not collect groups of user %s\n", state->username); tevent_req_error(req, ret); return;
- } else if (ret == EOK && failed_to_resolve_group) {
This one looks good, but this change also got me wondering -- why change the simple_check_get_groups_ request at all and not just return a separate error code? ERR_SIMPLE_GROUPS_MISSING instead of branching on EOK and another boolean?
DEBUG(SSSDBG_OP_FAILURE,
"Could not collect groups of user %s\n", state->username);
if (state->ctx->deny_groups == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,
"But no deny groups were defined so we can continue.\n");
} else {
DEBUG(SSSDBG_OP_FAILURE,
"Some deny groups were defined, we can't continue\n");
tevent_req_error(req, ret);
return;
}
}
ret = simple_check_groups(state->ctx, state->group_names,
-- 2.1.0
On 04/27/2015 03:36 PM, Jakub Hrozek wrote:
On Mon, Apr 27, 2015 at 03:05:32PM +0200, Pavel Reichl wrote:
@@ -772,9 +776,11 @@ static void simple_access_check_done(struct tevent_req *subreq) struct simple_access_check_state *state = tevent_req_data(req, struct simple_access_check_state); errno_t ret;
bool failed_to_resolve_group;
/* We know the names now. Run the check. */
- ret = simple_check_get_groups_recv(subreq, state, &state->group_names);
- ret = simple_check_get_groups_recv(subreq, state, &state->group_names,
&failed_to_resolve_group); talloc_zfree(subreq); if (ret == ENOENT) { /* If the user wasn't found, just shortcut */
@@ -786,6 +792,18 @@ static void simple_access_check_done(struct tevent_req *subreq) "Could not collect groups of user %s\n", state->username); tevent_req_error(req, ret); return;
- } else if (ret == EOK && failed_to_resolve_group) {
This one looks good, but this change also got me wondering -- why change the simple_check_get_groups_ request at all and not just return a separate error code? ERR_SIMPLE_GROUPS_MISSING instead of branching on EOK and another boolean?
Yes, its better your way, sorry for missing that.
DEBUG(SSSDBG_OP_FAILURE,
"Could not collect groups of user %s\n", state->username);
if (state->ctx->deny_groups == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,
"But no deny groups were defined so we can continue.\n");
} else {
DEBUG(SSSDBG_OP_FAILURE,
"Some deny groups were defined, we can't continue\n");
tevent_req_error(req, ret);
return;
} } ret = simple_check_groups(state->ctx, state->group_names,
-- 2.1.0
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Apr 27, 2015 at 04:24:33PM +0200, Pavel Reichl wrote:
From 27d499c2b34cb3918f506f7911af930d80025492 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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.
This patch looks good to me and passed my testing
ACK
On Tue, Apr 28, 2015 at 10:40:28AM +0200, Jakub Hrozek wrote:
On Mon, Apr 27, 2015 at 04:24:33PM +0200, Pavel Reichl wrote:
From 27d499c2b34cb3918f506f7911af930d80025492 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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.
This patch looks good to me and passed my testing
ACK
* master: 82a958e6592c4a4078e45b7197bbe4751b70f511 * sssd-1-12: 45a089a7bcf54e27fb46dc1a2c08c21ac07db96a
sssd-devel@lists.fedorahosted.org