Ensure a consistent TLS configuration (#173) (#178) - backport to 1.11

.github/workflows/tests.yaml

@@ -15,7 +15,7 @@ jobs:
       - uses: actions/checkout@v2
       - uses: actions/setup-go@v2
         with:
-          go-version: 1.18.0
+          go-version: 1.19.1
       - uses: actions/cache@v2
         with:
           path: |
@@ -32,23 +32,23 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        kind-k8s-version: [1.21.10, 1.22.7, 1.23.6, 1.24.0]
+        kind-k8s-version: [1.22.13, 1.23.10, 1.24.4, 1.25.0]
     steps:
       - uses: actions/checkout@v2
       - name: Create K8s Kind Cluster
         uses: helm/kind-action@v1.2.0
         with:
-          version: v0.13.0
+          version: v0.14.0
           cluster_name: vault-plugin-auth-kubernetes
           config: integrationtest/kind/config.yaml
           node_image: kindest/node:v${{ matrix.kind-k8s-version }}
       # Must come _after_ kind-action, because the kind step also sets up a kubectl binary.
       - uses: azure/setup-kubectl@v2.0
         with:
-          version: 'v1.24.0'
+          version: 'v1.25.0'
       - uses: actions/setup-go@v2
         with:
-          go-version: 1.18.0
+          go-version: 1.19.1
       - uses: actions/cache@v2
         with:
           path: |

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.24.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:
@@ -52,12 +53,13 @@ vault-image:
 setup-integration-test: teardown-integration-test vault-image
 	kind --name ${KIND_CLUSTER_NAME} load docker-image hashicorp/vault:dev
 	kubectl create namespace test
-	helm install vault vault --repo https://helm.releases.hashicorp.com --version=0.19.0 \
+	helm install vault vault --repo https://helm.releases.hashicorp.com --version=0.22.0 \
 		--wait --timeout=5m \
 		--namespace=test \
 		--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
@@ -69,4 +71,4 @@ setup-integration-test: teardown-integration-test vault-image
 .PHONY: teardown-integration-test
 teardown-integration-test:
 	helm uninstall vault --namespace=test || true
-	kubectl delete --ignore-not-found namespace test
+	kubectl delete --ignore-not-found namespace test

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 {