USHIFT-6925 USHIFT-6851: Introduce Post-Quantum Curves to Ingress defaults and FIPs detection#6622
USHIFT-6925 USHIFT-6851: Introduce Post-Quantum Curves to Ingress defaults and FIPs detection#6622eslutsky wants to merge 5 commits into
Conversation
Introduce detectFIPS() to check whether the cluster is running in FIPS mode via the FIPS_ENABLED env var or /proc/sys/crypto/fips_enabled. The result is stored in the package-level isFIPSEnabled variable for use by subsequent FIPS-aware configuration logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
On FIPS-enabled clusters, remove non-FIPS-compliant TLS 1.3 cipher suites (e.g. TLS_CHACHA20_POLY1305_SHA256) from ROUTER_CIPHERSUITES. HAProxy would fail TLS handshakes when a client offers a non-FIPS cipher that is listed in ssl-default-bind-ciphersuites but excluded by the OS FIPS policy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Set ROUTER_CURVES on the ingress router deployment to configure TLS supportedGroups. Non-FIPS clusters use X25519MLKEM768:X25519:P-256:P-384:P-521 (including post-quantum ML-KEM). FIPS clusters use P-256:P-384:P-521 only, since ML-KEM and X25519 are not supported by OpenSSL FIPS 140-3. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eslutsky 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 |
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds FIPS detection and, when enabled, filters TLS 1.3 cipher suites and removes non‑FIPS curves; exposes the resulting RouterTLSCurves to the router Deployment and adds a Robot test that verifies ML‑KEM curve negotiation. ChangesFIPS-Aware TLS Configuration
Sequence DiagramsequenceDiagram
participant System as System / Environment
participant Controller as Controller Logic
participant TLSConfig as generateIngressParams
participant Template as Deployment Template
System->>Controller: read FIPS_ENABLED or /proc/sys/crypto/fips_enabled
Controller->>Controller: set isFIPSEnabled
Controller->>TLSConfig: generateIngressParams(cfg, fipsEnabled)
alt FIPS Enabled
TLSConfig->>TLSConfig: filter tls13Ciphers to fipsApprovedTLS13Ciphers
TLSConfig->>TLSConfig: remove ML-KEM and X25519 from tlsCurves
else FIPS Disabled
TLSConfig->>TLSConfig: keep full cipher and curve lists
end
TLSConfig->>Controller: return tlsCurves
Controller->>Template: render with RouterTLSCurves
Template->>Template: inject ROUTER_CURVES env var
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." ... [truncated 29740 characters] ... elet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/components/controllers.go (1)
30-31: 💤 Low valuePackage-level FIPS detection executes at init time.
This is evaluated once when the package loads. Acceptable for production but makes unit testing harder—tests cannot easily inject different FIPS states. Consider exposing a test hook or making
isFIPSEnableda function if testability becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/components/controllers.go` around lines 30 - 31, The package currently initializes a package-level variable isFIPSEnabled by calling detectFIPS() at load time which hinders tests; change this to either (A) replace the variable with a function IsFIPSEnabled() that calls detectFIPS() (and update all call sites that reference isFIPSEnabled), or (B) keep a backed variable but add a test hook SetFIPSEnabledForTest(value bool) and use lazy evaluation (e.g., sync.Once) so tests can override it; update references to use the new function or the setter and ensure detectFIPS remains the production implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/components/controllers.go`:
- Around line 30-31: The package currently initializes a package-level variable
isFIPSEnabled by calling detectFIPS() at load time which hinders tests; change
this to either (A) replace the variable with a function IsFIPSEnabled() that
calls detectFIPS() (and update all call sites that reference isFIPSEnabled), or
(B) keep a backed variable but add a test hook SetFIPSEnabledForTest(value bool)
and use lazy evaluation (e.g., sync.Once) so tests can override it; update
references to use the new function or the setter and ensure detectFIPS remains
the production implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b3f7424d-471c-407c-ae3a-c14164ec1e36
📒 Files selected for processing (2)
assets/components/openshift-router/deployment.yamlpkg/components/controllers.go
eslutsky
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
/test all |
|
@eslutsky: This pull request references USHIFT-6925 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. |
eslutsky
changed the title
eslutsky
changed the title
|
@eslutsky: This pull request references USHIFT-6925 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. This pull request references USHIFT-6851 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. |
|
/test all |
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 `@test/suites/optional/tls-scanner.robot`:
- Around line 52-54: The test "Ingress Router TLS Curves supports ML-KEM Post
Quantum Curves" unconditionally asserts presence of the ML-KEM curve
X25519MLKEM768 but FIPS mode removes ML-KEM curves; update the test to check the
ROUTER_CURVES variable first (e.g., Run Keyword Unless '${X25519MLKEM768}' in
'${ROUTER_CURVES}' Skip Test "ML-KEM curves not present (FIPS mode)"), or
add a pre-check keyword that inspects ROUTER_CURVES for 'X25519MLKEM768' and
skips the test when not present before performing the openssl/negotiation
assertion. Ensure the same pre-check is applied to the related tests covering
lines 130-144.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6146a461-c936-4f2c-adfb-606034e53972
📒 Files selected for processing (1)
test/suites/optional/tls-scanner.robot
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
eslutsky
force-pushed
the
microshift-fips-detection
branch
from
6f1b76f to
9e1e64a
Compare
|
/test all |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-aws-tests-periodic |
pacevedom
left a comment
pacevedom
left a comment
There was a problem hiding this comment.
/retitle USHIFT-6925 USHIFT-6851: Introduce Post-Quantum Curves to Ingress defaults and FIPs detection
openshift-ci
Bot
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/components/controllers_test.go (2)
63-63: ⚡ Quick winAvoid panic-prone type assertions in test assertions.
At Line 63, Line 97, and Line 120, direct
.(string)assertions can panic and hide failure context. Prefer checked extraction withok+t.Fatalf(...).Proposed change
+func requireStringParam(t *testing.T, params map[string]interface{}, key string) string { + t.Helper() + v, ok := params[key] + if !ok { + t.Fatalf("missing param %q", key) + } + s, ok := v.(string) + if !ok { + t.Fatalf("param %q has type %T, want string", key, v) + } + return s +} + func TestGenerateIngressParamsFIPSCiphers(t *testing.T) { cfg := newTestConfig() @@ - cipherSuites := params["RouterCiphersSuites"].(string) + cipherSuites := requireStringParam(t, params, "RouterCiphersSuites") @@ - cipherSuites := params["RouterCiphersSuites"].(string) + cipherSuites := requireStringParam(t, params, "RouterCiphersSuites") @@ - curves := params["RouterTLSCurves"].(string) + curves := requireStringParam(t, params, "RouterTLSCurves") @@ - curves := params["RouterTLSCurves"].(string) + curves := requireStringParam(t, params, "RouterTLSCurves")Also applies to: 97-97, 120-120
🤖 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/components/controllers_test.go` at line 63, Replace the panic-prone direct type assertions like cipherSuites := params["RouterCiphersSuites"].(string) with checked extractions using the comma-ok form (e.g., v, ok := params["RouterCiphersSuites"]; s, ok := v.(string); if !ok { t.Fatalf("expected RouterCiphersSuites to be string, got %T", v) }) so tests fail gracefully with clear messages; apply the same pattern to the other assertions referenced (the similar direct .(string) uses in the same test around the RouterCiphersSuites/corresponding variables at the other two occurrences) and update the t.Fatalf messages to include the actual type/value for easier debugging.
55-56: ⚡ Quick winUse a fresh config per subtest to avoid hidden coupling.
At Line 55 and Line 89, shared
cfgacrosst.Runblocks can make tests order-dependent ifgenerateIngressParamsever mutates input.Proposed change
func TestGenerateIngressParamsFIPSCiphers(t *testing.T) { - cfg := newTestConfig() - t.Run("FIPS enabled filters non-FIPS TLS 1.3 ciphers", func(t *testing.T) { + cfg := newTestConfig() params, err := generateIngressParams(cfg, true) @@ t.Run("non-FIPS keeps all TLS 1.3 ciphers", func(t *testing.T) { + cfg := newTestConfig() params, err := generateIngressParams(cfg, false) @@ func TestGenerateIngressParamsFIPSCurves(t *testing.T) { - cfg := newTestConfig() - t.Run("FIPS enabled uses only NIST curves", func(t *testing.T) { + cfg := newTestConfig() params, err := generateIngressParams(cfg, true) @@ t.Run("non-FIPS includes PQC hybrid curve", func(t *testing.T) { + cfg := newTestConfig() params, err := generateIngressParams(cfg, false)Also applies to: 89-90
🤖 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/components/controllers_test.go` around lines 55 - 56, The tests share a single cfg created by newTestConfig() and pass it into multiple t.Run subtests, which can create order-dependent failures if generateIngressParams mutates cfg; update the test to create a fresh config inside each subtest (call newTestConfig() within each t.Run closure or otherwise deep-copy cfg before passing it to generateIngressParams) so each subtest gets an independent config instance; ensure references to cfg in both the subtest at line ~55 and the one at ~89 are replaced with per-subtest locals.
🤖 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.
Nitpick comments:
In `@pkg/components/controllers_test.go`:
- Line 63: Replace the panic-prone direct type assertions like cipherSuites :=
params["RouterCiphersSuites"].(string) with checked extractions using the
comma-ok form (e.g., v, ok := params["RouterCiphersSuites"]; s, ok :=
v.(string); if !ok { t.Fatalf("expected RouterCiphersSuites to be string, got
%T", v) }) so tests fail gracefully with clear messages; apply the same pattern
to the other assertions referenced (the similar direct .(string) uses in the
same test around the RouterCiphersSuites/corresponding variables at the other
two occurrences) and update the t.Fatalf messages to include the actual
type/value for easier debugging.
- Around line 55-56: The tests share a single cfg created by newTestConfig() and
pass it into multiple t.Run subtests, which can create order-dependent failures
if generateIngressParams mutates cfg; update the test to create a fresh config
inside each subtest (call newTestConfig() within each t.Run closure or otherwise
deep-copy cfg before passing it to generateIngressParams) so each subtest gets
an independent config instance; ensure references to cfg in both the subtest at
line ~55 and the one at ~89 are replaced with per-subtest locals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0a707ca9-bfd2-4d7a-a8af-973768114373
📒 Files selected for processing (3)
pkg/components/controllers.gopkg/components/controllers_test.gotest/suites/optional/tls-scanner.robot
🚧 Files skipped from review as they are similar to previous changes (2)
- test/suites/optional/tls-scanner.robot
- pkg/components/controllers.go
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
eslutsky
force-pushed
the
microshift-fips-detection
branch
from
b3be376 to
a9aaf1e
Compare
|
@eslutsky: 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. |
3 participants
Summary by CodeRabbit
New Features
Tests