Skip to content

Conversation

@evgenyz
Copy link
Member

@evgenyz evgenyz commented Sep 15, 2020

Change the audit=1 kernel option rule for RHCOS to be able to pass after remediation.

The coreos_kernel_option template only checks if the latest boot entry is compliant (acc. to #5285 (comment)).

@JAORMX I ended up with a different template for CoreOS. Once the problem of stalled boot entry will be solved this rule (and other kernel option related rules) could be switched to bls_entries_option template (which would check all boot entries).

@evgenyz evgenyz requested review from JAORMX and yuumasato September 15, 2020 18:11
@mildas
Copy link
Contributor

mildas commented Sep 15, 2020

Changes identified:
Profile moderate on rhcos4:
 Rule bls_audit_option removed from moderate profile.
 Rule coreos_audit_option added to moderate profile.
Others:
 Python abstract syntax tree change found in ssg/templates.py.

Recommended tests to execute:
 (cd build && cmake ../ && ctest -j4)
 build_product rhcos4
 tests/test_suite.py profile --libvirt qemu:///system test-suite-vm --datastream build/ssg-rhcos4-ds.xml moderate

@redhatrises
Copy link
Contributor

What's the follow up to fix BLS for all entries? Not having all entries configured is usually an audit finding.

@JAORMX
Copy link
Contributor

JAORMX commented Sep 16, 2020

/retest

@JAORMX
Copy link
Contributor

JAORMX commented Sep 16, 2020

What's the follow up to fix BLS for all entries? Not having all entries configured is usually an audit finding.

@redhatrises Hadn't we mentioned agreed that this would the solution while we figured out a way to fix this in coreos? Let me add a task for the team to follow up on this.

@evgenyz
Copy link
Member Author

evgenyz commented Sep 16, 2020

/retest

@JAORMX
Copy link
Contributor

JAORMX commented Sep 16, 2020

/test e2e-aws-rhcos4-moderate
cluster bootstrapping error

@JAORMX
Copy link
Contributor

JAORMX commented Sep 16, 2020

/retest

<unix:filepath operation="pattern match">^/boot/loader/entries/ostree-2-*\.conf</unix:filepath>
</unix:file_object>

</def-group>
Copy link
Contributor

Choose a reason for hiding this comment

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

For completeness; what about checking /proc/cmdline as well? That would ensure that the current configuration is compliant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still not sure if it should be a part of this check. Other bootloader-related rules don't have the runtime counterpart (because runtime check might be different even when bootloaders are the same). While this is not really applicable to this particular hackish rule, it would definitely be a problem for generic BLS-compatible check bls_entries_option in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would suggest creating a proc_cmdline_option template and a new rule based on this. It might be more preferable to have composite rules that could combine multiple templates into a single runtime+configuration check, but there is no way to make such a thing ATM, AFAIK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right; however, this is already a coreos specificy check. So maybe in this case it would make sense to add the proc/cmdline check here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw contrary to applying remediations on RHEL, applying a MachineConfig remediation through MachineConfigOperator always reboots the nodes, so when you apply this remediation, the machines in the cluster would automatically reboot into the approved config and at that point the /proc/cmdline check should pass as well. There is no extra disruption for the admin.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm working on it in #6100.

@evgenyz
Copy link
Member Author

evgenyz commented Sep 16, 2020

/test e2e-aws-rhcos4-moderate

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

LGTM.

CoreOS replaces the first boot entry with the second boot entry when updating right? So that there are only 2 boot entries.
I would a comment somewhere in the template mentioning that the check relies on this behavior.

Comment on lines +13 to +18
<criteria operator="AND">
<criterion test_ref="test_coreos_{{{ SANITIZED_ARG_NAME }}}_entry_1_options"
comment="Check if {{{ ARG_NAME_VALUE }}} is present in the 'options' line in /boot/loader/entries/ostree-1-*.conf" />
<criterion test_ref="test_coreos_{{{ SANITIZED_ARG_NAME }}}_entry_2_does_not_exists"
comment="Check if /boot/loader/entries/ostree-2-*.conf is not present" />
</criteria>
Copy link
Member

@yuumasato yuumasato Sep 17, 2020

Choose a reason for hiding this comment

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

This whole criteria is here to check the case when there wasn't a kernel update yet, thus entry 2 doesn't exist yet, right?
I would add a comment to the criteria, I was a bit confused a first, why ensure that entry 2 doesn't exist.

Copy link
Member Author

@evgenyz evgenyz Sep 18, 2020

Choose a reason for hiding this comment

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

I continued working on these rules in #6100, let's move there.

@evgenyz evgenyz marked this pull request as draft September 17, 2020 14:20
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Sep 17, 2020
@evgenyz evgenyz mentioned this pull request Sep 18, 2020
@evgenyz evgenyz marked this pull request as ready for review September 18, 2020 07:54
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Sep 18, 2020
@evgenyz
Copy link
Member Author

evgenyz commented Sep 18, 2020

I think we can merge it, follow-up improvements are in #6100.

@jan-cerny jan-cerny self-assigned this Sep 21, 2020
@jan-cerny jan-cerny added this to the 0.1.53 milestone Sep 21, 2020
@jan-cerny
Copy link
Collaborator

LGTM

@jan-cerny jan-cerny merged commit 28d6a7c into ComplianceAsCode:master Sep 21, 2020
JAORMX added a commit to JAORMX/content that referenced this pull request Sep 30, 2020
A recent commit [1] introduced an enhanced check for kernel arguments
that works in CoreOS. This commit takes them into use in rhcos4's
moderate profile. The needed checks were created with appropriate text.

[1] ComplianceAsCode#6088
JAORMX added a commit to JAORMX/content that referenced this pull request Sep 30, 2020
A recent commit [1] introduced an enhanced check for kernel arguments
that works in CoreOS. This commit takes them into use in rhcos4's
moderate & ncp profiles. The needed checks were created with appropriate text.

[1] ComplianceAsCode#6088
JAORMX added a commit to JAORMX/content that referenced this pull request Sep 30, 2020
A recent commit [1] introduced an enhanced check for kernel arguments
that works in CoreOS. This commit takes them into use in rhcos4's
moderate & ncp profiles. The needed checks were created with appropriate text.

[1] ComplianceAsCode#6088
JAORMX added a commit to JAORMX/content that referenced this pull request Sep 30, 2020
A recent commit [1] introduced an enhanced check for kernel arguments
that works in CoreOS. This commit takes them into use in rhcos4's
moderate & ncp profiles. The needed checks were created with appropriate text.

[1] ComplianceAsCode#6088
JAORMX added a commit to JAORMX/content that referenced this pull request Oct 1, 2020
A recent commit [1] introduced an enhanced check for kernel arguments
that works in CoreOS. This commit takes them into use in rhcos4's
moderate & ncp profiles. The needed checks were created with appropriate text.

[1] ComplianceAsCode#6088
JAORMX added a commit to JAORMX/content that referenced this pull request Oct 6, 2020
A recent commit [1] introduced an enhanced check for kernel arguments
that works in CoreOS. This commit takes them into use in rhcos4's
moderate & ncp profiles. The needed checks were created with appropriate text.

[1] ComplianceAsCode#6088
@evgenyz evgenyz deleted the coreos-kernel-option branch July 9, 2021 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants