[FEATURE] Warn when change of home-dir for existing user would fail

[mirko]

Description
Within a working sls file for managing users and their home dirs I'm changing the path to a user's homedir to another one.
This fails with:

          ID: user1
    Function: user.present
      Result: False
     Comment: These values could not be changed: {'home': '/tmp/user1'}
     Started: 22:55:58.740104
    Duration: 19.577 ms
     Changes:

Setup

Steps to Reproduce the behavior

Managing a user:

user1:
  user.present:
    - uid: 501
    - gid: 501
    - password: !
    - home: /home/user1
    - shell: /bin/sh
    - require:
      - group: user1
  group.present:
    - gid: 501

Execute sls on host, everything is fine.
Then changing the user's homedir, though, e.g. by changing the whole section from above to (ensuring /tmp/user1 exists and is owned by 'user1'):

user1:
  user.present:
    - uid: 501
    - gid: 501
    - password: !
    - home: /tmp/user1 # <<<<----------------
    - shell: /bin/sh
    - require:
      - group: user1
  group.present:
    - gid: 501

fails with:

          ID: user1
    Function: user.present
      Result: False
     Comment: These values could not be changed: {'home': '/tmp/user1'}
     Started: 23:06:12.741230
    Duration: 17.295 ms
     Changes:   

Expected behavior
Change of home dir successful.

Versions Report

salt --versions-report

Salt Version:
          Salt: 3002.2
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: 2.7.3
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 2.10
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 0.5.6
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: 2.6.1
  pycryptodome: 3.6.1
        pygit2: Not Installed
        Python: 3.7.3 (default, Jul 25 2020, 13:03:44)
  python-gnupg: Not Installed
        PyYAML: 3.13
         PyZMQ: 17.1.2
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.1
 
System Versions:
          dist: debian 10 buster
        locale: UTF-8
       machine: x86_64
       release: 4.19.0-14-amd64
        system: Linux
       version: Debian GNU/Linux 10 buster

Additional context
Even though it's considered expected behaviour, I'd really fancy an override as it was implemented for uid/gid changes (allow_uid_change=True).

[danielrobbins]

I believe the exact functionality you want already exists. Specify "createhome: true" to enable the creation of a new home directory. This setting should allow the home directory to be changed. Without this setting specified, the home directory is immutable.

Please try this and let me know if it resolves the issue.

[mirko]

Hello, thanks for the reply!

createhome: True is exactly what I do not want, as in my usecases either
(a) the new homedir already exists
or
(b) the new homedir is non-existent (literally '/nonexistent', which is a common value at least on Debian/Ubuntu as part of /etc/passwd for users not having a home dir)

[danielrobbins]

@mirko Just for documentation purposes of this issue, based on your original issue description, where you are wanting to change a user's existing home directory to another new home directory, and you want it to be created, createhome: true will do what you want. (I think in your case (a) -- it will still do what you want.)

The problem is, as you point out in case (b), there is no way to change a user's home directory to a value without also telling salt to create it. I will investigate the most elegant way to implement the functionality you want.

[danielrobbins]

@mirko here is what I am thinking. user.present is really geared toward setting up legitimate user accounts, with real home directories. It was designed with the assumption that this is the case. That is why createhome works this way. Then we have user.absent for removing a user account. But we are missing functionality for disabling user accounts, without removing them from the system. I am concerned that adding more functionality to user.present may make things more complicated than it needs to be -- so why not add another state for this purpose?

So I am thinking of the possibility of creating a user.disable state that is specifically geared towards making a 'regular' user account into a disabled account, or creating a disabled account if it doesn't exist. It would be designed to have the default shell set to /sbin/nologin and other features like setting the home directory to a non-existent directory. There could be other capabilities like zapping the /etc/shadow password entry and other things you may want to do for a disabled user account, following best practices in this area. So more useful functionality could be added in the future that is specific to disabling an account.

This, to me, seems like a better approach than tweaking user.present to deal with disabled accounts.

We would then have the following states:

user.present - geared towards creating legitimate user accounts, or moving a user account from disabled to active.
user.absent - geared towards ensuring user accounts don't exist.
user.disable - geared towards creating disabled user accounts, or moving a user account from active to disabled.

Let me know your thoughts.

[mirko]

@danielfrg I'm so sorry I didn't get back to you and this earlier - thanks for all the effort at this point!

@mirko here is what I am thinking. user.present is really geared toward setting up legitimate user accounts, with real home directories. It was designed with the assumption that this is the case. That is why createhome works this way. Then we have user.absent for removing a user account. But we are missing functionality for disabling user accounts, without removing them from the system. I am concerned that adding more functionality to user.present may make things more complicated than it needs to be

I totally follow and understand the thoughts and concerns here. However I rather to tend to also consider users with non-existent home directories as "valid" and "enabled" users. They come with the biggest linux distributions - even in minimal config - pre-configured. They're valid and enabled users to me and it appears to be valid and common to configure them that way - just without an existing home directory.

So I am thinking of the possibility of creating a user.disable state that is specifically geared towards making a 'regular' user account into a disabled account, or creating a disabled account if it doesn't exist. It would be designed to have the default shell set to /sbin/nologin and other features like setting the home directory to a non-existent directory.

..and hence (because of what I wrote above) I personally do not see those user accounts as invalid or disabled. They "just" have a non-existent home dir which neither makes the config invalid nor does it disable the user.

I do not feel like a user being "disabled" is the same as a user account with the /valid/ configuration of having a non-existent directory.

So while I still understand your hassle with tweaking user.present, to me it feels like being the better approach than labeling such configured users as disabled accounts. It's prone to misunderstandings and will probably lead other issues (user accounts e.g. can be indeed considered disabled, depending on whether you set a *, ! or alike as password in /etc/passwd which has nothing to do with its home dir existing or not anymore)

[sagetherage]

@mirko
Reviewed this issue in Tuesday Triage 2021-MAY-25 - very interesting! Daniel Robbins has since moved onto another team so we were simply reviewing and may have more comments.

[waynew]

Hey @mirko, there's definitely something weird happening here. As Sage
mentioned we took a look at this and all our tests behaved exactly the way you
expected it to work.

Yet, you're seeing some screwy behavior so the question is why? We were able to
manually introduce an error that would make this behavior appear 100% of the
time, but it's not clear why it would ever be failing.

But this should get us the information that we need:

log.warn("XXX --> %s %s %s %s", type(lusr['home']), repr(lusr['home']), type(home), repr(home))

If you can put that right before this line:

salt/salt/states/user.py

Line 129 in 5457218

if home and lusr["home"] != home:

If you don't already know where the file is locally, you should be able to get
access to it via:

`salt-call --local grains.get pythonexecutable --output=txt | cut -d":" -f2` -c 'import salt.states.user as u; print(u.__file__)'

That gets the python that is running salt in the form of
local: /path/to/python, which gets cut into /path/to/python. That gets
exec'd with

`...`

So ultimately it's just:

python -c 'import salt.states.user as u; print(u.__file__)'

Anyway, if you can put that logging statement in, and re-run your state
(salt-call --local state.apply yourstate on the minion is a great way to see
the logging output), then provide the output, I'm expecting that will identify
the issue - namely that lusr['home'] and home end out different. Why is a
different question, but that will at least (in)validate my theory.

Thanks!

[mirko]

Hey, thanks a lot for putting so much effort into this and coming back to me!

The good news: I'm still able to reproduce it
The "other" news: While trying to reproduce again, I narrowed it down to only a special case which I didn't see as such before and now I'm afraid of having wasted your time.

The issue only occurs, when user1 is the user I'm logging into the minion as via salt-ssh.

So, when using user1 as user salt-ssh is using and changing e.g. user2's home-dir to something non-existent, everything works as expected.

Question now is: is it still considered a bug? Even if so, probably with much lower severity, as only triggered in described edge case. Wondering, though, what's exactly (technically) preventing the logged in user from changing his/her home-dir...

Do you still need the debug output from above mentioned modifications? Happy to still provide them, though for the moment I'm assuming my findings change the situation a bit.

Now you might ask why on earth I'm trying to change salt-ssh's admin user's home-dir to /nonexistent, especially when it's common having the user's ssh-keys lying around in ~/.ssh/authroized_keys: that's exactly what I'm trying to change.

The idea is having the public keys moved out of user's home-dirs into /etc/ssh/authorized_keys/%u - incl. the admin user salt-ssh uses to log into its minions.

For reference: for moving the ssh pub keys out of the user's home-dir to disallow users to edit their authorized_keys, there's another pending ticket: #60045 (comment) as currently the user still has write access to these files which contradicts the idea of preventing users from doing so.

[piterpunk]

Hi @mirko

The issue only occurs, when user1 is the user I'm logging into the minion as via salt-ssh.

So, when using user1 as user salt-ssh is using and changing e.g. user2's home-dir to something non-existent, everything works as expected.

Question now is: is it still considered a bug? Even if so, probably with much lower severity, as only triggered in described edge case. Wondering, though, what's exactly (technically) preventing the logged in user from changing his/her home-dir...

Salt uses usermod under the hood to do the home directory change. Some user attributes can't be changed while the user is connected or if any process of this user is running. From usermod's man page:

You must make certain that the named user is not executing any processes when this command is being executed if the user's numerical user ID, the user's name, or the user's home directory is being changed. usermod checks this on Linux. On other platforms it only uses utmp to check if the user is logged in.

This is what is preventing the logged user to change his home directory. IMHO it should not be considered a bug.

[waynew]

@piterpunk thanks for weighing in here! That definitely explains it. I don't know if it makes sense for us to do some kind of check within Salt for this -- if someone opened a PR with tests, I feel like that would be a reasonable enhancement, but I don't think that's necessarily Salt(the program)'s responsibility 🤔

[Foorack]

Hello, thanks for the reply!

createhome: True is exactly what I do not want, as in my usecases either (a) the new homedir already exists or (b) the new homedir is non-existent (literally '/nonexistent', which is a common value at least on Debian/Ubuntu as part of /etc/passwd for users not having a home dir)

Ran into this today, whereby the user.present state does not allow /nonexistent as a home dir for a system account.

What could be ways forward for this? Create a flag which allows to ignore the home folder is missing? Ignore it when it is a system account?