diff --git a/etc/config.sample.toml b/etc/config.sample.toml index aa51a28124a..83f6b49de2d 100644 --- a/etc/config.sample.toml +++ b/etc/config.sample.toml @@ -340,12 +340,15 @@ # Determines whether HTTPS is enabled. # https-enabled = false - # The SSL certificate to use when HTTPS is enabled. + # The TLS certificate to use when HTTPS is enabled. # https-certificate = "/etc/ssl/influxdb.pem" # Use a separate private key location. # https-private-key = "" + # Allows insecure file permissions for https-certificate and https-private-key when enabled. + # https-insecure-certificate = false + # The JWT auth shared secret to validate requests using JSON web tokens. # shared-secret = "" @@ -534,9 +537,19 @@ # database = "opentsdb" # retention-policy = "" # consistency-level = "one" + + # Turns on TLS for this OpenTSDB instance. # tls-enabled = false + + # TLS certificate to use when TLS is enabled. # certificate= "/etc/ssl/influxdb.pem" + # TLS private key when TLS is enabled. If blank, defaults to assuming key is in certificate. + # private-key = "" + + # Allows insecure file permissions on certificate and private-key when enabled. + # insecure-certificate = false + # Log an error for every malformed point. # log-point-errors = true diff --git a/pkg/tlsconfig/certconfig.go b/pkg/tlsconfig/certconfig.go index 268c339ef04..092bd249925 100644 --- a/pkg/tlsconfig/certconfig.go +++ b/pkg/tlsconfig/certconfig.go @@ -59,13 +59,35 @@ func (lc *LoadedCertificate) IsValid() bool { return lc.valid } +// loadCertificateConfig is an internal config for LoadCertificate. +type loadCertificateConfig struct { + // ignoreFilePermissions indicates if file permissions should be ignored during load. + ignoreFilePermissions bool +} + +// LoadCertificateOpt are functions to change the behavior of LoadCertificate. +type LoadCertificateOpt func(*loadCertificateConfig) + +// WithLoadCertificateIgnoreFilePermissions instructs LoadCertificate to ignore file permissions +// if ignore is true. +func WithLoadCertificateIgnoreFilePermissions(ignore bool) LoadCertificateOpt { + return func(c *loadCertificateConfig) { + c.ignoreFilePermissions = ignore + } +} + // LoadCertificate loads a key pair from certPath and keyPath, performing several checks // along the way. If any checks fail or an error occurs loading the files, then an error is returned. // If keyPath is empty, then certPath is assumed to contain both the certificate and the private key. // Only trusted input (standard configuration files) should be used for certPath and keyPath. -func LoadCertificate(certPath, keyPath string) (LoadedCertificate, error) { +func LoadCertificate(certPath, keyPath string, opts ...LoadCertificateOpt) (LoadedCertificate, error) { fail := func(err error) (LoadedCertificate, error) { return LoadedCertificate{valid: false}, err } + config := loadCertificateConfig{} + for _, o := range opts { + o(&config) + } + if certPath == "" { return fail(fmt.Errorf("LoadCertificate: certificate: %w", ErrPathEmpty)) } @@ -95,9 +117,11 @@ func LoadCertificate(certPath, keyPath string) (LoadedCertificate, error) { } }() - if err := file.VerifyFilePermissivenessF(f, maxPerms); err != nil { - // VerifyFilePermissivenessF includes a lot context in its errors. No need to add duplicate here. - return nil, fmt.Errorf("LoadCertificate: %w", err) + if !config.ignoreFilePermissions { + if err := file.VerifyFilePermissivenessF(f, maxPerms); err != nil { + // VerifyFilePermissivenessF includes a lot context in its errors. No need to add duplicate here. + return nil, fmt.Errorf("LoadCertificate: %w", err) + } } data, err := io.ReadAll(f) if err != nil { @@ -157,12 +181,18 @@ type TLSCertLoader struct { // certificateCheckInterval determines the duration between each certificate check. certificateCheckInterval time.Duration + // ignoreFilePermissions is true if file permission checks should be bypassed. + ignoreFilePermissions bool + // closeOnce is used to close closeCh exactly one time. closeOnce sync.Once // closeCh is used to trigger closing the monitor. closeCh chan struct{} + // monitorStartWg can be used to detect if the monitor goroutine has started. + monitorStartWg sync.WaitGroup + // mu protects all members below. mu sync.Mutex @@ -181,28 +211,35 @@ type TLSCertLoader struct { type TLSCertLoaderOpt func(*TLSCertLoader) -// WithExpirationAdvanced sets the how far ahead a CertLoader will +// WithCertLoaderExpirationAdvanced sets the how far ahead a CertLoader will // warn about a certificate that is about to expire. -func WithExpirationAdvanced(d time.Duration) TLSCertLoaderOpt { +func WithCertLoaderExpirationAdvanced(d time.Duration) TLSCertLoaderOpt { return func(cl *TLSCertLoader) { cl.expirationAdvanced = d } } -// WithCertificateCheckInterval sets how often to check for certificate expiration. -func WithCertificateCheckInterval(d time.Duration) TLSCertLoaderOpt { +// WithCertLoaderCertificateCheckInterval sets how often to check for certificate expiration. +func WithCertLoaderCertificateCheckInterval(d time.Duration) TLSCertLoaderOpt { return func(cl *TLSCertLoader) { cl.certificateCheckInterval = d } } -// WithLogger assigns a logger for to use. -func WithLogger(logger *zap.Logger) TLSCertLoaderOpt { +// WithCertLoaderLogger assigns a logger for to use. +func WithCertLoaderLogger(logger *zap.Logger) TLSCertLoaderOpt { return func(cl *TLSCertLoader) { cl.logger = logger } } +// WithCertLoaderIgnoreFilePermissions skips file permission checking when loading certificates. +func WithCertLoaderIgnoreFilePermissions(ignore bool) TLSCertLoaderOpt { + return func(cl *TLSCertLoader) { + cl.ignoreFilePermissions = ignore + } +} + // NewTLSCertLoader creates a TLSCertLoader loaded with the certifcate found in certPath and keyPath. // Only trusted input (standard configuration files) should be used for certPath and keyPath. // 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 } // Start monitoring certificate. - var monitorWg sync.WaitGroup - monitorWg.Add(1) - go cl.monitorCert(&monitorWg) - monitorWg.Wait() + cl.monitorStartWg.Add(1) + go cl.monitorCert(&cl.monitorStartWg) return cl, nil } @@ -308,16 +343,22 @@ func (cl *TLSCertLoader) GetCertificate(*tls.ClientHelloInfo) (*tls.Certificate, // certificate. func (cl *TLSCertLoader) GetClientCertificate(cri *tls.CertificateRequestInfo) (*tls.Certificate, error) { if cri == nil { - return new(tls.Certificate), ErrCertificateRequestInfoNil + return new(tls.Certificate), fmt.Errorf("tls client: %w", ErrCertificateRequestInfoNil) } cert := cl.Certificate() if cert == nil { - return new(tls.Certificate), ErrCertificateNil + return new(tls.Certificate), fmt.Errorf("tls client: %w", ErrCertificateNil) } - if err := cri.SupportsCertificate(cert); err != nil { - return new(tls.Certificate), err + + // Will our certificate be accepted by server? + if err := cri.SupportsCertificate(cert); err == nil { + return cert, nil } - return cert, nil + + // We don't have a certificate that would be accepted by the server. Don't return an error. + // This replicates Go's behavior when tls.Config.Certificates is used instead of GetClientCertificate + // and gives a better error on both the client and server side. + return new(tls.Certificate), nil } // Leaf returns the parsed x509 certificate of the currently loaded certificate. @@ -328,13 +369,17 @@ func (cl *TLSCertLoader) Leaf() *x509.Certificate { return cl.leaf } +func (cl *TLSCertLoader) loadCertificate(certPath, keyPath string) (LoadedCertificate, error) { + return LoadCertificate(certPath, keyPath, WithLoadCertificateIgnoreFilePermissions(cl.ignoreFilePermissions)) +} + // Load loads the certificate at the given certificate path and private keyfile path. // Only trusted input (standard configuration files) should be used for certPath and keyPath. func (cl *TLSCertLoader) Load(certPath, keyPath string) (rErr error) { log, logEnd := logger.NewOperation(cl.logger, "Loading TLS certificate", "tls_load_cert", zap.String("cert", certPath), zap.String("key", keyPath)) defer logEnd() - loadedCert, err := LoadCertificate(certPath, keyPath) + loadedCert, err := cl.loadCertificate(certPath, keyPath) cl.mu.Lock() defer cl.mu.Unlock() @@ -365,7 +410,7 @@ func (cl *TLSCertLoader) Load(certPath, keyPath string) (rErr error) { // If the certificate can be loaded, a function that will apply the certificate reload is // returned. Otherwise, an error is returned. func (cl *TLSCertLoader) PrepareLoad(certPath, keyPath string) (func() error, error) { - loadedCert, err := LoadCertificate(certPath, keyPath) + loadedCert, err := cl.loadCertificate(certPath, keyPath) if err != nil { return nil, err } @@ -457,6 +502,12 @@ func (cl *TLSCertLoader) checkCurrentCert() { } } +// WaitForMonitorStart will wait for the certificate monitor goroutine to start. This is mainly useful +// for tests to avoid race conditions. +func (cl *TLSCertLoader) WaitForMonitorStart() { + cl.monitorStartWg.Wait() +} + // monitorCert periodically logs errors with the currently loaded certificate. func (cl *TLSCertLoader) monitorCert(wg *sync.WaitGroup) { cl.logger.Info("Starting TLS certificate monitor") diff --git a/pkg/tlsconfig/certconfig_test.go b/pkg/tlsconfig/certconfig_test.go index c3597cb5dc2..b818b6ca26a 100644 --- a/pkg/tlsconfig/certconfig_test.go +++ b/pkg/tlsconfig/certconfig_test.go @@ -25,12 +25,14 @@ func TestTLSCertLoader_HappyPath(t *testing.T) { logger := zap.New(core) // Start cert loader - cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger)) + cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithCertLoaderLogger(logger)) require.NoError(t, err) require.NotNil(t, cl) defer func() { require.NoError(t, cl.Close()) }() + cl.WaitForMonitorStart() + cl.WaitForMonitorStart() // should be able to safely call multiple times { // Check for expected log output @@ -108,12 +110,13 @@ func TestTLSCertLoader_GoodCertPersists(t *testing.T) { logger := zap.New(core) // Start cert loader - cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger)) + cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithCertLoaderLogger(logger)) require.NoError(t, err) require.NotNil(t, cl) defer func() { require.NoError(t, cl.Close()) }() + cl.WaitForMonitorStart() var goodSerial big.Int { @@ -281,12 +284,13 @@ func TestTLSCertLoader_PrematureCertificateLogging(t *testing.T) { core, logs := observer.New(zapcore.InfoLevel) logger := zap.New(core) - cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger), WithCertificateCheckInterval(testCheckTime)) + cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithCertLoaderLogger(logger), WithCertLoaderCertificateCheckInterval(testCheckTime)) require.NoError(t, err) require.NotNil(t, cl) defer func() { require.NoError(t, cl.Close()) }() + cl.WaitForMonitorStart() checkWarning := func(t *testing.T) { warning := logs.FilterMessage("Certificate is not valid yet").TakeAll() @@ -313,12 +317,13 @@ func TestTLSCertLoader_ExpiredCertificateLogging(t *testing.T) { core, logs := observer.New(zapcore.InfoLevel) logger := zap.New(core) - cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger), WithCertificateCheckInterval(testCheckTime)) + cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithCertLoaderLogger(logger), WithCertLoaderCertificateCheckInterval(testCheckTime)) require.NoError(t, err) require.NotNil(t, cl) defer func() { require.NoError(t, cl.Close()) }() + cl.WaitForMonitorStart() checkWarning := func(t *testing.T) { warning := logs.FilterMessage("Certificate is expired").TakeAll() @@ -346,12 +351,16 @@ func TestTLSCertLoader_CertificateExpiresSoonLogging(t *testing.T) { core, logs := observer.New(zapcore.InfoLevel) logger := zap.New(core) - cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, WithLogger(logger), WithCertificateCheckInterval(testCheckTime), WithExpirationAdvanced(2*24*time.Hour)) + cl, err := NewTLSCertLoader(ss.CertPath, ss.KeyPath, + WithCertLoaderLogger(logger), + WithCertLoaderCertificateCheckInterval(testCheckTime), + WithCertLoaderExpirationAdvanced(2*24*time.Hour)) require.NoError(t, err) require.NotNil(t, cl) defer func() { require.NoError(t, cl.Close()) }() + cl.WaitForMonitorStart() checkWarning := func(t *testing.T) { warning := logs.FilterMessage("Certificate will expire soon").TakeAll() @@ -534,8 +543,10 @@ func TestTLSCertLoader_GetClientCertificate(t *testing.T) { }, } + // We should get an empty certificate with no error. This replicates Go's behavior when + // tls.Config.Certificates is used and none of the certificates are accepted by the server. cert, err := cl.GetClientCertificate(cri) - require.ErrorContains(t, err, "doesn't support any of the certificate's signature algorithms") + require.NoError(t, err) // GetClientCertificate must return a non-nil certificate even on error // (per the tls.Config.GetClientCertificate contract). require.NotNil(t, cert) @@ -591,8 +602,10 @@ func TestTLSCertLoader_GetClientCertificate(t *testing.T) { AcceptableCAs: [][]byte{parsedCA2.RawSubject}, } + // This should return an empty certificate with no error to replicate + // Go's behavior when tls.Config.Certificates is used. cert, err := cl.GetClientCertificate(cri) - require.ErrorContains(t, err, "not signed by an acceptable CA") + require.NoError(t, err) require.NotNil(t, cert) require.Empty(t, cert.Certificate) }) diff --git a/pkg/tlsconfig/configmanager.go b/pkg/tlsconfig/configmanager.go index 450203c49eb..b0a4d1e6565 100644 --- a/pkg/tlsconfig/configmanager.go +++ b/pkg/tlsconfig/configmanager.go @@ -2,8 +2,14 @@ package tlsconfig import ( "crypto/tls" + "crypto/x509" "errors" + "fmt" "net" + "os" + "time" + + "go.uber.org/zap" ) var ( @@ -20,19 +26,244 @@ type TLSConfigManager struct { certLoader *TLSCertLoader } -// NewTLSConfigManager returns a TLSConfigManager with the given configuration. If useTLS is true, -// then the certificate is loaded immediately if specified and the tls.Config instantiated. -// If no certPath and no keyPath is provided, then no TLSCertLoader is created. For this case, the returned -// TLSConfigManager can be used for client operations (e.g. Dial), but not for server operations (e.g. Listen). -// The allowInsecure parameter has no effect on server operations. -func NewTLSConfigManager(useTLS bool, baseConfig *tls.Config, certPath, keyPath string, allowInsecure bool, certLoaderOpts ...TLSCertLoaderOpt) (*TLSConfigManager, error) { +// caConfig contains configuration to configure a TLS CA config. +type caConfig struct { + // custom indicates if caConfig specifies a custom CA config. If false, + // the rest of the configuration is ignored. + custom bool + + // includeSystem indicates if system root CA should be included. + includeSystem bool + + // files lists paths to PEM files to include in CA. + files []string +} + +// setIncludeSystem sets includeSystem and marks the config as custom. +func (c *caConfig) setIncludeSystem(include bool) { + c.custom = true + c.includeSystem = include +} + +// addFiles adds a list of files to be included in CA configuration and marks config as custom. +func (c *caConfig) addFiles(files ...string) { + c.custom = true + c.files = append(c.files, files...) +} + +// newCertPool returns a x509.CertPool for the configuration in c. If c is not a custom config, then +// nil is returned. +func (c *caConfig) newCertPool() (*x509.CertPool, error) { + // Only create a CertPool for a custom CA config. + if !c.custom { + return nil, nil + } + + // Create new CertPool, with system CA store if requested. + var cp *x509.CertPool + if c.includeSystem { + var err error + cp, err = x509.SystemCertPool() + if err != nil { + return nil, fmt.Errorf("error getting system CA pool during newCertPool: %w", err) + } + } else { + cp = x509.NewCertPool() + } + + // Add PEM files to CA store. + for _, fn := range c.files { + pem, err := os.ReadFile(fn) + if err != nil { + return nil, fmt.Errorf("error reading file %q for CA store: %w", fn, err) + } + if ok := cp.AppendCertsFromPEM(pem); !ok { + return nil, fmt.Errorf("error adding certificates from %q to CA store: no valid certificates found", fn) + } + } + + return cp, nil +} + +// tlsConfigManagerConfig holds all options for a TLSConfigManager. +type tlsConfigManagerConfig struct { + // useTLS indicates if TLS should be used. If not, the rest of the configuration is ignored. + useTLS bool + + // baseConfig is the *tls.Config to use as the basis for the manager's *tls.Config. + baseConfig *tls.Config + + // certPath is the path to the server certificate. + certPath string + + // keyPath is the path to the server private key. + keyPath string + + // allowInsecure indicates if certificate checks should be ignored. + allowInsecure bool + + // rootCAConfig is the root CA config. + rootCAConfig caConfig + + // clientCAConfig is the CA config for servers to use for verifying client certificates. + clientCAConfig caConfig + + // clientAuth indicates the type of ClientAuth required by a server. + clientAuth tls.ClientAuthType + + // certLoaderOpts are options for the underlying TLSCertLoader. + certLoaderOpts []TLSCertLoaderOpt +} + +// addCertLoaderOpt adds a TLSCertLoaderOpt to the configuration for the TLSCertLoader. +func (cl *tlsConfigManagerConfig) addCertLoaderOpt(o TLSCertLoaderOpt) { + cl.certLoaderOpts = append(cl.certLoaderOpts, o) +} + +// TLSConfigManagerOpt is an option for use with NewTLSConfigManager and related constructors. +type TLSConfigManagerOpt func(*tlsConfigManagerConfig) + +// WithUseTLS sets if the config manager should use TLS. +func WithUseTLS(useTLS bool) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.useTLS = useTLS + } +} + +// WithBaseConfig sets the config manager's base *tls.Config. +func WithBaseConfig(baseConfig *tls.Config) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.baseConfig = baseConfig + } +} + +// WithCertificate sets the config manager's certificate and private key path. +func WithCertificate(certPath, keyPath string) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.certPath = certPath + cp.keyPath = keyPath + } +} + +// WithAllowInsecure sets if the config manager should allow insecure TLS. +func WithAllowInsecure(allowInsecure bool) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.allowInsecure = allowInsecure + } +} + +// WithRootCAIncludeSystem specifies if the system CA should be included in the root CA. +func WithRootCAIncludeSystem(includeSystem bool) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.rootCAConfig.setIncludeSystem(includeSystem) + } +} + +// WithRootCAFiles specifies a list of paths to PEM files that contain root CAs. +func WithRootCAFiles(pemFiles ...string) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.rootCAConfig.addFiles(pemFiles...) + } +} + +// WithClientCAIncludeSystem specifies if the system CA should be included in the client CA for client authentication. +func WithClientCAIncludeSystem(includeSystem bool) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.clientCAConfig.setIncludeSystem(includeSystem) + } +} + +// WithClientCAFiles specifies a list of paths to PEM files that contain root CA for client authentication. +func WithClientCAFiles(pemFiles ...string) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.clientCAConfig.addFiles(pemFiles...) + } +} + +// WithClientAuth specifies the type TLS client authentication a server should perform. +func WithClientAuth(auth tls.ClientAuthType) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.clientAuth = auth + } +} + +// WithExpirationAdvanced sets the how far ahead the underlying CertLoader will +// warn about a certificate that is about to expire. +func WithExpirationAdvanced(d time.Duration) TLSConfigManagerOpt { + return func(cp *tlsConfigManagerConfig) { + cp.addCertLoaderOpt(WithCertLoaderExpirationAdvanced(d)) + } +} + +// WithCertificateCheckInterval sets how often to check for certificate expiration. +func WithCertificateCheckInterval(d time.Duration) TLSConfigManagerOpt { + return func(cl *tlsConfigManagerConfig) { + cl.addCertLoaderOpt(WithCertLoaderCertificateCheckInterval(d)) + } +} + +// WithLogger assigns a logger for to use. +func WithLogger(logger *zap.Logger) TLSConfigManagerOpt { + return func(cl *tlsConfigManagerConfig) { + cl.addCertLoaderOpt(WithCertLoaderLogger(logger)) + } +} + +// WithIgnoreFilePermissions ignores file permissions when loading certificates. +func WithIgnoreFilePermissions(ignore bool) TLSConfigManagerOpt { + return func(cl *tlsConfigManagerConfig) { + cl.addCertLoaderOpt(WithCertLoaderIgnoreFilePermissions(ignore)) + } +} + +// newTLSConfigManager returns a TLSConfigManager configured by opts. +func newTLSConfigManager(opts ...TLSConfigManagerOpt) (*TLSConfigManager, error) { + c := tlsConfigManagerConfig{} + for _, o := range opts { + o(&c) + } + + // Create and setup base tls.Config + var tlsConfig *tls.Config var certLoader *TLSCertLoader - tlsConfig := newTLSConfig(useTLS, baseConfig, allowInsecure) - if tlsConfig != nil { + if c.useTLS { + // Create / clone TLS configuration as necessary. + tlsConfig = c.baseConfig.Clone() // nil configs are clonable. + if tlsConfig == nil { + tlsConfig = new(tls.Config) + } + + // Modify configuration. + tlsConfig.InsecureSkipVerify = c.allowInsecure + + // Only overwrite default value of ClientAuth. + if tlsConfig.ClientAuth == tls.NoClientCert { + tlsConfig.ClientAuth = c.clientAuth + } + + // Setup CA pools. + setupPool := func(pool **x509.CertPool, c caConfig) error { + if p, err := c.newCertPool(); err != nil { + return err + } else if p != nil { + // Don't overwrite an existing pool from baseConfig with a nil pool. + *pool = p + } + return nil + } + + if err := setupPool(&tlsConfig.RootCAs, c.rootCAConfig); err != nil { + return nil, fmt.Errorf("error creating root CA pool: %w", err) + } + if err := setupPool(&tlsConfig.ClientCAs, c.clientCAConfig); err != nil { + return nil, fmt.Errorf("error creating client CA pool: %w", err) + } + + // Create TLSCertLoader and configure it. // Create TLSCertLoader and configure tlsConfig to use it. No loader is created if no cert // is provided. This is useful for client-only use cases. - if certPath != "" || keyPath != "" { - if cl, err := NewTLSCertLoader(certPath, keyPath, certLoaderOpts...); err != nil { + if c.certPath != "" || c.keyPath != "" { + if cl, err := NewTLSCertLoader(c.certPath, c.keyPath, c.certLoaderOpts...); err != nil { return nil, err } else { certLoader = cl @@ -47,35 +278,35 @@ func NewTLSConfigManager(useTLS bool, baseConfig *tls.Config, certPath, keyPath }, nil } -// newTLSConfig creates a new tls.Config based on the passed parameters. If useTLS is false, then -// no tls.Config is generated. -func newTLSConfig(useTLS bool, baseConfig *tls.Config, allowInsecure bool) *tls.Config { - var tlsConfig *tls.Config - - if useTLS { - // Create / clone TLS configuration as necessary. - tlsConfig = baseConfig.Clone() // nil configs are clonable. - if tlsConfig == nil { - tlsConfig = new(tls.Config) - } - - // Modify configuration. - tlsConfig.InsecureSkipVerify = allowInsecure - } - - return tlsConfig +// NewTLSConfigManager returns a TLSConfigManager with the given configuration. If useTLS is true, +// then the certificate is loaded immediately if specified and the tls.Config instantiated. +// If no certPath and no keyPath is provided, then no TLSCertLoader is created. For this case, the returned +// TLSConfigManager can be used for client operations (e.g. Dial), but not for server operations (e.g. Listen). +// The allowInsecure parameter has no effect on server operations. +func NewTLSConfigManager(useTLS bool, baseConfig *tls.Config, certPath, keyPath string, allowInsecure bool, opts ...TLSConfigManagerOpt) (*TLSConfigManager, error) { + // Values from explicit parameters should override values set in opts. + co := make([]TLSConfigManagerOpt, 0, len(opts)+4) + co = append(co, opts...) + co = append(co, + WithUseTLS(useTLS), + WithBaseConfig(baseConfig), + WithCertificate(certPath, keyPath), + WithAllowInsecure(allowInsecure)) + return newTLSConfigManager(co...) } // NewClientTLSConfigManager creates a TLSConfigManager that is only useful for clients without // client certificates. TLS is enabled when useTLS is true. Certificate verification is skipped // when allowInsecure is true. // This is convenience wrapper for NewTLSConfigManager(useTLS, baseConfig, "", "", allowInsecure). -// In addition to being slightly more compact, NewClientTLSConfigManager can not return an error. -func NewClientTLSConfigManager(useTLS bool, baseConfig *tls.Config, allowInsecure bool) *TLSConfigManager { - return &TLSConfigManager{ - tlsConfig: newTLSConfig(useTLS, baseConfig, allowInsecure), - certLoader: nil, - } +func NewClientTLSConfigManager(useTLS bool, baseConfig *tls.Config, allowInsecure bool, opts ...TLSConfigManagerOpt) (*TLSConfigManager, error) { + co := make([]TLSConfigManagerOpt, 0, len(opts)+3) + co = append(co, opts...) + co = append(co, + WithUseTLS(useTLS), + WithBaseConfig(baseConfig), + WithAllowInsecure(allowInsecure)) + return newTLSConfigManager(co...) } // NewDisabledTLSConfigManager creates a TLSConfigManager that has TLS disabled. diff --git a/pkg/tlsconfig/configmanager_test.go b/pkg/tlsconfig/configmanager_test.go index 55d867c5e24..ddad4ecbf61 100644 --- a/pkg/tlsconfig/configmanager_test.go +++ b/pkg/tlsconfig/configmanager_test.go @@ -2,12 +2,20 @@ package tlsconfig import ( "crypto/tls" + "crypto/x509" "errors" + "fmt" "net" + "os" + "path" "testing" + "time" "github.com/influxdata/influxdb/pkg/testing/selfsigned" "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" ) func TestTLSConfigManager_ConsistentInstances(t *testing.T) { @@ -272,6 +280,13 @@ func TestTLSConfigManager_UseTLSFalse(t *testing.T) { require.NoError(t, manager.Close()) require.NoError(t, manager.Close()) // idempotent }) + + t.Run("explicit parameters override opts", func(t *testing.T) { + manager, err := NewTLSConfigManager(false, nil, "", "", false, WithUseTLS(true)) + require.NoError(t, err) + require.NotNil(t, manager) + require.False(t, manager.UseTLS()) + }) } func TestTLSConfigManager_UseTLSWithoutCert(t *testing.T) { @@ -684,7 +699,8 @@ func TestNewClientTLSConfigManager(t *testing.T) { // so we just verify the two constructors produce equivalent managers for various parameter combinations. t.Run("TLS disabled", func(t *testing.T) { - client := NewClientTLSConfigManager(false, nil, false) + client, err := NewClientTLSConfigManager(false, nil, false) + require.NoError(t, err) require.NotNil(t, client) explicit, err := NewTLSConfigManager(false, nil, "", "", false) require.NoError(t, err) @@ -699,7 +715,8 @@ func TestNewClientTLSConfigManager(t *testing.T) { }) t.Run("TLS enabled allowInsecure false", func(t *testing.T) { - client := NewClientTLSConfigManager(true, nil, false) + client, err := NewClientTLSConfigManager(true, nil, false) + require.NoError(t, err) require.NotNil(t, client) explicit, err := NewTLSConfigManager(true, nil, "", "", false) require.NoError(t, err) @@ -714,7 +731,8 @@ func TestNewClientTLSConfigManager(t *testing.T) { }) t.Run("TLS enabled allowInsecure true", func(t *testing.T) { - client := NewClientTLSConfigManager(true, nil, true) + client, err := NewClientTLSConfigManager(true, nil, true) + require.NoError(t, err) require.NotNil(t, client) explicit, err := NewTLSConfigManager(true, nil, "", "", true) require.NoError(t, err) @@ -734,7 +752,8 @@ func TestNewClientTLSConfigManager(t *testing.T) { ServerName: "test.example.com", } - client := NewClientTLSConfigManager(true, baseConfig, false) + client, err := NewClientTLSConfigManager(true, baseConfig, false) + require.NoError(t, err) require.NotNil(t, client) explicit, err := NewTLSConfigManager(true, baseConfig, "", "", false) require.NoError(t, err) @@ -756,7 +775,8 @@ func TestNewClientTLSConfigManager(t *testing.T) { ServerName: "original.example.com", } - client := NewClientTLSConfigManager(true, baseConfig, false) + client, err := NewClientTLSConfigManager(true, baseConfig, false) + require.NoError(t, err) require.NotNil(t, client) // Verify config is cloned (not same instance) @@ -768,6 +788,672 @@ func TestNewClientTLSConfigManager(t *testing.T) { require.NoError(t, client.Close()) }) + + t.Run("explicit parameters override opts", func(t *testing.T) { + client, err := NewClientTLSConfigManager(false, nil, false, WithUseTLS(true)) + require.NoError(t, err) + require.NotNil(t, client) + require.False(t, client.UseTLS()) + }) +} + +func simpleEchoServer(serverDone chan error, listener net.Listener, bufSize int) (rErr error) { + defer func() { + serverDone <- rErr + }() + conn, err := listener.Accept() + if err != nil { + return fmt.Errorf("error in Accept: %w", err) + } + defer func() { + if err := conn.Close(); err != nil { + rErr = errors.Join(rErr, fmt.Errorf("error in Close: %w", err)) + } + }() + buf := make([]byte, bufSize) + n, err := conn.Read(buf) + if err != nil { + return fmt.Errorf("error in Read: %w", err) + } + if _, err = conn.Write(buf[:n]); err != nil { + return fmt.Errorf("error in Write: %w", err) + } + return nil +} + +func TestTLSConfigManager_WithRootCAFiles(t *testing.T) { + t.Run("sets RootCAs from file", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Build expected CertPool + expectedPool := x509.NewCertPool() + caCert, err := os.ReadFile(ss.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(caCert)) + + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithRootCAFiles(ss.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.RootCAs) + require.True(t, tlsConfig.RootCAs.Equal(expectedPool), "RootCAs should match expected pool") + }) + + t.Run("client trusts server with CA", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Server manager with certificate + serverManager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false) + require.NoError(t, err) + defer func() { + require.NoError(t, serverManager.Close()) + }() + + // Client manager trusting the CA that signed the server certificate + clientManager, err := NewClientTLSConfigManager(true, nil, false, + WithRootCAFiles(ss.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, clientManager.Close()) + }() + + // Start server + listener, err := serverManager.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer func() { + require.NoError(t, listener.Close()) + }() + + // Server accepts and echoes + testData := []byte("hello") + serverDone := make(chan error, 1) + go simpleEchoServer(serverDone, listener, len(testData)) + + // Client connects with CA-trusted connection + conn, err := clientManager.Dial("tcp", listener.Addr().String()) + require.NoError(t, err) + defer func() { + require.NoError(t, conn.Close()) + }() + + n, err := conn.Write(testData) + require.NoError(t, err) + require.Equal(t, 5, n) + + buf := make([]byte, len(testData)) + n, err = conn.Read(buf) + require.NoError(t, err) + require.Equal(t, len(testData), n) + require.Equal(t, testData, buf) + + require.NoError(t, <-serverDone) + }) + + t.Run("client rejects server without CA", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Server manager + serverManager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false) + require.NoError(t, err) + defer func() { + require.NoError(t, serverManager.Close()) + }() + + // Client manager with empty RootCA pool (explicitly set, no system CAs) + clientManager, err := NewClientTLSConfigManager(true, nil, false, + WithRootCAIncludeSystem(false)) + require.NoError(t, err) + defer func() { + require.NoError(t, clientManager.Close()) + }() + + // Start server + listener, err := serverManager.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer func() { + require.NoError(t, listener.Close()) + }() + + // Server accepts and waits for handshake to complete (will fail due to client rejecting cert) + testData := []byte("test") + serverDone := make(chan error, 1) + go simpleEchoServer(serverDone, listener, len(testData)) + + // Client connection should fail certificate verification + conn, err := clientManager.Dial("tcp", listener.Addr().String()) + require.ErrorContains(t, err, "tls: failed to verify certificate: x509: certificate signed by unknown authority") + require.Nil(t, conn) + + // Server will see a handshake failure error (client sent bad_certificate alert) + serverErr := <-serverDone + require.ErrorContains(t, serverErr, "tls: bad certificate") + }) + + t.Run("multiple CA files", func(t *testing.T) { + ss1 := selfsigned.NewSelfSignedCert(t, selfsigned.WithCASubject("org1", "CA1")) + ss2 := selfsigned.NewSelfSignedCert(t, selfsigned.WithCASubject("org2", "CA2")) + + // Build expected CertPool with both CAs + expectedPool := x509.NewCertPool() + caCert1, err := os.ReadFile(ss1.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(caCert1)) + caCert2, err := os.ReadFile(ss2.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(caCert2)) + + manager, err := NewClientTLSConfigManager(true, nil, false, + WithRootCAFiles(ss1.CACertPath, ss2.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.RootCAs) + require.True(t, tlsConfig.RootCAs.Equal(expectedPool), "RootCAs should contain both CAs") + }) + + t.Run("error on nonexistent file", func(t *testing.T) { + _, err := NewClientTLSConfigManager(true, nil, false, + WithRootCAFiles("/nonexistent/ca.pem")) + require.ErrorIs(t, err, os.ErrNotExist) + require.ErrorContains(t, err, "error creating root CA pool: error reading file \"/nonexistent/ca.pem\" for CA store: open /nonexistent/ca.pem: no such file or directory") + }) + + t.Run("error on invalid PEM file", func(t *testing.T) { + // Create a temporary file with invalid PEM content + tmpDir := t.TempDir() + tmpFile := path.Join(tmpDir, "invalid.pem") + require.NoError(t, os.WriteFile(tmpFile, []byte("not a valid PEM file"), 0644)) + + manager, err := NewClientTLSConfigManager(true, nil, false, + WithRootCAFiles(tmpFile)) + require.ErrorContains(t, err, "error creating root CA pool: error adding certificates from \""+tmpFile+"\" to CA store: no valid certificates found") + require.Nil(t, manager) + }) +} + +func TestTLSConfigManager_WithRootCAIncludeSystem(t *testing.T) { + t.Run("includes system CA pool", func(t *testing.T) { + // Get expected system pool + expectedPool, err := x509.SystemCertPool() + require.NoError(t, err) + + manager, err := NewClientTLSConfigManager(true, nil, false, + WithRootCAIncludeSystem(true)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.RootCAs) + require.True(t, tlsConfig.RootCAs.Equal(expectedPool), "RootCAs should equal system pool") + }) + + t.Run("excludes system CA pool", func(t *testing.T) { + // Expected: empty pool + expectedPool := x509.NewCertPool() + + manager, err := NewClientTLSConfigManager(true, nil, false, + WithRootCAIncludeSystem(false)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.RootCAs) + require.True(t, tlsConfig.RootCAs.Equal(expectedPool), "RootCAs should be empty pool") + }) + + t.Run("combined with CA files", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Build expected pool: system + custom CA + expectedPool, err := x509.SystemCertPool() + require.NoError(t, err) + caCert, err := os.ReadFile(ss.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(caCert)) + + manager, err := NewClientTLSConfigManager(true, nil, false, + WithRootCAIncludeSystem(true), + WithRootCAFiles(ss.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.RootCAs) + require.True(t, tlsConfig.RootCAs.Equal(expectedPool), "RootCAs should contain system + custom CA") + }) +} + +func TestTLSConfigManager_WithClientCAFiles(t *testing.T) { + t.Run("sets ClientCAs from file", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Build expected CertPool + expectedPool := x509.NewCertPool() + caCert, err := os.ReadFile(ss.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(caCert)) + + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithClientCAFiles(ss.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.ClientCAs) + require.True(t, tlsConfig.ClientCAs.Equal(expectedPool), "ClientCAs should match expected pool") + }) + + t.Run("server verifies client certificate", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Server manager that requires client certificates. + serverManager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithClientCAFiles(ss.CACertPath), + WithClientAuth(tls.RequireAndVerifyClientCert)) + require.NoError(t, err) + defer func() { + require.NoError(t, serverManager.Close()) + }() + + // Start server + listener, err := serverManager.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer func() { + require.NoError(t, listener.Close()) + }() + + // Server accepts and echoes + testData := []byte("hello") + serverDone := make(chan error, 1) + go simpleEchoServer(serverDone, listener, len(testData)) + + // Client config with client certificate. Ignore certificate validity from server, we're testing + // client certificate functionality here. + clientManager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, true) + require.NoError(t, err) + defer func() { + require.NoError(t, clientManager.Close()) + }() + conn, err := clientManager.Dial("tcp", listener.Addr().String()) + require.NoError(t, err) + defer func() { + require.NoError(t, conn.Close()) + }() + + n, err := conn.Write(testData) + require.NoError(t, err) + require.Equal(t, len(testData), n) + + buf := make([]byte, len(testData)) + n, err = conn.Read(buf) + require.NoError(t, err) + require.Equal(t, len(testData), n) + require.Equal(t, testData, buf) + + require.NoError(t, <-serverDone) + }) + + t.Run("server rejects client without valid certificate", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + otherSS := selfsigned.NewSelfSignedCert(t, selfsigned.WithCASubject("other", "Other CA")) + + // Server manager that requires client certificates + serverManager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithClientAuth(tls.RequireAndVerifyClientCert), + WithClientCAFiles(ss.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, serverManager.Close()) + }() + serverTLSConfig := serverManager.TLSConfig() + require.NotNil(t, serverTLSConfig) + require.Equal(t, tls.RequireAndVerifyClientCert, serverTLSConfig.ClientAuth) + + // Client with certificate signed by different CA. Ignore server certificate validity. We're + // checking client certificate functionality here. + clientManager, err := NewTLSConfigManager(true, nil, otherSS.CertPath, otherSS.KeyPath, true) + require.NoError(t, err) + defer func() { + require.NoError(t, clientManager.Close()) + }() + + // Start server + listener, err := serverManager.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer func() { + require.NoError(t, listener.Close()) + }() + + // Server accepts and waits for handshake (will fail due to untrusted client cert) + testData := []byte("hello") + serverDone := make(chan error, 1) + go simpleEchoServer(serverDone, listener, len(testData)) + + // Client connects - Dial may succeed but handshake fails during Read + conn, dialErr := clientManager.Dial("tcp", listener.Addr().String()) + if dialErr == nil { + defer func() { + require.NoError(t, conn.Close()) + }() + // Read to wait for server response - this will fail when server rejects our cert + buf := make([]byte, 1) + _, dialErr = conn.Read(buf) + } + // Client sees remote error when server requires cert but client sends untrusted cert. + // (TLS client doesn't send cert if it's not signed by a CA the server trusts) + require.ErrorContains(t, dialErr, "remote error: tls: certificate required") + + // Server sees that client didn't provide a certificate (because client's cert + // wasn't signed by a CA in server's ClientCAs, so TLS stack didn't send it) + serverErr := <-serverDone + require.ErrorContains(t, serverErr, "tls: client didn't provide a certificate") + }) + + t.Run("multiple CA files", func(t *testing.T) { + ss1 := selfsigned.NewSelfSignedCert(t, selfsigned.WithCASubject("org1", "CA1")) + ss2 := selfsigned.NewSelfSignedCert(t, selfsigned.WithCASubject("org2", "CA2")) + + // Build expected CertPool with both CAs + expectedPool := x509.NewCertPool() + caCert1, err := os.ReadFile(ss1.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(caCert1)) + caCert2, err := os.ReadFile(ss2.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(caCert2)) + + manager, err := NewTLSConfigManager(true, nil, ss1.CertPath, ss1.KeyPath, false, + WithClientCAFiles(ss1.CACertPath, ss2.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.ClientCAs) + require.True(t, tlsConfig.ClientCAs.Equal(expectedPool), "ClientCAs should contain both CAs") + }) + + t.Run("error on nonexistent file", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + _, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithClientCAFiles("/nonexistent/ca.pem")) + require.ErrorIs(t, err, os.ErrNotExist) + require.ErrorContains(t, err, "error creating client CA pool: error reading file \"/nonexistent/ca.pem\" for CA store: open /nonexistent/ca.pem: no such file or directory") + }) + + t.Run("error on invalid PEM file", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Create a temporary file with invalid PEM content + tmpDir := t.TempDir() + tmpFile := path.Join(tmpDir, "invalid.pem") + require.NoError(t, os.WriteFile(tmpFile, []byte("not a valid PEM file"), 0644)) + + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithClientCAFiles(tmpFile)) + require.ErrorContains(t, err, "error creating client CA pool: error adding certificates from \""+tmpFile+"\" to CA store: no valid certificates found") + require.Nil(t, manager) + }) +} + +func TestTLSConfigManager_WithClientCAIncludeSystem(t *testing.T) { + t.Run("includes system CA pool", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Get expected system pool + expectedPool, err := x509.SystemCertPool() + require.NoError(t, err) + + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithClientCAIncludeSystem(true)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.ClientCAs) + require.True(t, tlsConfig.ClientCAs.Equal(expectedPool), "ClientCAs should equal system pool") + }) + + t.Run("excludes system CA pool", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Expected: empty pool + expectedPool := x509.NewCertPool() + + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithClientCAIncludeSystem(false)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.ClientCAs) + require.True(t, tlsConfig.ClientCAs.Equal(expectedPool), "ClientCAs should be empty pool") + }) + + t.Run("combined with CA files", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + // Build expected pool: system + custom CA + expectedPool, err := x509.SystemCertPool() + require.NoError(t, err) + caCert, err := os.ReadFile(ss.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(caCert)) + + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithClientCAIncludeSystem(true), + WithClientCAFiles(ss.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.ClientCAs) + require.True(t, tlsConfig.ClientCAs.Equal(expectedPool), "ClientCAs should contain system + custom CA") + }) +} + +func TestTLSConfigManager_CAOptionsNotSetByDefault(t *testing.T) { + // When no CA options are specified, the TLS config should not have + // custom CA pools set (allowing Go's default behavior) + ss := selfsigned.NewSelfSignedCert(t) + + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.Nil(t, tlsConfig.RootCAs, "RootCAs should be nil when no CA options specified") + require.Nil(t, tlsConfig.ClientCAs, "ClientCAs should be nil when no CA options specified") +} + +func TestTLSConfigManager_CAOptionsWithBaseConfig(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + anotherSS := selfsigned.NewSelfSignedCert(t, selfsigned.WithCASubject("another", "Another CA")) + + // Create a base config with existing RootCAs + basePool := x509.NewCertPool() + baseCACert, err := os.ReadFile(ss.CACertPath) + require.NoError(t, err) + require.True(t, basePool.AppendCertsFromPEM(baseCACert)) + + baseConfig := &tls.Config{ + RootCAs: basePool, + } + + // Build expected pool from anotherSS CA only + expectedPool := x509.NewCertPool() + anotherCACert, err := os.ReadFile(anotherSS.CACertPath) + require.NoError(t, err) + require.True(t, expectedPool.AppendCertsFromPEM(anotherCACert)) + + // Create manager with different CA file - should override base config + manager, err := NewClientTLSConfigManager(true, baseConfig, false, + WithRootCAFiles(anotherSS.CACertPath)) + require.NoError(t, err) + defer func() { + require.NoError(t, manager.Close()) + }() + + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + require.NotNil(t, tlsConfig.RootCAs) + // The RootCAs should match the new CA, not the base config + require.True(t, tlsConfig.RootCAs.Equal(expectedPool), "RootCAs should be overridden by WithRootCAFiles") + require.False(t, tlsConfig.RootCAs.Equal(basePool), "RootCAs should not equal base pool") +} + +// testManagerCheckTime is the TLS certificate check time in logging tests. +const testManagerCheckTime = 333 * time.Millisecond + +// testManagerCheckCapture time is how long to capture logs during logging tests. To prevent flaky tests, +// it should be more than testManagerCheckTime, but less than 2 * testManagerCheckTime. +const testManagerCheckCapture = 500 * time.Millisecond + +func TestTLSConfigManager_WithCertLoaderOptions(t *testing.T) { + // This test verifies that WithLogger, WithCertificateCheckInterval, and WithExpirationAdvanced + // properly pass through to the underlying TLSCertLoader. + + // Create a certificate that expires in 24 hours + notBefore := time.Now().UTC().Truncate(time.Minute).Add(-7 * 24 * time.Hour) + notAfter := time.Now().UTC().Truncate(time.Hour).Add(24 * time.Hour) + + ss := selfsigned.NewSelfSignedCert(t, selfsigned.WithNotBefore(notBefore), selfsigned.WithNotAfter(notAfter)) + + core, logs := observer.New(zapcore.InfoLevel) + logger := zap.New(core) + + // Create TLSConfigManager with all three options: + // - WithLogger: to capture log output + // - WithCertificateCheckInterval: to set a short check interval + // - WithExpirationAdvanced: to set the expiration warning window to 2 days (> 24 hours remaining) + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithLogger(logger), + WithCertificateCheckInterval(testManagerCheckTime), + WithExpirationAdvanced(2*24*time.Hour)) + require.NoError(t, err) + require.NotNil(t, manager) + defer func() { + require.NoError(t, manager.Close()) + }() + + // Wait for the certificate monitor to start + certLoader := manager.TLSCertLoader() + require.NotNil(t, certLoader) + certLoader.WaitForMonitorStart() + + // Verify the "Certificate will expire soon" warning is logged + checkWarning := func(t *testing.T) { + t.Helper() + warning := logs.FilterMessage("Certificate will expire soon").TakeAll() + require.Len(t, warning, 1) + require.Equal(t, zap.WarnLevel, warning[0].Level) + require.Equal(t, ss.CertPath, warning[0].ContextMap()["cert"]) + require.Equal(t, ss.KeyPath, warning[0].ContextMap()["key"]) + require.Equal(t, notAfter, warning[0].ContextMap()["NotAfter"]) + untilExpires, ok := warning[0].ContextMap()["untilExpires"].(time.Duration) + require.True(t, ok, "untilExpires should be a time.Duration") + timeExpires := time.Now().Add(untilExpires) + require.WithinDuration(t, notAfter, timeExpires, 2*time.Minute, "untilExpires varies more than expected") + logs.TakeAll() // dump all logs + } + checkWarning(t) + + // Check for warning during monitor (after another check interval) + require.Zero(t, logs.Len(), "init logs not dumped properly") + time.Sleep(testManagerCheckCapture) + checkWarning(t) +} + +func TestTLSConfigManager_WithIgnoreFilePermissions(t *testing.T) { + t.Run("fails without ignore when cert permissions too open", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + require.NoError(t, os.Chmod(ss.CertPath, 0660)) + _, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false) + require.ErrorContains(t, err, fmt.Sprintf("LoadCertificate: file permissions are too open: for %q, maximum is 0644 (-rw-r--r--) but found 0660 (-rw-rw----); extra permissions: 0020 (-----w----)", ss.CertPath)) + }) + + t.Run("succeeds with ignore when cert permissions too open", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + require.NoError(t, os.Chmod(ss.CertPath, 0660)) + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithIgnoreFilePermissions(true)) + require.NoError(t, err) + require.NotNil(t, manager) + defer func() { + require.NoError(t, manager.Close()) + }() + + // Verify the manager works correctly + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + certLoader := manager.TLSCertLoader() + require.NotNil(t, certLoader) + }) + + t.Run("fails without ignore when key permissions too open", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + require.NoError(t, os.Chmod(ss.KeyPath, 0644)) + _, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false) + require.ErrorContains(t, err, fmt.Sprintf("LoadCertificate: file permissions are too open: for %q, maximum is 0600 (-rw-------) but found 0644 (-rw-r--r--); extra permissions: 0044 (----r--r--)", ss.KeyPath)) + }) + + t.Run("succeeds with ignore when key permissions too open", func(t *testing.T) { + ss := selfsigned.NewSelfSignedCert(t) + + require.NoError(t, os.Chmod(ss.KeyPath, 0644)) + manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false, + WithIgnoreFilePermissions(true)) + require.NoError(t, err) + require.NotNil(t, manager) + defer func() { + require.NoError(t, manager.Close()) + }() + + // Verify the manager works correctly + tlsConfig := manager.TLSConfig() + require.NotNil(t, tlsConfig) + certLoader := manager.TLSCertLoader() + require.NotNil(t, certLoader) + }) } func TestTLSConfigManager_DialWithDialer(t *testing.T) { diff --git a/services/httpd/config.go b/services/httpd/config.go index 2b4439205ef..387c0d44beb 100644 --- a/services/httpd/config.go +++ b/services/httpd/config.go @@ -32,65 +32,67 @@ const ( // Config represents a configuration for a HTTP service. type Config struct { - Enabled bool `toml:"enabled"` - BindAddress string `toml:"bind-address"` - AuthEnabled bool `toml:"auth-enabled"` - LogEnabled bool `toml:"log-enabled"` - SuppressWriteLog bool `toml:"suppress-write-log"` - WriteTracing bool `toml:"write-tracing"` - FluxEnabled bool `toml:"flux-enabled"` - FluxLogEnabled bool `toml:"flux-log-enabled"` - FluxTesting bool `toml:"flux-testing"` - PprofEnabled bool `toml:"pprof-enabled"` - PprofAuthEnabled bool `toml:"pprof-auth-enabled"` - DebugPprofEnabled bool `toml:"debug-pprof-enabled"` - PingAuthEnabled bool `toml:"ping-auth-enabled"` - PromReadAuthEnabled bool `toml:"prom-read-auth-enabled"` - HTTPHeaders map[string]string `toml:"headers"` - HTTPSEnabled bool `toml:"https-enabled"` - HTTPSCertificate string `toml:"https-certificate"` - HTTPSPrivateKey string `toml:"https-private-key"` - MaxRowLimit int `toml:"max-row-limit"` - MaxConnectionLimit int `toml:"max-connection-limit"` - SharedSecret string `toml:"shared-secret"` - Realm string `toml:"realm"` - UnixSocketEnabled bool `toml:"unix-socket-enabled"` - UnixSocketGroup *toml.Group `toml:"unix-socket-group"` - UnixSocketPermissions toml.FileMode `toml:"unix-socket-permissions"` - BindSocket string `toml:"bind-socket"` - MaxBodySize int `toml:"max-body-size"` - AccessLogPath string `toml:"access-log-path"` - AccessLogStatusFilters []StatusFilter `toml:"access-log-status-filters"` - MaxConcurrentWriteLimit int `toml:"max-concurrent-write-limit"` - MaxEnqueuedWriteLimit int `toml:"max-enqueued-write-limit"` - EnqueuedWriteTimeout time.Duration `toml:"enqueued-write-timeout"` - TLS *tls.Config `toml:"-"` + Enabled bool `toml:"enabled"` + BindAddress string `toml:"bind-address"` + AuthEnabled bool `toml:"auth-enabled"` + LogEnabled bool `toml:"log-enabled"` + SuppressWriteLog bool `toml:"suppress-write-log"` + WriteTracing bool `toml:"write-tracing"` + FluxEnabled bool `toml:"flux-enabled"` + FluxLogEnabled bool `toml:"flux-log-enabled"` + FluxTesting bool `toml:"flux-testing"` + PprofEnabled bool `toml:"pprof-enabled"` + PprofAuthEnabled bool `toml:"pprof-auth-enabled"` + DebugPprofEnabled bool `toml:"debug-pprof-enabled"` + PingAuthEnabled bool `toml:"ping-auth-enabled"` + PromReadAuthEnabled bool `toml:"prom-read-auth-enabled"` + HTTPHeaders map[string]string `toml:"headers"` + HTTPSEnabled bool `toml:"https-enabled"` + HTTPSCertificate string `toml:"https-certificate"` + HTTPSPrivateKey string `toml:"https-private-key"` + HTTPSInsecureCertificate bool `toml:"https-insecure-certificate"` + MaxRowLimit int `toml:"max-row-limit"` + MaxConnectionLimit int `toml:"max-connection-limit"` + SharedSecret string `toml:"shared-secret"` + Realm string `toml:"realm"` + UnixSocketEnabled bool `toml:"unix-socket-enabled"` + UnixSocketGroup *toml.Group `toml:"unix-socket-group"` + UnixSocketPermissions toml.FileMode `toml:"unix-socket-permissions"` + BindSocket string `toml:"bind-socket"` + MaxBodySize int `toml:"max-body-size"` + AccessLogPath string `toml:"access-log-path"` + AccessLogStatusFilters []StatusFilter `toml:"access-log-status-filters"` + MaxConcurrentWriteLimit int `toml:"max-concurrent-write-limit"` + MaxEnqueuedWriteLimit int `toml:"max-enqueued-write-limit"` + EnqueuedWriteTimeout time.Duration `toml:"enqueued-write-timeout"` + TLS *tls.Config `toml:"-"` } // NewConfig returns a new Config with default settings. func NewConfig() Config { return Config{ - Enabled: true, - FluxEnabled: false, - FluxLogEnabled: false, - FluxTesting: false, - BindAddress: DefaultBindAddress, - LogEnabled: true, - PprofEnabled: true, - PprofAuthEnabled: false, - DebugPprofEnabled: false, - PingAuthEnabled: false, - PromReadAuthEnabled: false, - HTTPSEnabled: false, - HTTPSCertificate: "/etc/ssl/influxdb.pem", - MaxRowLimit: 0, - Realm: DefaultRealm, - UnixSocketEnabled: false, - UnixSocketPermissions: 0777, - BindSocket: DefaultBindSocket, - MaxBodySize: DefaultMaxBodySize, - EnqueuedWriteTimeout: DefaultEnqueuedWriteTimeout, - AccessLogStatusFilters: make([]StatusFilter, 0), + Enabled: true, + FluxEnabled: false, + FluxLogEnabled: false, + FluxTesting: false, + BindAddress: DefaultBindAddress, + LogEnabled: true, + PprofEnabled: true, + PprofAuthEnabled: false, + DebugPprofEnabled: false, + PingAuthEnabled: false, + PromReadAuthEnabled: false, + HTTPSEnabled: false, + HTTPSCertificate: "/etc/ssl/influxdb.pem", + HTTPSInsecureCertificate: false, + MaxRowLimit: 0, + Realm: DefaultRealm, + UnixSocketEnabled: false, + UnixSocketPermissions: 0777, + BindSocket: DefaultBindSocket, + MaxBodySize: DefaultMaxBodySize, + EnqueuedWriteTimeout: DefaultEnqueuedWriteTimeout, + AccessLogStatusFilters: make([]StatusFilter, 0), } } @@ -103,13 +105,14 @@ func (c Config) Diagnostics() (*diagnostics.Diagnostics, error) { } return diagnostics.RowFromMap(map[string]interface{}{ - "enabled": true, - "bind-address": c.BindAddress, - "https-enabled": c.HTTPSEnabled, - "max-row-limit": c.MaxRowLimit, - "max-connection-limit": c.MaxConnectionLimit, - "access-log-path": c.AccessLogPath, - "flux-enabled": c.FluxEnabled, + "enabled": true, + "bind-address": c.BindAddress, + "https-enabled": c.HTTPSEnabled, + "https-insecure-certificate": c.HTTPSInsecureCertificate, + "max-row-limit": c.MaxRowLimit, + "max-connection-limit": c.MaxConnectionLimit, + "access-log-path": c.AccessLogPath, + "flux-enabled": c.FluxEnabled, }), nil } diff --git a/services/httpd/service.go b/services/httpd/service.go index 3d2c902df92..143f9bdabea 100644 --- a/services/httpd/service.go +++ b/services/httpd/service.go @@ -72,6 +72,9 @@ type Service struct { // exposed to client code because a key certificate might be loaded on a config reload. key string + // insecureCert is true if certificate file permissions should be ignored. + insecureCert bool + limit int tlsConfig *tls.Config err chan error @@ -113,6 +116,7 @@ func NewService(c Config) *Service { https: c.HTTPSEnabled, cert: c.HTTPSCertificate, key: c.HTTPSPrivateKey, + insecureCert: c.HTTPSInsecureCertificate, limit: c.MaxConnectionLimit, tlsConfig: c.TLS, err: make(chan error, 2), // There could be two serve calls that fail. @@ -150,11 +154,13 @@ func (s *Service) Open() error { s.Handler.Open() // Open listener. - if tm, err := tlsconfig.NewTLSConfigManager(s.https, s.tlsConfig, s.cert, s.key, false, tlsconfig.WithLogger(s.Logger)); err != nil { + tm, err := tlsconfig.NewTLSConfigManager(s.https, s.tlsConfig, s.cert, s.key, false, + tlsconfig.WithAllowInsecure(s.insecureCert), + tlsconfig.WithLogger(s.Logger)) + if err != nil { return fmt.Errorf("httpd: error creating TLS manager: %w", err) - } else { - s.tlsManager = tm } + s.tlsManager = tm if ln, err := s.tlsManager.Listen("tcp", s.addr); err != nil { return fmt.Errorf("httpd: error creating listener: %w", err) diff --git a/services/opentsdb/config.go b/services/opentsdb/config.go index d7004993a28..8d3bc5d9dd4 100644 --- a/services/opentsdb/config.go +++ b/services/opentsdb/config.go @@ -37,19 +37,20 @@ const ( // Config represents the configuration of the OpenTSDB service. type Config struct { - Enabled bool `toml:"enabled"` - BindAddress string `toml:"bind-address"` - Database string `toml:"database"` - RetentionPolicy string `toml:"retention-policy"` - ConsistencyLevel string `toml:"consistency-level"` - TLSEnabled bool `toml:"tls-enabled"` - Certificate string `toml:"certificate"` - PrivateKey string `toml:"private-key"` - BatchSize int `toml:"batch-size"` - BatchPending int `toml:"batch-pending"` - BatchTimeout toml.Duration `toml:"batch-timeout"` - LogPointErrors bool `toml:"log-point-errors"` - TLS *tls.Config `toml:"-"` + Enabled bool `toml:"enabled"` + BindAddress string `toml:"bind-address"` + Database string `toml:"database"` + RetentionPolicy string `toml:"retention-policy"` + ConsistencyLevel string `toml:"consistency-level"` + TLSEnabled bool `toml:"tls-enabled"` + Certificate string `toml:"certificate"` + PrivateKey string `toml:"private-key"` + InsecureCertificate bool `toml:"insecure-certificate"` + BatchSize int `toml:"batch-size"` + BatchPending int `toml:"batch-pending"` + BatchTimeout toml.Duration `toml:"batch-timeout"` + LogPointErrors bool `toml:"log-point-errors"` + TLS *tls.Config `toml:"-"` } // NewConfig returns a new config for the service. diff --git a/services/opentsdb/service.go b/services/opentsdb/service.go index d98111fe916..363e937e52c 100644 --- a/services/opentsdb/service.go +++ b/services/opentsdb/service.go @@ -50,12 +50,13 @@ type Service struct { ln net.Listener // main listener httpln *chanListener // http channel-based listener - wg sync.WaitGroup - tls bool - tlsManager *tlsconfig.TLSConfigManager - tlsConfig *tls.Config - cert string - privateKey string + wg sync.WaitGroup + tls bool + tlsManager *tlsconfig.TLSConfigManager + tlsConfig *tls.Config + cert string + privateKey string + insecureCert bool mu sync.RWMutex ready bool // Has the required database been created? @@ -95,6 +96,7 @@ func NewService(c Config) (*Service, error) { tlsConfig: d.TLS, cert: d.Certificate, privateKey: d.PrivateKey, + insecureCert: d.InsecureCertificate, BindAddress: d.BindAddress, Database: d.Database, RetentionPolicy: d.RetentionPolicy, @@ -133,17 +135,19 @@ func (s *Service) Open() error { go func() { defer s.wg.Done(); s.processBatches(s.batcher) }() // Open listener. - if cm, err := tlsconfig.NewTLSConfigManager(s.tls, s.tlsConfig, s.cert, s.privateKey, false, tlsconfig.WithLogger(s.Logger)); err != nil { + cm, err := tlsconfig.NewTLSConfigManager(s.tls, s.tlsConfig, s.cert, s.privateKey, false, + tlsconfig.WithIgnoreFilePermissions(s.insecureCert), + tlsconfig.WithLogger(s.Logger)) + if err != nil { return fmt.Errorf("opentsdb: error creating TLS manager: %w", err) - } else { - s.tlsManager = cm } + s.tlsManager = cm - if ln, err := s.tlsManager.Listen("tcp", s.BindAddress); err != nil { + ln, err := s.tlsManager.Listen("tcp", s.BindAddress) + if err != nil { return fmt.Errorf("opentsdb: error creating listener: %w", err) - } else { - s.ln = ln } + s.ln = ln s.Logger.Info("Listening on TCP", zap.Stringer("addr", s.ln.Addr()),