[SSSD] [PATCH] AD/IPA: add krb5_confd_path configuration option
Jakub Hrozek
jhrozek at redhat.com
Mon Nov 24 21:24:23 UTC 2014
On Mon, Nov 24, 2014 at 11:11:05AM +0100, Sumit Bose wrote:
> Hi,
>
> this patch provides a fix for https://fedorahosted.org/sssd/ticket/2473.
>
> I tired to not restrict it to the localauth plugin but make it possible
> to easily add other snippets in future.
>
> Currently I don't do any specific checks on the given path because if it
> is really changed to some other path by the admin we cannot expected and
> permissions or ownerships. If you think that some checks are need I'd be
> happy to add them.
>
> The first patch fixes an issues in the autoconf/automake machinery which
> I came across while testing the patch.
>
> bye,
> Sumit
> From 88c54586c141c38a21dcb70083707d2a2d3dcae9 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 21 Nov 2014 15:22:24 +0100
> Subject: [PATCH 1/2] Fix KRB5_CONF_PATH
ACK
I see some other uses of ${sysconfdir} in conf_macros.m4, do you think
it makes sense to fix those, too?
> From 67b370cb38e1d2396340f09c0c1f893c0744f1f6 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 21 Nov 2014 18:07:10 +0100
> Subject: [PATCH 2/2] AD/IPA: add krb5_confd_path configuration option
>
> With this new parameter the directory where Kerberos configuration
> snippets are created can be specified.
Functionally works well, there are even no AVC denials. See inline for
some comments and questions.
>
> Fixes https://fedorahosted.org/sssd/ticket/2473
> ---
> src/config/SSSDConfig/__init__.py.in | 1 +
> src/config/etc/sssd.api.d/sssd-ad.conf | 1 +
> src/config/etc/sssd.api.d/sssd-ipa.conf | 1 +
> src/man/sssd-ad.5.xml | 18 +++++
> src/man/sssd-ipa.5.xml | 18 +++++
> src/providers/ad/ad_common.c | 11 +++
> src/providers/ad/ad_common.h | 1 +
> src/providers/ad/ad_opts.h | 1 +
> src/providers/ad/ad_subdomains.c | 8 ++
> src/providers/ipa/ipa_common.c | 11 +++
> src/providers/ipa/ipa_common.h | 1 +
> src/providers/ipa/ipa_opts.h | 1 +
> src/providers/ipa/ipa_subdomains.c | 8 ++
> src/tests/cmocka/test_utils.c | 45 +++++++++++
> src/util/domain_info_utils.c | 129 +++++++++++++++++++++++++++++++-
> src/util/util.h | 6 ++
> 16 files changed, 258 insertions(+), 3 deletions(-)
>
> diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
> index 283ed2d..500bd71 100644
> --- a/src/config/SSSDConfig/__init__.py.in
> +++ b/src/config/SSSDConfig/__init__.py.in
> @@ -195,6 +195,7 @@ option_strings = {
> 'krb5_realm' : _('Kerberos realm'),
> 'krb5_auth_timeout' : _('Authentication timeout'),
> 'krb5_use_kdcinfo' : _('Whether to create kdcinfo files'),
> + 'krb5_confd_path' : _('Where to drop krb5 config snippets'),
>
> # [provider/krb5/auth]
> 'krb5_ccachedir' : _('Directory to store credential caches'),
> diff --git a/src/config/etc/sssd.api.d/sssd-ad.conf b/src/config/etc/sssd.api.d/sssd-ad.conf
> index 3daa256..3496fb4 100644
> --- a/src/config/etc/sssd.api.d/sssd-ad.conf
> +++ b/src/config/etc/sssd.api.d/sssd-ad.conf
> @@ -54,6 +54,7 @@ ldap_page_size = int, None, false
> ldap_deref_threshold = int, None, false
> ldap_connection_expire_timeout = int, None, false
> ldap_disable_paging = bool, None, false
> +krb5_confd_path = str, None, false
Why are these options IPA and AD specific? Do you think we will never
use this option in the generic KRB5 provider?
>
> [provider/ad/id]
> ldap_search_timeout = int, None, false
> diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf
> index 5df5258..2a3b7ef 100644
> --- a/src/config/etc/sssd.api.d/sssd-ipa.conf
> +++ b/src/config/etc/sssd.api.d/sssd-ipa.conf
> @@ -51,6 +51,7 @@ ldap_page_size = int, None, false
> ldap_deref_threshold = int, None, false
> ldap_connection_expire_timeout = int, None, false
> ldap_disable_paging = bool, None, false
> +krb5_confd_path = str, None, false
>
> [provider/ipa/id]
> ldap_search_timeout = int, None, false
> diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
> index f63a496..4e29d4f 100644
> --- a/src/man/sssd-ad.5.xml
> +++ b/src/man/sssd-ad.5.xml
> @@ -778,6 +778,24 @@ FOREST:EXAMPLE.COM:(memberOf=cn=admins,ou=groups,dc=example,dc=com)
> </para>
> </listitem>
> </varlistentry>
> +
> + <varlistentry>
> + <term>krb5_confd_path (string)</term>
> + <listitem>
> + <para>
> + Absolute path of a directory where SSSD should place
> + Kerberos configuration snippets.
> + </para>
> + <para>
> + To disable the creation of the configuration
> + snippets set the parameter to 'none'.
> + </para>
> + <para>
> + Default: not set (krb5.include.d subdirectory of
> + SSSD's pubconf directory)
> + </para>
> + </listitem>
> + </varlistentry>
Would it make sense to move the duplicated description into the include/
directory? (If yes, then we need to add the included file into the po4a
config)
> </variablelist>
> </para>
> </refsect1>
> diff --git a/src/man/sssd-ipa.5.xml b/src/man/sssd-ipa.5.xml
> index e8a716c..2d8654a 100644
> --- a/src/man/sssd-ipa.5.xml
> +++ b/src/man/sssd-ipa.5.xml
> @@ -447,6 +447,24 @@
> </varlistentry>
>
> <varlistentry>
> + <term>krb5_confd_path (string)</term>
> + <listitem>
> + <para>
> + Absolute path of a directory where SSSD should place
> + Kerberos configuration snippets.
> + </para>
> + <para>
> + To disable the creation of the configuration
> + snippets set the parameter to 'none'.
> + </para>
> + <para>
> + Default: not set (krb5.include.d subdirectory of
> + SSSD's pubconf directory)
> + </para>
> + </listitem>
> + </varlistentry>
> +
> + <varlistentry>
> <term>ipa_hbac_refresh (integer)</term>
> <listitem>
> <para>
> diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
> index 7b08c2b..10c80a0 100644
> --- a/src/providers/ad/ad_common.c
> +++ b/src/providers/ad/ad_common.c
> @@ -264,6 +264,7 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
> char *ad_hostname;
> char hostname[HOST_NAME_MAX + 1];
> char *case_sensitive_opt;
> + char *krb5_confd_path;
>
> opts = talloc_zero(mem_ctx, struct ad_options);
> if (!opts) return ENOMEM;
> @@ -333,6 +334,16 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
> goto done;
> }
>
> + krb5_confd_path = dp_opt_get_string(opts->basic, AD_KRB5_CONFD_PATH);
> + if (krb5_confd_path == NULL) {
> + ret = dp_opt_set_string(opts->basic, AD_KRB5_CONFD_PATH,
> + KRB5_MAPPING_DIR);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_OP_FAILURE, "dp_opt_set_string failed.\n");
> + goto done;
> + }
> + }
> +
How about adding KRB5_MAPPING_DIR as the default value in ad_opts.h ?
> /* Active Directory is always case-insensitive */
> ret = confdb_get_string(cdb, mem_ctx, conf_path,
> CONFDB_DOMAIN_CASE_SENSITIVE, "false",
> diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
> index df8dcff..b39ade4 100644
> --- a/src/providers/ad/ad_common.h
> +++ b/src/providers/ad/ad_common.h
> @@ -60,6 +60,7 @@ enum ad_basic_opt {
> AD_GPO_MAP_PERMIT,
> AD_GPO_MAP_DENY,
> AD_GPO_DEFAULT_RIGHT,
> + AD_KRB5_CONFD_PATH,
>
> AD_OPTS_BASIC /* opts counter */
> };
[...]
> diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c
> index d978137..9a30c1f 100644
> --- a/src/tests/cmocka/test_utils.c
> +++ b/src/tests/cmocka/test_utils.c
Tests, great :-)
> @@ -20,6 +20,8 @@
> along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#define _GNU_SOURCE
> +#include <stdio.h>
> #include <popt.h>
>
> #include "tests/cmocka/common_mock.h"
> @@ -983,6 +985,48 @@ void test_add_strings_lists(void **state)
> talloc_free(res);
> }
>
> +void test_sss_write_krb5_conf_snippet(void **state)
> +{
> + int ret;
> + char buf[PATH_MAX];
> + char *cwd;
> + char *path;
> + char *file;
> +
> + ret = sss_write_krb5_conf_snippet(NULL);
> + assert_int_equal(ret, EINVAL);
> +
> + ret = sss_write_krb5_conf_snippet("abc");
> + assert_int_equal(ret, EINVAL);
> +
> + ret = sss_write_krb5_conf_snippet("");
> + assert_int_equal(ret, EOK);
> +
> + ret = sss_write_krb5_conf_snippet("none");
> + assert_int_equal(ret, EOK);
> +
> + cwd = getcwd(buf, PATH_MAX);
> + assert_non_null(cwd);
> +
> + ret = asprintf(&path, "%s/%s", cwd, TESTS_PATH);
> + assert_true(ret > 0);
> +
> + ret = asprintf(&file, "%s/%s/localauth_plugin", cwd, TESTS_PATH);
> + assert_true(ret > 0);
> +
> + ret = sss_write_krb5_conf_snippet(path);
> + assert_int_equal(ret, EOK);
> +
> + /* Check if writing a second time will work as well */
> + ret = sss_write_krb5_conf_snippet(path);
> + assert_int_equal(ret, EOK);
> +
> +#ifdef BUILD_LIBWBCLIENT
> + ret = unlink(file);
> + assert_int_equal(ret, EOK);
> +#endif
Can you free(path) so that CI doesn't complain?
> +}
> +
> int main(int argc, const char *argv[])
> {
> poptContext pc;
> @@ -1030,6 +1074,7 @@ int main(int argc, const char *argv[])
> unit_test_setup_teardown(test_add_strings_lists,
> setup_add_strings_lists,
> teardown_add_strings_lists),
> + unit_test(test_sss_write_krb5_conf_snippet),
> };
>
> /* Set debug level to invalid value so we can deside if -d 0 was used. */
> diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
> index 4e2c14c..d24e64c 100644
> --- a/src/util/domain_info_utils.c
> +++ b/src/util/domain_info_utils.c
> @@ -24,9 +24,6 @@
> #include "db/sysdb.h"
> #include "util/util.h"
>
> -/* the directory domain - realm mappings are written to */
> -#define KRB5_MAPPING_DIR PUBCONF_PATH"/krb5.include.d"
> -
> struct sss_domain_info *get_domains_head(struct sss_domain_info *domain)
> {
> struct sss_domain_info *dom = NULL;
> @@ -637,3 +634,129 @@ done:
> talloc_free(tmp_ctx);
> return ret;
> }
> +
> +#define LOCALAUTH_PLUGIN_CONFIG \
> +"[plugins]\n" \
> +" localauth = {\n" \
> +" module = sssd:"APP_MODULES_PATH"/sssd_krb5_localauth_plugin.so\n" \
> +" enable_only = sssd\n" \
> +" }"
> +
> +errno_t sss_write_krb5_conf_snippet(const char *path)
> +{
> + errno_t ret;
> + errno_t err;
> + TALLOC_CTX *tmp_ctx;
I wonder if the variables only used inside the #ifdef should be moved
there (or the ifdef split into a separate function) ?
I built one test build on a machine that didn't have BUILD_LIBWBCLIENT
defined and got warnings about unused variables.
> + const char *file_name;
> + char *tmp_file = NULL;
> + int fd = -1;
> + mode_t old_mode;
> + ssize_t written;
> + size_t size;
> +
> + if (path != NULL && (*path == '\0' || strcasecmp(path, "none") == 0)) {
> + DEBUG(SSSDBG_TRACE_FUNC, "Empty path, nothing to do.\n");
> + return EOK;
> + }
> +
> + if (path == NULL || *path != '/') {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Invalid or missing path [%s]-\n",
> + path == NULL ? "missing" : path);
> + return EINVAL;
> + }
> +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
> + return ENOMEM;
> + }
> +
> +#ifdef BUILD_LIBWBCLIENT
Did you mean HAVE_KRB5_LOCALAUTH_PLUGIN ?
> + file_name = talloc_asprintf(tmp_ctx, "%s/localauth_plugin",
> + path);
strange indent
> + if (file_name == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + DEBUG(SSSDBG_FUNC_DATA, "File for localauth plugin configuration is [%s]\n",
> + file_name);
The rest looks good.
More information about the sssd-devel
mailing list