-
Notifications
You must be signed in to change notification settings - Fork 684
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
Update ol7 stig references and severity values #5575
Update ol7 stig references and severity values #5575
Conversation
Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
Hi @iokomin. 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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
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 trust you that you know what you are doing with those IDs, so the only thing I comment on are severity overrides.
I think that there is no reason to protect an "unknown" severity, but I feel that severity of a rule is generally context-dependent, and it's best handled in the profile that selects the rule.
@@ -10,14 +10,19 @@ rationale: |- | |||
Removing the <tt>vsftpd</tt> package decreases the risk of its | |||
accidental activation. | |||
|
|||
severity: low | |||
severity: {{% if product == "ol7" -%}} |
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.
Another way how to do things like this is a rule refinement in a profile - for example here: https://github.com/ComplianceAsCode/content/blob/master/rhel8/profiles/ospp.profile#L125 I find refinements preferable, as changing the default severity of the rule may have consequences down the road.
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.
@matejak thanks for the hint, makes sense to me. Really, I've seen this adjustment as an example and profile looks like more reasonable place for it. I'll update this PR to move severity refinement to the profile.
Note: Making this change I just followed another existing approach:
severity: {{% if product == "rhel7" -%}} |
Probably it's worth to update places like these as well to avoid confusion and note severity adjustment howto in the Developers Guide (if not done already:))?
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.
@matejak I've noticed refined severity taken into account in eval scan results/report only. Generated guide uses rule default value. Is it by design, as it sounds reasonable to see refinements in generated guides as well?
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.
As guides are profile-specific, I would expect them to show the same severity as the scanner does during the scan. In other words, it looks like to be a bug in the scanner.
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.
Opened new issue for tracking: OpenSCAP/openscap#1512
medium | ||
{{%- else -%}} | ||
high | ||
{{%- endif %}} |
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 wouldn't make this change at all. This severity has to be high
.
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.
@redhatrises thanks for your guidance.
I don't mind to have it "high" and was confused by "medium" for this rule as well. However latter is in use in DISAs OL7 STIG v1r1. Do you think this was a mistake on their side and expected to be fixed in the next STIG update?
medium | ||
{{%- else -%}} | ||
unknown | ||
{{%- endif %}} |
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.
Always replace unknown
.
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.
@redhatrises acked, I'll update PR.
Does it make sense to mention it in the Developer Guide?
As not being the rule contributor, change it's default severity value to one from prodtype specific security guide just looks not obvious.
medium | ||
{{%- else -%}} | ||
unknown | ||
{{%- endif %}} |
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.
Always replace unknown
medium | ||
{{%- else -%}} | ||
unknown | ||
{{%- endif %}} |
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.
Always replace unknown
low | ||
{{%- else -%}} | ||
unknown | ||
{{%- endif %}} |
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.
Always replace unknown
medium | ||
{{%- else -%}} | ||
unknown | ||
{{%- endif %}} |
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.
Always replace unknown
Per review severity refinment moved to the profile, unknown severities replaced with values according to OL7 STIG v1r1. Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
@iokomin both DISA CCIs and SRGs shouldn't be product specific. The should be merged into the top-level SRG and CCI references. |
@redhatrises product specific srg, disa items product specific in places where OL7 ids differ from values in rules now, taking existing rhel6 approach as an example. |
There is actually no such thing as product specific SRGs and CCIs. The rhel6 example is from very old SRGs vs the new SRGs that are in use in operating systems version 7+. They are high-level generic references which is why they can all be merged together. STIGIDs, on the other hand, are product specific. |
Yes..... just adding them (non-duplicates) is fine. |
Ack, I'll make suggested changes and update this request. |
Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
@redhatrises please find requested change implemented. |
@iokomin looks good to me. |
@redhatrises I noticed "Changes requested" status still enabled. Is there anything else I should work on or this PR can be merged in master? |
It can be merged as is. Thanks! |
Description:
This large commit is mostly ol7 stig references and severity update according to DISA OL7 STIG v1r1.
Changes:
Rationale:
DISA OL7 STIG v1r1 released, rules need to be updated to match STIG references and severity.
Testing: