[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