On Thu, Apr 30, 2015 at 01:45:01PM +0200, Pavel Reichl wrote:
Hi,
it seems this patch review stalled. I'll try to restart it..
From 2c7239f5466acb4a0989c4843b0b13e85f1d40b3 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 30 Apr 2015 06:40:43 -0400
Subject: [PATCH 1/3] utils: new function split_str
I'm with Lukas here. It seems to me that the split function could be
defined in the Kerberos tree and could use split_on_separator() as the
underlying implementation. I don't see that this change requires a
separate util function.
Feel free to add a function for krb5_utils instead that calls into
split_on_separator()..
Or were you concerned about the delimeter being present in the Kerberos
principal?
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 30 Apr 2015 06:43:05 -0400
Subject: [PATCH 2/3] 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
---
[...]
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>krb5_map_user (string)</term>
+ <listitem>
+ <para>
+ The list of mappings is given as a comma-separated
+ list of pairs `username:principal` where `username`
It's not username:principal, but username:primary. I agree though that
the primary term is not well known, so maybe could be say something like
"username:primary, where primary is typically the user part of the
principal" ?
+ is a UNIX user name and `principal` is
a kerberos
+ principal name. This mapping is used when user is
+ authenticating using `auth_provider = krb5`.
+ </para>
+
+ <para>
+ example:
+ krb5_map_user = joe:juser,vince:rraines
+ </para>
+ <para>
+ `joe` and `vince` are UNIX user names and `juser`
+ and `rraines` are kerberos principal names.
Please add krb5_realm to the example and illustrate the full principals
here.
+ </para>
+
+ <para>
+ Default: not set
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
</refsect1>
diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
index 0b7255a828e95785d31437968a37bc20fbf62aef..b03c74612d3141170dac84ab805529184fec5a49
100644
--- a/src/providers/ad/ad_opts.h
+++ b/src/providers/ad/ad_opts.h
@@ -168,6 +168,7 @@ struct dp_option ad_def_krb5_opts[] = {
{ "krb5_canonicalize", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
{ "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
+ { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING },
DP_OPTION_TERMINATOR
};
diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h
index f2f164bc3cc6b6e13c30dbc6f5b37a03b4c5e289..5f215d7fbe2926eb020cbe58226727dacf0f1ba2
100644
--- a/src/providers/ipa/ipa_opts.h
+++ b/src/providers/ipa/ipa_opts.h
@@ -310,6 +310,7 @@ struct dp_option ipa_def_krb5_opts[] = {
{ "krb5_canonicalize", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
{ "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
+ { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING },
DP_OPTION_TERMINATOR
};
diff --git a/src/providers/krb5/krb5_access.c b/src/providers/krb5/krb5_access.c
index 7fda2a37922a537f7fe53d629c4e0cb4df1bd4da..72b93180f9cdeb53ad78c821ed73281d38cda920
100644
--- a/src/providers/krb5/krb5_access.c
+++ b/src/providers/krb5/krb5_access.c
@@ -106,8 +106,8 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx,
break;
case 1:
ret = find_or_guess_upn(state, res->msgs[0], krb5_ctx,
- be_ctx->domain, pd->user, pd->domain,
- &state->kr->upn);
+ be_ctx->domain, state->kr->mapped_name,
+ pd->domain, &state->kr->upn);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "find_or_guess_upn failed.\n");
goto done;
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index 25caf7b788a3f373f47e9d8aad38a2ea6fc12621..f665fab786b27bf1e85d238cdb2567915d0ae0b8
100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -175,10 +175,33 @@ static int krb5_cleanup(void *ptr)
return EOK;
}
+errno_t get_krb_princ(struct map_id_name_to_krb_princ *name_to_princ, int size,
+ char *id_prov_name, const char **_krb_princ)
Please make the function static and use size_t for the size. Or even
cleaner, allocate one extra map_id_name_to_krb5_princ and set it to
NULL:NULL as a sentinel, then we don't need size at all.
+{
+ errno_t ret;
+ int i;
+
+ for (i=0; i < size; i++) {
+ if (strcmp(name_to_princ[i].id_name, id_prov_name) == 0) {
What about case-sensitiveness? What about using sss_utf8_case_eq() here
instead?
+ *_krb_princ = name_to_princ[i].krb_princ;
+ ret = EOK;
+ goto done;
+ }
+ }
+
+ /* Handle also the case of size being zero */
+ 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;
This variable name is fine..but the variable in "kr" should be named
differently. I think using just 'user' would be fine, same as what we
use in "pd".
+ errno_t ret;
kr = talloc_zero(mem_ctx, struct krb5child_req);
if (kr == NULL) {
@@ -192,6 +215,20 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
kr->pd = pd;
kr->krb5_ctx = krb5_ctx;
+ ret = get_krb_princ(krb5_ctx->name_to_princ, krb5_ctx->name_to_princ_size,
+ pd->user, &mapped_name);
+ if (ret == EOK) {
+ DEBUG(SSSDBG_TRACE_FUNC, "Setting mapped name to: %s\n",
mapped_name);
+ kr->mapped_name = mapped_name;
+ } else if (ret == ENOENT) {
+ DEBUG(SSSDBG_TRACE_FUNC, "No mapping for: %s\n", pd->user);
I would decrease the DEBUG_LEVEL even lower, it would be quite a corner
case to actually have the username mapped.
+ kr->mapped_name = pd->user;
+ } else {
+ DEBUG(SSSDBG_MINOR_FAILURE, "get_krb_princ failed - %s:[%d]\n",
+ sss_strerror(ret), ret);
+ kr->mapped_name = pd->user;
I think it's better to just fail here instead of allowing to run with a
wrong principal name.
+ }
+
*krb5_req = kr;
return EOK;
@@ -350,6 +387,7 @@ struct krb5_auth_state {
static void krb5_auth_resolve_done(struct tevent_req *subreq);
static void krb5_auth_done(struct tevent_req *subreq);
+
Extra blank line.
struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
struct be_ctx *be_ctx,
@@ -510,7 +548,7 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
case 1:
ret = find_or_guess_upn(state, res->msgs[0], krb5_ctx,
- be_ctx->domain, pd->user, pd->domain,
+ be_ctx->domain, kr->mapped_name, pd->domain,
&kr->upn);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "find_or_guess_upn failed.\n");
diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h
index 00cb658c418ade2e6a1d9b5362a40f97e0dc6c6b..354c74f5d806c9bd9355a9abdc2976f1fcf8b1aa
100644
--- a/src/providers/krb5/krb5_auth.h
+++ b/src/providers/krb5/krb5_auth.h
@@ -56,6 +56,8 @@ struct krb5child_req {
bool run_as_user;
bool upn_from_different_realm;
bool send_pac;
+
+ const char *mapped_name;
As said elsewhere, the name is not really mapped in most cases.
};
errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
diff --git a/src/providers/krb5/krb5_common.h b/src/providers/krb5/krb5_common.h
index 81e64688a6d93a4b3ecf41d75d1bf6166b4619ce..99ee3b4930fb275efebc09e510cd0459d11bf19e
100644
--- a/src/providers/krb5/krb5_common.h
+++ b/src/providers/krb5/krb5_common.h
@@ -67,6 +67,7 @@ enum krb5_opts {
KRB5_CANONICALIZE,
KRB5_USE_ENTERPRISE_PRINCIPAL,
KRB5_USE_KDCINFO,
+ KRB5_MAP_USER,
KRB5_OPTS
};
@@ -89,6 +90,11 @@ enum krb5_config_type {
K5C_IPA_SERVER
};
+struct map_id_name_to_krb_princ {
+ char *id_name;
+ char* krb_princ;
Could we make these const?
+};
+
struct krb5_ctx {
/* opts taken from kinit */
/* in seconds */
@@ -128,6 +134,9 @@ struct krb5_ctx {
hash_table_t *wait_queue_hash;
enum krb5_config_type config_type;
+
+ struct map_id_name_to_krb_princ *name_to_princ;
+ int name_to_princ_size;
};
struct remove_info_files_ctx {
diff --git a/src/providers/krb5/krb5_init_shared.c
b/src/providers/krb5/krb5_init_shared.c
index 3b4bf096ef49d0f2d369fda20462b7fd56d61127..e0c010fefe79ffa594a63a88c43279aecd4559c2
100644
--- a/src/providers/krb5/krb5_init_shared.c
+++ b/src/providers/krb5/krb5_init_shared.c
@@ -26,6 +26,98 @@
#include "providers/krb5/krb5_auth.h"
#include "providers/krb5/krb5_init_shared.h"
+static errno_t
+fill_name_to_princ_map(TALLOC_CTX *mem_ctx, char **map,
+ struct map_id_name_to_krb_princ *name_to_princ,
+ int size)
+{
+ int i;
+ errno_t ret;
+
+ for (i=0; i < size; i++) {
+ ret = split_str(mem_ctx, ':', map[i],
+ &name_to_princ[i].id_name,
+ &name_to_princ[i].krb_princ);
As said earlier, I'm not sure if split_str needs to be in the generic
util code.
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "split_str failed - %s:[%d]\n", sss_strerror(ret), ret);
+ goto done;
+ } else if (name_to_princ[i].id_name == NULL ||
+ name_to_princ[i].krb_princ == NULL) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "split_str failed - %s:%s\n",
+ name_to_princ[i].id_name, name_to_princ[i].krb_princ );
+ ret = EINVAL;
+ goto done;
+ }
+ }
+
+ ret = EOK;
+
+done:
+ return ret;
+}
+
+errno_t parse_krb5_map_user(TALLOC_CTX *mem_ctx, struct dp_option *opts,
+ struct map_id_name_to_krb_princ **_name_to_princ,
+ int *_size)
+{
+ int size;
+ char **map;
+ errno_t ret;
+ TALLOC_CTX *tmp_ctx;
+ const char *krb5_map_user;
+ struct map_id_name_to_krb_princ *name_to_princ;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ krb5_map_user = dp_opt_get_cstring(opts, KRB5_MAP_USER);
+ if (krb5_map_user == NULL) {
+ DEBUG(SSSDBG_FUNC_DATA, "Option krb5_map_user is empty!\n");
+ size = 0;
Here we could set the map to NULL and then shortcut in get_krb_princ()
on receiving NULL.
+ ret = EOK;
+ goto done;
+ }
+
+ 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_princ = talloc_array(tmp_ctx,
+ struct map_id_name_to_krb_princ, size);
+ if (name_to_princ == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ ret = fill_name_to_princ_map(name_to_princ, map, name_to_princ, size);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "fill_name_to_princ_map failed:
%s:[%d]\n",
+ sss_strerror(ret), ret);
+ goto done;
+ }
+
+ ret = EOK;
+
+done:
+ if (ret == EOK) {
+ if (size == 0) {
+ *_name_to_princ = NULL;
+ } else {
+ *_name_to_princ = talloc_steal(mem_ctx, name_to_princ);
+ }
+ *_size = size;
+ }
+ talloc_free(tmp_ctx);
+ return ret;
+}
+
errno_t krb5_child_init(struct krb5_ctx *krb5_auth_ctx,
struct be_ctx *bectx)
{
@@ -90,6 +182,15 @@ errno_t krb5_child_init(struct krb5_ctx *krb5_auth_ctx,
goto done;
}
+ ret = parse_krb5_map_user(krb5_auth_ctx, krb5_auth_ctx->opts,
+ &krb5_auth_ctx->name_to_princ,
+ &krb5_auth_ctx->name_to_princ_size);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "parse_krb5_map_user failed: %s:[%d]\n",
+ sss_strerror(ret), ret);
+ goto done;
+ }
+
ret = EOK;
done:
diff --git a/src/providers/krb5/krb5_opts.h b/src/providers/krb5/krb5_opts.h
index db62cc3b23d8a5eadd3df97a102fb84128b740da..50d701b8ba431719dbf67799230ec345cdee30a9
100644
--- a/src/providers/krb5/krb5_opts.h
+++ b/src/providers/krb5/krb5_opts.h
@@ -45,6 +45,7 @@ struct dp_option default_krb5_opts[] = {
{ "krb5_canonicalize", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
{ "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
+ { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING },
DP_OPTION_TERMINATOR
};
--
2.1.0
From ffe3d9f0e0e697f565fb28861df41fdd9c7bbbe2 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Wed, 29 Apr 2015 06:03:04 -0400
Subject: [PATCH 3/3] krb5: remove field run_as_user
Oops, this is a leftover from the rootless sssd changes. ACK.