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 <[email protected]> > >> >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 <[email protected]> > >> >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? > >I prefer not to use the author tag at all to be honest. I never know who to > >put there. In this case, I only moved some declarations, I didn't 'create' > >the code, so I copied the author tag from the person who really wrote it. > > > >I'm fine with dropping the author line. > > > I do not know details about your contract. > But my contract with RH says that all work done at working hours > must have "copyright RH". IIUC it is because easier enforcing a low from > RH side. > > Anyway, it's old code written by Sumit and you just moved it. > I do not think we shoudl change author or copyright. Ah, yes, copyright should be red hat, author should be IMO blank. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
