From 6721c1246d48bdbc68def598d1086c659e850a15 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 3 Feb 2022 13:28:43 -0500 Subject: [PATCH] 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 {