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.
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.
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.
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?
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.
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 :)
+ 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?
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.
+ 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).
+
+ 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.
+ }
+ 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?
Could we simplify the code to use sss_domain_info->parent as indication
on whether to use the subdomain expansion or not?
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?
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.