Skip to content

Commit

Permalink
Merge pull request #12267 from hashicorp/dnephin/ca-relax-key-bit-val…
Browse files Browse the repository at this point in the history 
…idation

ca: change the PrivateKey type/bits validation
  • Loading branch information
@dnephin @hc-github-team-consul-core
dnephin authored and hc-github-team-consul-core committed Feb 4, 2022
1 parent 68151ed commit 285704b
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 83 deletions.
3 changes: 3 additions & 0 deletions .changelog/12267.txt
View file
Original file line number Diff line number Diff line change
@@ -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.
```
53 changes: 28 additions & 25 deletions agent/connect/ca/provider_vault.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
110 changes: 53 additions & 57 deletions agent/consul/connect_ca_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
24 changes: 23 additions & 1 deletion agent/consul/leader_connect_ca.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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 {
Expand Down

0 comments on commit 285704b

Please sign in to comment.