MCO-2441: MCO-2442: MCO-2443: Support Openshift —> RHEL translation for tlsSecurityProfile options - #6297
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change derives Fedora crypto policies from OpenShift TLS profiles, passes them through template rendering into MachineConfig files, and applies changed policies during node updates with a FIPS safeguard. Tests cover policy mapping, rendering, daemon behavior, and related call-site updates. ChangesCrypto policy lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIServer
participant TemplateController
participant MachineConfig
participant NodeDaemon
participant CryptoPolicyTool
APIServer->>TemplateController: provide TLS security profile
TemplateController->>MachineConfig: render crypto-policy files
MachineConfig->>NodeDaemon: deliver changed policy paths
NodeDaemon->>CryptoPolicyTool: apply validated crypto policy
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkg/daemon/update.go (2)
2656-2660: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
/etc/crypto-policies/configpath limits test isolation.Unlike
fipsFile, which is a package-level var so tests can override it, the crypto-policies config path is a literal string used directly. Extracting it into a package var (mirroringfipsFile) would let tests point at a temp file instead of the real host path, avoiding host-state coupling inTestApplyCryptoPolicy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/daemon/update.go` around lines 2656 - 2660, The crypto-policies config path is hardcoded, preventing test isolation. Extract the path into a package-level variable near fipsFile, update the ReadFile call in the crypto-policy application logic to use that variable, and preserve the existing default path so TestApplyCryptoPolicy can override it with a temporary file.
2669-2673: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout/cancellation around the
update-crypto-policiesexternal call.
runCmdSync("update-crypto-policies")is invoked with no visible context or timeout; if the binary hangs, this stalls the daemon's wholeupdate()flow.As per path instructions, "context.Context for cancellation and timeouts" for
**/*.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/daemon/update.go` around lines 2669 - 2673, Add context-based cancellation with an appropriate timeout around the external command in the crypto-policy update flow, replacing the unbounded runCmdSync call. Update the relevant helper or call path near the policy application logic to accept and propagate context.Context, while preserving the existing wrapped error and successful return behavior.Source: Path instructions
pkg/controller/template/kubelet_config_dir_test.go (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnkeyed
RenderConfig{}literals are fragile as the struct grows.Both call sites now pass 7 positional values (
&RenderConfig{&controllerConfig.Spec, ..., "", "", nil}) to account for the newCryptoPolicy/CryptoPolicySubModfields. Positional literals only verify field count, not field identity — a future field reorder or insertion could silently shift values into the wrong fields without a compile error, unlike keyed literals (as already used at the production call site intemplate_controller.go).♻️ Suggested fix
- cfgs, err := generateTemplateMachineConfigs(&RenderConfig{&controllerConfig.Spec, `{"dummy":"dummy"}`, "dummy", nil, "", "", nil}, templateDir) + cfgs, err := generateTemplateMachineConfigs(&RenderConfig{ + ControllerConfigSpec: &controllerConfig.Spec, + PullSecret: `{"dummy":"dummy"}`, + TLSMinVersion: "dummy", + TLSCipherSuites: nil, + CryptoPolicy: "", + CryptoPolicySubMod: "", + }, templateDir)(Adjust remaining trailing field name to match
RenderConfig's actual definition.)Also applies to: 76-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/template/kubelet_config_dir_test.go` at line 49, Replace the unkeyed RenderConfig literals at both test call sites with keyed field literals, explicitly assigning each value to the corresponding RenderConfig fields, including the trailing CryptoPolicy and CryptoPolicySubMod fields according to the struct definition. Preserve the existing values and behavior.pkg/daemon/update_test.go (1)
984-1023: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-hermetic assertions in
TestApplyCryptoPolicy.The "triggers update" cases assume the failure comes from the missing
update-crypto-policiesbinary, but the function first reads real/proc/sys/crypto/fips_enabledand/etc/crypto-policies/configfrom the host. On a FIPS-enabled CI runner, the error would instead originate from the FIPS guard (or a file-read failure) rather than the missing binary — the test still passes (since it only assertsexpectError) but for a different reason than documented, and behavior is host-dependent. Tying this to injectable paths (see suggestion inupdate.go) and asserting on error content would make the test deterministic and actually validate the intended code path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/daemon/update_test.go` around lines 984 - 1023, The TestApplyCryptoPolicy cases should avoid reading host-specific /proc and crypto-policy files and should verify the intended missing-binary path deterministically. Update applyCryptoPolicy and its callers to use injectable paths or dependencies, configure those in TestApplyCryptoPolicy for a controlled non-FIPS environment, and assert the returned error content identifies the unavailable update-crypto-policies command rather than only checking that an error occurred.pkg/controller/template/render_test.go (1)
85-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keyed struct literals for
RenderConfigin tests.These call sites use positional literals for a 7-field struct where several fields share type
string/[]string.go vet's unkeyed-composite-literal check doesn't fire here since the literal is same-package, so a future field reorder could silently swap values without a compile error. This PR already had to touch every call site to insert 2 new positional args.♻️ Example fix for one call site
- got, err := renderTemplate(RenderConfig{&config.Spec, `{"dummy":"dummy"}`, "dummy", nil, "", "", nil}, name, dummyTemplate) + got, err := renderTemplate(RenderConfig{ + ControllerConfigSpec: &config.Spec, + PullSecret: `{"dummy":"dummy"}`, + TLSMinVersion: "dummy", + }, name, dummyTemplate)Also applies to: 146-146, 240-240, 247-247, 258-258, 380-380, 513-513
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/template/render_test.go` at line 85, Replace the positional RenderConfig literals in all listed renderTemplate test call sites with keyed field literals, using the actual RenderConfig field names. Preserve each call’s existing values while making every argument explicit so future field additions or reordering cannot silently change behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/common/helpers.go`:
- Around line 1276-1297: Update GetCryptoPolicyFromTLSProfile for
TLSProfileModernType to use the documented TLS 1.3-only crypto-policy module
syntax that removes inherited older protocols, replacing the current
protocol@TLS = TLS1.3+ value. Leave the other profile mappings unchanged.
In `@pkg/daemon/update.go`:
- Around line 2649-2667: Update the FIPS policy validation inside processFips to
accept the exact policy “FIPS” and subpolicies beginning with “FIPS:”, while
continuing to reject all other values. Preserve the existing file-read and error
propagation behavior.
- Around line 1220-1223: Update the update() flow around applyCryptoPolicy so
failures after the system-wide crypto policy change also restore the previously
active policy, keeping it consistent with the rolled-back configuration file.
Either register rollback handling that reruns update-crypto-policies with the
prior policy or move applyCryptoPolicy after all fallible update steps, while
preserving existing error propagation.
---
Nitpick comments:
In `@pkg/controller/template/kubelet_config_dir_test.go`:
- Line 49: Replace the unkeyed RenderConfig literals at both test call sites
with keyed field literals, explicitly assigning each value to the corresponding
RenderConfig fields, including the trailing CryptoPolicy and CryptoPolicySubMod
fields according to the struct definition. Preserve the existing values and
behavior.
In `@pkg/controller/template/render_test.go`:
- Line 85: Replace the positional RenderConfig literals in all listed
renderTemplate test call sites with keyed field literals, using the actual
RenderConfig field names. Preserve each call’s existing values while making
every argument explicit so future field additions or reordering cannot silently
change behavior.
In `@pkg/daemon/update_test.go`:
- Around line 984-1023: The TestApplyCryptoPolicy cases should avoid reading
host-specific /proc and crypto-policy files and should verify the intended
missing-binary path deterministically. Update applyCryptoPolicy and its callers
to use injectable paths or dependencies, configure those in
TestApplyCryptoPolicy for a controlled non-FIPS environment, and assert the
returned error content identifies the unavailable update-crypto-policies command
rather than only checking that an error occurred.
In `@pkg/daemon/update.go`:
- Around line 2656-2660: The crypto-policies config path is hardcoded,
preventing test isolation. Extract the path into a package-level variable near
fipsFile, update the ReadFile call in the crypto-policy application logic to use
that variable, and preserve the existing default path so TestApplyCryptoPolicy
can override it with a temporary file.
- Around line 2669-2673: Add context-based cancellation with an appropriate
timeout around the external command in the crypto-policy update flow, replacing
the unbounded runCmdSync call. Update the relevant helper or call path near the
policy application logic to accept and propagate context.Context, while
preserving the existing wrapped error and successful return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d46eccc3-835c-4337-a414-4e9c223b5255
📒 Files selected for processing (10)
pkg/controller/common/helpers.gopkg/controller/common/helpers_test.gopkg/controller/template/kubelet_config_dir_test.gopkg/controller/template/render.gopkg/controller/template/render_test.gopkg/controller/template/template_controller.gopkg/daemon/update.gopkg/daemon/update_test.gotemplates/common/_base/files/etc-crypto-policies-config.yamltemplates/common/_base/files/etc-crypto-policies-modules-tls13only-pmod.yaml
268846f to
d2220a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/common/helpers.go`:
- Around line 1301-1321: Update the cipher-suite mapping and its consuming
validation flow around the OpenSSL-to-node crypto-policy translation to reject
deprecated DES-CBC3/SHA1 suites, or emit the established warning and prevent
them from enabling policy entries. Apply this to both DES-CBC3-SHA and
ECDHE-RSA-DES-CBC3-SHA before they become cipher@TLS/mac@TLS values, while
preserving supported suite mappings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0b0b6827-58d0-417d-8ef5-d431b486eafe
📒 Files selected for processing (4)
pkg/controller/common/helpers.gopkg/controller/common/helpers_test.gopkg/controller/template/render_test.gotemplates/common/_base/files/etc-crypto-policies-modules-openshift-pmod.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/template/render_test.go
- pkg/controller/common/helpers_test.go
| // Stub out a test directory structure -- we need to create /etc so createOrigFile can use it | ||
| etcDir := filepath.Join(testDir, "etc") | ||
| err := os.MkdirAll(etcDir, 0755) | ||
| err := os.MkdirAll(etcDir, 0o755) |
There was a problem hiding this comment.
Note to reviewer: The only true change in this file is the addition of TestApplyCryptoPolicy. The other changes are just the result of formatting this document.
|
/test unit |
| return nil | ||
| } | ||
|
|
||
| if err := processFips(func(nodeFIPS bool) error { |
There was a problem hiding this comment.
Note to reviewer: FIPS check here was inspired by the checkFIPS function at
machine-config-operator/pkg/daemon/update.go
Lines 1633 to 1652 in cb061e1
| FileEmbedded1: ign3types.FileEmbedded1{Contents: ign3types.Resource{Source: &testfiledata}, Mode: &mode}} | ||
| tempFileOverwriteFalse := ign3types.File{Node: ign3types.Node{Path: "/etc/testfileconfig2", Overwrite: boolToPtr(false)}, | ||
| FileEmbedded1: ign3types.FileEmbedded1{Contents: ign3types.Resource{Source: &testfiledata}, Mode: &mode}} | ||
| tempFileNoDefault := ign3types.File{ |
There was a problem hiding this comment.
Note to reviewer: It looks like there are a lot of changes in this file, but that is because the formatting was fixed. The only true change is the addition of the TestGetCryptoPolicyFromTLSProfile test.
| // opensslCipherInfo maps OpenSSL cipher suite names to their decomposed | ||
| // Fedora crypto-policy components. The source of truth for which OpenSSL | ||
| // cipher names exist is library-go's openSSLToIANACiphersMap in | ||
| // github.com/openshift/library-go/pkg/crypto/crypto.go — this table must | ||
| // stay in sync with it. | ||
| var opensslCipherInfo = map[string]cipherComponents{ |
There was a problem hiding this comment.
Note to reviewer: There is a plan to automate keeping this list up to date in https://redhat.atlassian.net/browse/MCO-2444.
There was a problem hiding this comment.
The automation for this and the other automation comment seems like it could be pretty interesting!
There was a problem hiding this comment.
I was thinking about this one. Could us use a go generator to inject them at build time instead of maintaing a job?
There was a problem hiding this comment.
I think that's a fair option, but I think then we'd need some helpers to automatically split the cipher into the policy components. Maybe I can schedule a meeting for us to chat about options here?
| // tlsGroupToCryptoPolicy maps OpenShift TLSGroup enum values to Fedora | ||
| // crypto-policy group names. The canonical names are in the .pol files at | ||
| // https://gitlab.com/redhat-crypto/fedora-crypto-policies/-/tree/master/policies | ||
| var tlsGroupToCryptoPolicy = map[configv1.TLSGroup]string{ |
There was a problem hiding this comment.
Note to reviewer: There is a plan to automate keeping this list up to date in https://redhat.atlassian.net/browse/MCO-2445.
35f778a to
4d2b7a0
Compare
openshift-ci
Bot
added
the
needs-rebase
5b9ac89 to
5c8e2d2
Compare
openshift-ci
Bot
removed
the
needs-rebase
isabella-janssen
changed the title
tlsSecurityProfile options
openshift-ci-robot
added
the
jira/valid-reference
|
@isabella-janssen: This pull request references MCO-2441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
5c8e2d2 to
6d11a4d
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
/test unit |
|
/test e2e-gcp-op-ocl-part1 |
angelcerveraroldan
left a comment
angelcerveraroldan
left a comment
There was a problem hiding this comment.
Overall looks good to me! Just two questions.
… between OCP `TLSSecurityProfile` opinions and machine understandable policies
3b2f570 to
50988d6
Compare
|
/test unit |
pablintino
left a comment
pablintino
left a comment
There was a problem hiding this comment.
It makes sense to me. Waiting to hear what the crypto team has in mind.
Have we planned the testing side? (I'm sure you have, but I'm not sure what's the plan)
| // opensslCipherInfo maps OpenSSL cipher suite names to their decomposed | ||
| // Fedora crypto-policy components. The source of truth for which OpenSSL | ||
| // cipher names exist is library-go's openSSLToIANACiphersMap in | ||
| // github.com/openshift/library-go/pkg/crypto/crypto.go — this table must | ||
| // stay in sync with it. | ||
| var opensslCipherInfo = map[string]cipherComponents{ |
There was a problem hiding this comment.
I was thinking about this one. Could us use a go generator to inject them at build time instead of maintaing a job?
| configv1.VersionTLS10: {"TLS1.0", "TLS1.1", "DTLS1.0"}, | ||
| configv1.VersionTLS11: {"TLS1.0", "TLS1.1", "DTLS1.0"}, | ||
| configv1.VersionTLS12: {"TLS1.0", "TLS1.1", "DTLS1.0"}, | ||
| configv1.VersionTLS13: {"TLS1.0", "TLS1.1", "TLS1.2", "DTLS1.0", "DTLS1.2"}, |
There was a problem hiding this comment.
ack: Seems fine to have DTLS1.2 as discussed to allow DTLS1.3 to show up if added in Fedora.
@pablintino I am working on the testing at the moment - the idea is to run a subset of all the CoreOS tests with these policies enabled. So this should test that libraries that use backends such as openssl still pass all our tests with their modified configs. This would mean no testing for Custom, but I think that is fine, since Custom has a big "danger" warning attached to it :D |
cheesesashimi
left a comment
cheesesashimi
left a comment
There was a problem hiding this comment.
Overall, this looks good! My feedback is non-blocking; just some questions and general thoughts.
| return string(profileSpec.MinTLSVersion), crypto.OpenSSLToIANACipherSuites(profileSpec.Ciphers) | ||
| } | ||
|
|
||
| // cipherComponents holds the Fedora crypto-policy component names that an |
There was a problem hiding this comment.
suggestion (non-blocking): To keep the contents of this directory more organized, let's put this and the associated tests into a separate file called crypto.go or cryptopolicy.go / crypto_test.go or cryptopolicy_test.go, respectively.
| // opensslCipherInfo maps OpenSSL cipher suite names to their decomposed | ||
| // Fedora crypto-policy components. The source of truth for which OpenSSL | ||
| // cipher names exist is library-go's openSSLToIANACiphersMap in | ||
| // github.com/openshift/library-go/pkg/crypto/crypto.go — this table must | ||
| // stay in sync with it. | ||
| var opensslCipherInfo = map[string]cipherComponents{ |
There was a problem hiding this comment.
The automation for this and the other automation comment seems like it could be pretty interesting!
| macSet[info.mac] = struct{}{} | ||
| } | ||
|
|
||
| var lines []string |
There was a problem hiding this comment.
thought (non-blocking): Whenever I construct a multi-line string like this, I like to use the strings.Builder object in the stdlib. My reasoning is because it allows me to use the fmt.Fprint(), fmt.Fprintf(), and fmt.Fprintln() functions along with format directives in a clear way.
Just to be clear: I'm not asking you to change this, I'm just pointing it out for the future 😄.
| if err := processFips(func(nodeFIPS bool) error { | ||
| if nodeFIPS { | ||
| result.Insert( | ||
| "/etc/crypto-policies/config", |
There was a problem hiding this comment.
question (non-blocking): Would it make sense to make /etc/crypto-policies/config / /etc/crypto-policies/policies/modules/OPENSHIFT.pmod constants?
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, isabella-janssen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@isabella-janssen: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
5 participants
- What I did
This adds the main functionality updates needed to support PQC efforts in the MCO. This adds a translation layer between the OCP API's
TLSSecurityProfileoption to policies understood by the machines in a cluster.- How to verify it
To verify this, check that each OCP
TLSSecurityProfileaccurately maps to a correct policy on the machine for clusters based on RHEL9 and RHEL10. Note that TLS 1.0 and 1.1 are not supported on RHEL9 or RHEL10, so those TLS versions and any ciphers restricted to those versions will not be reflected in the machine's policy. On a cluster install, the machine should be adhering to the the standards set by theIntermediateprofile. To change between profiles, use commands of the following form:Note that FIPS clusters cannot have
TLSSecurityProfileoptions set, so any machines in FIPS clusters should be unaffected if a profile option is set and the machine should appropriately degrade.- Description for the changelog
MCO-2441: MCO-2442: MCO-2443: Support Openshift —> RHEL translation for
tlsSecurityProfileoptionsSummary by CodeRabbit
/etc/crypto-policies/configand (when provided) anOPENSHIFTpolicy module.