crypto: add TLS group/curve mapping support#2347
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds TLS group-to-curve mappings in ChangesTLS Group Mapping
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: damdo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 (1)
pkg/crypto/crypto_test.go (1)
40-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
SecPto the group discovery prefixes inpkg/crypto/crypto_test.go:48soTestConstantMapscatches Go 1.26’stls.SecP256r1MLKEM768/tls.SecP384r1MLKEM1024constants and forces thegoTLSGroupsTODO to be updated.🤖 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/crypto/crypto_test.go` around lines 40 - 50, Add SecP to the group-prefix discovery in TestConstantMaps so the crypto test suite picks up Go 1.26’s new tls.SecP256r1MLKEM768 and tls.SecP384r1MLKEM1024 constants. Update the prefix checks in the loop over pkg.Scope().Names() alongside the existing CurveP and X25519 handling, so discoveredGroups includes these symbols and the goTLSGroups TODO will fail until it is updated.
🤖 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/crypto/crypto.go`:
- Around line 392-397: The doc comments for CurveIDForTLSGroup and
ValidTLSGroups are misleading for the PQ-hybrid groups because the returned
bool/list currently reflects mapped or known TLSGroup values, not actual Go
version support. Update the comments to describe that these APIs return
known/mapped groups (including the hybrid no-op IDs) rather than claiming
“supported by this Go version,” and keep the wording aligned with
tlsGroupToCurveID and ValidTLSGroups so callers don’t infer unsupported groups
are rejected.
---
Nitpick comments:
In `@pkg/crypto/crypto_test.go`:
- Around line 40-50: Add SecP to the group-prefix discovery in TestConstantMaps
so the crypto test suite picks up Go 1.26’s new tls.SecP256r1MLKEM768 and
tls.SecP384r1MLKEM1024 constants. Update the prefix checks in the loop over
pkg.Scope().Names() alongside the existing CurveP and X25519 handling, so
discoveredGroups includes these symbols and the goTLSGroups TODO will fail until
it is updated.
🪄 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: e6fcba51-5d2f-4da6-9863-513f119e4e68
📒 Files selected for processing (2)
pkg/crypto/crypto.gopkg/crypto/crypto_test.go
Map OpenShift API TLSGroup constants to Go's tls.CurveID values, following the same pattern used for cipher suite mapping. This enables downstream consumers (observer, operators) to resolve curve preferences from TLS security profiles. SecP256r1MLKEM768 and SecP384r1MLKEM1024 use raw IANA IDs (4587, 4590) because Go 1.25 does not yet define named constants for them. Go silently ignores CurveIDs it does not implement, so these are no-ops until Go 1.26.
damdo
force-pushed
the
tls-curve-preferences-crypto
branch
from
41e0932 to
fdc7682
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 @sanchezl @davidesalerno @joelanford @ardaguclu |
| "X25519": tls.X25519, // IANA 29 | ||
| "X25519MLKEM768": tls.X25519MLKEM768, // IANA 4588 | ||
| // TODO: add "SecP256r1MLKEM768": tls.SecP256r1MLKEM768 (IANA 4587) when Go 1.26 is the minimum version | ||
| // TODO: add "SecP384r1MLKEM1024": tls.SecP384r1MLKEM1024 (IANA 4590) when Go 1.26 is the minimum version |
There was a problem hiding this comment.
| // TODO: add "SecP384r1MLKEM1024": tls.SecP384r1MLKEM1024 (IANA 4590) when Go 1.26 is the minimum version | |
| // TODO: add "SecP384r1MLKEM1024": tls.SecP384r1MLKEM1024 (IANA 4589) when Go 1.26 is the minimum version |
https://www.ietf.org/archive/id/draft-ietf-tls-ecdhe-mlkem-01.html#name-secp384r1mlkem1024
probably safer to document as 4589 - unless I'm incorrect and this is something specific to Go
| if strings.HasPrefix(declName, "TLS_RSA_") || strings.HasPrefix(declName, "TLS_ECDHE_") || strings.HasPrefix(declName, "TLS_AES_") || strings.HasPrefix(declName, "TLS_CHACHA20_") { | ||
| discoveredCiphers[declName] = true | ||
| } | ||
| if strings.HasPrefix(declName, "CurveP") || strings.HasPrefix(declName, "X25519") { |
There was a problem hiding this comment.
Does this filter need SecP and MLKEM prefixes? Or will that be updated when addressing the TODOs for go 1.26 >= ?
| "CurveP521": tls.CurveP521, // IANA 25 | ||
| "X25519": tls.X25519, // IANA 29 | ||
| "X25519MLKEM768": tls.X25519MLKEM768, // IANA 4588 | ||
| // TODO: add "SecP256r1MLKEM768": tls.SecP256r1MLKEM768 (IANA 4587) when Go 1.26 is the minimum version |
There was a problem hiding this comment.
Might be antipattern, but we could reduce all these TODOs by creating a separate file with something like:
//go:build go1.26
package crypto
import (
"crypto/tls"
configv1 "github.com/openshift/api/config/v1"
)
func init() {
goTLSGroups["SecP256r1MLKEM768"] = tls.SecP256r1MLKEM768
goTLSGroups["SecP384r1MLKEM1024"] = tls.SecP384r1MLKEM1024
tlsGroupToCurveID[configv1.TLSGroupSecP256r1MLKEM768] = tls.SecP256r1MLKEM768
tlsGroupToCurveID[configv1.TLSGroupSecP384r1MLKEM1024] = tls.SecP384r1MLKEM1024
}Not sure if we normally leverage this in the project, just trying to think of ways to not have to come back to the TODOs 😄
There was a problem hiding this comment.
Seems like a good idea to me. This would make it possible for projects using older go to continue using newer library-go.
| discoveredVersions := map[string]bool{} | ||
| discoveredCiphers := map[string]bool{} | ||
| discoveredGroups := map[string]bool{} | ||
| for _, declName := range pkg.Scope().Names() { |
There was a problem hiding this comment.
I wonder if we should have a default kind of case here, where we register all other known unrelated constants in the package. That way if a new constant is added that doesn't match one of our existing patterns, this test fails and we are forced to decide:
- is this a new version?
- is this a new cipher?
- is this a new group?
- is it unrelated
And add it to the respective set.
6 participants
Map OpenShift API TLSGroup constants to Go's tls.CurveID values, following the same pattern used for cipher suite mapping. This enables downstream consumers (observer, operators) to resolve curve preferences from TLS security profiles.
SecP256r1MLKEM768 and SecP384r1MLKEM1024 use raw IANA IDs (4587, 4590) because Go 1.25 does not yet define named constants for them. Go silently ignores CurveIDs it does not implement, so these are no-ops until Go 1.26.
Summary by CodeRabbit