Skip to content

Commit 049b1f7

Browse files
gwossumgwossum
authored
feat: add TLSConfigManager (#27216)
Add `tlsconfig.TLSConfigManager` for managing TLS configurations, handling certificate reloads, logging certificate expiration warnings, etc. This is backport of the following master-1.x PRs to the 1.12 branch: - #27100: feat: add TLSConfigManager for managing TLS configuration (clean cherry-pick) - #27103: feat: add TLSConfigManager.DialWithDialer method (clean cherry-pick) - #27106: feat: add NewClientTLSConfigManager and NewDisabledTLSConfigManager (clean cherry-pick) - #27120: feat: add TLSConfigManager.UseTLS (clean cherry-pick) - #27150: fix: Clone *tls.Config returned by TLSConfigManager.TLSConfig (clean cherry-pick) - #27162: feat: multiple TLS configuration improvements (almost clean cherry-pick)
1 parent 82729cf commit 049b1f7

File tree

9 files changed

+2162
-139
lines changed

9 files changed

+2162
-139
lines changed

‎etc/config.sample.toml‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,12 +340,15 @@
340340
# Determines whether HTTPS is enabled.
341341
# https-enabled = false
342342

343-
# The SSL certificate to use when HTTPS is enabled.
343+
# The TLS certificate to use when HTTPS is enabled.
344344
# https-certificate = "/etc/ssl/influxdb.pem"
345345

346346
# Use a separate private key location.
347347
# https-private-key = ""
348348

349+
# Allows insecure file permissions for https-certificate and https-private-key when enabled.
350+
# https-insecure-certificate = false
351+
349352
# The JWT auth shared secret to validate requests using JSON web tokens.
350353
# shared-secret = ""
351354

@@ -534,9 +537,19 @@
534537
# database = "opentsdb"
535538
# retention-policy = ""
536539
# consistency-level = "one"
540+
541+
# Turns on TLS for this OpenTSDB instance.
537542
# tls-enabled = false
543+
544+
# TLS certificate to use when TLS is enabled.
538545
# certificate= "/etc/ssl/influxdb.pem"
539546

547+
# TLS private key when TLS is enabled. If blank, defaults to assuming key is in certificate.
548+
# private-key = ""
549+
550+
# Allows insecure file permissions on certificate and private-key when enabled.
551+
# insecure-certificate = false
552+
540553
# Log an error for every malformed point.
541554
# log-point-errors = true
542555

‎pkg/tlsconfig/certconfig.go‎

Lines changed: 75 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,35 @@ func (lc *LoadedCertificate) IsValid() bool {
5959
return lc.valid
6060
}
6161

62+
// loadCertificateConfig is an internal config for LoadCertificate.
63+
type loadCertificateConfig struct {
64+
// ignoreFilePermissions indicates if file permissions should be ignored during load.
65+
ignoreFilePermissions bool
66+
}
67+
68+
// LoadCertificateOpt are functions to change the behavior of LoadCertificate.
69+
type LoadCertificateOpt func(*loadCertificateConfig)
70+
71+
// WithLoadCertificateIgnoreFilePermissions instructs LoadCertificate to ignore file permissions
72+
// if ignore is true.
73+
func WithLoadCertificateIgnoreFilePermissions(ignore bool) LoadCertificateOpt {
74+
return func(c *loadCertificateConfig) {
75+
c.ignoreFilePermissions = ignore
76+
}
77+
}
78+
6279
// LoadCertificate loads a key pair from certPath and keyPath, performing several checks
6380
// along the way. If any checks fail or an error occurs loading the files, then an error is returned.
6481
// If keyPath is empty, then certPath is assumed to contain both the certificate and the private key.
6582
// Only trusted input (standard configuration files) should be used for certPath and keyPath.
66-
func LoadCertificate(certPath, keyPath string) (LoadedCertificate, error) {
83+
func LoadCertificate(certPath, keyPath string, opts ...LoadCertificateOpt) (LoadedCertificate, error) {
6784
fail := func(err error) (LoadedCertificate, error) { return LoadedCertificate{valid: false}, err }
6885

86+
config := loadCertificateConfig{}
87+
for _, o := range opts {
88+
o(&config)
89+
}
90+
6991
if certPath == "" {
7092
return fail(fmt.Errorf("LoadCertificate: certificate: %w", ErrPathEmpty))
7193
}
@@ -95,9 +117,11 @@ func LoadCertificate(certPath, keyPath string) (LoadedCertificate, error) {
95117
}
96118
}()
97119

98-
if err := file.VerifyFilePermissivenessF(f, maxPerms); err != nil {
99-
// VerifyFilePermissivenessF includes a lot context in its errors. No need to add duplicate here.
100-
return nil, fmt.Errorf("LoadCertificate: %w", err)
120+
if !config.ignoreFilePermissions {
121+
if err := file.VerifyFilePermissivenessF(f, maxPerms); err != nil {
122+
// VerifyFilePermissivenessF includes a lot context in its errors. No need to add duplicate here.
123+
return nil, fmt.Errorf("LoadCertificate: %w", err)
124+
}
101125
}
102126
data, err := io.ReadAll(f)
103127
if err != nil {
@@ -120,7 +144,7 @@ func LoadCertificate(certPath, keyPath string) (LoadedCertificate, error) {
120144
// Create key pair from loaded data
121145
cert, err := tls.X509KeyPair(certData, keyData)
122146
if err != nil {
123-
return fail(fmt.Errorf("error loading x509 key pair: %w", err))
147+
return fail(fmt.Errorf("error loading x509 key pair (%q / %q): %w", certPath, keyPath, err))
124148
}
125149

126150
// Parse the first X509 certificate in cert's chain.
@@ -129,11 +153,11 @@ func LoadCertificate(certPath, keyPath string) (LoadedCertificate, error) {
129153
if err != nil {
130154
// This should be impossible to reach because `tls.X509KeyPair` will fail
131155
// if the leaf certificate can't be parsed.
132-
return fail(fmt.Errorf("error parsing leaf certificate: %w", err))
156+
return fail(fmt.Errorf("error parsing leaf certificate (%q / %q): %w", certPath, keyPath, err))
133157
}
134158
if leaf == nil {
135159
// This shouldn't happen, but we should be extra careful with TLS certs.
136-
return fail(fmt.Errorf("error parsing leaf certificate: %w", ErrCertificateNil))
160+
return fail(fmt.Errorf("error parsing leaf certificate (%q / %q): %w", certPath, keyPath, ErrCertificateNil))
137161
}
138162

139163
return LoadedCertificate{
@@ -157,12 +181,18 @@ type TLSCertLoader struct {
157181
// certificateCheckInterval determines the duration between each certificate check.
158182
certificateCheckInterval time.Duration
159183

184+
// ignoreFilePermissions is true if file permission checks should be bypassed.
185+
ignoreFilePermissions bool
186+
160187
// closeOnce is used to close closeCh exactly one time.
161188
closeOnce sync.Once
162189

163190
// closeCh is used to trigger closing the monitor.
164191
closeCh chan struct{}
165192

193+
// monitorStartWg can be used to detect if the monitor goroutine has started.
194+
monitorStartWg sync.WaitGroup
195+
166196
// mu protects all members below.
167197
mu sync.Mutex
168198

@@ -181,28 +211,35 @@ type TLSCertLoader struct {
181211

182212
type TLSCertLoaderOpt func(*TLSCertLoader)
183213

184-
// WithExpirationAdvanced sets the how far ahead a CertLoader will
214+
// WithCertLoaderExpirationAdvanced sets the how far ahead a CertLoader will
185215
// warn about a certificate that is about to expire.
186-
func WithExpirationAdvanced(d time.Duration) TLSCertLoaderOpt {
216+
func WithCertLoaderExpirationAdvanced(d time.Duration) TLSCertLoaderOpt {
187217
return func(cl *TLSCertLoader) {
188218
cl.expirationAdvanced = d
189219
}
190220
}
191221

192-
// WithCertificateCheckInterval sets how often to check for certificate expiration.
193-
func WithCertificateCheckInterval(d time.Duration) TLSCertLoaderOpt {
222+
// WithCertLoaderCertificateCheckInterval sets how often to check for certificate expiration.
223+
func WithCertLoaderCertificateCheckInterval(d time.Duration) TLSCertLoaderOpt {
194224
return func(cl *TLSCertLoader) {
195225
cl.certificateCheckInterval = d
196226
}
197227
}
198228

199-
// WithLogger assigns a logger for to use.
200-
func WithLogger(logger *zap.Logger) TLSCertLoaderOpt {
229+
// WithCertLoaderLogger assigns a logger for to use.
230+
func WithCertLoaderLogger(logger *zap.Logger) TLSCertLoaderOpt {
201231
return func(cl *TLSCertLoader) {
202232
cl.logger = logger
203233
}
204234
}
205235

236+
// WithCertLoaderIgnoreFilePermissions skips file permission checking when loading certificates.
237+
func WithCertLoaderIgnoreFilePermissions(ignore bool) TLSCertLoaderOpt {
238+
return func(cl *TLSCertLoader) {
239+
cl.ignoreFilePermissions = ignore
240+
}
241+
}
242+
206243
// NewTLSCertLoader creates a TLSCertLoader loaded with the certifcate found in certPath and keyPath.
207244
// Only trusted input (standard configuration files) should be used for certPath and keyPath.
208245
// If the certificate can not be loaded, an error is returned. On success, a monitor is setup to
@@ -230,10 +267,8 @@ func NewTLSCertLoader(certPath, keyPath string, opts ...TLSCertLoaderOpt) (rCert
230267
}
231268

232269
// Start monitoring certificate.
233-
var monitorWg sync.WaitGroup
234-
monitorWg.Add(1)
235-
go cl.monitorCert(&monitorWg)
236-
monitorWg.Wait()
270+
cl.monitorStartWg.Add(1)
271+
go cl.monitorCert(&cl.monitorStartWg)
237272

238273
return cl, nil
239274
}
@@ -308,16 +343,22 @@ func (cl *TLSCertLoader) GetCertificate(*tls.ClientHelloInfo) (*tls.Certificate,
308343
// certificate.
309344
func (cl *TLSCertLoader) GetClientCertificate(cri *tls.CertificateRequestInfo) (*tls.Certificate, error) {
310345
if cri == nil {
311-
return new(tls.Certificate), ErrCertificateRequestInfoNil
346+
return new(tls.Certificate), fmt.Errorf("tls client: %w", ErrCertificateRequestInfoNil)
312347
}
313348
cert := cl.Certificate()
314349
if cert == nil {
315-
return new(tls.Certificate), ErrCertificateNil
350+
return new(tls.Certificate), fmt.Errorf("tls client: %w", ErrCertificateNil)
316351
}
317-
if err := cri.SupportsCertificate(cert); err != nil {
318-
return new(tls.Certificate), err
352+
353+
// Will our certificate be accepted by server?
354+
if err := cri.SupportsCertificate(cert); err == nil {
355+
return cert, nil
319356
}
320-
return cert, nil
357+
358+
// We don't have a certificate that would be accepted by the server. Don't return an error.
359+
// This replicates Go's behavior when tls.Config.Certificates is used instead of GetClientCertificate
360+
// and gives a better error on both the client and server side.
361+
return new(tls.Certificate), nil
321362
}
322363

323364
// Leaf returns the parsed x509 certificate of the currently loaded certificate.
@@ -328,13 +369,17 @@ func (cl *TLSCertLoader) Leaf() *x509.Certificate {
328369
return cl.leaf
329370
}
330371

372+
func (cl *TLSCertLoader) loadCertificate(certPath, keyPath string) (LoadedCertificate, error) {
373+
return LoadCertificate(certPath, keyPath, WithLoadCertificateIgnoreFilePermissions(cl.ignoreFilePermissions))
374+
}
375+
331376
// Load loads the certificate at the given certificate path and private keyfile path.
332377
// Only trusted input (standard configuration files) should be used for certPath and keyPath.
333378
func (cl *TLSCertLoader) Load(certPath, keyPath string) (rErr error) {
334379
log, logEnd := logger.NewOperation(cl.logger, "Loading TLS certificate", "tls_load_cert", zap.String("cert", certPath), zap.String("key", keyPath))
335380
defer logEnd()
336381

337-
loadedCert, err := LoadCertificate(certPath, keyPath)
382+
loadedCert, err := cl.loadCertificate(certPath, keyPath)
338383

339384
cl.mu.Lock()
340385
defer cl.mu.Unlock()
@@ -365,7 +410,7 @@ func (cl *TLSCertLoader) Load(certPath, keyPath string) (rErr error) {
365410
// If the certificate can be loaded, a function that will apply the certificate reload is
366411
// returned. Otherwise, an error is returned.
367412
func (cl *TLSCertLoader) PrepareLoad(certPath, keyPath string) (func() error, error) {
368-
loadedCert, err := LoadCertificate(certPath, keyPath)
413+
loadedCert, err := cl.loadCertificate(certPath, keyPath)
369414
if err != nil {
370415
return nil, err
371416
}
@@ -457,6 +502,12 @@ func (cl *TLSCertLoader) checkCurrentCert() {
457502
}
458503
}
459504

505+
// WaitForMonitorStart will wait for the certificate monitor goroutine to start. This is mainly useful
506+
// for tests to avoid race conditions.
507+
func (cl *TLSCertLoader) WaitForMonitorStart() {
508+
cl.monitorStartWg.Wait()
509+
}
510+
460511
// monitorCert periodically logs errors with the currently loaded certificate.
461512
func (cl *TLSCertLoader) monitorCert(wg *sync.WaitGroup) {
462513
cl.logger.Info("Starting TLS certificate monitor")

‎pkg/tlsconfig/certconfig_test.go‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,14 @@ func TestTLSCertLoader_HappyPath(t *testing.T) {
2525
logger := zap.New(core)
2626

2727
// Start cert loader
28-
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger))
28+
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithCertLoaderLogger(logger))
2929
require.NoError(t, err)
3030
require.NotNil(t, cl)
3131
defer func() {
3232
require.NoError(t, cl.Close())
3333
}()
34+
cl.WaitForMonitorStart()
35+
cl.WaitForMonitorStart() // should be able to safely call multiple times
3436

3537
{
3638
// Check for expected log output
@@ -108,12 +110,13 @@ func TestTLSCertLoader_GoodCertPersists(t *testing.T) {
108110
logger := zap.New(core)
109111

110112
// Start cert loader
111-
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger))
113+
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithCertLoaderLogger(logger))
112114
require.NoError(t, err)
113115
require.NotNil(t, cl)
114116
defer func() {
115117
require.NoError(t, cl.Close())
116118
}()
119+
cl.WaitForMonitorStart()
117120

118121
var goodSerial big.Int
119122
{
@@ -144,7 +147,7 @@ func TestTLSCertLoader_GoodCertPersists(t *testing.T) {
144147
require.NoError(t, emptyFile.Close())
145148

146149
loadErr := cl.Load(emptyPath, emptyPath)
147-
require.ErrorContains(t, loadErr, "error loading x509 key pair: tls: failed to find any PEM data in certificate input")
150+
require.ErrorContains(t, loadErr, fmt.Sprintf("error loading x509 key pair (%q / %q): tls: failed to find any PEM data in certificate input", emptyPath, emptyPath))
148151

149152
// Check that we are still using the previously loaded certificate
150153
cp, kp := cl.Paths()
@@ -281,12 +284,13 @@ func TestTLSCertLoader_PrematureCertificateLogging(t *testing.T) {
281284
core, logs := observer.New(zapcore.InfoLevel)
282285
logger := zap.New(core)
283286

284-
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger), WithCertificateCheckInterval(testCheckTime))
287+
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithCertLoaderLogger(logger), WithCertLoaderCertificateCheckInterval(testCheckTime))
285288
require.NoError(t, err)
286289
require.NotNil(t, cl)
287290
defer func() {
288291
require.NoError(t, cl.Close())
289292
}()
293+
cl.WaitForMonitorStart()
290294

291295
checkWarning := func(t *testing.T) {
292296
warning := logs.FilterMessage("Certificate is not valid yet").TakeAll()
@@ -313,12 +317,13 @@ func TestTLSCertLoader_ExpiredCertificateLogging(t *testing.T) {
313317
core, logs := observer.New(zapcore.InfoLevel)
314318
logger := zap.New(core)
315319

316-
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger), WithCertificateCheckInterval(testCheckTime))
320+
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithCertLoaderLogger(logger), WithCertLoaderCertificateCheckInterval(testCheckTime))
317321
require.NoError(t, err)
318322
require.NotNil(t, cl)
319323
defer func() {
320324
require.NoError(t, cl.Close())
321325
}()
326+
cl.WaitForMonitorStart()
322327

323328
checkWarning := func(t *testing.T) {
324329
warning := logs.FilterMessage("Certificate is expired").TakeAll()
@@ -346,12 +351,16 @@ func TestTLSCertLoader_CertificateExpiresSoonLogging(t *testing.T) {
346351
core, logs := observer.New(zapcore.InfoLevel)
347352
logger := zap.New(core)
348353

349-
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger), WithCertificateCheckInterval(testCheckTime), WithExpirationAdvanced(2*24*time.Hour))
354+
cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath,
355+
WithCertLoaderLogger(logger),
356+
WithCertLoaderCertificateCheckInterval(testCheckTime),
357+
WithCertLoaderExpirationAdvanced(2*24*time.Hour))
350358
require.NoError(t, err)
351359
require.NotNil(t, cl)
352360
defer func() {
353361
require.NoError(t, cl.Close())
354362
}()
363+
cl.WaitForMonitorStart()
355364

356365
checkWarning := func(t *testing.T) {
357366
warning := logs.FilterMessage("Certificate will expire soon").TakeAll()
@@ -534,8 +543,10 @@ func TestTLSCertLoader_GetClientCertificate(t *testing.T) {
534543
},
535544
}
536545

546+
// We should get an empty certificate with no error. This replicates Go's behavior when
547+
// tls.Config.Certificates is used and none of the certificates are accepted by the server.
537548
cert, err := cl.GetClientCertificate(cri)
538-
require.ErrorContains(t, err, "doesn't support any of the certificate's signature algorithms")
549+
require.NoError(t, err)
539550
// GetClientCertificate must return a non-nil certificate even on error
540551
// (per the tls.Config.GetClientCertificate contract).
541552
require.NotNil(t, cert)
@@ -591,8 +602,10 @@ func TestTLSCertLoader_GetClientCertificate(t *testing.T) {
591602
AcceptableCAs: [][]byte{parsedCA2.RawSubject},
592603
}
593604

605+
// This should return an empty certificate with no error to replicate
606+
// Go's behavior when tls.Config.Certificates is used.
594607
cert, err := cl.GetClientCertificate(cri)
595-
require.ErrorContains(t, err, "not signed by an acceptable CA")
608+
require.NoError(t, err)
596609
require.NotNil(t, cert)
597610
require.Empty(t, cert.Certificate)
598611
})

0 commit comments

Comments
 (0)