[SSSD] sssd not setting IPA AD trusted user homedir

Jakub Hrozek jhrozek at redhat.com
Tue Jan 28 20:45:10 UTC 2014


On Sat, Jan 25, 2014 at 09:06:37PM +0100, Pavel Reichl wrote:
> Hi Jakub,
> 
> thank you for your thorough review. I have (hopefully) addressed all
> your comments and fixed a few things on my own. 
> 
> Please see attached patches.
> 
> PR  
> 

> From 4300f985377bfe16c40faf3f373e1875e5f80433 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Tue, 21 Jan 2014 15:06:37 +0000
> Subject: [PATCH 1/2] Revert "NSS: add support for subdomain_homedir"

ACK

> From 11222a22c512d3da80294f32f394f1653964a10b Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Wed, 22 Jan 2014 16:47:22 +0000
> Subject: [PATCH 2/2] AD: support for subdomain_homedir
> 
> If users from AD don't have set homedir then subdomain_homedir is used
> as default.
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2169

The patch now works as expected - in other words, subdomain_homedir is
honored in IPA server mode but not used for users from a trusted AD
domain on a pure AD client. Both requests by name and requests by ID
work fine.

Please amend the subdomain_homedir documentation to make it clear that
currently the parameter only works in the IPA-AD scenario. Perhaps it
should even be moved to the IPA man page for the time being.

I still have some comments about the code, see below. Some of the
comments might be my personal preference or a matter of taste, so maybe
other developers will correct me :-)

> ---
>  src/db/sysdb.h                        |   6 ++
>  src/db/sysdb_ops.c                    |  68 ++++++++++++++++++
>  src/providers/ipa/ipa_subdomains_id.c | 131 ++++++++++++++++++++++++++++++++++
>  3 files changed, 205 insertions(+)
> 
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index 1f779875db13f887ef4b211450f1b4e170628907..d6969c938a6d8ce991190f26b8c954e95edaff3b 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -596,6 +596,12 @@ int sysdb_add_user(struct sss_domain_info *domain,
>                     int cache_timeout,
>                     time_t now);
>  
> +/* Replace user homedir */
> +errno_t sysdb_store_homedir_of_user(TALLOC_CTX *mem_ctx,
> +                                    struct sss_domain_info *domain,
> +                                    const char *fqname,
> +                                    const char *homedir);
> +

This function doesn't allocate any data, so I don't think there should
be a talloc context in its signature.

Moreover I'm wondering if it makes sense to keep the function in the
sysdb API, because it seems a bit special to me. Did you place it to the
sysdb module in preparation for
https://fedorahosted.org/sssd/ticket/2205 already?

If not, then in my opinion it makes more sense to move the function to
the IPA code..otherwise the function should have a unit test.

>  /* Add group (only basic attrs and w/o checks) */
>  int sysdb_add_basic_group(struct sss_domain_info *domain,
>                            const char *name, gid_t gid);
> diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
> index cb331e1e245335c608d984d8f491f4378549185e..0d4e32c931dd29a84a7c1441e0e883c1250b33f7 100644
> --- a/src/db/sysdb_ops.c
> +++ b/src/db/sysdb_ops.c
> @@ -988,6 +988,74 @@ done:
>      return ret;
>  }
>  
> +/* =Replace-Homedir-Attribute-Of-User======================================== */
> +
> +errno_t
> +sysdb_store_homedir_of_user(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain,
> +                            const char *fqname, const char *homedir)
> +{
> +    errno_t ret;
> +    errno_t sret;
> +    TALLOC_CTX *tmp_ctx;
> +    bool in_transaction = false;
> +    struct sysdb_attrs *attrs;
> +    struct sysdb_ctx *sysdb = domain->sysdb;
> +
> +    tmp_ctx = talloc_new(mem_ctx);

If you don't pass a memory context, then tmp_ctx will be allocated on
NULL.

I think I confused you earlier about how we use the memory contexts.
There are no hard rules, but typically for sync helper functions, we tend
to allocate the tmp_ctx on NULL and free tmp_ctx in the done: label.

This approach has both advantages and disadvantages. The good part is
that you can catch a leak with valgrind. The bad part is that it's
easier to create the leak this way. Allocating the contexts in strictly
hierarchical manner means that some per-request context is usually at
the top of the hierarchy and when the request ends, the nested 'leaked'
context would be freed as well. I personally prefer allocating temporary
contexts on NULL, but some developers disagree.

In a tevent request, I think it's much more acceptable to use 'state' as
a talloc context for the duration of the request (if the request is not
too long).

> +    if (tmp_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    attrs = sysdb_new_attrs(tmp_ctx);
> +    if (attrs == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +

[snip]

> +static errno_t
> +apply_subdomain_homedir(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
> +                        int filter_type, const char *filter_value)
> +{
> +    errno_t ret;
> +    uint32_t uid;
> +    const char *fqname;
> +    const char *homedir;
> +    struct ldb_result *res;
> +
> +    if (filter_type == BE_FILTER_NAME) {
> +        ret = sysdb_getpwnam(mem_ctx, dom, filter_value, &res);
> +    } else if (filter_type == BE_FILTER_IDNUM) {
> +        errno = 0;
> +        uid = strtouint32(filter_value, NULL, 10);
> +        if (errno != 0) {
> +            ret = errno;
> +            goto done;
> +        }
> +        ret = sysdb_getpwuid(mem_ctx, dom, uid, &res);
> +    } else {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              ("Unsoported filter type: [%d].\n", filter_type));

Typo :-)

> +        ret = EINVAL;
> +        goto done;
> +    }
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              ("Failed to make request to our cache: [%d]: [%s]\n",
> +               ret, sss_strerror(ret)));
> +        goto done;
> +    }
> +
> +    if (res->count == 0) {
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    homedir = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_HOMEDIR, NULL);
> +
> +    /*
> +     * If user has explicitly set homedir it is NOT overriden by
> +     * subdoman_homedir.
                ^^
             typo
> +     */
> +    if (homedir == NULL) {

I think this part needs explicit discussion with IPA developers as well
to make sure everyone expects this functionality.

Because currently the extended operation towards IPA client only returns
name,uid and gid, the subdomain_homedir is always in effect on IPA
clients, even if there was a homedir set on the AD side.

But the legacy clients read what the SSSD publishes to the compat tree
and they can leverage the homedir coming from AD already. So we need to
decide if we want to be consistent with the IPA clients and always use
subdomain_homedir or make the homedir available to legacy clients as
your patch does now. I think your approach is better, but it needs to be
discussed.

> +        fqname = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_NAME, NULL);
> +        uid = ldb_msg_find_attr_as_uint64(res->msgs[0], SYSDB_UIDNUM, 0);
> +        if (uid == 0) {
> +            DEBUG(SSSDBG_OP_FAILURE, ("UID for user [%s] is not known.\n",
> +                                      filter_value));
> +            ret = ENOENT;
> +            goto done;
> +        }
> +
> +        ret = get_subdomain_homedir_of_user(mem_ctx, dom, fqname, uid,
> +                                            &homedir);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_OP_FAILURE,
> +                  ("get_subdomain_homedir_of_user failed: [%d]: [%s]\n",
> +                   ret, sss_strerror(ret)));
> +            goto done;
> +        }
> +
> +        ret = sysdb_store_homedir_of_user(mem_ctx, dom, fqname, homedir);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_OP_FAILURE,
> +                  ("sysdb_store_homedir_of_user failed: [%d]: [%s]\n",
> +                   ret, sss_strerror(ret)));
> +            goto done;
> +        }
> +    }

Please put a blank line here.

> +done:
> +    return ret;
> +}
> +



More information about the sssd-devel mailing list