CMP-4040, CMP-4041: Add support for CEL based rules and profiles#14597
CMP-4040, CMP-4041: Add support for CEL based rules and profiles#14597yuumasato wants to merge 14 commits intoComplianceAsCode:masterfrom
Conversation
yuumasato
requested review from
Mab879,
ggbecker,
jan-cerny,
marcusburghardt,
matusmarhefka and
vojtapolasek
as code owners
|
I verified PR #14597 and PR ComplianceAsCode/compliance-operator#1103 together. Generally it is good. The only problem is there is no |
yuumasato
force-pushed
the
add-cel-rules
branch
from
e7d189f to
af527ae
Compare
|
Thanks for the review @xiaojiey. Hopefully I have addessed the BuildConfig issue in the last commit. |
yuumasato
force-pushed
the
add-cel-rules
branch
from
af527ae to
188024f
Compare
yuumasato
force-pushed
the
add-cel-rules
branch
from
188024f to
25fe7a6
Compare
|
@yuumasato Sorry, I forgot to highlight, there is one more need to be updated. The ################# without --datastream-only parameter |
applications/openshift-virtualization/kubevirt-nonroot-feature-gate-is-enabled/rule.yml
Outdated
Show resolved
Hide resolved
Vincent056
left a comment
Vincent056
left a comment
There was a problem hiding this comment.
I think the PR looks good, just some questions on formatting and templating.
|
@xiaojiey Thanks, instead of removing '--datastream-only' I have added a new parameter '--cel-content=ocp4'. |
yuumasato
changed the title
|
@yuumasato Thanks for the update. Now with/without CEL profiles, the profiles can be created successfully. |
|
/retest |
build_product
Outdated
| printf '\t%s\n' "-t, --thin, --no-thin: Build thin data streams for each rule. Do not build any of the guides, tables, etc (off by default)" | ||
| printf '\t%s\n' "-r, --rule-id: Rule ID: Build a thin data stream with the specified rule. Do not build any of the guides, tables, etc (off by default)" | ||
| printf '\t%s\n' "-d, --datastream-only, --no-datastream-only: Build the data stream only. Do not build any of the guides, tables, etc (off by default)" | ||
| printf '\t%s\n' "--datastream, --no-datastream: Build the data stream. Do not build any of the guides, tables, etc (off by default)" |
There was a problem hiding this comment.
--no-datastream that name is confusing to me.
There was a problem hiding this comment.
Lol, it just following the pattern of the script.
There was a problem hiding this comment.
@Mab879 Thanks for review, hopefully I have addressed your comments in the new commits I added.
I kept the historical --no-datastream-only to be deprecated / removed when convenient, 😂
We expect this profile to exclusively leverage the CEL rules.
Add a new build-script along with a new output type that builds the CEL rules into the yaml that can be loaded by Compliance Operator.
Copies the CEL content file to the content images.
Adds --cel-content parameter that takes a comma separated list of products to build cel-content for. Add the new parameter with OCP4 product where it makes sense.
With addition of '--cel-content' as an option to build CEL content. And with it being additional to data stream builds, having '--datastream-only' parameter feels weird. This add '--datastream' so that we can move away from '--datastream-only' and be more consistent.
Keep only the --datastream option, which builds the CMake target that generates the data stream files, in addition to any other target defined during script invocation.
Keeps the fields pertainint to CEL scanning engine separate from the rule.yml, which can remain agnostic. This facilitates the implementation of templates later on. 'scanner_type' is completely removed from rules, and inferred by presence of 'cel' directory or presence of 'expression' and 'input' keys.
|
@Mab879 I have moved the CEL specific keys to its own file. |
|
/retest |
|
/test e2e-aws-openshift-node-compliance |
Mab879
left a comment
Mab879
left a comment
There was a problem hiding this comment.
Thanks for moving cel keys to the cel folder. Looks like the docs need to be updated.
| Run the following command: | ||
| <pre>$ oc get pods</pre> | ||
|
|
||
| failure_reason: |- # Optional: Custom failure message |
There was a problem hiding this comment.
This should be moved to cel folder in the docs as well.
| } | ||
| } | ||
| }, | ||
| "scanner_type": { |
There was a problem hiding this comment.
If this no longer used please remove from here and the code.
|
|
||
| description: |- | ||
| This profile defines a baseline that aligns to the Center for Internet Security® | ||
| Red Hat OpenShift Virtual Machine Extention Benchmark™, V1.0.0. |
There was a problem hiding this comment.
| Red Hat OpenShift Virtual Machine Extention Benchmark™, V1.0.0. | |
| Red Hat OpenShift Virtual Machine Extension Benchmark™, V1.0.0. |
rhmdnd
left a comment
rhmdnd
left a comment
There was a problem hiding this comment.
A few comments inline (most can be addressed in follow ups). Primary questions are focused on the ergonomics of build_product.
| - name: hcoList | ||
| kubernetes_input_spec: | ||
| api_version: hco.kubevirt.io/v1beta1 | ||
| resource: hyperconvergeds |
There was a problem hiding this comment.
followup: could add resource_name and resource_namespace here to simplify the filter (lines 11 - 12) below.
| - name: hcoList | ||
| kubernetes_input_spec: | ||
| api_version: hco.kubevirt.io/v1beta1 | ||
| resource: hyperconvergeds |
There was a problem hiding this comment.
followup: could add resource_name and resource_namespace here to simplify the filter (lines 11 - 12) below.
| - name: hcoList | ||
| kubernetes_input_spec: | ||
| api_version: hco.kubevirt.io/v1beta1 | ||
| resource: hyperconvergeds |
There was a problem hiding this comment.
followup: could add resource_name and resource_namespace here to simplify the filter (lines 11 - 12) below.
| - name: vms | ||
| kubernetes_input_spec: | ||
| api_version: kubevirt.io/v1 | ||
| resource: VirtualMachine |
There was a problem hiding this comment.
followup: virtualmachine to be consistent with the case used in the other rules (hyperconvergeds)
| # A rule uses CEL if it has both expression and inputs | ||
| # (loaded from cel/shared.yml during rule compilation) | ||
| has_expression = hasattr(rule, 'expression') and rule.expression | ||
| has_inputs = hasattr(rule, 'inputs') and rule.inputs |
There was a problem hiding this comment.
Aha - so this is the part that makes it so we don't need a scanner-type attribute in the rule?
| # ARG_OPTIONAL_BOOLEAN([bash-scripts],[],[Build Bash remediation scripts for every profile],[on]) | ||
| # ARG_OPTIONAL_BOOLEAN([datastream-only],[d],[Build the data stream only. Do not build any of the guides, tables, etc],[off]) | ||
| # ARG_OPTIONAL_ACTION([datastream],[],[Build the data stream. Do not build any of the guides, tables, etc]) | ||
| # ARG_OPTIONAL_SINGLE([cel-content],[],[Product(s) to build CEL content for (comma-separated)],[off]) |
There was a problem hiding this comment.
The datastream-only and datastream arguments are specific to SCAP, and we're adding another variant that essentially does something similar with a new format (cel-content).
I wonder if we need something more generic to allow for better ergonomics?
# build it all (cel, scap datastreams, guides, tables)
$ build_project ...
# equivalent to legacy --datastream-only (but without two arguments for specifying datastream)
$ build_project --output-implementations scap
# only build CEL content
$ build_project --output-implementations cel
| printf '\t%s\n' "-t, --thin, --no-thin: Build thin data streams for each rule. Do not build any of the guides, tables, etc (off by default)" | ||
| printf '\t%s\n' "-r, --rule-id: Rule ID: Build a thin data stream with the specified rule. Do not build any of the guides, tables, etc (off by default)" | ||
| printf '\t%s\n' "-d, --datastream-only, --no-datastream-only: Build the data stream only. Do not build any of the guides, tables, etc (off by default)" | ||
| printf '\t%s\n' "--datastream: Build the data stream. Do not build any of the guides, tables, etc" |
There was a problem hiding this comment.
I find it confusing that there are 2 options with almost same description. It would be nice to document that -d --datastream-only is legacy but does the same thing as --datastream.
|
|
||
| CEL (Common Expression Language) provides native Kubernetes resource evaluation without requiring shell access or OVAL checks. Rules using the CEL checking engine are used by the compliance-operator for Kubernetes and OpenShift compliance scanning. | ||
|
|
||
| **Important:** Rules with CEL checks are **excluded** from XCCDF/OVAL DataStreams and are generated as a separate `${PRODUCT}-cel-content.yaml` file. |
There was a problem hiding this comment.
Data streams are always "data streams", never "DataStreams", "datastreams" or anything else.
| **Important:** Rules with CEL checks are **excluded** from XCCDF/OVAL DataStreams and are generated as a separate `${PRODUCT}-cel-content.yaml` file. | |
| **Important:** Rules with CEL checks are **excluded** from XCCDF/OVAL data streams and are generated as a separate `${PRODUCT}-cel-content.yaml` file. |
| ``` | ||
|
|
||
| **Note:** | ||
| - Profiles use `scanner_type: CEL` to indicate they target the CEL checking engine and should be excluded from XCCDF/datastream builds. |
There was a problem hiding this comment.
What determines if a rule is included in CEL and if it's included in XCCDF? Is that determined by rule presence in profiles?
| ocil_clause: 'insecure registries are configured' | ||
|
|
||
| ocil: |- |
There was a problem hiding this comment.
What is the benefit of having OCIL in a CEL rule? Should the OCIL be present in CEL rules?
| ) | ||
| parser.add_argument( | ||
| "--resolved-rules-dir", required=True, | ||
| help="Directory containing resolved rule YAML files. " |
There was a problem hiding this comment.
at this moment they are JSON files, dtto for profiles
6 participants
Description:
ocp4product.Rationale:
Review Hints: