Ensure stability of revInfo issuer identification

As mentioned by Kit, iterating through each revInfoEntry and associating
the first issuer which matches it can cause churn when many (equivalent)
issuers are in the system and issuers come and go (via CRLSigning usage,
which has been modified in this release as well). Because we'd not
include issuers without CRLSigning usage, we'd cause our verification
helper, isRevInfoIssuerValid, to think the issuer ID is no longer value
(when instead, it just lacks crlSigning bits).

We address this by pulling in all issuers we know of for the
identification. This allows us to keep valid-but-not-for-signing
issuers, and use other representatives of their identity set for
signing/building the CRL (if they are enabled for such usage).

As a side effect, we now no longer place these entries on the default
CRL in the event all issuers in the CRL set are without the usage.

Signed-off-by: Alexander Scheel <[email protected]>

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_util.go

@@ -491,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()
@@ -541,10 +550,22 @@ 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.
+				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...)
@@ -553,10 +574,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)
@@ -567,6 +596,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()