Merge pull request #12267 from hashicorp/dnephin/ca-relax-key-bit-validation

ca: change the PrivateKey type/bits validation

.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.
+```

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

agent/consul/connect_ca_endpoint_test.go

@@ -558,92 +558,88 @@ func TestConnectCAConfig_Vault_TriggerRotation_Fails(t *testing.T) {
 	t.Parallel()
 
 	testVault := ca.NewTestVaultServer(t)
-	defer testVault.Stop()
 
-	_, s1 := testServerWithConfig(t, func(c *Config) {
+	newConfig := func(keyType string, keyBits int) map[string]interface{} {
-		c.Build = "1.6.0"
+		return map[string]interface{}{
-		c.PrimaryDatacenter = "dc1"
+			"Address":             testVault.Addr,
-		c.CAConfig = &structs.CAConfiguration{
+			"Token":               testVault.RootToken,
-			Provider: "vault",
+			"RootPKIPath":         "pki-root/",
-			Config: map[string]interface{}{
+			"IntermediatePKIPath": "pki-intermediate/",
-				"Address":             testVault.Addr,
+			"PrivateKeyType":      keyType,
-				"Token":               testVault.RootToken,
+			"PrivateKeyBits":      keyBits,
-				"RootPKIPath":         "pki-root/",
-				"IntermediatePKIPath": "pki-intermediate/",
-			},
 		}
-	})
-	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 {
+	_, s1 := testServerWithConfig(t, func(c *Config) {
+		c.CAConfig = &structs.CAConfiguration{
+			Provider: "vault",
+			Config:   newConfig(connect.DefaultPrivateKeyType, connect.DefaultPrivateKeyBits),
+		}
+	})
+	testrpc.WaitForTestAgent(t, s1.RPC, "dc1")
+
+	// 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",
+			name: "allow modifying key type and bits from default",
-			configFn: func() (*structs.CAConfiguration, error) {
+			configFn: func() *structs.CAConfiguration {
 				return &structs.CAConfiguration{
-					Provider: "vault",
+					Provider:                 "vault",
-					Config: map[string]interface{}{
+					Config:                   newConfig("rsa", 4096),
-						"Address":             testVault.Addr,
-						"Token":               testVault.RootToken,
-						"RootPKIPath":         "pki-root/",
-						"IntermediatePKIPath": "pki-intermediate/",
-						//
-						"PrivateKeyType": "ec",
-						"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",
+			name: "error when trying to modify key bits",
-			configFn: func() (*structs.CAConfiguration, error) {
+			configFn: func() *structs.CAConfiguration {
 				return &structs.CAConfiguration{
-					Provider: "vault",
+					Provider:                 "vault",
-					Config: map[string]interface{}{
+					Config:                   newConfig("rsa", 2048),
-						"Address":             testVault.Addr,
-						"Token":               testVault.RootToken,
-						"RootPKIPath":         "pki-root/",
-						"IntermediatePKIPath": "pki-intermediate/",
-						//
-						"PrivateKeyType": "rsa",
-						"PrivateKeyBits": 4096,
-					},
 					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 {

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 {