[SSSD] [PATCHES] krb5: new option krb5_map_user

Jakub Hrozek jhrozek at redhat.com
Tue May 26 09:13:38 UTC 2015


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 at 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..



More information about the sssd-devel mailing list