Ensure a consistent TLS configuration (#173) (#178)

* Ensure a consistent TLS configuration for k8s API requests Previously, it was possible for the http.Client's Transport to be missing the necessary root CAs to ensure that all TLS connections between the auth engine and the Kubernetes API were validated against a configured set of CA certificates. This fix ensures that the http.Client's Transport is always consistent with the configured CA cert chain, by introducing a periodic TLS configuration checker that is started as part of the backend's initialization. Other fixes: - only update the client's transport when the CA certificate pool has changed. Co-authored-by: Tom Proctor <[email protected]>
hashicorp · Jan 13, 2023 · e608c62 · e608c62
1 parent 2ad6f99
commit e608c62
Show file tree

Hide file tree

Showing 8 changed files with 867 additions and 137 deletions.
diff --git a/Makefile b/Makefile
@@ -1,8 +1,9 @@
+TESTARGS  ?= '-test.v'
 # kind cluster name
-KIND_CLUSTER_NAME?=vault-plugin-auth-kubernetes
+KIND_CLUSTER_NAME ?= vault-plugin-auth-kubernetes
 
 # kind k8s version
-KIND_K8S_VERSION?=v1.25.0
+KIND_K8S_VERSION ?= v1.25.0
 
 .PHONY: default
 default: dev
@@ -13,11 +14,11 @@ dev:
 
 .PHONY: test
 test: fmtcheck
-	CGO_ENABLED=0 go test ./... $(TESTARGS) -timeout=20m
+	CGO_ENABLED=0 go test $(TESTARGS) -timeout=20m ./...
 
 .PHONY: integration-test
 integration-test:
-	INTEGRATION_TESTS=true CGO_ENABLED=0 go test github.com/hashicorp/vault-plugin-auth-kubernetes/integrationtest/... $(TESTARGS) -count=1 -timeout=20m
+	INTEGRATION_TESTS=true CGO_ENABLED=0 go test $(TESTARGS) -count=1 -timeout=20m github.com/hashicorp/vault-plugin-auth-kubernetes/integrationtest/...
 
 .PHONY: fmtcheck
 fmtcheck:
@@ -58,6 +59,7 @@ setup-integration-test: teardown-integration-test vault-image
 		--set server.dev.enabled=true \
 		--set server.image.tag=dev \
 		--set server.image.pullPolicy=Never \
+		--set server.logLevel=trace \
 		--set injector.enabled=false \
 		--set server.extraArgs="-dev-plugin-dir=/vault/plugin_directory"
 	kubectl patch --namespace=test statefulset vault --patch-file integrationtest/vault/hostPortPatch.yaml

diff --git a/backend.go b/backend.go
@@ -6,6 +6,7 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -26,6 +27,7 @@ const (
 	aliasNameSourceSAUid   = "serviceaccount_uid"
 	aliasNameSourceSAName  = "serviceaccount_name"
 	aliasNameSourceDefault = aliasNameSourceSAUid
+	minTLSVersion          = tls.VersionTLS12
 )
 
 var (
@@ -44,6 +46,17 @@ var (
 	// caReloadPeriod is the time period how often the in-memory copy of local
 	// CA cert can be used, before reading it again from disk.
 	caReloadPeriod = 1 * time.Hour
+
+	// defaultHorizon provides the default duration to be used
+	// in the tlsConfigUpdater's time.Ticker, setup in runTLSConfigUpdater()
+	defaultHorizon = time.Second * 30
+
+	// defaultMinHorizon provides the minimum duration that can be specified
+	// in the tlsConfigUpdater's time.Ticker, setup in runTLSConfigUpdater()
+	defaultMinHorizon = time.Second * 5
+
+	errTLSConfigNotSet  = errors.New("TLSConfig not set")
+	errHTTPClientNotSet = errors.New("http.Client not set")
 )
 
 // kubeAuthBackend implements logical.Backend
@@ -53,8 +66,11 @@ type kubeAuthBackend struct {
 	// default HTTP client for connection reuse
 	httpClient *http.Client
 
+	// tlsConfig is periodically updated whenever the CA certificate configuration changes.
+	tlsConfig *tls.Config
+
 	// reviewFactory is used to configure the strategy for doing a token review.
-	// Currently the only options are using the kubernetes API or mocking the
+	// Currently, the only options are using the kubernetes API or mocking the
 	// review. Mocks should only be used in tests.
 	reviewFactory tokenReviewFactory
 
@@ -71,22 +87,47 @@ type kubeAuthBackend struct {
 	// - disable_local_ca_jwt is false
 	localCACertReader *cachingFileReader
 
+	// tlsConfigUpdaterRunning is used to signal the current state of the tlsConfig updater routine.
+	tlsConfigUpdaterRunning bool
+
+	// tlsConfigUpdateCancelFunc should be called in the backend's Clean(), set in initialize().
+	tlsConfigUpdateCancelFunc context.CancelFunc
+
 	l sync.RWMutex
+
+	// tlsMu provides the lock for synchronizing updates to the tlsConfig.
+	tlsMu sync.RWMutex
 }
 
 // Factory returns a new backend as logical.Backend.
 func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, error) {
 	b := Backend()
+
 	if err := b.Setup(ctx, conf); err != nil {
 		return nil, err
 	}
+
 	return b, nil
 }
 
+var getDefaultHTTPClient = cleanhttp.DefaultPooledClient
+
+func getDefaultTLSConfig() *tls.Config {
+	return &tls.Config{
+		MinVersion: minTLSVersion,
+	}
+}
+
 func Backend() *kubeAuthBackend {
 	b := &kubeAuthBackend{
 		localSATokenReader: newCachingFileReader(localJWTPath, jwtReloadPeriod, time.Now),
 		localCACertReader:  newCachingFileReader(localCACertPath, caReloadPeriod, time.Now),
+		// Set default HTTP client
+		httpClient: getDefaultHTTPClient(),
+		// Set the default TLSConfig
+		tlsConfig: getDefaultTLSConfig(),
+		// Set the review factory to default to calling into the kubernetes API.
+		reviewFactory: tokenReviewAPIFactory,
 	}
 
 	b.Backend = &framework.Backend{
@@ -109,46 +150,129 @@ func Backend() *kubeAuthBackend {
 			pathsRole(b),
 		),
 		InitializeFunc: b.initialize,
+		Clean:          b.cleanup,
 	}
 
-	// Set default HTTP client
-	b.httpClient = cleanhttp.DefaultPooledClient()
-
-	// Set the review factory to default to calling into the kubernetes API.
-	b.reviewFactory = tokenReviewAPIFactory
-
 	return b
 }
 
 // initialize is used to handle the state of config values just after the K8s plugin has been mounted
 func (b *kubeAuthBackend) initialize(ctx context.Context, req *logical.InitializationRequest) error {
-	// Try to load the config on initialization
-	config, err := b.loadConfig(ctx, req.Storage)
+	updaterCtx, cancel := context.WithCancel(context.Background())
+	if err := b.runTLSConfigUpdater(updaterCtx, req.Storage, defaultHorizon); err != nil {
+		cancel()
+		return err
+	}
+
+	b.tlsConfigUpdateCancelFunc = cancel
+
+	config, err := b.config(ctx, req.Storage)
 	if err != nil {
 		return err
 	}
-	if config == nil {
+
+	if config != nil {
+		if err := b.updateTLSConfig(config); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func (b *kubeAuthBackend) cleanup(_ context.Context) {
+	b.shutdownTLSConfigUpdater()
+}
+
+// validateHTTPClientInit that the Backend's HTTPClient and TLSConfig has been properly instantiated.
+func (b *kubeAuthBackend) validateHTTPClientInit() error {
+	if b.httpClient == nil {
+		return errHTTPClientNotSet
+	}
+	if b.tlsConfig == nil {
+		return errTLSConfigNotSet
+	}
+
+	return nil
+}
+
+// runTLSConfigUpdater sets up a routine that periodically calls b.updateTLSConfig(). This ensures that the
+// httpClient's TLS configuration is consistent with the backend's stored configuration.
+func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Storage, horizon time.Duration) error {
+	b.tlsMu.Lock()
+	defer b.tlsMu.Unlock()
+
+	if b.tlsConfigUpdaterRunning {
 		return nil
 	}
 
-	b.l.Lock()
-	defer b.l.Unlock()
-	// If we have a CA cert build the TLSConfig
-	if len(config.CACert) > 0 {
-		certPool := x509.NewCertPool()
-		certPool.AppendCertsFromPEM([]byte(config.CACert))
+	if horizon < defaultMinHorizon {
+		return fmt.Errorf("update horizon must be equal to or greater than %s", defaultMinHorizon)
+	}
+
+	if err := b.validateHTTPClientInit(); err != nil {
+		return err
+	}
+
+	updateTLSConfig := func(ctx context.Context, s logical.Storage) error {
+		config, err := b.config(ctx, s)
+		if err != nil {
+			return fmt.Errorf("failed config read, err=%w", err)
+		}
 
-		tlsConfig := &tls.Config{
-			MinVersion: tls.VersionTLS12,
-			RootCAs:    certPool,
+		if config == nil {
+			b.Logger().Trace("Skipping TLSConfig update, no configuration set")
+			return nil
+		}
+
+		if err := b.updateTLSConfig(config); err != nil {
+			return err
 		}
 
-		b.httpClient.Transport.(*http.Transport).TLSClientConfig = tlsConfig
+		return nil
 	}
 
+	var wg sync.WaitGroup
+	wg.Add(1)
+	ticker := time.NewTicker(horizon)
+	go func(ctx context.Context, s logical.Storage) {
+		defer func() {
+			b.tlsMu.Lock()
+			defer b.tlsMu.Unlock()
+			ticker.Stop()
+			b.tlsConfigUpdaterRunning = false
+			b.Logger().Trace("TLSConfig updater shutdown completed")
+		}()
+
+		b.Logger().Trace("TLSConfig updater starting", "horizon", horizon)
+		b.tlsConfigUpdaterRunning = true
+		wg.Done()
+		for {
+			select {
+			case <-ctx.Done():
+				b.Logger().Trace("TLSConfig updater shutting down")
+				return
+			case <-ticker.C:
+				if err := updateTLSConfig(ctx, s); err != nil {
+					b.Logger().Warn("TLSConfig update failed, retrying",
+						"horizon", defaultHorizon.String(), "err", err)
+				}
+			}
+		}
+	}(ctx, s)
+	wg.Wait()
+
 	return nil
 }
 
+func (b *kubeAuthBackend) shutdownTLSConfigUpdater() {
+	if b.tlsConfigUpdateCancelFunc != nil {
+		b.Logger().Debug("TLSConfig updater shutdown requested")
+		b.tlsConfigUpdateCancelFunc()
+		b.tlsConfigUpdateCancelFunc = nil
+	}
+}
+
 // config takes a storage object and returns a kubeConfig object.
 // It does not return local token and CA file which are specific to the pod we run in.
 func (b *kubeAuthBackend) config(ctx context.Context, s logical.Storage) (*kubeConfig, error) {
@@ -255,6 +379,70 @@ func (b *kubeAuthBackend) role(ctx context.Context, s logical.Storage, name stri
 	return role, nil
 }
 
+// getHTTPClient return the backend's HTTP client for connecting to the Kubernetes API.
+func (b *kubeAuthBackend) getHTTPClient() (*http.Client, error) {
+	b.tlsMu.RLock()
+	defer b.tlsMu.RUnlock()
+
+	if err := b.validateHTTPClientInit(); err != nil {
+		return nil, err
+	}
+
+	return b.httpClient, nil
+}
+
+// updateTLSConfig ensures that the httpClient's TLS configuration is consistent
+// with the backend's stored configuration.
+func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error {
+	b.tlsMu.Lock()
+	defer b.tlsMu.Unlock()
+
+	if err := b.validateHTTPClientInit(); err != nil {
+		return err
+	}
+
+	// attempt to read the CA certificates from the config directly or from the filesystem.
+	var caCertBytes []byte
+	if config.CACert != "" {
+		caCertBytes = []byte(config.CACert)
+	} else if !config.DisableLocalCAJwt && b.localCACertReader != nil {
+		data, err := b.localCACertReader.ReadFile()
+		if err != nil {
+			return err
+		}
+		caCertBytes = []byte(data)
+	}
+
+	certPool := x509.NewCertPool()
+	if len(caCertBytes) > 0 {
+		if ok := certPool.AppendCertsFromPEM(caCertBytes); !ok {
+			b.Logger().Warn("Configured CA PEM data contains no valid certificates, TLS verification will fail")
+		}
+	} else {
+		// provide an empty certPool
+		b.Logger().Warn("No CA certificates configured, TLS verification will fail")
+		// TODO: think about supporting host root CA certificates via a configuration toggle,
+		// in which case RootCAs should be set to nil
+	}
+
+	// only refresh the Root CAs if they have changed since the last full update.
+	if !b.tlsConfig.RootCAs.Equal(certPool) {
+		b.Logger().Trace("Root CA certificate pool has changed, updating the client's transport")
+		transport, ok := b.httpClient.Transport.(*http.Transport)
+		if !ok {
+			// should never happen
+			return fmt.Errorf("type assertion failed for %T", b.httpClient.Transport)
+		}
+
+		b.tlsConfig.RootCAs = certPool
+		transport.TLSClientConfig = b.tlsConfig
+	} else {
+		b.Logger().Trace("Root CA certificate pool is unchanged, no update required")
+	}
+
+	return nil
+}
+
 func validateAliasNameSource(source string) error {
 	for _, s := range aliasNameSources {
 		if s == source {