Skip to content

Commit a902bbf

Browse files
linkvtlinkvt
authored
Replace NewConfigFromEnv with DefaultConfigFromEnv (#3328)
DefaultConfigFromEnv replaces NewConfigFromEnv by returning a full default tls.Config with overrides from env vars. This avoids specifying e.g. the TLS MinVersion explicitely.
1 parent 1f39e94 commit a902bbf

4 files changed

Lines changed: 65 additions & 132 deletions

File tree

‎tls/config.go‎

Lines changed: 7 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,22 +33,14 @@ const (
3333
CurvePreferencesEnvKey = "TLS_CURVE_PREFERENCES"
3434
)
3535

36-
// Config holds parsed TLS configuration values that can be used
37-
// to build a *crypto/tls.Config.
38-
type Config struct {
39-
MinVersion uint16
40-
MaxVersion uint16
41-
CipherSuites []uint16
42-
CurvePreferences []cryptotls.CurveID
43-
}
44-
45-
// NewConfigFromEnv reads TLS configuration from environment variables and
46-
// returns a Config. The prefix is prepended to each standard env-var suffix;
36+
// DefaultConfigFromEnv returns a tls.Config with secure defaults.
37+
// The prefix is prepended to each standard env-var suffix;
4738
// for example with prefix "WEBHOOK_" the function reads
4839
// WEBHOOK_TLS_MIN_VERSION, WEBHOOK_TLS_MAX_VERSION, etc.
49-
// Fields whose corresponding env var is unset are left at their zero value.
50-
func NewConfigFromEnv(prefix string) (*Config, error) {
51-
var cfg Config
40+
func DefaultConfigFromEnv(prefix string) (*cryptotls.Config, error) {
41+
cfg := &cryptotls.Config{
42+
MinVersion: cryptotls.VersionTLS13,
43+
}
5244

5345
if v := os.Getenv(prefix + MinVersionEnvKey); v != "" {
5446
ver, err := parseVersion(v)
@@ -82,19 +74,7 @@ func NewConfigFromEnv(prefix string) (*Config, error) {
8274
cfg.CurvePreferences = curves
8375
}
8476

85-
return &cfg, nil
86-
}
87-
88-
// TLSConfig constructs a *crypto/tls.Config from the parsed configuration.
89-
// The caller typically adds additional fields such as GetCertificate.
90-
func (c *Config) TLSConfig() *cryptotls.Config {
91-
//nolint:gosec // Min version is caller-configurable; default is TLS 1.3.
92-
return &cryptotls.Config{
93-
MinVersion: c.MinVersion,
94-
MaxVersion: c.MaxVersion,
95-
CipherSuites: c.CipherSuites,
96-
CurvePreferences: c.CurvePreferences,
97-
}
77+
return cfg, nil
9878
}
9979

10080
// parseVersion converts a TLS version string to the corresponding

‎tls/config_test.go‎

Lines changed: 16 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -213,14 +213,14 @@ func Test_parseCurvePreferences(t *testing.T) {
213213
}
214214
}
215215

216-
func TestNewConfigFromEnv(t *testing.T) {
217-
t.Run("no env vars set returns zero value", func(t *testing.T) {
218-
cfg, err := NewConfigFromEnv("")
216+
func TestDefaultConfigFromEnv(t *testing.T) {
217+
t.Run("no env vars returns TLS 1.3 default", func(t *testing.T) {
218+
cfg, err := DefaultConfigFromEnv("")
219219
if err != nil {
220220
t.Fatal("unexpected error:", err)
221221
}
222-
if cfg.MinVersion != 0 {
223-
t.Errorf("MinVersion = %d, want 0", cfg.MinVersion)
222+
if cfg.MinVersion != cryptotls.VersionTLS13 {
223+
t.Errorf("MinVersion = %d, want %d", cfg.MinVersion, cryptotls.VersionTLS13)
224224
}
225225
if cfg.MaxVersion != 0 {
226226
t.Errorf("MaxVersion = %d, want 0", cfg.MaxVersion)
@@ -233,9 +233,9 @@ func TestNewConfigFromEnv(t *testing.T) {
233233
}
234234
})
235235

236-
t.Run("min version from env", func(t *testing.T) {
236+
t.Run("min version from env overrides default", func(t *testing.T) {
237237
t.Setenv(MinVersionEnvKey, "1.2")
238-
cfg, err := NewConfigFromEnv("")
238+
cfg, err := DefaultConfigFromEnv("")
239239
if err != nil {
240240
t.Fatal("unexpected error:", err)
241241
}
@@ -246,7 +246,7 @@ func TestNewConfigFromEnv(t *testing.T) {
246246

247247
t.Run("max version from env", func(t *testing.T) {
248248
t.Setenv(MaxVersionEnvKey, "1.3")
249-
cfg, err := NewConfigFromEnv("")
249+
cfg, err := DefaultConfigFromEnv("")
250250
if err != nil {
251251
t.Fatal("unexpected error:", err)
252252
}
@@ -257,7 +257,7 @@ func TestNewConfigFromEnv(t *testing.T) {
257257

258258
t.Run("cipher suites from env", func(t *testing.T) {
259259
t.Setenv(CipherSuitesEnvKey, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256")
260-
cfg, err := NewConfigFromEnv("")
260+
cfg, err := DefaultConfigFromEnv("")
261261
if err != nil {
262262
t.Fatal("unexpected error:", err)
263263
}
@@ -268,7 +268,7 @@ func TestNewConfigFromEnv(t *testing.T) {
268268

269269
t.Run("curve preferences from env", func(t *testing.T) {
270270
t.Setenv(CurvePreferencesEnvKey, "X25519,CurveP256")
271-
cfg, err := NewConfigFromEnv("")
271+
cfg, err := DefaultConfigFromEnv("")
272272
if err != nil {
273273
t.Fatal("unexpected error:", err)
274274
}
@@ -285,7 +285,7 @@ func TestNewConfigFromEnv(t *testing.T) {
285285

286286
t.Run("prefix is prepended to env key", func(t *testing.T) {
287287
t.Setenv("WEBHOOK_TLS_MIN_VERSION", "1.2")
288-
cfg, err := NewConfigFromEnv("WEBHOOK_")
288+
cfg, err := DefaultConfigFromEnv("WEBHOOK_")
289289
if err != nil {
290290
t.Fatal("unexpected error:", err)
291291
}
@@ -300,7 +300,7 @@ func TestNewConfigFromEnv(t *testing.T) {
300300
t.Setenv(CipherSuitesEnvKey, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384")
301301
t.Setenv(CurvePreferencesEnvKey, "X25519,P-256")
302302

303-
cfg, err := NewConfigFromEnv("")
303+
cfg, err := DefaultConfigFromEnv("")
304304
if err != nil {
305305
t.Fatal("unexpected error:", err)
306306
}
@@ -320,66 +320,33 @@ func TestNewConfigFromEnv(t *testing.T) {
320320

321321
t.Run("invalid min version", func(t *testing.T) {
322322
t.Setenv(MinVersionEnvKey, "1.0")
323-
_, err := NewConfigFromEnv("")
323+
_, err := DefaultConfigFromEnv("")
324324
if err == nil {
325325
t.Fatal("expected error for invalid min version")
326326
}
327327
})
328328

329329
t.Run("invalid max version", func(t *testing.T) {
330330
t.Setenv(MaxVersionEnvKey, "bad")
331-
_, err := NewConfigFromEnv("")
331+
_, err := DefaultConfigFromEnv("")
332332
if err == nil {
333333
t.Fatal("expected error for invalid max version")
334334
}
335335
})
336336

337337
t.Run("invalid cipher suite", func(t *testing.T) {
338338
t.Setenv(CipherSuitesEnvKey, "NOT_A_REAL_CIPHER")
339-
_, err := NewConfigFromEnv("")
339+
_, err := DefaultConfigFromEnv("")
340340
if err == nil {
341341
t.Fatal("expected error for invalid cipher suite")
342342
}
343343
})
344344

345345
t.Run("invalid curve", func(t *testing.T) {
346346
t.Setenv(CurvePreferencesEnvKey, "NotACurve")
347-
_, err := NewConfigFromEnv("")
347+
_, err := DefaultConfigFromEnv("")
348348
if err == nil {
349349
t.Fatal("expected error for invalid curve")
350350
}
351351
})
352352
}
353-
354-
func TestConfig_TLSConfig(t *testing.T) {
355-
t.Setenv(MinVersionEnvKey, "1.2")
356-
t.Setenv(MaxVersionEnvKey, "1.3")
357-
t.Setenv(CipherSuitesEnvKey, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256")
358-
t.Setenv(CurvePreferencesEnvKey, "X25519,CurveP256")
359-
360-
cfg, err := NewConfigFromEnv("")
361-
if err != nil {
362-
t.Fatal("unexpected error:", err)
363-
}
364-
365-
tc := cfg.TLSConfig()
366-
367-
if tc.MinVersion != cryptotls.VersionTLS12 {
368-
t.Errorf("MinVersion = %d, want %d", tc.MinVersion, cryptotls.VersionTLS12)
369-
}
370-
if tc.MaxVersion != cryptotls.VersionTLS13 {
371-
t.Errorf("MaxVersion = %d, want %d", tc.MaxVersion, cryptotls.VersionTLS13)
372-
}
373-
if len(tc.CipherSuites) != 1 || tc.CipherSuites[0] != cryptotls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 {
374-
t.Errorf("CipherSuites = %v, want [%d]", tc.CipherSuites, cryptotls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256)
375-
}
376-
if len(tc.CurvePreferences) != 2 {
377-
t.Fatalf("CurvePreferences has %d entries, want 2", len(tc.CurvePreferences))
378-
}
379-
if tc.CurvePreferences[0] != cryptotls.X25519 {
380-
t.Errorf("CurvePreferences[0] = %d, want %d", tc.CurvePreferences[0], cryptotls.X25519)
381-
}
382-
if tc.CurvePreferences[1] != cryptotls.CurveP256 {
383-
t.Errorf("CurvePreferences[1] = %d, want %d", tc.CurvePreferences[1], cryptotls.CurveP256)
384-
}
385-
}

‎webhook/env.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func SecretNameFromEnv(defaultSecretName string) string {
7272
return secret
7373
}
7474

75-
// Deprecated: Use knative.dev/pkg/tls.NewConfigFromEnv instead.
75+
// Deprecated: Use knative.dev/pkg/tls.DefaultConfigFromEnv instead.
7676
// TLS configuration is now read automatically inside webhook.New via the shared tls package.
7777
func TLSMinVersionFromEnv(defaultTLSMinVersion uint16) uint16 {
7878
switch tlsMinVersion := os.Getenv(tlsMinVersionEnvKey); tlsMinVersion {

‎webhook/webhook.go‎

Lines changed: 41 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -191,36 +191,29 @@ func New(
191191

192192
logger := logging.FromContext(ctx)
193193

194-
tlsCfg, err := knativetls.NewConfigFromEnv("WEBHOOK_")
194+
tlsCfg, err := knativetls.DefaultConfigFromEnv("WEBHOOK_")
195195
if err != nil {
196196
return nil, fmt.Errorf("reading TLS configuration from environment: %w", err)
197197
}
198198

199-
// Replace the TLS configuration with the one from the environment if not set.
200-
// Default to TLS 1.3 as the minimum version when neither the caller nor the
201-
// environment specifies one.
202-
if opts.TLSMinVersion == 0 {
203-
if tlsCfg.MinVersion != 0 {
204-
opts.TLSMinVersion = tlsCfg.MinVersion
205-
} else {
206-
opts.TLSMinVersion = tls.VersionTLS13
207-
}
199+
if opts.TLSMinVersion != 0 {
200+
tlsCfg.MinVersion = opts.TLSMinVersion
208201
}
209-
if opts.TLSMaxVersion == 0 && tlsCfg.MaxVersion != 0 {
210-
opts.TLSMaxVersion = tlsCfg.MaxVersion
202+
if opts.TLSMaxVersion != 0 {
203+
tlsCfg.MaxVersion = opts.TLSMaxVersion
211204
}
212-
if opts.TLSCipherSuites == nil && len(tlsCfg.CipherSuites) > 0 {
213-
opts.TLSCipherSuites = tlsCfg.CipherSuites
205+
if opts.TLSCipherSuites != nil {
206+
tlsCfg.CipherSuites = opts.TLSCipherSuites
214207
}
215-
if opts.TLSCurvePreferences == nil && len(tlsCfg.CurvePreferences) > 0 {
216-
opts.TLSCurvePreferences = tlsCfg.CurvePreferences
208+
if opts.TLSCurvePreferences != nil {
209+
tlsCfg.CurvePreferences = opts.TLSCurvePreferences
217210
}
218211

219-
if opts.TLSMinVersion != 0 && opts.TLSMinVersion != tls.VersionTLS12 && opts.TLSMinVersion != tls.VersionTLS13 {
220-
return nil, fmt.Errorf("unsupported TLS minimum version %d: must be TLS 1.2 or TLS 1.3", opts.TLSMinVersion)
212+
if tlsCfg.MinVersion != tls.VersionTLS12 && tlsCfg.MinVersion != tls.VersionTLS13 {
213+
return nil, fmt.Errorf("unsupported TLS minimum version %d: must be TLS 1.2 or TLS 1.3", tlsCfg.MinVersion)
221214
}
222-
if opts.TLSMaxVersion != 0 && opts.TLSMinVersion > opts.TLSMaxVersion {
223-
return nil, fmt.Errorf("TLS minimum version (%#x) is greater than maximum version (%#x)", opts.TLSMinVersion, opts.TLSMaxVersion)
215+
if tlsCfg.MaxVersion != 0 && tlsCfg.MinVersion > tlsCfg.MaxVersion {
216+
return nil, fmt.Errorf("TLS minimum version (%#x) is greater than maximum version (%#x)", tlsCfg.MinVersion, tlsCfg.MaxVersion)
224217
}
225218

226219
syncCtx, cancel := context.WithCancel(context.Background())
@@ -240,42 +233,35 @@ func New(
240233
// a new secret informer from it.
241234
secretInformer := kubeinformerfactory.Get(ctx).Core().V1().Secrets()
242235

243-
//nolint:gosec // operator configures TLS min version (default is 1.3)
244-
webhook.tlsConfig = &tls.Config{
245-
MinVersion: opts.TLSMinVersion,
246-
MaxVersion: opts.TLSMaxVersion,
247-
CipherSuites: opts.TLSCipherSuites,
248-
CurvePreferences: opts.TLSCurvePreferences,
249-
250-
// If we return (nil, error) the client sees - 'tls: internal error"
251-
// If we return (nil, nil) the client sees - 'tls: no certificates configured'
252-
//
253-
// We'll return (nil, nil) when we don't find a certificate
254-
GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
255-
secret, err := secretInformer.Lister().Secrets(system.Namespace()).Get(opts.SecretName)
256-
if err != nil {
257-
logger.Errorw("failed to fetch secret", zap.Error(err))
258-
return nil, nil
259-
}
260-
webOpts := GetOptions(ctx)
261-
sKey, sCert := getSecretDataKeyNamesOrDefault(webOpts.ServerPrivateKeyName, webOpts.ServerCertificateName)
262-
serverKey, ok := secret.Data[sKey]
263-
if !ok {
264-
logger.Warn("server key missing")
265-
return nil, nil
266-
}
267-
serverCert, ok := secret.Data[sCert]
268-
if !ok {
269-
logger.Warn("server cert missing")
270-
return nil, nil
271-
}
272-
cert, err := tls.X509KeyPair(serverCert, serverKey)
273-
if err != nil {
274-
return nil, err
275-
}
276-
return &cert, nil
277-
},
236+
// If we return (nil, error) the client sees - 'tls: internal error'
237+
// If we return (nil, nil) the client sees - 'tls: no certificates configured'
238+
//
239+
// We'll return (nil, nil) when we don't find a certificate
240+
tlsCfg.GetCertificate = func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
241+
secret, err := secretInformer.Lister().Secrets(system.Namespace()).Get(opts.SecretName)
242+
if err != nil {
243+
logger.Errorw("failed to fetch secret", zap.Error(err))
244+
return nil, nil
245+
}
246+
webOpts := GetOptions(ctx)
247+
sKey, sCert := getSecretDataKeyNamesOrDefault(webOpts.ServerPrivateKeyName, webOpts.ServerCertificateName)
248+
serverKey, ok := secret.Data[sKey]
249+
if !ok {
250+
logger.Warn("server key missing")
251+
return nil, nil
252+
}
253+
serverCert, ok := secret.Data[sCert]
254+
if !ok {
255+
logger.Warn("server cert missing")
256+
return nil, nil
257+
}
258+
cert, err := tls.X509KeyPair(serverCert, serverKey)
259+
if err != nil {
260+
return nil, err
261+
}
262+
return &cert, nil
278263
}
264+
webhook.tlsConfig = tlsCfg
279265
}
280266

281267
webhook.mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {

0 commit comments

Comments
 (0)