[SSSD] [PATCHES] krb5: new option krb5_map_user

Jakub Hrozek jhrozek at redhat.com
Thu May 14 15:10:18 UTC 2015


On Thu, May 14, 2015 at 01:55:01PM +0200, Pavel Reichl wrote:
> Please see updated patch.

Thanks, this looks much better. I only have some nitpicks now:

> From 4f0bd09258e52ae633bc91603e30edf9ce02b279 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

[...]

> +                        </para>
> +                        <para>
> +                            `joe` and `vince` are UNIX user names and `juser`
> +                            and `rraines` are primaries of kerberos principals.
> +                            For user `joe` resp. `dick` SSSD will try to kinit
> +                            as `juser at REALM` resp. `richard at REALM`

Could we use something else than backticks in the XML. Remember we used
XML because it's possible to render other formats than troff, we already
render HTML. Would <quote> be a better choice?

> +                        </para>
> +
> +                        <para>
> +                            Default: not set
> +                        </para>
> +                    </listitem>
> +                </varlistentry>
> +
>              </variablelist>
>          </para>
>      </refsect1>

[...]

> +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;
> +    talloc(mem_ctx, int);

huh?

> +
> +    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");
> +        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,
> +                       int size)

Can we use size_t for 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;
> +        ret = EOK;
> +        goto done;
> +    }

I won't block the patch over this but I still don't understand why you
prefer to use the NULL pointer for "an empty mapping" instead of always
allocating the sentinel. I'd argue that using the sentinel would
ultimately make the code easier because you wouldn't have to check for
NULL mapping pointer, but just loop over the mapping array..one less
special case..

> +
> +    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);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_MINOR_FAILURE,
> +              "fill_name_to_primary_map failed: %s:[%d]\n",
> +              sss_strerror(ret), ret);
> +        goto done;
> +    }
> +
> +    ret = EOK;
> +
> +done:
> +    if (ret == EOK) {
> +        if (size == 0) {
> +            *_name_to_primary = NULL;
> +        } else {
> +            *_name_to_primary = talloc_steal(mem_ctx, name_to_primary);
> +        }
> +    }
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}



More information about the sssd-devel mailing list