On Thu, May 14, 2015 at 06:37:37PM +0200, Pavel Reichl wrote:
See some more comments inline. Hopefully we're almost there :-)
From 2b027b7f702bfdc8453e0a4c7096e6943d7141ca Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 30 Apr 2015 06:43:05 -0400
Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to
Kerberos principals.
Resolves:
https://fedorahosted.org/sssd/ticket/2509
---
src/config/SSSDConfig/__init__.py.in | 1 +
src/config/SSSDConfigTest.py | 9 ++-
src/config/etc/sssd.api.d/sssd-ad.conf | 1 +
src/config/etc/sssd.api.d/sssd-ipa.conf | 1 +
src/config/etc/sssd.api.d/sssd-krb5.conf | 1 +
src/man/sssd-krb5.5.xml | 36 ++++++++++
src/providers/ad/ad_opts.h | 1 +
src/providers/ipa/ipa_opts.h | 1 +
src/providers/krb5/krb5_access.c | 5 +-
src/providers/krb5/krb5_auth.c | 55 +++++++++++++--
src/providers/krb5/krb5_auth.h | 2 +
src/providers/krb5/krb5_common.h | 8 +++
src/providers/krb5/krb5_init_shared.c | 11 +++
src/providers/krb5/krb5_opts.h | 1 +
src/providers/krb5/krb5_utils.c | 110 ++++++++++++++++++++++++++++++
src/providers/krb5/krb5_utils.h | 5 ++
src/tests/krb5_utils-tests.c | 112 +++++++++++++++++++++++++++++++
17 files changed, 349 insertions(+), 11 deletions(-)
[...]
+static errno_t
+get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
+ char *id_prov_name, const char **_krb_primary)
+{
+ errno_t ret;
+ int i = 0;
+
+ while(name_to_primary != NULL &&
+ name_to_primary[i].id_name != NULL &&
+ name_to_primary[i].krb_primary != NULL) {
+ ret = sss_utf8_case_eq((const uint8_t*)name_to_primary[i].id_name,
+ (const uint8_t*)id_prov_name);
This is mostly my fault, I told you to use sss_utf8_case_eq..but this
should only be done for case-insensitive domains :-)
It looks like sss_string_equal() is a nice wrapper we can use.
+ if (ret == EOK) {
+ *_krb_primary = name_to_primary[i].krb_primary;
+ goto done;
+ }
+ i++;
+ }
+
+ /* Handle also the case of name_to_primary being NULL */
+ ret = ENOENT;
+
+done:
+ return ret;
+}
+
errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
struct krb5_ctx *krb5_ctx, struct krb5child_req **krb5_req)
{
struct krb5child_req *kr = NULL;
+ const char *mapped_name;
+ errno_t ret;
kr = talloc_zero(mem_ctx, struct krb5child_req);
if (kr == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
- return ENOMEM;
+ ret = ENOMEM;
+ goto done;
}
kr->is_offline = false;
talloc_set_destructor((TALLOC_CTX *) kr, krb5_cleanup);
@@ -191,9 +221,25 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
kr->pd = pd;
kr->krb5_ctx = krb5_ctx;
+ ret = get_krb_primary(krb5_ctx->name_to_primary,
+ pd->user, &mapped_name);
+ if (ret == EOK) {
+ DEBUG(SSSDBG_TRACE_FUNC, "Setting mapped name to: %s\n",
mapped_name);
+ kr->user = mapped_name;
+ } else if (ret == ENOENT) {
+ DEBUG(SSSDBG_TRACE_ALL, "No mapping for: %s\n", pd->user);
+ kr->user = pd->user;
+ } else {
+ DEBUG(SSSDBG_CRIT_FAILURE, "get_krb_primary failed - %s:[%d]\n",
+ sss_strerror(ret), ret);
+ goto done;
+ }
+
*krb5_req = kr;
+ ret = EOK;
- return EOK;
+done:
Since we added the done handler, can we also free kr on error to avoid
polluting mem_ctx in that case?
+ return ret;
}
[...]
--- a/src/providers/krb5/krb5_utils.c
+++ b/src/providers/krb5/krb5_utils.c
@@ -465,3 +465,113 @@ errno_t get_domain_or_subdomain(struct be_ctx *be_ctx,
return EOK;
}
+
+static errno_t split_tuple(TALLOC_CTX *mem_ctx, const char *tuple,
+ const char **_first, const char **_second)
+{
+ errno_t ret;
+ char **list;
+ int n;
+
+ ret = split_on_separator(mem_ctx, tuple, ':', true, true, &list,
&n);
+
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "split_on_separator failed - %s:[%d]\n",
+ sss_strerror(ret), ret);
+ goto done;
+ } else if (n != 2) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "split_on_separator failed.\n");
Please use a better error message that will make it clear the tuple has
unexpected number of components.
+ ret = EINVAL;
+ goto done;
+ }
+
+ *_first = list[0];
+ *_second = list[1];
+
+done:
+ return ret;
+}
+
+static errno_t
+fill_name_to_primary_map(TALLOC_CTX *mem_ctx, char **map,
+ struct map_id_name_to_krb_primary *name_to_primary,
+ size_t size)
+{
+ int i;
+ errno_t ret;
+
+ for (i = 0; i < size; i++) {
+ ret = split_tuple(mem_ctx, map[i],
+ &name_to_primary[i].id_name,
+ &name_to_primary[i].krb_primary);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "split_tuple failed - %s:[%d]\n", sss_strerror(ret), ret);
+ goto done;
+ }
+ }
+
+ ret = EOK;
+
+done:
+ return ret;
+}
+
+errno_t
+parse_krb5_map_user(TALLOC_CTX *mem_ctx, const char *krb5_map_user,
+ struct map_id_name_to_krb_primary **_name_to_primary)
+{
+ int size;
+ char **map;
+ errno_t ret;
+ TALLOC_CTX *tmp_ctx;
+ struct map_id_name_to_krb_primary *name_to_primary;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ if (krb5_map_user == NULL || strlen(krb5_map_user) == 0) {
+ DEBUG(SSSDBG_FUNC_DATA, "Warning: krb5_map_user is empty!\n");
+ size = 0;
+ } else {
+ ret = split_on_separator(tmp_ctx, krb5_map_user, ',', true, true,
+ &map, &size);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to parse krb5_map_user!\n");
+ goto done;
+ }
+ }
+
+ name_to_primary = talloc_zero_array(tmp_ctx,
+ struct map_id_name_to_krb_primary,
+ size + 1);
+ if (name_to_primary == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ /* sentinel */
+ name_to_primary[size].id_name = NULL;
+ name_to_primary[size].krb_primary = NULL;
+
+ ret = fill_name_to_primary_map(name_to_primary, map, name_to_primary,
+ size);
Coverity complains here:
sssd-1.12.90/src/providers/krb5/krb5_utils.c:526: var_decl: Declaring variable
"map" without initializer.
sssd-1.12.90/src/providers/krb5/krb5_utils.c:560: uninit_use_in_call: Using uninitialized
value "map" when calling "fill_name_to_primary_map".
sssd-1.12.90/src/providers/krb5/krb5_utils.c:505:9: read_parm: Reading a parameter value.
# 558| name_to_primary[size].krb_primary = NULL;
# 559|
# 560|-> ret = fill_name_to_primary_map(name_to_primary, map, name_to_primary,
# 561| size);
# 562| if (ret != EOK) {
I'll test the patch now..