[SSSD] [PATCHES] krb5: new option krb5_map_user

Jakub Hrozek jhrozek at redhat.com
Mon May 11 11:56:19 UTC 2015


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.



More information about the sssd-devel mailing list