feat: implement TLS groups/curve preferences support#22
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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
damdo
changed the title
|
/hold This PR depends on openshift/library-go#2347 merging upstream. Once that PR is merged and released, we can drop the temporary replace directive and revendor to use the official library-go version. |
openshift-ci
Bot
added
the
do-not-merge/hold
damdo
force-pushed
the
implement-curve-preferences
branch
from
d584c5e to
79a541a
Compare
|
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 ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThis pull request updates go.mod dependency versions, including a bump to openshift/api, refreshed go-openapi/swag indirect dependencies, removal of josharian/intern and mailru/easyjson, and updates to various other indirect dependencies and the library-go replace directive. Separately, NewTLSConfigFromProfile in pkg/tls/tls.go now derives tls.Config.CurvePreferences from profile.Groups, setting it only when preferences exist, and returns a combined list of unsupported cipher and curve group names via a renamed return value. Corresponding test cases were added covering supported groups, unsupported groups, empty groups, and Intermediate profile behavior. Sequence Diagram(s)Not applicable; changes are dependency updates and configuration logic without a multi-component interaction flow warranting a sequence diagram. Related Issues: #20 Related PRs: None identified. Suggested labels: dependencies, enhancement Suggested reviewers: damdo 🐰 A curve, a group, a cipher's name, 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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: 1
🧹 Nitpick comments (2)
pkg/tls/tls_test.go (1)
357-371: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the conditional assertion in the Intermediate-profile Groups test.
The test only asserts
CurvePreferencesis setif len(profile.Groups) > 0; if the Intermediate profile definition ever ships with an emptyGroupslist (e.g., upstream API regression), this test silently passes without verifying anything.✅ Proposed fix to make the test deterministic
It("should set CurvePreferences from the profile", func() { profile := *configv1.TLSProfiles[configv1.TLSProfileIntermediateType] + Expect(profile.Groups).NotTo(BeEmpty(), "Intermediate profile is expected to define Groups") tlsConfigFn, _ := NewTLSConfigFromProfile(profile) tlsConf := &tls.Config{} tlsConfigFn(tlsConf) - // Intermediate profile now includes Groups - if len(profile.Groups) > 0 { - Expect(tlsConf.CurvePreferences).NotTo(BeNil()) - } + Expect(tlsConf.CurvePreferences).NotTo(BeNil()) })🤖 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/tls/tls_test.go` around lines 357 - 371, The Intermediate profile Groups test in NewTLSConfigFromProfile is conditional on len(profile.Groups), so it can pass without checking anything if Groups is empty. Make the test deterministic by asserting the expected Intermediate profile data explicitly before calling tlsConfigFn, then always verify tlsConf.CurvePreferences is populated from the profile’s Groups rather than skipping the assertion.pkg/tls/tls.go (1)
119-121: 🧹 Nitpick | 🔵 TrivialPost-quantum curve gap noted and tracked.
The TODO correctly flags that post-quantum hybrid curves (SecP256r1MLKEM768, SecP384r1MLKEM1024) are not supported in Go 1.25 and are gracefully filtered as unsupported. Since this shared TLS helper may end up protecting long-lived secrets across consuming controller-runtime projects, worth ensuring downstream consumers are aware of the current PQ gap until Go 1.26+ lands support.
As per path instructions for files matching
**/*{crypt,cipher,sign,hash,tls,ssl,cert,key,token}*, "Post-quantum: flag if protecting long-lived secrets."🤖 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/tls/tls.go` around lines 119 - 121, The TLS helper in CurveIDsForTLSGroups already filters unsupported post-quantum hybrid curves, but the comment needs to make the downstream exposure explicit for consumers handling long-lived secrets. Update the TODO/documentation around curvePrefs and unsupportedGroups in tls.go to clearly call out that SecP256r1MLKEM768 and SecP384r1MLKEM1024 are currently unavailable in Go 1.25, that they are being dropped by CurveIDsForTLSGroups, and that controller-runtime consumers should not rely on PQ protection until Go 1.26+ support is available.Source: Path instructions
🤖 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/tls/tls.go`:
- Around line 116-137: Normalize the unsupported group names returned from
NewTLSConfigFromProfile so they match the expected lowercase underscore format
instead of the raw enum string. Update the unsupported-group handling in
CurveIDsForTLSGroups (or immediately after its result is used in
NewTLSConfigFromProfile) to map values like SecP256r1MLKEM768 to
secp256r1_mlkem768 before appending to unsupported, while keeping
unsupportedCiphers behavior unchanged.
---
Nitpick comments:
In `@pkg/tls/tls_test.go`:
- Around line 357-371: The Intermediate profile Groups test in
NewTLSConfigFromProfile is conditional on len(profile.Groups), so it can pass
without checking anything if Groups is empty. Make the test deterministic by
asserting the expected Intermediate profile data explicitly before calling
tlsConfigFn, then always verify tlsConf.CurvePreferences is populated from the
profile’s Groups rather than skipping the assertion.
In `@pkg/tls/tls.go`:
- Around line 119-121: The TLS helper in CurveIDsForTLSGroups already filters
unsupported post-quantum hybrid curves, but the comment needs to make the
downstream exposure explicit for consumers handling long-lived secrets. Update
the TODO/documentation around curvePrefs and unsupportedGroups in tls.go to
clearly call out that SecP256r1MLKEM768 and SecP384r1MLKEM1024 are currently
unavailable in Go 1.25, that they are being dropped by CurveIDsForTLSGroups, and
that controller-runtime consumers should not rely on PQ protection until Go
1.26+ support is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| func NewTLSConfigFromProfile(profile configv1.TLSProfileSpec) (tlsConfig func(*tls.Config), unsupported []string) { | ||
| minVersion := libgocrypto.TLSVersionOrDie(string(profile.MinTLSVersion)) | ||
| cipherSuites, unsupportedCiphers := cipherCodes(profile.Ciphers) | ||
| // TODO: post-quantum hybrid curves (SecP256r1MLKEM768, SecP384r1MLKEM1024) are not supported in Go 1.25. | ||
| // These will be returned as unsupported and gracefully filtered. Update this when Go 1.26+ support is available. | ||
| curvePrefs, unsupportedGroups := libgocrypto.CurveIDsForTLSGroups(profile.Groups) | ||
|
|
||
| unsupported = append(unsupportedCiphers, unsupportedGroups...) | ||
|
|
||
| return func(tlsConf *tls.Config) { | ||
| tlsConf.MinVersion = minVersion | ||
| // TODO: add curve preferences from profile once https://github.com/openshift/api/pull/2583 merges. | ||
| // tlsConf.CurvePreferences <<<<<< profile.Curves | ||
| if len(curvePrefs) > 0 { | ||
| tlsConf.CurvePreferences = curvePrefs | ||
| } | ||
|
|
||
| // TLS 1.3 cipher suites are not configurable in Go (https://github.com/golang/go/issues/29349), so only set CipherSuites accordingly. | ||
| // TODO: revisit this once we get an answer on the best way to handle this here: | ||
| // https://docs.google.com/document/d/1cMc9E8psHfnoK06ntR8kHSWB8d3rMtmldhnmM4nImjs/edit?disco=AAABu_nPcYg | ||
| if minVersion != tls.VersionTLS13 { | ||
| tlsConf.CipherSuites = cipherSuites | ||
| } | ||
| }, unsupportedCiphers | ||
| }, unsupported |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the vendored library-go crypto package and inspect CurveIDsForTLSGroups signature
fd -a 'crypto.go' --path '*library-go/pkg/crypto*'
rg -n -A 15 'func CurveIDsForTLSGroups' -g '*.go'Repository: openshift/controller-runtime-common
Length of output: 405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== go.mod references =='
rg -n 'library-go|CurveIDsForTLSGroups|TLSGroup|TLSProfileSpec' go.mod go.sum vendor . || true
echo
echo '== locate tls.go =='
fd -a 'tls.go' pkg || true
echo
echo '== outline pkg/tls/tls.go =='
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline pkg/tls/tls.go --view expanded || true
fi
echo
echo '== relevant lines in pkg/tls/tls.go =='
nl -ba pkg/tls/tls.go | sed -n '1,220p'Repository: openshift/controller-runtime-common
Length of output: 26899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== dependency metadata =='
rg -n 'openshift/library-go|library-go' go.mod go.sum vendor . || true
echo
echo '== search for CurveIDsForTLSGroups in repo =='
rg -n 'CurveIDsForTLSGroups|TLSGroup' . || true
echo
echo '== inspect pkg/tls tests =='
fd -a 'tls_test.go' pkg || true
if [ -f pkg/tls/tls_test.go ]; then
nl -ba pkg/tls/tls_test.go | sed -n '1,260p'
fiRepository: openshift/controller-runtime-common
Length of output: 9811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== vendor CurveIDsForTLSGroups implementation =='
sed -n '392,420p' vendor/github.com/openshift/library-go/pkg/crypto/crypto.go
echo
echo '== tls tests around unsupported groups =='
sed -n '286,360p' pkg/tls/tls_test.goRepository: openshift/controller-runtime-common
Length of output: 3714
Normalize unsupported group names
CurveIDsForTLSGroups appends string(group) for unsupported entries, so configv1.TLSGroupSecP256r1MLKEM768 is reported as SecP256r1MLKEM768, not secp256r1_mlkem768. If the lowercase underscore form is required here, normalize before returning.
🤖 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/tls/tls.go` around lines 116 - 137, Normalize the unsupported group names
returned from NewTLSConfigFromProfile so they match the expected lowercase
underscore format instead of the raw enum string. Update the unsupported-group
handling in CurveIDsForTLSGroups (or immediately after its result is used in
NewTLSConfigFromProfile) to map values like SecP256r1MLKEM768 to
secp256r1_mlkem768 before appending to unsupported, while keeping
unsupportedCiphers behavior unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tls/tls_test.go (1)
357-371: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWeak assertion: test can pass without exercising anything.
The assertion is gated behind
if len(profile.Groups) > 0, so if the Intermediate profile'sGroupsfield is ever empty (e.g., due to anopenshift/apiregression), this test silently passes without validating any behavior.🧪 Proposed fix to assert the precondition
Context("when using Intermediate profile with Groups", func() { It("should set CurvePreferences from the profile", func() { profile := *configv1.TLSProfiles[configv1.TLSProfileIntermediateType] + Expect(profile.Groups).NotTo(BeEmpty()) tlsConfigFn, _ := NewTLSConfigFromProfile(profile) tlsConf := &tls.Config{} tlsConfigFn(tlsConf) - // Intermediate profile now includes Groups - if len(profile.Groups) > 0 { - Expect(tlsConf.CurvePreferences).NotTo(BeNil()) - } + Expect(tlsConf.CurvePreferences).NotTo(BeNil()) }) })🤖 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/tls/tls_test.go` around lines 357 - 371, The test in tls_test.go has a weak assertion because it only checks CurvePreferences when profile.Groups is non-empty, so it can silently pass without validating behavior. In the Context for NewTLSConfigFromProfile with configv1.TLSProfileIntermediateType, assert the precondition on profile.Groups directly (for example, require it to be non-empty before checking tlsConf.CurvePreferences) so the test fails if the Intermediate profile no longer includes groups, and keep the existing CurvePreferences expectation tied to that same setup.
🤖 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/tls/tls_test.go`:
- Around line 357-371: The test in tls_test.go has a weak assertion because it
only checks CurvePreferences when profile.Groups is non-empty, so it can
silently pass without validating behavior. In the Context for
NewTLSConfigFromProfile with configv1.TLSProfileIntermediateType, assert the
precondition on profile.Groups directly (for example, require it to be non-empty
before checking tlsConf.CurvePreferences) so the test fails if the Intermediate
profile no longer includes groups, and keep the existing CurvePreferences
expectation tied to that same setup.
Bump openshift/api to pick up the TLS Groups field in TLSProfileSpec and TLSGroup type/constants added in openshift/api#2583. Add a replace directive for library-go pointing to the damdo fork which implements CurveIDsForTLSGroups.
Use libgocrypto.CurveIDsForTLSGroups to map TLS groups from the profile spec to Go tls.CurveID values, and set CurvePreferences on the TLS config when groups are specified. Fixes openshift#20
Test supported groups, unsupported groups (post-quantum hybrids), empty groups, and predefined Intermediate profile with groups.
damdo
force-pushed
the
implement-curve-preferences
branch
from
79a541a to
fc0245f
Compare
|
@damdo: 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. |
|
/assign @joelanford |
| Expect(tlsConf.CurvePreferences).To(ContainElement(tls.X25519)) | ||
| Expect(tlsConf.CurvePreferences).To(ContainElement(tls.CurveP256)) |
There was a problem hiding this comment.
Should we promise anything about the order of the groups?
| if len(profile.Groups) > 0 { | ||
| Expect(tlsConf.CurvePreferences).NotTo(BeNil()) | ||
| } |
There was a problem hiding this comment.
Should we expect the lengths to be equal?
2 participants
Summary
Implements TLS groups/curve preferences support in
NewTLSConfigFromProfile()by leveraging the library-go crypto package.Fixes #20
libgocrypto.CurveIDsForTLSGroups()to map TLS groups/curve preferences to Go curve IDsSample PR that leverages this is here: openshift/cluster-machine-approver#308
Test plan
TODO
replace github.com/openshift/library-godirective in go.mod and revendor once crypto: add TLS group/curve mapping support library-go#2347 merges