From 70b62f92c88d01417a60af06dcb12c0e3a4c782e Mon Sep 17 00:00:00 2001 From: Coleen Iona Quadros Date: Mon, 10 Aug 2026 14:44:01 +0400 Subject: [PATCH 1/2] Add support for configurable TLS profiles Add --tls-min-version and --tls-cipher-suites flags to enable oauth-proxy to honor cluster-wide TLS security profiles. - Use library-go (oscrypto) for TLS validation and parsing - Parse comma-separated cipher suites (MCO compatibility) - Reject TLS 1.0/1.1 and insecure ciphers (RC4, 3DES, CBC-SHA1) - Validate in Options.Validate() for early error detection - Comprehensive unit tests for validation and security Fixes: #352 Signed-off-by: Coleen Iona Quadros --- go.mod | 2 +- http.go | 40 ++++++++++++- http_test.go | 125 ++++++++++++++++++++++++++++++++++++++++ main.go | 15 +++++ options.go | 41 +++++++++++++ options_tls_test.go | 136 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 355 insertions(+), 4 deletions(-) create mode 100644 http_test.go create mode 100644 options_tls_test.go diff --git a/go.mod b/go.mod index f02f111ed..0f1a37c80 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( k8s.io/apimachinery v0.33.3 k8s.io/apiserver v0.33.3 k8s.io/client-go v0.33.3 + k8s.io/component-base v0.33.3 k8s.io/utils v0.0.0-20241210054802-24370beab758 ) @@ -103,7 +104,6 @@ require ( gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/component-base v0.33.3 // indirect k8s.io/klog/v2 v2.130.1 // indirect k8s.io/kms v0.33.3 // indirect k8s.io/kube-openapi v0.0.0-20250318190949-c8a335a9a2ff // indirect diff --git a/http.go b/http.go index 32fd62362..3af5204ed 100644 --- a/http.go +++ b/http.go @@ -72,14 +72,48 @@ func (s *Server) ServeHTTP() { log.Printf("HTTP: closing %s", listener.Addr()) } -func (s *Server) ServeHTTPS(ctx context.Context) { - addr := s.Opts.HttpsAddress - +// buildTLSConfig constructs a TLS configuration from Options. +// It starts with library-go secure defaults and applies user overrides. +func (s *Server) buildTLSConfig() *tls.Config { + // Start with secure defaults from library-go config := oscrypto.SecureTLSConfig(&tls.Config{}) if config.NextProtos == nil { config.NextProtos = []string{"http/1.1"} } + // Override with user-specified TLS settings if provided + // Note: validation happens in Options.Validate(), but we log warnings defensively + if s.Opts.TLSMinVersion != "" { + minVersion, err := oscrypto.TLSVersion(s.Opts.TLSMinVersion) + if err != nil { + log.Printf("WARNING: invalid tls-min-version %q: %v", s.Opts.TLSMinVersion, err) + } else if minVersion != 0 { + config.MinVersion = minVersion + } + } + + if len(s.Opts.TLSCipherSuites) > 0 { + cipherSuites := make([]uint16, 0, len(s.Opts.TLSCipherSuites)) + for _, cipherName := range s.Opts.TLSCipherSuites { + cipher, err := oscrypto.CipherSuite(cipherName) + if err != nil { + log.Printf("WARNING: invalid cipher suite %q: %v", cipherName, err) + continue + } + cipherSuites = append(cipherSuites, cipher) + } + if len(cipherSuites) > 0 { + config.CipherSuites = cipherSuites + } + } + + return config +} + +func (s *Server) ServeHTTPS(ctx context.Context) { + addr := s.Opts.HttpsAddress + config := s.buildTLSConfig() + var err error servingCertProvider, err := dynamiccertificates.NewDynamicServingContentFromFiles("serving", s.Opts.TLSCertFile, s.Opts.TLSKeyFile) if err != nil { diff --git a/http_test.go b/http_test.go new file mode 100644 index 000000000..66e53079a --- /dev/null +++ b/http_test.go @@ -0,0 +1,125 @@ +package main + +import ( + "crypto/tls" + "testing" +) + +func TestBuildTLSConfig_Defaults(t *testing.T) { + // Test that without TLS flags, library-go defaults are used + opts := &Options{ + HttpsAddress: ":8443", + // TLSMinVersion and TLSCipherSuites are empty + } + + s := &Server{Opts: opts} + config := s.buildTLSConfig() + + // Verify library-go defaults are applied + if config.MinVersion != tls.VersionTLS12 { + t.Errorf("Expected default MinVersion TLS 1.2 (%d), got %d", tls.VersionTLS12, config.MinVersion) + } + + if len(config.CipherSuites) == 0 { + t.Error("Expected library-go to set default cipher suites, got empty list") + } + + if config.NextProtos == nil || len(config.NextProtos) == 0 { + t.Error("Expected NextProtos to be set") + } +} + +func TestBuildTLSConfig_MinVersion(t *testing.T) { + tests := []struct { + name string + minVersion string + expected uint16 + wantWarning bool + }{ + { + name: "VersionTLS12 overrides default", + minVersion: "VersionTLS12", + expected: tls.VersionTLS12, + wantWarning: false, + }, + { + name: "VersionTLS13 overrides default", + minVersion: "VersionTLS13", + expected: tls.VersionTLS13, + wantWarning: false, + }, + { + name: "invalid version logs warning but doesn't crash", + minVersion: "VersionTLS99", + expected: tls.VersionTLS12, // Falls back to library-go default + wantWarning: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := &Options{ + TLSMinVersion: tt.minVersion, + } + + s := &Server{Opts: opts} + config := s.buildTLSConfig() + + if config.MinVersion != tt.expected { + t.Errorf("Expected MinVersion %d, got %d", tt.expected, config.MinVersion) + } + }) + } +} + +func TestBuildTLSConfig_CipherSuites(t *testing.T) { + tests := []struct { + name string + cipherSuites []string + expectCount int + wantWarning bool + }{ + { + name: "single valid cipher", + cipherSuites: []string{ + "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + }, + expectCount: 1, + wantWarning: false, + }, + { + name: "multiple valid ciphers", + cipherSuites: []string{ + "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", + }, + expectCount: 2, + wantWarning: false, + }, + { + name: "invalid cipher logs warning and skips it", + cipherSuites: []string{ + "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + "TLS_INVALID_CIPHER", + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", + }, + expectCount: 2, // Only valid ciphers + wantWarning: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := &Options{ + TLSCipherSuites: tt.cipherSuites, + } + + s := &Server{Opts: opts} + config := s.buildTLSConfig() + + if len(config.CipherSuites) != tt.expectCount { + t.Errorf("Expected %d cipher suites, got %d", tt.expectCount, len(config.CipherSuites)) + } + }) + } +} diff --git a/main.go b/main.go index 637874d70..5e6f0e29b 100644 --- a/main.go +++ b/main.go @@ -32,6 +32,7 @@ func main() { openshiftCAs := NewStringArray() clientCA := "" upstreamCAs := NewStringArray() + tlsCipherSuitesStr := "" config := flagSet.String("config", "", "path to config file") showVersion := flagSet.Bool("version", false, "print version string") @@ -42,6 +43,8 @@ func main() { flagSet.String("tls-cert", "", "path to certificate file") flagSet.String("tls-key", "", "path to private key file") flagSet.StringVar(&clientCA, "tls-client-ca", clientCA, "path to a CA file for admitting client certificates.") + flagSet.String("tls-min-version", "", "minimum TLS version (e.g., VersionTLS12, VersionTLS13). If not set, uses secure default (TLS 1.2)") + flagSet.StringVar(&tlsCipherSuitesStr, "tls-cipher-suites", "", "comma-separated list of TLS cipher suites to support (e.g., TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256). If not set, uses secure defaults") flagSet.String("redirect-url", "", "the OAuth Redirect URL. ie: \"https://internalapp.yourcompany.com/oauth/callback\"") flagSet.Bool("set-xauthrequest", false, "set X-Auth-Request-User and X-Auth-Request-Email response headers (useful in Nginx auth_request mode)") flagSet.Var(upstreams, "upstream", "the http url(s) of the upstream endpoint or file:// paths for static files. Routing is based on the path") @@ -131,6 +134,18 @@ func main() { cfg.LoadEnvForStruct(opts) options.Resolve(opts, flagSet, cfg) + // Parse comma-separated TLS cipher suites from CLI (overrides config file) + if tlsCipherSuitesStr != "" { + cipherSuites := strings.Split(tlsCipherSuitesStr, ",") + opts.TLSCipherSuites = make([]string, 0, len(cipherSuites)) + for _, cipher := range cipherSuites { + trimmed := strings.TrimSpace(cipher) + if trimmed != "" { + opts.TLSCipherSuites = append(opts.TLSCipherSuites, trimmed) + } + } + } + var p providers.Provider switch opts.Provider { case "openshift": diff --git a/options.go b/options.go index 33f3faf01..f1a3a3861 100644 --- a/options.go +++ b/options.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "fmt" "io/ioutil" + "log" "net/http" "net/url" "regexp" @@ -13,6 +14,7 @@ import ( "time" "github.com/18F/hmacauth" + k8sapiflag "k8s.io/component-base/cli/flag" oscrypto "github.com/openshift/library-go/pkg/crypto" @@ -40,6 +42,8 @@ type Options struct { TLSCertFile string `flag:"tls-cert" cfg:"tls_cert_file"` TLSKeyFile string `flag:"tls-key" cfg:"tls_key_file"` TLSClientCAFile string `flag:"tls-client-ca" cfg:"tls_client_ca"` + TLSMinVersion string `flag:"tls-min-version" cfg:"tls_min_version"` + TLSCipherSuites []string `cfg:"tls_cipher_suites"` // No flag tag - manually parsed after Resolve() AuthenticatedEmailsFile string `flag:"authenticated-emails-file" cfg:"authenticated_emails_file"` EmailDomains []string `flag:"email-domain" cfg:"email_domains"` @@ -326,6 +330,43 @@ func (o *Options) Validate(p providers.Provider) error { http.DefaultClient = &http.Client{Transport: insecureTransport} } + // Validate TLS configuration early + if o.TLSMinVersion != "" || len(o.TLSCipherSuites) > 0 { + // Warn if TLS flags are set but HTTPS is disabled + if o.HttpsAddress == "" { + log.Printf("WARNING: TLS flags (tls-min-version, tls-cipher-suites) are set but HTTPS is disabled (--https-address=\"\")") + } + + // Validate TLS minimum version + if o.TLSMinVersion != "" { + minVersion, err := oscrypto.TLSVersion(o.TLSMinVersion) + if err != nil { + msgs = append(msgs, fmt.Sprintf("invalid tls-min-version: %v", err)) + } else if minVersion != 0 && minVersion < tls.VersionTLS12 { + msgs = append(msgs, fmt.Sprintf("tls-min-version must be VersionTLS12 or VersionTLS13, got: %s (TLS 1.0 and 1.1 are insecure and not supported)", o.TLSMinVersion)) + } + } + + // Validate TLS cipher suites + if len(o.TLSCipherSuites) > 0 { + insecureCiphers := k8sapiflag.InsecureTLSCiphers() + + for _, cipherName := range o.TLSCipherSuites { + // Validate cipher name exists (library-go) + _, err := oscrypto.CipherSuite(cipherName) + if err != nil { + msgs = append(msgs, fmt.Sprintf("invalid cipher suite %q: %v", cipherName, err)) + continue + } + + // Reject insecure cipher suites (RC4, 3DES, CBC with SHA1, etc.) + if _, isInsecure := insecureCiphers[cipherName]; isInsecure { + msgs = append(msgs, fmt.Sprintf("insecure cipher suite %q is not allowed (vulnerable to known attacks)", cipherName)) + } + } + } + } + msgs = append(msgs, o.validateProvider(p)...) if len(msgs) != 0 { return fmt.Errorf("Invalid configuration:\n %s", diff --git a/options_tls_test.go b/options_tls_test.go new file mode 100644 index 000000000..3ba49755f --- /dev/null +++ b/options_tls_test.go @@ -0,0 +1,136 @@ +package main + +import ( + "strings" + "testing" +) + +func TestOptions_Validate_TLS(t *testing.T) { + tests := []struct { + name string + tlsMinVersion string + tlsCipherSuites []string + httpsAddress string + wantErr bool + errContains string + }{ + { + name: "empty TLS config is valid", + tlsMinVersion: "", + httpsAddress: ":8443", + wantErr: false, + }, + { + name: "TLS 1.2 is allowed", + tlsMinVersion: "VersionTLS12", + httpsAddress: ":8443", + wantErr: false, + }, + { + name: "TLS 1.3 is allowed", + tlsMinVersion: "VersionTLS13", + httpsAddress: ":8443", + wantErr: false, + }, + { + name: "TLS 1.0 is rejected (insecure)", + tlsMinVersion: "VersionTLS10", + httpsAddress: ":8443", + wantErr: true, + errContains: "tls-min-version must be VersionTLS12 or VersionTLS13", + }, + { + name: "TLS 1.1 is rejected (insecure)", + tlsMinVersion: "VersionTLS11", + httpsAddress: ":8443", + wantErr: true, + errContains: "tls-min-version must be VersionTLS12 or VersionTLS13", + }, + { + name: "unknown TLS version is rejected", + tlsMinVersion: "VersionTLS99", + httpsAddress: ":8443", + wantErr: true, + errContains: "invalid tls-min-version", + }, + { + name: "valid cipher suites are allowed", + tlsCipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"}, + httpsAddress: ":8443", + wantErr: false, + }, + { + name: "multiple valid cipher suites are allowed", + tlsCipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"}, + httpsAddress: ":8443", + wantErr: false, + }, + { + name: "unknown cipher suite is rejected", + tlsCipherSuites: []string{"TLS_UNKNOWN_CIPHER"}, + httpsAddress: ":8443", + wantErr: true, + errContains: "invalid cipher suite", + }, + { + name: "insecure cipher RC4 is rejected", + tlsCipherSuites: []string{"TLS_RSA_WITH_RC4_128_SHA"}, + httpsAddress: ":8443", + wantErr: true, + errContains: "insecure cipher suite", + }, + { + name: "insecure cipher 3DES is rejected", + tlsCipherSuites: []string{"TLS_RSA_WITH_3DES_EDE_CBC_SHA"}, + httpsAddress: ":8443", + wantErr: true, + errContains: "insecure cipher suite", + }, + { + name: "mix of secure and insecure ciphers is rejected", + tlsCipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_RSA_WITH_RC4_128_SHA"}, + httpsAddress: ":8443", + wantErr: true, + errContains: "insecure cipher suite", + }, + { + name: "TLS config with HTTPS disabled", + tlsMinVersion: "VersionTLS12", + httpsAddress: "", // HTTPS disabled + wantErr: false, + // Note: This triggers a WARNING but doesn't fail validation + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Start with defaults + opts := NewOptions() + opts.Upstreams = []string{"http://localhost:8080"} + opts.CookieSecret = "0123456789abcdef" // 16 bytes + opts.ClientID = "client" + opts.ClientSecret = "secret" + opts.EmailDomains = []string{"*"} + opts.RedirectURL = "https:///" + opts.TLSMinVersion = tt.tlsMinVersion + opts.TLSCipherSuites = tt.tlsCipherSuites + opts.HttpsAddress = tt.httpsAddress + + err := opts.Validate(&testProvider{}) + + if tt.wantErr { + if err == nil { + t.Errorf("Validate() expected error containing %q, got nil", tt.errContains) + return + } + if !strings.Contains(err.Error(), tt.errContains) { + t.Errorf("Validate() error = %q, want error containing %q", err.Error(), tt.errContains) + } + } else { + if err != nil { + t.Errorf("Validate() unexpected error = %v", err) + } + } + }) + } +} From fe67cea3caec3270e0713b88c99fe978d8a87574 Mon Sep 17 00:00:00 2001 From: Coleen Iona Quadros Date: Fri, 21 Aug 2026 17:14:34 +0400 Subject: [PATCH 2/2] Simplify TLS config: drop validation, default to TLS 1.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per reviewer feedback, oauth-proxy is a sidecar — the deploying operator owns TLS policy decisions. Remove all validation from Validate(), use Go stdlib crypto/tls directly instead of library-go for TLS config construction, and default to TLS 1.3 minimum. Signed-off-by: Coleen Iona Quadros --- go.mod | 2 +- http.go | 53 +++++++++-------- http_test.go | 92 +++++++++--------------------- main.go | 4 +- options.go | 39 ------------- options_tls_test.go | 136 -------------------------------------------- 6 files changed, 58 insertions(+), 268 deletions(-) delete mode 100644 options_tls_test.go diff --git a/go.mod b/go.mod index 0f1a37c80..f02f111ed 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,6 @@ require ( k8s.io/apimachinery v0.33.3 k8s.io/apiserver v0.33.3 k8s.io/client-go v0.33.3 - k8s.io/component-base v0.33.3 k8s.io/utils v0.0.0-20241210054802-24370beab758 ) @@ -104,6 +103,7 @@ require ( gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect + k8s.io/component-base v0.33.3 // indirect k8s.io/klog/v2 v2.130.1 // indirect k8s.io/kms v0.33.3 // indirect k8s.io/kube-openapi v0.0.0-20250318190949-c8a335a9a2ff // indirect diff --git a/http.go b/http.go index 3af5204ed..363de8182 100644 --- a/http.go +++ b/http.go @@ -11,8 +11,6 @@ import ( "k8s.io/apiserver/pkg/server/dynamiccertificates" - oscrypto "github.com/openshift/library-go/pkg/crypto" - "github.com/openshift/oauth-proxy/util" ) @@ -72,44 +70,51 @@ func (s *Server) ServeHTTP() { log.Printf("HTTP: closing %s", listener.Addr()) } -// buildTLSConfig constructs a TLS configuration from Options. -// It starts with library-go secure defaults and applies user overrides. func (s *Server) buildTLSConfig() *tls.Config { - // Start with secure defaults from library-go - config := oscrypto.SecureTLSConfig(&tls.Config{}) - if config.NextProtos == nil { - config.NextProtos = []string{"http/1.1"} + config := &tls.Config{ + MinVersion: tls.VersionTLS13, + NextProtos: []string{"http/1.1"}, } - // Override with user-specified TLS settings if provided - // Note: validation happens in Options.Validate(), but we log warnings defensively if s.Opts.TLSMinVersion != "" { - minVersion, err := oscrypto.TLSVersion(s.Opts.TLSMinVersion) - if err != nil { - log.Printf("WARNING: invalid tls-min-version %q: %v", s.Opts.TLSMinVersion, err) - } else if minVersion != 0 { - config.MinVersion = minVersion + if v, ok := tlsVersionMap[s.Opts.TLSMinVersion]; ok { + config.MinVersion = v } } if len(s.Opts.TLSCipherSuites) > 0 { - cipherSuites := make([]uint16, 0, len(s.Opts.TLSCipherSuites)) - for _, cipherName := range s.Opts.TLSCipherSuites { - cipher, err := oscrypto.CipherSuite(cipherName) - if err != nil { - log.Printf("WARNING: invalid cipher suite %q: %v", cipherName, err) - continue + suites := make([]uint16, 0, len(s.Opts.TLSCipherSuites)) + for _, name := range s.Opts.TLSCipherSuites { + if id, ok := tlsCipherSuiteMap[name]; ok { + suites = append(suites, id) } - cipherSuites = append(cipherSuites, cipher) } - if len(cipherSuites) > 0 { - config.CipherSuites = cipherSuites + if len(suites) > 0 { + config.CipherSuites = suites } } return config } +var tlsVersionMap = map[string]uint16{ + "VersionTLS10": tls.VersionTLS10, + "VersionTLS11": tls.VersionTLS11, + "VersionTLS12": tls.VersionTLS12, + "VersionTLS13": tls.VersionTLS13, +} + +var tlsCipherSuiteMap = func() map[string]uint16 { + m := make(map[string]uint16) + for _, cs := range tls.CipherSuites() { + m[cs.Name] = cs.ID + } + for _, cs := range tls.InsecureCipherSuites() { + m[cs.Name] = cs.ID + } + return m +}() + func (s *Server) ServeHTTPS(ctx context.Context) { addr := s.Opts.HttpsAddress config := s.buildTLSConfig() diff --git a/http_test.go b/http_test.go index 66e53079a..ffcbf43f9 100644 --- a/http_test.go +++ b/http_test.go @@ -6,63 +6,40 @@ import ( ) func TestBuildTLSConfig_Defaults(t *testing.T) { - // Test that without TLS flags, library-go defaults are used - opts := &Options{ - HttpsAddress: ":8443", - // TLSMinVersion and TLSCipherSuites are empty - } - - s := &Server{Opts: opts} + s := &Server{Opts: &Options{}} config := s.buildTLSConfig() - // Verify library-go defaults are applied - if config.MinVersion != tls.VersionTLS12 { - t.Errorf("Expected default MinVersion TLS 1.2 (%d), got %d", tls.VersionTLS12, config.MinVersion) - } - - if len(config.CipherSuites) == 0 { - t.Error("Expected library-go to set default cipher suites, got empty list") - } - - if config.NextProtos == nil || len(config.NextProtos) == 0 { - t.Error("Expected NextProtos to be set") + if config.MinVersion != tls.VersionTLS13 { + t.Errorf("Expected default MinVersion TLS 1.3 (%d), got %d", tls.VersionTLS13, config.MinVersion) } } func TestBuildTLSConfig_MinVersion(t *testing.T) { tests := []struct { - name string - minVersion string - expected uint16 - wantWarning bool + name string + minVersion string + expected uint16 }{ { - name: "VersionTLS12 overrides default", - minVersion: "VersionTLS12", - expected: tls.VersionTLS12, - wantWarning: false, + name: "VersionTLS12", + minVersion: "VersionTLS12", + expected: tls.VersionTLS12, }, { - name: "VersionTLS13 overrides default", - minVersion: "VersionTLS13", - expected: tls.VersionTLS13, - wantWarning: false, + name: "VersionTLS13", + minVersion: "VersionTLS13", + expected: tls.VersionTLS13, }, { - name: "invalid version logs warning but doesn't crash", - minVersion: "VersionTLS99", - expected: tls.VersionTLS12, // Falls back to library-go default - wantWarning: true, + name: "unknown version keeps default", + minVersion: "VersionTLS99", + expected: tls.VersionTLS13, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - opts := &Options{ - TLSMinVersion: tt.minVersion, - } - - s := &Server{Opts: opts} + s := &Server{Opts: &Options{TLSMinVersion: tt.minVersion}} config := s.buildTLSConfig() if config.MinVersion != tt.expected { @@ -77,44 +54,27 @@ func TestBuildTLSConfig_CipherSuites(t *testing.T) { name string cipherSuites []string expectCount int - wantWarning bool }{ { - name: "single valid cipher", - cipherSuites: []string{ - "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", - }, - expectCount: 1, - wantWarning: false, + name: "single valid cipher", + cipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"}, + expectCount: 1, }, { - name: "multiple valid ciphers", - cipherSuites: []string{ - "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", - }, - expectCount: 2, - wantWarning: false, + name: "multiple valid ciphers", + cipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"}, + expectCount: 2, }, { - name: "invalid cipher logs warning and skips it", - cipherSuites: []string{ - "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", - "TLS_INVALID_CIPHER", - "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", - }, - expectCount: 2, // Only valid ciphers - wantWarning: true, + name: "unknown cipher is skipped", + cipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_INVALID_CIPHER"}, + expectCount: 1, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - opts := &Options{ - TLSCipherSuites: tt.cipherSuites, - } - - s := &Server{Opts: opts} + s := &Server{Opts: &Options{TLSCipherSuites: tt.cipherSuites}} config := s.buildTLSConfig() if len(config.CipherSuites) != tt.expectCount { diff --git a/main.go b/main.go index 5e6f0e29b..1cd6d0aff 100644 --- a/main.go +++ b/main.go @@ -43,8 +43,8 @@ func main() { flagSet.String("tls-cert", "", "path to certificate file") flagSet.String("tls-key", "", "path to private key file") flagSet.StringVar(&clientCA, "tls-client-ca", clientCA, "path to a CA file for admitting client certificates.") - flagSet.String("tls-min-version", "", "minimum TLS version (e.g., VersionTLS12, VersionTLS13). If not set, uses secure default (TLS 1.2)") - flagSet.StringVar(&tlsCipherSuitesStr, "tls-cipher-suites", "", "comma-separated list of TLS cipher suites to support (e.g., TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256). If not set, uses secure defaults") + flagSet.String("tls-min-version", "", "minimum TLS version (e.g., VersionTLS12, VersionTLS13). Defaults to TLS 1.3") + flagSet.StringVar(&tlsCipherSuitesStr, "tls-cipher-suites", "", "comma-separated list of TLS cipher suites") flagSet.String("redirect-url", "", "the OAuth Redirect URL. ie: \"https://internalapp.yourcompany.com/oauth/callback\"") flagSet.Bool("set-xauthrequest", false, "set X-Auth-Request-User and X-Auth-Request-Email response headers (useful in Nginx auth_request mode)") flagSet.Var(upstreams, "upstream", "the http url(s) of the upstream endpoint or file:// paths for static files. Routing is based on the path") diff --git a/options.go b/options.go index f1a3a3861..3fca3bd31 100644 --- a/options.go +++ b/options.go @@ -6,7 +6,6 @@ import ( "encoding/base64" "fmt" "io/ioutil" - "log" "net/http" "net/url" "regexp" @@ -14,7 +13,6 @@ import ( "time" "github.com/18F/hmacauth" - k8sapiflag "k8s.io/component-base/cli/flag" oscrypto "github.com/openshift/library-go/pkg/crypto" @@ -330,43 +328,6 @@ func (o *Options) Validate(p providers.Provider) error { http.DefaultClient = &http.Client{Transport: insecureTransport} } - // Validate TLS configuration early - if o.TLSMinVersion != "" || len(o.TLSCipherSuites) > 0 { - // Warn if TLS flags are set but HTTPS is disabled - if o.HttpsAddress == "" { - log.Printf("WARNING: TLS flags (tls-min-version, tls-cipher-suites) are set but HTTPS is disabled (--https-address=\"\")") - } - - // Validate TLS minimum version - if o.TLSMinVersion != "" { - minVersion, err := oscrypto.TLSVersion(o.TLSMinVersion) - if err != nil { - msgs = append(msgs, fmt.Sprintf("invalid tls-min-version: %v", err)) - } else if minVersion != 0 && minVersion < tls.VersionTLS12 { - msgs = append(msgs, fmt.Sprintf("tls-min-version must be VersionTLS12 or VersionTLS13, got: %s (TLS 1.0 and 1.1 are insecure and not supported)", o.TLSMinVersion)) - } - } - - // Validate TLS cipher suites - if len(o.TLSCipherSuites) > 0 { - insecureCiphers := k8sapiflag.InsecureTLSCiphers() - - for _, cipherName := range o.TLSCipherSuites { - // Validate cipher name exists (library-go) - _, err := oscrypto.CipherSuite(cipherName) - if err != nil { - msgs = append(msgs, fmt.Sprintf("invalid cipher suite %q: %v", cipherName, err)) - continue - } - - // Reject insecure cipher suites (RC4, 3DES, CBC with SHA1, etc.) - if _, isInsecure := insecureCiphers[cipherName]; isInsecure { - msgs = append(msgs, fmt.Sprintf("insecure cipher suite %q is not allowed (vulnerable to known attacks)", cipherName)) - } - } - } - } - msgs = append(msgs, o.validateProvider(p)...) if len(msgs) != 0 { return fmt.Errorf("Invalid configuration:\n %s", diff --git a/options_tls_test.go b/options_tls_test.go deleted file mode 100644 index 3ba49755f..000000000 --- a/options_tls_test.go +++ /dev/null @@ -1,136 +0,0 @@ -package main - -import ( - "strings" - "testing" -) - -func TestOptions_Validate_TLS(t *testing.T) { - tests := []struct { - name string - tlsMinVersion string - tlsCipherSuites []string - httpsAddress string - wantErr bool - errContains string - }{ - { - name: "empty TLS config is valid", - tlsMinVersion: "", - httpsAddress: ":8443", - wantErr: false, - }, - { - name: "TLS 1.2 is allowed", - tlsMinVersion: "VersionTLS12", - httpsAddress: ":8443", - wantErr: false, - }, - { - name: "TLS 1.3 is allowed", - tlsMinVersion: "VersionTLS13", - httpsAddress: ":8443", - wantErr: false, - }, - { - name: "TLS 1.0 is rejected (insecure)", - tlsMinVersion: "VersionTLS10", - httpsAddress: ":8443", - wantErr: true, - errContains: "tls-min-version must be VersionTLS12 or VersionTLS13", - }, - { - name: "TLS 1.1 is rejected (insecure)", - tlsMinVersion: "VersionTLS11", - httpsAddress: ":8443", - wantErr: true, - errContains: "tls-min-version must be VersionTLS12 or VersionTLS13", - }, - { - name: "unknown TLS version is rejected", - tlsMinVersion: "VersionTLS99", - httpsAddress: ":8443", - wantErr: true, - errContains: "invalid tls-min-version", - }, - { - name: "valid cipher suites are allowed", - tlsCipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"}, - httpsAddress: ":8443", - wantErr: false, - }, - { - name: "multiple valid cipher suites are allowed", - tlsCipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"}, - httpsAddress: ":8443", - wantErr: false, - }, - { - name: "unknown cipher suite is rejected", - tlsCipherSuites: []string{"TLS_UNKNOWN_CIPHER"}, - httpsAddress: ":8443", - wantErr: true, - errContains: "invalid cipher suite", - }, - { - name: "insecure cipher RC4 is rejected", - tlsCipherSuites: []string{"TLS_RSA_WITH_RC4_128_SHA"}, - httpsAddress: ":8443", - wantErr: true, - errContains: "insecure cipher suite", - }, - { - name: "insecure cipher 3DES is rejected", - tlsCipherSuites: []string{"TLS_RSA_WITH_3DES_EDE_CBC_SHA"}, - httpsAddress: ":8443", - wantErr: true, - errContains: "insecure cipher suite", - }, - { - name: "mix of secure and insecure ciphers is rejected", - tlsCipherSuites: []string{"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_RSA_WITH_RC4_128_SHA"}, - httpsAddress: ":8443", - wantErr: true, - errContains: "insecure cipher suite", - }, - { - name: "TLS config with HTTPS disabled", - tlsMinVersion: "VersionTLS12", - httpsAddress: "", // HTTPS disabled - wantErr: false, - // Note: This triggers a WARNING but doesn't fail validation - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Start with defaults - opts := NewOptions() - opts.Upstreams = []string{"http://localhost:8080"} - opts.CookieSecret = "0123456789abcdef" // 16 bytes - opts.ClientID = "client" - opts.ClientSecret = "secret" - opts.EmailDomains = []string{"*"} - opts.RedirectURL = "https:///" - opts.TLSMinVersion = tt.tlsMinVersion - opts.TLSCipherSuites = tt.tlsCipherSuites - opts.HttpsAddress = tt.httpsAddress - - err := opts.Validate(&testProvider{}) - - if tt.wantErr { - if err == nil { - t.Errorf("Validate() expected error containing %q, got nil", tt.errContains) - return - } - if !strings.Contains(err.Error(), tt.errContains) { - t.Errorf("Validate() error = %q, want error containing %q", err.Error(), tt.errContains) - } - } else { - if err != nil { - t.Errorf("Validate() unexpected error = %v", err) - } - } - }) - } -}