Skip to content
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

fixes in file_groupownership template #10666

Merged

Conversation

vojtapolasek
Copy link
Collaborator

@vojtapolasek vojtapolasek commented Jun 1, 2023

Description:

Please see commit messages.

Rationale:

Recent changes to the template made the OVAL check consider both GIDs and group names. But the parameter filegid was considered to be a numeric GID only in case it was equal to 0. This created a problem in case a content author tried to use a gid different than 0 - the result was incorrect, it tried to search for a group name instead of using the gid.
This PR tries to fix the behavior - if the parameter is an integer, it is considered to be a gid. If it is a string, it is considered to be a group name.
This should not be a problem since group names can't be numeric.

Also as Automatus tests have been run on many rules, I have discovered two problems in test scenarios for file_groupownership, file_owner and file_permissions templates.
The first problem is that scenarios try to use the is_directory template variable, but this variable is never stored. It is created only at runtime when templates are used to render checks and remediations. This made test scenarios not working correctly in case the the filepath parameter was a directory.
Also I found out that test scenarios did not account for the case when the filepath parameter was a directory and it did not exist on the system prior the test run.

Review Hints:

You can run automatus on the file_groupownership template in template mode:

python automatus.py template --libvirt qemu:///system ssgts_vm file_groupownership
python automatus.py template --libvirt qemu:///system ssgts_vm --remediate-using ansible file_groupownership

Or you can try for example these rules:

  • file_groupowner_var_log_syslog
  • file_groupownership_sshd_private_key
  • file_groupowner_etc_shadow

@vojtapolasek vojtapolasek added bugfix Fixes to reported bugs. Ansible Ansible remediation update. OVAL OVAL update. Related to the systems assessments. Bash Bash remediation update. labels Jun 1, 2023
@github-actions
Copy link

github-actions bot commented Jun 1, 2023

Start a new ephemeral environment with changes proposed in this pull request:

rhel8 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@Mab879 Mab879 self-assigned this Jun 1, 2023
@Mab879 Mab879 added this to the 0.1.68 milestone Jun 1, 2023
Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good. However, there is a merge commit from another PR in it. Please take look. Also, I assume you want this to go to stabilization as well.

Copy link
Member

@matejak matejak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename the filegid key to gid_or_name or something, and not hesitate to mass-rename everything in the project. Changing meaning of a variable implies that you should also rename it if you can, and here, we can do that very easily. Documenting unexpected behavior is better than nothing, but let's follow "working software over comprehensive documentation".
I would also consider enhancing the Python template part to try to convert the gid_or_name to int, and if it succeeds, indicate that we have indeed a gid.

Moreover, constructs s.a. if FILEGID | int(-1) != -1 violate our guidelines.

@vojtapolasek
Copy link
Collaborator Author

@Mab879 feedback from @matejak is reasonable. I don't think I will push for this to stabilization, I will rather propose reverting of problematic PRs which led to this situation (in stabilization).
I will rather polish out this PR and merge it later to master only.

@github-actions
Copy link

github-actions bot commented Jun 7, 2023

This datastream diff is auto generated by the check Compare DS/Generate Diff

Click here to see the full diff
bash remediation for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_audit_configuration' differs.
--- xccdf_org.ssgproject.content_rule_file_groupownership_audit_configuration
+++ xccdf_org.ssgproject.content_rule_file_groupownership_audit_configuration
@@ -1,9 +1,9 @@
 # Remediation is applicable only in certain platforms
 if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ] && rpm --quiet -q audit; then
 
-find /etc/audit/ -maxdepth 1 -type f ! -gid 0 -regex '^audit(\.rules|d\.conf)$' -exec chgrp 0 {} \;
+find /etc/audit/ -maxdepth 1 -type f ! -group 0 -regex '^audit(\.rules|d\.conf)$' -exec chgrp 0 {} \;
 
-find /etc/audit/rules.d/ -maxdepth 1 -type f ! -gid 0 -regex '^.*\.rules$' -exec chgrp 0 {} \;
+find /etc/audit/rules.d/ -maxdepth 1 -type f ! -group 0 -regex '^.*\.rules$' -exec chgrp 0 {} \;
 
 else
     >&2 echo 'Remediation is not applicable, nothing was done'

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_audit_configuration' differs.
--- xccdf_org.ssgproject.content_rule_file_groupownership_audit_configuration
+++ xccdf_org.ssgproject.content_rule_file_groupownership_audit_configuration
@@ -10,7 +10,7 @@
   - no_reboot_needed
 
 - name: Find /etc/audit/ file(s) matching ^audit(\.rules|d\.conf)$
-  command: find -H /etc/audit/ -maxdepth 1 -type f ! -gid 0 -regex "^audit(\.rules|d\.conf)$"
+  command: find -H /etc/audit/ -maxdepth 1 -type f ! -group 0 -regex "^audit(\.rules|d\.conf)$"
   register: files_found
   changed_when: false
   failed_when: false
@@ -45,7 +45,7 @@
   - no_reboot_needed
 
 - name: Find /etc/audit/rules.d/ file(s) matching ^.*\.rules$
-  command: find -H /etc/audit/rules.d/ -maxdepth 1 -type f ! -gid 0 -regex "^.*\.rules$"
+  command: find -H /etc/audit/rules.d/ -maxdepth 1 -type f ! -group 0 -regex "^.*\.rules$"
   register: files_found
   changed_when: false
   failed_when: false

bash remediation for rule 'xccdf_org.ssgproject.content_rule_root_permissions_syslibrary_files' differs.
--- xccdf_org.ssgproject.content_rule_root_permissions_syslibrary_files
+++ xccdf_org.ssgproject.content_rule_root_permissions_syslibrary_files
@@ -1,8 +1,8 @@
 
-find /lib/  -type f ! -gid 0 -regex '^.*$' -exec chgrp 0 {} \;
+find /lib/  -type f ! -group 0 -regex '^.*$' -exec chgrp 0 {} \;
 
-find /lib64/  -type f ! -gid 0 -regex '^.*$' -exec chgrp 0 {} \;
+find /lib64/  -type f ! -group 0 -regex '^.*$' -exec chgrp 0 {} \;
 
-find /usr/lib/  -type f ! -gid 0 -regex '^.*$' -exec chgrp 0 {} \;
+find /usr/lib/  -type f ! -group 0 -regex '^.*$' -exec chgrp 0 {} \;
 
-find /usr/lib64/  -type f ! -gid 0 -regex '^.*$' -exec chgrp 0 {} \;
+find /usr/lib64/  -type f ! -group 0 -regex '^.*$' -exec chgrp 0 {} \;

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_root_permissions_syslibrary_files' differs.
--- xccdf_org.ssgproject.content_rule_root_permissions_syslibrary_files
+++ xccdf_org.ssgproject.content_rule_root_permissions_syslibrary_files
@@ -1,5 +1,5 @@
 - name: Find /lib/ file(s) matching ^.*$ recursively
-  command: find -H /lib/  -type f ! -gid 0 -regex "^.*$"
+  command: find -H /lib/  -type f ! -group 0 -regex "^.*$"
   register: files_found
   changed_when: false
   failed_when: false
@@ -36,7 +36,7 @@
   - root_permissions_syslibrary_files
 
 - name: Find /lib64/ file(s) matching ^.*$ recursively
-  command: find -H /lib64/  -type f ! -gid 0 -regex "^.*$"
+  command: find -H /lib64/  -type f ! -group 0 -regex "^.*$"
   register: files_found
   changed_when: false
   failed_when: false
@@ -73,7 +73,7 @@
   - root_permissions_syslibrary_files
 
 - name: Find /usr/lib/ file(s) matching ^.*$ recursively
-  command: find -H /usr/lib/  -type f ! -gid 0 -regex "^.*$"
+  command: find -H /usr/lib/  -type f ! -group 0 -regex "^.*$"
   register: files_found
   changed_when: false
   failed_when: false
@@ -110,7 +110,7 @@
   - root_permissions_syslibrary_files
 
 - name: Find /usr/lib64/ file(s) matching ^.*$ recursively
-  command: find -H /usr/lib64/  -type f ! -gid 0 -regex "^.*$"
+  command: find -H /usr/lib64/  -type f ! -group 0 -regex "^.*$"
   register: files_found
   changed_when: false
   failed_when: false

bash remediation for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_sshd_private_key' differs.
--- xccdf_org.ssgproject.content_rule_file_groupownership_sshd_private_key
+++ xccdf_org.ssgproject.content_rule_file_groupownership_sshd_private_key
@@ -1,7 +1,7 @@
 # Remediation is applicable only in certain platforms
 if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ]; then
 
-find /etc/ssh/ -maxdepth 1 -type f ! -gid ssh_keys -regex '^.*_key$' -exec chgrp ssh_keys {} \;
+find /etc/ssh/ -maxdepth 1 -type f ! -group ssh_keys -regex '^.*_key$' -exec chgrp ssh_keys {} \;
 
 else
     >&2 echo 'Remediation is not applicable, nothing was done'

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_sshd_private_key' differs.
--- xccdf_org.ssgproject.content_rule_file_groupownership_sshd_private_key
+++ xccdf_org.ssgproject.content_rule_file_groupownership_sshd_private_key
@@ -1,5 +1,5 @@
 - name: Find /etc/ssh/ file(s) matching ^.*_key$
-  command: find -H /etc/ssh/ -maxdepth 1 -type f ! -gid ssh_keys -regex "^.*_key$"
+  command: find -H /etc/ssh/ -maxdepth 1 -type f ! -group ssh_keys -regex "^.*_key$"
   register: files_found
   changed_when: false
   failed_when: false

bash remediation for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_sshd_pub_key' differs.
--- xccdf_org.ssgproject.content_rule_file_groupownership_sshd_pub_key
+++ xccdf_org.ssgproject.content_rule_file_groupownership_sshd_pub_key
@@ -1,7 +1,7 @@
 # Remediation is applicable only in certain platforms
 if [ ! -f /.dockerenv ] && [ ! -f /run/.containerenv ]; then
 
-find /etc/ssh/ -maxdepth 1 -type f ! -gid 0 -regex '^.*\.pub$' -exec chgrp 0 {} \;
+find /etc/ssh/ -maxdepth 1 -type f ! -group 0 -regex '^.*\.pub$' -exec chgrp 0 {} \;
 
 else
     >&2 echo 'Remediation is not applicable, nothing was done'

ansible remediation for rule 'xccdf_org.ssgproject.content_rule_file_groupownership_sshd_pub_key' differs.
--- xccdf_org.ssgproject.content_rule_file_groupownership_sshd_pub_key
+++ xccdf_org.ssgproject.content_rule_file_groupownership_sshd_pub_key
@@ -1,5 +1,5 @@
 - name: Find /etc/ssh/ file(s) matching ^.*\.pub$
-  command: find -H /etc/ssh/ -maxdepth 1 -type f ! -gid 0 -regex "^.*\.pub$"
+  command: find -H /etc/ssh/ -maxdepth 1 -type f ! -group 0 -regex "^.*\.pub$"
   register: files_found
   changed_when: false
   failed_when: false

@vojtapolasek
Copy link
Collaborator Author

vojtapolasek commented Jun 7, 2023

@Mab879 @matejak I have reworked the PR. See the updated description and commit messages.

@vojtapolasek
Copy link
Collaborator Author

/retest

@vojtapolasek vojtapolasek force-pushed the fix_template_file_groupownership branch 2 times, most recently from bb1e8d8 to 6435e04 Compare June 7, 2023 08:27
@matejak
Copy link
Member

matejak commented Jun 7, 2023

I like the style of changes introduced very much, great work!
That said, test results show legit failures that need investigation.
I also believe that at least some of the codeclimate warnings are easy to handle, and should be considered.

Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does improve the clarity around the GID or name parameter in the file_groupowner template. I agree with @matejak that test failures should be looked at. Also, please take look CodeClimate issues, the PEP8 issues need to be fixed before merging, but also take look at the other issues as well.

@Mab879 Mab879 modified the milestones: 0.1.68, 0.1.69 Jun 9, 2023
@vojtapolasek vojtapolasek force-pushed the fix_template_file_groupownership branch 2 times, most recently from 6b0b757 to cb9ebce Compare June 13, 2023 11:24
@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Jun 13, 2023
change parameter filegid of file_groupownership template to gid_or_name
explain its behavior
change the parameter name from filgeid to gid_or_name
additionally, use the -group parameter instead of -gid parameter in find command invocation
the -group parameter accepts both gid and group names, where the -gid parameter accepts only gid
establish a new variable which carries information if the parameter is a gid or a group name
decide if to use group ID or group name based on the variable declared in the preprocessing function
change parameter name from file_gid to gid_or_name
The test did not account for a case when a filepath is a directory and it does not exist on the system.
Also the test made use of is_directory parameter which is only available at runtime when building the template and it is not saved anywhere.
Instead of this parameter, scenarios now check if the path ends with a slash. This signifies the filepath is a directory as mentioned in the tempate documentation.
…le_owner template

These test scenarios would not work correctly if the filepath parameter was a directory.
This rule uses directory + regex, it needs a special test scenario because the template does only generic testing.
@vojtapolasek vojtapolasek force-pushed the fix_template_file_groupownership branch from cb9ebce to d3dcd79 Compare June 14, 2023 12:55
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Jun 14, 2023
@Mab879
Copy link
Member

Mab879 commented Jun 15, 2023

/packit build

@vojtapolasek vojtapolasek force-pushed the fix_template_file_groupownership branch from 2fb2889 to d3dcd79 Compare June 15, 2023 13:25
…s exists before using it in test scenario

it migt not exist in FEdora container which is used as test environment
the missing_file_test test scenario should fail, not pass.
At the same time, we actually can't create a new failing test scenario - removing the ssh private key will break the testing workflow.
@codeclimate
Copy link

codeclimate bot commented Jun 22, 2023

Code Climate has analyzed commit 7a3e7a1 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 11.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 52.8% (0.0% change).

View more on Code Climate.

@vojtapolasek
Copy link
Collaborator Author

vojtapolasek commented Jun 23, 2023

I think this is ready to be merged.
The failing Codeclimate test... I think the code duplication is not worth extracting this piece of code into some module loaded by templates. I think it would decrease code readability.
The Automatus test on Fedora is failing on rules file_groupowner_etc_issue and file_groupowner_etc_issue_net. The reason is that on Fedora those files are symlinks. But mentioned rules ignore symlinks.

@Mab879 Mab879 merged commit b411169 into ComplianceAsCode:master Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ansible Ansible remediation update. Bash Bash remediation update. bugfix Fixes to reported bugs. OVAL OVAL update. Related to the systems assessments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error during evaluation of file_groupowner_var_log_syslog on RHEL 7
4 participants