From a847f415592a794a7a54c0cd3ece68a3a374f550 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Fri, 25 Nov 2022 18:34:28 -0500 Subject: [PATCH 01/12] Ensure a consistent TLS configuration 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, but introducing a periodic TLS configuration checker that is started as part of the backend's initialization. The periodic checker provides a signal handler that will update the backend's TLS configuration on SIGHUP. Other fixes: - only update the client's transport when the CA certificate pool has changed. --- backend.go | 176 +++++++++++++++++++++++++++++++++++++++++++------ path_config.go | 33 ++-------- path_login.go | 7 +- 3 files changed, 166 insertions(+), 50 deletions(-) diff --git a/backend.go b/backend.go index 2c090c6b..b9166022 100644 --- a/backend.go +++ b/backend.go @@ -8,8 +8,11 @@ import ( "encoding/json" "fmt" "net/http" + "os" + "os/signal" "strings" "sync" + "syscall" "time" "github.com/hashicorp/go-cleanhttp" @@ -53,6 +56,9 @@ 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 // review. Mocks should only be used in tests. @@ -83,10 +89,22 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, return b, nil } +var getDefaultHTTPClient = cleanhttp.DefaultPooledClient + +func defaultTLSConfig() *tls.Config { + return &tls.Config{ + MinVersion: tls.VersionTLS12, + } +} + 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 review factory to default to calling into the kubernetes API. + reviewFactory: tokenReviewAPIFactory, } b.Backend = &framework.Backend{ @@ -111,41 +129,82 @@ func Backend() *kubeAuthBackend { InitializeFunc: b.initialize, } - // 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) + config, err := b.config(ctx, req.Storage) if err != nil { return err } - if config == nil { - return nil + + if config != nil { + if err := b.updateTLSConfig(config); err != nil { + return err + } } - 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)) - - tlsConfig := &tls.Config{ - MinVersion: tls.VersionTLS12, - RootCAs: certPool, + return b.runTLSConfigUpdater(context.Background(), req.Storage) +} + +// 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) error { + updateTLSConfig := func(ctx context.Context, s logical.Storage, force bool) error { + config, err := b.config(ctx, s) + if err != nil { + return fmt.Errorf("failed config read, err=%w", err) + } + + 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 } + horizon := time.Second * 30 + ticker := time.NewTicker(horizon) + wCtx, cancel := context.WithCancel(ctx) + go func(ctx context.Context, cancel context.CancelFunc, s logical.Storage) { + sigs := make(chan os.Signal, 1) + signal.Notify(sigs, syscall.SIGHUP, syscall.SIGTERM, syscall.SIGINT) + defer signal.Stop(sigs) + + b.Logger().Trace("Starting TLS config updater", "horizon", horizon.String()) + for { + select { + case <-ctx.Done(): + b.Logger().Trace("Shutting down TLS config updater") + return + case <-ticker.C: + if err := updateTLSConfig(ctx, s, false); err != nil { + b.Logger().Warn("Retrying failed update", "horizon", horizon.String(), "err", err) + } + case sig := <-sigs: + b.Logger().Trace(fmt.Sprintf("Caught signal %v", sig)) + switch sig { + case syscall.SIGHUP: + // update the TLS configuration when the plugin process receives a SIGHUP + b.Logger().Trace(fmt.Sprintf("Calling updateTLSConfig() on signal %v", sig)) + if err := updateTLSConfig(ctx, s, true); err != nil { + b.Logger().Warn("Retrying failed update", "horizon", horizon.String(), "err", err) + } + default: + // shutdown on all other signals + b.Logger().Trace(fmt.Sprintf("Calling cancel() on signal %v", sig)) + cancel() + } + } + } + }(wCtx, cancel, s) + return nil } @@ -255,6 +314,81 @@ 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(config *kubeConfig) (*http.Client, error) { + if b.httpClient == nil { + return nil, fmt.Errorf("the backend's http.Client has not been initialized") + } + + if b.tlsConfig == nil { + // ensure that HTTP client's transport TLS configuration is initialized + // this adds some belt-and-suspenders, + // since in most cases the TLS configuration would have already been initialized. + if err := b.updateTLSConfig(config); err != nil { + return nil, err + } + } + + return b.httpClient, nil +} + +// updateTLSConfig ensures that the httpClient's TLS configuration is consistent +// with backend's stored configuration. +func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { + b.l.Lock() + defer b.l.Unlock() + + if b.httpClient == nil { + return fmt.Errorf("the backend's http.Client has not been initialized") + } + + // attempt to read the CA certificates the config directly or from the filesystem. + var caCertBytes []byte + if config.CACert != "" { + caCertBytes = []byte(config.CACert) + } else if !config.DisableLocalCAJwt && b.localCACertReader != nil { + // TODO: this may block on I/O, investigate a proper mitigation + data, err := b.localCACertReader.ReadFile() + if err != nil { + return err + } + caCertBytes = []byte(data) + } + + transport, ok := b.httpClient.Transport.(*http.Transport) + if !ok { + // should never happen + return fmt.Errorf("type assertion failed for %T", b.httpClient.Transport) + } + + if b.tlsConfig == nil { + b.tlsConfig = defaultTLSConfig() + } + + 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") + 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 { diff --git a/path_config.go b/path_config.go index 2c46b9c0..c9760801 100644 --- a/path_config.go +++ b/path_config.go @@ -5,11 +5,9 @@ import ( "crypto" "crypto/ecdsa" "crypto/rsa" - "crypto/tls" "crypto/x509" "encoding/pem" "errors" - "net/http" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" @@ -157,32 +155,6 @@ func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Requ DisableLocalCAJwt: disableLocalJWT, } - b.l.Lock() - defer b.l.Unlock() - - // Determine if we load the local CA cert or the CA cert provided - // by the kubernetes_ca_cert path into the backend's HTTP client - certPool := x509.NewCertPool() - tlsConfig := &tls.Config{ - MinVersion: tls.VersionTLS12, - } - if disableLocalJWT || len(caCert) > 0 { - certPool.AppendCertsFromPEM([]byte(config.CACert)) - tlsConfig.RootCAs = certPool - - b.httpClient.Transport.(*http.Transport).TLSClientConfig = tlsConfig - } else { - localCACert, err := b.localCACertReader.ReadFile() - if err != nil { - return nil, err - } - - certPool.AppendCertsFromPEM([]byte(localCACert)) - tlsConfig.RootCAs = certPool - - b.httpClient.Transport.(*http.Transport).TLSClientConfig = tlsConfig - } - var err error for i, pem := range pemList { config.PublicKeys[i], err = parsePublicKeyPEM([]byte(pem)) @@ -191,6 +163,10 @@ func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Requ } } + if err := b.updateTLSConfig(config); err != nil { + return logical.ErrorResponse(err.Error()), nil + } + entry, err := logical.StorageEntryJSON(configPath, config) if err != nil { return nil, err @@ -199,6 +175,7 @@ func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Requ if err := req.Storage.Put(ctx, entry); err != nil { return nil, err } + return nil, nil } diff --git a/path_login.go b/path_login.go index ad745a2e..672daa83 100644 --- a/path_login.go +++ b/path_login.go @@ -129,8 +129,13 @@ func (b *kubeAuthBackend) pathLogin(ctx context.Context, req *logical.Request, d return nil, err } + client, err := b.getHTTPClient(config) + if err != nil { + return nil, logical.ErrUnrecoverable + } + // look up the JWT token in the kubernetes API - err = serviceAccount.lookup(ctx, b.httpClient, jwtStr, b.reviewFactory(config)) + err = serviceAccount.lookup(ctx, client, jwtStr, b.reviewFactory(config)) if err != nil { b.Logger().Debug(`login unauthorized`, "err", err) From 81e5219a7c5d1236f455d906c8443377d64561d5 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Tue, 29 Nov 2022 14:20:58 -0800 Subject: [PATCH 02/12] Fix logic error when comparing root CA pools --- Makefile | 1 + backend.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 0eee1f26..fba09fc3 100644 --- a/Makefile +++ b/Makefile @@ -58,6 +58,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 index b9166022..8e55b9dc 100644 --- a/backend.go +++ b/backend.go @@ -378,7 +378,7 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { } // only refresh the Root CAs if they have changed since the last full update. - if b.tlsConfig.RootCAs.Equal(certPool) { + if !b.tlsConfig.RootCAs.Equal(certPool) { b.Logger().Trace("Root CA certificate pool has changed, updating the client's transport") b.tlsConfig.RootCAs = certPool transport.TLSClientConfig = b.tlsConfig From 97824be4ff1b28017efef16c2786fc4ccbdcfc7a Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Tue, 6 Dec 2022 18:44:15 -0500 Subject: [PATCH 03/12] Add unit tests for TLSConfig handling --- backend.go | 49 +++-- backend_test.go | 514 ++++++++++++++++++++++++++++++++++++++++++++ common_test.go | 105 +++++++++ path_config_test.go | 81 ------- 4 files changed, 656 insertions(+), 93 deletions(-) create mode 100644 backend_test.go create mode 100644 common_test.go diff --git a/backend.go b/backend.go index 8e55b9dc..d525b69f 100644 --- a/backend.go +++ b/backend.go @@ -29,6 +29,7 @@ const ( aliasNameSourceSAUid = "serviceaccount_uid" aliasNameSourceSAName = "serviceaccount_name" aliasNameSourceDefault = aliasNameSourceSAUid + minTLSVersion = tls.VersionTLS12 ) var ( @@ -47,6 +48,12 @@ 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 for the tlsConfigUpdater. + defaultHorizon = time.Second * 30 + + // defaultMinHorizon for the tlsConfigUpdater. + defaultMinHorizon = time.Second * 5 ) // kubeAuthBackend implements logical.Backend @@ -77,6 +84,9 @@ 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 + l sync.RWMutex } @@ -93,7 +103,7 @@ var getDefaultHTTPClient = cleanhttp.DefaultPooledClient func defaultTLSConfig() *tls.Config { return &tls.Config{ - MinVersion: tls.VersionTLS12, + MinVersion: minTLSVersion, } } @@ -145,12 +155,22 @@ func (b *kubeAuthBackend) initialize(ctx context.Context, req *logical.Initializ } } - return b.runTLSConfigUpdater(context.Background(), req.Storage) + return b.runTLSConfigUpdater(context.Background(), req.Storage, defaultHorizon) } // 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) error { +func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Storage, horizon time.Duration) error { + b.l.Lock() + defer b.l.Unlock() + if b.tlsConfigUpdaterRunning { + return nil + } + + if horizon < defaultMinHorizon { + return fmt.Errorf("update horizon must be equal to or greater than %s", defaultMinHorizon) + } + updateTLSConfig := func(ctx context.Context, s logical.Storage, force bool) error { config, err := b.config(ctx, s) if err != nil { @@ -169,42 +189,47 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto return nil } - horizon := time.Second * 30 ticker := time.NewTicker(horizon) wCtx, cancel := context.WithCancel(ctx) go func(ctx context.Context, cancel context.CancelFunc, s logical.Storage) { sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGHUP, syscall.SIGTERM, syscall.SIGINT) defer signal.Stop(sigs) + defer func() { + b.tlsConfigUpdaterRunning = false + }() - b.Logger().Trace("Starting TLS config updater", "horizon", horizon.String()) + b.Logger().Trace("Starting TLS config updater", "horizon", horizon) for { + var err error select { case <-ctx.Done(): b.Logger().Trace("Shutting down TLS config updater") return case <-ticker.C: - if err := updateTLSConfig(ctx, s, false); err != nil { - b.Logger().Warn("Retrying failed update", "horizon", horizon.String(), "err", err) - } + err = updateTLSConfig(ctx, s, false) case sig := <-sigs: b.Logger().Trace(fmt.Sprintf("Caught signal %v", sig)) switch sig { case syscall.SIGHUP: // update the TLS configuration when the plugin process receives a SIGHUP b.Logger().Trace(fmt.Sprintf("Calling updateTLSConfig() on signal %v", sig)) - if err := updateTLSConfig(ctx, s, true); err != nil { - b.Logger().Warn("Retrying failed update", "horizon", horizon.String(), "err", err) - } + err = updateTLSConfig(ctx, s, true) default: // shutdown on all other signals b.Logger().Trace(fmt.Sprintf("Calling cancel() on signal %v", sig)) cancel() } } + + if err != nil { + b.Logger().Warn("TLSConfig update failed, retrying", "horizon", defaultHorizon.String(), "err", err) + } } }(wCtx, cancel, s) + b.tlsConfigUpdaterRunning = true + return nil } @@ -333,7 +358,7 @@ func (b *kubeAuthBackend) getHTTPClient(config *kubeConfig) (*http.Client, error } // updateTLSConfig ensures that the httpClient's TLS configuration is consistent -// with backend's stored configuration. +// with the backend's stored configuration. func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { b.l.Lock() defer b.l.Unlock() diff --git a/backend_test.go b/backend_test.go new file mode 100644 index 00000000..20cd560d --- /dev/null +++ b/backend_test.go @@ -0,0 +1,514 @@ +package kubeauth + +import ( + "context" + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "net/http" + "os" + "path/filepath" + "reflect" + "testing" + "time" + + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/vault/sdk/framework" + "github.com/hashicorp/vault/sdk/logical" +) + +func Test_kubeAuthBackend_updateTLSConfig(t *testing.T) { + defaultCertPool := getTestCertPool(t, testCACert) + localCertPool := getTestCertPool(t, testLocalCACert) + otherCertPool := getTestCertPool(t, testOtherCACert) + + type testConfig struct { + config *kubeConfig + expectTLSConfig *tls.Config + localCACert string + wantErr bool + expectError error + } + tests := []struct { + name string + httpClient *http.Client + wantErr bool + configs []testConfig + }{ + { + name: "fail-client-not-initialized", + httpClient: nil, + configs: []testConfig{ + { + wantErr: true, + expectError: errors.New("the backend's http.Client has not been initialized"), + }, + }, + }, + { + name: "ca-certs-from-config-source", + httpClient: getDefaultHTTPClient(), + wantErr: false, + configs: []testConfig{ + { + config: &kubeConfig{ + CACert: testCACert, + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: defaultCertPool, + }, + }, + { + config: &kubeConfig{ + CACert: testLocalCACert, + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: localCertPool, + }, + }, + { + config: &kubeConfig{ + CACert: testCACert, + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: defaultCertPool, + }, + }, + }, + }, + { + name: "ca-certs-from-file-source", + httpClient: getDefaultHTTPClient(), + configs: []testConfig{ + { + config: &kubeConfig{ + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: defaultCertPool, + }, + localCACert: testCACert, + }, + { + config: &kubeConfig{ + DisableLocalCAJwt: false, + }, + localCACert: testLocalCACert, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: localCertPool, + }, + }, + }, + wantErr: false, + }, + { + name: "ca-certs-mixed-source", + httpClient: getDefaultHTTPClient(), + configs: []testConfig{ + { + config: &kubeConfig{ + CACert: testCACert, + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: defaultCertPool, + }, + }, + { + config: &kubeConfig{ + DisableLocalCAJwt: false, + }, + localCACert: testLocalCACert, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: localCertPool, + }, + }, + { + config: &kubeConfig{ + CACert: testOtherCACert, + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: otherCertPool, + }, + }, + { + config: &kubeConfig{ + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: defaultCertPool, + }, + localCACert: testCACert, + }, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := &kubeAuthBackend{ + Backend: &framework.Backend{}, + httpClient: tt.httpClient, + } + + if err := b.Setup(context.Background(), + &logical.BackendConfig{ + Logger: hclog.NewNullLogger(), + }); err != nil { + t.Fatalf("failed to setup the backend, err=%v", err) + } + + localFile := filepath.Join(t.TempDir(), "ca.crt") + b.localCACertReader = &cachingFileReader{ + path: localFile, + currentTime: time.Now().UTC, + ttl: 0, + } + for idx, config := range tt.configs { + t.Run(fmt.Sprintf("config-%d", idx), func(t *testing.T) { + if config.localCACert != "" { + if err := os.WriteFile(localFile, []byte(config.localCACert), 0600); err != nil { + t.Fatalf("failed to write local file %q", localFile) + } + t.Cleanup(func() { + if err := os.Remove(localFile); err != nil { + t.Fatal(err) + } + }) + } + + err := b.updateTLSConfig(config.config) + if config.wantErr && err == nil { + t.Fatalf("updateTLSConfig() error = %v, wantErr %v", err, config.wantErr) + } + + if !reflect.DeepEqual(err, config.expectError) { + t.Fatalf("updateTLSConfig() error = %v, expectErr %v", err, config.expectError) + } + + if config.wantErr { + return + } + + assertTLSConfigEquals(t, b.tlsConfig, config.expectTLSConfig) + assertValidTransport(t, b, config.expectTLSConfig) + }) + } + }) + } +} + +func Test_kubeAuthBackend_initialize(t *testing.T) { + defaultCertPool := getTestCertPool(t, testCACert) + + tests := []struct { + name string + httpClient *http.Client + ctx context.Context + req *logical.InitializationRequest + config *kubeConfig + tlsConfig *tls.Config + expectTLSConfig *tls.Config + wantErr bool + expectErr error + }{ + { + name: "fail-client-not-initialized", + ctx: context.Background(), + httpClient: nil, + req: &logical.InitializationRequest{ + Storage: &logical.InmemStorage{}, + }, + config: &kubeConfig{ + CACert: testCACert, + DisableLocalCAJwt: false, + }, + wantErr: true, + expectErr: errors.New("the backend's http.Client has not been initialized"), + }, + { + name: "no-config", + ctx: context.Background(), + httpClient: getDefaultHTTPClient(), + req: &logical.InitializationRequest{ + Storage: &logical.InmemStorage{}, + }, + wantErr: false, + expectErr: nil, + }, + { + name: "initialized-from-config", + ctx: context.Background(), + httpClient: getDefaultHTTPClient(), + req: &logical.InitializationRequest{ + Storage: &logical.InmemStorage{}, + }, + config: &kubeConfig{ + CACert: testCACert, + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: defaultCertPool, + }, + wantErr: false, + expectErr: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := &kubeAuthBackend{ + Backend: &framework.Backend{}, + httpClient: tt.httpClient, + tlsConfig: tt.tlsConfig, + } + + if err := b.Setup(context.Background(), + &logical.BackendConfig{ + Logger: hclog.NewNullLogger(), + StorageView: tt.req.Storage, + }); err != nil { + t.Fatalf("failed to setup the backend, err=%v", err) + } + + if tt.config != nil { + entry, err := logical.StorageEntryJSON(configPath, tt.config) + if err != nil { + t.Fatal(err) + } + + if err := tt.req.Storage.Put(tt.ctx, entry); err != nil { + t.Fatal(err) + } + } + + if b.tlsConfigUpdaterRunning { + t.Fatalf("tlsConfigUpdater started before initialize()") + } + + ctx, _ := context.WithTimeout(tt.ctx, time.Second*30) + err := b.initialize(ctx, tt.req) + if tt.wantErr && err == nil { + t.Errorf("initialize() error = %v, wantErr %v", err, tt.wantErr) + + } + + if !reflect.DeepEqual(err, tt.expectErr) { + t.Fatalf("initialize() error = %v, expectErr %v", err, tt.expectErr) + } + + if tt.wantErr { + return + } + + if tt.config != nil { + assertTLSConfigEquals(t, b.tlsConfig, tt.expectTLSConfig) + assertValidTransport(t, b, tt.expectTLSConfig) + } else { + if b.tlsConfig != nil { + t.Errorf("initialize(), unexpected tlsConfig initialization") + } + } + + if !b.tlsConfigUpdaterRunning { + t.Fatalf("tlsConfigUpdater not started from initialize()") + } + }) + } +} + +func Test_kubeAuthBackend_runTLSConfigUpdater(t *testing.T) { + defaultCertPool := getTestCertPool(t, testCACert) + otherCertPool := getTestCertPool(t, testOtherCACert) + + type testConfig struct { + config *kubeConfig + expectTLSConfig *tls.Config + } + + tests := []struct { + name string + ctx context.Context + storage logical.Storage + tlsConfig *tls.Config + horizon time.Duration + minHorizon time.Duration + wantErr bool + expectErr error + configs []*testConfig + }{ + { + name: "initialized-from-config", + ctx: context.Background(), + storage: &logical.InmemStorage{}, + horizon: time.Millisecond * 500, + minHorizon: time.Millisecond * 499, + wantErr: false, + expectErr: nil, + configs: []*testConfig{ + { + config: &kubeConfig{ + CACert: testCACert, + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: defaultCertPool, + }, + }, + { + config: &kubeConfig{ + CACert: testOtherCACert, + DisableLocalCAJwt: false, + }, + expectTLSConfig: &tls.Config{ + MinVersion: minTLSVersion, + RootCAs: otherCertPool, + }, + }, + }, + }, + { + name: "fail-min-horizon", + ctx: context.Background(), + storage: &logical.InmemStorage{}, + horizon: time.Millisecond * 500, + wantErr: true, + expectErr: fmt.Errorf("update horizon must be equal to or greater than %s", defaultMinHorizon), + }, + } + + d := defaultMinHorizon + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.minHorizon > 0 { + defer (func() { + defaultMinHorizon = d + })() + defaultMinHorizon = tt.minHorizon + } + b := &kubeAuthBackend{ + Backend: &framework.Backend{}, + httpClient: getDefaultHTTPClient(), + tlsConfig: tt.tlsConfig, + } + + if err := b.Setup(context.Background(), + &logical.BackendConfig{ + Logger: hclog.NewNullLogger(), + StorageView: tt.storage, + }); err != nil { + t.Fatalf("failed to setup the backend, err=%v", err) + } + + if b.tlsConfigUpdaterRunning { + t.Fatalf("tlsConfigUpdater already started") + } + + configCount := len(tt.configs) + ctx, cancel := context.WithTimeout(tt.ctx, tt.horizon*time.Duration(configCount+1)) + defer cancel() + err := b.runTLSConfigUpdater(ctx, tt.storage, tt.horizon) + if tt.wantErr && err == nil { + t.Errorf("runTLSConfigUpdater() error = %v, wantErr %v", err, tt.wantErr) + + } + + if !reflect.DeepEqual(err, tt.expectErr) { + t.Fatalf("runTLSConfigUpdater() error = %v, expectErr %v", err, tt.expectErr) + } + + if tt.wantErr { + return + } + + if !b.tlsConfigUpdaterRunning { + t.Fatalf("tlsConfigUpdater not started") + } + + if configCount > 0 { + for idx := 0; idx < configCount; idx++ { + t.Run(fmt.Sprintf("config-%d", idx), func(t *testing.T) { + config := tt.configs[idx] + if config.config != nil { + entry, err := logical.StorageEntryJSON(configPath, config.config) + if err != nil { + t.Fatal(err) + } + + if err := tt.storage.Put(tt.ctx, entry); err != nil { + t.Fatal(err) + } + } + + time.Sleep(tt.horizon * 2) + if b.tlsConfig == nil { + t.Fatalf("runTLSConfigUpdater(), ") + } + assertTLSConfigEquals(t, b.tlsConfig, config.expectTLSConfig) + assertValidTransport(t, b, config.expectTLSConfig) + }) + } + } else { + if b.tlsConfig != nil { + t.Errorf("runTLSConfigUpdater(), unexpected tlsConfig initialization") + } + } + + cancel() + time.Sleep(tt.horizon) + if b.tlsConfigUpdaterRunning { + t.Fatalf("tlsConfigUpdater did not shutdown cleanly") + } + }) + } +} + +func assertTLSConfigEquals(t *testing.T, actual, expected *tls.Config) { + t.Helper() + + if !actual.RootCAs.Equal(expected.RootCAs) { + t.Errorf("updateTLSConfig() actual RootCAs = %v, expected RootCAs %v", + actual.RootCAs, expected.RootCAs) + } + if actual.MinVersion != expected.MinVersion { + t.Errorf("updateTLSConfig() actual MinVersion = %v, expected MinVersion %v", + actual.MinVersion, expected.MinVersion) + } + +} + +func assertValidTransport(t *testing.T, b *kubeAuthBackend, expected *tls.Config) { + t.Helper() + + transport, ok := b.httpClient.Transport.(*http.Transport) + if !ok { + t.Fatalf("type assertion failed for %T", b.httpClient.Transport) + } + + assertTLSConfigEquals(t, transport.TLSClientConfig, expected) +} + +func getTestCertPool(t *testing.T, cert string) *x509.CertPool { + t.Helper() + + pool := x509.NewCertPool() + if ok := pool.AppendCertsFromPEM([]byte(cert)); !ok { + t.Fatalf("test certificate contains no valid certificates") + } + return pool +} diff --git a/common_test.go b/common_test.go new file mode 100644 index 00000000..7adf0195 --- /dev/null +++ b/common_test.go @@ -0,0 +1,105 @@ +package kubeauth + +const ( + testLocalCACert = `-----BEGIN CERTIFICATE----- +MIIDVDCCAjwCCQDFiyFY1M6afTANBgkqhkiG9w0BAQsFADBsMQswCQYDVQQGEwJV +UzETMBEGA1UECAwKV2FzaGluZ3RvbjEQMA4GA1UEBwwHU2VhdHRsZTEgMB4GA1UE +CgwXVmF1bHQgVGVzdGluZyBBdXRob3JpdHkxFDASBgNVBAMMC2V4YW1wbGUubmV0 +MB4XDTIwMDkxODAxMjkxM1oXDTQ1MDkxODAxMjkxM1owbDELMAkGA1UEBhMCVVMx +EzARBgNVBAgMCldhc2hpbmd0b24xEDAOBgNVBAcMB1NlYXR0bGUxIDAeBgNVBAoM +F1ZhdWx0IFRlc3RpbmcgQXV0aG9yaXR5MRQwEgYDVQQDDAtleGFtcGxlLm5ldDCC +ASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALCA9oKv+ESRHX2e/iq1PlGr +zD23/MBS0V+fWQDY0hyEqY98CGwRtF6pEcLEYsreArj5/zznsIevLkNOD+beg43y +WpEJlCPgDhGXI/Oima6ooHVEIMaIKLjK7GrSzAb3rNRGACwrR/u/IKaFl+XJG0qx +g8mOZ3fByaAlIk+shVLUcIedNN1tNR+6/4ZpHg7PDjrZXP4XKrmKPTh4yqfu+BtZ +9IY2oyregqEsGW1/3h1NM+LHGVakTV2d/mwMYHhwoq9Y8BD+PemT5z8TmhH/cIk5 +P8Q8ud5/q6YTIJg9TELKebLAeNtRNnNoHeUoRTjiW1MBwNHtgyTTY+H3W/9Dne0C +AwEAATANBgkqhkiG9w0BAQsFAAOCAQEAXmygFkGIBnXxKlsTDiV8RW2iHLgFdZFJ +hcU8UpxZhhaL5JbQl6byfbHjrX31q7ii8uC8FcbW0AEdnEQAb9Ui6a+if7HwXNmI +DTlYl+lMlk9RtWvExw6AEEbg5nCpGaKexm7wJgzYGP9by9pQ7wX/CS7ofCzCK+Al +uSIqjPkMC201ZXH39n1lxxq6BacdYjv8wo4mMzi8iTSQGVWPdjHZVYOClFgN6hoj +8SkrrSe888a0H+i7EknRxC4sLRaMUK/FAvwtXaSZi2djruAtQzQGQ56m1phC2C/k +k9aL00AQ9Y4KTfiJD7LK8YIZDnFKLOCJhYgKCLCOVwOHb7836SNCxA== +-----END CERTIFICATE-----` + + testLocalJWT = `eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IlZhdWx0IFRlc3QiLCJpYXQiOjExMjM1OH0.GOC8w-MyhorgojB20SPNyH_ECsBjYJH89hjntOxSywA` + + testRSACert = `-----BEGIN CERTIFICATE----- +MIIDcjCCAlqgAwIBAgIBAjANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwptaW5p +a3ViZUNBMB4XDTE3MDgzMDE5MDgzNloXDTE4MDgzMDE5MDgzNlowLDEXMBUGA1UE +ChMOc3lzdGVtOm1hc3RlcnMxETAPBgNVBAMTCG1pbmlrdWJlMIIBIjANBgkqhkiG +9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxD3eM3+WNc4phxAeQxNOmcybKlNJWowuC12u +v+cGJWxxpDx/OoEIxKI5wmgHxEwFCZL545sjfLqyBcgxQR2xSCib+bYzjBtfA6uV +6d/35nurzz21okcMffc5xKMyZhEwt98WAvYWD71Bihz7iGBq5Sw9md6pqnkNoScR +Hhi3Vl94a6D6shwb6nXA2hlwYLcnoKtpe3Ptq6MW6CpfBA8C11q5eeW4xdvrwKt3 +Vd1TgFeEnnqwzUWGapU2uwwUfbRkLTDvrp6791uq0Vo7mzz00xYhV1PLCeAdpJEK +3Vr74FT7jHIbPlzi/qjRBVFKf9IRXnhbjrCl7S0Ayev1Fao4TQIDAQABo4G1MIGy +MA4GA1UdDwEB/wQEAwIFoDAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIw +DAYDVR0TAQH/BAIwADBzBgNVHREEbDBqgiRrdWJlcm5ldGVzLmRlZmF1bHQuc3Zj +LmNsdXN0ZXIubG9jYWyCFmt1YmVybmV0ZXMuZGVmYXVsdC5zdmOCEmt1YmVybmV0 +ZXMuZGVmYXVsdIIKa3ViZXJuZXRlc4cEwKhjZIcECgAAATANBgkqhkiG9w0BAQsF +AAOCAQEAIw8rKuryhhl527wf9q/VrWixzZ1jCLvyc/60z9rWpXxKFxT8AyCsHirM +F4fHXW4Brcoh/Dc2ci36cUbuywIyxHjgVUG45D4jPPWskY1++ZSfJfSXAuA8eFew +c+No3WPkmZB6ZOZ6q5iPY+FOgDZC7ddWmGuZrle51gBL347cU7H1BrTm6Lm6kXRs +fHRZJX2+B8lnsXsS3QF2BTU0ymuCxCCQxub/GhPZVz3nNNtro1z7/szLUVP1c1/8 +p7HP3k7caxfp346TZ/HgbV9sJEkHP7Ym7n9E7LSyUTSxXwBRPraH1WQzEgFNPSUV +V0n6FBLiejOTPKapJ2F0tIqAyJHFug== +-----END CERTIFICATE-----` + + testECCert = `-----BEGIN CERTIFICATE----- +MIICZDCCAeugAwIBAgIJALM9NbK8WRuBMAkGByqGSM49BAEwRTELMAkGA1UEBhMC +dXMxEzARBgNVBAgTClNvbWUtU3RhdGUxITAfBgNVBAoTGEludGVybmV0IFdpZGdp +dHMgUHR5IEx0ZDAeFw0xNzA5MTExNzQ2NDNaFw0yNzA5MDkxNzQ2NDNaMEUxCzAJ +BgNVBAYTAnVzMRMwEQYDVQQIEwpTb21lLVN0YXRlMSEwHwYDVQQKExhJbnRlcm5l +dCBXaWRnaXRzIFB0eSBMdGQwdjAQBgcqhkjOPQIBBgUrgQQAIgNiAATcqsBLxKP+ +UHk7Y6ktGGFvfrIfIXHxeZe3Xwt691CWfdmJFvrGzyzW5/AbJIuO1utdOsqUStAm +W/Scfxop/FGadKqR4nAWLNBI4intgnf0r1rzBCSOmanolHqxQPqQ0UOjgacwgaQw +HQYDVR0OBBYEFHxh1pTd8ApEzg0gKMwwt01aA10TMHUGA1UdIwRuMGyAFHxh1pTd +8ApEzg0gKMwwt01aA10ToUmkRzBFMQswCQYDVQQGEwJ1czETMBEGA1UECBMKU29t +ZS1TdGF0ZTEhMB8GA1UEChMYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkggkAsz01 +srxZG4EwDAYDVR0TBAUwAwEB/zAJBgcqhkjOPQQBA2gAMGUCMCR+CvAoNBhqSe2M +4qWWD/9XX/0qmf0O442Qowcg5MWH1+mwl1s7ozinvbTPDPaYDwIxAM54qKhuL6xt +GxqJpa7Onn15Hu8zTsdzeYBqUUXA6wtn+Pa7197CgUkfty9yc2eeQw== +-----END CERTIFICATE-----` + + testCACert = ` +-----BEGIN CERTIFICATE----- +MIIC5zCCAc+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwptaW5p +a3ViZUNBMB4XDTE3MDgxMDIzMTQ1NVoXDTI3MDgwODIzMTQ1NVowFTETMBEGA1UE +AxMKbWluaWt1YmVDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAN8d +w2p/KXRkm+vzOO0eT1vYBWP7fKsnng9/g5nnXAJlt9NxpOSolRcyItm/04R0E1jx +jpgsdzkybc+QU5ZiszOYN833/D5hCNVAABVivpDd2P8wVKXN46cB99e24etUVBqG +5aR0Ku3IBsJjCN9efhF+XRCA2gy/KaXMdKJhHfdtc8hCr7G9+2wO2G58FLmIfEyH +owviOGt0BSnCtMpsA8ZgGQyfqgSd5u466aCv6oj0MyzsMnfS38niM53Rlv4IY6ol +taYbWXtCNndQ2S687qE0qTCxhE95Bm2Nfkqct4R1798sJz83xNv8hALvxr/vPK/J +2XkIm3oo3YKG4n/CHXcCAwEAAaNCMEAwDgYDVR0PAQH/BAQDAgKkMB0GA1UdJQQW +MBQGCCsGAQUFBwMCBggrBgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3 +DQEBCwUAA4IBAQCSkrhE1PczqeqXfRaWayJUbXWPwKFbszO0MhGB1zwnPZq39qjY +ySQiGvnjV3fP+N5CTQAwMNe79Xiw31fSoexgceCPJpraWrTOLdCv04SbGDBapMFM +aezBu9jzZm0CNt60jHXWXuHHVPFX6u7ZR8W+RiBvsT8GZ5U6sNs3aN3M9Vym06BL +aSphIw1v+hRlPfnrlJwUnQp158DRgkt/9ncTa/k88KoIoZAbulaiGB4zHxxkbura +GSlgpZzhHSrBDLuXf65GHwwGxSExhgY5AA/n8rumGVvE8IYohS9yg/jOG0xP2WQH +u/ABoYtOyseO+lgElA8R4PB9MtwgN6c/b0xH +-----END CERTIFICATE-----` + + testOtherCACert = `-----BEGIN CERTIFICATE----- +MIIC/jCCAeagAwIBAgIBADANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwprdWJl +cm5ldGVzMB4XDTIyMTEyOTIwNDYwNVoXDTMyMTEyNjIwNDYwNVowFTETMBEGA1UE +AxMKa3ViZXJuZXRlczCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAOYO +AGi6hysYe4PF3sCyVXAFM72X96F51ncmDg+Ihhvt9Touj7Yo0LLMmy1UPM3YzEdV +z2IV1iksLIBObkct2eBGt2SFjLMhphZdA34mPAzFZhpvNgn1U2uUSqfp5phg00Mg +DdXLE0LFIVqAGkoBysr89l6P2MaTibcKoZwAhpMbATLcn1QcXF/NLzYuO9FPvrUL +mAR/HslDz10LBsMjtgRKXd2dX4yrQlYSB7YmLlu/bLKdjiE1a/+EO4wNcl/JJ+vu +fzPwxWALej/k61mcP+4JxfjgY53AM7vaZ+P9Yb0bGw7r1bozgP7+FGL1f6onTxeG +7+FECpErmrv9IQocgh8CAwEAAaNZMFcwDgYDVR0PAQH/BAQDAgKkMA8GA1UdEwEB +/wQFMAMBAf8wHQYDVR0OBBYEFE0Y+CYtusQDfr8tqSZZ7PEcDJjwMBUGA1UdEQQO +MAyCCmt1YmVybmV0ZXMwDQYJKoZIhvcNAQELBQADggEBAJK57wm9rH0yVmjmY1ES +kE8e+pTnXZkKaqUce/d7cPRn1+0Dtutvxl/j3P4DUjba7lVGYYNKp1xy2xVvg7Dl +mXyJigvBoTGyzJzNDIUJz8Kgse4eCrwl59WP94K83cVRLeUq+3amLwzubUNbezsW +QwcCyACuzTetR5ZXEg7iIS4HDy+ER2yjuY6d0GPLG+FH02WMrlE7mmxNfZOSy/5E +pEDcN/HcXM47TP7XgWW0rfQli3RucuqMV7LHvvpiGIWwfutrK9g7Py91W2JbQCA0 +D14XDzgsruCwlWAP1FMvLMIPhSknpIJd9Xql+0/Ae1yl9f3Uamj3mDtBKg8/U5nJ +0wU= +-----END CERTIFICATE----- +` +) diff --git a/path_config_test.go b/path_config_test.go index 3ee9a9ad..335c49c1 100644 --- a/path_config_test.go +++ b/path_config_test.go @@ -535,84 +535,3 @@ func TestConfig_LocalJWTRenewal(t *testing.T) { t.Fatalf("got unexpected JWT: expected %#v\n got %#v\n", token2, conf.TokenReviewerJWT) } } - -var testLocalCACert string = `-----BEGIN CERTIFICATE----- -MIIDVDCCAjwCCQDFiyFY1M6afTANBgkqhkiG9w0BAQsFADBsMQswCQYDVQQGEwJV -UzETMBEGA1UECAwKV2FzaGluZ3RvbjEQMA4GA1UEBwwHU2VhdHRsZTEgMB4GA1UE -CgwXVmF1bHQgVGVzdGluZyBBdXRob3JpdHkxFDASBgNVBAMMC2V4YW1wbGUubmV0 -MB4XDTIwMDkxODAxMjkxM1oXDTQ1MDkxODAxMjkxM1owbDELMAkGA1UEBhMCVVMx -EzARBgNVBAgMCldhc2hpbmd0b24xEDAOBgNVBAcMB1NlYXR0bGUxIDAeBgNVBAoM -F1ZhdWx0IFRlc3RpbmcgQXV0aG9yaXR5MRQwEgYDVQQDDAtleGFtcGxlLm5ldDCC -ASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALCA9oKv+ESRHX2e/iq1PlGr -zD23/MBS0V+fWQDY0hyEqY98CGwRtF6pEcLEYsreArj5/zznsIevLkNOD+beg43y -WpEJlCPgDhGXI/Oima6ooHVEIMaIKLjK7GrSzAb3rNRGACwrR/u/IKaFl+XJG0qx -g8mOZ3fByaAlIk+shVLUcIedNN1tNR+6/4ZpHg7PDjrZXP4XKrmKPTh4yqfu+BtZ -9IY2oyregqEsGW1/3h1NM+LHGVakTV2d/mwMYHhwoq9Y8BD+PemT5z8TmhH/cIk5 -P8Q8ud5/q6YTIJg9TELKebLAeNtRNnNoHeUoRTjiW1MBwNHtgyTTY+H3W/9Dne0C -AwEAATANBgkqhkiG9w0BAQsFAAOCAQEAXmygFkGIBnXxKlsTDiV8RW2iHLgFdZFJ -hcU8UpxZhhaL5JbQl6byfbHjrX31q7ii8uC8FcbW0AEdnEQAb9Ui6a+if7HwXNmI -DTlYl+lMlk9RtWvExw6AEEbg5nCpGaKexm7wJgzYGP9by9pQ7wX/CS7ofCzCK+Al -uSIqjPkMC201ZXH39n1lxxq6BacdYjv8wo4mMzi8iTSQGVWPdjHZVYOClFgN6hoj -8SkrrSe888a0H+i7EknRxC4sLRaMUK/FAvwtXaSZi2djruAtQzQGQ56m1phC2C/k -k9aL00AQ9Y4KTfiJD7LK8YIZDnFKLOCJhYgKCLCOVwOHb7836SNCxA== ------END CERTIFICATE-----` - -var testLocalJWT string = `eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IlZhdWx0IFRlc3QiLCJpYXQiOjExMjM1OH0.GOC8w-MyhorgojB20SPNyH_ECsBjYJH89hjntOxSywA` - -var testRSACert string = `-----BEGIN CERTIFICATE----- -MIIDcjCCAlqgAwIBAgIBAjANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwptaW5p -a3ViZUNBMB4XDTE3MDgzMDE5MDgzNloXDTE4MDgzMDE5MDgzNlowLDEXMBUGA1UE -ChMOc3lzdGVtOm1hc3RlcnMxETAPBgNVBAMTCG1pbmlrdWJlMIIBIjANBgkqhkiG -9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxD3eM3+WNc4phxAeQxNOmcybKlNJWowuC12u -v+cGJWxxpDx/OoEIxKI5wmgHxEwFCZL545sjfLqyBcgxQR2xSCib+bYzjBtfA6uV -6d/35nurzz21okcMffc5xKMyZhEwt98WAvYWD71Bihz7iGBq5Sw9md6pqnkNoScR -Hhi3Vl94a6D6shwb6nXA2hlwYLcnoKtpe3Ptq6MW6CpfBA8C11q5eeW4xdvrwKt3 -Vd1TgFeEnnqwzUWGapU2uwwUfbRkLTDvrp6791uq0Vo7mzz00xYhV1PLCeAdpJEK -3Vr74FT7jHIbPlzi/qjRBVFKf9IRXnhbjrCl7S0Ayev1Fao4TQIDAQABo4G1MIGy -MA4GA1UdDwEB/wQEAwIFoDAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIw -DAYDVR0TAQH/BAIwADBzBgNVHREEbDBqgiRrdWJlcm5ldGVzLmRlZmF1bHQuc3Zj -LmNsdXN0ZXIubG9jYWyCFmt1YmVybmV0ZXMuZGVmYXVsdC5zdmOCEmt1YmVybmV0 -ZXMuZGVmYXVsdIIKa3ViZXJuZXRlc4cEwKhjZIcECgAAATANBgkqhkiG9w0BAQsF -AAOCAQEAIw8rKuryhhl527wf9q/VrWixzZ1jCLvyc/60z9rWpXxKFxT8AyCsHirM -F4fHXW4Brcoh/Dc2ci36cUbuywIyxHjgVUG45D4jPPWskY1++ZSfJfSXAuA8eFew -c+No3WPkmZB6ZOZ6q5iPY+FOgDZC7ddWmGuZrle51gBL347cU7H1BrTm6Lm6kXRs -fHRZJX2+B8lnsXsS3QF2BTU0ymuCxCCQxub/GhPZVz3nNNtro1z7/szLUVP1c1/8 -p7HP3k7caxfp346TZ/HgbV9sJEkHP7Ym7n9E7LSyUTSxXwBRPraH1WQzEgFNPSUV -V0n6FBLiejOTPKapJ2F0tIqAyJHFug== ------END CERTIFICATE-----` - -var testECCert string = `-----BEGIN CERTIFICATE----- -MIICZDCCAeugAwIBAgIJALM9NbK8WRuBMAkGByqGSM49BAEwRTELMAkGA1UEBhMC -dXMxEzARBgNVBAgTClNvbWUtU3RhdGUxITAfBgNVBAoTGEludGVybmV0IFdpZGdp -dHMgUHR5IEx0ZDAeFw0xNzA5MTExNzQ2NDNaFw0yNzA5MDkxNzQ2NDNaMEUxCzAJ -BgNVBAYTAnVzMRMwEQYDVQQIEwpTb21lLVN0YXRlMSEwHwYDVQQKExhJbnRlcm5l -dCBXaWRnaXRzIFB0eSBMdGQwdjAQBgcqhkjOPQIBBgUrgQQAIgNiAATcqsBLxKP+ -UHk7Y6ktGGFvfrIfIXHxeZe3Xwt691CWfdmJFvrGzyzW5/AbJIuO1utdOsqUStAm -W/Scfxop/FGadKqR4nAWLNBI4intgnf0r1rzBCSOmanolHqxQPqQ0UOjgacwgaQw -HQYDVR0OBBYEFHxh1pTd8ApEzg0gKMwwt01aA10TMHUGA1UdIwRuMGyAFHxh1pTd -8ApEzg0gKMwwt01aA10ToUmkRzBFMQswCQYDVQQGEwJ1czETMBEGA1UECBMKU29t -ZS1TdGF0ZTEhMB8GA1UEChMYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkggkAsz01 -srxZG4EwDAYDVR0TBAUwAwEB/zAJBgcqhkjOPQQBA2gAMGUCMCR+CvAoNBhqSe2M -4qWWD/9XX/0qmf0O442Qowcg5MWH1+mwl1s7ozinvbTPDPaYDwIxAM54qKhuL6xt -GxqJpa7Onn15Hu8zTsdzeYBqUUXA6wtn+Pa7197CgUkfty9yc2eeQw== ------END CERTIFICATE-----` - -var testCACert string = ` ------BEGIN CERTIFICATE----- -MIIC5zCCAc+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwptaW5p -a3ViZUNBMB4XDTE3MDgxMDIzMTQ1NVoXDTI3MDgwODIzMTQ1NVowFTETMBEGA1UE -AxMKbWluaWt1YmVDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAN8d -w2p/KXRkm+vzOO0eT1vYBWP7fKsnng9/g5nnXAJlt9NxpOSolRcyItm/04R0E1jx -jpgsdzkybc+QU5ZiszOYN833/D5hCNVAABVivpDd2P8wVKXN46cB99e24etUVBqG -5aR0Ku3IBsJjCN9efhF+XRCA2gy/KaXMdKJhHfdtc8hCr7G9+2wO2G58FLmIfEyH -owviOGt0BSnCtMpsA8ZgGQyfqgSd5u466aCv6oj0MyzsMnfS38niM53Rlv4IY6ol -taYbWXtCNndQ2S687qE0qTCxhE95Bm2Nfkqct4R1798sJz83xNv8hALvxr/vPK/J -2XkIm3oo3YKG4n/CHXcCAwEAAaNCMEAwDgYDVR0PAQH/BAQDAgKkMB0GA1UdJQQW -MBQGCCsGAQUFBwMCBggrBgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3 -DQEBCwUAA4IBAQCSkrhE1PczqeqXfRaWayJUbXWPwKFbszO0MhGB1zwnPZq39qjY -ySQiGvnjV3fP+N5CTQAwMNe79Xiw31fSoexgceCPJpraWrTOLdCv04SbGDBapMFM -aezBu9jzZm0CNt60jHXWXuHHVPFX6u7ZR8W+RiBvsT8GZ5U6sNs3aN3M9Vym06BL -aSphIw1v+hRlPfnrlJwUnQp158DRgkt/9ncTa/k88KoIoZAbulaiGB4zHxxkbura -GSlgpZzhHSrBDLuXf65GHwwGxSExhgY5AA/n8rumGVvE8IYohS9yg/jOG0xP2WQH -u/ABoYtOyseO+lgElA8R4PB9MtwgN6c/b0xH ------END CERTIFICATE-----` From dc905772047080ff04fc056171a0752a8d7bee76 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Wed, 7 Dec 2022 15:43:40 -0500 Subject: [PATCH 04/12] Always start the TLS config updater on init --- backend.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/backend.go b/backend.go index d525b69f..2ceb3554 100644 --- a/backend.go +++ b/backend.go @@ -144,6 +144,10 @@ func Backend() *kubeAuthBackend { // 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 { + if err := b.runTLSConfigUpdater(context.Background(), req.Storage, defaultHorizon); err != nil { + return err + } + config, err := b.config(ctx, req.Storage) if err != nil { return err @@ -155,7 +159,7 @@ func (b *kubeAuthBackend) initialize(ctx context.Context, req *logical.Initializ } } - return b.runTLSConfigUpdater(context.Background(), req.Storage, defaultHorizon) + return nil } // runTLSConfigUpdater sets up a routine that periodically calls b.updateTLSConfig(). This ensures that the @@ -171,7 +175,7 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto return fmt.Errorf("update horizon must be equal to or greater than %s", defaultMinHorizon) } - updateTLSConfig := func(ctx context.Context, s logical.Storage, force bool) error { + 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) @@ -207,14 +211,14 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto b.Logger().Trace("Shutting down TLS config updater") return case <-ticker.C: - err = updateTLSConfig(ctx, s, false) + err = updateTLSConfig(ctx, s) case sig := <-sigs: b.Logger().Trace(fmt.Sprintf("Caught signal %v", sig)) switch sig { case syscall.SIGHUP: // update the TLS configuration when the plugin process receives a SIGHUP b.Logger().Trace(fmt.Sprintf("Calling updateTLSConfig() on signal %v", sig)) - err = updateTLSConfig(ctx, s, true) + err = updateTLSConfig(ctx, s) default: // shutdown on all other signals b.Logger().Trace(fmt.Sprintf("Calling cancel() on signal %v", sig)) @@ -223,7 +227,8 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto } if err != nil { - b.Logger().Warn("TLSConfig update failed, retrying", "horizon", defaultHorizon.String(), "err", err) + b.Logger().Warn("TLSConfig update failed, retrying", + "horizon", defaultHorizon.String(), "err", err) } } }(wCtx, cancel, s) From c9cfbdc8eeaac1bd951cb6531f59ba34880df7db Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Fri, 9 Dec 2022 10:23:46 -0500 Subject: [PATCH 05/12] Drop signal handling Further clarify the docs related to the horizon vars. --- backend.go | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/backend.go b/backend.go index 2ceb3554..a46bd7d5 100644 --- a/backend.go +++ b/backend.go @@ -8,11 +8,8 @@ import ( "encoding/json" "fmt" "net/http" - "os" - "os/signal" "strings" "sync" - "syscall" "time" "github.com/hashicorp/go-cleanhttp" @@ -49,10 +46,12 @@ var ( // CA cert can be used, before reading it again from disk. caReloadPeriod = 1 * time.Hour - // defaultHorizon for the tlsConfigUpdater. + // defaultHorizon provides the default duration to be used + // in the tlsConfigUpdater's time.Ticker, setup in runTLSConfigUpdater() defaultHorizon = time.Second * 30 - // defaultMinHorizon for the tlsConfigUpdater. + // defaultMinHorizon provides the minimum duration that can be specified + // in the tlsConfigUpdater's time.Ticker, setup in runTLSConfigUpdater() defaultMinHorizon = time.Second * 5 ) @@ -196,9 +195,6 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto ticker := time.NewTicker(horizon) wCtx, cancel := context.WithCancel(ctx) go func(ctx context.Context, cancel context.CancelFunc, s logical.Storage) { - sigs := make(chan os.Signal, 1) - signal.Notify(sigs, syscall.SIGHUP, syscall.SIGTERM, syscall.SIGINT) - defer signal.Stop(sigs) defer func() { b.tlsConfigUpdaterRunning = false }() @@ -212,18 +208,6 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto return case <-ticker.C: err = updateTLSConfig(ctx, s) - case sig := <-sigs: - b.Logger().Trace(fmt.Sprintf("Caught signal %v", sig)) - switch sig { - case syscall.SIGHUP: - // update the TLS configuration when the plugin process receives a SIGHUP - b.Logger().Trace(fmt.Sprintf("Calling updateTLSConfig() on signal %v", sig)) - err = updateTLSConfig(ctx, s) - default: - // shutdown on all other signals - b.Logger().Trace(fmt.Sprintf("Calling cancel() on signal %v", sig)) - cancel() - } } if err != nil { From 783a31ab1976dcc864432255c1a6bc7fb56df707 Mon Sep 17 00:00:00 2001 From: Ben Ash <32777270+benashz@users.noreply.github.com> Date: Tue, 13 Dec 2022 11:10:22 -0500 Subject: [PATCH 06/12] Update backend.go Co-authored-by: Tom Proctor --- backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend.go b/backend.go index a46bd7d5..f994b6aa 100644 --- a/backend.go +++ b/backend.go @@ -356,7 +356,7 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { return fmt.Errorf("the backend's http.Client has not been initialized") } - // attempt to read the CA certificates the config directly or from the filesystem. + // attempt to read the CA certificates from the config directly or from the filesystem. var caCertBytes []byte if config.CACert != "" { caCertBytes = []byte(config.CACert) From 9b2f28f2c983c61bc0da1107cc51457512a99dc6 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Tue, 10 Jan 2023 18:19:57 -0500 Subject: [PATCH 07/12] Post review fixups --- backend.go | 61 +++++++++++++++++++++++++++++++++++++-------------- path_login.go | 1 + 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/backend.go b/backend.go index f994b6aa..0335b19d 100644 --- a/backend.go +++ b/backend.go @@ -66,7 +66,7 @@ type kubeAuthBackend struct { 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 @@ -86,6 +86,9 @@ type kubeAuthBackend struct { // 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 } @@ -136,6 +139,7 @@ func Backend() *kubeAuthBackend { pathsRole(b), ), InitializeFunc: b.initialize, + Clean: b.cleanup, } return b @@ -143,10 +147,14 @@ func Backend() *kubeAuthBackend { // 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 { - if err := b.runTLSConfigUpdater(context.Background(), req.Storage, defaultHorizon); err != nil { + 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 @@ -161,11 +169,16 @@ func (b *kubeAuthBackend) initialize(ctx context.Context, req *logical.Initializ return nil } +func (b *kubeAuthBackend) cleanup(_ context.Context) { + b.shutdownTLSConfigUpdater() +} + // 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.l.Lock() defer b.l.Unlock() + if b.tlsConfigUpdaterRunning { return nil } @@ -193,18 +206,23 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto } ticker := time.NewTicker(horizon) - wCtx, cancel := context.WithCancel(ctx) - go func(ctx context.Context, cancel context.CancelFunc, s logical.Storage) { + go func(ctx context.Context, s logical.Storage) { defer func() { + b.l.Lock() + defer b.l.Unlock() + ticker.Stop() b.tlsConfigUpdaterRunning = false + b.tlsConfigUpdateCancelFunc = nil + b.Logger().Trace("TLSConfig updater shutdown completed") }() - b.Logger().Trace("Starting TLS config updater", "horizon", horizon) + b.Logger().Trace("TLSConfig updater starting", "horizon", horizon) + b.tlsConfigUpdaterRunning = true + var err error for { - var err error select { case <-ctx.Done(): - b.Logger().Trace("Shutting down TLS config updater") + b.Logger().Trace("TLSConfig updater shutting down") return case <-ticker.C: err = updateTLSConfig(ctx, s) @@ -215,11 +233,27 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto "horizon", defaultHorizon.String(), "err", err) } } - }(wCtx, cancel, s) + }(ctx, s) - b.tlsConfigUpdaterRunning = true + // wait for the updater to start since we hold the "start" lock + delay := time.Millisecond * 50 + var elapsed time.Duration + for elapsed < delay*10 { + if b.tlsConfigUpdaterRunning { + return nil + } - return nil + time.Sleep(delay) + elapsed += delay + } + return fmt.Errorf("TLSConfig updater failed to start after %s", elapsed) +} + +func (b *kubeAuthBackend) shutdownTLSConfigUpdater() { + if b.tlsConfigUpdateCancelFunc != nil { + b.Logger().Debug("TLSConfig updater shutdown requested") + b.tlsConfigUpdateCancelFunc() + } } // config takes a storage object and returns a kubeConfig object. @@ -335,12 +369,7 @@ func (b *kubeAuthBackend) getHTTPClient(config *kubeConfig) (*http.Client, error } if b.tlsConfig == nil { - // ensure that HTTP client's transport TLS configuration is initialized - // this adds some belt-and-suspenders, - // since in most cases the TLS configuration would have already been initialized. - if err := b.updateTLSConfig(config); err != nil { - return nil, err - } + return nil, fmt.Errorf("the backend's TLSConfig has not been initialized") } return b.httpClient, nil diff --git a/path_login.go b/path_login.go index 672daa83..7e3d714c 100644 --- a/path_login.go +++ b/path_login.go @@ -131,6 +131,7 @@ func (b *kubeAuthBackend) pathLogin(ctx context.Context, req *logical.Request, d client, err := b.getHTTPClient(config) if err != nil { + b.Logger().Error(`Failed to get the HTTP client`, "err", err) return nil, logical.ErrUnrecoverable } From d80a8ce99a9879cc52e2181c92ed483b95fc26a9 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Wed, 11 Jan 2023 10:34:01 -0500 Subject: [PATCH 08/12] Add separate mutex for TLSConfig update operations --- backend.go | 19 +++++++++++-------- path_config.go | 3 +++ path_login.go | 2 +- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/backend.go b/backend.go index 0335b19d..899e4bf9 100644 --- a/backend.go +++ b/backend.go @@ -90,6 +90,9 @@ type kubeAuthBackend struct { 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. @@ -176,8 +179,8 @@ func (b *kubeAuthBackend) cleanup(_ context.Context) { // 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.l.Lock() - defer b.l.Unlock() + b.tlsMu.Lock() + defer b.tlsMu.Unlock() if b.tlsConfigUpdaterRunning { return nil @@ -208,11 +211,10 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto ticker := time.NewTicker(horizon) go func(ctx context.Context, s logical.Storage) { defer func() { - b.l.Lock() - defer b.l.Unlock() + b.tlsMu.Lock() + defer b.tlsMu.Unlock() ticker.Stop() b.tlsConfigUpdaterRunning = false - b.tlsConfigUpdateCancelFunc = nil b.Logger().Trace("TLSConfig updater shutdown completed") }() @@ -253,6 +255,7 @@ func (b *kubeAuthBackend) shutdownTLSConfigUpdater() { if b.tlsConfigUpdateCancelFunc != nil { b.Logger().Debug("TLSConfig updater shutdown requested") b.tlsConfigUpdateCancelFunc() + b.tlsConfigUpdateCancelFunc = nil } } @@ -363,7 +366,7 @@ func (b *kubeAuthBackend) role(ctx context.Context, s logical.Storage, name stri } // getHTTPClient return the backend's HTTP client for connecting to the Kubernetes API. -func (b *kubeAuthBackend) getHTTPClient(config *kubeConfig) (*http.Client, error) { +func (b *kubeAuthBackend) getHTTPClient() (*http.Client, error) { if b.httpClient == nil { return nil, fmt.Errorf("the backend's http.Client has not been initialized") } @@ -378,8 +381,8 @@ func (b *kubeAuthBackend) getHTTPClient(config *kubeConfig) (*http.Client, error // updateTLSConfig ensures that the httpClient's TLS configuration is consistent // with the backend's stored configuration. func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { - b.l.Lock() - defer b.l.Unlock() + b.tlsMu.Lock() + defer b.tlsMu.Unlock() if b.httpClient == nil { return fmt.Errorf("the backend's http.Client has not been initialized") diff --git a/path_config.go b/path_config.go index c9760801..40af5270 100644 --- a/path_config.go +++ b/path_config.go @@ -120,6 +120,9 @@ func (b *kubeAuthBackend) pathConfigRead(ctx context.Context, req *logical.Reque // pathConfigWrite handles create and update commands to the config func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + b.l.Lock() + defer b.l.Unlock() + host := data.Get("kubernetes_host").(string) if host == "" { return logical.ErrorResponse("no host provided"), nil diff --git a/path_login.go b/path_login.go index 7e3d714c..819020cc 100644 --- a/path_login.go +++ b/path_login.go @@ -129,7 +129,7 @@ func (b *kubeAuthBackend) pathLogin(ctx context.Context, req *logical.Request, d return nil, err } - client, err := b.getHTTPClient(config) + client, err := b.getHTTPClient() if err != nil { b.Logger().Error(`Failed to get the HTTP client`, "err", err) return nil, logical.ErrUnrecoverable From c0f88f58daedef0e421cd5a5faf0732242de9c8d Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Wed, 11 Jan 2023 11:40:11 -0500 Subject: [PATCH 09/12] Sleep longer between runTLSConfigUpdater tests --- Makefile | 9 +++++---- backend_test.go | 6 +++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Makefile b/Makefile index fba09fc3..1487963e 100644 --- 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=1 go test -race $(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: diff --git a/backend_test.go b/backend_test.go index 20cd560d..3888ddb5 100644 --- a/backend_test.go +++ b/backend_test.go @@ -420,7 +420,7 @@ func Test_kubeAuthBackend_runTLSConfigUpdater(t *testing.T) { } configCount := len(tt.configs) - ctx, cancel := context.WithTimeout(tt.ctx, tt.horizon*time.Duration(configCount+1)) + ctx, cancel := context.WithTimeout(tt.ctx, tt.horizon*time.Duration(configCount*2)) defer cancel() err := b.runTLSConfigUpdater(ctx, tt.storage, tt.horizon) if tt.wantErr && err == nil { @@ -455,9 +455,9 @@ func Test_kubeAuthBackend_runTLSConfigUpdater(t *testing.T) { } } - time.Sleep(tt.horizon * 2) + time.Sleep(tt.horizon * 3) if b.tlsConfig == nil { - t.Fatalf("runTLSConfigUpdater(), ") + t.Fatalf("runTLSConfigUpdater(), expected tlsConfig initialization") } assertTLSConfigEquals(t, b.tlsConfig, config.expectTLSConfig) assertValidTransport(t, b, config.expectTLSConfig) From 84d9b4ab722b5d7c8385d94481e4e2cc00bea045 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Wed, 11 Jan 2023 11:42:42 -0500 Subject: [PATCH 10/12] Remove -race flag for tests --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 1487963e..de7c4b44 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ dev: .PHONY: test test: fmtcheck - CGO_ENABLED=1 go test -race $(TESTARGS) -timeout=20m ./... + CGO_ENABLED=0 go test $(TESTARGS) -timeout=20m ./... .PHONY: integration-test integration-test: From 89ae775c1a646e242358166315c32efd4c86656a Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 12 Jan 2023 15:10:08 -0500 Subject: [PATCH 11/12] Post review updates --- backend.go | 85 ++++++++++++++++++++++++++--------------------- backend_test.go | 32 +++++++++++++----- path_role_test.go | 8 +++-- 3 files changed, 75 insertions(+), 50 deletions(-) diff --git a/backend.go b/backend.go index 899e4bf9..c0a9118f 100644 --- a/backend.go +++ b/backend.go @@ -6,6 +6,7 @@ import ( "crypto/tls" "crypto/x509" "encoding/json" + "errors" "fmt" "net/http" "strings" @@ -53,6 +54,9 @@ var ( // 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 @@ -98,15 +102,17 @@ type kubeAuthBackend struct { // 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 defaultTLSConfig() *tls.Config { +func getDefaultTLSConfig() *tls.Config { return &tls.Config{ MinVersion: minTLSVersion, } @@ -118,6 +124,8 @@ func Backend() *kubeAuthBackend { 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, } @@ -176,6 +184,18 @@ 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 { @@ -190,6 +210,10 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto 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 { @@ -208,6 +232,8 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto return nil } + var wg sync.WaitGroup + wg.Add(1) ticker := time.NewTicker(horizon) go func(ctx context.Context, s logical.Storage) { defer func() { @@ -220,35 +246,23 @@ func (b *kubeAuthBackend) runTLSConfigUpdater(ctx context.Context, s logical.Sto b.Logger().Trace("TLSConfig updater starting", "horizon", horizon) b.tlsConfigUpdaterRunning = true - var err error + wg.Done() for { select { case <-ctx.Done(): b.Logger().Trace("TLSConfig updater shutting down") return case <-ticker.C: - err = updateTLSConfig(ctx, s) - } - - if err != nil { - b.Logger().Warn("TLSConfig update failed, retrying", - "horizon", defaultHorizon.String(), "err", err) + if err := updateTLSConfig(ctx, s); err != nil { + b.Logger().Warn("TLSConfig update failed, retrying", + "horizon", defaultHorizon.String(), "err", err) + } } } }(ctx, s) + wg.Wait() - // wait for the updater to start since we hold the "start" lock - delay := time.Millisecond * 50 - var elapsed time.Duration - for elapsed < delay*10 { - if b.tlsConfigUpdaterRunning { - return nil - } - - time.Sleep(delay) - elapsed += delay - } - return fmt.Errorf("TLSConfig updater failed to start after %s", elapsed) + return nil } func (b *kubeAuthBackend) shutdownTLSConfigUpdater() { @@ -367,12 +381,11 @@ func (b *kubeAuthBackend) role(ctx context.Context, s logical.Storage, name stri // getHTTPClient return the backend's HTTP client for connecting to the Kubernetes API. func (b *kubeAuthBackend) getHTTPClient() (*http.Client, error) { - if b.httpClient == nil { - return nil, fmt.Errorf("the backend's http.Client has not been initialized") - } + b.tlsMu.RLock() + defer b.tlsMu.RUnlock() - if b.tlsConfig == nil { - return nil, fmt.Errorf("the backend's TLSConfig has not been initialized") + if err := b.validateHTTPClientInit(); err != nil { + return nil, err } return b.httpClient, nil @@ -384,8 +397,9 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { b.tlsMu.Lock() defer b.tlsMu.Unlock() - if b.httpClient == nil { - return fmt.Errorf("the backend's http.Client has not been initialized") + // ensure that the + if err := b.validateHTTPClientInit(); err != nil { + return err } // attempt to read the CA certificates from the config directly or from the filesystem. @@ -393,7 +407,6 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { if config.CACert != "" { caCertBytes = []byte(config.CACert) } else if !config.DisableLocalCAJwt && b.localCACertReader != nil { - // TODO: this may block on I/O, investigate a proper mitigation data, err := b.localCACertReader.ReadFile() if err != nil { return err @@ -401,16 +414,6 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { caCertBytes = []byte(data) } - transport, ok := b.httpClient.Transport.(*http.Transport) - if !ok { - // should never happen - return fmt.Errorf("type assertion failed for %T", b.httpClient.Transport) - } - - if b.tlsConfig == nil { - b.tlsConfig = defaultTLSConfig() - } - certPool := x509.NewCertPool() if len(caCertBytes) > 0 { if ok := certPool.AppendCertsFromPEM(caCertBytes); !ok { @@ -426,6 +429,12 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { // 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 { diff --git a/backend_test.go b/backend_test.go index 3888ddb5..f4f0f0de 100644 --- a/backend_test.go +++ b/backend_test.go @@ -4,7 +4,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "errors" "fmt" "net/http" "os" @@ -33,22 +32,34 @@ func Test_kubeAuthBackend_updateTLSConfig(t *testing.T) { tests := []struct { name string httpClient *http.Client + tlsConfig *tls.Config wantErr bool configs []testConfig }{ { - name: "fail-client-not-initialized", + name: "fail-client-not-set", httpClient: nil, configs: []testConfig{ { wantErr: true, - expectError: errors.New("the backend's http.Client has not been initialized"), + expectError: errHTTPClientNotSet, + }, + }, + }, + { + name: "fail-tlsConfig-not-set", + httpClient: getDefaultHTTPClient(), + configs: []testConfig{ + { + wantErr: true, + expectError: errTLSConfigNotSet, }, }, }, { name: "ca-certs-from-config-source", httpClient: getDefaultHTTPClient(), + tlsConfig: getDefaultTLSConfig(), wantErr: false, configs: []testConfig{ { @@ -86,6 +97,7 @@ func Test_kubeAuthBackend_updateTLSConfig(t *testing.T) { { name: "ca-certs-from-file-source", httpClient: getDefaultHTTPClient(), + tlsConfig: getDefaultTLSConfig(), configs: []testConfig{ { config: &kubeConfig{ @@ -113,6 +125,7 @@ func Test_kubeAuthBackend_updateTLSConfig(t *testing.T) { { name: "ca-certs-mixed-source", httpClient: getDefaultHTTPClient(), + tlsConfig: getDefaultTLSConfig(), configs: []testConfig{ { config: &kubeConfig{ @@ -163,6 +176,7 @@ func Test_kubeAuthBackend_updateTLSConfig(t *testing.T) { b := &kubeAuthBackend{ Backend: &framework.Backend{}, httpClient: tt.httpClient, + tlsConfig: tt.tlsConfig, } if err := b.Setup(context.Background(), @@ -227,9 +241,10 @@ func Test_kubeAuthBackend_initialize(t *testing.T) { expectErr error }{ { - name: "fail-client-not-initialized", + name: "fail-client-not-set", ctx: context.Background(), httpClient: nil, + tlsConfig: getDefaultTLSConfig(), req: &logical.InitializationRequest{ Storage: &logical.InmemStorage{}, }, @@ -238,12 +253,13 @@ func Test_kubeAuthBackend_initialize(t *testing.T) { DisableLocalCAJwt: false, }, wantErr: true, - expectErr: errors.New("the backend's http.Client has not been initialized"), + expectErr: errHTTPClientNotSet, }, { name: "no-config", ctx: context.Background(), httpClient: getDefaultHTTPClient(), + tlsConfig: getDefaultTLSConfig(), req: &logical.InitializationRequest{ Storage: &logical.InmemStorage{}, }, @@ -254,6 +270,7 @@ func Test_kubeAuthBackend_initialize(t *testing.T) { name: "initialized-from-config", ctx: context.Background(), httpClient: getDefaultHTTPClient(), + tlsConfig: getDefaultTLSConfig(), req: &logical.InitializationRequest{ Storage: &logical.InmemStorage{}, }, @@ -318,10 +335,6 @@ func Test_kubeAuthBackend_initialize(t *testing.T) { if tt.config != nil { assertTLSConfigEquals(t, b.tlsConfig, tt.expectTLSConfig) assertValidTransport(t, b, tt.expectTLSConfig) - } else { - if b.tlsConfig != nil { - t.Errorf("initialize(), unexpected tlsConfig initialization") - } } if !b.tlsConfigUpdaterRunning { @@ -353,6 +366,7 @@ func Test_kubeAuthBackend_runTLSConfigUpdater(t *testing.T) { }{ { name: "initialized-from-config", + tlsConfig: getDefaultTLSConfig(), ctx: context.Background(), storage: &logical.InmemStorage{}, horizon: time.Millisecond * 500, diff --git a/path_role_test.go b/path_role_test.go index 3b63113c..317b34c6 100644 --- a/path_role_test.go +++ b/path_role_test.go @@ -19,6 +19,9 @@ func getBackend(t *testing.T) (logical.Backend, logical.Storage) { defaultLeaseTTLVal := time.Hour * 12 maxLeaseTTLVal := time.Hour * 24 b := Backend() + if err := b.validateHTTPClientInit(); err != nil { + t.Fatalf("unable to create backend: %v", err) + } config := &logical.BackendConfig{ Logger: logging.NewVaultLogger(log.Trace), @@ -29,9 +32,8 @@ func getBackend(t *testing.T) (logical.Backend, logical.Storage) { }, StorageView: &logical.InmemStorage{}, } - err := b.Setup(context.Background(), config) - if err != nil { - t.Fatalf("unable to create backend: %v", err) + if err := b.Setup(context.Background(), config); err != nil { + t.Fatalf("unable to setup backend: %v", err) } return b, config.StorageView From 7afaf40ee0f5aa2089ff3f1e37b11c45bc7ec6c2 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 12 Jan 2023 15:29:52 -0500 Subject: [PATCH 12/12] Remove extraneous comment --- backend.go | 1 - 1 file changed, 1 deletion(-) diff --git a/backend.go b/backend.go index c0a9118f..da8284c2 100644 --- a/backend.go +++ b/backend.go @@ -397,7 +397,6 @@ func (b *kubeAuthBackend) updateTLSConfig(config *kubeConfig) error { b.tlsMu.Lock() defer b.tlsMu.Unlock() - // ensure that the if err := b.validateHTTPClientInit(); err != nil { return err }