From 64a70483d80133cefb5cb373d300122589400b30 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Thu, 27 May 2021 15:22:11 -0500 Subject: [PATCH 1/7] connect/ca: require new vault mount points when updating the key type/bits for the vault connect CA provider progress on #9572 --- agent/connect/ca/provider_vault.go | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index caab8e4831b5..fab911042812 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -163,7 +163,7 @@ func (v *VaultProvider) GenerateRoot() error { } // Set up the root PKI backend if necessary. - _, err := v.ActiveRoot() + rootPEM, err := v.ActiveRoot() switch err { case ErrBackendNotMounted: err := v.client.Sys().Mount(v.config.RootPKIPath, &vaultapi.MountInput{ @@ -197,6 +197,31 @@ func (v *VaultProvider) GenerateRoot() error { if err != nil { return err } + + if rootPEM != "" { + rootCert, err := connect.ParseCert(rootPEM) + if err != nil { + return err + } + + // Vault PKI doesn't allow in-place cert/key regeneration. That + // means if you need to change either the key type or key bits then + // you also need to provide new mount points. + // https://www.vaultproject.io/api-docs/secret/pki#generate-root + // + // A separate bug in vault likely also requires that you use the + // ForceWithoutCrossSigning option when changing key types. + foundKeyType, foundKeyBits, err := connect.KeyInfoFromCert(rootCert) + if err != nil { + return err + } + if v.config.PrivateKeyType != foundKeyType { + return fmt.Errorf("cannot update the PrivateKeyType field without choosing a new PKI mount for the root CA") + } + if v.config.PrivateKeyBits != foundKeyBits { + return fmt.Errorf("cannot update the PrivateKeyBits field without choosing a new PKI mount for the root CA") + } + } } return nil From 96e71255b405a7ac97f699cd3dd90edf364eba79 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Wed, 30 Jun 2021 16:30:52 -0500 Subject: [PATCH 2/7] adding a heavyweight test --- GNUmakefile | 4 +- agent/consul/connect_ca_endpoint_test.go | 109 +++++++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/GNUmakefile b/GNUmakefile index 891996b0fb20..54e4514128f7 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -360,14 +360,14 @@ ifeq ("$(CIRCLECI)","true") # Run in CI gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report.xml" -- -cover -coverprofile=coverage.txt ./agent/connect/ca # Run leader tests that require Vault - gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-leader.xml" -- -cover -coverprofile=coverage-leader.txt -run TestLeader_Vault_ ./agent/consul + gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-leader.xml" -- -cover -coverprofile=coverage-leader.txt -run '.*_Vault_' ./agent/consul # Run agent tests that require Vault gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-agent.xml" -- -cover -coverprofile=coverage-agent.txt -run '.*_Vault_' ./agent else # Run locally @echo "Running /agent/connect/ca tests in verbose mode" @go test -v ./agent/connect/ca - @go test -v ./agent/consul -run 'TestLeader_Vault_' + @go test -v ./agent/consul -run '.*_Vault_' @go test -v ./agent -run '.*_Vault_' endif diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index 0bd2e3fad97a..58ed26de51a3 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -17,6 +17,7 @@ import ( "github.com/hashicorp/consul/agent/connect" ca "github.com/hashicorp/consul/agent/connect/ca" "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" ) @@ -511,6 +512,114 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { } } +func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + + ca.SkipIfVaultNotPresent(t) + + t.Parallel() + + testVault := ca.NewTestVaultServer(t) + defer testVault.Stop() + + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.Build = "1.6.0" + c.PrimaryDatacenter = "dc1" + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": testVault.Addr, + "Token": testVault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + }, + } + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForTestAgent(t, s1.RPC, "dc1") + + // Capture the current root. + // var originalRoot *structs.CARoot + { + rootList, _, err := getTestRoots(s1, "dc1") + require.NoError(t, err) + require.Len(t, rootList.Roots, 1) + // originalRoot = activeRoot + } + + cases := []struct { + name string + configFn func() (*structs.CAConfiguration, error) + expectErr string + }{ + { + name: "cannot edit key bits", + configFn: func() (*structs.CAConfiguration, error) { + return &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": testVault.Addr, + "Token": testVault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + // + "PrivateKeyType": "ec", + "PrivateKeyBits": 384, + }, + ForceWithoutCrossSigning: true, + }, nil + }, + expectErr: `error generating CA root certificate: cannot update the PrivateKeyBits field without choosing a new PKI mount for the root CA`, + }, + { + name: "cannot edit key type", + configFn: func() (*structs.CAConfiguration, error) { + return &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": testVault.Addr, + "Token": testVault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + // + "PrivateKeyType": "rsa", + "PrivateKeyBits": 4096, + }, + ForceWithoutCrossSigning: true, + }, nil + }, + expectErr: `error generating CA root certificate: cannot update the PrivateKeyType field without choosing a new PKI mount for the root CA`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + newConfig, err := tc.configFn() + require.NoError(t, err) + + args := &structs.CARequest{ + Datacenter: "dc1", + Config: newConfig, + } + var reply interface{} + + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply) + if tc.expectErr == "" { + require.NoError(t, err) + } else { + testutil.RequireErrorContains(t, err, tc.expectErr) + } + }) + } +} + func TestConnectCAConfig_UpdateSecondary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") From db5299e8d37d23b41bcd9f72b672517e00d0bda8 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Wed, 30 Jun 2021 16:31:48 -0500 Subject: [PATCH 3/7] changelog --- .changelog/10331.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/10331.txt diff --git a/.changelog/10331.txt b/.changelog/10331.txt new file mode 100644 index 000000000000..b312e195d2d0 --- /dev/null +++ b/.changelog/10331.txt @@ -0,0 +1,3 @@ +```release-note:bug +connect/ca: require new vault mount points when updating the key type/bits for the vault connect CA provider +``` From e39672bdb76c3823b7e245e92e0ca04a220d18bf Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Tue, 13 Jul 2021 10:27:49 -0500 Subject: [PATCH 4/7] Update agent/consul/connect_ca_endpoint_test.go Co-authored-by: Daniel Nephin --- agent/consul/connect_ca_endpoint_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index 58ed26de51a3..894da311c4d8 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -546,7 +546,6 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { testrpc.WaitForTestAgent(t, s1.RPC, "dc1") // Capture the current root. - // var originalRoot *structs.CARoot { rootList, _, err := getTestRoots(s1, "dc1") require.NoError(t, err) From 2b2272bf443290f8342bd386097bb3bfd44a89be Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Tue, 13 Jul 2021 10:27:58 -0500 Subject: [PATCH 5/7] Update agent/consul/connect_ca_endpoint_test.go Co-authored-by: Daniel Nephin --- agent/consul/connect_ca_endpoint_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index 894da311c4d8..7b3daee92762 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -550,7 +550,6 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { rootList, _, err := getTestRoots(s1, "dc1") require.NoError(t, err) require.Len(t, rootList.Roots, 1) - // originalRoot = activeRoot } cases := []struct { From b3f10b45112ed3a764d5ab0e0a4e62b7d0add495 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Tue, 13 Jul 2021 10:28:06 -0500 Subject: [PATCH 6/7] Update agent/consul/connect_ca_endpoint_test.go Co-authored-by: Daniel Nephin --- agent/consul/connect_ca_endpoint_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index 7b3daee92762..6736f74c1a52 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -537,7 +537,6 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { }, } }) - defer os.RemoveAll(dir1) defer s1.Shutdown() codec := rpcClient(t, s1) From 5098e7bea4a43c3e13482ad7f9fc9670ea50fd14 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Tue, 13 Jul 2021 10:46:12 -0500 Subject: [PATCH 7/7] unused var --- agent/consul/connect_ca_endpoint_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index 6736f74c1a52..b248f8085df9 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -524,7 +524,7 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { testVault := ca.NewTestVaultServer(t) defer testVault.Stop() - dir1, s1 := testServerWithConfig(t, func(c *Config) { + _, s1 := testServerWithConfig(t, func(c *Config) { c.Build = "1.6.0" c.PrimaryDatacenter = "dc1" c.CAConfig = &structs.CAConfiguration{