ca: fix stored CARoot representation with Vault provider

We were not adding the local signing cert to the CARoot. This commit
fixes that bug, and also adds support for fixing existing CARoot on
upgrade.

Also update the tests for both primary and secondary to be more strict.
Check the SigningKeyID is correct after initialization and rotation.

.changelog/11671.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+ca: fixes a bug that caused the intermediate cert used to sign leaf certs to be missing from the /connect/ca/roots API response when the Vault provider was used.
+```

agent/consul/leader_connect_ca.go

@@ -520,12 +520,26 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf
 		}
 	}
 
+	var rootUpdateRequired bool
+
 	// Versions prior to 1.9.3, 1.8.8, and 1.7.12 incorrectly used the primary
 	// rootCA's subjectKeyID here instead of the intermediate. For
 	// provider=consul this didn't matter since there are no intermediates in
 	// the primaryDC, but for vault it does matter.
 	expectedSigningKeyID := connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId)
-	needsSigningKeyUpdate := (rootCA.SigningKeyID != expectedSigningKeyID)
+	if rootCA.SigningKeyID != expectedSigningKeyID {
+		c.logger.Info("Correcting stored CARoot values",
+			"previous-signing-key", rootCA.SigningKeyID, "updated-signing-key", expectedSigningKeyID)
+		rootCA.SigningKeyID = expectedSigningKeyID
+		rootUpdateRequired = true
+	}
+
+	// Add the local leaf signing cert to the rootCA struct. This handles both
+	// upgrades of existing state, and new rootCA.
+	if rootCA.LeafSigningCert() != interPEM {
+		rootCA.IntermediateCerts = append(rootCA.IntermediateCerts, interPEM)
+		rootUpdateRequired = true
+	}
 
 	// Check if the CA root is already initialized and exit if it is,
 	// adding on any existing intermediate certs since they aren't directly
@@ -537,26 +551,21 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf
 	if err != nil {
 		return err
 	}
-	if activeRoot != nil && needsSigningKeyUpdate {
+	if activeRoot != nil && !rootUpdateRequired {
-		c.logger.Info("Correcting stored SigningKeyID value", "previous", rootCA.SigningKeyID, "updated", expectedSigningKeyID)
-
-	} else if activeRoot != nil && !needsSigningKeyUpdate {
 		// This state shouldn't be possible to get into because we update the root and
 		// CA config in the same FSM operation.
 		if activeRoot.ID != rootCA.ID {
 			return fmt.Errorf("stored CA root %q is not the active root (%s)", rootCA.ID, activeRoot.ID)
 		}
 
+		// TODO: why doesn't this c.setCAProvider(provider, activeRoot) ?
 		rootCA.IntermediateCerts = activeRoot.IntermediateCerts
 		c.setCAProvider(provider, rootCA)
 
+		c.logger.Info("initialized primary datacenter CA from existing CARoot with provider", "provider", conf.Provider)
 		return nil
 	}
 
-	if needsSigningKeyUpdate {
-		rootCA.SigningKeyID = expectedSigningKeyID
-	}
-
 	// Get the highest index
 	idx, _, err := state.CARoots(nil)
 	if err != nil {

agent/consul/leader_connect_test.go

@@ -396,23 +396,34 @@ func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) {
 	provider, _ := getCAProviderWithLock(s1)
 	intermediatePEM, err := provider.ActiveIntermediate()
 	require.NoError(err)
-	_, err = connect.ParseCert(intermediatePEM)
+	intermediateCert, err := connect.ParseCert(intermediatePEM)
 	require.NoError(err)
 
+	// Check that the state store has the correct intermediate
+	store := s1.caManager.delegate.State()
+	_, activeRoot, err := store.CARootActive(nil)
+	require.NoError(err)
+	require.Equal(intermediatePEM, activeRoot.LeafSigningCert())
+	require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID)
+
 	// Wait for dc1's intermediate to be refreshed.
 	// It is possible that test fails when the blocking query doesn't return.
 	retry.Run(t, func(r *retry.R) {
 		provider, _ = getCAProviderWithLock(s1)
 		newIntermediatePEM, err := provider.ActiveIntermediate()
 		r.Check(err)
-		_, err = connect.ParseCert(intermediatePEM)
-		r.Check(err)
 		if newIntermediatePEM == intermediatePEM {
 			r.Fatal("not a renewed intermediate")
 		}
+		intermediateCert, err = connect.ParseCert(newIntermediatePEM)
+		r.Check(err)
 		intermediatePEM = newIntermediatePEM
 	})
+
+	_, activeRoot, err = store.CARootActive(nil)
 	require.NoError(err)
+	require.Equal(intermediatePEM, activeRoot.LeafSigningCert())
+	require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID)
 
 	// Get the root from dc1 and validate a chain of:
 	// dc1 leaf -> dc1 intermediate -> dc1 root
@@ -439,6 +450,8 @@ func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) {
 	// Check that the leaf signed by the new intermediate can be verified using the
 	// returned cert chain (signed intermediate + remote root).
 	intermediatePool := x509.NewCertPool()
+	// TODO: do not explicitly add the intermediatePEM, we should have it available
+	// from leafPEM. Use connect.ParseLeafCerts to do the right thing.
 	intermediatePool.AppendCertsFromPEM([]byte(intermediatePEM))
 	rootPool := x509.NewCertPool()
 	rootPool.AppendCertsFromPEM([]byte(caRoot.RootCert))
@@ -515,10 +528,10 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 	secondaryProvider, _ := getCAProviderWithLock(s2)
 	intermediatePEM, err := secondaryProvider.ActiveIntermediate()
 	require.NoError(err)
-	cert, err := connect.ParseCert(intermediatePEM)
+	intermediateCert, err := connect.ParseCert(intermediatePEM)
 	require.NoError(err)
-	currentCertSerialNumber := cert.SerialNumber
+	currentCertSerialNumber := intermediateCert.SerialNumber
-	currentCertAuthorityKeyId := cert.AuthorityKeyId
+	currentCertAuthorityKeyId := intermediateCert.AuthorityKeyId
 
 	// Capture the current root
 	var originalRoot *structs.CARoot
@@ -532,6 +545,12 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 	waitForActiveCARoot(t, s1, originalRoot)
 	waitForActiveCARoot(t, s2, originalRoot)
 
+	store := s2.fsm.State()
+	_, activeRoot, err := store.CARootActive(nil)
+	require.NoError(err)
+	require.Equal(intermediatePEM, activeRoot.LeafSigningCert())
+	require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID)
+
 	// Wait for dc2's intermediate to be refreshed.
 	// It is possible that test fails when the blocking query doesn't return.
 	// When https://github.com/hashicorp/consul/pull/3777 is merged
@@ -548,8 +567,13 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 			currentCertAuthorityKeyId = cert.AuthorityKeyId
 			r.Fatal("not a renewed intermediate")
 		}
+		intermediateCert = cert
 	})
+
+	_, activeRoot, err = store.CARootActive(nil)
 	require.NoError(err)
+	require.Equal(intermediatePEM, activeRoot.LeafSigningCert())
+	require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID)
 
 	// Get the root from dc1 and validate a chain of:
 	// dc2 leaf -> dc2 intermediate -> dc1 root
@@ -570,17 +594,19 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 	leafPEM, err := secondaryProvider.Sign(leafCsr)
 	require.NoError(err)
 
-	cert, err = connect.ParseCert(leafPEM)
+	intermediateCert, err = connect.ParseCert(leafPEM)
 	require.NoError(err)
 
 	// Check that the leaf signed by the new intermediate can be verified using the
 	// returned cert chain (signed intermediate + remote root).
 	intermediatePool := x509.NewCertPool()
+	// TODO: do not explicitly add the intermediatePEM, we should have it available
+	// from leafPEM. Use connect.ParseLeafCerts to do the right thing.
 	intermediatePool.AppendCertsFromPEM([]byte(intermediatePEM))
 	rootPool := x509.NewCertPool()
 	rootPool.AppendCertsFromPEM([]byte(caRoot.RootCert))
 
-	_, err = cert.Verify(x509.VerifyOptions{
+	_, err = intermediateCert.Verify(x509.VerifyOptions{
 		Intermediates: intermediatePool,
 		Roots:         rootPool,
 	})

agent/structs/connect_ca.go

@@ -142,6 +142,23 @@ func (c *CARoot) Clone() *CARoot {
 	return &newCopy
 }
 
+// LeafSigningCert returns the PEM encoded certificate that should be used to
+// sign leaf certificates in the local datacenter. The SubjectKeyId of the
+// returned cert should always match the SigningKeyID of the CARoot.
+//
+// This method has no knowledge of the provider, so it makes assumptions about
+// which cert is used based on established convention. Generally you should
+// check CAManager.isIntermediateUsedToSignLeaf first.
+//
+// If there are no IntermediateCerts the RootCert is returned. If there are
+// IntermediateCerts the last one in the list is returned.
+func (c *CARoot) LeafSigningCert() string {
+	if len(c.IntermediateCerts) == 0 {
+		return c.RootCert
+	}
+	return c.IntermediateCerts[len(c.IntermediateCerts)-1]
+}
+
 // CARoots is a list of CARoot structures.
 type CARoots []*CARoot