From 0b4876f906c8e49fe989636c84b7f4cf03506512 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Wed, 21 Oct 2020 12:49:21 -0700 Subject: [PATCH] connect: Add logic for updating secondary DC intermediate on config set --- agent/connect/testing_spiffe.go | 8 +- agent/consul/connect_ca_endpoint.go | 27 ++++- agent/consul/connect_ca_endpoint_test.go | 134 +++++++++++++++++++++++ agent/consul/leader_connect.go | 68 +++++++----- 4 files changed, 207 insertions(+), 30 deletions(-) diff --git a/agent/connect/testing_spiffe.go b/agent/connect/testing_spiffe.go index c7fa6f753..4a0577880 100644 --- a/agent/connect/testing_spiffe.go +++ b/agent/connect/testing_spiffe.go @@ -12,10 +12,16 @@ func TestSpiffeIDService(t testing.T, service string) *SpiffeIDService { // TestSpiffeIDServiceWithHost returns a SPIFFE ID representing a service with // the specified trust domain. func TestSpiffeIDServiceWithHost(t testing.T, service, host string) *SpiffeIDService { + return TestSpiffeIDServiceWithHostDC(t, service, host, "dc1") +} + +// TestSpiffeIDServiceWithHostDC returns a SPIFFE ID representing a service with +// the specified trust domain for the given datacenter. +func TestSpiffeIDServiceWithHostDC(t testing.T, service, host, datacenter string) *SpiffeIDService { return &SpiffeIDService{ Host: host, Namespace: "default", - Datacenter: "dc1", + Datacenter: datacenter, Service: service, } } diff --git a/agent/consul/connect_ca_endpoint.go b/agent/consul/connect_ca_endpoint.go index 2198c0ec6..9733f12b4 100644 --- a/agent/consul/connect_ca_endpoint.go +++ b/agent/consul/connect_ca_endpoint.go @@ -163,6 +163,27 @@ func (s *ConnectCA) ConfigurationSet( } }() + // If this is a secondary, check if the intermediate needs to be regenerated. + if s.srv.config.Datacenter != s.srv.config.PrimaryDatacenter { + // Get the current root certs from the primary DC. + var roots structs.IndexedCARoots + rootArgs := structs.DCSpecificRequest{ + Datacenter: s.srv.config.PrimaryDatacenter, + } + if err := s.srv.forwardDC("ConnectCA.Roots", s.srv.config.PrimaryDatacenter, &rootArgs, &roots); err != nil { + return fmt.Errorf("Error retrieving the primary datacenter's roots: %v", err) + } + + s.srv.caProviderReconfigurationLock.Lock() + defer s.srv.caProviderReconfigurationLock.Unlock() + if err := s.srv.initializeSecondaryCA(newProvider, roots, args.Config); err != nil { + return fmt.Errorf("Error updating secondary datacenter CA config: %v", err) + } + cleanupNewProvider = false + s.logger.Info("Secondary CA provider config updated") + return nil + } + if err := newProvider.GenerateRoot(); err != nil { return fmt.Errorf("error generating CA root certificate: %v", err) } @@ -192,10 +213,8 @@ func (s *ConnectCA) ConfigurationSet( return err } - // If the root didn't change or if this is a secondary DC, just update the - // config and return. - if (s.srv.config.Datacenter != s.srv.config.PrimaryDatacenter) || - root != nil && root.ID == newActiveRoot.ID { + // If the root didn't change, just update the config and return. + if root != nil && root.ID == newActiveRoot.ID { args.Op = structs.CAOpSetConfig resp, err := s.srv.raftApply(structs.ConnectCARequestType, args) if err != nil { diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index d55c7e4b6..315358911 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -405,6 +405,140 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) { } } +func TestConnectCAConfig_UpdateSecondary(t *testing.T) { + t.Parallel() + + assert := assert.New(t) + require := require.New(t) + + // Initialize primary as the primary DC + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.Datacenter = "primary" + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + + testrpc.WaitForLeader(t, s1.RPC, "primary") + + // secondary as a secondary DC + dir2, s2 := testServerWithConfig(t, func(c *Config) { + c.Datacenter = "secondary" + c.PrimaryDatacenter = "primary" + }) + defer os.RemoveAll(dir2) + defer s2.Shutdown() + codec := rpcClient(t, s2) + defer codec.Close() + + // Create the WAN link + joinWAN(t, s2, s1) + testrpc.WaitForLeader(t, s2.RPC, "secondary") + + // Capture the current root + rootList, activeRoot, err := getTestRoots(s1, "primary") + require.NoError(err) + require.Len(rootList.Roots, 1) + rootCert := activeRoot + + waitForActiveCARoot(t, s1, rootCert) + waitForActiveCARoot(t, s2, rootCert) + + // Capture the current intermediate + rootList, activeRoot, err = getTestRoots(s2, "secondary") + require.NoError(err) + require.Len(rootList.Roots, 1) + require.Len(activeRoot.IntermediateCerts, 1) + oldIntermediatePEM := activeRoot.IntermediateCerts[0] + + // Update the secondary CA config to use a new private key, which should + // cause a re-signing with a new intermediate. + _, newKey, err := connect.GeneratePrivateKey() + assert.NoError(err) + newConfig := &structs.CAConfiguration{ + Provider: "consul", + Config: map[string]interface{}{ + "PrivateKey": newKey, + "RootCert": "", + "RotationPeriod": 90 * 24 * time.Hour, + }, + } + { + args := &structs.CARequest{ + Datacenter: "secondary", + Config: newConfig, + } + var reply interface{} + + require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply)) + } + + // Make sure the new intermediate has replaced the old one in the active root, + // and that the root itself hasn't changed. + var newIntermediatePEM string + { + args := &structs.DCSpecificRequest{ + Datacenter: "secondary", + } + var reply structs.IndexedCARoots + require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply)) + require.Len(reply.Roots, 1) + require.Len(reply.Roots[0].IntermediateCerts, 1) + newIntermediatePEM = reply.Roots[0].IntermediateCerts[0] + require.NotEqual(oldIntermediatePEM, newIntermediatePEM) + require.Equal(reply.Roots[0].RootCert, rootCert.RootCert) + } + + // Verify the new config was set. + { + args := &structs.DCSpecificRequest{ + Datacenter: "secondary", + } + var reply structs.CAConfiguration + require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply)) + + 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) + } + + // Verify that new leaf certs get the new intermediate bundled + { + // Generate a CSR and request signing + spiffeId := connect.TestSpiffeIDServiceWithHostDC(t, "web", connect.TestClusterID+".consul", "secondary") + csr, _ := connect.TestCSR(t, spiffeId) + args := &structs.CASignRequest{ + Datacenter: "secondary", + CSR: csr, + } + var reply structs.IssuedCert + require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply)) + + // Verify the leaf cert has the new intermediate. + { + roots := x509.NewCertPool() + assert.True(roots.AppendCertsFromPEM([]byte(rootCert.RootCert))) + leaf, err := connect.ParseCert(reply.CertPEM) + require.NoError(err) + + intermediates := x509.NewCertPool() + require.True(intermediates.AppendCertsFromPEM([]byte(newIntermediatePEM))) + + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: roots, + Intermediates: intermediates, + }) + require.NoError(err) + } + + // Verify other fields + assert.Equal("web", reply.Service) + assert.Equal(spiffeId.URI().String(), reply.ServiceURI) + } +} + // Test CA signing func TestConnectCASign(t *testing.T) { t.Parallel() diff --git a/agent/consul/leader_connect.go b/agent/consul/leader_connect.go index f87641a1c..67c56520a 100644 --- a/agent/consul/leader_connect.go +++ b/agent/consul/leader_connect.go @@ -214,7 +214,7 @@ func (s *Server) initializeCA() error { if err := s.initializeSecondaryProvider(provider, roots); err != nil { return fmt.Errorf("error configuring provider: %v", err) } - if err := s.initializeSecondaryCA(provider, roots); err != nil { + if err := s.initializeSecondaryCA(provider, roots, nil); err != nil { return err } @@ -337,7 +337,7 @@ func (s *Server) initializeRootCA(provider ca.Provider, conf *structs.CAConfigur // intermediate. // It is being called while holding caProviderReconfigurationLock // which means it must never take that lock itself or call anything that does. -func (s *Server) initializeSecondaryCA(provider ca.Provider, primaryRoots structs.IndexedCARoots) error { +func (s *Server) initializeSecondaryCA(provider ca.Provider, primaryRoots structs.IndexedCARoots, config *structs.CAConfiguration) error { activeIntermediate, err := provider.ActiveIntermediate() if err != nil { return err @@ -432,19 +432,25 @@ func (s *Server) initializeSecondaryCA(provider ca.Provider, primaryRoots struct if err != nil { return err } + + // Determine whether a root update is needed, and persist the roots/config accordingly. + var newRoot *structs.CARoot if activeRoot == nil || activeRoot.ID != newActiveRoot.ID || newIntermediate { - if err := s.persistNewRoot(provider, newActiveRoot); err != nil { - return err - } + newRoot = newActiveRoot + } + if err := s.persistNewRootAndConfig(provider, newRoot, config); err != nil { + return err } s.setCAProvider(provider, newActiveRoot) return nil } -// persistNewRoot is being called while holding caProviderReconfigurationLock +// persistNewRootAndConfig is being called while holding caProviderReconfigurationLock // which means it must never take that lock itself or call anything that does. -func (s *Server) persistNewRoot(provider ca.Provider, newActiveRoot *structs.CARoot) error { +// If newActiveRoot is non-nil, it will be appended to the current roots list. +// If config is non-nil, it will be used to overwrite the existing config. +func (s *Server) persistNewRootAndConfig(provider ca.Provider, newActiveRoot *structs.CARoot, config *structs.CAConfiguration) error { connectLogger := s.loggers.Named(logging.Connect) state := s.fsm.State() idx, oldRoots, err := state.CARoots(nil) @@ -452,15 +458,23 @@ func (s *Server) persistNewRoot(provider ca.Provider, newActiveRoot *structs.CAR return err } - _, config, err := state.CAConfig(nil) + var newConf structs.CAConfiguration + _, storedConfig, err := state.CAConfig(nil) if err != nil { return err } - if config == nil { + if storedConfig == nil { return fmt.Errorf("local CA not initialized yet") } - newConf := *config - newConf.ClusterID = newActiveRoot.ExternalTrustDomain + if config != nil { + newConf = *config + } else { + newConf = *storedConfig + } + newConf.ModifyIndex = storedConfig.ModifyIndex + if newActiveRoot != nil { + newConf.ClusterID = newActiveRoot.ExternalTrustDomain + } // Persist any state the provider needs us to newConf.State, err = provider.State() @@ -468,21 +482,25 @@ func (s *Server) persistNewRoot(provider ca.Provider, newActiveRoot *structs.CAR return fmt.Errorf("error getting provider state: %v", err) } - // Copy the root list and append the new active root, updating the old root - // with the time it was rotated out. + // If there's a new active root, copy the root list and append it, updating + // the old root with the time it was rotated out. var newRoots structs.CARoots - for _, r := range oldRoots { - newRoot := *r - if newRoot.Active { - newRoot.Active = false - newRoot.RotatedOutAt = time.Now() + if newActiveRoot != nil { + for _, r := range oldRoots { + newRoot := *r + if newRoot.Active { + newRoot.Active = false + newRoot.RotatedOutAt = time.Now() + } + if newRoot.ExternalTrustDomain == "" { + newRoot.ExternalTrustDomain = config.ClusterID + } + newRoots = append(newRoots, &newRoot) } - if newRoot.ExternalTrustDomain == "" { - newRoot.ExternalTrustDomain = config.ClusterID - } - newRoots = append(newRoots, &newRoot) + newRoots = append(newRoots, newActiveRoot) + } else { + newRoots = oldRoots } - newRoots = append(newRoots, newActiveRoot) args := &structs.CARequest{ Op: structs.CAOpSetRootsAndConfig, @@ -748,7 +766,7 @@ func (s *Server) intermediateCertRenewalWatch(ctx context.Context) error { return err } - if err := s.persistNewRoot(provider, activeRoot); err != nil { + if err := s.persistNewRootAndConfig(provider, activeRoot, nil); err != nil { return err } @@ -808,7 +826,7 @@ func (s *Server) secondaryCARootWatch(ctx context.Context) error { // Run the secondary CA init routine to see if we need to request a new // intermediate. if s.configuredSecondaryCA() { - if err := s.initializeSecondaryCA(provider, roots); err != nil { + if err := s.initializeSecondaryCA(provider, roots, nil); err != nil { return fmt.Errorf("Failed to initialize the secondary CA: %v", err) } }