[SSSD] Re: [PATCH SET] AD_PROVIDER: ad_enabled_domains

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@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@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@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@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@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.