fix: permissions on sudoers include dir were wrong

kmosher commented 3 years ago

PR progress checklist (to be filled in by reviewers)

[ ] Changes to documentation are appropriate (or tick if not required)
[ ] Changes to tests are appropriate (or tick if not required)
[ ] Reviews completed

What type of PR is this?

Primary type

[ ] [build] Changes related to the build system
[ ] [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ ] [ci] Changes to the continuous integration configuration
[ ] [feat] A new feature
[x] [fix] A bug fix
[ ] [perf] A code change that improves performance
[ ] [refactor] A code change that neither fixes a bug nor adds a feature
[ ] [revert] A change used to revert a previous commit
[ ] [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[ ] [docs] Documentation changes
[ ] [test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

Describe the changes you're proposing

sudo started throwing errors about not being able to stat files in /etc/sudoers.d after pulling the latest version. Tracked it down to a missing +x permission on the include directory that was brought under salt management in the prior commit.

Pillar / config required to test the proposed changes

N/A

Debug log showing how the proposed changes work

      ID: /etc/sudoers.d
Function: file.directory
  Result: True
 Comment: Directory /etc/sudoers.d updated
 Started: 21:15:15.532054
Duration: 4.157 ms
 Changes:   
          ----------
          /etc/sudoers.d:
              ----------
              mode:
                  0750
          mode:
              0750

Documentation checklist

[ ] Updated the README (e.g. Available states).
[ ] Updated pillar.example.

Testing checklist

[ ] Included in Kitchen (i.e. under state_top).
[ ] Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
[ ] Updated the relevant test pillar.

Additional context

jynolen commented 3 years ago

This PR should be merge as soon as possible as it could lock people out of server

myii commented 3 years ago

Thanks for providing this fix @kmosher (and appreciate the confirmation, @jynolen). One question, is 750 documented anywhere? The default permissions for that directory appear to be 755, when checking across various platforms.

@daks Regression introduced in #66. Perhaps we should merge this ASAP and then finalise whether it should be 750 or 755.

jynolen commented 3 years ago

Agreed first fix, after find a better solutions. I'm on my way to find additionals informations about sudoers file perm

jynolen commented 3 years ago

This is what i got when the folder perm is 440

sudo su -
sudo: unable to stat /etc/sudoers.d/README: Permission denied
sudo: unable to stat /etc/sudoers.d/debian: Permission denied

According to search that includes Topic https://askubuntu.com/questions/482932/sudo-unable-to-stat-etc-sudoers-d-readme-no-such-file-or-directory Review of source package (debian)

curl -L -o sudo.deb http://security.debian.org/debian-security/pool/updates/main/s/sudo/sudo_1.8.10p3-1+deb8u7_amd64.deb
dpkg -c sudo.deb | grep etc/sudo
-rw-r--r-- root/root       669 2020-02-02 00:15 ./etc/sudoers
drwxr-xr-x root/root         0 2020-02-02 00:15 ./etc/sudoers.d/
-r--r----- root/root       958 2020-02-02 00:15 ./etc/sudoers.d/README

-- Restoring default package perms

# chmod 755 /etc/sudoers.d/
# chown root:root /etc/sudoers.d/debian
# chmod 440 /etc/sudoers.d/debian
# chmod 644 /etc/sudoers

-- Trying again with debian user

# whoami
debian
# sudo su -
# whoami
root

More over here the content of sudoers.d README

# # As of Debian version 1.7.2p1-1, the default /etc/sudoers 
...
# Note that there must be at least one file in the sudoers.d directory (this one will do), and all files in this directory should be mode 0440.
...

daks commented 3 years ago

Not sure why I set it to 440 but in fact on Debian 9 or 10 it's 755 so no problem for me to merge this PR as soon as possible.

One improvement could be to add a basic test on this directory mode, here https://github.com/saltstack-formulas/sudoers-formula/blob/master/test/integration/default/controls/config.rb, with something like

  describe directory('/etc/sudoers.d/') do
    it { should be_owned_by 'root' }
    it { should be_grouped_into 'root' }
    its('mode') { should cmp '0755' }
  end

A better one (but I'm not sure I know how to run it) would be to set some NOPASSWD sudo rules and try to use it.

daks commented 3 years ago

I will merge this PR as it is, we can add the inspec test later

myii commented 3 years ago

Added #71 as a reminder for what still could/should be done.

saltstack-formulas-travis commented 3 years ago

:tada: This PR is included in version 0.23.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

saltstack-formulas / sudoers-formula