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 <jhrozek(a)redhat.com>
>> >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 <jhrozek(a)redhat.com>
>> >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 <jhrozek(a)redhat.com>
>> >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 <jhrozek(a)redhat.com>
>> >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 <jhrozek(a)redhat.com>
>> >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 <jhrozek(a)redhat.com>
>> >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