From d707173253b995e616d4d98aeae2cd1041d737b0 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 3 Feb 2022 14:03:32 -0500 Subject: [PATCH 1/4] ca: small cleanup of TestConnectCAConfig_Vault_TriggerRotation_Fails Before adding more test cases --- agent/consul/connect_ca_endpoint_test.go | 33 ++++++------------------ 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index d170729db4ae..eba3f732395d 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -558,11 +558,8 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.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{}{ @@ -573,28 +570,16 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { }, } }) - 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) + configFn func() *structs.CAConfiguration expectErr string }{ { name: "cannot edit key bits", - configFn: func() (*structs.CAConfiguration, error) { + configFn: func() *structs.CAConfiguration { return &structs.CAConfiguration{ Provider: "vault", Config: map[string]interface{}{ @@ -607,13 +592,13 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { "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) { + configFn: func() *structs.CAConfiguration { return &structs.CAConfiguration{ Provider: "vault", Config: map[string]interface{}{ @@ -626,7 +611,7 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { "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`, }, @@ -634,16 +619,14 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { 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, + Config: tc.configFn(), } var reply interface{} - err = msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply) + codec := rpcClient(t, s1) + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply) if tc.expectErr == "" { require.NoError(t, err) } else { From 608597c7b6efc971796ac793a13019174ae69890 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 3 Feb 2022 13:28:43 -0500 Subject: [PATCH 2/4] ca: relax and move private key type/bit validation for vault This commit makes two changes to the validation. Previously we would call this validation in GenerateRoot, which happens both on initialization (when a follower becomes leader), and when a configuration is updated. We only want to do this validation during config update so the logic was moved to the UpdateConfiguration function. Previously we would compare the config values against the actual cert. This caused problems when the cert was created manually in Vault (not created by Consul). Now we compare the new config against the previous config. Using a already created CA cert should never error now. Adding the key bit and types to the config should only error when the previous values were not the defaults. --- agent/connect/ca/provider_vault.go | 53 +++++++++++++----------- agent/consul/connect_ca_endpoint_test.go | 53 ++++++++++++++++++++---- agent/consul/leader_connect_ca.go | 24 ++++++++++- 3 files changed, 97 insertions(+), 33 deletions(-) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index c7b99a13f3b6..9e107a2990ae 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -160,6 +160,34 @@ func (v *VaultProvider) Configure(cfg ProviderConfig) error { return nil } +func (v *VaultProvider) ValidateConfigUpdate(prevRaw, nextRaw map[string]interface{}) error { + prev, err := ParseVaultCAConfig(prevRaw) + if err != nil { + return err + } + next, err := ParseVaultCAConfig(nextRaw) + if err != nil { + return err + } + + if prev.RootPKIPath != next.RootPKIPath { + return nil + } + + if prev.PrivateKeyType != "" && prev.PrivateKeyType != connect.DefaultPrivateKeyType { + if prev.PrivateKeyType != next.PrivateKeyType { + return fmt.Errorf("cannot update the PrivateKeyType field without changing RootPKIPath") + } + } + + if prev.PrivateKeyBits != 0 && prev.PrivateKeyBits != connect.DefaultPrivateKeyBits { + if prev.PrivateKeyBits != next.PrivateKeyBits { + return fmt.Errorf("cannot update the PrivateKeyBits field without changing RootPKIPath") + } + } + return nil +} + // renewToken uses a vaultapi.LifetimeWatcher to repeatedly renew our token's lease. // If the token can no longer be renewed and auth method is set, // it will re-authenticate to Vault using the auth method and restart the renewer with the new token. @@ -272,31 +300,6 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) { if err != nil { return RootResult{}, err } - - if rootPEM != "" { - rootCert, err := connect.ParseCert(rootPEM) - if err != nil { - return RootResult{}, 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 RootResult{}, err - } - if v.config.PrivateKeyType != foundKeyType { - return RootResult{}, fmt.Errorf("cannot update the PrivateKeyType field without choosing a new PKI mount for the root CA") - } - if v.config.PrivateKeyBits != foundKeyBits { - return RootResult{}, fmt.Errorf("cannot update the PrivateKeyBits field without choosing a new PKI mount for the root CA") - } - } } return RootResult{PEM: rootPEM}, nil diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index eba3f732395d..bb4d7188e1cd 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -572,13 +572,53 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { }) testrpc.WaitForTestAgent(t, s1.RPC, "dc1") - cases := []struct { + // note: unlikely many table tests, the ordering of these cases does matter + // because any non-errored case will modify the CA config, and any subsequent + // tests will use the same agent with that new CA config. + testSteps := []struct { name string configFn func() *structs.CAConfiguration expectErr string }{ { - name: "cannot edit key bits", + name: "allow modifying key type and bits from default", + configFn: func() *structs.CAConfiguration { + 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, + } + }, + }, + { + name: "error when trying to modify key bits", + configFn: func() *structs.CAConfiguration { + return &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": testVault.Addr, + "Token": testVault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + // + "PrivateKeyType": "rsa", + "PrivateKeyBits": 2048, + }, + ForceWithoutCrossSigning: true, + } + }, + expectErr: `cannot update the PrivateKeyBits field without changing RootPKIPath`, + }, + { + name: "error when trying to modify key type", configFn: func() *structs.CAConfiguration { return &structs.CAConfiguration{ Provider: "vault", @@ -589,15 +629,15 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { "IntermediatePKIPath": "pki-intermediate/", // "PrivateKeyType": "ec", - "PrivateKeyBits": 384, + "PrivateKeyBits": 256, }, ForceWithoutCrossSigning: true, } }, - expectErr: `error generating CA root certificate: cannot update the PrivateKeyBits field without choosing a new PKI mount for the root CA`, + expectErr: `cannot update the PrivateKeyType field without changing RootPKIPath`, }, { - name: "cannot edit key type", + name: "allow update that does not change key type or bits", configFn: func() *structs.CAConfiguration { return &structs.CAConfiguration{ Provider: "vault", @@ -613,11 +653,10 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { ForceWithoutCrossSigning: true, } }, - 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 { + for _, tc := range testSteps { t.Run(tc.name, func(t *testing.T) { args := &structs.CARequest{ Datacenter: "dc1", diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 5b795727fec0..47b5bafcee2c 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -781,7 +781,7 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error) }() // Attempt to initialize the config if we failed to do so in Initialize for some reason - _, err = c.initializeCAConfig() + prevConfig, err := c.initializeCAConfig() if err != nil { return err } @@ -832,6 +832,15 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error) RawConfig: args.Config.Config, State: args.Config.State, } + + if args.Config.Provider == config.Provider { + if validator, ok := newProvider.(ValidateConfigUpdater); ok { + if err := validator.ValidateConfigUpdate(prevConfig.Config, args.Config.Config); err != nil { + return fmt.Errorf("new configuration is incompatible with previous configuration: %w", err) + } + } + } + if err := newProvider.Configure(pCfg); err != nil { return fmt.Errorf("error configuring provider: %v", err) } @@ -858,6 +867,19 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error) return nil } +// ValidateConfigUpdater is an optional interface that may be implemented +// by a ca.Provider. If the provider implements this interface, the +// ValidateConfigurationUpdate will be called when a user attempts to change the +// CA configuration, and the provider type has not changed from the previous +// configuration. +type ValidateConfigUpdater interface { + // ValidateConfigUpdate should return an error if the next configuration is + // incompatible with the previous configuration. + // + // TODO: use better types after https://github.com/hashicorp/consul/issues/12238 + ValidateConfigUpdate(previous, next map[string]interface{}) error +} + func (c *CAManager) primaryUpdateRootCA(newProvider ca.Provider, args *structs.CARequest, config *structs.CAConfiguration) error { providerRoot, err := newProvider.GenerateRoot() if err != nil { From 81a977ce1df6b4b7f6f0345ae25dd098f5ba0963 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 3 Feb 2022 17:39:36 -0500 Subject: [PATCH 3/4] add changelog --- .changelog/12267.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/12267.txt diff --git a/.changelog/12267.txt b/.changelog/12267.txt new file mode 100644 index 000000000000..ca9106c0e2ab --- /dev/null +++ b/.changelog/12267.txt @@ -0,0 +1,3 @@ +```release-note:bug +ca: adjust validation of PrivateKeyType/Bits with the Vault provider, to remove the error when the cert is created manually in Vault. +``` From 51b0f82d0e1541a5fcf5dcafb6736c16e26812ba Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 3 Feb 2022 18:44:09 -0500 Subject: [PATCH 4/4] Make test more readable And fix typo --- agent/connect/ca/provider_vault.go | 4 +- agent/consul/connect_ca_endpoint_test.go | 68 ++++++++---------------- 2 files changed, 23 insertions(+), 49 deletions(-) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index 9e107a2990ae..d837fe1a8501 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -163,11 +163,11 @@ func (v *VaultProvider) Configure(cfg ProviderConfig) error { func (v *VaultProvider) ValidateConfigUpdate(prevRaw, nextRaw map[string]interface{}) error { prev, err := ParseVaultCAConfig(prevRaw) if err != nil { - return err + return fmt.Errorf("failed to parse existing CA config: %w", err) } next, err := ParseVaultCAConfig(nextRaw) if err != nil { - return err + return fmt.Errorf("failed to parse new CA config: %w", err) } if prev.RootPKIPath != next.RootPKIPath { diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index bb4d7188e1cd..f32bf6ec1d91 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -559,20 +559,26 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { testVault := ca.NewTestVaultServer(t) + newConfig := func(keyType string, keyBits int) map[string]interface{} { + return map[string]interface{}{ + "Address": testVault.Addr, + "Token": testVault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + "PrivateKeyType": keyType, + "PrivateKeyBits": keyBits, + } + } + _, s1 := testServerWithConfig(t, func(c *Config) { c.CAConfig = &structs.CAConfiguration{ Provider: "vault", - Config: map[string]interface{}{ - "Address": testVault.Addr, - "Token": testVault.RootToken, - "RootPKIPath": "pki-root/", - "IntermediatePKIPath": "pki-intermediate/", - }, + Config: newConfig(connect.DefaultPrivateKeyType, connect.DefaultPrivateKeyBits), } }) testrpc.WaitForTestAgent(t, s1.RPC, "dc1") - // note: unlikely many table tests, the ordering of these cases does matter + // note: unlike many table tests, the ordering of these cases does matter // because any non-errored case will modify the CA config, and any subsequent // tests will use the same agent with that new CA config. testSteps := []struct { @@ -584,16 +590,8 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { name: "allow modifying key type and bits from default", configFn: func() *structs.CAConfiguration { 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, - }, + Provider: "vault", + Config: newConfig("rsa", 4096), ForceWithoutCrossSigning: true, } }, @@ -602,16 +600,8 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { name: "error when trying to modify key bits", configFn: func() *structs.CAConfiguration { return &structs.CAConfiguration{ - Provider: "vault", - Config: map[string]interface{}{ - "Address": testVault.Addr, - "Token": testVault.RootToken, - "RootPKIPath": "pki-root/", - "IntermediatePKIPath": "pki-intermediate/", - // - "PrivateKeyType": "rsa", - "PrivateKeyBits": 2048, - }, + Provider: "vault", + Config: newConfig("rsa", 2048), ForceWithoutCrossSigning: true, } }, @@ -621,16 +611,8 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { name: "error when trying to modify key type", configFn: func() *structs.CAConfiguration { return &structs.CAConfiguration{ - Provider: "vault", - Config: map[string]interface{}{ - "Address": testVault.Addr, - "Token": testVault.RootToken, - "RootPKIPath": "pki-root/", - "IntermediatePKIPath": "pki-intermediate/", - // - "PrivateKeyType": "ec", - "PrivateKeyBits": 256, - }, + Provider: "vault", + Config: newConfig("ec", 256), ForceWithoutCrossSigning: true, } }, @@ -640,16 +622,8 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { name: "allow update that does not change key type or bits", configFn: func() *structs.CAConfiguration { 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, - }, + Provider: "vault", + Config: newConfig("rsa", 4096), ForceWithoutCrossSigning: true, } },