-
Notifications
You must be signed in to change notification settings - Fork 690
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
Review and Update pcidss_4 control file #11214
Review and Update pcidss_4 control file #11214
Conversation
Skipping CI for Draft Pull Request. |
Don't worry about CI errors for now. We will fix all of them before moving the PR from |
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 have reviewed section 3 as of 35aefaa . I approve this section.
I have reviewed section 4 as of 35aefaa. I approve this section. |
I have reviewed section 5 as of 35aefaa and I approve it. |
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.
Done for sections 1-2.
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 have reviewed section 7 and I have no objections.
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 have reviewed section 8 and have some suggestions.
5f6665a
to
1a65f22
Compare
I rebased the PR and resolved the conflict. |
@teacup-on-rockingchair , could you check these 3 rules and include SLE cce to them or remove sle from their prodtypes, please?
It was detected by CI tests. |
8394639
to
a5b7ef4
Compare
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 have reviewed section 6 as of d58d76e . I have few remarks.
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.
Just some minor points on section 9.
I have reviewed section 11, I have no objections. |
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.
One suggestion.
Setting status to "Request changes" to keep it for the other section.
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.
Review of the appendix is done.
I have couple of changes.
4f0ef28
to
5c8a589
Compare
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 have reviewed section 10. I have some comments, please take look at them.
@@ -2045,1274 +2629,1420 @@ controls: | |||
- service_auditd_enabled |
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 that rules grub2_audit_argument and grub2_audit_backlog_limit_argument belong here as well. They ensure that processes started before the actual audit process are auditable as well; events are collected and then consumed once the audit daemon starts.
Moreover, I think this requirement does not need to be related to Audit only. What about rsyslog_cron_logging, rsyslog_logging_configured, service_systemd-journald_enabled, service_rsyslog_enabled.
There are more rules which enable logging e.g. for SSH, httpd, FTP, DHCP... but I would consider those rather related.
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.
grub2_audit_argument
is already in10.7.2
.- The
10.2.1
is more about enabling the auditing and I believe10.7.2
is a better candidate for these specific configurations.
- The
grub2_audit_backlog_limit_argument
included in 10.7.2. Thanks for finding it.rsyslog_cron_logging
,rsyslog_logging_configured
,service_systemd-journald_enabled
,service_rsyslog_enabled
were included to related rules.- The PCI-DSS is not specific about products and solutions so I prefer to include less than more rules.
- But keeping them in related_rules is good to keep us aware and we can reconsider this along the time.
Changes in 11a80be
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 do not think that rules which make processes auditable before the audit daemon starts belong to 10.7.2, I see them more as assurance that the logging is active as soon as possible... but this can be disputed later, I don't think it is something what should block this PR.
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.
Sure @vojtapolasek . We can better discuss this in a separate PR. I believe along the time we will find many other opportunities of improvements to polish the control file. Thanks for the hints and all valid points you shared during your reviews.
5c8a589
to
103c1b7
Compare
Rebased |
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.
The sections I reviewed LTGM.
Thanks for getting this done!
Included the grub2_audit_backlog_limit_argument rule in order to explicitly configure a buffer used by audit during high loads. This buffer prevents events to be lost.
Replace the audit_rules_privileged_commands rule by audit_rules_suid_privilege_function which is more robust in this case.
Included directory_access_var_log_audit rule to restrict access to audit logs.
It is more about login failures and not about already logged users. Moved the rules to related_rules.
Included rule permissions_local_var_log.
More conservative approach to avoid potential conflicts of rules until we can better investigate.
The chronyd_client_only rule is not required but is related to the requirement.
Explicitly define the prodtype based on filter of current controls and profiles using it.
Code Climate has analyzed commit b158267 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 58.8%. View more on Code Climate. |
|
Reviewed section 10 as of b158267 . |
Great! I would like to thank @Mab879 , @vojtapolasek and @teacup-on-rockingchair for the great support you gave reviewing this huge, but necessary PR. Great collaboration guys!! |
Description:
A valuable work was done by introducing a control file for
PCI-DSSv4
in the project. Thanks @teacup-on-rockingchairThis is an important policy and I would like to facilitate the control file adoption by other products.
The entire control file was reviewed and updated in alignment to the PCI-DSS policy.
This PR is not introducing the
pcidss_4
control file to other products. It is only about reviewing the control file to ensure it is in the best possible state so other products can more easily adopt it.In General, this is how I reviewed the control file:
manual
and rules entry was removed.automated
.partial
and some notes about pending items were included.automated
without huge efforts, its status was moved toplanned
and some notes about what is expected to be done were included.pending
.Rationale:
pcidss_4
control file format and requirements so its adoption by other products is facilitated.Review Hints:
First of all, don't be scared by the number of commits.
This Policy is huge and many requirements are very related to each other.
It is common that a rule could fit in more than one requirement.
So along the review each change was in a separate commit to make the review much easier.
During the review, follow the flow of commits while checking the policy requirements to confirm the change makes sense. There are cases where a rule was inserted in a requirement from section 2 but when analyzing the section 8 it was concluded the rule better fits the section 8 and vice-versa. This is the reason I believe the review is much easier when checking commit by commit.
At the end, when all commits were checked and the context of each change is known, it time to a general review of the final state of the control file.
I hope these hints help to make the review less boring. : )
Additional Information:
After this PR, this should be the state of PCI-DSSv4 control file.