luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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.
This commit is contained in:
Daniel Nephin 2022-02-03 13:28:43 -05:00
parent 3b78f81f9a
commit 6721c1246d
3 changed files with 113 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
agent/connect/ca/provider_vault.go
View File

@ -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

85
agent/consul/connect_ca_endpoint_test.go
View File

@ -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",

24
agent/consul/leader_connect_ca.go
View File

@ -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 {