connect/ca: ensure edits to the key type/bits for the connect builtin CA will regenerate the roots (#10330)

progress on #9572

.changelog/10330.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+connect/ca: ensure edits to the key type/bits for the connect builtin CA will regenerate the roots
+```

agent/connect/ca/provider_consul.go

@@ -60,6 +60,11 @@ type ConsulProviderStateDelegate interface {
 	ApplyCARequest(*structs.CARequest) (interface{}, error)
 }
 
+func hexStringHash(input string) string {
+	hash := sha256.Sum256([]byte(input))
+	return connect.HexString(hash[:])
+}
+
 // Configure sets up the provider using the given configuration.
 func (c *ConsulProvider) Configure(cfg ProviderConfig) error {
 	// Parse the raw config and update our ID.
@@ -68,8 +73,7 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error {
 		return err
 	}
 	c.config = config
-	hash := sha256.Sum256([]byte(fmt.Sprintf("%s,%s,%v", config.PrivateKey, config.RootCert, cfg.IsPrimary)))
+	c.id = hexStringHash(fmt.Sprintf("%s,%s,%s,%d,%v", config.PrivateKey, config.RootCert, config.PrivateKeyType, config.PrivateKeyBits, cfg.IsPrimary))
-	c.id = connect.HexString(hash[:])
 	c.clusterID = cfg.ClusterID
 	c.isPrimary = cfg.IsPrimary
 	c.spiffeID = connect.SpiffeIDSigningForCluster(&structs.CAConfiguration{ClusterID: c.clusterID})
@@ -87,8 +91,13 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error {
 		return nil
 	}
 
-	// Check if there's an entry with the old ID scheme.
+	oldIDs := []string{
-	oldID := fmt.Sprintf("%s,%s", config.PrivateKey, config.RootCert)
+		hexStringHash(fmt.Sprintf("%s,%s,%v", config.PrivateKey, config.RootCert, cfg.IsPrimary)),
+		fmt.Sprintf("%s,%s", config.PrivateKey, config.RootCert),
+	}
+
+	// Check if there any entries with old ID schemes.
+	for _, oldID := range oldIDs {
 		_, providerState, err = c.Delegate.State().CAProviderState(oldID)
 		if err != nil {
 			return err
@@ -117,6 +126,7 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error {
 
 			return nil
 		}
+	}
 
 	args := &structs.CARequest{
 		Op:            structs.CAOpSetProviderState,

agent/connect/ca/provider_consul_test.go

@@ -438,9 +438,22 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) {
 }
 
 func TestConsulCAProvider_MigrateOldID(t *testing.T) {
-	t.Parallel()
+	cases := []struct {
+		name  string
+		oldID string
+	}{
+		{
+			name:  "original-unhashed",
+			oldID: ",",
+		},
+		{
+			name:  "hash-v1",
+			oldID: hexStringHash(",,true"),
+		},
+	}
 
-	require := require.New(t)
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
 			conf := testConsulCAConfig()
 			delegate := newMockDelegate(t, conf)
 
@@ -448,20 +461,22 @@ func TestConsulCAProvider_MigrateOldID(t *testing.T) {
 			_, err := delegate.ApplyCARequest(&structs.CARequest{
 				Op: structs.CAOpSetProviderState,
 				ProviderState: &structs.CAConsulProviderState{
-			ID: ",",
+					ID: tc.oldID,
 				},
 			})
-	require.NoError(err)
+			require.NoError(t, err)
-	_, providerState, err := delegate.state.CAProviderState(",")
+			_, providerState, err := delegate.state.CAProviderState(tc.oldID)
-	require.NoError(err)
+			require.NoError(t, err)
-	require.NotNil(providerState)
+			require.NotNil(t, providerState)
 
 			provider := TestConsulProvider(t, delegate)
-	require.NoError(provider.Configure(testProviderConfig(conf)))
+			require.NoError(t, provider.Configure(testProviderConfig(conf)))
-	require.NoError(provider.GenerateRoot())
+			require.NoError(t, provider.GenerateRoot())
 
 			// After running Configure, the old ID entry should be gone.
-	_, providerState, err = delegate.state.CAProviderState(",")
+			_, providerState, err = delegate.state.CAProviderState(tc.oldID)
-	require.NoError(err)
+			require.NoError(t, err)
-	require.Nil(providerState)
+			require.Nil(t, providerState)
+		})
+	}
 }

agent/consul/connect_ca_endpoint_test.go

@@ -361,8 +361,57 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 
 	t.Parallel()
 
-	assert := assert.New(t)
+	cases := []struct {
-	require := require.New(t)
+		name     string
+		configFn func() (*structs.CAConfiguration, error)
+	}{
+		{
+			name: "new private key provided",
+			configFn: func() (*structs.CAConfiguration, error) {
+				// Update the provider config to use a new private key, which should
+				// cause a rotation.
+				_, newKey, err := connect.GeneratePrivateKey()
+				if err != nil {
+					return nil, err
+				}
+
+				return &structs.CAConfiguration{
+					Provider: "consul",
+					Config: map[string]interface{}{
+						"PrivateKey": newKey,
+						"RootCert":   "",
+					},
+				}, nil
+			},
+		},
+		{
+			name: "update private key bits",
+			configFn: func() (*structs.CAConfiguration, error) {
+				return &structs.CAConfiguration{
+					Provider: "consul",
+					Config: map[string]interface{}{
+						"PrivateKeyType": "ec",
+						"PrivateKeyBits": 384,
+					},
+				}, nil
+			},
+		},
+		{
+			name: "update private key type",
+			configFn: func() (*structs.CAConfiguration, error) {
+				return &structs.CAConfiguration{
+					Provider: "consul",
+					Config: map[string]interface{}{
+						"PrivateKeyType": "rsa",
+						"PrivateKeyBits": "2048",
+					},
+				}, nil
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
 			dir1, s1 := testServer(t)
 			defer os.RemoveAll(dir1)
 			defer s1.Shutdown()
@@ -376,21 +425,13 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 				Datacenter: "dc1",
 			}
 			var rootList structs.IndexedCARoots
-	require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList))
+			require.Nil(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList))
-	assert.Len(rootList.Roots, 1)
+			assert.Len(t, rootList.Roots, 1)
 			oldRoot := rootList.Roots[0]
 
-	// Update the provider config to use a new private key, which should
+			newConfig, err := tc.configFn()
-	// cause a rotation.
+			require.NoError(t, err)
-	_, newKey, err := connect.GeneratePrivateKey()
+
-	assert.NoError(err)
-	newConfig := &structs.CAConfiguration{
-		Provider: "consul",
-		Config: map[string]interface{}{
-			"PrivateKey": newKey,
-			"RootCert":   "",
-		},
-	}
 			{
 				args := &structs.CARequest{
 					Datacenter: "dc1",
@@ -398,35 +439,35 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 				}
 				var reply interface{}
 
-		require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply))
+				require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply))
 			}
 
 			// Make sure the new root has been added along with an intermediate
 			// cross-signed by the old root.
 			var newRootPEM string
-	{
+			runStep(t, "ensure roots look correct", func(t *testing.T) {
 				args := &structs.DCSpecificRequest{
 					Datacenter: "dc1",
 				}
 				var reply structs.IndexedCARoots
-		require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply))
+				require.Nil(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply))
-		assert.Len(reply.Roots, 2)
+				assert.Len(t, reply.Roots, 2)
 
 				for _, r := range reply.Roots {
 					if r.ID == oldRoot.ID {
 						// The old root should no longer be marked as the active root,
 						// and none of its other fields should have changed.
-				assert.False(r.Active)
+						assert.False(t, r.Active)
-				assert.Equal(r.Name, oldRoot.Name)
+						assert.Equal(t, r.Name, oldRoot.Name)
-				assert.Equal(r.RootCert, oldRoot.RootCert)
+						assert.Equal(t, r.RootCert, oldRoot.RootCert)
-				assert.Equal(r.SigningCert, oldRoot.SigningCert)
+						assert.Equal(t, r.SigningCert, oldRoot.SigningCert)
-				assert.Equal(r.IntermediateCerts, oldRoot.IntermediateCerts)
+						assert.Equal(t, r.IntermediateCerts, oldRoot.IntermediateCerts)
 					} else {
 						newRootPEM = r.RootCert
 						// The new root should have a valid cross-signed cert from the old
 						// root as an intermediate.
-				assert.True(r.Active)
+						assert.True(t, r.Active)
-				assert.Len(r.IntermediateCerts, 1)
+						assert.Len(t, r.IntermediateCerts, 1)
 
 						xc := testParseCert(t, r.IntermediateCerts[0])
 						oldRootCert := testParseCert(t, oldRoot.RootCert)
@@ -434,35 +475,33 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 
 						// Should have the authority key ID and signature algo of the
 						// (old) signing CA.
-				assert.Equal(xc.AuthorityKeyId, oldRootCert.AuthorityKeyId)
+						assert.Equal(t, xc.AuthorityKeyId, oldRootCert.AuthorityKeyId)
-				assert.NotEqual(xc.SubjectKeyId, oldRootCert.SubjectKeyId)
+						assert.NotEqual(t, xc.SubjectKeyId, oldRootCert.SubjectKeyId)
-				assert.Equal(xc.SignatureAlgorithm, oldRootCert.SignatureAlgorithm)
+						assert.Equal(t, xc.SignatureAlgorithm, oldRootCert.SignatureAlgorithm)
 
 						// The common name and SAN should not have changed.
-				assert.Equal(xc.Subject.CommonName, newRootCert.Subject.CommonName)
+						assert.Equal(t, xc.Subject.CommonName, newRootCert.Subject.CommonName)
-				assert.Equal(xc.URIs, newRootCert.URIs)
+						assert.Equal(t, xc.URIs, newRootCert.URIs)
-			}
 					}
 				}
+			})
 
-	// Verify the new config was set.
+			runStep(t, "verify the new config was set", func(t *testing.T) {
-	{
 				args := &structs.DCSpecificRequest{
 					Datacenter: "dc1",
 				}
 				var reply structs.CAConfiguration
-		require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply))
+				require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply))
 
 				actual, err := ca.ParseConsulCAConfig(reply.Config)
-		require.NoError(err)
+				require.NoError(t, err)
 				expected, err := ca.ParseConsulCAConfig(newConfig.Config)
-		require.NoError(err)
+				require.NoError(t, err)
-		assert.Equal(reply.Provider, newConfig.Provider)
+				assert.Equal(t, reply.Provider, newConfig.Provider)
-		assert.Equal(actual, expected)
+				assert.Equal(t, actual, expected)
-	}
+			})
 
-	// Verify that new leaf certs get the cross-signed intermediate bundled
+			runStep(t, "verify that new leaf certs get the cross-signed intermediate bundled", func(t *testing.T) {
-	{
 				// Generate a CSR and request signing
 				spiffeId := connect.TestSpiffeIDService(t, "web")
 				csr, _ := connect.TestCSR(t, spiffeId)
@@ -471,44 +510,45 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 					CSR:        csr,
 				}
 				var reply structs.IssuedCert
-		require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply))
+				require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply))
 
-		// Verify that the cert is signed by the new CA
+				runStep(t, "verify that the cert is signed by the new CA", func(t *testing.T) {
-		{
 					roots := x509.NewCertPool()
-			require.True(roots.AppendCertsFromPEM([]byte(newRootPEM)))
+					require.True(t, roots.AppendCertsFromPEM([]byte(newRootPEM)))
 					leaf, err := connect.ParseCert(reply.CertPEM)
-			require.NoError(err)
+					require.NoError(t, err)
 					_, err = leaf.Verify(x509.VerifyOptions{
 						Roots: roots,
 					})
-			require.NoError(err)
+					require.NoError(t, err)
-		}
+				})
 
-		// And that it validates via the intermediate
+				runStep(t, "and that it validates via the intermediate", func(t *testing.T) {
-		{
 					roots := x509.NewCertPool()
-			assert.True(roots.AppendCertsFromPEM([]byte(oldRoot.RootCert)))
+					assert.True(t, roots.AppendCertsFromPEM([]byte(oldRoot.RootCert)))
 					leaf, err := connect.ParseCert(reply.CertPEM)
-			require.NoError(err)
+					require.NoError(t, err)
 
 					// Make sure the intermediate was returned as well as leaf
 					_, rest := pem.Decode([]byte(reply.CertPEM))
-			require.NotEmpty(rest)
+					require.NotEmpty(t, rest)
 
 					intermediates := x509.NewCertPool()
-			require.True(intermediates.AppendCertsFromPEM(rest))
+					require.True(t, intermediates.AppendCertsFromPEM(rest))
 
 					_, err = leaf.Verify(x509.VerifyOptions{
 						Roots:         roots,
 						Intermediates: intermediates,
 					})
-			require.NoError(err)
+					require.NoError(t, err)
-		}
+				})
 
-		// Verify other fields
+				runStep(t, "verify other fields", func(t *testing.T) {
-		assert.Equal("web", reply.Service)
+					assert.Equal(t, "web", reply.Service)
-		assert.Equal(spiffeId.URI().String(), reply.ServiceURI)
+					assert.Equal(t, spiffeId.URI().String(), reply.ServiceURI)
+				})
+			})
+		})
 	}
 }
 

agent/consul/leader_connect_test.go

@@ -24,6 +24,156 @@ import (
 	"github.com/hashicorp/consul/testrpc"
 )
 
+func TestLeader_Builtin_PrimaryCA_ChangeKeyConfig(t *testing.T) {
+	if testing.Short() {
+		t.Skip("too slow for testing.Short")
+	}
+
+	types := []struct {
+		keyType string
+		keyBits int
+	}{
+		{connect.DefaultPrivateKeyType, connect.DefaultPrivateKeyBits},
+		{"ec", 256},
+		{"ec", 384},
+		{"rsa", 2048},
+		{"rsa", 4096},
+	}
+
+	for _, src := range types {
+		for _, dst := range types {
+			if src == dst {
+				continue // skip
+			}
+			src := src
+			dst := dst
+			t.Run(fmt.Sprintf("%s-%d to %s-%d", src.keyType, src.keyBits, dst.keyType, dst.keyBits), func(t *testing.T) {
+				t.Parallel()
+
+				providerState := map[string]string{"foo": "dc1-value"}
+
+				// Initialize primary as the primary DC
+				dir1, srv := testServerWithConfig(t, func(c *Config) {
+					c.Datacenter = "dc1"
+					c.Build = "1.6.0"
+					c.CAConfig.Config["PrivateKeyType"] = src.keyType
+					c.CAConfig.Config["PrivateKeyBits"] = src.keyBits
+					c.CAConfig.Config["test_state"] = providerState
+				})
+				defer os.RemoveAll(dir1)
+				defer srv.Shutdown()
+				codec := rpcClient(t, srv)
+				defer codec.Close()
+
+				testrpc.WaitForLeader(t, srv.RPC, "dc1")
+				testrpc.WaitForActiveCARoot(t, srv.RPC, "dc1", nil)
+
+				var (
+					provider ca.Provider
+					caRoot   *structs.CARoot
+				)
+				retry.Run(t, func(r *retry.R) {
+					provider, caRoot = getCAProviderWithLock(srv)
+					require.NotNil(r, caRoot)
+					// Sanity check CA is using the correct key type
+					require.Equal(r, src.keyType, caRoot.PrivateKeyType)
+					require.Equal(r, src.keyBits, caRoot.PrivateKeyBits)
+				})
+
+				runStep(t, "sign leaf cert and make sure chain is correct", func(t *testing.T) {
+					spiffeService := &connect.SpiffeIDService{
+						Host:       "node1",
+						Namespace:  "default",
+						Datacenter: "dc1",
+						Service:    "foo",
+					}
+					raw, _ := connect.TestCSR(t, spiffeService)
+
+					leafCsr, err := connect.ParseCSR(raw)
+					require.NoError(t, err)
+
+					leafPEM, err := provider.Sign(leafCsr)
+					require.NoError(t, err)
+
+					// Check that the leaf signed by the new cert can be verified using the
+					// returned cert chain
+					require.NoError(t, connect.ValidateLeaf(caRoot.RootCert, leafPEM, []string{}))
+				})
+
+				runStep(t, "verify persisted state is correct", func(t *testing.T) {
+					state := srv.fsm.State()
+					_, caConfig, err := state.CAConfig(nil)
+					require.NoError(t, err)
+					require.Equal(t, providerState, caConfig.State)
+				})
+
+				runStep(t, "change roots", func(t *testing.T) {
+					// Update a config value
+					newConfig := &structs.CAConfiguration{
+						Provider: "consul",
+						Config: map[string]interface{}{
+							"PrivateKey":     "",
+							"RootCert":       "",
+							"PrivateKeyType": dst.keyType,
+							"PrivateKeyBits": dst.keyBits,
+							// This verifies the state persistence for providers although Consul
+							// provider doesn't actually use that mechanism outside of tests.
+							"test_state": providerState,
+						},
+					}
+
+					args := &structs.CARequest{
+						Datacenter: "dc1",
+						Config:     newConfig,
+					}
+					var reply interface{}
+					require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply))
+				})
+
+				var (
+					newProvider ca.Provider
+					newCaRoot   *structs.CARoot
+				)
+				retry.Run(t, func(r *retry.R) {
+					newProvider, newCaRoot = getCAProviderWithLock(srv)
+					require.NotNil(r, newCaRoot)
+					// Sanity check CA is using the correct key type
+					require.Equal(r, dst.keyType, newCaRoot.PrivateKeyType)
+					require.Equal(r, dst.keyBits, newCaRoot.PrivateKeyBits)
+				})
+
+				runStep(t, "sign leaf cert and make sure NEW chain is correct", func(t *testing.T) {
+					spiffeService := &connect.SpiffeIDService{
+						Host:       "node1",
+						Namespace:  "default",
+						Datacenter: "dc1",
+						Service:    "foo",
+					}
+					raw, _ := connect.TestCSR(t, spiffeService)
+
+					leafCsr, err := connect.ParseCSR(raw)
+					require.NoError(t, err)
+
+					leafPEM, err := newProvider.Sign(leafCsr)
+					require.NoError(t, err)
+
+					// Check that the leaf signed by the new cert can be verified using the
+					// returned cert chain
+					require.NoError(t, connect.ValidateLeaf(newCaRoot.RootCert, leafPEM, []string{}))
+				})
+
+				runStep(t, "verify persisted state is still correct", func(t *testing.T) {
+					state := srv.fsm.State()
+					_, caConfig, err := state.CAConfig(nil)
+					require.NoError(t, err)
+					require.Equal(t, providerState, caConfig.State)
+				})
+			})
+		}
+	}
+
+}
+
 func TestLeader_SecondaryCA_Initialize(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")