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

@@ -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.

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)
 	})
 }