Split RHEL 8 CIS profile using new controls file format #6976
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
Hi @alexhaydock. 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 |
openshift-ci-robot
added
ok-to-test
|
Changes identified: Show detailsProfile cis_level1_server on rhel8: Recommended tests to execute: |
|
I'm getting some errors in the CI jobs related to this change like: and I've pushed a commit which updates the RHEL8 |
|
/retest |
|
@openscap-ci test this please |
|
Pushed a new commit to fix the build issue the CI job was uncovering. I'd added a new HTML table type for CIS to the RHEL 8 |
|
At this point I'm getting a bit stuck getting this PR in order. I'm not sure my last commit at 24872eb actually fixes things or whether it's a band-aid over the CI failure I was getting before. I've tried to read the developer docs to get a better understanding of how the HTML tables are generated (which seems to be what's failing currently), but I'm struggling to see the process that's going on. I also don't want to patch things up by just copying things 4 times for these CIS profiles, when there could be a single unified table schema referred to once. If anyone's able to help point me in the right direction, that would be much appreciated. 😄 |
|
Something is going wrong with the table build process with the current commit. An example is below, that gets built as <html xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:cdf="http://checklists.nist.gov/xccdf/1.1" xmlns:xhtml="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<title></title>
</head>
<body>
<br><br><div style="text-align: center; font-size: x-large; font-weight:bold"></div>
<br><br><style type="text/css">
table
{
border-collapse:collapse;
}
table, th, td
{
border: 2px solid #dcdcdc;
border-left: none;
border-right: none;
vertical-align: top;
padding: 2px;
font-family: verdana,arial,sans-serif;
font-size:11px;
}
pre {
white-space: pre-wrap;
white-space: -moz-pre-wrap !important;
word-wrap:break-word;
}
table tr:nth-child(2n+2) { background-color: #f4f4f4; }
thead
{
display: table-header-group;
font-weight: bold;
background-color: #dedede;
}
</style>
<table><thead>
<td>CCE ID</td>
<td>Rule Title</td>
<td>Description</td>
<td>Rationale</td>
<td>Variable Setting</td>
<td>CIS v1.1.0 Mapping</td>
</thead></table>
</body>
</html>Clearly something isn't being referenced right here in the table generation process. It's not being populated with the right content and the test is failing as a result. |
|
Hello, could you please look at this discussion? |
Thanks! 😄 I replied to that other thread. If my goal was to try and land this in RHEL 8.5 (maybe ambitious I know) do you think it would be more sensible to just go ahead and split into the 4 Happy to wait if you think it's not going to be feasible either way, but just thought I'd ask. Edit: I'm not suggesting merging this until the build issues are fixed of course. I know it's still not working perfectly yet due to the HTML table build issues. 😄 |
|
I suggest to wait for introduction of tags into the policy definition files. |
What would be the next steps to work towards this do you think? I think some of the discussion in #6996 is now focusing on aspects which might be more CIS-specific and might be more relevant upstream (which is great! but might not help us in the shorter-term). |
|
@alexhaydock This PR introduces changes to the way in which policy levels are defined. |
@vojtapolasek That's my plan. I think overall this PR will need a complete rework, but I think we're in a much better situation now that #7072 has been merged. I'll try and put aside some time to work on this soon. In the meantime I've been trying to increase the coverage of some of the CIS rules that are missing. |
|
I've brought this branch back in line with |
Thanks @matejak I think I've addressed the failures in the build now. I guess we will find out when the next run completes. I did try the tool in #7249 and even after running This is what I was trying to run, inside the root dir of 8c8d6c7 and with the latest ~/fixrules/fix-rules.py -p products/rhel8/product.yml add-cce root_path_no_dot file_groupowner_efi_grub2_cfg file_owner_efi_grub2_cfg file_permissions_efi_grub2_cfg |
|
|
||
| references: | ||
| cis@rhel7: "5.7" | ||
| cis@rhel8: 5.7 |
There was a problem hiding this comment.
I am sorry, but you have to quote the 5.7 for the time being, because otherwise it is interpreted as a number, which confuses the build system.
There was a problem hiding this comment.
@matejak Ah that makes sense! I guess the references with multiple . delimiters are treated as strings automatically?
|
Nice! And one more thing - if you rebase against master (i.e. you run |
|
Hello @alexhaydock! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
|
@matejak ...I don't think those merge commands went quite as expected. Uh oh 😬 (or is that what you were expecting?) |
|
No, I think that everything went fine. Those failed actions indicate that an OVAL may be broken, but it probably doesn't have anything to do with the rebase - I have checked the branch before and after, and it looks the same. |
I guess the GitHub UI just presents it in a confusing way then. It went from the "Files Changed" tab showing just my changed files to showing 149 changed files, mostly from commits other than mine. But if that's expected then no problem! I'll have a look through the build output when they've all finished running and see whether I can track down the broken OVAL. |
|
@matejak I agree with @alexhaydock, something looks funny here. Did you (Alex) perhaps do the rebase/merge the wrong direction, and pulled stuff from master into this branch, rather than rebasing your branch on top of master? Just a thought. |
|
Alex probably rebased against an old master. File contents are fine (that's what I meant), but when I have tried to rebase, I got just over 50 commits, so yes, the PR is probably correct, but it is cluttered. So Alex please sync your master with upstream (i.e. this repository - |
Ah! I think you're absolutely right 🤦 I always manage to mess up merges somehow. Thank goodness for |
|
@alexhaydock: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
That's it, thank you very much for the effort! |
|
/retest |
matejak
dismissed
vojtapolasek’s stale review
The outstanding item has been fixed, and Vojta is on PTO, unable to acknowledge that.
|
Thanks for the help and feedback everyone! Glad to finally get this one done and merged. 😄 |
|
Out of interest @matejak (completely understand if you can't say) but do you know whether it's likely that CAC 0.1.58 will make it into RHEL 8.5, or is it more likely to be the 0.1.57 release that's already out? |
Unfortunately we can't comment on our plans, but you can try to read between the lines, and see how it went when 8.5 is out :-) |
yuumasato
added
the
Highlight
marcusburghardt
added
RHEL8
Description:
The current CIS profile for RHEL 8 is a monolithic single-profile combining the CIS rules. This does not address the fact that the CIS benchmark is published by CIS as 4 overlapping but separate sub-profiles:
This PR is an attempt to address this (for RHEL 8 initially, but I will expand this to RHEL 7 and 6 later). I intend to also do the same for CentOS and Rocky eventually.
Rationale:
This split uses the current profile format and simply splits it into 4 separate profiles which acknowledge the different rules which are enabled/disabled in each.
More information about this rationale can be found in my discussion thread #6924.
I considered an approach involving the more modular approach that the ANSSI profile is now taking, but since the CIS benchmarks are not cleanly "hierarchical" this doesn't work as cleanly here. At the very least we'd still need separate Server and Workstation profiles.
I know there might still be more work needed before this can be merged, and I'm also aware that there's a number of outstanding controls here which need rules adding. I can see that the upcoming RHEL 8.4 beta has rebased their CAC content to 0.1.54.
My (possibly ambitious) goal here is to land this change (or a similar one), and attempt to clean up any missing CIS rules in time for RHEL 8.5 to include a complete set of CIS profiles when it releases in late 2021/early 2022.
I would especially welcome any comments from @cipherboy or @vojtapolasek on this one, as I know they were involved in some of the discussions I had when planning for this. (No pressure though, I know big monolithic changes like this one can't be fun to review).
Thanks all 😄