feat(ordering): optionally append includefiles to main config

noelmcloughlin commented 3 years ago

PR progress checklist (to be filled in by reviewers)

[x] Changes to documentation are appropriate (or tick if not required)
[x] 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
[x] [feat] A new feature
[x] [fix] A bug fix
[ ] [perf] A code change that improves performance
[x] [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
[x] [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

This PR covers the scenario where included files are not being applied because the main config (i.e. /etc/sudoers) has entries inserted after the "#includedir /etc/sudoers.d" line, changing the expected outcome of adding our "included files":

#includedir /etc/sudoers.d
# line someone appended that matched, skipping our include file 1
# line someone appended that matched, skipping our include file 2
 # line someone appended that matched, skipping our include file 3

The sudoers man page says the last match wins. To guarantee the matching of our "included files" this PR is an option to append these to the end of /etc/sudoers and add CI tests to confirm changes are applied:

#includedir /etc/sudoers.d
# line someone appended that matched, skipping our include file 1
# line someone appended that matched, skipping our include file 2
# line someone appended that matched, skipping our include file 3
#include /etc/sudoers.d/extra-file1
#include /etc/sudoers.d/extra-file2
#include /etc/sudoers.d/extra-file3

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

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

Testing checklist

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

Additional context

noelmcloughlin commented 3 years ago

Selfie-merging as no reviewers/codeowners available and open 10 days

saltstack-formulas-travis commented 3 years ago

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

The release is available on GitHub release

Your semantic-release bot :package::rocket:

myii commented 3 years ago

Thanks, @noelmcloughlin.

Reviewed and standardised in 9ea8e3ddce2ebab4d5b1f286d9fceba6eb1734e7 (using https://github.com/myii/ssf-formula/pull/363):

Use the appended suite for all platforms.
Fix the Gentoo _mapdata verification files.

daks commented 3 years ago

What a big PR I would have liked to review if I had been 'ping'-ed.

I'm not sure but I have the feeling that a complete new state and a complete new test suite is overkill, but I may be wrong and it could be that it's the only solution.

myii commented 3 years ago

What a big PR I would have liked to review if I had been 'ping'-ed.

I'm not sure but I have the feeling that a complete new state and a complete new test suite is overkill, but I may be wrong and it could be that it's the only solution.

@daks If you'd like to get pinged for PRs here, would you consider being added as a code owner?

https://github.com/saltstack-formulas/sudoers-formula/blob/4570bb6bef903a2b117db5d3f83232be6b505ae4/CODEOWNERS#L8-L9

noelmcloughlin commented 3 years ago

It is actually a small PR, adding state + true/false flag and false by default. But we have extensive CI hooks so about 60 files need updates for CI to pass.

daks commented 2 years ago

@noelmcloughlin I don't call a 60 modified files PR a small one, but YMMV :) (for salt formulas at least)

I took a moment to re-read quietly:

there is no new state as I thought first, it's just a rename from sudoers/included.sls to sudoers/included/install.sls which I don't see the point but OK (maybe later other states in included could be necessary)
maybe the new suite was in fact necessary because you can't test the inclusion and non-inclusion in one single suite
and about all those files which makes a 60+-modified files PR... OK in fact a lot of them is the _mapdata verification step, not useful for the feature itself but it's verification

In reality seems quite fine :)

daks commented 2 years ago

I finally understand the purpose of this PR.

Standard sudo configuration is to have #includedir /etc/sudoers.d at the end of /etc/sudoers file and then rely on correct ordering for included infiles present in /etc/sudoers, like for example using 01, 02 prefixes for filenames. (see https://unix.stackexchange.com/questions/423294/are-the-files-in-etc-sudoers-d-read-in-a-particular-order)

What you added is the possibility to specify the include order directly in main file, without managing this file.

In my point of view, if you manage the data provided to the formula to create the included files (with pillar or anything else) you also can manage the name you give to those files, and you can order them. Without this new feature.

With all respect @noelmcloughlin , I am doubtful about the usefulness of this feature. Could you give us more details?

noelmcloughlin commented 2 years ago

With all respect @noelmcloughlin , I am doubtful about the usefulness of this feature. Could you give us more details?

I wanted to append config to a system deployed by someone else with a /etc/sudoers file that was unconventional & not best practice. My additions to /etc/sudoers.d were blocked by lines appended to bottom of /etc/sudoers during system provisioning and this PR was only solution to force my addition to be applied last. If I controlled /etc/sudoers I would not have the problem and PR would not be useful, but I did not control that file (and did not want to). The feature is not useful for anyone else probably so is off by default.

daks commented 2 years ago

OK, but couldn't you just change the filenames deployed to /etc/sudoers.d? This is the way to do it not modifying /etc/sudoers (that's what I think at least)

I think I understand your problem and I see it's really specific. I'm not really sure all this code (code/tests/doc/test suite) which has to be maintained was worth to be added for this very very specific setup.

This now has been merged and you need it, so let it stay here.

noelmcloughlin commented 2 years ago

Its not about filenames. Adding files to /etc/sudoers.d is only guaranteed to work in two situations:

The line #include /etc/sudoers.d has precedence in /etc/sudoers file (last entry) OR
All lines with precedence over #include /etc/sudoers.d do not mask /etc/sudoers.d configuration.

If both statements are false, this PR is only solution.

daks commented 2 years ago

I'm not sure I understand, but on every Linux distribution I saw default /etc/sudoers has the line #include /etc/sudoers.d at the end of the file, so you can always override any values in this file with a file in /etc/sudoers.d. And in this directory, it's when the filenames are important to evaluate them.

daks commented 2 years ago

What I mean is: if you have no control over /etc/sudoers main file but you have control over /etc/sudoers.d files, enforcing that the line #includedir /etc/sudoers.d is at the end of the main file is sufficient, no need to enforce each one of the files. (if you choose carefully the filenames in this directory of course).

noelmcloughlin commented 2 years ago

I have no control over /etc/sudoers and there is no guarantee #includedir /etc/sudoers.d will ever appear at the end of that file. On vanilla linux yes. On a security hardened system, that line is rarely the last entry in the main file. The PR is necessary when those additional entries mask /etc/sudoers.d feature and block my configuration, perhaps unintentionally.

On Mon, Sep 20, 2021 at 3:21 PM Éric Veiras Galisson < @.***> wrote:

What I mean is: if you have no control over /etc/sudoers main file but you have control over /etc/sudoers.d files, enforcing that the line #includedir /etc/sudoers.d is at the end of the main file is sufficient, no need to enforce each one of the files. (if you choose carefully the filenames in this directory of course).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/saltstack-formulas/sudoers-formula/pull/78#issuecomment-922974274, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFUUQTRYCUZKP2SAZDEKUTUC47NBANCNFSM5CNAVPIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

saltstack-formulas / sudoers-formula