[SSSD] [PATCHES] krb5: new option krb5_map_user
Pavel Reichl
preichl at redhat.com
Thu May 14 11:55:01 UTC 2015
On 05/11/2015 01:56 PM, Jakub Hrozek wrote:
> 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 at 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 at 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 at 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.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Please see updated patch.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-krb5-new-option-krb5_map_user.patch
Type: text/x-patch
Size: 23586 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150514/f0601e18/attachment.bin>
More information about the sssd-devel
mailing list