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

create_srg_export: Enable reading check and fix from controls even if they have rules listed #10769

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

jhrozek
Copy link
Collaborator

@jhrozek jhrozek commented Jun 30, 2023

Description:

The OCP4 STIG if a bit of an weird case. First, it mixes OCP4 and RHCOS
controls into a single stig and second, because of how the MachineConfig
remediations are not exactly user-friendly, we often lumped several
MachineConfigs from several rules into a single SRG row in the STIG
spreadsheet.

The current script presumes that every rule would emit a single row in
the sheet and that every rule's policy would be used exactly ones. The
most straightforward way of solving this is to just put the checks and
the fixes in the controls and then let the script optionally, with a new
command line flag that this patch adds, prefer those over policy files.

That way, we can have a single row even if the control specifies several
rules.

Additionally, because the current script presumes that rows whose check
and fix are read from the control files are either NA or IM and should
be separate in the resulting sheet, the script implements a rudimentary
deduplication of duplicate rows.

This behaviour is opt-in and would only be used by the OCP4 STIG for
the time being.

The other patches actually add check and fix to several control files and
enable the new flag for SRG export in GH actions for the OCP4 product.

Rationale:

  • OCP4 STIG

Review Hints:

  • Check out the sheet
  • Review the code

@github-actions
Copy link

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

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@jhrozek
Copy link
Collaborator Author

jhrozek commented Jun 30, 2023

@Mab879 are you OK with this approach?

@Mab879 Mab879 self-assigned this Jun 30, 2023
@Mab879
Copy link
Member

Mab879 commented Jul 3, 2023

/packit rebuild-failed

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.

I think this is a good approach. I have left a few comments. Please take at look the style issues found by code climate. The issue "Function "handle_control" has 8 parameters, which is greater than the 7 authorized" can be ignored for now.

old["SRGID"] = old["SRGID"] + "," + new["SRGID"]


def extend_results(results: list, rows: list, prefer_controls: bool) -> list:
Copy link
Member

Choose a reason for hiding this comment

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

The return type list doesn't match the None returned by this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, I refactored the code but forgot to refactor the signature

return True


def merge_rows(old: dict, new: dict):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def merge_rows(old: dict, new: dict):
def merge_rows(old: dict, new: dict) -> None:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

suggestion taken, thanks

@jhrozek
Copy link
Collaborator Author

jhrozek commented Jul 6, 2023

@Mab879 does this patch look any better now?

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.

I just have couple of comments.


# We didn't find any match, so we just add the row as new
results.extend(rows)

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the spaces on line 427.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, thanks

@@ -7,4 +7,62 @@ controls:
rules:
- audit_rules_sysadmin_actions
- audit_rules_usergroup_modification
check: |-
Verify Red Hat Enterprise Linux CoreOS (RHCOS) generates audit records for all account creations, modifications, disabling, and termination events that affect ""/etc/shadow"".
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Verify Red Hat Enterprise Linux CoreOS (RHCOS) generates audit records for all account creations, modifications, disabling, and termination events that affect ""/etc/shadow"".
Verify Red Hat Enterprise Linux CoreOS (RHCOS) generates audit records for all account creations, modifications, disabling, and termination events that affect "/etc/shadow".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops, thanks. I was grabbing this data from the spreadsheet with a script and for some reason the script read double quotes from the spredsheet..gotta fix that. Thanks!

…om the control over rules

The OCP4 STIG if a bit of an weird case. First, it mixes OCP4 and RHCOS
controls into a single stig and second, because of how the MachineConfig
remediations are not exactly user-friendly, we often lumped several
MachineConfigs from several rules into a single SRG row in the STIG
spreadsheet.

The current script presumes that every rule would emit a single row in
the sheet and that every rule's policy would be used exactly ones. The
most straightforward way of solving this is to just put the checks and
the fixes in the controls and then let the script optionally, with a new
command line flag that this patch adds, prefer those over policy files.

That way, we can have a single row even if the control specifies several
rules.

Additionally, because the current script presumes that rows whose check
and fix are read from the control files are either NA or IM and should
be separate in the resulting sheet, the script implements a rudimentary
deduplication of duplicate rows.

This behaviour is opt-in and would only be used by the OCP4 STIG for
the time being.
To enable the create_srg_export to create a single row for these SRGs
and later dedup them, let's add the check and the fix to the controls
directly.
@Mab879 Mab879 added this to the 0.1.69 milestone Jul 7, 2023
@Mab879 Mab879 added Infrastructure Our content build system OpenShift OpenShift product related. labels Jul 7, 2023
@codeclimate
Copy link

codeclimate bot commented Jul 7, 2023

Code Climate has analyzed commit 6f5b204 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

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 53.5% (0.0% change).

View more on Code Climate.

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.

Thanks for the PR!

@Mab879
Copy link
Member

Mab879 commented Jul 7, 2023

For now, I don't think it's worth fix the last code climate issues.

@Mab879 Mab879 merged commit e786d65 into ComplianceAsCode:master Jul 7, 2023
31 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Our content build system OpenShift OpenShift product related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants