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?
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?
[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.