Include specific UID and GID checks in modules.file.check_perms

[rhodesn]

If the same username exists in more than one nsswitch database, it's possible to end up in a situation where users can be associated with more than one UID.

What does this PR do?

The PR updates the file.check_perms function to check whether the UID and GID values of the file match the function arguments for user and group should they be IDs.

The PR also attempts to reduce the number of calls to get_user and get_group which result in repeat calls to stat. Instead use a single "post" file.stats call and compare user/group/mode changes using that.

What issues does this PR fix or reference?

Fixes: #62818

Previous Behavior

Files owned by a user who can be mapped to two UIDs, but have file permissions for the "wrong" UID will not be updated by file.check_perms. This is due to file.check_perms only comparing ownership by names, and not also IDs.

Under normal circumstances, if you supply an ID for user: in file.directory, and that ID either represents a non-existent user, or maps to a username via file.uid_to_user, then salt will correctly update file and directory permissions recursively. However if you end up in the unique situation where during a migration period, a username is present in two name databases but with different UIDs, salt will compare the output from stat (which returns the correct username but with the "previous" UIDs) for the file only against a username due to uid_to_user(name) being used. This results in the UID owning the file not being updated.

Example where file uid wont get updated even if a uid is supplied to file.directory:

% cat srv/salt/common.sls
passwd-cache-reset:
  file.absent:
    - name: /etc/passwd.cache

nsswitch-reset:
  file.line:
    - name: /etc/nsswitch.conf
    - mode: replace
    - content: "passwd:         files"
    - match: "^passwd:"

dir-absent:
  file.absent:
    - name: /test

testing-create:
  user.present:
    - name: testing
    - uid: 2000
    - usergroup: true

testfile:
  file.managed:
    - name: /test/testfile
    - makedirs: true
    - user: testing
    - group: testing
    - replace: false
    - require:
      - file: dir-absent
      - user: testing-create

libnss-cache:
  pkg.installed

nsswitch-update:
  file.line:
    - name: /etc/nsswitch.conf
    - mode: replace
    - content: "passwd:         cache files"
    - match: "^passwd:"

/etc/passwd.cache:
  file.managed:
    - contents: |
        testing:x:8000:2000::/home/testing:/bin/sh

file_stats_pre:
  module.run:
    - file.stats:
      - path: /test/testfile
    - require:
      - file: /etc/passwd.cache

dir-by-user-new-id:
  file.directory:
    - name: /test
    - user: 8000
    - recurse:
      - user
    - require:
      - file: /etc/passwd.cache

user_info_post:
  module.run:
    - user.info:
      - name: testing
    - require:
      - file: /etc/passwd.cache

file_stats_post:
  module.run:
    - file.stats:
      - path: /test/testfile
    - require:
      - file: testfile


# salt-call --version
salt-call 3006.0 (Sulfur)

# salt-call state.apply common;
local:
----------
          ID: passwd-cache-reset
    Function: file.absent
        Name: /etc/passwd.cache
      Result: True
     Comment: Removed file /etc/passwd.cache
     Started: 09:32:01.306346
    Duration: 3.933 ms
     Changes:
              ----------
              removed:
                  /etc/passwd.cache
----------
          ID: nsswitch-reset
    Function: file.line
        Name: /etc/nsswitch.conf
      Result: True
     Comment: Changes were made
     Started: 09:32:01.310382
    Duration: 2.674 ms
     Changes:
              ----------
              diff:
                  ---
                  +++
                  @@ -1,4 +1,4 @@
                  -passwd:         cache files
                  +passwd:         files
                   group:          files
                   shadow:         files
                   gshadow:        files
----------
          ID: dir-absent
    Function: file.absent
        Name: /test
      Result: True
     Comment: Removed directory /test
     Started: 09:32:01.313207
    Duration: 0.577 ms
     Changes:
              ----------
              removed:
                  /test
----------
          ID: testing-create
    Function: user.present
        Name: testing
      Result: True
     Comment: User testing is present and up to date
     Started: 09:32:01.314274
    Duration: 11.486 ms
     Changes:
----------
          ID: testfile
    Function: file.managed
        Name: /test/testfile
      Result: True
     Comment: Empty file
     Started: 09:32:01.326149
    Duration: 1.756 ms
     Changes:
              ----------
              group:
                  testing
              new:
                  file /test/testfile created
              user:
                  testing
----------
          ID: libnss-cache
    Function: pkg.installed
      Result: True
     Comment: All specified packages are already installed
     Started: 09:32:02.181527
    Duration: 23.372 ms
     Changes:
----------
          ID: nsswitch-update
    Function: file.line
        Name: /etc/nsswitch.conf
      Result: True
     Comment: Changes were made
     Started: 09:32:02.205086
    Duration: 2.051 ms
     Changes:
              ----------
              diff:
                  ---
                  +++
                  @@ -1,4 +1,4 @@
                  -passwd:         files
                  +passwd:         cache files
                   group:          files
                   shadow:         files
                   gshadow:        files
----------
          ID: /etc/passwd.cache
    Function: file.managed
      Result: True
     Comment: File /etc/passwd.cache updated
     Started: 09:32:02.207244
    Duration: 3.282 ms
     Changes:
              ----------
              diff:
                  New file
----------
          ID: file_stats_pre
    Function: module.run
      Result: True
     Comment: file.stats: Success
     Started: 09:32:02.212124
    Duration: 0.823 ms
     Changes:
              ----------
              file.stats:
                  ----------
                  atime:
                      1664616721.326989
                  ctime:
                      1664616721.326989
                  gid:
                      2000
                  group:
                      testing
                  inode:
                      1318636
                  mode:
                      0644
                  mtime:
                      1664616721.326989
                  size:
                      0
                  target:
                      /test/testfile
                  type:
                      file
                  uid:
                      2000
                  user:
                      testing
----------
          ID: dir-by-user-new-id              <---- note changes are returned
    Function: file.directory
        Name: /test
      Result: True
     Comment: Directory /test updated
     Started: 09:32:02.213108
    Duration: 1.702 ms
     Changes:
              ----------
              /test:
                  ----------
                  user:
                      8000
              /test/testfile:
                  ----------
                  user:
                      8000
----------
          ID: user_info_post
    Function: module.run
      Result: True
     Comment: user.info: Success
     Started: 09:32:02.214970
    Duration: 0.928 ms
     Changes:
              ----------
              user.info:
                  ----------
                  fullname:
                  gid:
                      2000
                  groups:
                      - testing
                  home:
                      /home/testing
                  homephone:
                  name:
                      testing
                  other:
                  passwd:
                      x
                  roomnumber:
                  shell:
                      /bin/sh
                  uid:
                      8000
                  workphone:
----------
          ID: file_stats_post
    Function: module.run
      Result: True
     Comment: file.stats: Success
     Started: 09:32:02.216141
    Duration: 0.717 ms
     Changes:
              ----------
              file.stats:
                  ----------
                  atime:
                      1664616721.326989
                  ctime:
                      1664616721.326989
                  gid:
                      2000
                  group:
                      testing
                  inode:
                      1318636
                  mode:
                      0644
                  mtime:
                      1664616721.326989
                  size:
                      0
                  target:
                      /test/testfile
                  type:
                      file
                  uid:                      <---- but not actually implemented
                      2000
                  user:
                      testing

Summary for local
-------------
Succeeded: 12 (changed=10)
Failed:     0
-------------
Total states run:     12
Total run time:   53.301 ms

When using:

dir-by-user-new-id:
  file.directory:
    - name: /test
    - user: testing                  <---- username this time
    - recurse:
      - user
    - require:
      - file: /etc/passwd.cache

The file.directory state returns changes: {}, which makes sense as the usernames match even though the uid doesn't.

Previously file.check_perms attempted to retrieve usernames via file.uid_to_user. An empty check was done on the return value which would signify a username not being present on the system - a hard failure as os.chown takes IDs, but no username means no ID. The PR changes this so that username checks are left to the file.chown function. The error string returned by file.chown is appended to ret["comments"] so it can be returned to the user should either the username or group name not be present.

New Behavior

Salt will check both file username and UID permissions against the supplied user and group arguments instead of trying to convert IDs to names and then comparing. This resolves an admittedly pretty unique situation.

I should add that I looked into making salt capable of accepting a username, and performing both name and ID checks using that, but those changes are more significant.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[rhodesn]

The mode refactor bit me here - gradually fixing CI failures.

[Ch3LL]

This will require a changelog file

[garethgreenaway]

@nrhodes91 Just making sure you saw @Ch3LL's comment about needing a changelog on this one. Thanks!

[rhodesn]

Created issue #62818 in keeping with the preferred changelog format.

[rhodesn]

Added changelog/62818.fixed - went with fixed as I think that matches best. Let me know if it should be changed 👍

[welcome]

Congratulations on your first PR being merged! 🎉