From 285704bb389030d0982a99955f6e58b7213c8c19 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 4 Feb 2022 12:44:08 -0500 Subject: [PATCH] Merge pull request #12267 from hashicorp/dnephin/ca-relax-key-bit-validation ca: change the PrivateKey type/bits validation --- .changelog/12267.txt | 3 + agent/connect/ca/provider_vault.go | 53 +++++------ agent/consul/connect_ca_endpoint_test.go | 110 +++++++++++------------ agent/consul/leader_connect_ca.go | 24 ++++- 4 files changed, 107 insertions(+), 83 deletions(-) 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. +``` diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index c7b99a13f3b6..d837fe1a8501 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 fmt.Errorf("failed to parse existing CA config: %w", err) + } + next, err := ParseVaultCAConfig(nextRaw) + if err != nil { + return fmt.Errorf("failed to parse new CA config: %w", 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 40a384f9024c..e8b6d03ad9dd 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -562,92 +562,88 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) { t.Parallel() testVault := ca.NewTestVaultServer(t) - defer testVault.Stop() + + 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.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/", - }, + Config: newConfig(connect.DefaultPrivateKeyType, connect.DefaultPrivateKeyBits), } }) - 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 { + // 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 { name string - configFn func() (*structs.CAConfiguration, error) + configFn func() *structs.CAConfiguration expectErr string }{ { - name: "cannot edit key bits", - configFn: func() (*structs.CAConfiguration, error) { + 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": "ec", - "PrivateKeyBits": 384, - }, + Provider: "vault", + Config: newConfig("rsa", 4096), 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) { + 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": 4096, - }, + Provider: "vault", + Config: newConfig("rsa", 2048), ForceWithoutCrossSigning: true, - }, nil + } + }, + 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: newConfig("ec", 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", + Config: newConfig("rsa", 4096), + 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) { - 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 { 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 {