Fix the generation of SCE checks in the output datastream #10015

nightmared · 2023-01-02T10:51:34Z

Description:

The support for SCE scripts appear to be have been broken since the switch from oscap ds sds-compose, as the SCE scripts are no longer embedded in the output datastream.
In fact, trying to build SCE scripts should currently fail because some function calls were not updated to account for recently added parameters.

This PR:

skips over CPE enumeration while generating SCE metadata (otherwise we have a dependency loop, as we need to process CPE metadata before generating SCE, but the CPE generation itself may require SCE metadata!).
fixes a slight omission where SCE metadata did not add the 'xccdf_org.ssgproject.content_value_' prefix to the check variables
adds the content of the SCE checks to the output datastream, as well as the glue (the component references) needed for OpenSCAP to find those checks in the datastream

Rationale:

A customer of my employer is relying on SCE checks for custom automated checks.
This customer builds his own fork of ComplianceAsCode with a dedicated profile that combines these company-specific checks atop an existing profile for compliance with government standards.
We ran into problems with SCE scripts when rebasing their changes against the latest version of CAC (v0.1.65).
We were able to use successfully use SCE scripts by applying this patchset.

openshift-ci · 2023-01-02T10:51:45Z

Hi @nightmared. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

github-actions · 2023-01-02T10:53:06Z

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

Fedora Environment

Oracle Linux 8 Environment

nightmared · 2023-01-02T13:18:55Z

I rewrote the last commit to remove the use of strings (it requires python >= 3.6, and it appears the centOS7 builder in the CI uses python3.5).

jan-cerny

Thank you very much for pointing this problem.

I tried to check whether the build works. I wasn't exactly sure how to do that. I have found that at this moment the only product that contains a SCE check in the project is the Ubuntu 20.04. There is only one SCE check and that is linux_os/guide/system/accounts/accounts-session/accounts_users_own_home_directories/sce/ubuntu2004.sh. I used the following command to build it:

[jcerny@thinkpad scap-security-guide{pr/10015}]$ ADDITIONAL_CMAKE_OPTIONS="-DSSG_SCE_ENABLED=ON" ./build_product -d ubuntu2004

On master branch, this trace backs. With this PR it builds successfully and it doesn't trace back. That's great. However, the built SCAP source data stream doesn't contain the aforementioned SCE file anywhere and I also can't find any component with that.

I think you should make it actually working and make sure the SCE content really gets populated to the generated SCAP source data stream. I think the problem related to the XPath you use (see comments inline).

I'm wondering how to prevent this bug in future. It seems that the bug hasn't been caught by any tests. Perhaps it would be very useful to add a test to this PR that verifies the SCE content build. I also wonder what setup is used by the customers. We should add measures to prevent breaking their use case again.

build-scripts/compose_ds.py

jan-cerny · 2023-01-05T09:29:57Z

build-scripts/compose_ds.py

+    # The component ID is the component-ref href without leading '#'
+    checklist_component_id = checklists_component_ref.get(xlink_href)[1:]
+
+    # Locate the <xccdf:check> inside <xccdf:Rule> and detect the SCE checks


The problem with comments is that they can become easily outdated. They feel superflous. By better naming and better structuring code the code will become selfexplanatory and you will not need the comments.

build-scripts/compose_ds.py

jan-cerny · 2023-01-05T09:39:47Z

build-scripts/compose_ds.py

+# When SCE support is enabled, retrieve the SCE checks and embed them in the datastream.
+# Collect every SCE check, extract the path to the script content, and include
+# every scripts in the datastream.
+def collect_sce_checks(datastreamtree, checklists, refdir):


we need unit tests for this function

build-scripts/compose_ds.py

nightmared · 2023-01-07T15:44:14Z

Thanks for your numerous comments!

I am working on a new version of this patch that will fix these shortcomings.

As for the use of SCE by our customer, some SCE checks were written to either add checks outside of official recommendations, or to add support for specified by checks that lacked an implementation yet, like #10017.
That fork of ComplianceAsCode is built internally and distributed to customers via internal repos and/or custom RHEL ISOs.

I do not believe it would be easy for you to detect such breakage, except maybe by having an SCE checks enabled by default in a standard profile (something like #10017 maybe?). I'm sure automated tests would help too, but I'm not very experienced with your test scripts, so even though I'm going to take a look at adding SCE tests, I can't guarantee much about their quality :)

…gproject.content_value_'

…n the output datastream

nightmared · 2023-01-15T17:51:16Z

@jan-cerny Hello, sorry for the spam, but is there anything I can do to improve further this patch?

jan-cerny

Hi, Thank you for your improvements. It looks great, the code seems to be OK. I only have some last comments, see below.

ssg/build_sce.py

jan-cerny · 2023-01-16T08:26:12Z

build_product

@@ -378,7 +378,7 @@ for chosen_product in "${_arg_product[@]}"; do
 	fi
 done

-CMAKE_OPTIONS=(${ADDITIONAL_CMAKE_OPTIONS} "${build_type_option}" "${oval_major_version_option}" "${oval_minor_version_option}" '-DSSG_PRODUCT_DEFAULT=OFF' "${cmake_enable_args[@]}" -G "$cmake_generator")
+CMAKE_OPTIONS=(${ADDITIONAL_CMAKE_OPTIONS} "${build_type_option}" "${oval_major_version_option}" "${oval_minor_version_option}" '-DSSG_PRODUCT_DEFAULT=OFF' '-DSSG_SCE_ENABLED:BOOL=ON' "${cmake_enable_args[@]}" -G "$cmake_generator")


We have discussed this with the team and we think that we shouldn't enable SCE by default because at this moment we don't have any ways of ensuring quality of the contents of SCE files, ie. Automatus tests don't support SCE. Please remove this change. People can enable it on demand by adding the '-DSSG_SCE_ENABLED:BOOL=ON to the ADDITIONAL_CMAKE_OPTIONS bash variable.

I think that you have added this due to enablement of the tests in the CI. I think that instead this can be achieved by enabling it explicitly in the GitHub Actions definitions in .github/workflows/gate.yaml. For example, it can be added to the Ubuntu job that builds the Ubuntu content because that already contains the SCE check. What do you think about this?

Sure, I hadn't thought of using ADDITIONAL_CMAKE_OPTIONS.
Would the new patch (that only enables it for Ubuntu 20.04 in the CI via the method your proposed) be acceptable in that regard?

codeclimate · 2023-01-16T11:39:39Z

Code Climate has analyzed commit 921e8e1 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 12.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 49.7% (0.0% change).

View more on Code Climate.

jan-cerny

I have checked that the SCE script is present in the form of an extended component in the generated SCAP source data stream for the Ubuntu 20.04 and I have verified that it happens only when ADDITIONAL_CMAKE_OPTIONS="-DSSG_SCE_ENABLED=ON". Also, I have seen that the test ds-sce is passing in the Ubuntu 20.04 GitHub Actions CI job.

Great job! Thank you!

openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label Jan 2, 2023

nightmared force-pushed the fix_sce_datastream branch from a265c67 to d4fa0d6 Compare January 2, 2023 13:17

nightmared force-pushed the fix_sce_datastream branch 2 times, most recently from 0e3591b to 822b39d Compare January 2, 2023 14:10

nightmared mentioned this pull request Jan 2, 2023

Add a new rule ssh_keys_passphrase_protected #10017

Merged

jan-cerny requested changes Jan 5, 2023

View reviewed changes

jan-cerny self-assigned this Jan 5, 2023

jan-cerny added Infrastructure Our content build system bugfix Fixes to reported bugs. labels Jan 5, 2023

nightmared force-pushed the fix_sce_datastream branch 2 times, most recently from e56f906 to 8d8fd7b Compare January 7, 2023 16:00

nightmared added 3 commits January 7, 2023 17:08

build support: ignore empty CPE directories (needed for SCE generation)

e73dbe1

build support: prefix the variables in SCE scripts with 'xccdf_org.ss…

2a3341e

…gproject.content_value_'

build support: when SCE support is enabled, include the SCE scripts i…

a6c33d8

…n the output datastream

nightmared force-pushed the fix_sce_datastream branch from 8d8fd7b to a6c33d8 Compare January 7, 2023 16:08

nightmared requested a review from jan-cerny January 7, 2023 18:36

nightmared force-pushed the fix_sce_datastream branch 2 times, most recently from f10f307 to 44fe729 Compare January 7, 2023 18:41

jan-cerny added this to the 0.1.66 milestone Jan 11, 2023

jan-cerny requested changes Jan 16, 2023

View reviewed changes

nightmared added 2 commits January 16, 2023 12:02

build scripts: introduce a test for SCE checks

9879ce7

build_product script: enable SCE checks for ubuntu20.04

921e8e1

nightmared force-pushed the fix_sce_datastream branch from 44fe729 to 921e8e1 Compare January 16, 2023 11:02

jan-cerny approved these changes Jan 17, 2023

View reviewed changes

jan-cerny merged commit 2b5e5ab into ComplianceAsCode:master Jan 17, 2023

nightmared deleted the fix_sce_datastream branch February 13, 2023 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the generation of SCE checks in the output datastream #10015

Fix the generation of SCE checks in the output datastream #10015

nightmared commented Jan 2, 2023 •

edited

Loading

openshift-ci bot commented Jan 2, 2023

github-actions bot commented Jan 2, 2023

nightmared commented Jan 2, 2023 •

edited

Loading

jan-cerny left a comment

jan-cerny Jan 5, 2023

jan-cerny Jan 5, 2023

nightmared commented Jan 7, 2023

nightmared commented Jan 15, 2023

jan-cerny left a comment

jan-cerny Jan 16, 2023

nightmared Jan 16, 2023

codeclimate bot commented Jan 16, 2023

jan-cerny left a comment

Fix the generation of SCE checks in the output datastream #10015

Fix the generation of SCE checks in the output datastream #10015

Conversation

nightmared commented Jan 2, 2023 • edited Loading

Description:

Rationale:

openshift-ci bot commented Jan 2, 2023

github-actions bot commented Jan 2, 2023

nightmared commented Jan 2, 2023 • edited Loading

jan-cerny left a comment

Choose a reason for hiding this comment

jan-cerny Jan 5, 2023

Choose a reason for hiding this comment

jan-cerny Jan 5, 2023

Choose a reason for hiding this comment

nightmared commented Jan 7, 2023

nightmared commented Jan 15, 2023

jan-cerny left a comment

Choose a reason for hiding this comment

jan-cerny Jan 16, 2023

Choose a reason for hiding this comment

nightmared Jan 16, 2023

Choose a reason for hiding this comment

codeclimate bot commented Jan 16, 2023

jan-cerny left a comment

Choose a reason for hiding this comment

nightmared commented Jan 2, 2023 •

edited

Loading

nightmared commented Jan 2, 2023 •

edited

Loading