On Tue, Nov 13, 2012 at 05:23:13PM +0100, Jakub Hrozek wrote: > On Tue, Nov 13, 2012 at 01:43:25PM +0100, Sumit Bose wrote: > > Hi, > > > > the following two patches fix https://fedorahosted.org/sssd/ticket/1613 > > by running the PAC responder and the subdomains provider for an IPA > > domain with being explicitly configured, i.e. the sssd.conf file > > created by ipa-client-install does not have to be changed if trusts are > > enabled in the server. Please see commit messages for further details. > > > > bye, > > Sumit > > Hi, > > The functionality seems to be working as expected for both a client enrolled > with an IPA server with trusts and without. I only have a couple of comments:
Thank you for the review. > > Maybe it would be nice to split the first patch into two - one that > includes the new functions add_string_to_list and string_in_list and one > that uses them in monitor. I don't require this change, though. done > > Given that the pac responder is only compiled on systems with reasonably > new libkrb5, I think the pac service should only be added for > configurations that have the pac service compiled in. done, I hope the way I tried to avoid ifdef's in the main code is not too pedantic. > > Is there a way to disable the automatic enabling of pac responder for > environments that only want to run the minimal required set of services? > Maybe we could have a separate ticket to not spawn pac responder if the > subdomains_provider is explicitly set to none. This doesn't block > inclusion of new patches either. Simo had the idea to add an option for the PAM responder to enable/disable the PAC responder as well. I'm not sure if he already opened a ticket for this. I would prefer this approach because there might be setups where you do not want/need the subdomains provider but the PAC responder. > > There is a compilation warning: > src/monitor/monitor.c: In function ‘add_implicit_services’: > src/monitor/monitor.c:855:5: warning: passing argument 2 of > ‘string_in_list’ from incompatible pointer type [enabled by default] > In file included from src/monitor/monitor.c:22:0: > ./src/util/util.h:542:6: note: expected ‘const char **’ but argument is > of type ‘char **’ fixed > > I asked our docs expert to check the man page paragraph in the second > patch, here is what she proposed: > + <refsect1 id='subdomains_provider'> > + <title>SUBDOMAINS PROVIDER</title> > + <para> > + The IPA subdomains provider behaves slightly differently > + if it is configured explicitly or implicitly. > + </para> > + <para> > + If the option 'subdomains_provider = ipa' is found in the > + domain section of sssd.conf, the IPA subdomains provider is > + configured explicitly, and all subdomain requests are sent to the > + IPA server if necessary. > + </para> > + <para> > + If the option 'subdomains_provider' is not set in the domain > + section of sssd.conf but there is the option 'id_provider = > ipa,' the IPA subdomains > + provider is configured implictly. In this case, if a subdomain > + request fails and indicates that the server does not support > + subdomains, i.e. is not configured for trusts, the IPA subdomains > + provider is disabled. After an hour or after the IPA provider > goes > + online, the subdomains provider is enabled again. > + </para> > + </refsect1> added the new version. New patches attached. bye, Sumit > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From bddfb5f9663893662294bac5e881abe6dad8c9c1 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Tue, 13 Nov 2012 21:20:32 +0100 Subject: [PATCH 1/3] Add string_in_list() and add_string_to_list() with tests string_in_list() and add_string_to_list() are two utilities for NULL terminated strings arrays. add_string_to_list() adds a new string to an existing list or creates a new one with the strings as only item if there is not list. string_in_list() checks if a given string is in the list. It can be used case sensitive or in-sensitive. --- src/tests/util-tests.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.c | 63 ++++++++++++++++++++++++++++++++++++ src/util/util.h | 6 +++ 3 files changed, 152 insertions(+), 0 deletions(-) diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c index 80c03c7..ff809a5 100644 --- a/src/tests/util-tests.c +++ b/src/tests/util-tests.c @@ -37,6 +37,84 @@ char *filename; int atio_fd; +START_TEST(test_add_string_to_list) +{ + int ret; + + char **list = NULL; + + ret = add_string_to_list(NULL, NULL, NULL); + fail_unless(ret == EINVAL, "NULL input accepted"); + + ret = add_string_to_list(global_talloc_context, "ABC", &list); + fail_unless(ret == EOK, "Adding string to non-existing list failed."); + fail_unless(list != NULL, "No new list created."); + fail_unless(list[0] != NULL, "String not added to new list."); + fail_unless(strcmp(list[0], "ABC") == 0, + "Wrong string added to newly created list."); + fail_unless(list[1] == NULL, + "Missing terminating NULL in newly created list."); + + ret = add_string_to_list(global_talloc_context, "DEF", &list); + fail_unless(ret == EOK, "Adding string to list failed."); + fail_unless(list != NULL, "No list returned."); + fail_unless(strcmp(list[0], "ABC") == 0, "Wrong first string in new list."); + fail_unless(strcmp(list[1], "DEF") == 0, "Wrong string added to list."); + fail_unless(list[2] == NULL, "Missing terminating NULL."); + + list[0] = NULL; + ret = add_string_to_list(global_talloc_context, "ABC", &list); + fail_unless(ret == EOK, "Adding string to empty list failed."); + fail_unless(list != NULL, "No list returned."); + fail_unless(list[0] != NULL, "String not added to empty list."); + fail_unless(strcmp(list[0], "ABC") == 0, + "Wrong string added to empty list."); + fail_unless(list[1] == NULL, + "Missing terminating NULL in newly created list."); + + talloc_free(list); +} +END_TEST + +START_TEST(test_string_in_list) +{ + bool is_in; + char *empty_list[] = {NULL}; + char *list[] = {discard_const("ABC"), + discard_const("DEF"), + discard_const("GHI"), + NULL}; + + is_in = string_in_list(NULL, NULL, false); + fail_unless(!is_in, "NULL string is in NULL list."); + + is_in = string_in_list(NULL, empty_list, false); + fail_unless(!is_in, "NULL string is in empty list."); + + is_in = string_in_list(NULL, list, false); + fail_unless(!is_in, "NULL string is in list."); + + is_in = string_in_list("ABC", NULL, false); + fail_unless(!is_in, "String is in NULL list."); + + is_in = string_in_list("ABC", empty_list, false); + fail_unless(!is_in, "String is in empty list."); + + is_in = string_in_list("ABC", list, false); + fail_unless(is_in, "String is not list."); + + is_in = string_in_list("abc", list, false); + fail_unless(is_in, "String is not case in-sensitive list."); + + is_in = string_in_list("abc", list, true); + fail_unless(!is_in, "Wrong string found in case sensitive list."); + + is_in = string_in_list("123", list, false); + fail_unless(!is_in, "Wrong string found in list."); + +} +END_TEST + START_TEST(test_parse_args) { struct pa_testcase { @@ -646,10 +724,15 @@ Suite *util_suite(void) TCase *tc_util = tcase_create("util"); + tcase_add_checked_fixture(tc_util, + leak_check_setup, + leak_check_teardown); tcase_add_test (tc_util, test_diff_string_lists); tcase_add_test (tc_util, test_sss_filter_sanitize); tcase_add_test (tc_util, test_size_t_overflow); tcase_add_test (tc_util, test_parse_args); + tcase_add_test (tc_util, test_add_string_to_list); + tcase_add_test (tc_util, test_string_in_list); tcase_set_timeout(tc_util, 60); TCase *tc_utf8 = tcase_create("utf8"); diff --git a/src/util/util.c b/src/util/util.c index b812ef1..18df0e8 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -634,3 +634,66 @@ remove_ipv6_brackets(char *ipv6addr) return EOK; } + +errno_t add_string_to_list(TALLOC_CTX *mem_ctx, const char *string, + char ***list_p) +{ + size_t c; + char **old_list = NULL; + char **new_list = NULL; + + if (string == NULL || list_p == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("Missing string or list.\n")); + return EINVAL; + } + + old_list = *list_p; + + if (old_list == NULL) { + /* If the input is a NULL list a new one is created with the new + * string and the terminating NULL element. */ + c = 0; + new_list = talloc_array(mem_ctx, char *, 2); + } else { + for (c = 0; old_list[c] != NULL; c++); + new_list = talloc_realloc(mem_ctx, old_list, char *, c + 1); + } + + if (new_list == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_array/talloc_realloc failed.\n")); + return ENOMEM; + } + + new_list[c] = talloc_strdup(new_list, string); + if (new_list[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed.\n")); + talloc_free(new_list); + return ENOMEM; + } + + new_list[c + 1] = NULL; + + *list_p = new_list; + + return EOK; +} + +bool string_in_list(const char *string, char **list, bool case_sensitive) +{ + size_t c; + int(*compare)(const char *s1, const char *s2); + + if (string == NULL || list == NULL || *list == NULL) { + return false; + } + + compare = case_sensitive ? strcmp : strcasecmp; + + for (c = 0; list[c] != NULL; c++) { + if (compare(string, list[c]) == 0) { + return true; + } + } + + return false; +} diff --git a/src/util/util.h b/src/util/util.h index b6ecfc2..56e7e0b 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -535,6 +535,12 @@ sss_escape_ip_address(TALLOC_CTX *mem_ctx, int family, const char *addr); errno_t remove_ipv6_brackets(char *ipv6addr); + +errno_t add_string_to_list(TALLOC_CTX *mem_ctx, const char *string, + char ***list_p); + +bool string_in_list(const char *string, char **list, bool case_sensitive); + /* from sss_tc_utf8.c */ char * sss_tc_utf8_str_tolower(TALLOC_CTX *mem_ctx, const char *s); -- 1.7.7.6
From 66c3219637be9bf98aec7bf691a0a70e6666e101 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Tue, 13 Nov 2012 21:21:38 +0100 Subject: [PATCH 2/3] Always start PAC responder if IPA ID provider is configured Since the PAC responder is used during the authentication of users from trusted realms it is started automatically if the IPA ID provider is configured for a domain to simplify the configuration. Fixes https://fedorahosted.org/sssd/ticket/1613 --- src/man/sssd-ipa.5.xml | 6 ++++ src/monitor/monitor.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 6 ++++ 3 files changed, 84 insertions(+), 0 deletions(-) diff --git a/src/man/sssd-ipa.5.xml b/src/man/sssd-ipa.5.xml index db6aecf..da5a2ff 100644 --- a/src/man/sssd-ipa.5.xml +++ b/src/man/sssd-ipa.5.xml @@ -58,6 +58,12 @@ refer to freeipa.org for more information about HBAC. No configuration of access provider is required on the client side. </para> + <para> + The IPA provider will use the PAC responder if the Kerberos tickets + of users from trusted realms contain a PAC. To make configuration + easier the PAC responder is started automatically if the IPA ID + provider is configured. + </para> </refsect1> <refsect1 id='file-format'> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c index 956efb1..caa1571 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c @@ -804,6 +804,71 @@ static int check_local_domain_unique(struct sss_domain_info *domains) return EOK; } +static errno_t add_implicit_services(struct confdb_ctx *cdb, TALLOC_CTX *mem_ctx, + char ***_services) +{ + int ret; + char **domain_names; + TALLOC_CTX *tmp_ctx; + size_t c; + char *conf_path; + char *id_provider; + bool add_pac = false; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n")); + return ENOMEM; + } + + ret = confdb_get_string_as_list(cdb, tmp_ctx, + CONFDB_MONITOR_CONF_ENTRY, + CONFDB_MONITOR_ACTIVE_DOMAINS, + &domain_names); + if (ret == ENOENT) { + DEBUG(SSSDBG_OP_FAILURE, ("No domains configured!\n")); + goto done; + } + + for (c = 0; domain_names[c] != NULL; c++) { + conf_path = talloc_asprintf(tmp_ctx, CONFDB_DOMAIN_PATH_TMPL, + domain_names[c]); + if (conf_path == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_asprintf failed.\n")); + ret = ENOMEM; + goto done; + } + + ret = confdb_get_string(cdb, tmp_ctx, conf_path, + CONFDB_DOMAIN_ID_PROVIDER, NULL, &id_provider); + if (ret == EOK) { + if (strcasecmp(id_provider, "IPA") == 0) { + add_pac = true; + } + } else { + DEBUG(SSSDBG_OP_FAILURE, ("Failed to get id_provider for " \ + "domain [%s], trying next domain.\n", + domain_names[c])); + } + } + + if (BUILD_WITH_PAC_RESPONDER && add_pac && + !string_in_list("pac", *_services, false)) { + ret = add_string_to_list(mem_ctx, "pac", _services); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, ("add_string_to_list failed.\n")); + goto done; + } + } + + ret = EOK; + +done: + talloc_free(tmp_ctx); + + return ret; +} + static char *check_services(char **services) { const char *known_services[] = { "nss", "pam", "sudo", "autofs", "ssh", @@ -857,6 +922,13 @@ int get_monitor_config(struct mt_ctx *ctx) return EINVAL; } + ret = add_implicit_services(ctx->cdb, ctx->service_ctx, &ctx->services); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, ("Failed to add implicit configured " \ + "services. Some functionality might " \ + "be missing")); + } + badsrv = check_services(ctx->services); if (badsrv != NULL) { DEBUG(0, ("Invalid service %s\n", badsrv)); diff --git a/src/util/util.h b/src/util/util.h index 56e7e0b..2d63e73 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -588,4 +588,10 @@ errno_t sss_br_lock_file(int fd, size_t start, size_t len, #endif /* le32toh */ +#ifdef HAVE_PAC_RESPONDER +#define BUILD_WITH_PAC_RESPONDER true +#else +#define BUILD_WITH_PAC_RESPONDER false +#endif + #endif /* __SSSD_UTIL_H__ */ -- 1.7.7.6
From 1a87975216fec115081e90b13c5c9c13a075a9c9 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Fri, 9 Nov 2012 21:31:23 +0100 Subject: [PATCH 3/3] Run IPA subdomain provider if IPA ID provider is configured To make configuration easier the IPA subdomain provider should be always loaded if the IPA ID provider is configured and the subdomain provider is not explicitly disabled. But to avoid the overhead of regular subdomain requests in setups where no subdomains are used the IPA subdomain provider should behave differently if configured explicit or implicit. If the IPA subdomain provider is configured explicitly, i.e. 'subdomains_provider = ipa' can be found in the domain section of sssd.conf subdomain request are always send to the server if needed. If it is configured implicitly and a request to the server fails with an indication that the server currently does not support subdomains at all, e.g. is not configured to handle trust relationships, a new request will be only send to the server after a long timeout or after a going-online event. To be able to make this distinction this patch save the configuration status to the subdomain context. Fixes https://fedorahosted.org/sssd/ticket/1613 --- src/man/sssd-ipa.5.xml | 23 ++++++++++++ src/man/sssd.conf.5.xml | 14 +++++--- src/providers/data_provider_be.c | 3 +- src/providers/ipa/ipa_subdomains.c | 68 +++++++++++++++++++++++++++++++++++- 4 files changed, 101 insertions(+), 7 deletions(-) diff --git a/src/man/sssd-ipa.5.xml b/src/man/sssd-ipa.5.xml index da5a2ff..56220c3 100644 --- a/src/man/sssd-ipa.5.xml +++ b/src/man/sssd-ipa.5.xml @@ -595,6 +595,29 @@ </para> </refsect1> + <refsect1 id='subdomains_provider'> + <title>SUBDOMAINS PROVIDER</title> + <para> + The IPA subdomains provider behaves slightly differently + if it is configured explicitly or implicitly. + </para> + <para> + If the option 'subdomains_provider = ipa' is found in the + domain section of sssd.conf, the IPA subdomains provider is + configured explicitly, and all subdomain requests are sent to the + IPA server if necessary. + </para> + <para> + If the option 'subdomains_provider' is not set in the domain + section of sssd.conf but there is the option 'id_provider = ipa', + the IPA subdomains provider is configured implictly. In this case, + if a subdomain request fails and indicates that the server does not + support subdomains, i.e. is not configured for trusts, the IPA + subdomains provider is disabled. After an hour or after the IPA + provider goes online, the subdomains provider is enabled again. + </para> + </refsect1> + <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/failover.xml" /> <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/service_discovery.xml" /> diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 33d99c7..9f487fa 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -1411,8 +1411,9 @@ override_homedir = /home/%u <term>subdomains_provider (string)</term> <listitem> <para> - The provider which should handle fetching of subdomains. - This value should be always the same as id_provider. + The provider which should handle fetching of + subdomains. This value should be always the same as + id_provider. Supported subdomain providers are: </para> <para> @@ -1421,13 +1422,16 @@ override_homedir = /home/%u <citerefentry> <refentrytitle>sssd-ipa</refentrytitle> <manvolnum>5</manvolnum> - </citerefentry> for more information on configuring IPA. + </citerefentry> for more information on configuring + IPA. </para> <para> - <quote>none</quote> disallows fetching subdomains explicitly. + <quote>none</quote> disallows fetching subdomains + explicitly. </para> <para> - Default: none + Default: The value of <quote>id_provider</quote> is + used if it is set. </para> </listitem> </varlistentry> diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index 253dc46..685c666 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -2317,7 +2317,8 @@ int be_process_init(TALLOC_CTX *mem_ctx, } ret = load_backend_module(ctx, BET_SUBDOMAINS, - &ctx->bet_info[BET_SUBDOMAINS], NULL); + &ctx->bet_info[BET_SUBDOMAINS], + ctx->bet_info[BET_ID].mod_name); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, ("Subdomains are not supported for [%s] !!\n", be_domain)); } else { diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 36ffafd..0bf2e5d 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -47,6 +47,7 @@ /* refresh automatically every 4 hours */ #define IPA_SUBDOMAIN_REFRESH_PERIOD (3600 * 4) +#define IPA_SUBDOMAIN_DISABLED_PERIOD 3600 /* the directory domain - realm mappings are written to */ #define IPA_SUBDOMAIN_MAPPING_DIR PUBCONF_PATH"/krb5.include.d" @@ -74,6 +75,8 @@ struct ipa_subdomains_ctx { time_t last_refreshed; struct tevent_timer *timer_event; + bool configured_explicit; + time_t disabled_until; /* subdomain map cache */ int num_subdoms; @@ -899,6 +902,12 @@ static void ipa_subdomains_handler_master_done(struct tevent_req *req) * and we don't have the master domain record */ DEBUG(SSSDBG_CRIT_FAILURE, ("Master domain record not found!\n")); + + if (!ctx->sd_ctx->configured_explicit) { + ctx->sd_ctx->disabled_until = time(NULL) + + IPA_SUBDOMAIN_DISABLED_PERIOD; + } + ret = EIO; goto done; } @@ -932,6 +941,7 @@ static void ipa_subdom_online_cb(void *pvt) return; } + ctx->disabled_until = 0; ipa_subdomains_retrieve(ctx, NULL); tv = tevent_timeval_current_ofs(IPA_SUBDOMAIN_REFRESH_PERIOD, 0); @@ -953,9 +963,48 @@ static void ipa_subdom_offline_cb(void *pvt) } } +static errno_t get_config_status(struct be_ctx *be_ctx, + bool *configured_explicit) +{ + int ret; + TALLOC_CTX *tmp_ctx = NULL; + char *tmp_str; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n")); + return ENOMEM; + } + + ret = confdb_get_string(be_ctx->cdb, tmp_ctx, be_ctx->conf_path, + CONFDB_DOMAIN_SUBDOMAINS_PROVIDER, NULL, + &tmp_str); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, ("confdb_get_string failed.\n")); + goto done; + } + + if (tmp_str == NULL) { + *configured_explicit = false; + } else { + *configured_explicit = true; + } + + DEBUG(SSSDBG_TRACE_ALL, ("IPA subdomain provider is configured %s.\n", + *configured_explicit ? "explicit" : "implicit")); + + ret = EOK; + +done: + talloc_free(tmp_ctx); + + return ret; +} + void ipa_subdomains_handler(struct be_req *be_req) { struct ipa_subdomains_ctx *ctx; + time_t now; ctx = talloc_get_type(be_req->be_ctx->bet_info[BET_SUBDOMAINS].pvt_bet_data, struct ipa_subdomains_ctx); @@ -964,7 +1013,15 @@ void ipa_subdomains_handler(struct be_req *be_req) return; } - if (ctx->last_refreshed > time(NULL) - IPA_SUBDOMAIN_REFRESH_LIMIT) { + now = time(NULL); + + if (ctx->disabled_until > now) { + DEBUG(SSSDBG_TRACE_ALL, ("Subdomain provider disabled.\n")); + ipa_subdomains_reply(be_req, DP_ERR_OK, EOK); + return; + } + + if (ctx->last_refreshed > now - IPA_SUBDOMAIN_REFRESH_LIMIT) { ipa_subdomains_reply(be_req, DP_ERR_OK, EOK); return; } @@ -984,6 +1041,13 @@ int ipa_subdom_init(struct be_ctx *be_ctx, { struct ipa_subdomains_ctx *ctx; int ret; + bool configured_explicit = false; + + ret = get_config_status(be_ctx, &configured_explicit); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, ("get_config_status failed.\n")); + return ret; + } ctx = talloc_zero(id_ctx, struct ipa_subdomains_ctx); if (ctx == NULL) { @@ -996,6 +1060,8 @@ int ipa_subdom_init(struct be_ctx *be_ctx, ctx->search_bases = id_ctx->ipa_options->subdomains_search_bases; ctx->master_search_bases = id_ctx->ipa_options->master_domain_search_bases; ctx->ranges_search_bases = id_ctx->ipa_options->ranges_search_bases; + ctx->configured_explicit = configured_explicit; + ctx->disabled_until = 0; *ops = &ipa_subdomains_ops; *pvt_data = ctx; -- 1.7.7.6
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel