Merge pull request #11671 from hashicorp/dnephin/ca-fix-storing-vault-intermediate

ca: fix storing the leaf signing cert with Vault provider

.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

@@ -517,12 +517,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 c.getLeafSigningCertFromRoot(rootCA) != 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
@@ -534,26 +548,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 {
@@ -577,6 +586,22 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf
 	return nil
 }
 
+// getLeafSigningCertFromRoot 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.
+//
+// TODO: fix the data model so that we don't need this complicated lookup to
+// find the leaf signing cert. See github.com/hashicorp/consul/issues/11347.
+func (c *CAManager) getLeafSigningCertFromRoot(root *structs.CARoot) string {
+	if !c.isIntermediateUsedToSignLeaf() {
+		return root.RootCert
+	}
+	if len(root.IntermediateCerts) == 0 {
+		return ""
+	}
+	return root.IntermediateCerts[len(root.IntermediateCerts)-1]
+}
+
 // secondaryInitializeIntermediateCA 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. It should only be called while the state lock is held by setting the state
@@ -1133,10 +1158,8 @@ func (c *CAManager) RenewIntermediate(ctx context.Context, isPrimary bool) error
 
 	// If this is the primary, check if this is a provider that uses an intermediate cert. If
 	// it isn't, we don't need to check for a renewal.
-	if isPrimary {
+	if isPrimary && !primaryUsesIntermediate(provider) {
-		if _, ok := provider.(ca.PrimaryUsesIntermediate); !ok {
+		return nil
-			return nil
-		}
 	}
 
 	activeIntermediate, err := provider.ActiveIntermediate()
@@ -1520,3 +1543,16 @@ func (c *CAManager) checkExpired(pem string) error {
 	}
 	return nil
 }
+
+func primaryUsesIntermediate(provider ca.Provider) bool {
+	_, ok := provider.(ca.PrimaryUsesIntermediate)
+	return ok
+}
+
+func (c *CAManager) isIntermediateUsedToSignLeaf() bool {
+	if c.serverConf.Datacenter != c.serverConf.PrimaryDatacenter {
+		return true
+	}
+	provider, _ := c.getCAProvider()
+	return primaryUsesIntermediate(provider)
+}

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, s1.caManager.getLeafSigningCertFromRoot(activeRoot))
+	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, s1.caManager.getLeafSigningCertFromRoot(activeRoot))
+	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, s2.caManager.getLeafSigningCertFromRoot(activeRoot))
+	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, s2.caManager.getLeafSigningCertFromRoot(activeRoot))
+	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

@@ -86,11 +86,20 @@ type CARoot struct {
 	NotBefore time.Time
 	NotAfter  time.Time
 
-	// RootCert is the PEM-encoded public certificate.
+	// RootCert is the PEM-encoded public certificate for the root CA. The
+	// certificate is the same for all federated clusters.
 	RootCert string
 
 	// IntermediateCerts is a list of PEM-encoded intermediate certs to
-	// attach to any leaf certs signed by this CA.
+	// attach to any leaf certs signed by this CA. The list may include a
+	// certificate cross-signed by an old root CA, any subordinate CAs below the
+	// root CA, and the intermediate CA used to sign leaf certificates in the
+	// local Datacenter.
+	//
+	// If the provider which created this root uses an intermediate to sign
+	// leaf certificates (Vault provider), or this is a secondary Datacenter then
+	// the intermediate used to sign leaf certificates will be the last in the
+	// list.
 	IntermediateCerts []string
 
 	// SigningCert is the PEM-encoded signing certificate and SigningKey