✨ Add cipher suite support to gRPC server TLS config - #219
Conversation
|
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:
WalkthroughAdds exported TLS parsing helpers, new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
/hold |
![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
🧹 Nitpick comments (1)
pkg/server/grpc/options_test.go (1)
177-182: Add explicitCipherSuitesvalidation cases.Ignoring unexported fields here is fine, but this file still never exercises the new parser. A small table around
Validate()for valid names, insecure names, and bad names would keep this security-sensitive change from regressing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/grpc/options_test.go` around lines 177 - 182, Tests for GRPCServerOptions are not exercising the new CipherSuites parsing/validation, so add table-driven cases that call GRPCServerOptions.Validate() and assert expected outcomes for valid names (accepted), insecure names (rejected or warned per policy), and malformed names (validation error). Update the test in options_test.go to include a new subtest table that constructs GRPCServerOptions with various CipherSuites values, calls opts.Validate(), and checks for no error for valid lists and specific error presence for insecure/bad names; reference the GRPCServerOptions struct and its Validate() method and assert on the returned error or state to prevent regressions.
🤖 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/server/grpc/options.go`:
- Around line 118-136: The cipher-suite parsing currently accepts TLS 1.3-only
suites from tls.CipherSuites() into o.cipherSuiteIDs, which will be ignored by
tls.Config at runtime; update the loop in the function handling o.CipherSuites
(the block that calls tls.CipherSuites(), tls.InsecureCipherSuites(), and
findCipherSuiteID) to inspect the found tls.CipherSuite's SupportedVersions and
either reject or warn when a suite's SupportedVersions contains only TLS 1.3
(i.e., does not include TLS 1.0–1.2), returning an error for invalid configs (or
logging a clear warning for insecure/ignored entries) instead of silently
accepting them so that o.cipherSuiteIDs only contains suites compatible with
non‑TLS1.3 versions.
In `@pkg/server/grpc/server.go`:
- Around line 113-116: The code reads b.options.cipherSuiteIDs without ensuring
options are validated/populated, so callers that construct GRPCServerOptions
programmatically may leave cipherSuiteIDs empty; call
(*GRPCServerOptions).Validate() (or otherwise populate/parse cipher suites)
early in NewGRPCServer or at the start of Run() before this branch and/or ensure
cipherSuiteIDs is computed from the public CipherSuites field when nil; update
the logic around tlsConfig.CipherSuites to use the validated/populated
b.options.cipherSuiteIDs (or parse from b.options.CipherSuites) and only assign
when populated and TLSMinVersion < tls.VersionTLS13 to keep behavior consistent
across construction paths.
---
Nitpick comments:
In `@pkg/server/grpc/options_test.go`:
- Around line 177-182: Tests for GRPCServerOptions are not exercising the new
CipherSuites parsing/validation, so add table-driven cases that call
GRPCServerOptions.Validate() and assert expected outcomes for valid names
(accepted), insecure names (rejected or warned per policy), and malformed names
(validation error). Update the test in options_test.go to include a new subtest
table that constructs GRPCServerOptions with various CipherSuites values, calls
opts.Validate(), and checks for no error for valid lists and specific error
presence for insecure/bad names; reference the GRPCServerOptions struct and its
Validate() method and assert on the returned error or state to prevent
regressions.
🪄 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: 3d5c2455-1cdf-4916-aac3-878e0563e0e7
📒 Files selected for processing (3)
pkg/server/grpc/options.gopkg/server/grpc/options_test.gopkg/server/grpc/server.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/server/grpc/options.go (1)
150-157:⚠️ Potential issue | 🟠 MajorTLS 1.3-only suites still need to be rejected during validation.
Lines 151-157 accept any name returned by
tls.CipherSuites(), but that API includes TLS 1.3-only entries viaCipherSuite.SupportedVersions, whiletls.Config.CipherSuitesonly governs TLS 1.0–1.2. With Go's current filtering, a config made only of TLS 1.3-only names validates here but, whenTLSMinVersionis below TLS 1.3, produces an empty configured TLS 1.2 suite set at runtime. Please gate onSupportedVersionsbefore storing the ID. (pkg.go.dev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/grpc/options.go` around lines 150 - 157, The loop over o.CipherSuites accepts any suite IDs returned by findCipherSuiteID (from the secure/insecure lists) but must reject TLS 1.3‑only suites because tls.Config.CipherSuites only applies to TLS 1.0–1.2; update the logic in the loop that calls findCipherSuiteID to also inspect the corresponding tls.CipherSuite.SupportedVersions and only append the id if the suite supports TLS versions <= tls.VersionTLS12 (reject suites whose SupportedVersions are TLS 1.3‑only), keeping the same warning for insecure matches (variables: o.CipherSuites, findCipherSuiteID, secure, insecure, tls.CipherSuite.SupportedVersions, tls.Config.CipherSuites).
🤖 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/server/grpc/options.go`:
- Around line 143-145: The validateCipherSuites method returns early when
o.CipherSuites is empty but does not clear the derived cache o.cipherSuiteIDs,
so subsequent revalidations may reuse stale IDs; modify
GRPCServerOptions.validateCipherSuites to reset o.cipherSuiteIDs (set to nil or
empty) before returning when len(o.CipherSuites) == 0 so the server.go logic
will not apply old cipher suites on revalidation.
---
Duplicate comments:
In `@pkg/server/grpc/options.go`:
- Around line 150-157: The loop over o.CipherSuites accepts any suite IDs
returned by findCipherSuiteID (from the secure/insecure lists) but must reject
TLS 1.3‑only suites because tls.Config.CipherSuites only applies to TLS 1.0–1.2;
update the logic in the loop that calls findCipherSuiteID to also inspect the
corresponding tls.CipherSuite.SupportedVersions and only append the id if the
suite supports TLS versions <= tls.VersionTLS12 (reject suites whose
SupportedVersions are TLS 1.3‑only), keeping the same warning for insecure
matches (variables: o.CipherSuites, findCipherSuiteID, secure, insecure,
tls.CipherSuite.SupportedVersions, tls.Config.CipherSuites).
🪄 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: 77a45f58-24db-4f81-b92b-f3f6d7df38ad
📒 Files selected for processing (2)
pkg/server/grpc/options.gopkg/server/grpc/options_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/server/grpc/options_test.go
| func (o *GRPCServerOptions) validateCipherSuites() error { | ||
| if len(o.CipherSuites) == 0 { | ||
| return nil |
There was a problem hiding this comment.
Clear the cached cipher IDs when CipherSuites becomes empty.
Line 144 returns before resetting the derived o.cipherSuiteIDs cache. If this options object is revalidated after CipherSuites is cleared, pkg/server/grpc/server.go will keep applying the old suites.
💡 Proposed fix
func (o *GRPCServerOptions) validateCipherSuites() error {
if len(o.CipherSuites) == 0 {
+ o.cipherSuiteIDs = nil
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (o *GRPCServerOptions) validateCipherSuites() error { | |
| if len(o.CipherSuites) == 0 { | |
| return nil | |
| func (o *GRPCServerOptions) validateCipherSuites() error { | |
| if len(o.CipherSuites) == 0 { | |
| o.cipherSuiteIDs = nil | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/server/grpc/options.go` around lines 143 - 145, The validateCipherSuites
method returns early when o.CipherSuites is empty but does not clear the derived
cache o.cipherSuiteIDs, so subsequent revalidations may reuse stale IDs; modify
GRPCServerOptions.validateCipherSuites to reset o.cipherSuiteIDs (set to nil or
empty) before returning when len(o.CipherSuites) == 0 so the server.go logic
will not apply old cipher suites on revalidation.
There was a problem hiding this comment.
CipherSuites is set once at startup (from config file or CLI flags) and never cleared afterward. There is no real scenario where it transitions from non-empty to empty on the same options object, so clearing cipherSuiteIDs here is unnecessary.
There was a problem hiding this comment.
@zhujian7, that makes sense — since GRPCServerOptions is configured once at startup and the CipherSuites field is never mutated to an empty value after that point, there's no real revalidation path where stale IDs would be applied. I'll defer to your judgment here.
✏️ Learnings added
Learnt from: zhujian7
Repo: open-cluster-management-io/sdk-go PR: 219
File: pkg/server/grpc/options.go:0-0
Timestamp: 2026-04-01T14:51:02.810Z
Learning: In `pkg/server/grpc/options.go` (open-cluster-management-io/sdk-go), `GRPCServerOptions` (including its `CipherSuites` field) is treated as immutable after startup — it is populated once from a config file or CLI flags and never cleared or mutated afterward. Therefore, clearing the derived `cipherSuiteIDs` cache when `CipherSuites` is empty inside `validateCipherSuites`/`parseCipherSuiteIDs` is unnecessary, as the scenario where `CipherSuites` transitions from non-empty to empty on the same options object does not occur in practice.
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.
🧠 Learnings used
Learnt from: zhujian7
Repo: open-cluster-management-io/sdk-go PR: 218
File: pkg/tls/config.go:35-46
Timestamp: 2026-03-31T03:13:28.938Z
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.
Add a CipherSuites []string field to GRPCServerOptions for configuring TLS cipher suites using IANA names (e.g. TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256). Validate() parses the IANA names into uint16 IDs using tls.CipherSuites() and tls.InsecureCipherSuites(), returning an error for unrecognized names and logging a warning for insecure suites. The parsed IDs are applied to tls.Config.CipherSuites when non-empty and TLSMinVersion is below TLS 1.3 (TLS 1.3 cipher suites are not configurable in Go). Signed-off-by: zhujian <jiazhu@redhat.com>
Add ApplyTLSFlags(minVersion, cipherSuites) to GRPCServerOptions so that --tls-min-version and --tls-cipher-suites flag values from commonoptions can override TLS settings loaded from the config file. Refactor validateCipherSuites() out of Validate() so both paths share the same cipher suite parsing logic. Add parseTLSVersion() for converting version strings (VersionTLS12, TLSv1.2, etc.) to uint16. Add tests covering: valid overrides, invalid values, and flag precedence over config file settings. Signed-off-by: zhujian <jiazhu@redhat.com>
659a241 to
efa1e9c
Compare
Export ParseTLSVersion and ParseCipherSuites from pkg/tls and use them in grpc options, eliminating ~30 lines of duplicated parsing code. Also fix test cases that used TLS 1.3 cipher names not present in tls.CipherSuites(). Signed-off-by: Jia Zhu <jiazhu@redhat.com> Signed-off-by: zhujian <jiazhu@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/server/grpc/options.go (1)
147-155:⚠️ Potential issue | 🟠 MajorReset cached cipher IDs when
CipherSuitesis empty.At Line 147, returning early leaves
o.cipherSuiteIDsuntouched. If options are revalidated after clearingCipherSuites, stale IDs can still be applied later.💡 Proposed fix
func (o *GRPCServerOptions) validateCipherSuites() error { if len(o.CipherSuites) == 0 { + o.cipherSuiteIDs = nil return nil } cipherString := strings.Join(o.CipherSuites, ",") ids, unsupported := pkgtls.ParseCipherSuites(cipherString)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/grpc/options.go` around lines 147 - 155, When validating cipher suites in the options block, clear any previously cached IDs when CipherSuites is empty to avoid reusing stale values; specifically, in the branch that currently returns when len(o.CipherSuites) == 0, set o.cipherSuiteIDs = nil before returning (update the logic around the o.CipherSuites check in the function that sets o.cipherSuiteIDs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/server/grpc/options.go`:
- Around line 147-155: When validating cipher suites in the options block, clear
any previously cached IDs when CipherSuites is empty to avoid reusing stale
values; specifically, in the branch that currently returns when
len(o.CipherSuites) == 0, set o.cipherSuiteIDs = nil before returning (update
the logic around the o.CipherSuites check in the function that sets
o.cipherSuiteIDs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76117d58-b4bb-4890-a384-d7bb6cecae26
📒 Files selected for processing (6)
pkg/server/grpc/options.gopkg/server/grpc/options_test.gopkg/server/grpc/server.gopkg/tls/config.gopkg/tls/configmap.gopkg/tls/tls_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/server/grpc/server.go
efa1e9c to
bb0e27b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/server/grpc/options.go (1)
147-149:⚠️ Potential issue | 🟡 MinorReset derived cipher IDs when
CipherSuitesis empty.Line 147 exits without clearing
o.cipherSuiteIDs, so a reused options object can keep applying stale cipher IDs after suites are removed.💡 Proposed fix
func (o *GRPCServerOptions) validateCipherSuites() error { if len(o.CipherSuites) == 0 { + o.cipherSuiteIDs = nil return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/grpc/options.go` around lines 147 - 149, The early return when o.CipherSuites is empty leaves stale derived IDs in o.cipherSuiteIDs; update the branch in the function handling cipher suites (where o.CipherSuites is checked) to clear or reset o.cipherSuiteIDs (e.g., set to nil or empty slice) before returning so a reused options object does not retain old cipher IDs; ensure this change touches the same method that computes/uses o.cipherSuiteIDs so subsequent calls reflect the cleared state.
🧹 Nitpick comments (1)
pkg/server/grpc/options_test.go (1)
327-334: Assert cipher suite values (and order), not only counts.These checks pass even if parsing returns wrong suite content with the same length.
✅ Suggested test tightening
- if len(tt.expectedCiphers) > 0 { - if len(opts.CipherSuites) != len(tt.expectedCiphers) { - t.Errorf("expected %d cipher suites, got %d", len(tt.expectedCiphers), len(opts.CipherSuites)) - } + if len(tt.expectedCiphers) > 0 { + if diff := cmp.Diff(tt.expectedCiphers, opts.CipherSuites); diff != "" { + t.Errorf("cipher suites mismatch (-want +got):\n%s", diff) + } if len(opts.cipherSuiteIDs) != len(tt.expectedCiphers) { t.Errorf("expected %d parsed cipher IDs, got %d", len(tt.expectedCiphers), len(opts.cipherSuiteIDs)) } }- if len(opts.CipherSuites) != 2 { - t.Errorf("expected 2 cipher suites, got %d", len(opts.CipherSuites)) - } + expectedCiphers := []string{ + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + } + if diff := cmp.Diff(expectedCiphers, opts.CipherSuites); diff != "" { + t.Errorf("cipher suites mismatch (-want +got):\n%s", diff) + }Also applies to: 355-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/grpc/options_test.go` around lines 327 - 334, The test currently only checks lengths for opts.CipherSuites and opts.cipherSuiteIDs against tt.expectedCiphers; change it to assert the actual values and their order by comparing the slices element-by-element (or using reflect.DeepEqual or a slice comparison helper) and fail with a clear message showing expected vs actual for both opts.CipherSuites and opts.cipherSuiteIDs; apply the same stronger assertions to the other similar block that checks cipher suites in this test so you verify content and ordering, not just counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/server/grpc/options.go`:
- Around line 147-149: The early return when o.CipherSuites is empty leaves
stale derived IDs in o.cipherSuiteIDs; update the branch in the function
handling cipher suites (where o.CipherSuites is checked) to clear or reset
o.cipherSuiteIDs (e.g., set to nil or empty slice) before returning so a reused
options object does not retain old cipher IDs; ensure this change touches the
same method that computes/uses o.cipherSuiteIDs so subsequent calls reflect the
cleared state.
---
Nitpick comments:
In `@pkg/server/grpc/options_test.go`:
- Around line 327-334: The test currently only checks lengths for
opts.CipherSuites and opts.cipherSuiteIDs against tt.expectedCiphers; change it
to assert the actual values and their order by comparing the slices
element-by-element (or using reflect.DeepEqual or a slice comparison helper) and
fail with a clear message showing expected vs actual for both opts.CipherSuites
and opts.cipherSuiteIDs; apply the same stronger assertions to the other similar
block that checks cipher suites in this test so you verify content and ordering,
not just counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bfbb900-0880-4e22-ad10-a2fe88e81de8
📒 Files selected for processing (5)
pkg/server/grpc/options.gopkg/server/grpc/options_test.gopkg/tls/config.gopkg/tls/configmap.gopkg/tls/tls_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/tls/configmap.go
- pkg/tls/tls_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/tls/config.go
|
/cc @skeeey @qiujian16 |
| @@ -18,6 +21,7 @@ type GRPCServerOptions struct { | |||
| ClientCAFile string `json:"client_ca_file" yaml:"client_ca_file"` | |||
| TLSMinVersion uint16 `json:"tls_min_version" yaml:"tls_min_version"` | |||
There was a problem hiding this comment.
So currently we use the server-config file to configure the tls_min_version, but the type is uint16, so the user needs to configure it like:
tls_min_version: 772 # this looks not quite human-readable?
There was a problem hiding this comment.
@skeeey Should we change the TLSMinVersion and TLSMaxVersion from uint16(772) to string(VersionTLS13)?
There was a problem hiding this comment.
yeah, using string is more readable
There was a problem hiding this comment.
Has anyone already been using this? Will it break?
There was a problem hiding this comment.
How about we merge the current pr first, and if we decide to change the tls_min_version to string, we do it as a follow-up PR.
db78445 to
4049725
Compare
Change CipherSuites from []string to string, eliminating the split-then-join round-trip between ApplyTLSFlags and validateCipherSuites. The string is passed directly to pkgtls.ParseCipherSuites without intermediate conversions. Signed-off-by: Jia Zhu <jiazhu@redhat.com> Signed-off-by: zhujian <jiazhu@redhat.com>
4049725 to
e850e70
Compare
|
/unhold |
|
LGTM and I think we also need support this in https://github.com/open-cluster-management-io/sdk-go/blob/main/pkg/cloudevents/generic/options/cert/config.go, this is tls config for cloudevnet clients, these clients will be used by ocm components with grpc mod, right? |
@skeeey I don't think it's necessary for the client side right now. Cipher suite configuration is primarily a server-side concern — the server decides which suites to accept, and the client negotiates from what Go's TLS stack offers by default (which is already secure). For clients, Go's defaults are fine:
We can add it later if a concrete use case comes up. |
|
/assign @qiujian16 |
|
/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 |
996da9f
into
open-cluster-management-io:main
4 participants
Summary
CipherSuites []stringfield toGRPCServerOptionsfor configuring TLS cipher suites using IANA names (e.g.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) in YAML/JSON configValidate()parses IANA names intouint16IDs via the sharedpkg/tls.ParseCipherSuites(), returning an error for unrecognized names and logging a warning for insecure suitestls.Config.CipherSuitesonly when non-empty andTLSMinVersion < TLS 1.3(TLS 1.3 cipher suites are not configurable in Go)ApplyTLSFlags()allows--tls-min-versionand--tls-cipher-suitesCLI flags to override config file valuespkg/tls.ParseTLSVersionandpkg/tls.ParseCipherSuites, eliminating duplicated code betweenpkg/server/grpcandpkg/tlsRelated issue(s)
Fixes open-cluster-management-io/ocm#1443
Summary by CodeRabbit
New Features
Tests
Chores