Identify issuer on revocation (#16763)

* Identify issuer on revocation

When we attempt to revoke a leaf certificate, we already parse all of
the issuers within the mount (to x509.Certificate) to ensure we don't
accidentally revoke an issuer via the leaf revocation endpoint. We can
reuse this information to associate the issuer (via issuer/subject
comparison and signature checking) to the revoked cert in its revocation
info. This will help OCSP, avoiding the case where the OCSP handler
needs to associate a certificate to its issuer.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add test to ensure issuers are identified

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

builtin/logical/pki/crl_test.go

@@ -3,6 +3,7 @@ package pki
 import (
 	"context"
 	"encoding/asn1"
+	"encoding/json"
 	"fmt"
 	"strings"
 	"testing"
@@ -806,6 +807,7 @@ func TestAutoRebuild(t *testing.T) {
 		LogicalBackends: map[string]logical.Factory{
 			"pki": Factory,
 		},
+		EnableRaw: true,
 	}
 	oldPeriod := vault.SetRollbackPeriodForTesting(newPeriod)
 	cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
@@ -827,12 +829,16 @@ func TestAutoRebuild(t *testing.T) {
 	require.NoError(t, err)
 
 	// Generate root.
-	_, err = client.Logical().Write("pki/root/generate/internal", map[string]interface{}{
+	resp, err := client.Logical().Write("pki/root/generate/internal", map[string]interface{}{
 		"ttl":         "40h",
 		"common_name": "Root X1",
 		"key_type":    "ec",
 	})
 	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotEmpty(t, resp.Data)
+	require.NotEmpty(t, resp.Data["issuer_id"])
+	rootIssuer := resp.Data["issuer_id"].(string)
 
 	// Setup a testing role.
 	_, err = client.Logical().Write("pki/roles/local-testing", map[string]interface{}{
@@ -844,7 +850,7 @@ func TestAutoRebuild(t *testing.T) {
 
 	// Regression test: ensure we respond with the default values for CRL
 	// config when we haven't set any values yet.
-	resp, err := client.Logical().Read("pki/config/crl")
+	resp, err = client.Logical().Read("pki/config/crl")
 	require.NoError(t, err)
 	require.NotNil(t, resp)
 	require.NotNil(t, resp.Data)
@@ -904,6 +910,57 @@ func TestAutoRebuild(t *testing.T) {
 	})
 	require.NoError(t, err)
 
+	// Now, we want to test the issuer identification on revocation. This
+	// only happens as a distinct "step" when CRL building isn't done on
+	// each reovcation. Pull the storage from the cluster and verify the
+	// revInfo contains a matching cert. Some of this code is cribbed from
+	// kvv2_upgrade_test.go.
+	var pkiMount string
+	storage := cluster.Cores[0].UnderlyingStorage
+	mounts, err := storage.List(ctx, "logical/")
+	require.NoError(t, err)
+	require.NotEmpty(t, mounts)
+	for _, mount := range mounts {
+		// For whatever reason, OIDC gets provisioned as the first mount,
+		// but I'm not convinced that's a stable list. Let's look inside
+		// each mount until we find a revoked certs folder that we'd expect
+		// if its a real PKI mount. This is because mounts are just UUID
+		// strings...
+		mountFolders, err := storage.List(ctx, "logical/"+mount)
+		require.NoError(t, err)
+
+		isPkiMount := false
+		for _, folder := range mountFolders {
+			if folder == "revoked/" {
+				isPkiMount = true
+				break
+			}
+		}
+
+		if isPkiMount {
+			pkiMount = mount
+			break
+		}
+	}
+	require.NotEmpty(t, pkiMount)
+	revEntryPath := "logical/" + pkiMount + revokedPath + strings.ReplaceAll(newLeafSerial, ":", "-")
+	// storage above is a physical storage copy, not a logical storage. This
+	// difference means, if we were to do a storage.Get(...) on the above
+	// path, we'd read the barrier-encrypted value. This is less than useful
+	// for decoding, and fetching the proper storage view is a touch much
+	// work. So, assert EnableRaw above and (ab)use it here.
+	resp, err = client.Logical().Read("sys/raw/" + revEntryPath)
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotNil(t, resp.Data)
+	require.NotEmpty(t, resp.Data["value"])
+	revEntryValue := resp.Data["value"].(string)
+	fmt.Println(resp)
+	var revInfo revocationInfo
+	err = json.Unmarshal([]byte(revEntryValue), &revInfo)
+	require.NoError(t, err)
+	require.Equal(t, revInfo.CertificateIssuer, issuerID(rootIssuer))
+
 	// Serial should not appear on CRL.
 	crl = getCrlCertificateList(t, client, "pki")
 	thisCRLNumber := crl.Version

builtin/logical/pki/crl_util.go

@@ -253,6 +253,7 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st
 		issuers = []issuerID{legacyBundleShimID}
 	}
 
+	issuerIDCertMap := make(map[issuerID]*x509.Certificate, len(issuers))
 	for _, issuer := range issuers {
 		_, bundle, caErr := sc.fetchCertBundleByIssuerId(issuer, false)
 		if caErr != nil {
@@ -281,6 +282,8 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st
 		if colonSerial == serialFromCert(parsedBundle.Certificate) {
 			return logical.ErrorResponse(fmt.Sprintf("adding issuer (id: %v) to its own CRL is not allowed", issuer)), nil
 		}
+
+		issuerIDCertMap[issuer] = parsedBundle.Certificate
 	}
 
 	alreadyRevoked := false
@@ -352,6 +355,10 @@ func revokeCert(ctx context.Context, b *backend, req *logical.Request, serial st
 		revInfo.RevocationTime = currTime.Unix()
 		revInfo.RevocationTimeUTC = currTime.UTC()
 
+		// We may not find an issuer with this certificate; that's fine so
+		// ignore the return value.
+		associateRevokedCertWithIsssuer(&revInfo, cert, issuerIDCertMap)
+
 		revEntry, err = logical.StorageEntryJSON(revokedPath+normalizeSerial(serial), revInfo)
 		if err != nil {
 			return nil, fmt.Errorf("error creating revocation entry")
@@ -626,6 +633,20 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
 	return nil
 }
 
+func associateRevokedCertWithIsssuer(revInfo *revocationInfo, revokedCert *x509.Certificate, issuerIDCertMap map[issuerID]*x509.Certificate) bool {
+	for issuerId, issuerCert := range issuerIDCertMap {
+		if bytes.Equal(revokedCert.RawIssuer, issuerCert.RawSubject) {
+			if err := revokedCert.CheckSignatureFrom(issuerCert); err == nil {
+				// Valid mapping. Add it to the specified entry.
+				revInfo.CertificateIssuer = issuerId
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
 func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCertMap map[issuerID]*x509.Certificate) ([]pkix.RevokedCertificate, map[issuerID][]pkix.RevokedCertificate, error) {
 	var unassignedCerts []pkix.RevokedCertificate
 	revokedCertsMap := make(map[issuerID][]pkix.RevokedCertificate)
@@ -687,23 +708,13 @@ func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCe
 		}
 
 		// Now we need to assign the revoked certificate to an issuer.
-		foundParent := false
-		for issuerId, issuerCert := range issuerIDCertMap {
-			if bytes.Equal(revokedCert.RawIssuer, issuerCert.RawSubject) {
-				if err := revokedCert.CheckSignatureFrom(issuerCert); err == nil {
-					// Valid mapping. Add it to the specified entry.
-					revokedCertsMap[issuerId] = append(revokedCertsMap[issuerId], newRevCert)
-					revInfo.CertificateIssuer = issuerId
-					foundParent = true
-					break
-				}
-			}
-		}
-
+		foundParent := associateRevokedCertWithIsssuer(&revInfo, revokedCert, issuerIDCertMap)
 		if !foundParent {
 			// If the parent isn't found, add it to the unassigned bucket.
 			unassignedCerts = append(unassignedCerts, newRevCert)
 		} else {
+			revokedCertsMap[revInfo.CertificateIssuer] = append(revokedCertsMap[revInfo.CertificateIssuer], newRevCert)
+
 			// When the CertificateIssuer field wasn't found on the existing
 			// entry (or was invalid), and we've found a new value for it,
 			// we should update the entry to make future CRL builds faster.