From 577a0865ae72e7f630ec96f55fb7cc7cbbb7898a Mon Sep 17 00:00:00 2001 From: Vincent Link Date: Wed, 4 Mar 2026 10:38:02 +0100 Subject: [PATCH] Replace NewConfigFromEnv with DefaultConfigFromEnv DefaultConfigFromEnv replaces NewConfigFromEnv by returning a full default tls.Config with overrides from env vars. This avoids specifying e.g. the TLS MinVersion explicitely. --- tls/config.go | 34 ++++------------ tls/config_test.go | 65 ++++++++----------------------- webhook/env.go | 2 +- webhook/webhook.go | 96 ++++++++++++++++++++-------------------------- 4 files changed, 65 insertions(+), 132 deletions(-) diff --git a/tls/config.go b/tls/config.go index 4d07cbb5bb..6cd205baeb 100644 --- a/tls/config.go +++ b/tls/config.go @@ -33,22 +33,14 @@ const ( CurvePreferencesEnvKey = "TLS_CURVE_PREFERENCES" ) -// Config holds parsed TLS configuration values that can be used -// to build a *crypto/tls.Config. -type Config struct { - MinVersion uint16 - MaxVersion uint16 - CipherSuites []uint16 - CurvePreferences []cryptotls.CurveID -} - -// NewConfigFromEnv reads TLS configuration from environment variables and -// returns a Config. The prefix is prepended to each standard env-var suffix; +// DefaultConfigFromEnv returns a tls.Config with secure defaults. +// The prefix is prepended to each standard env-var suffix; // for example with prefix "WEBHOOK_" the function reads // WEBHOOK_TLS_MIN_VERSION, WEBHOOK_TLS_MAX_VERSION, etc. -// Fields whose corresponding env var is unset are left at their zero value. -func NewConfigFromEnv(prefix string) (*Config, error) { - var cfg Config +func DefaultConfigFromEnv(prefix string) (*cryptotls.Config, error) { + cfg := &cryptotls.Config{ + MinVersion: cryptotls.VersionTLS13, + } if v := os.Getenv(prefix + MinVersionEnvKey); v != "" { ver, err := parseVersion(v) @@ -82,19 +74,7 @@ func NewConfigFromEnv(prefix string) (*Config, error) { cfg.CurvePreferences = curves } - return &cfg, nil -} - -// TLSConfig constructs a *crypto/tls.Config from the parsed configuration. -// The caller typically adds additional fields such as GetCertificate. -func (c *Config) TLSConfig() *cryptotls.Config { - //nolint:gosec // Min version is caller-configurable; default is TLS 1.3. - return &cryptotls.Config{ - MinVersion: c.MinVersion, - MaxVersion: c.MaxVersion, - CipherSuites: c.CipherSuites, - CurvePreferences: c.CurvePreferences, - } + return cfg, nil } // parseVersion converts a TLS version string to the corresponding diff --git a/tls/config_test.go b/tls/config_test.go index b97e4044d5..3ee91da727 100644 --- a/tls/config_test.go +++ b/tls/config_test.go @@ -213,14 +213,14 @@ func Test_parseCurvePreferences(t *testing.T) { } } -func TestNewConfigFromEnv(t *testing.T) { - t.Run("no env vars set returns zero value", func(t *testing.T) { - cfg, err := NewConfigFromEnv("") +func TestDefaultConfigFromEnv(t *testing.T) { + t.Run("no env vars returns TLS 1.3 default", func(t *testing.T) { + cfg, err := DefaultConfigFromEnv("") if err != nil { t.Fatal("unexpected error:", err) } - if cfg.MinVersion != 0 { - t.Errorf("MinVersion = %d, want 0", cfg.MinVersion) + if cfg.MinVersion != cryptotls.VersionTLS13 { + t.Errorf("MinVersion = %d, want %d", cfg.MinVersion, cryptotls.VersionTLS13) } if cfg.MaxVersion != 0 { t.Errorf("MaxVersion = %d, want 0", cfg.MaxVersion) @@ -233,9 +233,9 @@ func TestNewConfigFromEnv(t *testing.T) { } }) - t.Run("min version from env", func(t *testing.T) { + t.Run("min version from env overrides default", func(t *testing.T) { t.Setenv(MinVersionEnvKey, "1.2") - cfg, err := NewConfigFromEnv("") + cfg, err := DefaultConfigFromEnv("") if err != nil { t.Fatal("unexpected error:", err) } @@ -246,7 +246,7 @@ func TestNewConfigFromEnv(t *testing.T) { t.Run("max version from env", func(t *testing.T) { t.Setenv(MaxVersionEnvKey, "1.3") - cfg, err := NewConfigFromEnv("") + cfg, err := DefaultConfigFromEnv("") if err != nil { t.Fatal("unexpected error:", err) } @@ -257,7 +257,7 @@ func TestNewConfigFromEnv(t *testing.T) { t.Run("cipher suites from env", func(t *testing.T) { t.Setenv(CipherSuitesEnvKey, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256") - cfg, err := NewConfigFromEnv("") + cfg, err := DefaultConfigFromEnv("") if err != nil { t.Fatal("unexpected error:", err) } @@ -268,7 +268,7 @@ func TestNewConfigFromEnv(t *testing.T) { t.Run("curve preferences from env", func(t *testing.T) { t.Setenv(CurvePreferencesEnvKey, "X25519,CurveP256") - cfg, err := NewConfigFromEnv("") + cfg, err := DefaultConfigFromEnv("") if err != nil { t.Fatal("unexpected error:", err) } @@ -285,7 +285,7 @@ func TestNewConfigFromEnv(t *testing.T) { t.Run("prefix is prepended to env key", func(t *testing.T) { t.Setenv("WEBHOOK_TLS_MIN_VERSION", "1.2") - cfg, err := NewConfigFromEnv("WEBHOOK_") + cfg, err := DefaultConfigFromEnv("WEBHOOK_") if err != nil { t.Fatal("unexpected error:", err) } @@ -300,7 +300,7 @@ func TestNewConfigFromEnv(t *testing.T) { t.Setenv(CipherSuitesEnvKey, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384") t.Setenv(CurvePreferencesEnvKey, "X25519,P-256") - cfg, err := NewConfigFromEnv("") + cfg, err := DefaultConfigFromEnv("") if err != nil { t.Fatal("unexpected error:", err) } @@ -320,7 +320,7 @@ func TestNewConfigFromEnv(t *testing.T) { t.Run("invalid min version", func(t *testing.T) { t.Setenv(MinVersionEnvKey, "1.0") - _, err := NewConfigFromEnv("") + _, err := DefaultConfigFromEnv("") if err == nil { t.Fatal("expected error for invalid min version") } @@ -328,7 +328,7 @@ func TestNewConfigFromEnv(t *testing.T) { t.Run("invalid max version", func(t *testing.T) { t.Setenv(MaxVersionEnvKey, "bad") - _, err := NewConfigFromEnv("") + _, err := DefaultConfigFromEnv("") if err == nil { t.Fatal("expected error for invalid max version") } @@ -336,7 +336,7 @@ func TestNewConfigFromEnv(t *testing.T) { t.Run("invalid cipher suite", func(t *testing.T) { t.Setenv(CipherSuitesEnvKey, "NOT_A_REAL_CIPHER") - _, err := NewConfigFromEnv("") + _, err := DefaultConfigFromEnv("") if err == nil { t.Fatal("expected error for invalid cipher suite") } @@ -344,42 +344,9 @@ func TestNewConfigFromEnv(t *testing.T) { t.Run("invalid curve", func(t *testing.T) { t.Setenv(CurvePreferencesEnvKey, "NotACurve") - _, err := NewConfigFromEnv("") + _, err := DefaultConfigFromEnv("") if err == nil { t.Fatal("expected error for invalid curve") } }) } - -func TestConfig_TLSConfig(t *testing.T) { - t.Setenv(MinVersionEnvKey, "1.2") - t.Setenv(MaxVersionEnvKey, "1.3") - t.Setenv(CipherSuitesEnvKey, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256") - t.Setenv(CurvePreferencesEnvKey, "X25519,CurveP256") - - cfg, err := NewConfigFromEnv("") - if err != nil { - t.Fatal("unexpected error:", err) - } - - tc := cfg.TLSConfig() - - if tc.MinVersion != cryptotls.VersionTLS12 { - t.Errorf("MinVersion = %d, want %d", tc.MinVersion, cryptotls.VersionTLS12) - } - if tc.MaxVersion != cryptotls.VersionTLS13 { - t.Errorf("MaxVersion = %d, want %d", tc.MaxVersion, cryptotls.VersionTLS13) - } - if len(tc.CipherSuites) != 1 || tc.CipherSuites[0] != cryptotls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 { - t.Errorf("CipherSuites = %v, want [%d]", tc.CipherSuites, cryptotls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) - } - if len(tc.CurvePreferences) != 2 { - t.Fatalf("CurvePreferences has %d entries, want 2", len(tc.CurvePreferences)) - } - if tc.CurvePreferences[0] != cryptotls.X25519 { - t.Errorf("CurvePreferences[0] = %d, want %d", tc.CurvePreferences[0], cryptotls.X25519) - } - if tc.CurvePreferences[1] != cryptotls.CurveP256 { - t.Errorf("CurvePreferences[1] = %d, want %d", tc.CurvePreferences[1], cryptotls.CurveP256) - } -} diff --git a/webhook/env.go b/webhook/env.go index 1dd38585b6..0a9f0b8ddb 100644 --- a/webhook/env.go +++ b/webhook/env.go @@ -72,7 +72,7 @@ func SecretNameFromEnv(defaultSecretName string) string { return secret } -// Deprecated: Use knative.dev/pkg/tls.NewConfigFromEnv instead. +// Deprecated: Use knative.dev/pkg/tls.DefaultConfigFromEnv instead. // TLS configuration is now read automatically inside webhook.New via the shared tls package. func TLSMinVersionFromEnv(defaultTLSMinVersion uint16) uint16 { switch tlsMinVersion := os.Getenv(tlsMinVersionEnvKey); tlsMinVersion { diff --git a/webhook/webhook.go b/webhook/webhook.go index 8f3a3701cc..ae25d777f8 100644 --- a/webhook/webhook.go +++ b/webhook/webhook.go @@ -191,36 +191,29 @@ func New( logger := logging.FromContext(ctx) - tlsCfg, err := knativetls.NewConfigFromEnv("WEBHOOK_") + tlsCfg, err := knativetls.DefaultConfigFromEnv("WEBHOOK_") if err != nil { return nil, fmt.Errorf("reading TLS configuration from environment: %w", err) } - // Replace the TLS configuration with the one from the environment if not set. - // Default to TLS 1.3 as the minimum version when neither the caller nor the - // environment specifies one. - if opts.TLSMinVersion == 0 { - if tlsCfg.MinVersion != 0 { - opts.TLSMinVersion = tlsCfg.MinVersion - } else { - opts.TLSMinVersion = tls.VersionTLS13 - } + if opts.TLSMinVersion != 0 { + tlsCfg.MinVersion = opts.TLSMinVersion } - if opts.TLSMaxVersion == 0 && tlsCfg.MaxVersion != 0 { - opts.TLSMaxVersion = tlsCfg.MaxVersion + if opts.TLSMaxVersion != 0 { + tlsCfg.MaxVersion = opts.TLSMaxVersion } - if opts.TLSCipherSuites == nil && len(tlsCfg.CipherSuites) > 0 { - opts.TLSCipherSuites = tlsCfg.CipherSuites + if opts.TLSCipherSuites != nil { + tlsCfg.CipherSuites = opts.TLSCipherSuites } - if opts.TLSCurvePreferences == nil && len(tlsCfg.CurvePreferences) > 0 { - opts.TLSCurvePreferences = tlsCfg.CurvePreferences + if opts.TLSCurvePreferences != nil { + tlsCfg.CurvePreferences = opts.TLSCurvePreferences } - if opts.TLSMinVersion != 0 && opts.TLSMinVersion != tls.VersionTLS12 && opts.TLSMinVersion != tls.VersionTLS13 { - return nil, fmt.Errorf("unsupported TLS minimum version %d: must be TLS 1.2 or TLS 1.3", opts.TLSMinVersion) + if tlsCfg.MinVersion != tls.VersionTLS12 && tlsCfg.MinVersion != tls.VersionTLS13 { + return nil, fmt.Errorf("unsupported TLS minimum version %d: must be TLS 1.2 or TLS 1.3", tlsCfg.MinVersion) } - if opts.TLSMaxVersion != 0 && opts.TLSMinVersion > opts.TLSMaxVersion { - return nil, fmt.Errorf("TLS minimum version (%#x) is greater than maximum version (%#x)", opts.TLSMinVersion, opts.TLSMaxVersion) + if tlsCfg.MaxVersion != 0 && tlsCfg.MinVersion > tlsCfg.MaxVersion { + return nil, fmt.Errorf("TLS minimum version (%#x) is greater than maximum version (%#x)", tlsCfg.MinVersion, tlsCfg.MaxVersion) } syncCtx, cancel := context.WithCancel(context.Background()) @@ -240,42 +233,35 @@ func New( // a new secret informer from it. secretInformer := kubeinformerfactory.Get(ctx).Core().V1().Secrets() - //nolint:gosec // operator configures TLS min version (default is 1.3) - webhook.tlsConfig = &tls.Config{ - MinVersion: opts.TLSMinVersion, - MaxVersion: opts.TLSMaxVersion, - CipherSuites: opts.TLSCipherSuites, - CurvePreferences: opts.TLSCurvePreferences, - - // If we return (nil, error) the client sees - 'tls: internal error" - // If we return (nil, nil) the client sees - 'tls: no certificates configured' - // - // We'll return (nil, nil) when we don't find a certificate - GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) { - secret, err := secretInformer.Lister().Secrets(system.Namespace()).Get(opts.SecretName) - if err != nil { - logger.Errorw("failed to fetch secret", zap.Error(err)) - return nil, nil - } - webOpts := GetOptions(ctx) - sKey, sCert := getSecretDataKeyNamesOrDefault(webOpts.ServerPrivateKeyName, webOpts.ServerCertificateName) - serverKey, ok := secret.Data[sKey] - if !ok { - logger.Warn("server key missing") - return nil, nil - } - serverCert, ok := secret.Data[sCert] - if !ok { - logger.Warn("server cert missing") - return nil, nil - } - cert, err := tls.X509KeyPair(serverCert, serverKey) - if err != nil { - return nil, err - } - return &cert, nil - }, + // If we return (nil, error) the client sees - 'tls: internal error' + // If we return (nil, nil) the client sees - 'tls: no certificates configured' + // + // We'll return (nil, nil) when we don't find a certificate + tlsCfg.GetCertificate = func(*tls.ClientHelloInfo) (*tls.Certificate, error) { + secret, err := secretInformer.Lister().Secrets(system.Namespace()).Get(opts.SecretName) + if err != nil { + logger.Errorw("failed to fetch secret", zap.Error(err)) + return nil, nil + } + webOpts := GetOptions(ctx) + sKey, sCert := getSecretDataKeyNamesOrDefault(webOpts.ServerPrivateKeyName, webOpts.ServerCertificateName) + serverKey, ok := secret.Data[sKey] + if !ok { + logger.Warn("server key missing") + return nil, nil + } + serverCert, ok := secret.Data[sCert] + if !ok { + logger.Warn("server cert missing") + return nil, nil + } + cert, err := tls.X509KeyPair(serverCert, serverKey) + if err != nil { + return nil, err + } + return &cert, nil } + webhook.tlsConfig = tlsCfg } webhook.mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {