From ae8b526be865090733337bed81ec0cd9e4a12f97 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Tue, 13 Jul 2021 11:12:07 -0500 Subject: [PATCH] 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 | 3 + agent/connect/ca/provider_consul.go | 60 +++-- agent/connect/ca/provider_consul_test.go | 61 +++-- agent/consul/connect_ca_endpoint_test.go | 304 +++++++++++++---------- agent/consul/leader_connect_test.go | 150 +++++++++++ 5 files changed, 398 insertions(+), 180 deletions(-) create mode 100644 .changelog/10330.txt diff --git a/.changelog/10330.txt b/.changelog/10330.txt new file mode 100644 index 000000000..6d4f1c16f --- /dev/null +++ b/.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 +``` diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index 887f1b206..1ac316b53 100644 --- a/agent/connect/ca/provider_consul.go +++ b/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 = connect.HexString(hash[:]) + c.id = hexStringHash(fmt.Sprintf("%s,%s,%s,%d,%v", config.PrivateKey, config.RootCert, config.PrivateKeyType, config.PrivateKeyBits, cfg.IsPrimary)) c.clusterID = cfg.ClusterID c.isPrimary = cfg.IsPrimary c.spiffeID = connect.SpiffeIDSigningForCluster(&structs.CAConfiguration{ClusterID: c.clusterID}) @@ -87,35 +91,41 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error { return nil } - // Check if there's an entry with the old ID scheme. - oldID := fmt.Sprintf("%s,%s", config.PrivateKey, config.RootCert) - _, providerState, err = c.Delegate.State().CAProviderState(oldID) - if err != nil { - return err + oldIDs := []string{ + hexStringHash(fmt.Sprintf("%s,%s,%v", config.PrivateKey, config.RootCert, cfg.IsPrimary)), + fmt.Sprintf("%s,%s", config.PrivateKey, config.RootCert), } - // Found an entry with the old ID, so update it to the new ID and - // delete the old entry. - if providerState != nil { - newState := *providerState - newState.ID = c.id - createReq := &structs.CARequest{ - Op: structs.CAOpSetProviderState, - ProviderState: &newState, - } - if _, err := c.Delegate.ApplyCARequest(createReq); err != nil { + // 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 } - deleteReq := &structs.CARequest{ - Op: structs.CAOpDeleteProviderState, - ProviderState: providerState, - } - if _, err := c.Delegate.ApplyCARequest(deleteReq); err != nil { - return err - } + // Found an entry with the old ID, so update it to the new ID and + // delete the old entry. + if providerState != nil { + newState := *providerState + newState.ID = c.id + createReq := &structs.CARequest{ + Op: structs.CAOpSetProviderState, + ProviderState: &newState, + } + if _, err := c.Delegate.ApplyCARequest(createReq); err != nil { + return err + } - return nil + deleteReq := &structs.CARequest{ + Op: structs.CAOpDeleteProviderState, + ProviderState: providerState, + } + if _, err := c.Delegate.ApplyCARequest(deleteReq); err != nil { + return err + } + + return nil + } } args := &structs.CARequest{ diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index 852e6387d..8eef80a4f 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/agent/connect/ca/provider_consul_test.go @@ -438,30 +438,45 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) { } func TestConsulCAProvider_MigrateOldID(t *testing.T) { - t.Parallel() - - require := require.New(t) - conf := testConsulCAConfig() - delegate := newMockDelegate(t, conf) - - // Create an entry with an old-style ID. - _, err := delegate.ApplyCARequest(&structs.CARequest{ - Op: structs.CAOpSetProviderState, - ProviderState: &structs.CAConsulProviderState{ - ID: ",", + cases := []struct { + name string + oldID string + }{ + { + name: "original-unhashed", + oldID: ",", }, - }) - require.NoError(err) - _, providerState, err := delegate.state.CAProviderState(",") - require.NoError(err) - require.NotNil(providerState) + { + name: "hash-v1", + oldID: hexStringHash(",,true"), + }, + } - provider := TestConsulProvider(t, delegate) - require.NoError(provider.Configure(testProviderConfig(conf))) - require.NoError(provider.GenerateRoot()) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + conf := testConsulCAConfig() + delegate := newMockDelegate(t, conf) - // After running Configure, the old ID entry should be gone. - _, providerState, err = delegate.state.CAProviderState(",") - require.NoError(err) - require.Nil(providerState) + // Create an entry with an old-style ID. + _, err := delegate.ApplyCARequest(&structs.CARequest{ + Op: structs.CAOpSetProviderState, + ProviderState: &structs.CAConsulProviderState{ + ID: tc.oldID, + }, + }) + require.NoError(t, err) + _, providerState, err := delegate.state.CAProviderState(tc.oldID) + require.NoError(t, err) + require.NotNil(t, providerState) + + provider := TestConsulProvider(t, delegate) + require.NoError(t, provider.Configure(testProviderConfig(conf))) + require.NoError(t, provider.GenerateRoot()) + + // After running Configure, the old ID entry should be gone. + _, providerState, err = delegate.state.CAProviderState(tc.oldID) + require.NoError(t, err) + require.Nil(t, providerState) + }) + } } diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index b248f8085..f5b7438e2 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -361,154 +361,194 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { t.Parallel() - assert := assert.New(t) - require := require.New(t) - dir1, s1 := testServer(t) - defer os.RemoveAll(dir1) - defer s1.Shutdown() - codec := rpcClient(t, s1) - defer codec.Close() + cases := []struct { + 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 + } - testrpc.WaitForTestAgent(t, s1.RPC, "dc1") - - // Store the current root - rootReq := &structs.DCSpecificRequest{ - Datacenter: "dc1", - } - var rootList structs.IndexedCARoots - require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList)) - assert.Len(rootList.Roots, 1) - oldRoot := rootList.Roots[0] - - // Update the provider config to use a new private key, which should - // cause a rotation. - _, newKey, err := connect.GeneratePrivateKey() - assert.NoError(err) - newConfig := &structs.CAConfiguration{ - Provider: "consul", - Config: map[string]interface{}{ - "PrivateKey": newKey, - "RootCert": "", + 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 + }, }, } - { - args := &structs.CARequest{ - Datacenter: "dc1", - Config: newConfig, - } - var reply interface{} - require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply)) - } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir1, s1 := testServer(t) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() - // Make sure the new root has been added along with an intermediate - // cross-signed by the old root. - var newRootPEM string - { - args := &structs.DCSpecificRequest{ - Datacenter: "dc1", - } - var reply structs.IndexedCARoots - require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply)) - assert.Len(reply.Roots, 2) + testrpc.WaitForTestAgent(t, s1.RPC, "dc1") - 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.Equal(r.Name, oldRoot.Name) - assert.Equal(r.RootCert, oldRoot.RootCert) - assert.Equal(r.SigningCert, oldRoot.SigningCert) - assert.Equal(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.Len(r.IntermediateCerts, 1) - - xc := testParseCert(t, r.IntermediateCerts[0]) - oldRootCert := testParseCert(t, oldRoot.RootCert) - newRootCert := testParseCert(t, r.RootCert) - - // Should have the authority key ID and signature algo of the - // (old) signing CA. - assert.Equal(xc.AuthorityKeyId, oldRootCert.AuthorityKeyId) - assert.NotEqual(xc.SubjectKeyId, oldRootCert.SubjectKeyId) - assert.Equal(xc.SignatureAlgorithm, oldRootCert.SignatureAlgorithm) - - // The common name and SAN should not have changed. - assert.Equal(xc.Subject.CommonName, newRootCert.Subject.CommonName) - assert.Equal(xc.URIs, newRootCert.URIs) + // Store the current root + rootReq := &structs.DCSpecificRequest{ + Datacenter: "dc1", } - } - } + var rootList structs.IndexedCARoots + require.Nil(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList)) + assert.Len(t, rootList.Roots, 1) + oldRoot := rootList.Roots[0] - // Verify the new config was set. - { - args := &structs.DCSpecificRequest{ - Datacenter: "dc1", - } - var reply structs.CAConfiguration - require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply)) + newConfig, err := tc.configFn() + require.NoError(t, err) - actual, err := ca.ParseConsulCAConfig(reply.Config) - require.NoError(err) - expected, err := ca.ParseConsulCAConfig(newConfig.Config) - require.NoError(err) - assert.Equal(reply.Provider, newConfig.Provider) - assert.Equal(actual, expected) - } + { + args := &structs.CARequest{ + Datacenter: "dc1", + Config: newConfig, + } + var reply interface{} - // Verify that new leaf certs get the cross-signed intermediate bundled - { - // Generate a CSR and request signing - spiffeId := connect.TestSpiffeIDService(t, "web") - csr, _ := connect.TestCSR(t, spiffeId) - args := &structs.CASignRequest{ - Datacenter: "dc1", - CSR: csr, - } - var reply structs.IssuedCert - require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply)) + require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply)) + } - // Verify that the cert is signed by the new CA - { - roots := x509.NewCertPool() - require.True(roots.AppendCertsFromPEM([]byte(newRootPEM))) - leaf, err := connect.ParseCert(reply.CertPEM) - require.NoError(err) - _, err = leaf.Verify(x509.VerifyOptions{ - Roots: roots, + // 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(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply)) + 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(t, r.Active) + assert.Equal(t, r.Name, oldRoot.Name) + assert.Equal(t, r.RootCert, oldRoot.RootCert) + assert.Equal(t, r.SigningCert, oldRoot.SigningCert) + 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(t, r.Active) + assert.Len(t, r.IntermediateCerts, 1) + + xc := testParseCert(t, r.IntermediateCerts[0]) + oldRootCert := testParseCert(t, oldRoot.RootCert) + newRootCert := testParseCert(t, r.RootCert) + + // Should have the authority key ID and signature algo of the + // (old) signing CA. + assert.Equal(t, xc.AuthorityKeyId, oldRootCert.AuthorityKeyId) + assert.NotEqual(t, xc.SubjectKeyId, oldRootCert.SubjectKeyId) + assert.Equal(t, xc.SignatureAlgorithm, oldRootCert.SignatureAlgorithm) + + // The common name and SAN should not have changed. + assert.Equal(t, xc.Subject.CommonName, newRootCert.Subject.CommonName) + assert.Equal(t, xc.URIs, newRootCert.URIs) + } + } }) - require.NoError(err) - } - // And that it validates via the intermediate - { - roots := x509.NewCertPool() - assert.True(roots.AppendCertsFromPEM([]byte(oldRoot.RootCert))) - leaf, err := connect.ParseCert(reply.CertPEM) - require.NoError(err) + runStep(t, "verify the new config was set", func(t *testing.T) { + args := &structs.DCSpecificRequest{ + Datacenter: "dc1", + } + var reply structs.CAConfiguration + require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply)) - // Make sure the intermediate was returned as well as leaf - _, rest := pem.Decode([]byte(reply.CertPEM)) - require.NotEmpty(rest) - - intermediates := x509.NewCertPool() - require.True(intermediates.AppendCertsFromPEM(rest)) - - _, err = leaf.Verify(x509.VerifyOptions{ - Roots: roots, - Intermediates: intermediates, + actual, err := ca.ParseConsulCAConfig(reply.Config) + require.NoError(t, err) + expected, err := ca.ParseConsulCAConfig(newConfig.Config) + require.NoError(t, err) + assert.Equal(t, reply.Provider, newConfig.Provider) + assert.Equal(t, actual, expected) }) - require.NoError(err) - } - // Verify other fields - assert.Equal("web", reply.Service) - assert.Equal(spiffeId.URI().String(), reply.ServiceURI) + 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) + args := &structs.CASignRequest{ + Datacenter: "dc1", + CSR: csr, + } + var reply structs.IssuedCert + require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply)) + + runStep(t, "verify that the cert is signed by the new CA", func(t *testing.T) { + roots := x509.NewCertPool() + require.True(t, roots.AppendCertsFromPEM([]byte(newRootPEM))) + leaf, err := connect.ParseCert(reply.CertPEM) + require.NoError(t, err) + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: roots, + }) + require.NoError(t, err) + }) + + runStep(t, "and that it validates via the intermediate", func(t *testing.T) { + roots := x509.NewCertPool() + assert.True(t, roots.AppendCertsFromPEM([]byte(oldRoot.RootCert))) + leaf, err := connect.ParseCert(reply.CertPEM) + require.NoError(t, err) + + // Make sure the intermediate was returned as well as leaf + _, rest := pem.Decode([]byte(reply.CertPEM)) + require.NotEmpty(t, rest) + + intermediates := x509.NewCertPool() + require.True(t, intermediates.AppendCertsFromPEM(rest)) + + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: roots, + Intermediates: intermediates, + }) + require.NoError(t, err) + }) + + runStep(t, "verify other fields", func(t *testing.T) { + assert.Equal(t, "web", reply.Service) + assert.Equal(t, spiffeId.URI().String(), reply.ServiceURI) + }) + }) + }) } } diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index a1e0c4a9e..4d713e031 100644 --- a/agent/consul/leader_connect_test.go +++ b/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")