🌱 refactor(tls) use IANA cipher suite names instead of OpenSSL names - #218
Conversation
WalkthroughThe pull request updates TLS cipher suite handling in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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/tls/config.go (1)
163-171: Consider caching the combined cipher suite list (optional).The
append(tls.CipherSuites(), tls.InsecureCipherSuites()...)on line 165 allocates a new slice on each call. SinceCipherSuitesToStringiterates over the suites slice, this results in O(n*m) allocations where n is the number of suites to convert and m is the total cipher count.For typical use cases (logging, flag generation), this is negligible. If performance becomes a concern, consider caching:
♻️ Optional: Cache the cipher suite list
+var allCipherSuites = func() []*tls.CipherSuite { + return append(tls.CipherSuites(), tls.InsecureCipherSuites()...) +}() + // cipherIDToName converts a cipher suite ID to its IANA name. func cipherIDToName(id uint16) string { - for _, s := range append(tls.CipherSuites(), tls.InsecureCipherSuites()...) { + for _, s := range allCipherSuites { if s.ID == id { return s.Name } } return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/config.go` around lines 163 - 171, The current cipherIDToName function repeatedly builds a combined slice with append(tls.CipherSuites(), tls.InsecureCipherSuites()...) on every call causing repeated allocations; to fix, create a cached combined slice (e.g., a package-level variable populated once in init or via sync.Once) and have cipherIDToName iterate that cached slice instead; reference cipherIDToName and any callers like CipherSuitesToString so they use the cached combined list rather than rebuilding it each call.
🤖 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/tls/config.go`:
- Around line 163-171: The current cipherIDToName function repeatedly builds a
combined slice with append(tls.CipherSuites(), tls.InsecureCipherSuites()...) on
every call causing repeated allocations; to fix, create a cached combined slice
(e.g., a package-level variable populated once in init or via sync.Once) and
have cipherIDToName iterate that cached slice instead; reference cipherIDToName
and any callers like CipherSuitesToString so they use the cached combined list
rather than rebuilding it each call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28a42813-fc65-4cdb-b7cd-a609db0bc516
📒 Files selected for processing (5)
pkg/tls/README.mdpkg/tls/cipher.gopkg/tls/config.gopkg/tls/configmap.gopkg/tls/tls_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/tls/config.go (1)
186-191: Avoid rebuilding cipher-suite lists on every ID lookup.
cipherIDToNamecurrently allocates and scans repeatedly. A package-level ID→name map built once will reduce allocations and simplify lookups.Refactor sketch
+var cipherSuiteNameByID = func() map[uint16]string { + m := map[uint16]string{} + for _, s := range tls.CipherSuites() { + m[s.ID] = s.Name + } + for _, s := range tls.InsecureCipherSuites() { + m[s.ID] = s.Name + } + return m +}() + func cipherIDToName(id uint16) string { - for _, s := range append(tls.CipherSuites(), tls.InsecureCipherSuites()...) { - if s.ID == id { - return s.Name - } - } - return "" + return cipherSuiteNameByID[id] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/config.go` around lines 186 - 191, cipherIDToName currently rebuilds and scans the cipher-suite slices on every call; replace that by adding a package-level map[uint16]string (e.g., cipherIDNameMap) that is populated once (in init() or via sync.Once) by iterating tls.CipherSuites() and tls.InsecureCipherSuites(), then change cipherIDToName to simply look up id in cipherIDNameMap and return the name (or empty string if missing). Ensure the map population handles duplicate IDs by first-write wins or overwriting consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/tls/config.go`:
- Around line 106-108: The check in ConfigFromFlags treats only raw strings so
whitespace-only inputs like " " bypass the early-return; trim whitespace from
minVersion and cipherSuites (e.g., use strings.TrimSpace or equivalent) before
the if-check and use the trimmed values for the rest of parsing so that
whitespace-only flags are treated as empty and the function can return nil, nil
as intended.
In `@pkg/tls/tls_test.go`:
- Around line 146-148: The test currently only checks cipher-suite length when
tc.expectedLen > 0 which can miss regressions; update the assertion in the test
to always compare len(cfg.CipherSuites) to tc.expectedLen (remove the `if
tc.expectedLen > 0` guard) and call t.Errorf("expected %d cipher suites, got
%d", tc.expectedLen, len(cfg.CipherSuites)) on mismatch; apply the same change
to the other identical check that references tc.expectedLen and cfg.CipherSuites
elsewhere in the file.
---
Nitpick comments:
In `@pkg/tls/config.go`:
- Around line 186-191: cipherIDToName currently rebuilds and scans the
cipher-suite slices on every call; replace that by adding a package-level
map[uint16]string (e.g., cipherIDNameMap) that is populated once (in init() or
via sync.Once) by iterating tls.CipherSuites() and tls.InsecureCipherSuites(),
then change cipherIDToName to simply look up id in cipherIDNameMap and return
the name (or empty string if missing). Ensure the map population handles
duplicate IDs by first-write wins or overwriting consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 268d45a4-351d-440d-af18-07706065a986
📒 Files selected for processing (2)
pkg/tls/config.gopkg/tls/tls_test.go
| if tc.expectedLen > 0 && len(cfg.CipherSuites) != tc.expectedLen { | ||
| t.Errorf("expected %d cipher suites, got %d", tc.expectedLen, len(cfg.CipherSuites)) | ||
| } |
There was a problem hiding this comment.
Always assert cipher-suite length, including expected zero.
The current expectedLen > 0 guard can hide regressions where suites are unexpectedly populated.
Proposed fix
- if tc.expectedLen > 0 && len(cfg.CipherSuites) != tc.expectedLen {
+ if len(cfg.CipherSuites) != tc.expectedLen {
t.Errorf("expected %d cipher suites, got %d", tc.expectedLen, len(cfg.CipherSuites))
}- if tc.expectedLen > 0 && len(cfg.CipherSuites) != tc.expectedLen {
+ if len(cfg.CipherSuites) != tc.expectedLen {
t.Errorf("expected %d cipher suites, got %d", tc.expectedLen, len(cfg.CipherSuites))
}Also applies to: 538-540
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/tls/tls_test.go` around lines 146 - 148, The test currently only checks
cipher-suite length when tc.expectedLen > 0 which can miss regressions; update
the assertion in the test to always compare len(cfg.CipherSuites) to
tc.expectedLen (remove the `if tc.expectedLen > 0` guard) and call
t.Errorf("expected %d cipher suites, got %d", tc.expectedLen,
len(cfg.CipherSuites)) on mismatch; apply the same change to the other identical
check that references tc.expectedLen and cfg.CipherSuites elsewhere in the file.
Switch cipherMap keys and cipherIDToName output from OpenSSL-style (e.g. ECDHE-RSA-AES128-GCM-SHA256) to IANA format (e.g. TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256), which matches what Go's crypto/tls package uses. cipherIDToName now looks up the name from tls.CipherSuites() and tls.InsecureCipherSuites() by ID instead of reverse-iterating cipherMap, so the returned name is always the authoritative Go/IANA name. Update tests and README accordingly. Signed-off-by: zhujian <jiazhu@redhat.com>
Replace the hand-maintained cipherMap allowlist with direct lookups against tls.CipherSuites() and tls.InsecureCipherSuites(): - Secure ciphers are accepted silently - Insecure ciphers are accepted but logged via klog.Warningf - Unrecognized names are still rejected (existing behavior) This removes the maintenance burden of keeping cipherMap in sync with Go's cipher suite lists, and automatically picks up new secure ciphers added in future Go releases. Previously excluded insecure ciphers (RC4-based, TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA) are now accepted with a warning instead of being silently rejected. Signed-off-by: zhujian <jiazhu@redhat.com>
0ff1f46 to
2596f85
Compare
zhujian7
changed the title
zhujian7
changed the title
Replace per-call tls.CipherSuites()/InsecureCipherSuites() iterations with three package-level maps built in init(): secureCiphersByName map[string]uint16 -- name→ID for secure suites insecureCiphersByName map[string]uint16 -- name→ID for insecure suites cipherNamesByID map[uint16]string -- ID→name for all suites parseCipherSuites now does O(1) map lookups instead of O(n) linear scans, and no longer needs the findCipherID helper. cipherIDToName is reduced to a single map lookup with no allocation. Signed-off-by: zhujian <jiazhu@redhat.com>
- ConfigFromFlags: trim whitespace from minVersion and cipherSuites before the empty check so whitespace-only flag values are treated as absent - tls_test.go: remove `expectedLen > 0` guard on CipherSuites length assertions so zero-length expectations are also verified Signed-off-by: zhujian <jiazhu@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/tls/config.go`:
- Around line 35-46: The maps secureCiphersByName and insecureCiphersByName are
populated from tls.CipherSuites() and tls.InsecureCipherSuites() without
filtering, so TLS 1.3–only cipher names (e.g., TLS_AES_128_GCM_SHA256,
TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256) remain registered and can
be accepted even when MinVersion < tls.VersionTLS13; add a helper (e.g.,
tls13Only that checks CipherSuite.SupportedVersions for only tls.VersionTLS13)
and use it when iterating results of tls.CipherSuites() and
tls.InsecureCipherSuites() to skip/register only non–TLS1.3-only entries so
parseCipherSuites will reject TLS1.3-only names for pre-1.3 configurations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b578748-8927-4dcf-bee6-ea3bdff6325f
📒 Files selected for processing (3)
pkg/tls/README.mdpkg/tls/config.gopkg/tls/tls_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/tls/README.md
- pkg/tls/tls_test.go
| secure := tls.CipherSuites() | ||
| insecure := tls.InsecureCipherSuites() | ||
|
|
||
| secureCiphersByName = make(map[string]uint16, len(secure)) | ||
| for _, s := range secure { | ||
| secureCiphersByName[s.Name] = s.ID | ||
| } | ||
|
|
||
| insecureCiphersByName = make(map[string]uint16, len(insecure)) | ||
| for _, s := range insecure { | ||
| insecureCiphersByName[s.Name] = s.ID | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && cat > verify_go_ciphers.go << 'EOF'
package main
import (
"crypto/tls"
"fmt"
)
func main() {
// Check what CipherSuites() returns
secure := tls.CipherSuites()
insecure := tls.InsecureCipherSuites()
fmt.Println("=== SECURE CIPHER SUITES ===")
tls13Count := 0
for _, s := range secure {
hasTLS13 := false
for _, v := range s.SupportedVersions {
if v == tls.VersionTLS13 {
hasTLS13 = true
break
}
}
isOnlyTLS13 := len(s.SupportedVersions) == 1 && s.SupportedVersions[0] == tls.VersionTLS13
if hasTLS13 {
fmt.Printf("%s | ID: %d | SupportedVersions: %v | Only TLS1.3: %v\n",
s.Name, s.ID, s.SupportedVersions, isOnlyTLS13)
if isOnlyTLS13 {
tls13Count++
}
}
}
fmt.Printf("\nTotal TLS 1.3-only suites in CipherSuites(): %d\n", tls13Count)
fmt.Println("\n=== INSECURE CIPHER SUITES ===")
for _, s := range insecure {
hasTLS13 := false
for _, v := range s.SupportedVersions {
if v == tls.VersionTLS13 {
hasTLS13 = true
break
}
}
if hasTLS13 {
fmt.Printf("%s | ID: %d | SupportedVersions: %v\n",
s.Name, s.ID, s.SupportedVersions)
}
}
}
EOF
go run verify_go_ciphers.go
</parameter>Repository: open-cluster-management-io/sdk-go
Length of output: 559
🏁 Script executed:
cd /tmp/repo && head -120 pkg/tls/config.go | tail -100Repository: open-cluster-management-io/sdk-go
Length of output: 136
🏁 Script executed:
cd /tmp/repo && sed -n '1,50p' pkg/tls/config.goRepository: open-cluster-management-io/sdk-go
Length of output: 136
🏁 Script executed:
cd /tmp/repo && sed -n '95,110p' pkg/tls/config.goRepository: open-cluster-management-io/sdk-go
Length of output: 136
🏁 Script executed:
cd /tmp/repo && sed -n '190,210p' pkg/tls/config.goRepository: open-cluster-management-io/sdk-go
Length of output: 136
🏁 Script executed:
cd /tmp/repo && rg -n "TLS13\|tls13\|VersionTLS13\|SupportedVersions" pkg/tls/Repository: open-cluster-management-io/sdk-go
Length of output: 136
🏁 Script executed:
find . -name "config.go" -path "*/tls/*" | head -5Repository: open-cluster-management-io/sdk-go
Length of output: 96
🏁 Script executed:
cat -n pkg/tls/config.go | head -60Repository: open-cluster-management-io/sdk-go
Length of output: 2105
🏁 Script executed:
cat -n pkg/tls/config.go | sed -n '95,110p'Repository: open-cluster-management-io/sdk-go
Length of output: 606
🏁 Script executed:
cat -n pkg/tls/config.go | sed -n '190,210p'Repository: open-cluster-management-io/sdk-go
Length of output: 869
🏁 Script executed:
rg -n "VersionTLS13|SupportedVersions" pkg/tls/Repository: open-cluster-management-io/sdk-go
Length of output: 1777
🏁 Script executed:
cat -n pkg/tls/tls_test.go | grep -A5 -B5 "TLS_AES\|TLS_CHACHA\|cipher.*test"Repository: open-cluster-management-io/sdk-go
Length of output: 1335
🏁 Script executed:
grep -n "secureCiphersByName\|insecureCiphersByName" pkg/tls/Repository: open-cluster-management-io/sdk-go
Length of output: 107
🏁 Script executed:
cat -n pkg/tls/config.go | sed -n '85,110p'Repository: open-cluster-management-io/sdk-go
Length of output: 982
🏁 Script executed:
grep -rn "secureCiphersByName\|insecureCiphersByName" pkg/tls/Repository: open-cluster-management-io/sdk-go
Length of output: 840
🏁 Script executed:
cat -n pkg/tls/config.go | sed -n '34,55p'Repository: open-cluster-management-io/sdk-go
Length of output: 783
Filter out TLS 1.3-only cipher names from the registry maps.
Lines 35–46 populate secureCiphersByName and insecureCiphersByName from tls.CipherSuites() without filtering. This includes 3 TLS 1.3-only entries (TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256) that have no effect on TLS 1.0–1.2 connections. When a user specifies these names in configuration with MinVersion < tls.VersionTLS13, they parse successfully but are silently ignored because tls.Config.CipherSuites only applies to TLS 1.0–1.2. Filter by CipherSuite.SupportedVersions at init time to reject TLS 1.3-only names before they reach parseCipherSuites.
Suggested fix
func init() {
secure := tls.CipherSuites()
insecure := tls.InsecureCipherSuites()
secureCiphersByName = make(map[string]uint16, len(secure))
for _, s := range secure {
+ if tls13Only(s) {
+ continue
+ }
secureCiphersByName[s.Name] = s.ID
}
insecureCiphersByName = make(map[string]uint16, len(insecure))
for _, s := range insecure {
+ if tls13Only(s) {
+ continue
+ }
insecureCiphersByName[s.Name] = s.ID
}func tls13Only(s *tls.CipherSuite) bool {
return len(s.SupportedVersions) == 1 && s.SupportedVersions[0] == tls.VersionTLS13
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/tls/config.go` around lines 35 - 46, The maps secureCiphersByName and
insecureCiphersByName are populated from tls.CipherSuites() and
tls.InsecureCipherSuites() without filtering, so TLS 1.3–only cipher names
(e.g., TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384,
TLS_CHACHA20_POLY1305_SHA256) remain registered and can be accepted even when
MinVersion < tls.VersionTLS13; add a helper (e.g., tls13Only that checks
CipherSuite.SupportedVersions for only tls.VersionTLS13) and use it when
iterating results of tls.CipherSuites() and tls.InsecureCipherSuites() to
skip/register only non–TLS1.3-only entries so parseCipherSuites will reject
TLS1.3-only names for pre-1.3 configurations.
There was a problem hiding this comment.
I'd prefer to keep the current behavior. Consider this valid config:
minVersion: VersionTLS12 with cipherSuites: TLS_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
Without this suggested fix, it works correctly:
- TLS 1.2 connections use
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256(Go silently skips the TLS 1.3-only ID) - TLS 1.3 connections use Go's built-in cipher set (always enabled, not configurable)
With this fix, TLS_AES_128_GCM_SHA256 would be filtered out of the lookup map and treated as unrecognized, causing parseCipherSuites to reject it and the entire configuration to fail — even though the config is perfectly valid.
There was a problem hiding this comment.
@zhujian7, you're right — my concern was misplaced. tls.Config.CipherSuites is described as "a list of enabled TLS 1.0–1.2 cipher suites" and "TLS 1.3 ciphersuites are not configurable." This means TLS 1.3-only entries in CipherSuites are silently ignored by the Go runtime, which is precisely the documented and intended behavior.
In your scenario — minVersion: VersionTLS12 with both TLS_AES_128_GCM_SHA256 and TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 — applying my suggested filter would cause TLS_AES_128_GCM_SHA256 to be treated as an unrecognized name, triggering an error in ConfigFromFlags and breaking the entire configuration. The current code handles this correctly by accepting the name (it is a valid IANA name from tls.CipherSuites()), passing it through, and letting the Go TLS stack harmlessly discard it for TLS 1.0–1.2 connections. TLS 1.3 uses a disjoint set from TLS 1.0–1.2, and all TLS 1.3 cipher suites are secure, so application developers shouldn't have to worry about them.
Withdrawing the suggestion. The current behavior is correct.
(╯°□°）╯ ︵ 🐇
✏️ Learnings added
Learnt from: zhujian7
Repo: open-cluster-management-io/sdk-go PR: 218
File: pkg/tls/config.go:35-46
Timestamp: 2026-03-31T03:13:26.462Z
Learning: In `pkg/tls/config.go` (open-cluster-management-io/sdk-go), the `init()` function intentionally includes TLS 1.3-only cipher names (e.g., `TLS_AES_128_GCM_SHA256`, `TLS_CHACHA20_POLY1305_SHA256`, `TLS_AES_256_GCM_SHA384`) in `secureCiphersByName` and `cipherNamesByID`. This is by design: Go's `tls.Config.CipherSuites` silently ignores TLS 1.3-only entries, so users can safely mix TLS 1.3-only and TLS 1.2 cipher names in their config. Filtering them out would cause valid mixed configurations to fail with an "unsupported cipher suite" error.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhujian7 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 |
0cc72e5
into
open-cluster-management-io:main
2 participants
Summary
cipherMapkeys from OpenSSL-style names (e.g.ECDHE-RSA-AES128-GCM-SHA256) to IANA format (e.g.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256), which matches what Go'scrypto/tlspackage usescipherIDToNameto look ups.Namefromtls.CipherSuites()andtls.InsecureCipherSuites()by ID, replacing the reverse map iteration — this guarantees the returned name is always the authoritative Go/IANA nameparseCipherSuitesandCipherSuitesToStringdoc comments; no structural changes to those functionsCipherSuitesToStringdescription to reference IANA formatRelated issue(s)
Fixes #open-cluster-management-io/ocm#1443
Summary by CodeRabbit
Documentation
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) instead of OpenSSL format.Tests