[PATCH] Handle unparseable names in the simple access list correctly

[Jakub Hrozek]

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.

[Lukas Slebodnik]

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 <jhrozek(a)redhat.com&gt;
> 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 <jhrozek(a)redhat.com&gt;
> 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=79f128801d598ca57a6...

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.

and commit says not existing user/group in
simple_allow_users/simple_allow_groups should not imply access denied.



> From 5bd29f76a8aee228b3ce07e2ec6453f7374a4acc Mon Sep 17 00:00:00
> 2001
> From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
> Date: Tue, 26 Jul 2016 12:13:43 +0200
> Subject: [PATCH 3/4] SIMPLE: Make the DP handlers testable
>
> To make it possible to call the whole DP handler in the unit test, not
> just the evaluator part.
> ---
>  Makefile.am                              |  1 +
>  src/providers/simple/simple_access.c     |  5 ++--
>  src/providers/simple/simple_access_pvt.h | 43 ++++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+), 2 deletions(-)
>  create mode 100644 src/providers/simple/simple_access_pvt.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 3d84bd868a8d0b8f9df329aa8978c215ba750a45..5d1d671096f986d9387e6199112c017e9bf30e1b
> 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -665,6 +665,7 @@ dist_noinst_HEADERS = \
>      src/providers/fail_over_srv.h \
>      src/util/child_common.h \
>      src/providers/simple/simple_access.h \
> +    src/providers/simple/simple_access_pvt.h \
>      src/providers/krb5/krb5_auth.h \
>      src/providers/krb5/krb5_common.h \
>      src/providers/krb5/krb5_utils.h \
> diff --git a/src/providers/simple/simple_access.c b/src/providers/simple/simple_access.c
> index 577e8354e9b574764734248b2bde4ef06c6fb4fc..521beee84833b47b547bd1045c24e3384aa4d9a5
> 100644
> --- a/src/providers/simple/simple_access.c
> +++ b/src/providers/simple/simple_access.c
> @@ -22,6 +22,7 @@
>  #include <security/pam_modules.h>
>  
>  #include "providers/simple/simple_access.h"
> +#include "providers/simple/simple_access_pvt.h"
>  #include "util/sss_utf8.h"
>  #include "providers/backend.h"
>  #include "db/sysdb.h"
> @@ -176,7 +177,7 @@ struct simple_access_handler_state {
>  
>  static void simple_access_handler_done(struct tevent_req *subreq);
>  
> -static struct tevent_req *
> +struct tevent_req *
>  simple_access_handler_send(TALLOC_CTX *mem_ctx,
>                             struct simple_ctx *simple_ctx,
>                             struct pam_data *pd,
> @@ -265,7 +266,7 @@ done:
>      tevent_req_done(req);
>  }
>  
> -static errno_t
> +errno_t
>  simple_access_handler_recv(TALLOC_CTX *mem_ctx,
>                         struct tevent_req *req,
>                         struct pam_data **_data)
> diff --git a/src/providers/simple/simple_access_pvt.h
> b/src/providers/simple/simple_access_pvt.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..c133e1c5531be35861178e0b23aa7a09db9f7703
> --- /dev/null
> +++ b/src/providers/simple/simple_access_pvt.h
> @@ -0,0 +1,43 @@
> +/*
> +   SSSD
> +
> +   Simple access control
> +
> +   Copyright (C) Sumit Bose <sbose(a)redhat.com&gt; 2010
> + 
Looks like a copy&paste issue.
Should we use Author + copyright to RH for new files?


> +   This program is free software; you can redistribute it and/or
> modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>;.
> +*/
> +
> +#ifndef __SIMPLE_ACCESS_PVT_H__
> +#define __SIMPLE_ACCESS_PVT_H__
> +
> +#include "providers/data_provider/dp.h"
> +
> +/* We only 'export' the functions in a private header file to be able to call
> + * them from unit tests
> + */
> +struct tevent_req *
> +simple_access_handler_send(TALLOC_CTX *mem_ctx,
> +                           struct simple_ctx *simple_ctx,
> +                           struct pam_data *pd,
> +                           struct dp_req_params *params);
> +
> +errno_t
> +simple_access_handler_recv(TALLOC_CTX *mem_ctx,
> +                           struct tevent_req *req,
> +                           struct pam_data **_data);
> +
> +int simple_access_obtain_filter_lists(struct simple_ctx *ctx);
> +
> +#endif /* __SIMPLE_ACCESS_PVT_H__ */
> -- 
> 2.4.11
>  
> From c01d768243e16fba9d1078b8a6b64b7d7ee1f43a Mon Sep 17 00:00:00
> 2001
> From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
> Date: Tue, 26 Jul 2016 12:14:47 +0200
> Subject: [PATCH 4/4] TESTS: Use the DP handlers in simple provider tests, add
>  more tests
>
> Use the full simple access control handlers, just like SSSD does in the
> tests.
> --- 
Thank you for unit test. But it might change if we drop 2nd patch

[Jakub Hrozek]

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 <jhrozek(a)redhat.com&gt;
>  >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 <jhrozek(a)redhat.com&gt;
>  >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=79f128801d598ca57a6...
>  
>  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..

>  
>  and commit says not existing user/group in
>  simple_allow_users/simple_allow_groups should not imply access denied. 
Yes, I'm fine with skipping failures in allow, but not in deny (I really
wish we didn't have any deny rules anywhere in the SSSD access providers..)

>  
>  
>  
>  >From 5bd29f76a8aee228b3ce07e2ec6453f7374a4acc Mon Sep 17 00:00:00 2001
>  >From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
>  >Date: Tue, 26 Jul 2016 12:13:43 +0200
>  >Subject: [PATCH 3/4] SIMPLE: Make the DP handlers testable
>  >
>  >To make it possible to call the whole DP handler in the unit test, not
>  >just the evaluator part.
>  >---
>  > Makefile.am                              |  1 +
>  > src/providers/simple/simple_access.c     |  5 ++--
>  > src/providers/simple/simple_access_pvt.h | 43 ++++++++++++++++++++++++++++++++
>  > 3 files changed, 47 insertions(+), 2 deletions(-)
>  > create mode 100644 src/providers/simple/simple_access_pvt.h
>  >
>  >diff --git a/Makefile.am b/Makefile.am
>  >index
> 3d84bd868a8d0b8f9df329aa8978c215ba750a45..5d1d671096f986d9387e6199112c017e9bf30e1b 100644
>  >--- a/Makefile.am
>  >+++ b/Makefile.am
>  >@@ -665,6 +665,7 @@ dist_noinst_HEADERS = \
>  >     src/providers/fail_over_srv.h \
>  >     src/util/child_common.h \
>  >     src/providers/simple/simple_access.h \
>  >+    src/providers/simple/simple_access_pvt.h \
>  >     src/providers/krb5/krb5_auth.h \
>  >     src/providers/krb5/krb5_common.h \
>  >     src/providers/krb5/krb5_utils.h \
>  >diff --git a/src/providers/simple/simple_access.c
> b/src/providers/simple/simple_access.c
>  >index
> 577e8354e9b574764734248b2bde4ef06c6fb4fc..521beee84833b47b547bd1045c24e3384aa4d9a5 100644
>  >--- a/src/providers/simple/simple_access.c
>  >+++ b/src/providers/simple/simple_access.c
>  >@@ -22,6 +22,7 @@
>  > #include <security/pam_modules.h>
>  > 
>  > #include "providers/simple/simple_access.h"
>  >+#include "providers/simple/simple_access_pvt.h"
>  > #include "util/sss_utf8.h"
>  > #include "providers/backend.h"
>  > #include "db/sysdb.h"
>  >@@ -176,7 +177,7 @@ struct simple_access_handler_state {
>  > 
>  > static void simple_access_handler_done(struct tevent_req *subreq);
>  > 
>  >-static struct tevent_req *
>  >+struct tevent_req *
>  > simple_access_handler_send(TALLOC_CTX *mem_ctx,
>  >                            struct simple_ctx *simple_ctx,
>  >                            struct pam_data *pd,
>  >@@ -265,7 +266,7 @@ done:
>  >     tevent_req_done(req);
>  > }
>  > 
>  >-static errno_t
>  >+errno_t
>  > simple_access_handler_recv(TALLOC_CTX *mem_ctx,
>  >                        struct tevent_req *req,
>  >                        struct pam_data **_data)
>  >diff --git a/src/providers/simple/simple_access_pvt.h
> b/src/providers/simple/simple_access_pvt.h
>  >new file mode 100644
>  >index
> 0000000000000000000000000000000000000000..c133e1c5531be35861178e0b23aa7a09db9f7703
>  >--- /dev/null
>  >+++ b/src/providers/simple/simple_access_pvt.h
>  >@@ -0,0 +1,43 @@
>  >+/*
>  >+   SSSD
>  >+
>  >+   Simple access control
>  >+
>  >+   Copyright (C) Sumit Bose <sbose(a)redhat.com&gt; 2010
>  >+
>  Looks like a copy&paste issue.
>  Should we use Author + copyright to RH for new files? 
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.

>  
>  
>  >+   This program is free software; you can redistribute it and/or modify
>  >+   it under the terms of the GNU General Public License as published by
>  >+   the Free Software Foundation; either version 3 of the License, or
>  >+   (at your option) any later version.
>  >+
>  >+   This program is distributed in the hope that it will be useful,
>  >+   but WITHOUT ANY WARRANTY; without even the implied warranty of
>  >+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  >+   GNU General Public License for more details.
>  >+
>  >+   You should have received a copy of the GNU General Public License
>  >+   along with this program.  If not, see <http://www.gnu.org/licenses/>;.
>  >+*/
>  >+
>  >+#ifndef __SIMPLE_ACCESS_PVT_H__
>  >+#define __SIMPLE_ACCESS_PVT_H__
>  >+
>  >+#include "providers/data_provider/dp.h"
>  >+
>  >+/* We only 'export' the functions in a private header file to be able to
> call
>  >+ * them from unit tests
>  >+ */
>  >+struct tevent_req *
>  >+simple_access_handler_send(TALLOC_CTX *mem_ctx,
>  >+                           struct simple_ctx *simple_ctx,
>  >+                           struct pam_data *pd,
>  >+                           struct dp_req_params *params);
>  >+
>  >+errno_t
>  >+simple_access_handler_recv(TALLOC_CTX *mem_ctx,
>  >+                           struct tevent_req *req,
>  >+                           struct pam_data **_data);
>  >+
>  >+int simple_access_obtain_filter_lists(struct simple_ctx *ctx);
>  >+
>  >+#endif /* __SIMPLE_ACCESS_PVT_H__ */
>  >-- 
>  >2.4.11
>  >
>  
>  >From c01d768243e16fba9d1078b8a6b64b7d7ee1f43a Mon Sep 17 00:00:00 2001
>  >From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
>  >Date: Tue, 26 Jul 2016 12:14:47 +0200
>  >Subject: [PATCH 4/4] TESTS: Use the DP handlers in simple provider tests, add
>  > more tests
>  >
>  >Use the full simple access control handlers, just like SSSD does in the
>  >tests.
>  >--- 
Let's discuss patch #2 first.

[Lukas Slebodnik]

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 <jhrozek(a)redhat.com&gt;
> > >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 <jhrozek(a)redhat.com&gt;
> > >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=79f128801d598ca57a6...
> > 
> > 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.


> > 
> > and commit says not existing user/group in
> > simple_allow_users/simple_allow_groups should not imply access denied.
>
> Yes, I'm fine with skipping failures in allow, but not in deny (I really
> wish we didn't have any deny rules anywhere in the SSSD access providers..)
>  
Agree but we have deny rules for simple access provider.

> > 
> > 
> > 
> > >From 5bd29f76a8aee228b3ce07e2ec6453f7374a4acc Mon Sep 17 00:00:00 2001
> > >From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
> > >Date: Tue, 26 Jul 2016 12:13:43 +0200
> > >Subject: [PATCH 3/4] SIMPLE: Make the DP handlers testable
> > >
> > >To make it possible to call the whole DP handler in the unit test, not
> > >just the evaluator part.
> > >---
> > > Makefile.am                              |  1 +
> > > src/providers/simple/simple_access.c     |  5 ++--
> > > src/providers/simple/simple_access_pvt.h | 43 ++++++++++++++++++++++++++++++++
> > > 3 files changed, 47 insertions(+), 2 deletions(-)
> > > create mode 100644 src/providers/simple/simple_access_pvt.h
> > >
> > >diff --git a/Makefile.am b/Makefile.am
> > >index
> 3d84bd868a8d0b8f9df329aa8978c215ba750a45..5d1d671096f986d9387e6199112c017e9bf30e1b 100644
> > >--- a/Makefile.am
> > >+++ b/Makefile.am
> > >@@ -665,6 +665,7 @@ dist_noinst_HEADERS = \
> > >     src/providers/fail_over_srv.h \
> > >     src/util/child_common.h \
> > >     src/providers/simple/simple_access.h \
> > >+    src/providers/simple/simple_access_pvt.h \
> > >     src/providers/krb5/krb5_auth.h \
> > >     src/providers/krb5/krb5_common.h \
> > >     src/providers/krb5/krb5_utils.h \
> > >diff --git a/src/providers/simple/simple_access.c
> b/src/providers/simple/simple_access.c
> > >index
> 577e8354e9b574764734248b2bde4ef06c6fb4fc..521beee84833b47b547bd1045c24e3384aa4d9a5 100644
> > >--- a/src/providers/simple/simple_access.c
> > >+++ b/src/providers/simple/simple_access.c
> > >@@ -22,6 +22,7 @@
> > > #include <security/pam_modules.h>
> > > 
> > > #include "providers/simple/simple_access.h"
> > >+#include "providers/simple/simple_access_pvt.h"
> > > #include "util/sss_utf8.h"
> > > #include "providers/backend.h"
> > > #include "db/sysdb.h"
> > >@@ -176,7 +177,7 @@ struct simple_access_handler_state {
> > > 
> > > static void simple_access_handler_done(struct tevent_req *subreq);
> > > 
> > >-static struct tevent_req *
> > >+struct tevent_req *
> > > simple_access_handler_send(TALLOC_CTX *mem_ctx,
> > >                            struct simple_ctx *simple_ctx,
> > >                            struct pam_data *pd,
> > >@@ -265,7 +266,7 @@ done:
> > >     tevent_req_done(req);
> > > }
> > > 
> > >-static errno_t
> > >+errno_t
> > > simple_access_handler_recv(TALLOC_CTX *mem_ctx,
> > >                        struct tevent_req *req,
> > >                        struct pam_data **_data)
> > >diff --git a/src/providers/simple/simple_access_pvt.h
> b/src/providers/simple/simple_access_pvt.h
> > >new file mode 100644
> > >index
> 0000000000000000000000000000000000000000..c133e1c5531be35861178e0b23aa7a09db9f7703
> > >--- /dev/null
> > >+++ b/src/providers/simple/simple_access_pvt.h
> > >@@ -0,0 +1,43 @@
> > >+/*
> > >+   SSSD
> > >+
> > >+   Simple access control
> > >+
> > >+   Copyright (C) Sumit Bose <sbose(a)redhat.com&gt; 2010
> > >+
> > Looks like a copy&paste issue.
> > Should we use Author + copyright to RH for new files?
>
> 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.

Sorry for confusion.

LS

[Jakub Hrozek]

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 <jhrozek(a)redhat.com&gt;
>  >> >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 <jhrozek(a)redhat.com&gt;
>  >> >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=79f128801d598ca57a6...
>  >> 
>  >> 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.

[Jakub Hrozek]

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 <jhrozek(a)redhat.com&gt;
>  > >> >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 <jhrozek(a)redhat.com&gt;
>  > >> >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=79f128801d598ca57a6...
>  > >> 
>  > >> 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?

[Lukas Slebodnik]

On (29/07/16 11:44), 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 <jhrozek(a)redhat.com&gt;
> >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 <jhrozek(a)redhat.com&gt;
> >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=79f128801d598ca57a6...
>
> 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.
>  
I checked the test case for #2519 and it passed.
The bug #3101 is fixed as well.

http://sssd-ci.duckdns.org/logs/job/51/33/summary.html

ACK

LS

[Jakub Hrozek]

On Wed, Aug 10, 2016 at 03:35:39PM +0200, Lukas Slebodnik wrote:
>  ACK 
Thank you for the careful review. Pushed to master:
    * 95de2cd9ea8083115f3bbbf0867aaf6b218cb624
    * c777f575b0ec0c48ce3b85ea2c5cc298db02450e
    * 79ac0e8a4840202c3615d6ce6584df3c08efb594
    * d2902de03738a3018445698650d8b974ae3cf230