[SSSD] sssd not setting IPA AD trusted user homedir

Jakub Hrozek jhrozek at redhat.com
Fri Jan 24 11:20:34 UTC 2014


On Wed, Jan 22, 2014 at 08:11:47PM +0100, Pavel Reichl wrote:
> On Wed, 2014-01-22 at 17:31 +0100, Jakub Hrozek wrote:
> > On Wed, Jan 22, 2014 at 05:13:38PM +0100, Pavel Reichl wrote:
> > > On Fri, 2014-01-17 at 13:02 +0100, Sumit Bose wrote:
> > > > On Fri, Jan 17, 2014 at 12:56:09PM +0100, Jakub Hrozek wrote:
> > > > > On Fri, Jan 17, 2014 at 12:43:04PM +0100, Sumit Bose wrote:
> > > > > > On Fri, Jan 17, 2014 at 12:04:38PM +0100, Jakub Hrozek wrote:
> > > > > > > On Fri, Jan 17, 2014 at 11:55:08AM +0100, Pavel Reichl wrote:
> > > > > > > > 
> > > > > > > > > We just have to remember to touch this code, if we start to support home
> > > > > > > > > directories defined in AD.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I'm sorry that I failed to see what you were implying. My patch will
> > > > > > > > override value of homedir for members of every subdomain by
> > > > > > > > subdomain_homedir. 
> > > > > > > > 
> > > > > > > > I missed that even if subdomain_homedir is not set explicitly in
> > > > > > > > sssd.conf it has a non NULL value. 
> > > > > > > 
> > > > > > > Me too and to be honest I failed to test the patch with AD subdomains
> > > > > > > with POSIX attributes. Sorry about that.
> > > > > > > 
> > > > > > > I think Sumit's suggestion makes the most sense now, but the
> > > > > > > documentation should be amended, too. Currently the man page implies
> > > > > > > that the subdomain_homedir parameter will have equal effect for all
> > > > > > > kinds of subdomains, while the code would only use it for IPA-AD trust
> > > > > > > cases (be it for the server mode or the client).
> > > > > > 
> > > > > > I also realized that having the right home directory in sysdb might be
> > > > > > useful because krb5_ccname_template allows to expand %h to the user's
> > > > > > home directory. And here the home directory is looked up in sysdb.
> > > > > > 
> > > > > > bye,
> > > > > > Sumit
> > > > > 
> > > > > But that also affects the other options, right? Sounds like a second
> > > > > problem to me, or did you want to solve both together?
> > > > 
> > > > no
> > > > 
> > > > > 
> > > > > About the subdomain homedir, I think we should:
> > > > >     1) revert the original patch that is commited
> > > > >     2) fix the IPA server mode the way you described in your reply to
> > > > >        Pavel's mail
> > > > >     3) Amend the subdomain_homedir documentation so that it's clear
> > > > >        where the option has effect
> > > > 
> > > > +1
> > > > 
> > > > bye,
> > > > Sumit
> > > > 
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > sssd-devel at lists.fedorahosted.org
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > sssd-devel at lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > 
> > > Hello,
> > > 
> > > please see attached patches. 
> > > 
> > > PR
> > > 
> > 
> > I haven't tried the patches yet, just read the diff..
> > 
> > > 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"
> > 
> > Looks good to me.
> > 
> > > From 5eb3fd0cc7a11b0f71fa10a3d41028738382bc8e 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
> > > 
> > 
> > See two comments inline
> > 
> > > If users from AD don't have set homedir then subdomain_homedir is used
> > > as default.
> > > 
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2169
> > > ---
> > >  src/providers/ipa/ipa_subdomains_id.c | 89 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 89 insertions(+)
> > > 
> > > diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
> > > index c29a2a3047af105966b636422105abd15e8a3992..a498a01e9dcc3e3a4c7b59323da9dae017876686 100644
> > > --- a/src/providers/ipa/ipa_subdomains_id.c
> > > +++ b/src/providers/ipa/ipa_subdomains_id.c
> > > @@ -350,6 +350,70 @@ ipa_get_ad_id_ctx(struct ipa_id_ctx *ipa_ctx,
> > >      return (iter) ? iter->ad_id_ctx : NULL;
> > >  }
> > >  
> > > +static errno_t
> > > +use_subdomain_homedir(TALLOC_CTX *mem_ctx, const struct sss_domain_info *dom,
> > > +                      const char *name, uint32_t uid)
> > > +{
> > > +    errno_t ret;
> > > +    char *fqname = NULL;
> > > +    char *homedir = NULL;
> > > +    struct sysdb_attrs *attrs = NULL;
> > > +    struct sysdb_ctx *sysdb = dom->sysdb;
> > 
> > You can create a child context here:
> >         TALLOC_CTX *tmp_ctx = NULL;
> > 
> >         tmp_ctx = talloc_new(mem_ctx);  /* or NULL */
> >         if (tmp_ctx == NULL) {
> >             return ENOMEM;
> >         }
> > > +
> > > +    fqname = talloc_asprintf(mem_ctx, "%s@%s", name, dom->name);
> > 
> > Then use tmp_ctx instead of mem_ctx through the function
> > 
> > > +    if (fqname == NULL) {
> > > +        ret = ENOMEM;
> > > +        goto done;
> > > +    }
> > > +
> > > +    attrs = sysdb_new_attrs(mem_ctx);
> > > +    if (attrs == NULL) {
> > > +        ret = ENOMEM;
> > > +        goto done;
> > > +    }
> > > +
> > > +    homedir = expand_homedir_template(mem_ctx, dom->subdomain_homedir, name,
> > > +                                      uid, NULL, dom->name, dom->flat_name);
> > > +    if (homedir == NULL) {
> > > +        DEBUG(SSSDBG_OP_FAILURE, ("expand_homedir_template failed\n"));
> > > +        ret = ENOMEM;
> > > +        goto done;
> > > +    }
> > > +
> > > +    ret = sysdb_attrs_add_string(attrs, SYSDB_HOMEDIR, homedir);
> > > +    if (ret) {
> > > +        DEBUG(SSSDBG_MINOR_FAILURE, ("Error setting homedir: [%s]\n",
> > > +                                     strerror(ret)));
> > > +        goto done;
> > > +    }
> > > +
> > > +    ret = sysdb_transaction_start(sysdb);
> > > +    if (ret != EOK) {
> > > +        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to start transaction\n"));
> > > +        goto done;
> > > +    }
> > > +
> > > +    ret = sysdb_set_user_attr(dom, fqname, attrs, SYSDB_MOD_REP);
> > > +    if (ret) {
> > > +        DEBUG(SSSDBG_CRIT_FAILURE,
> > > +              ("Failed to update Login attempt information!\n"));
> > > +    }
> > > +
> > > +    ret = sysdb_transaction_commit(sysdb);
> > > +    if (ret != EOK) {
> > > +        DEBUG(SSSDBG_CRIT_FAILURE,
> > > +              ("Cannot commit sysdb transaction [%d]: %s\n",
> > > +               ret, strerror(ret)));
> > > +        goto done;
> > > +    }
> > > +
> > > +done:
> > > +    talloc_zfree(attrs);
> > > +    talloc_zfree(fqname);
> > > +    talloc_zfree(homedir);
> > 
> > And finally since all three variables are allocated on tmp_ctx, you can just
> > free the context:
> >     talloc_free(tmp_ctx);
> > 
> > > +    return ret;
> > > +}
> > > +
> > >  static void
> > >  ipa_get_ad_acct_ad_part_done(struct tevent_req *subreq)
> > >  {
> > > @@ -358,6 +422,9 @@ ipa_get_ad_acct_ad_part_done(struct tevent_req *subreq)
> > >      struct ipa_get_ad_acct_state *state = tevent_req_data(req,
> > >                                                  struct ipa_get_ad_acct_state);
> > >      errno_t ret;
> > > +    uint32_t uid;
> > > +    char *homedir;
> > > +    struct ldb_result *res;
> > >  
> > >      ret = ad_handle_acct_info_recv(subreq, &state->dp_error, NULL);
> > >      talloc_zfree(subreq);
> > > @@ -367,6 +434,28 @@ ipa_get_ad_acct_ad_part_done(struct tevent_req *subreq)
> > >          return;
> > >      }
> > >  
> > > +    ret = sysdb_getpwnam(state, state->user_dom, state->ar->filter_value, &res);
> > > +    if (ret != EOK) {
> > > +        DEBUG(SSSDBG_OP_FAILURE,
> > > +              ("Failed to make request to our cache: [%d]: [%s]\n",
> > > +               ret, sss_strerror(ret)));
> > > +        return;
> > > +    }
> > 
> > I think here ^^^ you also need to branch based on the value of
> > ar->filter_type. The filter_type can also be an ID number, in which case
> > you'd search the user by ID.
> > 
> > > +
> > > +    homedir = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_HOMEDIR, NULL);
> > > +
> > > +    if (homedir == NULL) {
> > > +        uid = ldb_msg_find_attr_as_uint64(res->msgs[0], SYSDB_UIDNUM, 0);
> > > +        ret = use_subdomain_homedir(state, state->user_dom,
> > > +                                    state->ar->filter_value, uid);
> > > +        if (ret != EOK) {
> > > +            DEBUG(SSSDBG_OP_FAILURE,
> > > +                  ("use_subdomain_homedir failed: [%d]: [%s]\n",
> > > +                   ret, sss_strerror(ret)));
> > > +            return;
> > > +        }
> > > +    }
> > > +
> > >      if ((state->ar->entry_type & BE_REQ_TYPE_MASK) != BE_REQ_INITGROUPS) {
> > >          tevent_req_done(req);
> > >          return;
> > > -- 
> > > 1.8.4.2
> > > 
> Hi Jakub,
> 
> new patch addressing your comments is attached. 
> 
> PR
> 

Looks like you forgot to include some changes?

/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:393:15: error: implicit declaration of function 'expand_homedir_template' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    homedir = expand_homedir_template(tmp_ctx, dom->subdomain_homedir, name,
              ^
/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:393:13: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
    homedir = expand_homedir_template(tmp_ctx, dom->subdomain_homedir, name,

/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:414:31: warning: passing 'const struct sss_domain_info *' to parameter of type 'struct sss_domain_info *' discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
    ret = sysdb_set_user_attr(dom, fqname, attrs, SYSDB_MOD_REP);
                              ^~~
/home/remote/jhrozek/devel/sssd/src/db/sysdb.h:558:49: note: passing argument to parameter 'domain' here
int sysdb_set_user_attr(struct sss_domain_info *domain,
                                                ^
/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:373:9: warning: variable 'attrs' is uninitialized when used here [-Wuninitialized]
    if (attrs == NULL) {
        ^~~~~
/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:363:30: note: initialize the variable 'attrs' to silence this warning
    struct sysdb_attrs *attrs;
                             ^
                              = NULL


The patches look fairly well, though, see some comments inline:

> 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"

Looks good to me.

> From 4a2a326b383ae22fab1a8f572276a880b5b51ee2 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
> ---
>  src/providers/ipa/ipa_subdomains_id.c | 147 ++++++++++++++++++++++++++++++++++
>  1 file changed, 147 insertions(+)
> 
> diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
> index c29a2a3047af105966b636422105abd15e8a3992..50193b71ae293b0fd3a33714086784766f53ef35 100644
> --- a/src/providers/ipa/ipa_subdomains_id.c
> +++ b/src/providers/ipa/ipa_subdomains_id.c
> @@ -350,6 +350,143 @@ ipa_get_ad_id_ctx(struct ipa_id_ctx *ipa_ctx,
>      return (iter) ? iter->ad_id_ctx : NULL;
>  }
>  
> +static errno_t
> +store_subdomain_homedir(TALLOC_CTX *mem_ctx, const struct sss_domain_info *dom,
> +                      const char *fqname, uint32_t uid)
                      ^^^
                  indentiation is off
> +{
> +    char *name;
> +    errno_t ret;
> +    errno_t sret;
> +    char *homedir;
> +    char* end_of_name;
> +    TALLOC_CTX *tmp_ctx;
> +    struct sysdb_attrs *attrs;
> +    struct sysdb_ctx *sysdb = dom->sysdb;
> +
> +    tmp_ctx = talloc_new(mem_ctx);
> +    if (tmp_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    name = talloc_strdup(tmp_ctx, fqname);
> +    if (attrs == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    /* fqname = name at domain */

By default, fqname in sysdb is name at domain but not always, the user can
configure the format using the full_name_format option (even in sysdb)
I think you need to use sss_parse_name() in order to parse the fqdn into
(name,domain).

This is one of the places that would get fixed when Michal converts the
db to use a unified format internally and only use full_name_format for
the outside representation.

> +    end_of_name = strchr(name, '@');
> +    if (end_of_name == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, ("%s is not fully qualified name.\n", fqname));
> +        ret = EINVAL;
> +        goto done;
> +    }
> +    *end_of_name = '\0';
> +
> +    attrs = sysdb_new_attrs(tmp_ctx);
> +    if (attrs == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    homedir = expand_homedir_template(tmp_ctx, dom->subdomain_homedir, name,
> +                                      uid, NULL, dom->name, dom->flat_name);
> +    if (homedir == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, ("expand_homedir_template failed\n"));
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    ret = sysdb_attrs_add_string(attrs, SYSDB_HOMEDIR, homedir);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_MINOR_FAILURE, ("Error setting homedir: [%s]\n",
> +                                     strerror(ret)));
> +        goto done;
> +    }
> +
> +    ret = sysdb_transaction_start(sysdb);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to start transaction\n"));
> +        goto done;
> +    }
> +
> +    ret = sysdb_set_user_attr(dom, fqname, attrs, SYSDB_MOD_REP);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              ("Failed to update Login attempt information!\n"));
> +        sret = sysdb_transaction_cancel(sysdb);
> +        if (sret != EOK) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, ("Could not cancel transaction\n"));
> +        }
> +        goto done;

This construct is fine for a very short transaction, but normally I
prefer to use the same format as we use elsewhere with a boolean
in_transaction and call sysdb_transaction_cancel if the boolean is set
and ret is != EOK in the done handler. But this is mostly something to
keep in mind for future or refactoring.

> +    }
> +
> +    ret = sysdb_transaction_commit(sysdb);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              ("Cannot commit sysdb transaction [%d]: %s\n",
> +               ret, strerror(ret)));
> +        goto done;
> +    }
> +
> +done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +
> +
> +static errno_t
> +use_subdomain_homedir(TALLOC_CTX *mem_ctx, const struct sss_domain_info *dom,
> +                      int filter_type, const char *filter_value)
> +{
> +    errno_t ret;
> +    uint32_t uid;
> +    char *fqname;
> +    char *homedir;
> +    struct ldb_result *res;
> +
> +    if (filter_type == BE_FILTER_NAME) {
> +        ret = sysdb_getpwnam(mem_ctx, dom, filter_value, &res);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_OP_FAILURE,
> +                  ("Failed to make request to our cache: [%d]: [%s]\n",
> +                   ret, sss_strerror(ret)));
> +            goto done;
> +        }

The check for 'ret != EOK' can be moved after the whole if-elseif-else
block if you're concerned about too much code in the function.

> +        uid = ldb_msg_find_attr_as_uint64(res->msgs[0], SYSDB_UIDNUM, 0);
> +    } else if (filter_type == BE_FILTER_IDNUM) {
> +        uid = strtouint32(filter_value, NULL, 10);

I would prefer to check if the conversion went fine:
    uid = strtouint32(filter_value, &endptr, 10);
    if (errno || *endptr || (state->name == endptr)) {
        ret = EINVAL;
        goto done;
    }

It will always succeed, because we already searched using this conversion,
but I think static code analyzers might be confused that we check the
functions N-1 times out of N uses.

By the way, I wonder if we could add another field into the 'ar'
structure that would be the converted filter value (not now, just for
future). Something like:

union be_filter_val {
    id_t id;
    const char *name;
    conts char *sid;
};

Then we would only parse the value once on entering the ID provider..

> +        ret = sysdb_getpwuid(mem_ctx, dom, uid, &res);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_OP_FAILURE,
> +                  ("Failed to make request to our cache: [%d]: [%s]\n",
> +                   ret, sss_strerror(ret)));
> +            goto done;
> +        }
> +    } else {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              ("Unsoported filter type: [%d].\n", filter_type));
> +        ret = EINVAL;
> +        goto done;
> +    }
> +
> +    homedir = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_HOMEDIR, NULL);
> +    fqname = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_NAME, NULL);
> +
> +    if (homedir == NULL) {
> +        ret = store_subdomain_homedir(mem_ctx, dom, fqname, uid);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_OP_FAILURE,
> +                  ("use_subdomain_homedir failed: [%d]: [%s]\n",

Copy-n-paste error, store_subdomain_homedir failed, not
use_subdomain_homedir :-)

> +                   ret, sss_strerror(ret)));
> +            return;
> +        }
> +    }
> +done:
> +    return ret;
> +}
> +
>  static void
>  ipa_get_ad_acct_ad_part_done(struct tevent_req *subreq)
>  {
> @@ -367,6 +504,16 @@ ipa_get_ad_acct_ad_part_done(struct tevent_req *subreq)
>          return;
>      }
>  
> +    ret = use_subdomain_homedir(state, state->user_dom,
> +                                  state->ar->filter_type,
> +                                  state->ar->filter_value);
                                ^^^^
                        indentation
                
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              ("use_subdomain_homedir failed: [%d]: [%s]\n",
> +               ret, sss_strerror(ret)));
> +        return;
> +    }
> +

The patch is looking mostly good, these are just nitpicks :-)



More information about the sssd-devel mailing list