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(a)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(a)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.