From 0537922c6cbf6bdcbf0f21614466882e15c952ff Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Tue, 13 Jul 2021 11:11:46 -0500 Subject: [PATCH] connect/ca: require new vault mount points when updating the key type/bits for the vault connect CA provider (#10331) progress on #9572 --- .changelog/10331.txt | 3 + GNUmakefile | 4 +- agent/connect/ca/provider_vault.go | 27 +++++- agent/consul/connect_ca_endpoint_test.go | 106 +++++++++++++++++++++++ 4 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 .changelog/10331.txt diff --git a/.changelog/10331.txt b/.changelog/10331.txt new file mode 100644 index 000000000..b312e195d --- /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 +``` diff --git a/GNUmakefile b/GNUmakefile index 891996b0f..54e451412 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/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index caab8e483..fab911042 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 diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index 0bd2e3fad..b248f8085 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,111 @@ 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() + + _, 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 s1.Shutdown() + + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForTestAgent(t, s1.RPC, "dc1") + + // Capture the current root. + { + rootList, _, err := getTestRoots(s1, "dc1") + require.NoError(t, err) + require.Len(t, rootList.Roots, 1) + } + + 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")