Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions pkg/server/grpc/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@ import (
"fmt"
"math"
"os"
"strings"
"time"

"github.com/spf13/pflag"
"gopkg.in/yaml.v2"
"k8s.io/klog/v2"

pkgtls "open-cluster-management.io/sdk-go/pkg/tls"
)

type GRPCServerOptions struct {
Expand All @@ -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"`

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skeeey Should we change the TLSMinVersion and TLSMaxVersion from uint16(772) to string(VersionTLS13)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, using string is more readable

@zhujian7 zhujian7 Apr 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has anyone already been using this? Will it break?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ncr38 PTAL, can we change this? do you use it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that would be best

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skeeey No, we don't use tls_min_version.

TLSMaxVersion uint16 `json:"tls_max_version" yaml:"tls_max_version"`
CipherSuites []string `json:"cipher_suites" yaml:"cipher_suites"`
ServerBindPort string `json:"server_bind_port" yaml:"server_bind_port"`
MaxConcurrentStreams uint32 `json:"max_concurrent_streams" yaml:"max_concurrent_streams"`
MaxReceiveMessageSize int `json:"max_receive_message_size" yaml:"max_receive_message_size"`
Expand All @@ -31,6 +35,9 @@ type GRPCServerOptions struct {
ServerPingTimeout time.Duration `json:"server_ping_timeout" yaml:"server_ping_timeout"`
PermitPingWithoutStream bool `json:"permit_ping_without_stream" yaml:"permit_ping_without_stream"`
CertWatchInterval time.Duration `json:"cert_watch_interval" yaml:"cert_watch_interval"`

// cipherSuiteIDs holds the parsed uint16 IDs from CipherSuites, populated by Validate().
cipherSuiteIDs []uint16
}

func LoadGRPCServerOptions(configPath string) (*GRPCServerOptions, error) {
Expand Down Expand Up @@ -110,5 +117,41 @@ func (o *GRPCServerOptions) Validate() error {
if o.CertWatchInterval <= 30*time.Second {
return fmt.Errorf("cert_watch_interval (%v) must be greater than 30 seconds", o.CertWatchInterval)
}

return o.validateCipherSuites()
}

// ApplyTLSFlags overrides TLS settings loaded from the config file with values
// from --tls-min-version and --tls-cipher-suites command-line flags.
// Called after LoadGRPCServerOptions so flags take precedence over the config file.
func (o *GRPCServerOptions) ApplyTLSFlags(minVersion, cipherSuites string) error {
if minVersion != "" {
ver, err := pkgtls.ParseTLSVersion(minVersion)
if err != nil {
return fmt.Errorf("invalid --tls-min-version: %w", err)
}
o.TLSMinVersion = ver
}
if cipherSuites != "" {
o.CipherSuites = strings.Split(cipherSuites, ",")
for i := range o.CipherSuites {
o.CipherSuites[i] = strings.TrimSpace(o.CipherSuites[i])
}
}
return o.Validate()
}

// validateCipherSuites parses CipherSuites IANA names into uint16 IDs
// using the shared pkg/tls parsing utilities.
func (o *GRPCServerOptions) validateCipherSuites() error {
if len(o.CipherSuites) == 0 {
return nil

@coderabbitai coderabbitai Bot Mar 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
cipherString := strings.Join(o.CipherSuites, ",")
ids, unsupported := pkgtls.ParseCipherSuites(cipherString)
if len(unsupported) > 0 {
return fmt.Errorf("unrecognized cipher suite: %s", unsupported[0])
}
o.cipherSuiteIDs = ids
return nil
}
125 changes: 123 additions & 2 deletions pkg/server/grpc/options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
)

func TestLoadGRPCServerOptions(t *testing.T) {
Expand Down Expand Up @@ -173,12 +174,12 @@ connection_timeout: 90s
t.Fatalf("Unexpected error: %v", err)
}

if !cmp.Equal(opts, tc.expectedOpts) {
if !cmp.Equal(opts, tc.expectedOpts, cmpopts.IgnoreUnexported(GRPCServerOptions{})) {
t.Errorf("Loaded options do not match expected options.\nGot: %+v\nWant:%+v", opts, tc.expectedOpts)
}

if tc.checkDefaults {
if !cmp.Equal(opts, defaultOpts) {
if !cmp.Equal(opts, defaultOpts, cmpopts.IgnoreUnexported(GRPCServerOptions{})) {
t.Errorf("Expected default options, but got different values.\nGot: %+v\nWant:%+v", opts, defaultOpts)
}
}
Expand Down Expand Up @@ -239,6 +240,126 @@ func TestGRPCServerOptions_Validate_CertWatchInterval(t *testing.T) {
}
}

func TestApplyTLSFlags(t *testing.T) {
tests := []struct {
name string
minVersion string
cipherSuites string
expectErr bool
errorContains string
expectedMinVer uint16
expectedCiphers []string
}{
{
name: "valid min version override",
minVersion: "VersionTLS13",
expectedMinVer: tls.VersionTLS13,
},
{
name: "valid TLSv format",
minVersion: "TLSv1.3",
expectedMinVer: tls.VersionTLS13,
},
{
name: "valid cipher suite override",
cipherSuites: "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256",
expectedMinVer: tls.VersionTLS12,
expectedCiphers: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256"},
},
{
name: "invalid min version",
minVersion: "VersionTLS99",
expectErr: true,
errorContains: "unknown TLS version",
},
{
name: "unrecognized cipher name",
cipherSuites: "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,BOGUS-CIPHER",
expectErr: true,
errorContains: "unrecognized cipher suite",
},
{
name: "both min version and ciphers",
minVersion: "VersionTLS12",
cipherSuites: "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
expectedMinVer: tls.VersionTLS12,
expectedCiphers: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"},
},
{
name: "empty strings are no-ops",
minVersion: "",
cipherSuites: "",
expectedMinVer: tls.VersionTLS12,
},
{
name: "flags override config file values",
minVersion: "VersionTLS12",
cipherSuites: "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384",
expectedMinVer: tls.VersionTLS12,
expectedCiphers: []string{"TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
opts := NewGRPCServerOptions()

err := opts.ApplyTLSFlags(tt.minVersion, tt.cipherSuites)

if tt.expectErr {
if err == nil {
t.Fatalf("expected error but got none")
}
if tt.errorContains != "" && !contains(err.Error(), tt.errorContains) {
t.Errorf("expected error to contain %q, got: %v", tt.errorContains, err)
}
return
}

if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if opts.TLSMinVersion != tt.expectedMinVer {
t.Errorf("expected TLSMinVersion %d, got %d", tt.expectedMinVer, opts.TLSMinVersion)
}

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(opts.cipherSuiteIDs) != len(tt.expectedCiphers) {
t.Errorf("expected %d parsed cipher IDs, got %d", len(tt.expectedCiphers), len(opts.cipherSuiteIDs))
}
}
})
}
}

func TestApplyTLSFlags_OverridesConfigFile(t *testing.T) {
opts := NewGRPCServerOptions()
// Simulate config file values
opts.TLSMinVersion = tls.VersionTLS12
opts.CipherSuites = []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"}

// Flags override
err := opts.ApplyTLSFlags("VersionTLS13",
"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if opts.TLSMinVersion != tls.VersionTLS13 {
t.Errorf("expected TLSMinVersion TLS 1.3, got %d", opts.TLSMinVersion)
}
if len(opts.CipherSuites) != 2 {
t.Errorf("expected 2 cipher suites, got %d", len(opts.CipherSuites))
}
if len(opts.cipherSuiteIDs) != 2 {
t.Errorf("expected 2 parsed cipher IDs, got %d", len(opts.cipherSuiteIDs))
}
}

func contains(s, substr string) bool {
return len(s) >= len(substr) && (s == substr || len(substr) == 0 ||
(len(s) > 0 && len(substr) > 0 && findSubstring(s, substr)))
Expand Down
5 changes: 5 additions & 0 deletions pkg/server/grpc/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ func (b *GRPCServer) Run(ctx context.Context) error {
MaxVersion: b.options.TLSMaxVersion,
}

// TLS 1.3 cipher suites are not configurable in Go — only set for TLS 1.2 and below.
if len(b.options.cipherSuiteIDs) > 0 && b.options.TLSMinVersion < tls.VersionTLS13 {
tlsConfig.CipherSuites = b.options.cipherSuiteIDs
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if b.options.ClientCAFile != "" {
certPool := x509.NewCertPool()
caPEM, err := os.ReadFile(b.options.ClientCAFile)
Expand Down
14 changes: 8 additions & 6 deletions pkg/tls/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@ type TLSConfig struct {
CipherSuites []uint16
}

// parseTLSVersion converts a TLS version string to the corresponding crypto/tls constant
func parseTLSVersion(version string) (uint16, error) {
// ParseTLSVersion converts a TLS version string to the corresponding crypto/tls constant.
// Accepted formats: "VersionTLS10"/"TLSv1.0" through "VersionTLS13"/"TLSv1.3".
// An empty string defaults to TLS 1.2.
func ParseTLSVersion(version string) (uint16, error) {
version = strings.TrimSpace(version)
switch version {
case "VersionTLS10", "TLSv1.0":
Expand All @@ -78,11 +80,11 @@ func parseTLSVersion(version string) (uint16, error) {
}
}

// parseCipherSuites converts IANA cipher suite names to Go crypto/tls constants.
// ParseCipherSuites converts IANA cipher suite names to Go crypto/tls constants.
// Secure ciphers (tls.CipherSuites) are accepted silently. Insecure ciphers
// (tls.InsecureCipherSuites) are accepted but logged as a warning.
// Returns a list of cipher suite IDs and a list of unrecognized cipher names.
func parseCipherSuites(cipherString string) ([]uint16, []string) {
func ParseCipherSuites(cipherString string) ([]uint16, []string) {
if strings.TrimSpace(cipherString) == "" {
return nil, nil
}
Expand Down Expand Up @@ -132,7 +134,7 @@ func ConfigFromFlags(minVersion, cipherSuites string) (*TLSConfig, error) {

// Parse min version
if minVersion != "" {
ver, err := parseTLSVersion(minVersion)
ver, err := ParseTLSVersion(minVersion)
if err != nil {
return nil, fmt.Errorf("invalid --tls-min-version: %w", err)
}
Expand All @@ -143,7 +145,7 @@ func ConfigFromFlags(minVersion, cipherSuites string) (*TLSConfig, error) {

// Parse cipher suites
if cipherSuites != "" {
suites, unsupported := parseCipherSuites(cipherSuites)
suites, unsupported := ParseCipherSuites(cipherSuites)
if len(unsupported) > 0 {
return nil, fmt.Errorf("unsupported cipher suites: %v", unsupported)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/tls/configmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func parseTLSConfigFromConfigMap(cm *corev1.ConfigMap) (*TLSConfig, error) {
minVersionStr = defaultMinTLSVersion
}

minVersion, err := parseTLSVersion(minVersionStr)
minVersion, err := ParseTLSVersion(minVersionStr)
if err != nil {
return nil, fmt.Errorf("invalid minTLSVersion in ConfigMap: %w", err)
}
Expand All @@ -157,7 +157,7 @@ func parseTLSConfigFromConfigMap(cm *corev1.ConfigMap) (*TLSConfig, error) {
// Parse cipher suites
cipherSuitesStr := cm.Data[ConfigMapKeyCipherSuites]
if cipherSuitesStr != "" {
cipherSuites, unsupported := parseCipherSuites(cipherSuitesStr)
cipherSuites, unsupported := ParseCipherSuites(cipherSuitesStr)
if len(unsupported) > 0 {
klog.Warningf("Unsupported cipher suites in ConfigMap %s/%s: %v", cm.Namespace, cm.Name, unsupported)
if len(cipherSuites) == 0 {
Expand Down
4 changes: 2 additions & 2 deletions pkg/tls/tls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ func TestParseTLSVersion(t *testing.T) {

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
result, err := parseTLSVersion(tc.version)
result, err := ParseTLSVersion(tc.version)

if tc.expectError {
if err == nil {
Expand Down Expand Up @@ -932,7 +932,7 @@ func TestParseCipherSuites(t *testing.T) {

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
suites, unsupported := parseCipherSuites(tc.cipherString)
suites, unsupported := ParseCipherSuites(tc.cipherString)

if len(suites) != tc.expectedCount {
t.Errorf("expected %d cipher suites, got %d", tc.expectedCount, len(suites))
Expand Down
Loading