Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
38 changes: 38 additions & 0 deletions pkg/server/grpc/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"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 +20,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 +34,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 +116,37 @@ 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.parseCipherSuiteIDs()
}

// 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 = cipherSuites
}
return o.Validate()
}

// parseCipherSuiteIDs converts the CipherSuites IANA names into uint16 IDs
// using the shared pkg/tls parsing utilities.
func (o *GRPCServerOptions) parseCipherSuiteIDs() error {
if o.CipherSuites == "" {
return nil
}
ids, unsupported := pkgtls.ParseCipherSuites(o.CipherSuites)
if len(unsupported) > 0 {
return fmt.Errorf("unrecognized cipher suite: %s", unsupported[0])
}
o.cipherSuiteIDs = ids
return nil
}
119 changes: 117 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,120 @@ 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
expectedCipherCount int
}{
{
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,
expectedCipherCount: 2,
},
{
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,
expectedCipherCount: 1,
},
{
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,
expectedCipherCount: 1,
},
}

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

func TestApplyTLSFlags_OverridesConfigFile(t *testing.T) {
opts := NewGRPCServerOptions()
// Simulate config file values
opts.TLSMinVersion = tls.VersionTLS12
opts.CipherSuites = "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.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
9 changes: 9 additions & 0 deletions pkg/server/grpc/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ func (b *GRPCServer) WithStreamAuthorizer(authorizer authz.StreamAuthorizer) *GR
}

func (b *GRPCServer) Run(ctx context.Context) error {
if err := b.options.Validate(); err != nil {
return err
}

var grpcServerOptions []grpc.ServerOption
grpcServerOptions = append(grpcServerOptions, grpc.MaxRecvMsgSize(b.options.MaxReceiveMessageSize))
grpcServerOptions = append(grpcServerOptions, grpc.MaxSendMsgSize(b.options.MaxSendMessageSize))
Expand Down Expand Up @@ -110,6 +114,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