URL: https://github.com/SSSD/sssd/pull/703 Author: thalman Title: #703: nss: sssd returns '/' for emtpy home directories Action: opened
PR body: """ For empty home directory in passwd file sssd returns "/". Sssd should respect system behaviour and return the same as nsswitch "files" module - return empty string.
Resolves: https://pagure.io/SSSD/sssd/issue/ To be defined """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/703/head:pr703 git checkout pr703
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
simo5 commented: """ This should probably not be specific to the files provider. An empty home directory is a valid value and should be returned as is if the source database specifies an empty string. """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-443758006
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
thalman commented: """
This should probably not be specific to the files provider. An empty home directory is a valid value and should be returned as is if the source database specifies an empty string.
That's true but files provider is quite different from the others. Other providers have homedir fallback templates and other features around homedir. So we (I) want to deal with them separately. Probably in next PR
"""
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-443766083
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
thalman commented: """
This should probably not be specific to the files provider. An empty home directory is a valid value and should be returned as is if the source database specifies an empty string.
That's true but files provider is quite different from the others. Other providers have homedir fallback templates and other features around homedir. So we (I) want to deal with them separately. Probably in next PR
"""
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-443766083
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
simo5 commented: """ @thalman those other cases are already handled in the call right above your change. So if those are handled homdir will arelady be "not null".
I think all you need to do is return ""; unconditionally when homedir is NULL.
Later if you need additional handling you can do that, but the point is that there is no good reason to do the "right thing" only for files. """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-443779774
URL: https://github.com/SSSD/sssd/pull/703 Author: thalman Title: #703: nss: sssd returns '/' for emtpy home directories Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/703/head:pr703 git checkout pr703
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
thalman commented: """ @simo5 I thought about it more and I get to the same conclusion, PR updated """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-444059683
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """ You also need to amend `test_user_no_dir` in `src/tests/intg/test_files_provider.py` """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-444081640
URL: https://github.com/SSSD/sssd/pull/703 Author: thalman Title: #703: nss: sssd returns '/' for emtpy home directories Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/703/head:pr703 git checkout pr703
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """ Thanks, this passes the test. And of course the patch is correct, but after some more testing, I wonder if we should at least for one release default to fallback_homedir=$something at least for the AD provider. Because now with the completely minimal AD provider configuration (no POSIX attrs, ID mapping only) I can't log in with an AD user: ``` $ getent passwd administrator@win.trust.test administrator@win.trust.test:*:215000500:215000513:Administrator::/bin/bash $ su - administrator@win.trust.test su: user administrator@win.trust.test does not exist ``` Note that this is minimal config, realmd already adds fallback_homedir.
Or at least we should IMO add some backwards compatible handling when this patch makes it to fedora or RHEL otherwise admins might not be happy. From purely upstream point of view this change is probably OK with me. """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-444089136
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
simo5 commented: """ On Tue, 2018-12-04 at 04:51 -0800, Jakub Hrozek wrote:
Thanks, this passes the test. And of course the patch is correct, but after some more testing, I wonder if we should at least for one release default to fallback_homedir=$something at least for the AD provider. Because now with the completely minimal AD provider configuration (no POSIX attrs, ID mapping only) I can't log in with an AD user:
$ getent passwd administrator@win.trust.test administrator@win.trust.test:*:215000500:215000513:Administrator::/bi n/bash $ su - administrator@win.trust.test su: user administrator@win.trust.test does not existNote that this is minimal config, realmd already adds fallback_homedir.
Why this fails? Because of the missing homedir ?
Or at least we should IMO add some backwards compatible handling when this patch makes it to fedora or RHEL otherwise admins might not be happy. From purely upstream point of view this change is probably OK with me.
I think the AD provider should synthetize an home dir by default, without any specific option being set, it's what is considered normal also in winbind land, in fact I would look closely at what winbind does and do the same for AD users by default.
If fallback_homedir is set, skip the default and use what that setting specifies.
Simo.
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """
On Tue, 2018-12-04 at 04:51 -0800, Jakub Hrozek wrote: Thanks, this passes the test. And of course the patch is correct, but after some more testing, I wonder if we should at least for one release default to fallback_homedir=$something at least for the AD provider. Because now with the completely minimal AD provider configuration (no POSIX attrs, ID mapping only) I can't log in with an AD user: ``` $ getent passwd ***@***.*** ***@***.***:*:215000500:215000513:Administrator::/bi n/bash $ su - ***@***.*** su: user ***@***.*** does not exist ``` Note that this is minimal config, realmd already adds fallback_homedir. Why this fails? Because of the missing homedir ?
Yes, su checks the homedir: ``` »·······su->pwd = xgetpwnam(su->new_user, &su->pwdbuf); »·······if (!su->pwd »······· || !su->pwd->pw_passwd »······· || !su->pwd->pw_name || !*su->pwd->pw_name »······· || !su->pwd->pw_dir || !*su->pwd->pw_dir) »·······»·······errx(EXIT_FAILURE, _("user %s does not exist"), su->new_user) ``` ssh is more permissive and places you at `/`
Or at least we should IMO add some backwards compatible handling when this patch makes it to fedora or RHEL otherwise admins might not be happy. From purely upstream point of view this change is probably OK with me. I think the AD provider should synthetize an home dir by default, without any specific option being set, it's what is considered normal also in winbind land, in fact I would look closely at what winbind does and do the same for AD users by default. If fallback_homedir is set, skip the default and use what that setting specifies.
Then why not set a default value for fallback homedir? :-)
"""
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-444106317
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
thalman commented: """
Or at least we should IMO add some backwards compatible handling when this patch makes it to fedora or RHEL otherwise admins might not be happy. From purely upstream point of view this change is probably OK with me. I think the AD provider should synthetize an home dir by default, without any specific option being set, it's what is considered normal also in winbind land, in fact I would look closely at what winbind does and do the same for AD users by default. If fallback_homedir is set, skip the default and use what that setting specifies.
Then why not set a default value for fallback homedir? :-)
I see only one catch here - once we create not null default (for example /home/%u), how can an admin unset this to empty string and get the "original /etc/passwd" behaviour? And do we care about such use case?
"""
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-444112129
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """ I thought that fallback_homedir = "" would work but it doesn't, not even with escaping quotes. An empty attribute is silently ignored.
About whether we care about this use-case..I don't know, currently I don't think so. """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-444114458
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
simo5 commented: """ On Tue, 2018-12-04 at 05:51 -0800, Jakub Hrozek wrote:
Then why not set a default value for fallback homedir? :-)
Because that would override an empty home dir in all cases, and this is not ok. An empty home dir is a perfectly valid and intentional value in some entries.
It may even lead to security issues to suddenly replace an empty dir with something else, for example if someone is counting on the empty dir to make "su" fail.
Simo.
URL: https://github.com/SSSD/sssd/pull/703 Author: thalman Title: #703: nss: sssd returns '/' for emtpy home directories Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/703/head:pr703 git checkout pr703
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """ I also had a follow-up discussion with simo on IRC, let me paste rephrasing: - the AD provider should have an AD specific internal option that generates the homedir. This option doesn't have to be exposed as a generic config option to avoid having yet another configuration knob - if fallback_homedir is set, this option is ignored - the option should be ideally set to what winbind uses
I hope I haven't forgotten or mangled anything. """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-444132698
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
thalman commented: """ there is now one extra commit with default for ad, just to be on the same page """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-444133326
URL: https://github.com/SSSD/sssd/pull/703 Author: thalman Title: #703: nss: sssd returns '/' for emtpy home directories Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/703/head:pr703 git checkout pr703
URL: https://github.com/SSSD/sssd/pull/703 Author: thalman Title: #703: nss: sssd returns '/' for emtpy home directories Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/703/head:pr703 git checkout pr703
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """ Seems to work fine, by default I get /home/domain/username for all admins, when I set fallback_homedir=%o then the unixHomeDirectory attribute is used instead. """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-446140500
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
simo5 commented: """ Wait, does this mean it changes current behavior for AD domains ? """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-446198589
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
simo5 commented: """ Or would it previously return "/" unconditionally ? """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-446198697
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """ The patch does change the behaviour, but it's also just a fallback, so whatever you had defined in AD LDAP is still used.
Let me give an example: - before the patch: - user with no homedir: "/" - user with homedir: the homedir is used - after the patch: - user with no homedir: /home/domain/username - user with homedir: the homedir is used """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-446200551
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
simo5 commented: """ Thank @jhrozek this clears it! """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-446204376
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """ * master: 90f32399b4100ce39cf665649fde82d215e5eb49 """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-446378006
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
jhrozek commented: """ * sssd-1-16: 28792523a01a7d21bcc8931794164f253e691a68 """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-446378330
URL: https://github.com/SSSD/sssd/pull/703 Author: thalman Title: #703: nss: sssd returns '/' for emtpy home directories Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/703/head:pr703 git checkout pr703
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/703 Title: #703: nss: sssd returns '/' for emtpy home directories
pedrohc commented: """ CVE-2019-3811 was assigned to this issue. """
See the full comment at https://github.com/SSSD/sssd/pull/703#issuecomment-453608710
sssd-devel@lists.fedorahosted.org
-
jhrozek -
pedrohc
-
simo5
-
thalman