From f9521d3f4baf8fbbaa89e3e578e24042158c20a8 Mon Sep 17 00:00:00 2001 From: Ugo Giordano Date: Thu, 18 Jun 2026 12:08:14 +0200 Subject: [PATCH 1/3] feat: add ALPN/NextProtos helpers for TLS config Add SetNextProtos and WithHTTP2 composable functions that follow the existing func(*tls.Config) pattern used by controller-runtime's TLSOpts. Currently every operator using NewTLSConfigFromProfile must manually append a function to set tls.Config.NextProtos for ALPN negotiation. This is copy-pasted across 49+ repos in the openshift org, typically as a CVE-2023-44487 mitigation (disabling HTTP/2). These helpers eliminate that boilerplate: // Before: disableHTTP2 := func(c *tls.Config) { c.NextProtos = []string{"http/1.1"} } tlsOpts := []func(*tls.Config){tlsConfig, disableHTTP2} // After: tlsOpts := []func(*tls.Config){tlsConfig, openshifttls.WithHTTP2(false)} New exports: - HTTP2NextProtos: []string{"h2", "http/1.1"} - HTTP1NextProtos: []string{"http/1.1"} - SetNextProtos(protos ...string): generic ALPN setter - WithHTTP2(enabled bool): convenience for HTTP/2 enable/disable Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/tls/tls.go | 46 +++++++++++++++++++++++++ pkg/tls/tls_test.go | 82 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) diff --git a/pkg/tls/tls.go b/pkg/tls/tls.go index ce1e8c7d..dd6726ec 100644 --- a/pkg/tls/tls.go +++ b/pkg/tls/tls.go @@ -41,6 +41,15 @@ var ( DefaultTLSCiphers = configv1.TLSProfiles[configv1.TLSProfileIntermediateType].Ciphers //nolint:gochecknoglobals // DefaultMinTLSVersion is the default minimum TLS version for API servers. DefaultMinTLSVersion = configv1.TLSProfiles[configv1.TLSProfileIntermediateType].MinTLSVersion //nolint:gochecknoglobals + + // HTTP2NextProtos are the ALPN protocols advertised when HTTP/2 is enabled, + // with HTTP/1.1 fallback. + HTTP2NextProtos = []string{"h2", "http/1.1"} //nolint:gochecknoglobals + + // HTTP1NextProtos are the ALPN protocols advertised when HTTP/2 is disabled. + // Use this as a mitigation for HTTP/2 Rapid Reset vulnerabilities + // (CVE-2023-44487, CVE-2023-39325). + HTTP1NextProtos = []string{"http/1.1"} //nolint:gochecknoglobals ) // FetchAPIServerTLSProfile fetches the TLS profile spec configured in APIServer. @@ -131,6 +140,43 @@ func NewTLSConfigFromProfile(profile configv1.TLSProfileSpec) (tlsConfig func(*t }, unsupportedCiphers } +// SetNextProtos returns a TLS configuration function that sets the ALPN +// protocol negotiation list on a tls.Config. +// The returned function is intended to be used with controller-runtime's TLSOpts. +// +// Example: +// +// tlsOpts := []func(*tls.Config){ +// openshifttls.SetNextProtos("h2", "http/1.1"), +// } +func SetNextProtos(protos ...string) func(*tls.Config) { + p := make([]string, len(protos)) + copy(p, protos) + return func(c *tls.Config) { + c.NextProtos = p + } +} + +// WithHTTP2 returns a TLS configuration function that configures ALPN +// protocol negotiation based on whether HTTP/2 should be enabled. +// +// When enabled is true, both "h2" and "http/1.1" are advertised. +// When enabled is false, only "http/1.1" is advertised as a mitigation +// for HTTP/2 Rapid Reset vulnerabilities (CVE-2023-44487, CVE-2023-39325). +// +// The returned function is intended to be used with controller-runtime's TLSOpts. +// +// Example: +// +// tlsConfig, _ := openshifttls.NewTLSConfigFromProfile(profile) +// tlsOpts := []func(*tls.Config){tlsConfig, openshifttls.WithHTTP2(false)} +func WithHTTP2(enabled bool) func(*tls.Config) { + if enabled { + return SetNextProtos(HTTP2NextProtos...) + } + return SetNextProtos(HTTP1NextProtos...) +} + // cipherCode returns the TLS cipher code for an OpenSSL or IANA cipher name. // Returns 0 if the cipher is not supported. func cipherCode(cipher string) uint16 { diff --git a/pkg/tls/tls_test.go b/pkg/tls/tls_test.go index 973350e9..0d05972f 100644 --- a/pkg/tls/tls_test.go +++ b/pkg/tls/tls_test.go @@ -190,6 +190,88 @@ var _ = Describe("cipherCodes", func() { }) }) +var _ = Describe("SetNextProtos", func() { + It("should set NextProtos on tls.Config", func() { + fn := SetNextProtos("h2", "http/1.1") + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"h2", "http/1.1"})) + }) + + It("should handle a single protocol", func() { + fn := SetNextProtos("http/1.1") + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"http/1.1"})) + }) + + It("should handle no protocols", func() { + fn := SetNextProtos() + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(BeEmpty()) + }) + + It("should not be affected by modification of the original slice", func() { + protos := []string{"h2", "http/1.1"} + fn := SetNextProtos(protos...) + protos[0] = "modified" + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"h2", "http/1.1"})) + }) + + It("should override any existing NextProtos", func() { + fn := SetNextProtos("h2", "http/1.1") + c := &tls.Config{NextProtos: []string{"old-proto"}} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"h2", "http/1.1"})) + }) +}) + +var _ = Describe("WithHTTP2", func() { + Context("when HTTP/2 is enabled", func() { + It("should set NextProtos to h2 and http/1.1", func() { + fn := WithHTTP2(true) + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"h2", "http/1.1"})) + }) + }) + + Context("when HTTP/2 is disabled", func() { + It("should set NextProtos to http/1.1 only", func() { + fn := WithHTTP2(false) + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"http/1.1"})) + }) + }) + + It("should compose with NewTLSConfigFromProfile", func() { + profile := *configv1.TLSProfiles[configv1.TLSProfileIntermediateType] + tlsConfigFn, _ := NewTLSConfigFromProfile(profile) + + c := &tls.Config{} + tlsConfigFn(c) + WithHTTP2(false)(c) + + Expect(c.MinVersion).To(Equal(uint16(tls.VersionTLS12))) + Expect(c.CipherSuites).NotTo(BeEmpty()) + Expect(c.NextProtos).To(Equal([]string{"http/1.1"})) + }) +}) + +var _ = Describe("ALPN protocol variables", func() { + It("HTTP2NextProtos should contain h2 and http/1.1", func() { + Expect(HTTP2NextProtos).To(Equal([]string{"h2", "http/1.1"})) + }) + + It("HTTP1NextProtos should contain only http/1.1", func() { + Expect(HTTP1NextProtos).To(Equal([]string{"http/1.1"})) + }) +}) + var _ = Describe("NewTLSConfigFromProfile", func() { Context("when MinTLSVersion is TLS 1.2", func() { It("should set MinVersion and CipherSuites", func() { From d5be8b0b28bf2bd05321881491ca07de57a0ad63 Mon Sep 17 00:00:00 2001 From: Ugo Giordano Date: Thu, 18 Jun 2026 12:55:35 +0200 Subject: [PATCH 2/3] fix: address adversarial review findings for ALPN helpers 1. Reword HTTP1NextProtos comment to clarify this is defense-in-depth against CVE-2023-44487/CVE-2023-39325, not a standalone fix. The primary fixes are in Go 1.21.3+ and golang.org/x/net v0.17.0+. 2. Fix SetNextProtos to preserve nil semantics when called with zero arguments. Previously make([]string, 0) produced a non-nil empty slice, which has different TLS behavior than nil (nil = no ALPN negotiation, empty = ALPN extension with no protocols). 3. Update test assertion to use BeNil() instead of BeEmpty() to verify the nil vs empty distinction. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/tls/tls.go | 14 +++++++++----- pkg/tls/tls_test.go | 4 ++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/tls/tls.go b/pkg/tls/tls.go index dd6726ec..96ca187f 100644 --- a/pkg/tls/tls.go +++ b/pkg/tls/tls.go @@ -46,9 +46,10 @@ var ( // with HTTP/1.1 fallback. HTTP2NextProtos = []string{"h2", "http/1.1"} //nolint:gochecknoglobals - // HTTP1NextProtos are the ALPN protocols advertised when HTTP/2 is disabled. - // Use this as a mitigation for HTTP/2 Rapid Reset vulnerabilities - // (CVE-2023-44487, CVE-2023-39325). + // HTTP1NextProtos are the ALPN protocols advertised when HTTP/2 is disabled, + // restricting negotiation to HTTP/1.1 only. This provides defense-in-depth + // against HTTP/2 Rapid Reset (CVE-2023-44487, CVE-2023-39325) alongside + // the primary fixes in Go 1.21.3+ and golang.org/x/net v0.17.0+. HTTP1NextProtos = []string{"http/1.1"} //nolint:gochecknoglobals ) @@ -150,8 +151,11 @@ func NewTLSConfigFromProfile(profile configv1.TLSProfileSpec) (tlsConfig func(*t // openshifttls.SetNextProtos("h2", "http/1.1"), // } func SetNextProtos(protos ...string) func(*tls.Config) { - p := make([]string, len(protos)) - copy(p, protos) + var p []string + if len(protos) > 0 { + p = make([]string, len(protos)) + copy(p, protos) + } return func(c *tls.Config) { c.NextProtos = p } diff --git a/pkg/tls/tls_test.go b/pkg/tls/tls_test.go index 0d05972f..95b11d27 100644 --- a/pkg/tls/tls_test.go +++ b/pkg/tls/tls_test.go @@ -205,11 +205,11 @@ var _ = Describe("SetNextProtos", func() { Expect(c.NextProtos).To(Equal([]string{"http/1.1"})) }) - It("should handle no protocols", func() { + It("should handle no protocols by preserving nil semantics", func() { fn := SetNextProtos() c := &tls.Config{} fn(c) - Expect(c.NextProtos).To(BeEmpty()) + Expect(c.NextProtos).To(BeNil()) }) It("should not be affected by modification of the original slice", func() { From 2664adee34429aa3ddfd493edc664482cdac16b0 Mon Sep 17 00:00:00 2001 From: Ugo Giordano Date: Thu, 25 Jun 2026 11:06:47 +0200 Subject: [PATCH 3/3] refactor: drop WithHTTP2, add empty-string filtering to SetNextProtos Address review feedback from joelanford: WithHTTP2 is too opinionated for a shared library. It silently replaces the entire NextProtos list, which doesn't compose well when controller-runtime or other callers already have protos set. Changes: - Remove WithHTTP2 function entirely - Add empty-string filtering to SetNextProtos so callers can use conditional inclusion patterns without wrapper functions - Update godoc examples to show the well-known protocol list vars - Replace WithHTTP2 tests with empty-string filtering and composability tests Callers are now explicit about what they're setting: SetNextProtos(openshifttls.HTTP1NextProtos...) SetNextProtos(openshifttls.HTTP2NextProtos...) SetNextProtos("h2", "http/1.1") Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/tls/tls.go | 42 ++++++++++++++------------------------- pkg/tls/tls_test.go | 48 +++++++++++++++++++++++++-------------------- 2 files changed, 42 insertions(+), 48 deletions(-) diff --git a/pkg/tls/tls.go b/pkg/tls/tls.go index 96ca187f..53c159c7 100644 --- a/pkg/tls/tls.go +++ b/pkg/tls/tls.go @@ -142,45 +142,33 @@ func NewTLSConfigFromProfile(profile configv1.TLSProfileSpec) (tlsConfig func(*t } // SetNextProtos returns a TLS configuration function that sets the ALPN -// protocol negotiation list on a tls.Config. +// protocol negotiation list on a tls.Config. Empty strings are silently +// ignored, which allows conditional protocol inclusion. // The returned function is intended to be used with controller-runtime's TLSOpts. // // Example: // -// tlsOpts := []func(*tls.Config){ -// openshifttls.SetNextProtos("h2", "http/1.1"), -// } +// // Disable HTTP/2: +// openshifttls.SetNextProtos("http/1.1") +// +// // Enable HTTP/2 with fallback: +// openshifttls.SetNextProtos("h2", "http/1.1") +// +// // Using the well-known protocol lists: +// openshifttls.SetNextProtos(openshifttls.HTTP1NextProtos...) +// openshifttls.SetNextProtos(openshifttls.HTTP2NextProtos...) func SetNextProtos(protos ...string) func(*tls.Config) { var p []string - if len(protos) > 0 { - p = make([]string, len(protos)) - copy(p, protos) + for _, proto := range protos { + if proto != "" { + p = append(p, proto) + } } return func(c *tls.Config) { c.NextProtos = p } } -// WithHTTP2 returns a TLS configuration function that configures ALPN -// protocol negotiation based on whether HTTP/2 should be enabled. -// -// When enabled is true, both "h2" and "http/1.1" are advertised. -// When enabled is false, only "http/1.1" is advertised as a mitigation -// for HTTP/2 Rapid Reset vulnerabilities (CVE-2023-44487, CVE-2023-39325). -// -// The returned function is intended to be used with controller-runtime's TLSOpts. -// -// Example: -// -// tlsConfig, _ := openshifttls.NewTLSConfigFromProfile(profile) -// tlsOpts := []func(*tls.Config){tlsConfig, openshifttls.WithHTTP2(false)} -func WithHTTP2(enabled bool) func(*tls.Config) { - if enabled { - return SetNextProtos(HTTP2NextProtos...) - } - return SetNextProtos(HTTP1NextProtos...) -} - // cipherCode returns the TLS cipher code for an OpenSSL or IANA cipher name. // Returns 0 if the cipher is not supported. func cipherCode(cipher string) uint16 { diff --git a/pkg/tls/tls_test.go b/pkg/tls/tls_test.go index 95b11d27..5e63375b 100644 --- a/pkg/tls/tls_test.go +++ b/pkg/tls/tls_test.go @@ -212,6 +212,20 @@ var _ = Describe("SetNextProtos", func() { Expect(c.NextProtos).To(BeNil()) }) + It("should ignore empty strings", func() { + fn := SetNextProtos("", "http/1.1") + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"http/1.1"})) + }) + + It("should return nil when all protocols are empty strings", func() { + fn := SetNextProtos("", "") + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(BeNil()) + }) + It("should not be affected by modification of the original slice", func() { protos := []string{"h2", "http/1.1"} fn := SetNextProtos(protos...) @@ -227,26 +241,6 @@ var _ = Describe("SetNextProtos", func() { fn(c) Expect(c.NextProtos).To(Equal([]string{"h2", "http/1.1"})) }) -}) - -var _ = Describe("WithHTTP2", func() { - Context("when HTTP/2 is enabled", func() { - It("should set NextProtos to h2 and http/1.1", func() { - fn := WithHTTP2(true) - c := &tls.Config{} - fn(c) - Expect(c.NextProtos).To(Equal([]string{"h2", "http/1.1"})) - }) - }) - - Context("when HTTP/2 is disabled", func() { - It("should set NextProtos to http/1.1 only", func() { - fn := WithHTTP2(false) - c := &tls.Config{} - fn(c) - Expect(c.NextProtos).To(Equal([]string{"http/1.1"})) - }) - }) It("should compose with NewTLSConfigFromProfile", func() { profile := *configv1.TLSProfiles[configv1.TLSProfileIntermediateType] @@ -254,12 +248,24 @@ var _ = Describe("WithHTTP2", func() { c := &tls.Config{} tlsConfigFn(c) - WithHTTP2(false)(c) + SetNextProtos("http/1.1")(c) Expect(c.MinVersion).To(Equal(uint16(tls.VersionTLS12))) Expect(c.CipherSuites).NotTo(BeEmpty()) Expect(c.NextProtos).To(Equal([]string{"http/1.1"})) }) + + It("should work with the well-known protocol lists", func() { + fn := SetNextProtos(HTTP2NextProtos...) + c := &tls.Config{} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"h2", "http/1.1"})) + + fn = SetNextProtos(HTTP1NextProtos...) + c = &tls.Config{} + fn(c) + Expect(c.NextProtos).To(Equal([]string{"http/1.1"})) + }) }) var _ = Describe("ALPN protocol variables", func() {