From 50c879923cc70cb22b48f2400800360187129ebe Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Thu, 9 Jan 2020 09:28:16 -0600 Subject: [PATCH] connect: ensure that updates to the secondary root CA configuration use the correct signing key ID values for comparison (#7012) Fixes #6886 --- agent/consul/leader_connect.go | 44 ++++++++++++++++++++++++----- agent/consul/leader_connect_test.go | 36 ++++++++++++----------- 2 files changed, 56 insertions(+), 24 deletions(-) diff --git a/agent/consul/leader_connect.go b/agent/consul/leader_connect.go index 1555e0dc41..8196c9f101 100644 --- a/agent/consul/leader_connect.go +++ b/agent/consul/leader_connect.go @@ -319,7 +319,7 @@ func (s *Server) initializeRootCA(provider ca.Provider, conf *structs.CAConfigur // initializeSecondaryCA runs the routine for generating an intermediate CA CSR and getting // it signed by the primary DC if the root CA of the primary DC has changed since the last // intermediate. -func (s *Server) initializeSecondaryCA(provider ca.Provider, roots structs.IndexedCARoots) error { +func (s *Server) initializeSecondaryCA(provider ca.Provider, primaryRoots structs.IndexedCARoots) error { activeIntermediate, err := provider.ActiveIntermediate() if err != nil { return err @@ -328,8 +328,19 @@ func (s *Server) initializeSecondaryCA(provider ca.Provider, roots structs.Index var ( storedRootID string expectedSigningKeyID string + currentSigningKeyID string + activeSecondaryRoot *structs.CARoot ) if activeIntermediate != "" { + // In the event that we already have an intermediate, we must have + // already replicated some primary root information locally, so check + // to see if we're up to date by fetching the rootID and the + // signingKeyID used in the secondary. + // + // Note that for the same rootID the primary representation of the root + // will have a different SigningKeyID field than the secondary + // representation of the same root. This is because it's derived from + // the intermediate which is different in all datacenters. storedRoot, err := provider.ActiveRoot() if err != nil { return err @@ -337,7 +348,7 @@ func (s *Server) initializeSecondaryCA(provider ca.Provider, roots structs.Index storedRootID, err = connect.CalculateCertFingerprint(storedRoot) if err != nil { - return fmt.Errorf("error parsing root fingerprint: %v, %#v", err, roots) + return fmt.Errorf("error parsing root fingerprint: %v, %#v", err, primaryRoots) } intermediateCert, err := connect.ParseCert(activeIntermediate) @@ -345,11 +356,25 @@ func (s *Server) initializeSecondaryCA(provider ca.Provider, roots structs.Index return fmt.Errorf("error parsing active intermediate cert: %v", err) } expectedSigningKeyID = connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId) + + // This will fetch the secondary's exact current representation of the + // active root. Note that this data should only be used if the IDs + // match, otherwise it's out of date and should be regenerated. + _, activeSecondaryRoot, err = s.fsm.State().CARootActive(nil) + if err != nil { + return err + } + if activeSecondaryRoot != nil { + currentSigningKeyID = activeSecondaryRoot.SigningKeyID + } } + // Determine which of the provided PRIMARY representations of roots is the + // active one. We'll use this as a template to generate any new root + // representations meant for this secondary. var newActiveRoot *structs.CARoot - for _, root := range roots.Roots { - if root.ID == roots.ActiveRootID && root.Active { + for _, root := range primaryRoots.Roots { + if root.ID == primaryRoots.ActiveRootID && root.Active { newActiveRoot = root break } @@ -361,13 +386,13 @@ func (s *Server) initializeSecondaryCA(provider ca.Provider, roots structs.Index // Get a signed intermediate from the primary DC if the provider // hasn't been initialized yet or if the primary's root has changed. needsNewIntermediate := false - if activeIntermediate == "" || storedRootID != roots.ActiveRootID { + if activeIntermediate == "" || storedRootID != primaryRoots.ActiveRootID { needsNewIntermediate = true } // Also we take this opportunity to correct an incorrectly persisted SigningKeyID // in secondary datacenters (see PR-6513). - if expectedSigningKeyID != "" && newActiveRoot.SigningKeyID != expectedSigningKeyID { + if expectedSigningKeyID != "" && currentSigningKeyID != expectedSigningKeyID { needsNewIntermediate = true } @@ -394,12 +419,17 @@ func (s *Server) initializeSecondaryCA(provider ca.Provider, roots structs.Index return fmt.Errorf("error parsing intermediate cert: %v", err) } - // Append the new intermediate to our local active root entry. + // Append the new intermediate to our local active root entry. This is + // where the root representations start to diverge. newActiveRoot.IntermediateCerts = append(newActiveRoot.IntermediateCerts, intermediatePEM) newActiveRoot.SigningKeyID = connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId) newIntermediate = true s.logger.Printf("[INFO] connect: received new intermediate certificate from primary datacenter") + } else { + // Discard the primary's representation since our local one is + // sufficiently up to date. + newActiveRoot = activeSecondaryRoot } // Update the roots list in the state store if there's a new active root. diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index fc83dc0c0c..9c3049e4bb 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -334,36 +334,29 @@ func TestLeader_SecondaryCA_FixSigningKeyID_via_IntermediateRefresh(t *testing.T // Restore the pre-1.6.1 behavior of the SigningKeyID not being derived // from the intermediates. + var secondaryRootSigningKeyID string { - require := require.New(t) - state := s2pre.fsm.State() // Get the highest index - idx, roots, err := state.CARoots(nil) - require.NoError(err) + idx, activeSecondaryRoot, err := state.CARootActive(nil) + require.NoError(t, err) + require.NotNil(t, activeSecondaryRoot) - var activeRoot *structs.CARoot - for _, root := range roots { - if root.Active { - activeRoot = root - } - } - require.NotNil(activeRoot) - - rootCert, err := connect.ParseCert(activeRoot.RootCert) - require.NoError(err) + rootCert, err := connect.ParseCert(activeSecondaryRoot.RootCert) + require.NoError(t, err) // Force this to be derived just from the root, not the intermediate. - activeRoot.SigningKeyID = connect.EncodeSigningKeyID(rootCert.SubjectKeyId) + secondaryRootSigningKeyID = connect.EncodeSigningKeyID(rootCert.SubjectKeyId) + activeSecondaryRoot.SigningKeyID = secondaryRootSigningKeyID // Store the root cert in raft resp, err := s2pre.raftApply(structs.ConnectCARequestType, &structs.CARequest{ Op: structs.CAOpSetRoots, Index: idx, - Roots: []*structs.CARoot{activeRoot}, + Roots: []*structs.CARoot{activeSecondaryRoot}, }) - require.NoError(err) + require.NoError(t, err) if respErr, ok := resp.(error); ok { t.Fatalf("respErr: %v", respErr) } @@ -372,6 +365,7 @@ func TestLeader_SecondaryCA_FixSigningKeyID_via_IntermediateRefresh(t *testing.T // Shutdown s2pre and restart it to trigger the secondary CA init to correct // the SigningKeyID. s2pre.Shutdown() + dir2, s2 := testServerWithConfig(t, func(c *Config) { c.DataDir = s2pre.config.DataDir c.Datacenter = "dc2" @@ -401,7 +395,15 @@ func TestLeader_SecondaryCA_FixSigningKeyID_via_IntermediateRefresh(t *testing.T // Force this to be derived just from the root, not the intermediate. expect := connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId) + + // The in-memory representation was saw the correction via a setCAProvider call. require.Equal(r, expect, activeRoot.SigningKeyID) + + // The state store saw the correction, too. + _, activeSecondaryRoot, err := s2.fsm.State().CARootActive(nil) + require.NoError(r, err) + require.NotNil(r, activeSecondaryRoot) + require.Equal(r, expect, activeSecondaryRoot.SigningKeyID) }) }