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.)
Regards
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:
Option ad_enabled_domains disabled by default.
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.
- There is a lot of different environment setup. So this option has
slightly different meaning in each of them.
- 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 :-)
Regards
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:
Option ad_enabled_domains disabled by default.
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.
- There is a lot of different environment setup. So this option has
slightly different meaning in each of them.
- 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.
Regards
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:
Option ad_enabled_domains disabled by default.
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.
- There is a lot of different environment setup. So this option has
slightly different meaning in each of them.
- 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
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
LGTM.
On 07/05/2016 08:44 AM, Jakub Hrozek wrote:
On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
...
+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?
This is my attempt to satisfy the needs. I am trying to make: # const char** from char** I would like to return const char** because it is option. But the function split_on_separator() expects char**.
Does anybody know better solution?
Regards
On 07/11/2016 05:12 PM, Petr Cech wrote:
On 07/05/2016 08:44 AM, Jakub Hrozek wrote:
On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
...
+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?
This is my attempt to satisfy the needs. I am trying to make: # const char** from char** I would like to return const char** because it is option. But the function split_on_separator() expects char**.
Does anybody know better solution?
You can use discard_const for this case.
Regards
On 07/12/2016 10:31 AM, Pavel Březina wrote:
On 07/11/2016 05:12 PM, Petr Cech wrote:
On 07/05/2016 08:44 AM, Jakub Hrozek wrote:
On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
...
+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?
This is my attempt to satisfy the needs. I am trying to make: # const char** from char** I would like to return const char** because it is option. But the function split_on_separator() expects char**.
Does anybody know better solution?
You can use discard_const for this case.
Hi list,
I addressed all comments, new patch set is attached. Thanks for review and for help with discard_const(_p).
I am afraid that my patch set is not fully compatible with dp_refactor. It is not unfortunately fully tested yet. I have 'test manual' which I would like to pass before I can say work is done.
I hit issue with my local test environment (LTE) after pulling to recent master. My LTE consists of:
A (forest root) | |-- S1 (SSSD client) | |-- B (AD client) | |-- S2 (SSSD client)
S2 box is fine but S1 box has troubles... I make new S1 with new domain name, connect to A with 'sudo real join s1.domain' and I start SSSD. It crash very quickly.
Attachment data.tar.bz2 contains log and sssd.conf
Could you help me, please?
Regards...
On 07/12/2016 10:31 AM, Pavel Březina wrote:
On 07/11/2016 05:12 PM, Petr Cech wrote:
On 07/05/2016 08:44 AM, Jakub Hrozek wrote:
On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
...
+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?
This is my attempt to satisfy the needs. I am trying to make: # const char** from char** I would like to return const char** because it is option. But the function split_on_separator() expects char**.
Does anybody know better solution?
You can use discard_const for this case.
Hi list,
I addressed all comments, new patch set is attached. Thanks for review and for help with discard_const(_p).
I am afraid that my patch set is not fully compatible with dp_refactor. It is not unfortunately fully tested yet. I have 'test manual' which I would like to pass before I can say work is done.
I hit issue with my local test environment (LTE) after pulling to recent master. My LTE consists of:
A (forest root) | |-- S1 (SSSD client) | |-- B (AD client) | |-- S2 (SSSD client)
S2 box is fine but S1 box has troubles... I make new S1 with new domain name, connect to A with 'sudo real join s1.domain' and I start SSSD. It crash very quickly.
Attachment data.tar.bz2 contains log and sssd.conf
Could you help me, please?
Regards...
On 07/12/2016 10:31 AM, Pavel Březina wrote:
On 07/11/2016 05:12 PM, Petr Cech wrote:
On 07/05/2016 08:44 AM, Jakub Hrozek wrote:
On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
...
+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?
This is my attempt to satisfy the needs. I am trying to make: # const char** from char** I would like to return const char** because it is option. But the function split_on_separator() expects char**.
Does anybody know better solution?
You can use discard_const for this case.
Hi list,
I pulled master again and it started to work :-)
Regards...
On Fri, Jul 15, 2016 at 03:24:15PM +0200, Petr Cech wrote:
On 07/12/2016 10:31 AM, Pavel Březina wrote:
On 07/11/2016 05:12 PM, Petr Cech wrote:
On 07/05/2016 08:44 AM, Jakub Hrozek wrote:
On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
...
+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?
This is my attempt to satisfy the needs. I am trying to make: # const char** from char** I would like to return const char** because it is option. But the function split_on_separator() expects char**.
Does anybody know better solution?
You can use discard_const for this case.
Hi list,
I pulled master again and it started to work :-)
Regards...
I finally tested the patches and they work fine in my setup of two domains (root and child domain).
I'll also install and configure a third AD domain to do some more testing, but in the meantime, I tested that a user from disabled subdomain can't be lookup up and a user who is a member of a group from another domain reports correct group membership (the tokengroups code stores a range that corresponds to the trusted domain, but not the domain, subsequent lookup of that ID "fails" with idmap domain not found).
So functional ACK for now, I just need to do more testing with a third domain.
It would also be nice to add some debug messages, but that's a detail..
On Wed, Jul 20, 2016 at 12:24:11PM +0200, Jakub Hrozek wrote:
On Fri, Jul 15, 2016 at 03:24:15PM +0200, Petr Cech wrote:
On 07/12/2016 10:31 AM, Pavel Březina wrote:
On 07/11/2016 05:12 PM, Petr Cech wrote:
On 07/05/2016 08:44 AM, Jakub Hrozek wrote:
On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
...
+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?
This is my attempt to satisfy the needs. I am trying to make: # const char** from char** I would like to return const char** because it is option. But the function split_on_separator() expects char**.
Does anybody know better solution?
You can use discard_const for this case.
Hi list,
I pulled master again and it started to work :-)
Regards...
I finally tested the patches and they work fine in my setup of two domains (root and child domain).
I'll also install and configure a third AD domain to do some more testing, but in the meantime, I tested that a user from disabled subdomain can't be lookup up and a user who is a member of a group from another domain reports correct group membership (the tokengroups code stores a range that corresponds to the trusted domain, but not the domain, subsequent lookup of that ID "fails" with idmap domain not found).
So functional ACK for now, I just need to do more testing with a third domain.
OK, the patchset doesn't work with a third domain:
Program received signal SIGSEGV, Segmentation fault. 0x00007f9aac975716 in __strcmp_sse2_unaligned () from /lib64/libc.so.6 Missing separate debuginfos, use: dnf debuginfo-install gssproxy-0.4.1-4.fc23.x86_64 (gdb) bt #0 0x00007f9aac975716 in __strcmp_sse2_unaligned () from /lib64/libc.so.6 #1 0x00007f9ab0c5c24f in string_in_list (string=0xf22e50 "subdom.win.trust.test", list=0x7fff84129408, case_sensitive=true) at /sssd/src/util/util.c:712 #2 0x00007f9aa844e5a2 in ad_subdomains_process (mem_ctx=0xf26150, domain=0xe9c7f0, enabled_domains_list=0xef76a0, nsd=2, sd=0xf14960, root=0x0, _nsd_out=0x7fff841294e8, _sd_out=0x7fff841294e0) at /sssd/src/providers/ad/ad_subdomains.c:538 #3 0x00007f9aa844f272 in ad_get_slave_domain_done (subreq=0x0) at /sssd/src/providers/ad/ad_subdomains.c:809 #4 0x00007f9aa77142f0 in sdap_search_bases_ex_done (subreq=0x0) at /sssd/src/providers/ldap/sdap_ops.c:222 #5 0x00007f9aa76d94b5 in sdap_get_generic_done (subreq=0xf22710) at /sssd/src/providers/ldap/sdap_async.c:1871 #6 0x00007f9aa76d8eff in generic_ext_search_handler (subreq=0x0, opts=0xee9690) at /sssd/src/providers/ldap/sdap_async.c:1688 #7 0x00007f9aa76d925d in sdap_get_and_parse_generic_done (subreq=0xf23cc0) at /sssd/src/providers/ldap/sdap_async.c:1796 #8 0x00007f9aa76d89d2 in sdap_get_generic_op_finished (op=0xf24b00, reply=0xf14960, error=0, pvt=0xf23cc0) at /sssd/src/providers/ldap/sdap_async.c:1578 #9 0x00007f9aa76d4c34 in sdap_process_message (ev=0xe7bc60, sh=0xf11710, msg=0xf25c10) at /sssd/src/providers/ldap/sdap_async.c:353 #10 0x00007f9aa76d47b3 in sdap_process_result (ev=0xe7bc60, pvt=0xf11710) at /sssd/src/providers/ldap/sdap_async.c:197 #11 0x00007f9aa76d4476 in sdap_ldap_next_result (ev=0xe7bc60, te=0xf24a40, tv=..., pvt=0xf11710) at /sssd/src/providers/ldap/sdap_async.c:145 #12 0x00007f9aad2bcb2d in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #13 0x00007f9aad2bdb5a in epoll_event_loop_once () from /lib64/libtevent.so.0 ---Type <return> to continue, or q <return> to quit--- #14 0x00007f9aad2bc257 in std_event_loop_once () from /lib64/libtevent.so.0 #15 0x00007f9aad2b842d in _tevent_loop_once () from /lib64/libtevent.so.0 #16 0x00007f9aad2b85cb in tevent_common_loop_wait () from /lib64/libtevent.so.0 #17 0x00007f9aad2bc1f7 in std_event_loop_wait () from /lib64/libtevent.so.0 #18 0x00007f9ab0c605d2 in server_loop (main_ctx=0xe7d0b0) at /sssd/src/util/server.c:702 #19 0x0000000000409372 in main (argc=8, argv=0x7fff84129ca8) at /sssd/src/providers/data_provider_be.c:587 (gdb) frame 1 #1 0x00007f9ab0c5c24f in string_in_list (string=0xf22e50 "subdom.win.trust.test", list=0x7fff84129408, case_sensitive=true) at /sssd/src/util/util.c:712 712 if (compare(string, list[c]) == 0) { (gdb) p string $1 = 0xf22e50 "subdom.win.trust.test" (gdb) p list[c] $2 = 0x100000000ea03b0 <error: Cannot access memory at address 0x100000000ea03b0> (gdb)
In sssd.conf I had: ad_enabled_domains = win.trust.test, siblingdom.win.trust.test
On 07/21/2016 10:21 AM, Jakub Hrozek wrote:
On Wed, Jul 20, 2016 at 12:24:11PM +0200, Jakub Hrozek wrote:
On Fri, Jul 15, 2016 at 03:24:15PM +0200, Petr Cech wrote:
On 07/12/2016 10:31 AM, Pavel Březina wrote:
On 07/11/2016 05:12 PM, Petr Cech wrote:
On 07/05/2016 08:44 AM, Jakub Hrozek wrote:
On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote:
...
> +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?
This is my attempt to satisfy the needs. I am trying to make: # const char** from char** I would like to return const char** because it is option. But the function split_on_separator() expects char**.
Does anybody know better solution?
You can use discard_const for this case.
Hi list,
I pulled master again and it started to work :-)
Regards...
I finally tested the patches and they work fine in my setup of two domains (root and child domain).
I'll also install and configure a third AD domain to do some more testing, but in the meantime, I tested that a user from disabled subdomain can't be lookup up and a user who is a member of a group from another domain reports correct group membership (the tokengroups code stores a range that corresponds to the trusted domain, but not the domain, subsequent lookup of that ID "fails" with idmap domain not found).
So functional ACK for now, I just need to do more testing with a third domain.
OK, the patchset doesn't work with a third domain:
Program received signal SIGSEGV, Segmentation fault. 0x00007f9aac975716 in __strcmp_sse2_unaligned () from /lib64/libc.so.6 Missing separate debuginfos, use: dnf debuginfo-install gssproxy-0.4.1-4.fc23.x86_64 (gdb) bt #0 0x00007f9aac975716 in __strcmp_sse2_unaligned () from /lib64/libc.so.6 #1 0x00007f9ab0c5c24f in string_in_list (string=0xf22e50 "subdom.win.trust.test", list=0x7fff84129408, case_sensitive=true) at /sssd/src/util/util.c:712 #2 0x00007f9aa844e5a2 in ad_subdomains_process (mem_ctx=0xf26150, domain=0xe9c7f0, enabled_domains_list=0xef76a0, nsd=2, sd=0xf14960, root=0x0, _nsd_out=0x7fff841294e8,
problem is here ----- ^
Thanks for testing. It is in a call of the function string_in_list() as I assumed. I will fix it late today.
Regards
On 07/21/2016 10:21 AM, Jakub Hrozek wrote:
On Wed, Jul 20, 2016 at 12:24:11PM +0200, Jakub Hrozek wrote:
On Fri, Jul 15, 2016 at 03:24:15PM +0200, Petr Cech wrote:
On 07/12/2016 10:31 AM, Pavel Březina wrote:
> On 07/11/2016 05:12 PM, Petr Cech wrote: > > > > > > On 07/05/2016 08:44 AM, Jakub Hrozek wrote: > > > > > On Mon, Jun 27, 2016 at 03:37:25PM +0200, Petr Cech wrote: > > > > > > ... > > > > > > > > > > +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? > > > > > > This is my attempt to satisfy the needs. I am trying to make: > > > # const char** from char** > > > I would like to return const char** because it is option. But the > > > function split_on_separator() expects char**. > > > > > > Does anybody know better solution? > > You can use discard_const for this case. >
Hi list,
I pulled master again and it started to work :-)
Regards...
I finally tested the patches and they work fine in my setup of two domains (root and child domain).
I'll also install and configure a third AD domain to do some more testing, but in the meantime, I tested that a user from disabled subdomain can't be lookup up and a user who is a member of a group from another domain reports correct group membership (the tokengroups code stores a range that corresponds to the trusted domain, but not the domain, subsequent lookup of that ID "fails" with idmap domain not found).
So functional ACK for now, I just need to do more testing with a third domain.
OK, the patchset doesn't work with a third domain:
Program received signal SIGSEGV, Segmentation fault. 0x00007f9aac975716 in __strcmp_sse2_unaligned () from /lib64/libc.so.6 Missing separate debuginfos, use: dnf debuginfo-install gssproxy-0.4.1-4.fc23.x86_64 (gdb) bt #0 0x00007f9aac975716 in __strcmp_sse2_unaligned () from /lib64/libc.so.6 #1 0x00007f9ab0c5c24f in string_in_list (string=0xf22e50 "subdom.win.trust.test", list=0x7fff84129408, case_sensitive=true) at /sssd/src/util/util.c:712 #2 0x00007f9aa844e5a2 in ad_subdomains_process (mem_ctx=0xf26150, domain=0xe9c7f0, enabled_domains_list=0xef76a0, nsd=2, sd=0xf14960, root=0x0, _nsd_out=0x7fff841294e8, _sd_out=0x7fff841294e0) at /sssd/src/providers/ad/ad_subdomains.c:538 #3 0x00007f9aa844f272 in ad_get_slave_domain_done (subreq=0x0) at /sssd/src/providers/ad/ad_subdomains.c:809 #4 0x00007f9aa77142f0 in sdap_search_bases_ex_done (subreq=0x0) at /sssd/src/providers/ldap/sdap_ops.c:222 #5 0x00007f9aa76d94b5 in sdap_get_generic_done (subreq=0xf22710) at /sssd/src/providers/ldap/sdap_async.c:1871 #6 0x00007f9aa76d8eff in generic_ext_search_handler (subreq=0x0, opts=0xee9690) at /sssd/src/providers/ldap/sdap_async.c:1688 #7 0x00007f9aa76d925d in sdap_get_and_parse_generic_done (subreq=0xf23cc0) at /sssd/src/providers/ldap/sdap_async.c:1796 #8 0x00007f9aa76d89d2 in sdap_get_generic_op_finished (op=0xf24b00, reply=0xf14960, error=0, pvt=0xf23cc0) at /sssd/src/providers/ldap/sdap_async.c:1578 #9 0x00007f9aa76d4c34 in sdap_process_message (ev=0xe7bc60, sh=0xf11710, msg=0xf25c10) at /sssd/src/providers/ldap/sdap_async.c:353 #10 0x00007f9aa76d47b3 in sdap_process_result (ev=0xe7bc60, pvt=0xf11710) at /sssd/src/providers/ldap/sdap_async.c:197 #11 0x00007f9aa76d4476 in sdap_ldap_next_result (ev=0xe7bc60, te=0xf24a40, tv=..., pvt=0xf11710) at /sssd/src/providers/ldap/sdap_async.c:145 #12 0x00007f9aad2bcb2d in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #13 0x00007f9aad2bdb5a in epoll_event_loop_once () from /lib64/libtevent.so.0 ---Type <return> to continue, or q <return> to quit--- #14 0x00007f9aad2bc257 in std_event_loop_once () from /lib64/libtevent.so.0 #15 0x00007f9aad2b842d in _tevent_loop_once () from /lib64/libtevent.so.0 #16 0x00007f9aad2b85cb in tevent_common_loop_wait () from /lib64/libtevent.so.0 #17 0x00007f9aad2bc1f7 in std_event_loop_wait () from /lib64/libtevent.so.0 #18 0x00007f9ab0c605d2 in server_loop (main_ctx=0xe7d0b0) at /sssd/src/util/server.c:702 #19 0x0000000000409372 in main (argc=8, argv=0x7fff84129ca8) at /sssd/src/providers/data_provider_be.c:587 (gdb) frame 1 #1 0x00007f9ab0c5c24f in string_in_list (string=0xf22e50 "subdom.win.trust.test", list=0x7fff84129408, case_sensitive=true) at /sssd/src/util/util.c:712 712 if (compare(string, list[c]) == 0) { (gdb) p string $1 = 0xf22e50 "subdom.win.trust.test" (gdb) p list[c] $2 = 0x100000000ea03b0 <error: Cannot access memory at address 0x100000000ea03b0> (gdb)
In sssd.conf I had: ad_enabled_domains = win.trust.test, siblingdom.win.trust.test
Hello,
there is fixed patch set attached.
Segmentation fault was caused by wrong pointer :-(, sorry.
This new patch set has new debug message. I am open to dissccus the debug_level and content of message. Any improving idea?
I hit one issue during testing -- sometimes if I am connected to subdomain and I enable only sibling subdomain (the master is added automaticaly) and forest root is not enabled -- I see only master and sibling not. But if I added sleep for cycle (for using dbg) to function ad_subdomains_init() everythink is OK. Any idea?
Anyway, I would like to ask you for testing.
Regards
On Mon, Jul 25, 2016 at 06:18:28PM +0200, Petr Cech wrote:
Hello,
there is fixed patch set attached.
Segmentation fault was caused by wrong pointer :-(, sorry.
This new patch set has new debug message. I am open to dissccus the debug_level and content of message. Any improving idea?
I hit one issue during testing -- sometimes if I am connected to subdomain and I enable only sibling subdomain (the master is added automaticaly) and forest root is not enabled -- I see only master and sibling not. But if I added sleep for cycle (for using dbg) to function ad_subdomains_init() everythink is OK. Any idea?
Can you test that case with valgrind? This sounds like some uninitilized variable condition.
Anyway, I would like to ask you for testing.
Regards
-- Petr^4 Čech
From 7fd9ad8f42f274463c1d107b195d21290fd0b0f2 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
ACK
From dfa6e063d3ef4850a7809e2f5a6d3826ea061b27 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 | 82 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+)
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c index e9da04e384e598927f9c8c203a751bcccd29e895..9c0afb19418e44a3e3daa661baf1c7a82439d60d 100644 --- a/src/providers/ad/ad_subdomains.c +++ b/src/providers/ad/ad_subdomains.c @@ -57,6 +57,79 @@ /* 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;
- const char **domains = 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;
I think you wanted to dereference the pointer here? (it might also be a good idea to return EINVAL if the caller passed NULL as the output pointer)
ret = EOK;goto done;- }
From 4cd5c77f09750f9eb20c91eadc4759c3e4166093 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 9c0afb19418e44a3e3daa661baf1c7a82439d60d..eaa85f802dbb4022dc632cc4c05d89685eccd901 100644 --- a/src/providers/ad/ad_subdomains.c +++ b/src/providers/ad/ad_subdomains.c @@ -1163,6 +1163,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) {
@@ -1211,6 +1212,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) {
I only notices this now, but IIRC the domains are case insensitive, shouldn't we use strcasecmp here?
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.7.4
From 0d2b0c67875663d0e614b8361152ec7edc14c37e 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 | 49 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-)
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c index eaa85f802dbb4022dc632cc4c05d89685eccd901..32f7d7b3b2c4917d15d516c5e8beac159ffe164f 100644 --- a/src/providers/ad/ad_subdomains.c +++ b/src/providers/ad/ad_subdomains.c @@ -130,6 +130,23 @@ done: return ret; }
+static bool is_domain_enabled (const char *domain,
const char **enabled_doms)
^ We don't put a space between function name and the argument list.
+{
- bool ret;
- if (enabled_doms == NULL) {
ret = true;goto done;
Do we really need to use jump labels instead of just returning true?
- }
- ret = string_in_list(domain, discard_const_p(char *, enabled_doms), true) ?
true : false;
Can you also fix the indentation here? We could also just return the function return value and get rid of the 'ret' variable completely.
+done:
- return ret;
+}
static errno_t ad_subdom_ad_ctx_new(struct be_ctx *be_ctx, struct ad_id_ctx *id_ctx, @@ -485,6 +502,7 @@ done:
From 92af52a3b4d8fa138282030a72122d6cbe9c5413 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
ACK
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jakub Hrozek -
Pavel Březina -
Petr Cech