On Fri, Jul 29, 2016 at 03:08:52PM +0200, Jakub Hrozek wrote: > On Fri, Jul 29, 2016 at 02:59:46PM +0200, Lukas Slebodnik wrote: > > On (29/07/16 13:01), Jakub Hrozek wrote: > > >On Fri, Jul 29, 2016 at 11:44:53AM +0200, Lukas Slebodnik wrote: > > >> On (26/07/16 15:00), Jakub Hrozek wrote: > > >> >Hi, > > >> > > > >> >please see the attached patches. I'm not sure how this bug got in, > > >> >because in the patch that broke the functionality > > >> >(eef359b508b898ae99d2bf292a43f0f295a2ba5e) I said in the commit message > > >> >that I did the change that is only implemented in the first attached > > >> >patch. My guess is that the rebasing after the DP patches were merged > > >> >went wrong. > > >> > > > >> >To make sure we don't regress, I added more tests and switched the tests > > >> >to calling the DP handler. > > >> > > >> >From 9a60d3ed8bd2b0eeb51dff2c6f78771e0d29245e Mon Sep 17 00:00:00 2001 > > >> >From: Jakub Hrozek <jhro...@redhat.com> > > >> >Date: Thu, 21 Jul 2016 12:18:01 +0200 > > >> >Subject: [PATCH 1/4] SIMPLE: Do not parse names on startup > > >> > > > >> >It's not required to parse names on SSSD startup in the simple access > > >> >provider. We can instead just parse the name when the access request is > > >> >processed. > > >> > > > >> >Resolves: > > >> >https://fedorahosted.org/sssd/ticket/3101 > > >> >--- > > >> > src/providers/simple/simple_access.c | 7 ------- > > >> > 1 file changed, 7 deletions(-) > > >> > > > >> >diff --git a/src/providers/simple/simple_access.c > > >> >b/src/providers/simple/simple_access.c > > >> >index > > >> >cb72ada20727c63452936647876ef297106e17b0..ae90215351fe7db834898067d3b4bad71015ec5f > > >> > 100644 > > >> >--- a/src/providers/simple/simple_access.c > > >> >+++ b/src/providers/simple/simple_access.c > > >> >@@ -284,7 +284,6 @@ errno_t sssm_simple_access_init(TALLOC_CTX *mem_ctx, > > >> > struct dp_method *dp_methods) > > >> > { > > >> > struct simple_ctx *ctx; > > >> >- errno_t ret; > > >> > > > >> > ctx = talloc_zero(mem_ctx, struct simple_ctx); > > >> > if (ctx == NULL) { > > >> >@@ -296,12 +295,6 @@ errno_t sssm_simple_access_init(TALLOC_CTX > > >> >*mem_ctx, > > >> > ctx->be_ctx = be_ctx; > > >> > ctx->last_refresh_of_filter_lists = 0; > > >> > > > >> >- ret = simple_access_obtain_filter_lists(ctx); > > >> >- if (ret != EOK) { > > >> >- talloc_free(ctx); > > >> >- return ret; > > >> >- } > > >> >- > > >> > dp_set_method(dp_methods, DPM_ACCESS_HANDLER, > > >> > simple_access_handler_send, > > >> > simple_access_handler_recv, ctx, > > >> > struct simple_ctx, struct pam_data, struct pam_data > > >> > *); > > >> >-- > > >> >2.4.11 > > >> > > > >> ACK > > >> > > >> > > >> >From 5aeeedbb85e068ff1241868cf91596817540b009 Mon Sep 17 00:00:00 2001 > > >> >From: Jakub Hrozek <jhro...@redhat.com> > > >> >Date: Thu, 21 Jul 2016 13:33:18 +0200 > > >> >Subject: [PATCH 2/4] SIMPLE: Fail on any error parsing the access > > >> >control list > > >> > > > >> >Luckily this error was hidden by the fact that SSSD didn't start at all > > >> >when an unparseable name was encountered after startup. Otherwise, this > > >> >would have been a security issue. > > >> > > > >> >Nonetheless, we should just fail and deny access if we can't parse a > > >> >name in a simple access list. > > >> >--- > > >> > src/providers/simple/simple_access.c | 5 ++++- > > >> > 1 file changed, 4 insertions(+), 1 deletion(-) > > >> > > > >> >diff --git a/src/providers/simple/simple_access.c > > >> >b/src/providers/simple/simple_access.c > > >> >index > > >> >ae90215351fe7db834898067d3b4bad71015ec5f..577e8354e9b574764734248b2bde4ef06c6fb4fc > > >> > 100644 > > >> >--- a/src/providers/simple/simple_access.c > > >> >+++ b/src/providers/simple/simple_access.c > > >> >@@ -211,7 +211,10 @@ simple_access_handler_send(TALLOC_CTX *mem_ctx, > > >> > > > >> > ret = simple_access_obtain_filter_lists(simple_ctx); > > >> > if (ret != EOK) { > > >> >- DEBUG(SSSDBG_MINOR_FAILURE, "Failed to refresh filter > > >> >lists\n"); > > >> >+ DEBUG(SSSDBG_CRIT_FAILURE, > > >> >+ "Failed to refresh filter lists, denying all > > >> >access\n"); > > >> >+ pd->pam_status = PAM_PERM_DENIED; > > >> >+ goto immediately; > > >> > } > > >> I didn't test but Do we really need it. > > >> I think that unparsable names are covered by #2519 > > >> @see > > >> https://git.fedorahosted.org/cgit/sssd.git/commit/?id=79f128801d598ca57a6acebade01136525a47e00 > > >> > > >> IIRC the intention of #2519 was to be strict only for deny rules. > > >> There might be typos in allow rules because it isn't a security bug. > > > > > >If you prefer, I can return an error code only from failures parsing the > > >deny list, but according to my testing without this patch, a user was > > >allowed access if an unparseable name was in the deny list. Try to > > >remove this hunk and run the tests from the last patch.. > > > > > I tested only with the 1st patch and all simple access test for AD passed. > > But it's true that we previously failed in initialisation of access provider > > and we had resolved all domains. So refresh of filter_* could not cause > > a problem. > > > > Yes, so what should we do with this patch? Keep it as is or change to > only error out on typos in deny?
ping? Can we discuss this patchset again? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
