On Thu, Jun 27, 2013 at 01:27:28PM +0200, Jakub Hrozek wrote:
> Hi,
>
> during testing I found out that we mishandle UPNs for subdomain users
> when using Kerberos authentication.
>
> If there is no userPrincipal attribute we guess based on username@REALM.
> But for subdomain users the username is already qualified, so so you end
> up with username@DOMAIN@REALM. Currently first login works fine because
> krb5 auth code treats the result as an enterprise principal. But if you
> are checking existing ccache then the krb5 code errors out because one of
> the krb5_cc_* functions treats username@DOMAIN@REALM as invalid principal.
>
> The attached patch checks if the username is already qualified and
> replaces the domain name with realm name when guessing the UPN. I really
> don't like the result because parsing out is inherently fragile. I think
> we should store the plain username in an additional sysdb attribute,
> too.
...
> }
>
> errno_t krb5_get_simple_upn(TALLOC_CTX *mem_ctx, struct krb5_ctx *krb5_ctx,
> - const char *domain_name, const char *username,
> + struct sss_domain_info *dom, const char
> *username,
> const char *user_dom, char **_upn)
> {
> const char *realm = NULL;
> char *uc_dom = NULL;
> char *upn;
> + char *name;
> + char *domname;
> + TALLOC_CTX *tmp_ctx = NULL;
I'm not sure a tmp_ctx is really needed here.
> + errno_t ret;
>
> - if (user_dom != NULL && domain_name != NULL &&
> - strcasecmp(domain_name,user_dom) != 0) {
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_new failed.\n"));
> + return ENOMEM;
> + }
> +
> + if (user_dom != NULL && dom->name != NULL &&
> + strcasecmp(dom->name, user_dom) != 0) {
> uc_dom = get_uppercase_realm(mem_ctx, user_dom);
> if (uc_dom == NULL) {
> DEBUG(SSSDBG_OP_FAILURE, ("get_uppercase_realm failed.\n"));
> - return ENOMEM;
> + ret = ENOMEM;
> + goto done;
> }
> } else {
> realm = dp_opt_get_cstring(krb5_ctx->opts, KRB5_REALM);
> if (realm == NULL) {
> DEBUG(SSSDBG_OP_FAILURE, ("Missing Kerberos realm.\n"));
> - return ENOENT;
> + ret = ENOMEM;
> + goto done;
> }
> }
>
> + /* Subdomains already have a fully qualified name, which contains
> + * the domain name. We need to replace it with the realm name
> + */
> + ret = sss_parse_name(tmp_ctx, dom->names, username, &domname, &name);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_CRIT_FAILURE, ("Could not parse %s into name and " \
> + "domain components\n"));
> + goto done;
> + }
> +
> /* NOTE: this is a hack, works only in some environments */
> - upn = talloc_asprintf(mem_ctx, "%s@%s", username,
> + upn = talloc_asprintf(mem_ctx, "%s@%s", name,
> realm != NULL ? realm : uc_dom);
> + talloc_free(name);
if you call talloc_free(name) here you should also call
talloc_free(domname) after calling sss_parse_name(). And the you do not
need the tmp_ctx.
> talloc_free(uc_dom);
> if (upn == NULL) {
> DEBUG(1, ("talloc_asprintf failed.\n"));
> - return ENOMEM;
> + ret = ENOMEM;
> + goto done;
> }
>
> DEBUG(9, ("Using simple UPN [%s].\n", upn));
>
> - *_upn = upn;
> - return EOK;
> + *_upn = talloc_steal(mem_ctx, upn);
You used mem_ctx to allocate upn, so not stealing is needed here.
bye,
Sumit
> + ret = EOK;
> +done:
> + talloc_free(tmp_ctx);
> + return ret;
> }
>
> errno_t compare_principal_realm(const char *upn, const char *realm,
> diff --git a/src/providers/krb5/krb5_common.h
> b/src/providers/krb5/krb5_common.h
> index
> 501cdef10b0afc746ada7763a4253d4a0e3988b7..9eb602cfba7a032374ff57db53fd3c3638209153
> 100644
> --- a/src/providers/krb5/krb5_common.h
> +++ b/src/providers/krb5/krb5_common.h
> @@ -182,7 +182,7 @@ errno_t write_krb5info_file(const char *realm, const char
> *kdc,
> errno_t remove_krb5_info_files(TALLOC_CTX *mem_ctx, const char *realm);
>
> errno_t krb5_get_simple_upn(TALLOC_CTX *mem_ctx, struct krb5_ctx *krb5_ctx,
> - const char *domain_name, const char *username,
> + struct sss_domain_info *dom, const char
> *username,
> const char *user_dom, char **_upn);
>
> errno_t compare_principal_realm(const char *upn, const char *realm,
> diff --git a/src/providers/krb5/krb5_renew_tgt.c
> b/src/providers/krb5/krb5_renew_tgt.c
> index
> 0b1f26fd3a646e29a2f39ae6699714ca5318b66f..d6cdff8f8c0efc98445363c0a12a7ec19d12477f
> 100644
> --- a/src/providers/krb5/krb5_renew_tgt.c
> +++ b/src/providers/krb5/krb5_renew_tgt.c
> @@ -442,7 +442,7 @@ static errno_t check_ccache_files(struct renew_tgt_ctx
> *renew_tgt_ctx)
> }
>
> ret = find_or_guess_upn(tmp_ctx, msgs[c], renew_tgt_ctx->krb5_ctx,
> - renew_tgt_ctx->be_ctx->domain->name,
> + renew_tgt_ctx->be_ctx->domain,
> user_name, user_dom, &upn);
> if (ret != EOK) {
> DEBUG(SSSDBG_OP_FAILURE, ("find_or_guess_upn failed.\n"));
> diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c
> index
> 3f16faa7fb238bbb9801029beed66293cd873c15..1f7ed07452d8814e813692f84423c74c1c8ecb65
> 100644
> --- a/src/providers/krb5/krb5_utils.c
> +++ b/src/providers/krb5/krb5_utils.c
> @@ -32,7 +32,7 @@
>
> errno_t find_or_guess_upn(TALLOC_CTX *mem_ctx, struct ldb_message *msg,
> struct krb5_ctx *krb5_ctx,
> - const char *domain_name, const char *user,
> + struct sss_domain_info *dom, const char *user,
> const char *user_dom, char **_upn)
> {
> const char *upn;
> @@ -40,7 +40,7 @@ errno_t find_or_guess_upn(TALLOC_CTX *mem_ctx, struct
> ldb_message *msg,
>
> upn = ldb_msg_find_attr_as_string(msg, SYSDB_UPN, NULL);
> if (upn == NULL) {
> - ret = krb5_get_simple_upn(mem_ctx, krb5_ctx, domain_name, user,
> + ret = krb5_get_simple_upn(mem_ctx, krb5_ctx, dom, user,
> user_dom, _upn);
> if (ret != EOK) {
> DEBUG(SSSDBG_OP_FAILURE, ("krb5_get_simple_upn failed.\n"));
> diff --git a/src/providers/krb5/krb5_utils.h b/src/providers/krb5/krb5_utils.h
> index
> aad2770d4d3beb28c3ccb06278ee4cfc88562c85..2e1bec717ac7c3ac1c9bb87392d011eb854f21be
> 100644
> --- a/src/providers/krb5/krb5_utils.h
> +++ b/src/providers/krb5/krb5_utils.h
> @@ -34,7 +34,7 @@
>
> errno_t find_or_guess_upn(TALLOC_CTX *mem_ctx, struct ldb_message *msg,
> struct krb5_ctx *krb5_ctx,
> - const char *domain_name, const char *user,
> + struct sss_domain_info *dom, const char *user,
> const char *user_dom, char **_upn);
>
> errno_t check_if_cached_upn_needs_update(struct sysdb_ctx *sysdb,
> --
> 1.8.3.1
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
