[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