[SSSD] [PATCH] AD/IPA: add krb5_confd_path configuration option

Jakub Hrozek jhrozek at redhat.com
Tue Nov 25 12:40:15 UTC 2014


On Tue, Nov 25, 2014 at 01:09:51PM +0100, Jakub Hrozek wrote:
> On Tue, Nov 25, 2014 at 11:43:44AM +0100, Sumit Bose wrote:
> > On Mon, Nov 24, 2014 at 10:24:23PM +0100, Jakub Hrozek wrote:
> > > 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?
> > 
> > I have opened https://fedorahosted.org/sssd/ticket/2505 to check this
> > (and yes, I think there are still cases which needs fixing, but luckily
> > redhat-rpm-config hides them for us :-)
> > 
> > > 
> > > > 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?
> > 
> > Typically some specific data is needed for the plugins and since the KRB5
> > provider can not retrieve data from a server I think chances are low
> > that the KRB5 provider has to write config snippets.
> > 
> > > 
> > > >  
> > > >  [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)
> > 
> > I think a single option is not worth it.
> > 
> > > 
> > > >              </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 ?
> > 
> > done
> > 
> > > 
> > > >      /* 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?
> > 
> > done
> > 
> > > 
> > > > +}
> > > > +
> > > >  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) ?
> > 
> > done
> > 
> > > 
> > > 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 ?
> > 
> > d'oh, fixed
> > 
> > > 
> > > > +    file_name = talloc_asprintf(tmp_ctx, "%s/localauth_plugin",
> > > > +                                   path);
> > > 
> > > strange indent
> > 
> > fixed
> > 
> > > 
> > > > +    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.
> > 
> > Thank you for the review, new version attached.
> > 
> > bye,
> > Sumit
> 
> These patches work for me, so ACK.

* master:
    4fa184e2c60b377fd71e0115a618bd68dc73627d
    eaaeaa7e00c3d4bfa792cc4d3c6770dc1e28ef0c

> 
> There is an issue which I think is unrelated -- when I put the patches
> through Coverity, I saw new warnings:
> Error: CHECKED_RETURN (CWE-252):
> sssd-1.12.3/src/monitor/monitor.c:1688: check_return: Calling "unlink" without checking return value (as is done elsewhere 16 out of 20 times).
> sssd-1.12.3/src/monitor/monitor.c:1450: example_assign: Example 1: Assigning: "ret" = return value from "unlink("/var/run/sssd.pid")".
> sssd-1.12.3/src/monitor/monitor.c:1451: example_checked: Example 1 (cont.): "ret" has its value checked in "ret == -1".
> sssd-1.12.3/src/providers/krb5/krb5_common.c:839: example_assign: Example 2: Assigning: "ret" = return value from "unlink(file)".
> sssd-1.12.3/src/providers/krb5/krb5_common.c:840: example_checked: Example 2 (cont.): "ret" has its value checked in "ret == -1".
> sssd-1.12.3/src/responder/common/responder_common.c:617: example_assign: Example 3: Assigning: "ret" = return value from "unlink(sock_name)".
> sssd-1.12.3/src/responder/common/responder_common.c:618: example_checked: Example 3 (cont.): "ret" has its value checked in "ret != 0".
> sssd-1.12.3/src/responder/nss/nsssrv.c:486: example_assign: Example 4: Assigning: "ret" = return value from "unlink("/var/lib/sss/mc/clear_mc_flag")".
> sssd-1.12.3/src/responder/nss/nsssrv.c:487: example_checked: Example 4 (cont.): "ret" has its value checked in "ret != 0".
> sssd-1.12.3/src/responder/nss/nsssrv_mmap_cache.c:1064: example_assign: Example 5: Assigning: "uret" = return value from "unlink(mc_ctx->file)".
> sssd-1.12.3/src/responder/nss/nsssrv_mmap_cache.c:1065: example_checked: Example 5 (cont.): "uret" has its value checked in "uret == -1".
> 
> Error: CHECKED_RETURN (CWE-252):
> sssd-1.12.3/src/responder/ssh/sshsrv_cmd.c:657: check_return: Calling "unlink" without checking return value (as is done elsewhere 16 out of 20 times).
> sssd-1.12.3/src/monitor/monitor.c:1450: example_assign: Example 1: Assigning: "ret" = return value from "unlink("/var/run/sssd.pid")".
> sssd-1.12.3/src/monitor/monitor.c:1451: example_checked: Example 1 (cont.): "ret" has its value checked in "ret == -1".
> sssd-1.12.3/src/providers/krb5/krb5_common.c:839: example_assign: Example 2: Assigning: "ret" = return value from "unlink(file)".
> sssd-1.12.3/src/providers/krb5/krb5_common.c:840: example_checked: Example 2 (cont.): "ret" has its value checked in "ret == -1".
> sssd-1.12.3/src/responder/common/responder_common.c:617: example_assign: Example 3: Assigning: "ret" = return value from "unlink(sock_name)".
> sssd-1.12.3/src/responder/common/responder_common.c:618: example_checked: Example 3 (cont.): "ret" has its value checked in "ret != 0".
> sssd-1.12.3/src/responder/nss/nsssrv.c:486: example_assign: Example 4: Assigning: "ret" = return value from "unlink("/var/lib/sss/mc/clear_mc_flag")".
> sssd-1.12.3/src/responder/nss/nsssrv.c:487: example_checked: Example 4 (cont.): "ret" has its value checked in "ret != 0".
> sssd-1.12.3/src/responder/nss/nsssrv_mmap_cache.c:1064: example_assign: Example 5: Assigning: "uret" = return value from "unlink(mc_ctx->file)".
> sssd-1.12.3/src/responder/nss/nsssrv_mmap_cache.c:1065: example_checked: Example 5 (cont.): "uret" has its value checked in "uret == -1".
> 
> (and some more)
> 
> But what I think has happened is that your patch added a call to unlink
> that we check, but that caused Coverity to reach some threshold of
> checked/unchecked calls.
> 
> I will file a ticket upstream to add checks, even if it was just to
> report failure and silence Coverity.

https://fedorahosted.org/sssd/ticket/2506



More information about the sssd-devel mailing list