Rsyslog files rules remediations

[teacup-on-rockingchair]

Description:

• Add remediations for rsyslog_files_groupownership and rsyslog_files_ownership

Rationale:

• Add template to set parameters of log files from rsyslog

Review Hints:

• Code for bash and ansible templates adapted from rsyslog_files_permissions remediation rule if accepted will rework also that rule to use the template

[openshift-ci]

Hi @teacup-on-rockingchair. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[github-actions]

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

rhel8 (from CTF) Environment (using Fedora as testing environment)

Fedora Testing Environment

Oracle Linux 8 Environment

[github-actions]

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

Click here to see the full diff

New content has different text for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership'.
--- xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership
+++ xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership
@@ -4,15 +4,23 @@
 
 [description]:
 The group-owner of all log files written by
-rsyslog should be 'xccdf_org.ssgproject.content_value_file_groupowner_logfiles_value'.
+rsyslog should be
+
+root.
+
 These log files are determined by the second part of each Rule line in
 /etc/rsyslog.conf and typically all appear in /var/log.
 For each log file LOGFILE referenced in /etc/rsyslog.conf,
 run the following command to inspect the file's group owner:
 $ ls -l LOGFILE
-If the owner is not 'xccdf_org.ssgproject.content_value_file_groupowner_logfiles_value', run the following command to
+If the owner is not
+
+root,
+
+run the following command to
 correct this:
-$ sudo chgrp 'xccdf_org.ssgproject.content_value_file_groupowner_logfiles_value' LOGFILE
+
+$ sudo chgrp root LOGFILE
 
 [reference]:
 BP28(R46)

OCIL for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership' differs.
--- ocil:ssg-rsyslog_files_groupownership_ocil:questionnaire:1
+++ ocil:ssg-rsyslog_files_groupownership_ocil:questionnaire:1
@@ -1,4 +1,7 @@
-The group-owner of all log files written by rsyslog should be .
+The group-owner of all log files written by rsyslog should be
+
+root.
+
 These log files are determined by the second part of each Rule line in
 /etc/rsyslog.conf and typically all appear in /var/log.
 To see the group-owner of a given log file, run the following command:

New datastream adds bash remediation for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership'.
New datastream adds ansible remediation for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_groupownership'.
New content has different text for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_ownership'.
--- xccdf_org.ssgproject.content_rule_rsyslog_files_ownership
+++ xccdf_org.ssgproject.content_rule_rsyslog_files_ownership
@@ -4,15 +4,23 @@
 
 [description]:
 The owner of all log files written by
-rsyslog should be 'xccdf_org.ssgproject.content_value_file_owner_logfiles_value'.
+rsyslog should be
+
+root.
+
 These log files are determined by the second part of each Rule line in
 /etc/rsyslog.conf and typically all appear in /var/log.
 For each log file LOGFILE referenced in /etc/rsyslog.conf,
 run the following command to inspect the file's owner:
 $ ls -l LOGFILE
-If the owner is not 'xccdf_org.ssgproject.content_value_file_owner_logfiles_value', run the following command to
+If the owner is not
+
+root,
+
+run the following command to
 correct this:
-$ sudo chown 'xccdf_org.ssgproject.content_value_file_owner_logfiles_value' LOGFILE
+
+$ sudo chown root LOGFILE
 
 [reference]:
 BP28(R46)

OVAL for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_ownership' differs.
--- oval:ssg-rsyslog_files_ownership:def:1
+++ oval:ssg-rsyslog_files_ownership:def:1
@@ -1,2 +1,2 @@
-criteria None
+criteria AND
 criterion oval:ssg-test_rsyslog_files_ownership:tst:1

OCIL for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_ownership' differs.
--- ocil:ssg-rsyslog_files_ownership_ocil:questionnaire:1
+++ ocil:ssg-rsyslog_files_ownership_ocil:questionnaire:1
@@ -1,4 +1,7 @@
-The owner of all log files written by rsyslog should be .
+The owner of all log files written by rsyslog should be
+
+root.
+
 These log files are determined by the second part of each Rule line in
 /etc/rsyslog.conf and typically all appear in /var/log.
 To see the owner of a given log file, run the following command:

New datastream adds bash remediation for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_ownership'.
New datastream adds ansible remediation for rule 'xccdf_org.ssgproject.content_rule_rsyslog_files_ownership'.

[marcusburghardt]

@teacup-on-rockingchair , I liked this idea. Very good. It will be a pleasure to review it soon.
This meantime, did you consider the new RainerScript syntax: https://www.rsyslog.com/doc/v8-stable/rainerscript/index.html

If not, for these rules which collect the files paths from the rsyslog configuration files, I believe it is relatively easy to extend the patters to consider lines in the new format. Here is an example of line in the new format:
authpriv.* action(type="omfile" FileCreateMode="0600" fileOwner="root" fileGroup="root" File="/var/log/secure")

Not the pattern in File="/var/log/secure"

[teacup-on-rockingchair]

Thanks for the feedback, will add clause for the above to the templates, and also to process include ( method

[marcusburghardt]

As I said before, I liked the idea of this template and believe it will have a positive impact considering the rsyslog rules in the project and the support for RainerScript syntax.

There are some improvements we can do to make it more flexible/scalable and generic. Regarding the Ansible, it seems most of the tasks were based on the rsyslog_files_permissions rule but I believe we have an opportunity to make it better by using proper modules instead of shell and command.

Please, take a look in the comments.

[marcusburghardt]

@teacup-on-rockingchair , I can take a look probably on Friday. This meantime, could you take a look on the failed tests, please? The ones related to bash-shellcheck seems legit.

[teacup-on-rockingchair]

@teacup-on-rockingchair , I can take a look probably on Friday. This meantime, could you take a look on the failed tests, please? The ones related to bash-shellcheck seems legit.

🙇 Thanks a lot! Will make sure to handle shellcheck concerns later today

[marcusburghardt]

@teacup-on-rockingchair , please take a look in my comments.

I believe you could also include the OVAL and Tests in the template. They are pretty similar in these two rules. It would remove some files and would make the template easier to be consumed.

So far, the tests are passing locally so I believe it is in the right path. : )

[marcusburghardt]

Thanks for the updates @teacup-on-rockingchair . What are your plans to include OVAL and test scenarios in this template? This would be great.

[teacup-on-rockingchair]

Thanks for the updates @teacup-on-rockingchair . What are your plans to include OVAL and test scenarios in this template? This would be great.

My bad totally forgot for that one will try to make this improvement by tomorrow morning

[marcusburghardt]

@teacup-on-rockingchair , I have just proposed an update in your branch: teacup-on-rockingchair#1

In this update I only did some optimizations in the OVAL. I hope to help your with this PR. ; )
After this, the tests are no longer reporting notchecked (See the CI tests) and we can use the test scenario scripts. It seems the test scenarios also need some updates.

[marcusburghardt]

@teacup-on-rockingchair , I found the issue with the test scenarios and they are pretty simple to be fixed. Please, see my proposed suggestions. There is also a small detail with one regex (just a missing escape char).

With these small adjusts all tests have passed in my local lab and we should be fine to merge it. Could you take a look, please?

[teacup-on-rockingchair]

@teacup-on-rockingchair , I found the issue with the test scenarios and they are pretty simple to be fixed. Please, see my proposed suggestions. There is also a small detail with one regex (just a missing escape char).

With these small adjusts all tests have passed in my local lab and we should be fine to merge it. Could you take a look, please?

Thanks @marcusburghardt, 🙇 I think now everywhere in the tests owner is used instead of groupowner and logic is reversed :)

[marcusburghardt]

@teacup-on-rockingchair , it was a great job in this PR.

Since it involves two different sintaxes for rsyslog, it is relatively complex to test, so I executed some extra tests locally and noticed although the "includes" are pretty well covered in both syntaxes, there is no test scenarios for log entries in RainerScript. Once I simulated one, I saw the regex should be updated (see my suggestion in OVAL file).

That suggested regex worked fine in OVAL, but the remediations should also be updated to be able to fix the log files in the RainerScript. In short, we should still update the remediation to make sure they can remediate log files defined in the RainerScript syntax. Would you like to work on these improvements?

I am asking that because I am planning to update the rsyslog_files_permissions rule to also use this new template once it is merged. Therefore, I would be happy to contribute in this template and include more test scenarios when extending it. So, I would be fine to merge this PR after the regex update even without new test scenarios, unless you plan to include more test scenarios for it, which would be amazing.

Let me know your plans, please.

[codeclimate]

Code Climate has analyzed commit 7447c59 and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 49.5% (-0.2% change).

View more on Code Climate.

[marcusburghardt]

Great job on this PR @teacup-on-rockingchair . This template is very useful for the project. Other rules can also benefit from this.

[marcusburghardt]

@dodys , do you have any additional point for this PR? I believe your comments were also fixed, correct?

anivan-suse is no longer active in the project.

[marcusburghardt]

I privately talked to @dodys and he agreed to merge this PR. I will quickly work in a extension of the this new template to also include the rsyslog_files_permissions rule and propose a new PR.

I privately talked to @dodys and he agrees to merge this PR. I will ping him the subsequent PR for rsyslog_files_permissions rule