PROJQUAY-10703: feat(tls): propagate PQC groups for Modern profiles - #1303
PROJQUAY-10703: feat(tls): propagate PQC groups for Modern profiles#1303Marcusk19 wants to merge 2 commits into
Conversation
|
@Marcusk19: This pull request references [Jira Issue PROJQUAY-10703](https://redhat.atlassian.net/browse/PROJQUAY-10703), which is invalid:
Comment 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. |
|
Note
|
| Layer / File(s) | Summary |
|---|---|
TLS profile derivation and override handling pkg/context/context.go, controllers/quay/tls.go, controllers/quay/tls_test.go |
Modern API server TLS profiles derive X25519MLKEM768:X25519:prime256v1; user overrides and non-Modern profiles leave SSLECDHCurves empty. |
Generated configuration injection pkg/kustomize/kustomize.go, pkg/kustomize/kustomize_test.go |
SSL_ECDH_CURVES is conditionally rendered in generated config.yaml from the derived curve list. |
Modern profile integration validation test/chainsaw/tls_security_profile/chainsaw-test.yaml |
The integration test verifies Modern profiles include SSL_ECDH_CURVES and X25519MLKEM768. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Possibly related issues
- tls_test: add missing TLS 1.2 cipher assertions for Old and nil-Custom profile test cases #1297 — Extends TLS profile and generated configuration test coverage.
Possibly related PRs
- quay/quay-operator#1279 — Updates the same TLS profile handling flow.
- quay/quay-operator#1296 — Adds adjacent TLS-profile-derived configuration through the same context and Kustomize flow.
Suggested reviewers: jbpratt
🚥 Pre-merge checks | ✅ 9
✅ Passed checks (9 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title starts with the required Jira key and accurately summarizes the Modern TLS PQC groups change. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Crd Backward Compatibility | ✅ Passed | No files under apis/quay/v1 were modified; the PR only changes controller, context, kustomize, and tests. |
| No Weak Cryptographic Algorithms | ✅ Passed | Modified code only adds modern TLS supported groups and existing AES-GCM/ChaCha20 suites; no weak algorithms or InsecureSkipVerify found. |
| No Privileged Containers | ✅ Passed | No touched manifest or workload template sets privileged, hostPID, hostNetwork, hostIPC, or SYS_ADMIN; only restricted securityContexts are present. |
| No Sensitive Data In Logs | ✅ Passed | No new log/print paths expose secret values; the touched TLS code adds no logging, and existing kustomize logs only mention key names or operational status. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Comment @coderabbitai help to get the list of available commands.
|
@Marcusk19: This pull request references [Jira Issue PROJQUAY-10703](https://redhat.atlassian.net/browse/PROJQUAY-10703), which is invalid:
Comment 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@controllers/quay/tls.go`:
- Around line 67-69: Update the user-override guard in the TLS configuration
flow to return early when config["SSL_ECDH_CURVES"] is present, alongside the
existing TLS key checks. Ensure qctx.SSLECDHCurves is not populated from the
APIServer profile when this user override is supplied.
In `@test/chainsaw/tls_security_profile/chainsaw-test.yaml`:
- Around line 89-100: Update the PROFILE_TYPE lookup in the Modern-profile
validation block to fail the test when kubectl cannot read apiserver/cluster,
instead of falling back to an empty value and skipping validation. Preserve the
existing checks for SSL_ECDH_CURVES and X25519MLKEM768 once the profile type is
retrieved.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ada2d4c5-6073-487d-ab8d-695e7d6ee66c
📒 Files selected for processing (5)
controllers/quay/tls.gocontrollers/quay/tls_test.gopkg/context/context.gopkg/kustomize/kustomize.gotest/chainsaw/tls_security_profile/chainsaw-test.yaml
5a7dae3 to
67fdae8
Compare
|
@Marcusk19: This pull request references [Jira Issue PROJQUAY-10703](https://redhat.atlassian.net/browse/PROJQUAY-10703), which is invalid:
Comment 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. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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/kustomize/kustomize_test.go`:
- Around line 961-962: Update the test around Inflate to use require.NoError or
t.Fatalf instead of assert.NoError, so execution stops immediately when Inflate
fails before iterating pieces. Keep the existing successful-path assertions
unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ca1e8fcc-f902-4a4a-a1b9-102bc8b1ac02
📒 Files selected for processing (6)
controllers/quay/tls.gocontrollers/quay/tls_test.gopkg/context/context.gopkg/kustomize/kustomize.gopkg/kustomize/kustomize_test.gotest/chainsaw/tls_security_profile/chainsaw-test.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- test/chainsaw/tls_security_profile/chainsaw-test.yaml
- controllers/quay/tls_test.go
- pkg/kustomize/kustomize.go
- controllers/quay/tls.go
|
@Marcusk19: This pull request references [Jira Issue PROJQUAY-10703](https://redhat.atlassian.net/browse/PROJQUAY-10703), which is invalid:
Comment 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/kustomize/kustomize_test.go (1)
956-975: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover renderer-level override precedence.
This test verifies injection only. Add a case where
config.yamlalready containsSSL_ECDH_CURVESand assertInflatepreserves that value; the controller override test cannot catch a regression inpkg/kustomize/kustomize.go’s independent rendering guard.🤖 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/kustomize/kustomize_test.go` around lines 956 - 975, The TestInflateInjectsSSLECDHCurves test only verifies injection and must also cover renderer-level precedence. Add a case with SSL_ECDH_CURVES already present in config.yaml, run Inflate, and assert the existing value is preserved rather than replaced by the context override, targeting the independent rendering guard in Inflate.
🤖 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/kustomize/kustomize_test.go`:
- Around line 956-975: The TestInflateInjectsSSLECDHCurves test only verifies
injection and must also cover renderer-level precedence. Add a case with
SSL_ECDH_CURVES already present in config.yaml, run Inflate, and assert the
existing value is preserved rather than replaced by the context override,
targeting the independent rendering guard in Inflate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3f967879-60c0-4ab7-91e2-bdca86c18487
📒 Files selected for processing (6)
controllers/quay/tls.gocontrollers/quay/tls_test.gopkg/context/context.gopkg/kustomize/kustomize.gopkg/kustomize/kustomize_test.gotest/chainsaw/tls_security_profile/chainsaw-test.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/kustomize/kustomize.go
- test/chainsaw/tls_security_profile/chainsaw-test.yaml
- controllers/quay/tls_test.go
- pkg/context/context.go
2 participants
Summary
Propagate the post-quantum TLS supported groups required by Quay when the OpenShift cluster TLS security profile is
Modern.Root Cause / Rationale
The operator already propagated TLS versions and ciphers/ciphersuites, but it did not configure Quay nginx's supported groups. Consequently, a Quay image with ML-KEM-capable OpenSSL could still fail an
X25519MLKEM768handshake unlessSSL_ECDH_CURVESwas manually supplied.Changes
SSLECDHCurvestoQuayRegistryContext.Modernprofile toX25519MLKEM768:X25519:prime256v1.SSL_ECDH_CURVESinto generated Quay configuration when not user-specified.X25519MLKEM768for Modern.Test Plan
KUBEBUILDER_ASSETS=$PWD/bin/k8s/1.28.0-darwin-arm64 go test ./controllers/quay ./pkg/kustomize ./pkg/contextgo vet ./controllers/quay ./pkg/kustomize ./pkg/contextX25519MLKEM768and completed authenticated registry push/pull.JIRA Link
https://issues.redhat.com/browse/PROJQUAY-10703
Backport
Not required; this targets the current development line and depends on the existing TLS security profile integration.
Summary by CodeRabbit
New Features
SSL_ECDH_CURVESinto the generated Quay TLS config.X25519MLKEM768).Bug Fixes
SSL_ECDH_CURVESinconfig.yamlwithout overriding.SSL_ECDH_CURVESfor non-Modern/default profiles.Tests
SSL_ECDH_CURVESinjection and precedence.