From 3b78f81f9acad845273e0c0d8d326c7d6b63cbc8 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 d170729db..eba3f7323 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 6721c1246d48bdbc68def598d1086c659e850a15 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 | 85 +++++++++++++++++------- agent/consul/leader_connect_ca.go | 24 ++++++- 3 files changed, 113 insertions(+), 49 deletions(-) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index c7b99a13f..9e107a299 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 eba3f7323..bb4d7188e 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -572,32 +572,72 @@ 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", - 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": 384, - }, - ForceWithoutCrossSigning: true, - } - }, - 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", + 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", + Config: map[string]interface{}{ + "Address": testVault.Addr, + "Token": testVault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + // + "PrivateKeyType": "ec", + "PrivateKeyBits": 256, + }, + ForceWithoutCrossSigning: true, + } + }, + expectErr: `cannot update the PrivateKeyType field without changing RootPKIPath`, + }, + { + 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 5b795727f..47b5bafce 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 cc2d1bc2e7c41a7fb2bcbfc2e7cb1d436398ca61 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 000000000..ca9106c0e --- /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 7b466a024ba4cde76c57ed27392fcc9ecea282fd 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 9e107a299..d837fe1a8 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 bb4d7188e..f32bf6ec1 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, } },