-
Notifications
You must be signed in to change notification settings - Fork 794
File owners, groupowners, permissions should be able to recurse and file_regex #8404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
ggbecker
merged 12 commits into
ComplianceAsCode:stabilization-v0.1.61
from
yuumasato:file_owners_permissions_recurse_and_symlink
Mar 24, 2022
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
a896175
Prevent breaking file names if they have spaces
yuumasato 5af298b
Add test to check file permissions within dirs
yuumasato 21ef824
Improve template checks to recurse and regex file names
yuumasato bdc5989
Fix the ownership of the symlink
yuumasato 0e837f6
Bash: Only change ownership of incompliant files
yuumasato 4c71f7e
Ansible: Make file_regex and recurse independent in template
yuumasato f3d94f8
Ansible: Only change files when they are incompliant
yuumasato 39d19f7
Bash: Make file_regex and recurse independent
yuumasato c8de6f2
Document file only and directory only behavior
yuumasato 141f72c
Make sure that path pattern_match is achored
yuumasato 0e235b9
Update test to reflect current rule behaviour
yuumasato 98c2821
The mode should be interpreted as string
yuumasato File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,5 +69,6 @@ template: | |
| - /lib64/ | ||
| - /usr/lib/ | ||
| - /usr/lib64/ | ||
| recursive: 'true' | ||
| file_regex: ^.*$ | ||
| fileuid: '0' | ||
9 changes: 9 additions & 0 deletions
9
...ithin_important_dirs/file_ownership_library_dirs/tests/incorrect_owner_within_dir.fail.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # platform = multi_platform_sle,multi_platform_rhel,multi_platform_fedora,multi_platform_ubuntu | ||
|
|
||
| useradd user_test | ||
|
|
||
| TESTDIR="/usr/lib/dir/" | ||
|
|
||
| mkdir $TESTDIR | ||
| touch $TESTDIR/test_me | ||
| chown user_test $TESTDIR/test_me |
16 changes: 16 additions & 0 deletions
16
...issions_within_important_dirs/file_ownership_library_dirs/tests/incorrect_symlink.fail.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # platform = multi_platform_sle,multi_platform_rhel,multi_platform_fedora,multi_platform_ubuntu | ||
|
|
||
| useradd user_test | ||
|
|
||
| TESTDIR="/usr/lib/" | ||
|
|
||
| # The remediation performs a 'find' followed by a 'chwon' | ||
| # While 'find' doesn't follow symlinks by default, 'chown' does follow, | ||
| # so 'chown' will try to change owner of a non existent file while 'find' | ||
| # pointed out that the symlink has wrong owner. | ||
| ln -s $TESTDIR/mising_test_file $TESTDIR/faulty_symlink | ||
| chown -h user_test $TESTDIR/faulty_symlink | ||
|
|
||
| # The Check ignores symlink, so we need to put a reason to run the remediations | ||
| touch $TESTDIR/test_me | ||
| chown user_test $TESTDIR/test_me |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,5 +70,6 @@ template: | |
| - /lib64/ | ||
| - /usr/lib/ | ||
| - /usr/lib64/ | ||
| recursive: 'true' | ||
| file_regex: ^.*$ | ||
| filemode: '0755' | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really needed or is it an issue related to PR #8194 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is necessary, the check is not going down the directory tree.
Check this test scenario: https://github.com/ComplianceAsCode/content/blob/d5a23b41fad45249f125258225737f96300113fb/linux_os/guide/system/permissions/files/permissions_within_important_dirs/file_ownership_library_dirs/tests/incorrect_owner_within_dir.fail.sh
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are looking at the same problem, but proposed different solutions, that, at the end, depend on rule interpretation.
When I read the rule description, I interpret that it should go down into all libraries and check files in libraries' directories too, not just the first level.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I'm confused (I'll be honest, it's so many rules touching permissions that I have to re-read it all the time), but this rule specifically has the file_regex, shouldn't it be working recursively as you added to the documentation?
I ran a test and can see that the bash (not sure on ansible though) is correctly going through all the files. Isn't it just a matter of fixing the oval?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, my addition in the documentation is about what will be checked, directories vs files. It doesn't address recursion.
a. If
filepathis a directory andfile_regexis not specified, the rule will check and remediate the first level of directories in thefilepath.b. If
filepathis a directory andfile_regexis specified, the rule will check and remediate files in thefilepath.If along with case
a., recursion is set, the rule will check directories while recursing down.If along with case
b., recursion is set, the rule will check files while recursing down.Yes, the remediation is going down the directory tree, but should it happen when I didn't set
recursive?Yes, the OVAL was the part that initially needed fixing, but then I addressed inconsistencies between what the OVAL does and the remediation does.
I think if the check is recursive, the remediation should be too. But if the OVAL isn't recursive, the remediation shouldn't be.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, my point is that with the mentioned commit id, it started to filter out symlinks in the oval. Therefore, if
/libis a symlink it won't be checked. If it won't be checked than the number of files evaluated is 0 for that test, thus evaluating tofalseand the whole rule will fail as the oval is set toall_exist. By passingmissing_file_passin the rule, then it changes fromall_existtoany_existand allow the rule to pass. It is a quick fix.I don't really remember if most/any of the permissions/ownership/group ownership rules really mention about symlinks. If oval is set to exclude symlinks (as it is right now), then I don't see the point (and feel free to correct me here) of passing -h in the remediation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I could test with changes in this PR (using
--oval-results), in RHEL, even when/libis a symlink the scanner was able to collect objects.So actually, could it be that the scanner follows symlinks, but doesn't report them as collected objects, 🤔 ?
Attached is the oval-results file:
rhel7-oval-results.zip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think symlinks are mentioned anywhere.
I see, it is strange to change the owner of the symlink when the check ignores them.
The intent of the change was to avoid dereferencing erors when the symlink is broken. (I made that commit when I was still not aware of the symlink mess)
With
-hit means thatchownwill not affect the file pointed to, but as the thefindcommand doesn't follow symlinks by default (and-Lis not set), the file pointed to by the symlink wouldn't be changed anyway...but now we avoid dereferencing errors!And I see inconsistency here too.
To better align with the check the remediation should filter out symlinks before doing
chown... But then we are not sure if we actually want to filter out the symlinks...I'm aware that bdc5989 doesn't fix the symlinking situation, it just avoids errors in
chwoncommand with the current behaviour.This PR was to improve inconsistency with
recurseandfile_regexand I stumbled uponsymlinkandchwoninconsistency, 😂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just checked here my test from yesterday on master,
so
/liband/lib64are both symlinks to/usr/liband/usr/lib64respectively.One of the rules that I see failing
file_ownership_library_dirswill fail for/lib64as the only file inside it is a .so that is a symlink to a.sounder/usr/lib64.It doesn't fail for
/libas it "finds" some files there, which are actually files under/usr/lib.It is tricky and I need to dig deeper on those rules, just not having the time lately.
I have to test with your changes, it might be that all this recursive and changes might fix some of the weirdness I saw in the failing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is tricky. I had to take some time to wrap my head around what the rules do.
Let us know how the tests on your side go.