[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