Cleanup changes around issuer revocation

builtin/logical/pki/chain_test.go

@@ -535,7 +535,7 @@ func (c CBIssueLeaf) RevokeLeaf(t testing.TB, b *backend, s logical.Storage, kno
 		t.Fatalf("failed to revoke issued certificate (%v) under role %v / issuer %v: expected response parameter revocation_time was missing from response:\n%v", api_serial, c.Role, c.Issuer, resp.Data)
 	}
 
-	if !hasCRL && isDefault {
+	if !hasCRL {
 		// Nothing further we can test here. We could re-enable CRL building
 		// and check that it works, but that seems like a stretch. Other
 		// issuers might be functionally the same as this issuer (and thus
@@ -614,7 +614,7 @@ func (c CBIssueLeaf) RevokeLeaf(t testing.TB, b *backend, s logical.Storage, kno
 			}
 		}
 
-		t.Fatalf("expected to find certificate with serial [%v] on issuer %v's CRL but was missing: %v revoked certs\n\nCRL:\n[%v]\n\nLeaf:\n[%v]\n\nIssuer:\n[%v]\n", api_serial, c.Issuer, len(crl.TBSCertList.RevokedCertificates), raw_crl, raw_cert, raw_issuer)
+		t.Fatalf("expected to find certificate with serial [%v] on issuer %v's CRL but was missing: %v revoked certs\n\nCRL:\n[%v]\n\nLeaf:\n[%v]\n\nIssuer (hasCRL: %v):\n[%v]\n", api_serial, c.Issuer, len(crl.TBSCertList.RevokedCertificates), raw_crl, raw_cert, hasCRL, raw_issuer)
 	}
 }
 

builtin/logical/pki/crl_test.go

@@ -694,8 +694,14 @@ func TestIssuerRevocation(t *testing.T) {
 	_, err = CBRead(b, s, "crl/rotate")
 	require.NoError(t, err)
 
+	// Ensure the old cert isn't on its own CRL.
+	crl := getParsedCrlFromBackend(t, b, s, "issuer/root2/crl/der")
+	if requireSerialNumberInCRL(nil, crl.TBSCertList, revokedRootSerial) {
+		t.Fatalf("the serial number %v should not be on its own CRL as self-CRL appearance should not occur", revokedRootSerial)
+	}
+
 	// Ensure the old cert isn't on the one's CRL.
-	crl := getParsedCrlFromBackend(t, b, s, "issuer/root/crl/der")
+	crl = getParsedCrlFromBackend(t, b, s, "issuer/root/crl/der")
 	if requireSerialNumberInCRL(nil, crl.TBSCertList, revokedRootSerial) {
 		t.Fatalf("the serial number %v should not be on %v's CRL as they're separate roots", revokedRootSerial, oldRootSerial)
 	}
@@ -807,6 +813,8 @@ func TestAutoRebuild(t *testing.T) {
 		LogicalBackends: map[string]logical.Factory{
 			"pki": Factory,
 		},
+		// See notes below about usage of /sys/raw for reading cluster
+		// storage without barrier encryption.
 		EnableRaw: true,
 	}
 	oldPeriod := vault.SetRollbackPeriodForTesting(newPeriod)
@@ -912,43 +920,23 @@ func TestAutoRebuild(t *testing.T) {
 
 	// 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/")
+	// each revocation. Pull the storage from the cluster (via the sys/raw
+	// endpoint which requires the mount UUID) and verify the revInfo contains
+	// a matching issuer.
+	resp, err = client.Logical().Read("sys/mounts/pki")
 	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.NotNil(t, resp)
+	require.NotNil(t, resp.Data)
+	require.NotEmpty(t, resp.Data["uuid"])
+	pkiMount := resp.Data["uuid"].(string)
 	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.
+	revEntryPath := "logical/" + pkiMount + "/" + revokedPath + strings.ReplaceAll(newLeafSerial, ":", "-")
+
+	// storage from cluster.Core[0] 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)

builtin/logical/pki/crl_util.go

@@ -440,6 +440,18 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
 		return fmt.Errorf("error building CRL: while updating config: %v", err)
 	}
 
+	if globalCRLConfig.Disable && !forceNew {
+		// We build a single long-lived empty CRL in the event that we disable
+		// the CRL, but we don't keep updating it with newer, more-valid empty
+		// CRLs in the event that we later re-enable it. This is a historical
+		// behavior.
+		//
+		// So, since tidy can now associate issuers on revocation entries, we
+		// can skip the rest of this function and exit early without updating
+		// anything.
+		return nil
+	}
+
 	if !b.useLegacyBundleCaStorage() {
 		issuers, err = sc.listIssuers()
 		if err != nil {
@@ -479,11 +491,20 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
 			continue
 		}
 
-		// Skip entries which aren't enabled for CRL signing.
-		if err := thisEntry.EnsureUsage(CRLSigningUsage); err != nil {
-			continue
-		}
-
+		// n.b.: issuer usage check has been delayed. This occurred because
+		// we want to ensure any issuer (representative of a larger set) can
+		// be used to associate revocation entries and we won't bother
+		// rewriting that entry (causing churn) if the particular selected
+		// issuer lacks CRL signing capabilities.
+		//
+		// The result is that this map (and the other maps) contain all the
+		// issuers we know about, and only later do we check crlSigning before
+		// choosing our representative.
+		//
+		// The other side effect (making this not compatible with Vault 1.11
+		// behavior) is that _identified_ certificates whose issuer set is
+		// not allowed for crlSigning will no longer appear on the default
+		// issuer's CRL.
 		issuerIDEntryMap[issuer] = thisEntry
 
 		thisCert, err := thisEntry.GetCertificate()
@@ -529,10 +550,24 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
 			}
 
 			var revokedCerts []pkix.RevokedCertificate
-			representative := issuersSet[0]
+			representative := issuerID("")
 			var crlIdentifier crlID
 			var crlIdIssuer issuerID
 			for _, issuerId := range issuersSet {
+				// Skip entries which aren't enabled for CRL signing. We don't
+				// particularly care which issuer is ultimately chosen as the
+				// set representative for signing at this point, other than
+				// that it has crl-signing usage.
+				if err := issuerIDEntryMap[issuerId].EnsureUsage(CRLSigningUsage); err != nil {
+					continue
+				}
+
+				// Prefer to use the default as the representative of this
+				// set, if it is a member.
+				//
+				// If it is, we'll also pull in the unassigned certs to remain
+				// compatible with Vault's earlier, potentially questionable
+				// behavior.
 				if issuerId == config.DefaultIssuerId {
 					if len(unassignedCerts) > 0 {
 						revokedCerts = append(revokedCerts, unassignedCerts...)
@@ -541,10 +576,18 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
 					representative = issuerId
 				}
 
+				// Otherwise, use any other random issuer if we've not yet
+				// chosen one.
+				if representative == issuerID("") {
+					representative = issuerId
+				}
+
+				// Pull in the revoked certs associated with this member.
 				if thisRevoked, ok := revokedCertsMap[issuerId]; ok && len(thisRevoked) > 0 {
 					revokedCerts = append(revokedCerts, thisRevoked...)
 				}
 
+				// Finally, check our crlIdentifier.
 				if thisCRLId, ok := crlConfig.IssuerIDCRLMap[issuerId]; ok && len(thisCRLId) > 0 {
 					if len(crlIdentifier) > 0 && crlIdentifier != thisCRLId {
 						return fmt.Errorf("error building CRLs: two issuers with same keys/subjects (%v vs %v) have different internal CRL IDs: %v vs %v", issuerId, crlIdIssuer, thisCRLId, crlIdentifier)
@@ -555,6 +598,13 @@ func buildCRLs(ctx context.Context, b *backend, req *logical.Request, forceNew b
 				}
 			}
 
+			if representative == "" {
+				// Skip this set for the time being; while we have valid
+				// issuers and associated keys, this occurred because we lack
+				// crl-signing usage on all issuers in this set.
+				continue
+			}
+
 			if len(crlIdentifier) == 0 {
 				// Create a new random UUID for this CRL if none exists.
 				crlIdentifier = genCRLId()
@@ -656,6 +706,13 @@ func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCe
 		return nil, nil, errutil.InternalError{Err: fmt.Sprintf("error fetching list of revoked certs: %s", err)}
 	}
 
+	// Build a mapping of issuer serial -> certificate.
+	issuerSerialCertMap := make(map[string][]*x509.Certificate, len(issuerIDCertMap))
+	for _, cert := range issuerIDCertMap {
+		serialStr := serialFromCert(cert)
+		issuerSerialCertMap[serialStr] = append(issuerSerialCertMap[serialStr], cert)
+	}
+
 	for _, serial := range revokedSerials {
 		var revInfo revocationInfo
 		revokedEntry, err := req.Storage.Get(ctx, revokedPath+serial)
@@ -682,6 +739,34 @@ func getRevokedCertEntries(ctx context.Context, req *logical.Request, issuerIDCe
 			return nil, nil, errutil.InternalError{Err: fmt.Sprintf("unable to parse stored revoked certificate with serial %s: %s", serial, err)}
 		}
 
+		// We want to skip issuer certificate's revocationEntries for two
+		// reasons:
+		//
+		// 1. We canonically use augmentWithRevokedIssuers to handle this
+		//    case and this entry is just a backup. This prevents the issue
+		//    of duplicate serial numbers on the CRL from both paths.
+		// 2. We want to avoid a root's serial from appearing on its own
+		//    CRL. If it is a cross-signed or re-issued variant, this is OK,
+		//    but in the case we mark the root itself as "revoked", we want
+		//    to avoid it appearing on the CRL as that is definitely
+		//    undefined/little-supported behavior.
+		//
+		// This hash map lookup should be faster than byte comparison against
+		// each issuer proactively.
+		if candidates, present := issuerSerialCertMap[serialFromCert(revokedCert)]; present {
+			revokedCertIsIssuer := false
+			for _, candidate := range candidates {
+				if bytes.Equal(candidate.Raw, revokedCert.Raw) {
+					revokedCertIsIssuer = true
+					break
+				}
+			}
+
+			if revokedCertIsIssuer {
+				continue
+			}
+		}
+
 		// NOTE: We have to change this to UTC time because the CRL standard
 		// mandates it but Go will happily encode the CRL without this.
 		newRevCert := pkix.RevokedCertificate{

changelog/16874.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+secrets/pki: Improve stability of association of revoked cert with its parent issuer; when an issuer loses crl-signing usage, do not place certs on default issuer's CRL.
+```