On Mon, 2014-04-14 at 18:30 +0200, Jakub Hrozek wrote:
On Fri, Apr 04, 2014 at 07:39:30PM +0200, Pavel Reichl wrote:
> Hello,
>
> attached patches implement ticket:
>
>
https://fedorahosted.org/sssd/ticket/2205
>
> Maybe I split the changes into too many patches, please feel free to
> suggest squashing them.
Thank you Jakub for comments, please see responses inline.
No, I actually prefer to receive more smaller patches.
>
> Regards,
>
> Pavel Reichl
I haven't tried the patches yet. So far, the review has been based on
visual inspection only, because I think we need to agree on the
principles first (and I forgot some details about subdomain homedirs
already, sorry...)
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Tue, 1 Apr 2014 18:18:39 +0100
> Subject: [PATCH 01/12] NSS: use homedir from sysdb
>
> NSS returns user's home directory from sysdb.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2205
> @@ -372,12 +330,16 @@ static int fill_pwent(struct sss_packet *packet,
> } else {
> to_sized_string(&gecos, tmpstr);
> }
> - tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom, name.str, uid);
> - if (!tmpstr) {
> - to_sized_string(&homedir, "/");
> +
> + tmpstr = get_overriden_homedir(tmp_ctx, msg);
> + if (tmpstr == NULL || tmpstr[0] == '\0') {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "Failed to get valid homedir from sysdb.\n");
> + to_sized_string(&homedir, FALLBACK_HOMEDIR);
This fallback shouldn't be handled by sssd, but by glibc. I think SSSD
should return an empty string (I hope that's what to_sized_string()
yields when you pass a NULL second parameter) and glibc should pick its
own fallback string.
void to_sized_string(struct sized_string *out, const char *in)
{
out->str = in;
if (out->str) {
out->len = strlen(out->str) + 1;
} else {
out->len = 0;
}
}
If second parameter is NULL then I suppose that out->str is NULL and
out->len is 0. So I thing that check for NULL is still needed.
- tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom, name.str, uid);
- if (!tmpstr) {
- to_sized_string(&homedir, "/");
+
+ tmpstr = get_overriden_homedir(tmp_ctx, msg);
+ if (tmpstr == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Failed to get valid homedir from sysdb.\n");
+ to_sized_string(&homedir, "");
Other possibility is to change implementation of to_sized_string() to behave as you
expected, but I think that would require too much effort to check and change all places
where it is called from.
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Tue, 1 Apr 2014 17:30:52 +0100
> Subject: [PATCH 02/12] NSS: remove override_homedir and fallback_homedir
Visual ACK
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Tue, 1 Apr 2014 17:30:52 +0100
> Subject: [PATCH 03/12] SSSD: Rename sss_nss to sss_homedir module
The intent is fine, but since the functionality will only be used by
providers when saving entries, can we also move the file to
src/providers ?
The patch should also change the copyright blob, "Utility functions
related to ID information" doesn't sound right.
Will do.
> From d0489ec17eda99ea358d9750854b3bae760d7a15 Mon Sep 17
00:00:00 2001
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Tue, 1 Apr 2014 18:36:33 +0100
> Subject: [PATCH 04/12] confdb: set domain homedir overrides
Visual ACK, I'm just not sure there needs to be a new special variable
'nss_homedir', can you use 'tmp' like the other parameters?
Well, that was my first idea too, but please note that nss_homedir's
address is passed into confdb_get_string which expects (char**) and
would produce a warning if &tmp was passed in.
I thing Michal recently discussed this issue on list. I could use
discard_const hack, or (preferably) wrapper function, or keep current
solution.
const char *tmp;
char *nss_homedir;
> From 7f23a86c1e3662c4a120fa5bdbca364f130a0bfe Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Thu, 3 Apr 2014 11:01:37 +0100
> Subject: [PATCH 05/12] utils: common functions for homedir overrides
>
> Functions used by providers to perform overrides on user's home directories.
As a general comment, a unit test would be nice for this module.
Will do.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2205
> ---
> src/util/sss_homedir.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/sss_homedir.h | 13 ++++++
> 2 files changed, 123 insertions(+)
>
> diff --git a/src/util/sss_homedir.c b/src/util/sss_homedir.c
> index
94c54236b011fca65f2a798b174665fc8eb5ec03..097af2b4288af9a06f94c7cb82f6f12ae3a7e0aa 100644
> --- a/src/util/sss_homedir.c
> +++ b/src/util/sss_homedir.c
> @@ -158,3 +158,113 @@ done:
> talloc_zfree(tmp_ctx);
> return res;
> }
> +
> +static errno_t parse_lc_username(TALLOC_CTX *mem_ctx,
> + struct sss_names_ctx *snctx,
> + const char *fqname,
> + char **_lc_username)
> +{
> + errno_t ret;
> + char *username;
> + char *lc_username;
> + TALLOC_CTX *tmp_ctx;
> +
> + tmp_ctx = talloc_new(mem_ctx);
This is a point of discussion between developers for quite some time --
some prefer allocating temporary contexts on NULL to be able to see them
with valgrind, some prefer to use the parent context to make sure the
temporary context goes away even if we forget to free it explicitly.
My personal preference is to use NULL for utility functions and allow
using parent context for async requests. But I can be convinced other
way :)
Make sense, will do. (And I'll even try to remember next time.)
> + if (tmp_ctx == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + ret = sss_parse_name(tmp_ctx, snctx, fqname, NULL, &username);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into "
> + "name-value components.\n", fqname);
> + goto done;
> + }
> +
> + lc_username = sss_tc_utf8_str_tolower(tmp_ctx, username);
> + if (lc_username == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + if (_lc_username == NULL) {
> + ret = EINVAL;
> + goto done;
> + }
> + *_lc_username = talloc_steal(mem_ctx, lc_username);
> + ret = EOK;
> +
> +done:
> + talloc_free(tmp_ctx);
> + return ret;
> +}
> +
> +static const char *perform_homedir_override(TALLOC_CTX *mem_ctx,
> + const char* homedir,
> + struct sss_domain_info *dom,
> + const char *fqname,
> + uint32_t uid,
> + enum subdomain_homedir_usage sh_use)
> +{
> + char *lc_username;
> + errno_t ret;
> +
> + /* To be compatible with the old winbind based user lookups and IPA
> + * clients the user name in the home directory path will be lower-case.
> + */
> + ret = parse_lc_username(mem_ctx, dom->names, fqname, &lc_username);
Doesn't this code lowercase all homedirs for all users, even for those
coming from pure LDAP domains with mixed or upper case user names?
Yes, it does. I fill fix this.
So to make it clear, lower-cased user names should be used only for
users from AD and they should be used for expansions of all home dir
templates - override, fallback and subdomain?
Also, lc_username is never freed in this function, so there is a
side-effect the prototype doesn't advertise. Either free lc_username in
a done: label or use tmp_ctx.
OK, will fix that.
> + if (ret != EOK) {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "Failed to parse lowercased username from %s.\n", fqname);
> + return NULL;
> + }
> +
> + /* Check whether we are unconditionally overriding the server
> + * for home directory locations.
> + */
> + if (dom->override_homedir != NULL) {
> + return expand_homedir_template(mem_ctx, dom->override_homedir,
> + lc_username, uid, homedir, dom->name,
> + NULL);
> + }
> +
> + if (sh_use == USE_SUBDOMAIN_HOMEDIR && dom->subdomain_homedir) {
> + return expand_homedir_template(mem_ctx, dom->subdomain_homedir,
> + lc_username, uid, homedir, dom->name,
> + dom->flat_name);
> + }
I should probably test the patches at this point :) but if there was
both subdomain_homedir and override_homedir set, would the subdomain
users get the same homedir as before the patches? In other words, do we
change the evaluation order with this patch (that wouldn't be good).
Yes, it's same, override_homedir beats subdomain_homedir.
Subdomain-homedir was first stored to sysdb by back end, but NSS
responder beat it with override_homedir (if it was set).
I tested this for both ipa-client and in ipa-server mode.
> +
> + if (homedir == NULL || *homedir == '\0') {
> + /* In the case of a NULL or empty homedir, check to see if
> + * we have a fallback homedir to use.
> + */
> + if (dom->fallback_homedir != NULL) {
> + return expand_homedir_template(mem_ctx, dom->fallback_homedir,
> + lc_username, uid, homedir,
> + dom->name, NULL);
> + } else {
> + return NULL;
> + }
> + }
> +
> + /* Return the value we got from the provider */
> + return talloc_strdup(mem_ctx, homedir);
> +}
> +
> +const char *safe_homedir_override(TALLOC_CTX *mem_ctx,
> + const char* homedir,
> + struct sss_domain_info *dom,
> + const char *fqname,
> + uint32_t uid,
> + enum subdomain_homedir_usage sh_use)
> +{
> + const char *res;
> + res = perform_homedir_override(mem_ctx, homedir, dom, fqname, uid, sh_use);
> + if (res == NULL || res[0] == '\0') {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "perform_homedir_override returned invalid homedir.\n");
> + return FALLBACK_HOMEDIR;
As said earlier, the fallback should be handled by glibc. In case
expanding the homedir fails, I think we could just store the entry
without the homedir attribute completely and let the responder handle
returning the empty string which glibc should evaluate as "/".
Also, it seems confusing to me that the function can return both a
talloc pointer and a pointer to a static buffer. That means talloc_free
can't be safely called on the result.
Yes, you are right and I'm sorry for that. Now I don't see any point in
having safe_homedir_override function, because as you suggested fallback
should be handled by glibc.
> + }
> + return res;
> +}
> diff --git a/src/util/sss_homedir.h b/src/util/sss_homedir.h
> index
446b58e3df710085e640733797efd4f0434167ef..b14c041761037d4a7dafd840a0ab480cc7aec5ac 100644
> --- a/src/util/sss_homedir.h
> +++ b/src/util/sss_homedir.h
> @@ -29,4 +29,17 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx, const char
*template,
> const char *username, uint32_t uid,
> const char *original, const char *domain,
> const char *flatname);
> +
> +enum subdomain_homedir_usage {
> + USE_SUBDOMAIN_HOMEDIR,
> + DO_NOT_USE_SUBDOMAIN_HOMEDIR
> +};
> +
> +const char *safe_homedir_override(TALLOC_CTX *mem_ctx,
> + const char* homedir,
> + struct sss_domain_info *dom,
> + const char *fqname,
> + uint32_t uid,
> + enum subdomain_homedir_usage sh_use);
> +
> #endif
> --
> 1.8.4.2
>
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Thu, 3 Apr 2014 12:00:22 +0100
> Subject: [PATCH 06/12] ldap: add homedir overrides
>
> Overrides are no longer done by NSS. Instead overrides are performed by
> provider and stored into sysdb.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2205
> ---
> src/providers/ldap/sdap_async_users.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/providers/ldap/sdap_async_users.c
b/src/providers/ldap/sdap_async_users.c
> index
144092cb7aafbb2572d076e7475e26d3627b05c5..3ac58f8947edb6d581eea4c1ee9219fa7fd2468e 100644
> --- a/src/providers/ldap/sdap_async_users.c
> +++ b/src/providers/ldap/sdap_async_users.c
> @@ -482,6 +482,9 @@ int sdap_save_user(TALLOC_CTX *memctx,
> goto done;
> }
>
> + homedir = safe_homedir_override(memctx, homedir, dom, user_name, uid,
> + DO_NOT_USE_SUBDOMAIN_HOMEDIR);
> +
I know we discussed the issue several times already during 1.11
bugfixing, but what was the reason not to use subdomain_homedir for
users from AD sub domains?
Well, I think that sdap_save_user() isn't used only for users from AD
sub domains. But subdomain_homedir is supported only for domain in IPA
AD trust. (man sssd.conf: Use this homedir as default value for all
subdomains within this domain in IPA AD trust.)
Could we simplify the code to use sss_domain_info->parent as indication
on whether to use the subdomain expansion or not?
I'm sorry I don't follow...
> DEBUG(SSSDBG_TRACE_FUNC, "Storing info for user %s\n", user_name);
>
> ret = sysdb_store_user(dom, user_name, pwd, uid, gid,
> --
> 1.8.4.2
>
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Thu, 3 Apr 2014 12:04:38 +0100
> Subject: [PATCH 07/12] ipa: add homedir overides
>
> Overrides are no longer done by NSS. Instead overrides are performed by
> provider and stored into sysdb.
>
> Fixes expansion of '%o' (original home directory) in override_homedir. It
was
> wrongly evaluated as value of subdomain_homedir (in case of AD user queried on
> IPA client).
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2205
Looks good to me taking the comments about safe_homedir_override() into
account.
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Thu, 3 Apr 2014 12:49:52 +0100
> Subject: [PATCH 08/12] proxy: use no-NULL talloc context
>
> Modification to function 'save_user' to use no NULL talloc context which is
> prerequisite for the following patch.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2205
Visual ACK
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Thu, 3 Apr 2014 13:00:09 +0100
> Subject: [PATCH 09/12] proxy: add homedir overrides
>
> Overrides are no longer done by NSS. Instead overrides are performed by
> provider and stored into sysdb.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2205
Looks good to me taking the comments about safe_homedir_override() into
account.
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Thu, 3 Apr 2014 16:26:39 +0100
> Subject: [PATCH 10/12] PAC: add homedir overrides
Looks good to me taking the comments about safe_homedir_override() into
account.
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Fri, 4 Apr 2014 14:44:56 +0100
> Subject: [PATCH 11/12] ipa: add homedir overides
>
> Added expansions for OVERRIDE_HOMEDIR and FALLBACK_HOMEDIR when SSSD is run in
> server mode.
Ah, I see where the lc_homedir is coming from now..
But then I wonder if the lowercasing should be done on the side of the
caller, in ipa_subdomains_id.c since it's the only place where we need
to lowercase explicitly?
Well, I guess you are right, I'll fix this issue relevantly to the
discussion about patch 5.
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Fri, 4 Apr 2014 14:46:59 +0100
> Subject: [PATCH 12/12] utils: function expand_homedir_template made static
>
> expand_homedir_template made static.
> Parameter origin renamed to origin_homedir.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2205
Looks good to me.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel