On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
On 06/22/2016 06:25 AM, Petr Cech wrote:
> On 06/14/2016 05:21 PM, Petr Cech wrote:
> > Hello list,
> >
> > there is patch set attached resolving:
> >
https://fedorahosted.org/sssd/ticket/2828
> >
> > You can also see:
> >
https://github.com/celestian/sssd/commits/subdomains_v2
> >
> >
> > This patch set is without tests yet.
> > I will send tests after Pavel B. refactor of AD provider.
> >
> >
> > I would like to remark couple of things:
> >
> > 1) Option ad_enabled_domains disabled by default.
> >
> > 2) We use labels master domain and root domain in code. Master domain
> > belongs to one that SSSD is connected to. Root domain means AD forest
> > root.
> >
> > 3) There is a lot of different environment setup. So this option has
> > slightly different meaning in each of them.
> >
> > 4) There is secret feature if you forget fill in master domain to the
> > option it will be added automatically.
> >
> >
> > What is this option good for?
> > Let's imagine environment like:
> >
> > A (forest root)
> > |
> > |-- S1 (SSSD client)
> > |
> > |-- B (AD client)
> > | |
> > | |-- S2 (SSSD client)
> > |
> > |-- C (AD client)
> > |
> > |-- D (AD client)
> >
> > A ... forest root
> > B, C, D ... AD clients
> > S1 ... SSSD client connected to A
> > S2 ... SSSD client connected to B
> >
> > We could write:
> >
> > * on S1: ad_enabled_domain = A, B, C
> > and then S1 will not attempt to connect D.
> >
> > * on S1: ad_enabled_domain = B, C
> > the same as above because A is automagically
> > filled in.
> >
> > * on S2: ad_enabled_domain = A, C
> > and then B is filled in automatically
> > and S2 will not attempt to connect D.
> >
> > * on S2: ad_enabled_domain = C
> > and then B is filled in automatically
> > and S2 will not attempt to connect D and A.
> > (I am wondering that it is possible but it
> > happened during manual testing.)
>
> Hello list,
>
> I rebased this patch set due to refactoring of the data provider. New
> patch set is attached.
>
> I will add tests, I hope soon :-)
Hello list,
there is final patch.
I added tests and I fix some char * to const char * too.
Tests cover only option handling. More complex tests should be done as
integrating tests, not as unit tests.
Hi,
see some inline comments. So far, I didn't test the patches, just read
the code.
From 63610f3be85821c6daa98bc6b2b3be438520c82d Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Fri, 13 May 2016 05:21:07 -0400
Subject: [PATCH 1/5] AD_PROVIDER: Add ad_enabled_domains option
Resolves:
https://fedorahosted.org/sssd/ticket/2828
We need to add this option to the new config schema (it wasn't in master
when you started writing the patches).
From e6b6c461ec7d4e91299b1e8290e1998e21aa9576 Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Tue, 21 Jun 2016 08:34:15 +0200
Subject: [PATCH 2/5] AD_PROVIDER: Initializing of ad_enabled_domains
We add ad_enabled_domains into ad_subdomains_ctx.
Resolves:
https://fedorahosted.org/sssd/ticket/2828
---
src/providers/ad/ad_subdomains.c | 94 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
index 5b0bee86665a46f45fde5ec1034a9ccf8700d13a..af438078deb8b103f8a24dbef7cf818fab9df538
100644
--- a/src/providers/ad/ad_subdomains.c
+++ b/src/providers/ad/ad_subdomains.c
@@ -57,6 +57,91 @@
/* do not refresh more often than every 5 seconds for now */
#define AD_SUBDOMAIN_REFRESH_LIMIT 5
+static errno_t ad_get_enabled_domains(TALLOC_CTX *mem_ctx,
+ struct ad_id_ctx *ad_id_ctx,
+ const char *ad_domain,
+ const char ***_ad_enabled_domains)
+{
+ int ret;
+ const char *str;
+ const char *option_name;
+ char **domains = NULL;
+ const char **list = NULL;
+ int count;
+ bool is_ad_in_domains;
+ TALLOC_CTX *tmp_ctx = NULL;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ return ENOMEM;
+ }
+
+ str = dp_opt_get_cstring(ad_id_ctx->ad_options->basic, AD_ENABLED_DOMAINS);
+ if (str == NULL) {
+ _ad_enabled_domains = NULL;
+ ret = EOK;
+ goto done;
+ }
+
+ count = 0;
+ ret = split_on_separator(tmp_ctx, str, ',', true, true, &domains,
&count);
+ if (ret != EOK) {
+ option_name = ad_id_ctx->ad_options->basic[AD_ENABLED_DOMAINS].opt_name;
+ DEBUG(SSSDBG_CRIT_FAILURE, "Failed to parse option [%s], [%i]
[%s]!\n",
+ option_name, ret, sss_strerror(ret));
+ ret = EINVAL;
+ goto done;
+ }
+
+ list = talloc_array_size(tmp_ctx, sizeof(char*), count);
+ if (list == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ is_ad_in_domains = false;
+ for (int i = 0; i < count; i++) {
+ list[i] = talloc_strdup(list, domains[i]);
Do we need to duplicate the string here, wouldn't it be enough to steal
it?
+ if (list[i] == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ is_ad_in_domains += strcmp(ad_domain, domains[i]) == 0 ? true : false;
+ }
+
+ if (is_ad_in_domains == false) {
+ list = talloc_realloc(tmp_ctx, list, const char*, count + 1);
+ if (list == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ list[count] = talloc_strdup(list, ad_domain);
+ if (list[count] == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ count++;
+ }
+
+ list = talloc_realloc(tmp_ctx, list, const char*, count + 1);
Why don't you allocate count+1 from the start but realloc here instead?
+ if (list == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ list[count] = NULL;
+ count++;
+
+ *_ad_enabled_domains = talloc_steal(mem_ctx, list);
+ ret = EOK;
+
+done:
+ talloc_free(tmp_ctx);
+ return ret;
+}
From 6f5f06a892ff49c256e2476b5c365709cbf73164 Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Tue, 21 Jun 2016 09:48:52 +0200
Subject: [PATCH 3/5] AD_PROVIDER: ad_enabled_domains - only master
We can skip looking up other domains if option ad_enabled_domains
contains only master domain.
Resolves:
https://fedorahosted.org/sssd/ticket/2828
---
src/providers/ad/ad_subdomains.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
index af438078deb8b103f8a24dbef7cf818fab9df538..6fa134eed69aa8f0a672bc53c9b0d7dd0d7f37b1
100644
--- a/src/providers/ad/ad_subdomains.c
+++ b/src/providers/ad/ad_subdomains.c
@@ -1170,6 +1170,7 @@ static void ad_subdomains_refresh_connect_done(struct tevent_req
*subreq)
return;
}
+ /* connect to the DC we are a member of */
subreq = ad_master_domain_send(state, state->ev, state->id_ctx->conn,
state->sdap_op,
state->sd_ctx->domain_name);
if (subreq == NULL) {
@@ -1219,6 +1220,21 @@ static void ad_subdomains_refresh_master_done(struct tevent_req
*subreq)
goto done;
}
+ /*
+ * If ad_enabled_domains contains only master domain
+ * we shouldn't lookup other domains.
+ */
+ if (state->sd_ctx->ad_enabled_domains != NULL) {
+ if (talloc_array_length(state->sd_ctx->ad_enabled_domains) == 2) {
+ if (strcmp(state->sd_ctx->ad_enabled_domains[0],
+ state->be_ctx->domain->name) == 0) {
+ DEBUG(SSSDBG_TRACE_FUNC,
+ "No other enabled domain than master.\n");
+ goto done;
+ }
+ }
+ }
+
subreq = ad_get_root_domain_send(state, state->ev, forest,
sdap_id_op_handle(state->sdap_op),
state->sd_ctx);
--
2.5.5
From 3dcc8efe27567bfa5300db1dd5e971571b671706 Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Mon, 27 Jun 2016 11:51:30 +0200
Subject: [PATCH 4/5] AD_PROVIDER: ad_enabled_domains - other then master
We can skip looking up other domains if
option ad_enabled_domains doesn't contain them.
Resolves:
https://fedorahosted.org/sssd/ticket/2828
---
src/providers/ad/ad_subdomains.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
index 6fa134eed69aa8f0a672bc53c9b0d7dd0d7f37b1..f9e4ac57904514c89bcc3a8160194fd0be246a21
100644
--- a/src/providers/ad/ad_subdomains.c
+++ b/src/providers/ad/ad_subdomains.c
@@ -497,6 +497,7 @@ done:
static errno_t ad_subdomains_process(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
+ const char **enabled_domains_list,
size_t nsd, struct sysdb_attrs **sd,
struct sysdb_attrs *root,
size_t *_nsd_out,
@@ -505,9 +506,11 @@ static errno_t ad_subdomains_process(TALLOC_CTX *mem_ctx,
size_t i, sdi;
struct sysdb_attrs **sd_out;
const char *sd_name;
+ const bool is_sd_filtered = (enabled_domains_list == NULL) ? false : true;
+ bool is_sd_enabled;
errno_t ret;
- if (root == NULL) {
+ if (root == NULL && is_sd_filtered == false) {
/* We are connected directly to the root domain. The 'sd'
* list is complete and we can just use it
*/
@@ -534,6 +537,17 @@ static errno_t ad_subdomains_process(TALLOC_CTX *mem_ctx,
goto fail;
}
+ is_sd_enabled = true;
+ if (is_sd_filtered == true) {
+ is_sd_enabled = false;
+ for (size_t j = 0; enabled_domains_list[j] != NULL; j++) {
+ if (strcmp(sd_name, enabled_domains_list[j]) == 0) {
+ is_sd_enabled = true;
+ break;
+ }
+ }
+ }
+
Wouldn't it be easier to just call
if (string_in_list(sd_name, enabled_domains_list) == false) {
continue;
}
and always append the domain if neither the filtering condition nor the
subdomain-is-root matches?
From b8ce2c188f081ac3073c68c4b57e0371984faa58 Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Mon, 27 Jun 2016 11:53:19 +0200
Subject: [PATCH 5/5] TESTS: Adding tests for ad_enabled_domains option
There is special logic around ad_enabled_domains option:
* option is disabled by default
* master domain is always added to enabled domains
Resolves:
https://fedorahosted.org/sssd/ticket/2828
LGTM.