Skip to content

⚠️ refactor(grpc): change TLSMinVersion/TLSMaxVersion from uint16 to string #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Apr 3, 2026
Merged

⚠️ refactor(grpc): change TLSMinVersion/TLSMaxVersion from uint16 to string #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 22 additions & 15 deletions pkg/server/grpc/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ type GRPCServerOptions struct {
TLSCertFile string `json:"tls_cert_file" yaml:"tls_cert_file"`
TLSKeyFile string `json:"tls_key_file" yaml:"tls_key_file"`
ClientCAFile string `json:"client_ca_file" yaml:"client_ca_file"`
TLSMinVersion uint16 `json:"tls_min_version" yaml:"tls_min_version"`
TLSMaxVersion uint16 `json:"tls_max_version" yaml:"tls_max_version"`
TLSMinVersion string `json:"tls_min_version" yaml:"tls_min_version"`
TLSMaxVersion string `json:"tls_max_version" yaml:"tls_max_version"`
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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"`
Expand All @@ -35,7 +35,9 @@ type GRPCServerOptions struct {
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().
// Parsed TLS settings, populated by Validate().
tlsMinVersion uint16
tlsMaxVersion uint16
Comment thread
coderabbitai[bot] marked this conversation as resolved.
cipherSuiteIDs []uint16
}

Expand Down Expand Up @@ -67,8 +69,8 @@ func NewGRPCServerOptions() *GRPCServerOptions {
ClientCAFile: "/var/run/secrets/hub/grpc/ca/ca-bundle.crt",
TLSCertFile: "/var/run/secrets/hub/grpc/serving-cert/tls.crt",
TLSKeyFile: "/var/run/secrets/hub/grpc/serving-cert/tls.key",
TLSMinVersion: tls.VersionTLS12,
TLSMaxVersion: tls.VersionTLS13,
TLSMinVersion: "VersionTLS12",
TLSMaxVersion: "VersionTLS13",
ServerBindPort: "8090",
MaxConcurrentStreams: math.MaxUint32,
MaxReceiveMessageSize: 1024 * 1024 * 4,
Expand Down Expand Up @@ -105,13 +107,22 @@ func (o *GRPCServerOptions) AddFlags(flags *pflag.FlagSet) {

// Validate checks option ranges and cross-field constraints.
func (o *GRPCServerOptions) Validate() error {
// Enforce sane floor for TLS for security posture.
if o.TLSMinVersion < tls.VersionTLS12 {
return fmt.Errorf("tls_min_version (%d) is lower than TLS 1.2 (771); minimum supported is TLS 1.2", o.TLSMinVersion)
minVer, err := pkgtls.ParseTLSVersion(o.TLSMinVersion)
if err != nil {
return fmt.Errorf("invalid tls_min_version %q: %w", o.TLSMinVersion, err)
}
maxVer, err := pkgtls.ParseTLSVersion(o.TLSMaxVersion)
if err != nil {
return fmt.Errorf("invalid tls_max_version %q: %w", o.TLSMaxVersion, err)
}
if minVer < tls.VersionTLS12 {
return fmt.Errorf("tls_min_version %q is lower than TLS 1.2; minimum supported is TLS 1.2", o.TLSMinVersion)
}
if o.TLSMinVersion > o.TLSMaxVersion {
return fmt.Errorf("tls_min_version (%d) must be <= tls_max_version (%d)", o.TLSMinVersion, o.TLSMaxVersion)
if minVer > maxVer {
return fmt.Errorf("tls_min_version %q must be <= tls_max_version %q", o.TLSMinVersion, o.TLSMaxVersion)
}
o.tlsMinVersion = minVer
o.tlsMaxVersion = maxVer
// Validate certificate watch interval to prevent time.NewTicker panic
if o.CertWatchInterval <= 30*time.Second {
return fmt.Errorf("cert_watch_interval (%v) must be greater than 30 seconds", o.CertWatchInterval)
Expand All @@ -125,11 +136,7 @@ func (o *GRPCServerOptions) Validate() error {
// 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
o.TLSMinVersion = minVersion
}
if cipherSuites != "" {
o.CipherSuites = cipherSuites
Expand Down
14 changes: 7 additions & 7 deletions pkg/server/grpc/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ cert_watch_interval: 60s
TLSCertFile: "/test/tls.crt",
TLSKeyFile: "/test/tls.key",
ClientCAFile: "/test/ca.crt",
TLSMinVersion: tls.VersionTLS12,
TLSMaxVersion: tls.VersionTLS13,
TLSMinVersion: "VersionTLS12",
TLSMaxVersion: "VersionTLS13",
ServerBindPort: "9999",
MaxConcurrentStreams: 100,
MaxReceiveMessageSize: 2048,
Expand Down Expand Up @@ -320,8 +320,8 @@ func TestApplyTLSFlags(t *testing.T) {
t.Fatalf("unexpected error: %v", err)
}

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

if tt.expectedCipherCount > 0 {
Expand All @@ -336,7 +336,7 @@ func TestApplyTLSFlags(t *testing.T) {
func TestApplyTLSFlags_OverridesConfigFile(t *testing.T) {
opts := NewGRPCServerOptions()
// Simulate config file values
opts.TLSMinVersion = tls.VersionTLS12
opts.TLSMinVersion = "VersionTLS12"
opts.CipherSuites = "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"

// Flags override
Expand All @@ -346,8 +346,8 @@ func TestApplyTLSFlags_OverridesConfigFile(t *testing.T) {
t.Fatalf("unexpected error: %v", err)
}

if opts.TLSMinVersion != tls.VersionTLS13 {
t.Errorf("expected TLSMinVersion TLS 1.3, got %d", opts.TLSMinVersion)
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))
Expand Down
6 changes: 3 additions & 3 deletions pkg/server/grpc/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,12 @@ func (b *GRPCServer) Run(ctx context.Context) error {
// Use GetCertificate callback from certwatcher
// This allows dynamic certificate reloading on each TLS handshake
GetCertificate: certWatcher.GetCertificate,
MinVersion: b.options.TLSMinVersion,
MaxVersion: b.options.TLSMaxVersion,
MinVersion: b.options.tlsMinVersion,
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 {
if len(b.options.cipherSuiteIDs) > 0 && b.options.tlsMinVersion < tls.VersionTLS13 {
tlsConfig.CipherSuites = b.options.cipherSuiteIDs
}

Expand Down
Loading