From d638378d57ddb47f827b0281e5e55560397487db Mon Sep 17 00:00:00 2001 From: Michael Gaffney Date: Wed, 1 Nov 2023 15:59:31 -0400 Subject: [PATCH] fix(vault): Set the Vault TLS client options if specified Two Vault client settings were not being properly used when constructing a Vault client. The `TLS Skip Verify` setting was only being set if a `CA Cert` was also configured. This fix sets the `TLS Skip Verify` when configured regardless of other settings. Closes #3961. The `TLS Server Name` setting was never being set. Bad programmers. This fix now sets it on the Vault client if the Vault Credential Store has been configured to use a value for this setting. Closes #3962. --- internal/credential/vault/supported.go | 4 +- internal/credential/vault/testing.go | 83 ++++++++++++++--------- internal/credential/vault/testing_test.go | 64 +++++++++++++++-- internal/credential/vault/vault.go | 8 ++- 4 files changed, 118 insertions(+), 41 deletions(-) diff --git a/internal/credential/vault/supported.go b/internal/credential/vault/supported.go index 8d47213b08..e4a81def50 100644 --- a/internal/credential/vault/supported.go +++ b/internal/credential/vault/supported.go @@ -112,7 +112,7 @@ func gotNewServer(t testing.TB, opt ...TestOption) *TestVaultServer { dockerOptions.Env = append(dockerOptions.Env, fmt.Sprintf("VAULT_LOCAL_CONFIG=%s", clientTlsTemplate)) } - serverCert := testServerCert(t, testCaCert(t), "localhost") + serverCert := testServerCert(t, testCaCert(t), opt...) server.serverCertBundle = serverCert server.ServerCert = serverCert.Cert.Cert server.CaCert = serverCert.CA.Cert @@ -147,6 +147,8 @@ func gotNewServer(t testing.TB, opt ...TestOption) *TestVaultServer { return &vaultClientCert, nil } } + server.TlsSkipVerify = true + clientTLSConfig.InsecureSkipVerify = true } // NOTE(mgaffney) 05/2021: creating a docker network is not the default diff --git a/internal/credential/vault/testing.go b/internal/credential/vault/testing.go index 67b6461642..fdd4641e69 100644 --- a/internal/credential/vault/testing.go +++ b/internal/credential/vault/testing.go @@ -368,7 +368,7 @@ func testCaCert(t testing.TB) *testCert { } // testServerCert will generate a test x509 server cert. -func testServerCert(t testing.TB, ca *testCert, hosts ...string) *testCertBundle { +func testServerCert(t testing.TB, ca *testCert, opt ...TestOption) *testCertBundle { t.Helper() require := require.New(t) @@ -404,7 +404,8 @@ func testServerCert(t testing.TB, ca *testCert, hosts ...string) *testCertBundle BasicConstraintsValid: true, } - for _, h := range hosts { + opts := getTestOpts(t, opt...) + for _, h := range opts.serverCertHostNames { if ip := net.ParseIP(h); ip != nil { template.IPAddresses = append(template.IPAddresses, ip) } else { @@ -512,19 +513,20 @@ type TestOption func(testing.TB, *testOptions) // options = how options are represented type testOptions struct { - orphan bool - periodic bool - renewable bool - policies []string - allowedExtensions []string - mountPath string - roleName string - vaultTLS TestVaultTLS - dockerNetwork bool - skipCleanup bool - tokenPeriod time.Duration - clientKey *ecdsa.PrivateKey - vaultVersion string + orphan bool + periodic bool + renewable bool + policies []string + allowedExtensions []string + mountPath string + roleName string + vaultTLS TestVaultTLS + dockerNetwork bool + skipCleanup bool + tokenPeriod time.Duration + clientKey *ecdsa.PrivateKey + vaultVersion string + serverCertHostNames []string } func getDefaultTestOptions(t testing.TB) testOptions { @@ -537,15 +539,16 @@ func getDefaultTestOptions(t testing.TB) testOptions { } return testOptions{ - orphan: true, - periodic: true, - renewable: true, - policies: []string{"default", "boundary-controller"}, - mountPath: "", - roleName: "boundary", - vaultTLS: TestNoTLS, - dockerNetwork: false, - tokenPeriod: defaultPeriod, + orphan: true, + periodic: true, + renewable: true, + policies: []string{"default", "boundary-controller"}, + mountPath: "", + roleName: "boundary", + vaultTLS: TestNoTLS, + dockerNetwork: false, + tokenPeriod: defaultPeriod, + serverCertHostNames: []string{"localhost"}, } } @@ -587,6 +590,16 @@ func WithTestVaultTLS(s TestVaultTLS) TestOption { } } +// WithServerCertHostNames sets the host names or IP address to attach to +// the test server's TLS certificate. The default host name attached to the +// test server's TLS certificate is 'localhost'. +func WithServerCertHostNames(h []string) TestOption { + return func(t testing.TB, o *testOptions) { + t.Helper() + o.serverCertHostNames = h + } +} + // WithClientKey sets the private key that will be used to generate the // client certificate. The option is only valid when used together with TestClientTLS. func WithClientKey(k *ecdsa.PrivateKey) TestOption { @@ -699,11 +712,13 @@ func (v *TestVaultServer) ClientUsingToken(t testing.TB, token string) *client { ctx := context.Background() require := require.New(t) conf := &clientConfig{ - Addr: v.Addr, - Token: TokenSecret(token), - CaCert: v.CaCert, - ClientCert: v.ClientCert, - ClientKey: v.ClientKey, + Addr: v.Addr, + Token: TokenSecret(token), + CaCert: v.CaCert, + ClientCert: v.ClientCert, + ClientKey: v.ClientKey, + TlsServerName: v.TlsServerName, + TlsSkipVerify: v.TlsSkipVerify, } client, err := newClient(ctx, conf) @@ -1008,10 +1023,12 @@ type TestVaultServer struct { RootToken string Addr string - CaCert []byte - ServerCert []byte - ClientCert []byte - ClientKey []byte + CaCert []byte + ServerCert []byte + ClientCert []byte + ClientKey []byte + TlsServerName string + TlsSkipVerify bool serverCertBundle *testCertBundle clientCertBundle *testCertBundle diff --git a/internal/credential/vault/testing_test.go b/internal/credential/vault/testing_test.go index e9e3e0d677..b8faec6569 100644 --- a/internal/credential/vault/testing_test.go +++ b/internal/credential/vault/testing_test.go @@ -267,6 +267,58 @@ func TestNewVaultServer(t *testing.T) { require.NotNil(client) require.NoError(client.ping(ctx)) }) + t.Run("TestServerTLS-InsecureSkipVerify", func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + v := NewTestVaultServer(t, WithTestVaultTLS(TestServerTLS), WithServerCertHostNames([]string{"kaz"})) + require.NotNil(v) + + assert.NotEmpty(v.RootToken) + assert.NotEmpty(v.Addr) + assert.NotEmpty(v.CaCert) + + conf := &clientConfig{ + Addr: v.Addr, + Token: TokenSecret(v.RootToken), + CaCert: v.CaCert, + } + + client, err := newClient(ctx, conf) + require.NoError(err) + require.NotNil(client) + require.Error(client.ping(ctx)) + + conf.TlsSkipVerify = true + client, err = newClient(ctx, conf) + require.NoError(err) + require.NotNil(client) + require.NoError(client.ping(ctx)) + }) + t.Run("TestServerTLS-TlsServerName", func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + v := NewTestVaultServer(t, WithTestVaultTLS(TestServerTLS), WithServerCertHostNames([]string{"kaz"})) + require.NotNil(v) + + assert.NotEmpty(v.RootToken) + assert.NotEmpty(v.Addr) + assert.NotEmpty(v.CaCert) + + conf := &clientConfig{ + Addr: v.Addr, + Token: TokenSecret(v.RootToken), + CaCert: v.CaCert, + } + + client, err := newClient(ctx, conf) + require.NoError(err) + require.NotNil(client) + require.Error(client.ping(ctx)) + + conf.TlsServerName = "kaz" + client, err = newClient(ctx, conf) + require.NoError(err) + require.NotNil(client) + require.NoError(client.ping(ctx)) + }) t.Run("TestClientTLS", func(t *testing.T) { assert, require := assert.New(t), require.New(t) v := NewTestVaultServer(t, WithTestVaultTLS(TestClientTLS)) @@ -311,11 +363,13 @@ func TestNewVaultServer(t *testing.T) { assert.Equal(pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: k}), v.ClientKey) conf := &clientConfig{ - Addr: v.Addr, - Token: TokenSecret(v.RootToken), - CaCert: v.CaCert, - ClientCert: v.ClientCert, - ClientKey: v.ClientKey, + Addr: v.Addr, + Token: TokenSecret(v.RootToken), + CaCert: v.CaCert, + TlsServerName: v.TlsServerName, + TlsSkipVerify: v.TlsSkipVerify, + ClientCert: v.ClientCert, + ClientKey: v.ClientKey, } client, err := newClient(ctx, conf) diff --git a/internal/credential/vault/vault.go b/internal/credential/vault/vault.go index e415835ffc..21305cfa95 100644 --- a/internal/credential/vault/vault.go +++ b/internal/credential/vault/vault.go @@ -80,12 +80,16 @@ func newClient(ctx context.Context, c *clientConfig) (*client, error) { } vc := vault.DefaultConfig() vc.Address = c.Addr + tlsConfig := vc.HttpClient.Transport.(*http.Transport).TLSClientConfig + tlsConfig.InsecureSkipVerify = c.TlsSkipVerify + if c.TlsServerName != "" { + tlsConfig.ServerName = c.TlsServerName + } + if len(c.CaCert) > 0 { rootConfig := &rootcerts.Config{ CACertificate: c.CaCert, } - tlsConfig := vc.HttpClient.Transport.(*http.Transport).TLSClientConfig - tlsConfig.InsecureSkipVerify = c.TlsSkipVerify if err := rootcerts.ConfigureTLS(tlsConfig, rootConfig); err != nil { return nil, errors.Wrap(ctx, err, op) }