Review and Update pcidss_4 control file by marcusburghardt · Pull Request #11214 · ComplianceAsCode/content

marcusburghardt · 2023-10-20T14:44:49Z

Description:

A valuable work was done by introducing a control file for PCI-DSSv4 in the project. Thanks @teacup-on-rockingchair
This 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:

I read requirement by requirement in the policy to better understand its context.
For each requirement, I checked the equivalent entry in the control file.
- Basically all titles were edited to separate requirement title from its description and eventual notes.
- If the requirement is about paperwork, its status was moved to manual and rules entry was removed.
- If the requirement is automatable, existing rules were reviewed, new rules were included and unrelated rules were removed.
  - Other control files, like CIS, were used as reference for finding new rules related to the requirements.
- If the requirement is satisfied by selected rules, its status was moved to automated.
- If the requirement is partially satisfied by selected rules, but something is still missing, its status was moved to partial and some notes about pending items were included.
- If the requirement is clear and can be moved to automated without huge efforts, its status was moved to planned and some notes about what is expected to be done were included.
- If the requirement is unclear and needs more investigation, it was kept as pending.

Rationale:

Better PCI-DSS coverage.
Complete review of pcidss_4 control file format and requirements so its adoption by other products is facilitated.
Fixes PCI-DSS v4 was released on March 2022 #9161

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.

General stats:
all                 351 / 351 = 100.0%
applicable          349 / 351 = 99.43%
assessed            341 / 351 = 97.15%
full coverage       305 / 351 = 86.89%
not assessed          8 / 351 = 2.28%

Stats grouped by status:
automated            48 / 349 = 13.75%
manual              257 / 349 = 73.64%
not applicable        2 / 349 = 0.57%
partial              23 / 349 = 6.59%
pending               8 / 349 = 2.29%
planned               6 / 349 = 1.72%
supported             7 / 349 = 2.01%

Rules and Variables in pcidss_4 - base:
290 rules are selected
28 variables are explicitly defined

openshift-ci · 2023-10-20T14:44:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

github-actions · 2023-10-20T14:46:25Z

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

Fedora Environment

Oracle Linux 8 Environment

marcusburghardt · 2023-10-20T14:53:15Z

Don't worry about CI errors for now. We will fix all of them before moving the PR from draft to ready.

vojtapolasek

I have reviewed section 3 as of 35aefaa . I approve this section.

vojtapolasek · 2023-11-02T13:56:48Z

I have reviewed section 4 as of 35aefaa. I approve this section.

vojtapolasek · 2023-11-02T15:37:43Z

I have reviewed section 5 as of 35aefaa and I approve it.

Mab879

Done for sections 1-2.

Mab879

I have reviewed section 7 and I have no objections.

Mab879

I have reviewed section 8 and have some suggestions.

marcusburghardt · 2023-11-03T13:55:46Z

I rebased the PR and resolved the conflict.

marcusburghardt · 2023-11-03T14:19:29Z

@teacup-on-rockingchair , could you check these 3 rules and include SLE cce to them or remove sle from their prodtypes, please?

no_password_auth_for_systemaccounts
auditd_name_format
package_libselinux_installed

It was detected by CI tests.

vojtapolasek

I have reviewed section 6 as of d58d76e . I have few remarks.

Mab879

Just some minor points on section 9.

Mab879 · 2023-11-07T21:02:53Z

I have reviewed section 11, I have no objections.

Mab879

One suggestion.

Setting status to "Request changes" to keep it for the other section.

Mab879

Review of the appendix is done.

I have couple of changes.

vojtapolasek

I have reviewed section 10. I have some comments, please take look at them.

vojtapolasek · 2023-11-08T10:24:57Z

@@ -2045,1274 +2629,1420 @@ controls:
        - service_auditd_enabled


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.

grub2_audit_argument is already in 10.7.2.

The 10.2.1 is more about enabling the auditing and I believe 10.7.2 is a better candidate for these specific configurations.

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_enabledwere 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

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.

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.

marcusburghardt · 2023-11-08T14:02:28Z

Rebased

Mab879

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.

qlty-cloud-legacy · 2023-11-09T13:14:10Z

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.

marcusburghardt · 2023-11-09T15:03:34Z

Gate (AR / RHEL) / Build, Lint Ansible Roles on Fedora Latest (Container) (pull_request) failure is not related to this PR.

vojtapolasek · 2023-11-09T16:55:55Z

Reviewed section 10 as of b158267 .
I approve it and I am merging the whole update to the control file. Thank you @marcusburghardt for the thorough review and update, thanks go also to @Mab879 .

marcusburghardt · 2023-11-10T08:01:21Z

Great! I would like to thank @Mab879 , @vojtapolasek and @teacup-on-rockingchair for the great support you gave reviewing this huge, but necessary PR.
Also @mildas for helping with the #11257

Great collaboration guys!!

marcusburghardt added Update Profile Issues or pull requests related to Profiles updates. pci-dss labels Oct 20, 2023

marcusburghardt requested review from Mab879, ggbecker and teacup-on-rockingchair October 20, 2023 14:44

openshift-ci Bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 20, 2023

vojtapolasek reviewed Nov 2, 2023

View reviewed changes

Comment thread controls/pcidss_4.yml

openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Nov 2, 2023

Mab879 reviewed Nov 2, 2023

View reviewed changes

Comment thread controls/pcidss_4.yml Outdated

Mab879 reviewed Nov 2, 2023

View reviewed changes

Mab879 requested changes Nov 2, 2023

View reviewed changes

Comment thread controls/pcidss_4.yml

Comment thread controls/pcidss_4.yml Outdated

Comment thread controls/pcidss_4.yml Outdated

Comment thread controls/pcidss_4.yml

marcusburghardt force-pushed the pci-dss4_review branch from 5f6665a to 1a65f22 Compare November 3, 2023 13:55

openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Nov 3, 2023

marcusburghardt marked this pull request as ready for review November 3, 2023 13:56

openshift-ci Bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 3, 2023

marcusburghardt force-pushed the pci-dss4_review branch 2 times, most recently from 8394639 to a5b7ef4 Compare November 3, 2023 14:53

vojtapolasek requested changes Nov 7, 2023

View reviewed changes

Comment thread controls/pcidss_4.yml Outdated

Comment thread controls/pcidss_4.yml

Comment thread controls/pcidss_4.yml Outdated

Mab879 requested changes Nov 7, 2023

View reviewed changes

Comment thread controls/pcidss_4.yml Outdated

Comment thread controls/pcidss_4.yml Outdated

Mab879 requested changes Nov 7, 2023

View reviewed changes

Comment thread controls/pcidss_4.yml

Mab879 requested changes Nov 7, 2023

View reviewed changes

Comment thread controls/pcidss_4.yml Outdated

Comment thread controls/pcidss_4.yml Outdated

marcusburghardt force-pushed the pci-dss4_review branch from 4f0ef28 to 5c8a589 Compare November 8, 2023 08:22

vojtapolasek requested changes Nov 8, 2023

View reviewed changes

marcusburghardt force-pushed the pci-dss4_review branch from 5c8a589 to 103c1b7 Compare November 8, 2023 14:01

Mab879 assigned Mab879, teacup-on-rockingchair and marcusburghardt Nov 8, 2023

Mab879 added this to the 0.1.71 milestone Nov 8, 2023

Mab879 approved these changes Nov 8, 2023

View reviewed changes

Mab879 assigned vojtapolasek and unassigned marcusburghardt Nov 8, 2023

marcusburghardt added 9 commits November 9, 2023 12:26

Complement pcidss_4 requirement 10.7.2

11a80be

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.

Improve pcidss_4 requirement 10.2.1.2

40385e5

Replace the audit_rules_privileged_commands rule by audit_rules_suid_privilege_function which is more robust in this case.

Complement pcidss_4 requirement 10.3.1

eef659f

Included directory_access_var_log_audit rule to restrict access to audit logs.

Remove duplicated rule in pcidss_4 10.6.3

2093f48

Update pcidss_4 requirement 10.2.1.4

08a399c

It is more about login failures and not about already logged users. Moved the rules to related_rules.

Complement pcidss_4 requirement 10.3.1

439afb5

Included rule permissions_local_var_log.

Update pcidss_4 requirement 10.6.2

cbf4fab

More conservative approach to avoid potential conflicts of rules until we can better investigate.

Include related_rule for pcidss_4 requirement 10.6.3

73f25ae

The chronyd_client_only rule is not required but is related to the requirement.

Define prodtype for directory_access_var_log_audit

b158267

Explicitly define the prodtype based on filter of current controls and profiles using it.

vojtapolasek approved these changes Nov 9, 2023

View reviewed changes

marcusburghardt added the Highlight This PR/Issue should make it to the featured changelog. label Nov 9, 2023

vojtapolasek merged commit 7e7b7d1 into ComplianceAsCode:master Nov 10, 2023

marcusburghardt deleted the pci-dss4_review branch November 10, 2023 08:01

This was referenced Nov 10, 2023

Some rules in PCI-DSS profile are missing OCIL (RHEL8) #4913

Open

Update PCI-DSS profile for RHEL #11267

Merged

rhmdnd mentioned this pull request Aug 16, 2024

Add ocp4 pci dss references #12309

Merged

		@@ -2045,1274 +2629,1420 @@ controls:
		- service_auditd_enabled

Conversation

marcusburghardt commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Rationale:

Review Hints:

Additional Information:

Uh oh!

openshift-ci Bot commented Oct 20, 2023

Uh oh!

github-actions Bot commented Oct 20, 2023

Uh oh!

marcusburghardt commented Oct 20, 2023

Uh oh!

vojtapolasek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vojtapolasek commented Nov 2, 2023

Uh oh!

vojtapolasek commented Nov 2, 2023

Uh oh!

Mab879 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mab879 left a comment

Choose a reason for hiding this comment

Uh oh!

Mab879 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcusburghardt commented Nov 3, 2023

Uh oh!

marcusburghardt commented Nov 3, 2023

Uh oh!

vojtapolasek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mab879 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Mab879 commented Nov 7, 2023

Uh oh!

Mab879 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mab879 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vojtapolasek left a comment

Choose a reason for hiding this comment

Uh oh!

vojtapolasek Nov 8, 2023

Choose a reason for hiding this comment

Uh oh!

marcusburghardt Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vojtapolasek Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

marcusburghardt Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcusburghardt commented Oct 20, 2023 •

edited

Loading

marcusburghardt Nov 9, 2023 •

edited

Loading