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(a)lists.fedorahosted.org
> > >
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)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(a)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(a)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