[PATCH] Add a new option ad_access_filter

[Jakub Hrozek]

Hi,

the attached patches implement the new option ad_access_control as
designed in
https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryAccessControl

The only part not implemented exactly as per the design page is changing
the default. I will write an e-mail about the issue into the thread with
the design decision so that also people who filter out patches can
participate.

[Jakub Hrozek]

On Thu, Oct 10, 2013 at 01:49:40PM +0200, Jakub Hrozek wrote:
> Hi,
>
> the attached patches implement the new option ad_access_control as
> designed in
> https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryAccessControl
>
> The only part not implemented exactly as per the design page is changing
> the default. I will write an e-mail about the issue into the thread with
> the design decision so that also people who filter out patches can
> participate.

Attached are patches rebased on current master.

[Lukas Slebodnik]

On (25/10/13 17:45), Jakub Hrozek wrote:
> On Thu, Oct 10, 2013 at 01:49:40PM +0200, Jakub Hrozek wrote:
> > Hi,
> >
> > the attached patches implement the new option ad_access_control as
> > designed in
> > https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryAccessControl
> >
> > The only part not implemented exactly as per the design page is changing
> > the default. I will write an e-mail about the issue into the thread with
> > the design decision so that also people who filter out patches can
> > participate.
>
> Attached are patches rebased on current master.
> From 3f115bad21ce47bb7dee4541982613c0212ed229 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sun, 6 Oct 2013 20:23:07 +0200
> Subject: [PATCH 1/6] LDAP: Amend sdap_access_check to allow any connection

ACK
> From ffeaf255263671f8cc43bab3aec7b1304e076705 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Tue, 8 Oct 2013 19:10:41 +0200
> Subject: [PATCH 2/6] LDAP: Parse FQDN into name/domain for subdomain users

ACK
> From 7b7532f68058dfad4122890743041973bb6bdfc6 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Wed, 2 Oct 2013 17:48:49 +0200
> Subject: [PATCH 3/6] AD: Add a new option ad_access_filter

ACK
> From 1daec9096cc5ae8690b3fe567713cf9ef23096e7 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Mon, 7 Oct 2013 18:02:04 +0200
> Subject: [PATCH 4/6] AD: Use the ad_access_filter if it's set

ACK
> From e4b1e0ca7d4078e787ccdab5d87b73aa5c8a6ab7 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Tue, 8 Oct 2013 17:50:56 +0200
> Subject: [PATCH 5/6] AD: Search GC by default during access control, fall back
> to LDAP
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2082
>
> In order to allow the ad_access_filter option to work for subdomain
> users as well, the Global Catalog must be searched. This patch adds a
> wrapper request atop sdap_access_send that selects the right connection
> (GC or LDAP) and optionally falls back to LDAP.
>
> src/providers/ad/ad_access.c | 160 +++++++++++++++++++++++++++++++++++++++++--
> src/providers/ad/ad_access.h |   4 +-
> src/providers/ad/ad_init.c   |   5 +-
> 3 files changed, 159 insertions(+), 10 deletions(-)
>
> diff --git a/src/providers/ad/ad_access.c b/src/providers/ad/ad_access.c
> index cf6412f22c6146b26090646b5b8d060efa43365f..59e2f2b81a3f9d1850d673551be079f42a6a2ac5 100644
> --- a/src/providers/ad/ad_access.c
> +++ b/src/providers/ad/ad_access.c
> @@ -25,10 +25,158 @@
> #include "src/providers/data_provider.h"
> #include "src/providers/dp_backend.h"
> #include "src/providers/ad/ad_access.h"
> +#include "src/providers/ad/ad_common.h"
> #include "src/providers/ldap/sdap_access.h"
>
> static void
> ad_access_done(struct tevent_req *req);
> +static errno_t
> +ad_access_step(struct tevent_req *req, struct sdap_id_conn_ctx *conn);
>
> +struct ad_access_state {
> struct tevent_context *ev;
> struct ad_access_ctx *ctx;
> struct pam_data *pd;
> struct be_ctx *be_ctx;
> struct sss_domain_info *domain;
> struct sdap_id_conn_ctx **clist;
> int cindex;
>
> +};
>
> +static struct tevent_req *
> +ad_access_send(TALLOC_CTX *mem_ctx,
>           struct tevent_context *ev,
>           struct be_ctx *be_ctx,
>           struct sss_domain_info *domain,
>           struct ad_access_ctx *ctx,
>           struct pam_data *pd)
>
> +{
> struct tevent_req *req;
> struct ad_access_state *state;
> errno_t ret;
> req = tevent_req_create(mem_ctx, &state, struct ad_access_state);
> if (req == NULL) {
>    return NULL;
> }
> state->ev = ev;
> state->ctx = ctx;
> state->pd = pd;
> state->be_ctx = be_ctx;
> state->domain = domain;
> state->clist = talloc_zero_array(state, struct sdap_id_conn_ctx *, 3);
> if (state->clist == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> /* Always try GC first */
> state->clist[0] = ctx->gc_ctx;
> if (IS_SUBDOMAIN(domain) == false) {

This is the same situation like in file src/providers/ad/ad_id.c
          If GC is misconfigured, we should not go offline.
          SSSD should try to fall back to LDAP
>    /* With root domain users we have the option to
>     * fall back to LDAP in case ie POSIX attributes
>     * are used but not replicated to GC
>     */
>    state->clist[1] = ctx->ldap_ctx;
> }
> ret = ad_access_step(req, state->clist[state->cindex]);
> if (ret != EOK) {
>    goto done;
> }
> From 8d2cfbcc0d992b26f90c9494b10f3bf666498b66 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Tue, 8 Oct 2013 20:59:22 +0200
> Subject: [PATCH 6/6] AD: Add extended access filter
>
> https://fedorahosted.org/sssd/ticket/2082
>
> Adds a new option that allows the admin to specify a LDAP access filter
> that can be applied globally, per-domain or per-forest.
>
> Makefile.am                              |  29 ++-
> src/man/sssd-ad.5.xml                    |  41 +++-
> src/providers/ad/ad_access.c             | 213 ++++++++++++++++++-
> src/providers/ad/ad_init.c               |   5 +-
> src/tests/cmocka/test_ad_access_filter.c | 341 +++++++++++++++++++++++++++++++
> 5 files changed, 623 insertions(+), 6 deletions(-)
> create mode 100644 src/tests/cmocka/test_ad_access_filter.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 8d548e2bd7f5ffa1d9d7a46dc08026213e8e80bc..bb5136e24cdbee48d9d356ef2f2849334f397d48 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -153,7 +153,8 @@ if HAVE_CMOCKA
>         dyndns-tests \
>         fqnames-tests \
>         test_sss_idmap \
>    test_utils
>    test_utils \
>    ad_access_filter_tests
>
> endif
>
> check_PROGRAMS = \
> @@ -1389,6 +1390,32 @@ test_utils_LDADD = \
>     $(CMOCKA_LIBS) \
>     $(SSSD_INTERNAL_LTLIBS) \
>     libsss_test_common.la
>
> +ad_access_filter_tests_SOURCES = \
> $(TEST_MOCK_OBJ) \
> $(sssd_be_SOURCES) \
> src/util/sss_ldap.c \
> src/util/sss_krb5.c \
> src/util/find_uid.c \
> src/util/user_info_msg.c \
> src/tests/cmocka/test_ad_access_filter.c
>
> +ad_access_filter_tests_CFLAGS = \
> $(AM_CFLAGS) \
> $(SYSTEMD_LOGIN_CFLAGS) \
> -DUNIT_TESTING
>
> +ad_access_filter_tests_LDADD = \
> $(PAM_LIBS) \
> $(CMOCKA_LIBS) \
> $(SSSD_LIBS) \
> $(CARES_LIBS) \
> $(KRB5_LIBS) \
> $(SSSD_INTERNAL_LTLIBS) \
> $(SYSTEMD_LOGIN_LIBS) \
> libsss_idmap.la \
> libsss_ldap_common.la \
> libsss_krb5_common.la \
> libsss_test_common.la

Please change the order of libs.
libsss_idmap.la should be after libsss_ldap_common.la, because linking will
fail with LDFLAGS=-Wl,--as-needed

./.libs/libsss_ldap_common.so: undefined reference to `is_domain_sid'
./.libs/libsss_ldap_common.so: undefined reference to
`sss_idmap_bin_sid_to_sid'
./.libs/libsss_ldap_common.so: undefined reference to `idmap_error_string'
./.libs/libsss_ldap_common.so: undefined reference to
`sss_idmap_ctx_set_autorid'
./.libs/libsss_ldap_common.so: undefined reference to
`sss_idmap_ctx_set_rangesize'
./.libs/libsss_ldap_common.so: undefined reference to
`sss_idmap_domain_by_name_has_algorithmic_mapping'
./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_init'
./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_sid_to_unix'
./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_upper'
./.libs/libsss_ldap_common.so: undefined reference to
`sss_idmap_calculate_range'
./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_lower'
./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_add_domain_ex'
./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_get_upper'
./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_unix_to_sid'
./.libs/libsss_ldap_common.so: undefined reference to
`sss_idmap_domain_has_algorithmic_mapping'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

LS

[Jakub Hrozek]

On Fri, Oct 25, 2013 at 07:44:47PM +0200, Lukas Slebodnik wrote:
> On (25/10/13 17:45), Jakub Hrozek wrote:
> > On Thu, Oct 10, 2013 at 01:49:40PM +0200, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > the attached patches implement the new option ad_access_control as
> > > designed in
> > > https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryAccessControl
> > >
> > > The only part not implemented exactly as per the design page is changing
> > > the default. I will write an e-mail about the issue into the thread with
> > > the design decision so that also people who filter out patches can
> > > participate.
> >
> > Attached are patches rebased on current master.
> > From 3f115bad21ce47bb7dee4541982613c0212ed229 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Sun, 6 Oct 2013 20:23:07 +0200
> > Subject: [PATCH 1/6] LDAP: Amend sdap_access_check to allow any connection
>
> ACK
> > From ffeaf255263671f8cc43bab3aec7b1304e076705 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Tue, 8 Oct 2013 19:10:41 +0200
> > Subject: [PATCH 2/6] LDAP: Parse FQDN into name/domain for subdomain users
>
> ACK
> > From 7b7532f68058dfad4122890743041973bb6bdfc6 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Wed, 2 Oct 2013 17:48:49 +0200
> > Subject: [PATCH 3/6] AD: Add a new option ad_access_filter
>
> ACK
> > From 1daec9096cc5ae8690b3fe567713cf9ef23096e7 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Mon, 7 Oct 2013 18:02:04 +0200
> > Subject: [PATCH 4/6] AD: Use the ad_access_filter if it's set
>
> ACK
> > From e4b1e0ca7d4078e787ccdab5d87b73aa5c8a6ab7 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Tue, 8 Oct 2013 17:50:56 +0200
> > Subject: [PATCH 5/6] AD: Search GC by default during access control, fall back
> > to LDAP
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2082
> >
> > In order to allow the ad_access_filter option to work for subdomain
> > users as well, the Global Catalog must be searched. This patch adds a
> > wrapper request atop sdap_access_send that selects the right connection
> > (GC or LDAP) and optionally falls back to LDAP.
> >
> > src/providers/ad/ad_access.c | 160 +++++++++++++++++++++++++++++++++++++++++--
> > src/providers/ad/ad_access.h |   4 +-
> > src/providers/ad/ad_init.c   |   5 +-
> > 3 files changed, 159 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/providers/ad/ad_access.c b/src/providers/ad/ad_access.c
> > index cf6412f22c6146b26090646b5b8d060efa43365f..59e2f2b81a3f9d1850d673551be079f42a6a2ac5 100644
> > --- a/src/providers/ad/ad_access.c
> > +++ b/src/providers/ad/ad_access.c
> > @@ -25,10 +25,158 @@
> > #include "src/providers/data_provider.h"
> > #include "src/providers/dp_backend.h"
> > #include "src/providers/ad/ad_access.h"
> > +#include "src/providers/ad/ad_common.h"
> > #include "src/providers/ldap/sdap_access.h"
> >
> > static void
> > ad_access_done(struct tevent_req *req);
> > +static errno_t
> > +ad_access_step(struct tevent_req *req, struct sdap_id_conn_ctx *conn);
> >
> > +struct ad_access_state {
> > struct tevent_context *ev;
> > struct ad_access_ctx *ctx;
> > struct pam_data *pd;
> > struct be_ctx *be_ctx;
> > struct sss_domain_info *domain;
> > struct sdap_id_conn_ctx **clist;
> > int cindex;
> >
> > +};
> >
> > +static struct tevent_req *
> > +ad_access_send(TALLOC_CTX *mem_ctx,
> >           struct tevent_context *ev,
> >           struct be_ctx *be_ctx,
> >           struct sss_domain_info *domain,
> >           struct ad_access_ctx *ctx,
> >           struct pam_data *pd)
> >
> > +{
> > struct tevent_req *req;
> > struct ad_access_state *state;
> > errno_t ret;
> > req = tevent_req_create(mem_ctx, &state, struct ad_access_state);
> > if (req == NULL) {
> >    return NULL;
> > }
> > state->ev = ev;
> > state->ctx = ctx;
> > state->pd = pd;
> > state->be_ctx = be_ctx;
> > state->domain = domain;
> > state->clist = talloc_zero_array(state, struct sdap_id_conn_ctx *, 3);
> > if (state->clist == NULL) {
> >    ret = ENOMEM;
> >    goto done;
> > }
> > /* Always try GC first */
> > state->clist[0] = ctx->gc_ctx;
> > if (IS_SUBDOMAIN(domain) == false) {
>       This is the same situation like in file src/providers/ad/ad_id.c
>       If GC is misconfigured, we should not go offline.
>       SSSD should try to fall back to LDAP
> >    /* With root domain users we have the option to
> >     * fall back to LDAP in case ie POSIX attributes
> >     * are used but not replicated to GC
> >     */
> >    state->clist[1] = ctx->ldap_ctx;
> > }
> > ret = ad_access_step(req, state->clist[state->cindex]);
> > if (ret != EOK) {
> >    goto done;
> > }
> > From 8d2cfbcc0d992b26f90c9494b10f3bf666498b66 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Tue, 8 Oct 2013 20:59:22 +0200
> > Subject: [PATCH 6/6] AD: Add extended access filter
> >
> > https://fedorahosted.org/sssd/ticket/2082
> >
> > Adds a new option that allows the admin to specify a LDAP access filter
> > that can be applied globally, per-domain or per-forest.
> >
> > Makefile.am                              |  29 ++-
> > src/man/sssd-ad.5.xml                    |  41 +++-
> > src/providers/ad/ad_access.c             | 213 ++++++++++++++++++-
> > src/providers/ad/ad_init.c               |   5 +-
> > src/tests/cmocka/test_ad_access_filter.c | 341 +++++++++++++++++++++++++++++++
> > 5 files changed, 623 insertions(+), 6 deletions(-)
> > create mode 100644 src/tests/cmocka/test_ad_access_filter.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 8d548e2bd7f5ffa1d9d7a46dc08026213e8e80bc..bb5136e24cdbee48d9d356ef2f2849334f397d48 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -153,7 +153,8 @@ if HAVE_CMOCKA
> >         dyndns-tests \
> >         fqnames-tests \
> >         test_sss_idmap \
> >    test_utils
> >    test_utils \
> >    ad_access_filter_tests
> >
> > endif
> >
> > check_PROGRAMS = \
> > @@ -1389,6 +1390,32 @@ test_utils_LDADD = \
> >     $(CMOCKA_LIBS) \
> >     $(SSSD_INTERNAL_LTLIBS) \
> >     libsss_test_common.la
> >
> > +ad_access_filter_tests_SOURCES = \
> > $(TEST_MOCK_OBJ) \
> > $(sssd_be_SOURCES) \
> > src/util/sss_ldap.c \
> > src/util/sss_krb5.c \
> > src/util/find_uid.c \
> > src/util/user_info_msg.c \
> > src/tests/cmocka/test_ad_access_filter.c
> >
> > +ad_access_filter_tests_CFLAGS = \
> > $(AM_CFLAGS) \
> > $(SYSTEMD_LOGIN_CFLAGS) \
> > -DUNIT_TESTING
> >
> > +ad_access_filter_tests_LDADD = \
> > $(PAM_LIBS) \
> > $(CMOCKA_LIBS) \
> > $(SSSD_LIBS) \
> > $(CARES_LIBS) \
> > $(KRB5_LIBS) \
> > $(SSSD_INTERNAL_LTLIBS) \
> > $(SYSTEMD_LOGIN_LIBS) \
> > libsss_idmap.la \
> > libsss_ldap_common.la \
> > libsss_krb5_common.la \
> > libsss_test_common.la
>
> Please change the order of libs.
> libsss_idmap.la should be after libsss_ldap_common.la, because linking will
> fail with LDFLAGS=-Wl,--as-needed
>
> ./.libs/libsss_ldap_common.so: undefined reference to `is_domain_sid'
> ./.libs/libsss_ldap_common.so: undefined reference to
> `sss_idmap_bin_sid_to_sid'
> ./.libs/libsss_ldap_common.so: undefined reference to `idmap_error_string'
> ./.libs/libsss_ldap_common.so: undefined reference to
> `sss_idmap_ctx_set_autorid'
> ./.libs/libsss_ldap_common.so: undefined reference to
> `sss_idmap_ctx_set_rangesize'
> ./.libs/libsss_ldap_common.so: undefined reference to
> `sss_idmap_domain_by_name_has_algorithmic_mapping'
> ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_init'
> ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_sid_to_unix'
> ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_upper'
> ./.libs/libsss_ldap_common.so: undefined reference to
> `sss_idmap_calculate_range'
> ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_lower'
> ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_add_domain_ex'
> ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_get_upper'
> ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_unix_to_sid'
> ./.libs/libsss_ldap_common.so: undefined reference to
> `sss_idmap_domain_has_algorithmic_mapping'
> clang: error: linker command failed with exit code 1 (use -v to see invocation)
>
> LS

Thank you for the review. I squashed the patches in.

[Lukas Slebodnik]

On (25/10/13 19:52), Jakub Hrozek wrote:
> On Fri, Oct 25, 2013 at 07:44:47PM +0200, Lukas Slebodnik wrote:
> > On (25/10/13 17:45), Jakub Hrozek wrote:
> > > On Thu, Oct 10, 2013 at 01:49:40PM +0200, Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > the attached patches implement the new option ad_access_control as
> > > > designed in
> > > > https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryAccessControl
> > > >
> > > > The only part not implemented exactly as per the design page is changing
> > > > the default. I will write an e-mail about the issue into the thread with
> > > > the design decision so that also people who filter out patches can
> > > > participate.
> > >
> > > Attached are patches rebased on current master.
> > > From 3f115bad21ce47bb7dee4541982613c0212ed229 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Sun, 6 Oct 2013 20:23:07 +0200
> > > Subject: [PATCH 1/6] LDAP: Amend sdap_access_check to allow any connection
> >
> > ACK
> > > From ffeaf255263671f8cc43bab3aec7b1304e076705 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Tue, 8 Oct 2013 19:10:41 +0200
> > > Subject: [PATCH 2/6] LDAP: Parse FQDN into name/domain for subdomain users
> >
> > ACK
> > > From 7b7532f68058dfad4122890743041973bb6bdfc6 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Wed, 2 Oct 2013 17:48:49 +0200
> > > Subject: [PATCH 3/6] AD: Add a new option ad_access_filter
> >
> > ACK
> > > From 1daec9096cc5ae8690b3fe567713cf9ef23096e7 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Mon, 7 Oct 2013 18:02:04 +0200
> > > Subject: [PATCH 4/6] AD: Use the ad_access_filter if it's set
> >
> > ACK
> > > From e4b1e0ca7d4078e787ccdab5d87b73aa5c8a6ab7 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Tue, 8 Oct 2013 17:50:56 +0200
> > > Subject: [PATCH 5/6] AD: Search GC by default during access control, fall back
> > > to LDAP
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2082
> > >
> > > In order to allow the ad_access_filter option to work for subdomain
> > > users as well, the Global Catalog must be searched. This patch adds a
> > > wrapper request atop sdap_access_send that selects the right connection
> > > (GC or LDAP) and optionally falls back to LDAP.
> > >
> > > src/providers/ad/ad_access.c | 160 +++++++++++++++++++++++++++++++++++++++++--
> > > src/providers/ad/ad_access.h |   4 +-
> > > src/providers/ad/ad_init.c   |   5 +-
> > > 3 files changed, 159 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/providers/ad/ad_access.c b/src/providers/ad/ad_access.c
> > > index cf6412f22c6146b26090646b5b8d060efa43365f..59e2f2b81a3f9d1850d673551be079f42a6a2ac5 100644
> > > --- a/src/providers/ad/ad_access.c
> > > +++ b/src/providers/ad/ad_access.c
> > > @@ -25,10 +25,158 @@
> > > #include "src/providers/data_provider.h"
> > > #include "src/providers/dp_backend.h"
> > > #include "src/providers/ad/ad_access.h"
> > > +#include "src/providers/ad/ad_common.h"
> > > #include "src/providers/ldap/sdap_access.h"
> > >
> > > static void
> > > ad_access_done(struct tevent_req *req);
> > > +static errno_t
> > > +ad_access_step(struct tevent_req *req, struct sdap_id_conn_ctx *conn);
> > >
> > > +struct ad_access_state {
> > > struct tevent_context *ev;
> > > struct ad_access_ctx *ctx;
> > > struct pam_data *pd;
> > > struct be_ctx *be_ctx;
> > > struct sss_domain_info *domain;
> > > struct sdap_id_conn_ctx **clist;
> > > int cindex;
> > >
> > > +};
> > >
> > > +static struct tevent_req *
> > > +ad_access_send(TALLOC_CTX *mem_ctx,
> > >           struct tevent_context *ev,
> > >           struct be_ctx *be_ctx,
> > >           struct sss_domain_info *domain,
> > >           struct ad_access_ctx *ctx,
> > >           struct pam_data *pd)
> > >
> > > +{
> > > struct tevent_req *req;
> > > struct ad_access_state *state;
> > > errno_t ret;
> > > req = tevent_req_create(mem_ctx, &state, struct ad_access_state);
> > > if (req == NULL) {
> > >    return NULL;
> > > }
> > > state->ev = ev;
> > > state->ctx = ctx;
> > > state->pd = pd;
> > > state->be_ctx = be_ctx;
> > > state->domain = domain;
> > > state->clist = talloc_zero_array(state, struct sdap_id_conn_ctx *, 3);
> > > if (state->clist == NULL) {
> > >    ret = ENOMEM;
> > >    goto done;
> > > }
> > > /* Always try GC first */
> > > state->clist[0] = ctx->gc_ctx;
> > > if (IS_SUBDOMAIN(domain) == false) {
> >       This is the same situation like in file src/providers/ad/ad_id.c
> >       If GC is misconfigured, we should not go offline.
> >       SSSD should try to fall back to LDAP
> > >    /* With root domain users we have the option to
> > >     * fall back to LDAP in case ie POSIX attributes
> > >     * are used but not replicated to GC
> > >     */
> > >    state->clist[1] = ctx->ldap_ctx;
> > > }
> > > ret = ad_access_step(req, state->clist[state->cindex]);
> > > if (ret != EOK) {
> > >    goto done;
> > > }
> > > From 8d2cfbcc0d992b26f90c9494b10f3bf666498b66 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Tue, 8 Oct 2013 20:59:22 +0200
> > > Subject: [PATCH 6/6] AD: Add extended access filter
> > >
> > > https://fedorahosted.org/sssd/ticket/2082
> > >
> > > Adds a new option that allows the admin to specify a LDAP access filter
> > > that can be applied globally, per-domain or per-forest.
> > >
> > > Makefile.am                              |  29 ++-
> > > src/man/sssd-ad.5.xml                    |  41 +++-
> > > src/providers/ad/ad_access.c             | 213 ++++++++++++++++++-
> > > src/providers/ad/ad_init.c               |   5 +-
> > > src/tests/cmocka/test_ad_access_filter.c | 341 +++++++++++++++++++++++++++++++
> > > 5 files changed, 623 insertions(+), 6 deletions(-)
> > > create mode 100644 src/tests/cmocka/test_ad_access_filter.c
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 8d548e2bd7f5ffa1d9d7a46dc08026213e8e80bc..bb5136e24cdbee48d9d356ef2f2849334f397d48 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -153,7 +153,8 @@ if HAVE_CMOCKA
> > >         dyndns-tests \
> > >         fqnames-tests \
> > >         test_sss_idmap \
> > >    test_utils
> > >    test_utils \
> > >    ad_access_filter_tests
> > >
> > > endif
> > >
> > > check_PROGRAMS = \
> > > @@ -1389,6 +1390,32 @@ test_utils_LDADD = \
> > >     $(CMOCKA_LIBS) \
> > >     $(SSSD_INTERNAL_LTLIBS) \
> > >     libsss_test_common.la
> > >
> > > +ad_access_filter_tests_SOURCES = \
> > > $(TEST_MOCK_OBJ) \
> > > $(sssd_be_SOURCES) \
> > > src/util/sss_ldap.c \
> > > src/util/sss_krb5.c \
> > > src/util/find_uid.c \
> > > src/util/user_info_msg.c \
> > > src/tests/cmocka/test_ad_access_filter.c
> > >
> > > +ad_access_filter_tests_CFLAGS = \
> > > $(AM_CFLAGS) \
> > > $(SYSTEMD_LOGIN_CFLAGS) \
> > > -DUNIT_TESTING
> > >
> > > +ad_access_filter_tests_LDADD = \
> > > $(PAM_LIBS) \
> > > $(CMOCKA_LIBS) \
> > > $(SSSD_LIBS) \
> > > $(CARES_LIBS) \
> > > $(KRB5_LIBS) \
> > > $(SSSD_INTERNAL_LTLIBS) \
> > > $(SYSTEMD_LOGIN_LIBS) \
> > > libsss_idmap.la \
> > > libsss_ldap_common.la \
> > > libsss_krb5_common.la \
> > > libsss_test_common.la
> >
> > Please change the order of libs.
> > libsss_idmap.la should be after libsss_ldap_common.la, because linking will
> > fail with LDFLAGS=-Wl,--as-needed
> >
> > ./.libs/libsss_ldap_common.so: undefined reference to `is_domain_sid'
> > ./.libs/libsss_ldap_common.so: undefined reference to
> > `sss_idmap_bin_sid_to_sid'
> > ./.libs/libsss_ldap_common.so: undefined reference to `idmap_error_string'
> > ./.libs/libsss_ldap_common.so: undefined reference to
> > `sss_idmap_ctx_set_autorid'
> > ./.libs/libsss_ldap_common.so: undefined reference to
> > `sss_idmap_ctx_set_rangesize'
> > ./.libs/libsss_ldap_common.so: undefined reference to
> > `sss_idmap_domain_by_name_has_algorithmic_mapping'
> > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_init'
> > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_sid_to_unix'
> > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_upper'
> > ./.libs/libsss_ldap_common.so: undefined reference to
> > `sss_idmap_calculate_range'
> > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_lower'
> > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_add_domain_ex'
> > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_get_upper'
> > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_unix_to_sid'
> > ./.libs/libsss_ldap_common.so: undefined reference to
> > `sss_idmap_domain_has_algorithmic_mapping'
> > clang: error: linker command failed with exit code 1 (use -v to see invocation)
> >
> > LS
>
> Thank you for the review. I squashed the patches in.

There aren't new warnigns from static analysers.

ACK

LS

[Jakub Hrozek]

On Fri, Oct 25, 2013 at 08:06:04PM +0200, Lukas Slebodnik wrote:
> On (25/10/13 19:52), Jakub Hrozek wrote:
> > On Fri, Oct 25, 2013 at 07:44:47PM +0200, Lukas Slebodnik wrote:
> > > On (25/10/13 17:45), Jakub Hrozek wrote:
> > > > On Thu, Oct 10, 2013 at 01:49:40PM +0200, Jakub Hrozek wrote:
> > > > > Hi,
> > > > >
> > > > > the attached patches implement the new option ad_access_control as
> > > > > designed in
> > > > > https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryAccessControl
> > > > >
> > > > > The only part not implemented exactly as per the design page is changing
> > > > > the default. I will write an e-mail about the issue into the thread with
> > > > > the design decision so that also people who filter out patches can
> > > > > participate.
> > > >
> > > > Attached are patches rebased on current master.
> > > > From 3f115bad21ce47bb7dee4541982613c0212ed229 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Sun, 6 Oct 2013 20:23:07 +0200
> > > > Subject: [PATCH 1/6] LDAP: Amend sdap_access_check to allow any connection
> > >
> > > ACK
> > > > From ffeaf255263671f8cc43bab3aec7b1304e076705 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Tue, 8 Oct 2013 19:10:41 +0200
> > > > Subject: [PATCH 2/6] LDAP: Parse FQDN into name/domain for subdomain users
> > >
> > > ACK
> > > > From 7b7532f68058dfad4122890743041973bb6bdfc6 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Wed, 2 Oct 2013 17:48:49 +0200
> > > > Subject: [PATCH 3/6] AD: Add a new option ad_access_filter
> > >
> > > ACK
> > > > From 1daec9096cc5ae8690b3fe567713cf9ef23096e7 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Mon, 7 Oct 2013 18:02:04 +0200
> > > > Subject: [PATCH 4/6] AD: Use the ad_access_filter if it's set
> > >
> > > ACK
> > > > From e4b1e0ca7d4078e787ccdab5d87b73aa5c8a6ab7 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Tue, 8 Oct 2013 17:50:56 +0200
> > > > Subject: [PATCH 5/6] AD: Search GC by default during access control, fall back
> > > > to LDAP
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/2082
> > > >
> > > > In order to allow the ad_access_filter option to work for subdomain
> > > > users as well, the Global Catalog must be searched. This patch adds a
> > > > wrapper request atop sdap_access_send that selects the right connection
> > > > (GC or LDAP) and optionally falls back to LDAP.
> > > >
> > > > src/providers/ad/ad_access.c | 160 +++++++++++++++++++++++++++++++++++++++++--
> > > > src/providers/ad/ad_access.h |   4 +-
> > > > src/providers/ad/ad_init.c   |   5 +-
> > > > 3 files changed, 159 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/src/providers/ad/ad_access.c b/src/providers/ad/ad_access.c
> > > > index cf6412f22c6146b26090646b5b8d060efa43365f..59e2f2b81a3f9d1850d673551be079f42a6a2ac5 100644
> > > > --- a/src/providers/ad/ad_access.c
> > > > +++ b/src/providers/ad/ad_access.c
> > > > @@ -25,10 +25,158 @@
> > > > #include "src/providers/data_provider.h"
> > > > #include "src/providers/dp_backend.h"
> > > > #include "src/providers/ad/ad_access.h"
> > > > +#include "src/providers/ad/ad_common.h"
> > > > #include "src/providers/ldap/sdap_access.h"
> > > >
> > > > static void
> > > > ad_access_done(struct tevent_req *req);
> > > > +static errno_t
> > > > +ad_access_step(struct tevent_req *req, struct sdap_id_conn_ctx *conn);
> > > >
> > > > +struct ad_access_state {
> > > > struct tevent_context *ev;
> > > > struct ad_access_ctx *ctx;
> > > > struct pam_data *pd;
> > > > struct be_ctx *be_ctx;
> > > > struct sss_domain_info *domain;
> > > > struct sdap_id_conn_ctx **clist;
> > > > int cindex;
> > > >
> > > > +};
> > > >
> > > > +static struct tevent_req *
> > > > +ad_access_send(TALLOC_CTX *mem_ctx,
> > > >           struct tevent_context *ev,
> > > >           struct be_ctx *be_ctx,
> > > >           struct sss_domain_info *domain,
> > > >           struct ad_access_ctx *ctx,
> > > >           struct pam_data *pd)
> > > >
> > > > +{
> > > > struct tevent_req *req;
> > > > struct ad_access_state *state;
> > > > errno_t ret;
> > > > req = tevent_req_create(mem_ctx, &state, struct ad_access_state);
> > > > if (req == NULL) {
> > > >    return NULL;
> > > > }
> > > > state->ev = ev;
> > > > state->ctx = ctx;
> > > > state->pd = pd;
> > > > state->be_ctx = be_ctx;
> > > > state->domain = domain;
> > > > state->clist = talloc_zero_array(state, struct sdap_id_conn_ctx *, 3);
> > > > if (state->clist == NULL) {
> > > >    ret = ENOMEM;
> > > >    goto done;
> > > > }
> > > > /* Always try GC first */
> > > > state->clist[0] = ctx->gc_ctx;
> > > > if (IS_SUBDOMAIN(domain) == false) {
> > >       This is the same situation like in file src/providers/ad/ad_id.c
> > >       If GC is misconfigured, we should not go offline.
> > >       SSSD should try to fall back to LDAP
> > > >    /* With root domain users we have the option to
> > > >     * fall back to LDAP in case ie POSIX attributes
> > > >     * are used but not replicated to GC
> > > >     */
> > > >    state->clist[1] = ctx->ldap_ctx;
> > > > }
> > > > ret = ad_access_step(req, state->clist[state->cindex]);
> > > > if (ret != EOK) {
> > > >    goto done;
> > > > }
> > > > From 8d2cfbcc0d992b26f90c9494b10f3bf666498b66 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Tue, 8 Oct 2013 20:59:22 +0200
> > > > Subject: [PATCH 6/6] AD: Add extended access filter
> > > >
> > > > https://fedorahosted.org/sssd/ticket/2082
> > > >
> > > > Adds a new option that allows the admin to specify a LDAP access filter
> > > > that can be applied globally, per-domain or per-forest.
> > > >
> > > > Makefile.am                              |  29 ++-
> > > > src/man/sssd-ad.5.xml                    |  41 +++-
> > > > src/providers/ad/ad_access.c             | 213 ++++++++++++++++++-
> > > > src/providers/ad/ad_init.c               |   5 +-
> > > > src/tests/cmocka/test_ad_access_filter.c | 341 +++++++++++++++++++++++++++++++
> > > > 5 files changed, 623 insertions(+), 6 deletions(-)
> > > > create mode 100644 src/tests/cmocka/test_ad_access_filter.c
> > > >
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 8d548e2bd7f5ffa1d9d7a46dc08026213e8e80bc..bb5136e24cdbee48d9d356ef2f2849334f397d48 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -153,7 +153,8 @@ if HAVE_CMOCKA
> > > >         dyndns-tests \
> > > >         fqnames-tests \
> > > >         test_sss_idmap \
> > > >    test_utils
> > > >    test_utils \
> > > >    ad_access_filter_tests
> > > >
> > > > endif
> > > >
> > > > check_PROGRAMS = \
> > > > @@ -1389,6 +1390,32 @@ test_utils_LDADD = \
> > > >     $(CMOCKA_LIBS) \
> > > >     $(SSSD_INTERNAL_LTLIBS) \
> > > >     libsss_test_common.la
> > > >
> > > > +ad_access_filter_tests_SOURCES = \
> > > > $(TEST_MOCK_OBJ) \
> > > > $(sssd_be_SOURCES) \
> > > > src/util/sss_ldap.c \
> > > > src/util/sss_krb5.c \
> > > > src/util/find_uid.c \
> > > > src/util/user_info_msg.c \
> > > > src/tests/cmocka/test_ad_access_filter.c
> > > >
> > > > +ad_access_filter_tests_CFLAGS = \
> > > > $(AM_CFLAGS) \
> > > > $(SYSTEMD_LOGIN_CFLAGS) \
> > > > -DUNIT_TESTING
> > > >
> > > > +ad_access_filter_tests_LDADD = \
> > > > $(PAM_LIBS) \
> > > > $(CMOCKA_LIBS) \
> > > > $(SSSD_LIBS) \
> > > > $(CARES_LIBS) \
> > > > $(KRB5_LIBS) \
> > > > $(SSSD_INTERNAL_LTLIBS) \
> > > > $(SYSTEMD_LOGIN_LIBS) \
> > > > libsss_idmap.la \
> > > > libsss_ldap_common.la \
> > > > libsss_krb5_common.la \
> > > > libsss_test_common.la
> > >
> > > Please change the order of libs.
> > > libsss_idmap.la should be after libsss_ldap_common.la, because linking will
> > > fail with LDFLAGS=-Wl,--as-needed
> > >
> > > ./.libs/libsss_ldap_common.so: undefined reference to `is_domain_sid'
> > > ./.libs/libsss_ldap_common.so: undefined reference to
> > > `sss_idmap_bin_sid_to_sid'
> > > ./.libs/libsss_ldap_common.so: undefined reference to `idmap_error_string'
> > > ./.libs/libsss_ldap_common.so: undefined reference to
> > > `sss_idmap_ctx_set_autorid'
> > > ./.libs/libsss_ldap_common.so: undefined reference to
> > > `sss_idmap_ctx_set_rangesize'
> > > ./.libs/libsss_ldap_common.so: undefined reference to
> > > `sss_idmap_domain_by_name_has_algorithmic_mapping'
> > > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_init'
> > > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_sid_to_unix'
> > > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_upper'
> > > ./.libs/libsss_ldap_common.so: undefined reference to
> > > `sss_idmap_calculate_range'
> > > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_lower'
> > > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_add_domain_ex'
> > > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_get_upper'
> > > ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_unix_to_sid'
> > > ./.libs/libsss_ldap_common.so: undefined reference to
> > > `sss_idmap_domain_has_algorithmic_mapping'
> > > clang: error: linker command failed with exit code 1 (use -v to see invocation)
> > >
> > > LS
> >
> > Thank you for the review. I squashed the patches in.
>
> There aren't new warnigns from static analysers.
>
> ACK
>
> LS

Pushed all to master and sssd-1-11