Don't include issuers on delta CRLs (#17463)

When revoking an issuer, we immediately force a full rebuild of all CRLs
(complete and delta). However, we had forgotten to guard the delta CRL's
inclusion of augmented issuers, resulting in double-listing the issuer's
serial number on both the complete and the delta CRL. This isn't
necessary as the delta's referenced complete CRL number has incremented
to the point where the issuer itself was included on the complete CRL.

Avoid this double reference and don't include issuers on delta CRLs;
they should always appear only on the complete CRL.

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

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

builtin/logical/pki/crl_test.go

@@ -724,6 +724,14 @@ func TestIssuerRevocation(t *testing.T) {
 
 	b, s := createBackendWithStorage(t)
 
+	// Write a config with auto-rebuilding so that we can verify stuff doesn't
+	// appear on the delta CRL.
+	_, err := CBWrite(b, s, "config/crl", map[string]interface{}{
+		"auto_rebuild": true,
+		"enable_delta": true,
+	})
+	require.NoError(t, err)
+
 	// Create a root CA.
 	resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{
 		"common_name": "root example.com",
@@ -847,6 +855,10 @@ func TestIssuerRevocation(t *testing.T) {
 	require.NoError(t, err)
 	crl = getParsedCrlFromBackend(t, b, s, "issuer/root/crl/der")
 	requireSerialNumberInCRL(t, crl.TBSCertList, intCertSerial)
+	crl = getParsedCrlFromBackend(t, b, s, "issuer/root/crl/delta/der")
+	if requireSerialNumberInCRL(nil, crl.TBSCertList, intCertSerial) {
+		t.Fatalf("expected intermediate serial NOT to appear on root's delta CRL, but did")
+	}
 
 	// Ensure we can still revoke the issued leaf.
 	resp, err = CBWrite(b, s, "revoke", map[string]interface{}{

builtin/logical/pki/crl_util.go

@@ -841,10 +841,20 @@ func buildAnyCRLs(sc *storageContext, forceNew bool, isDelta bool) error {
 		if err != nil {
 			return fmt.Errorf("error building CRLs: unable to get revoked certificate entries: %v", err)
 		}
-	}
 
-	if err := augmentWithRevokedIssuers(issuerIDEntryMap, issuerIDCertMap, revokedCertsMap); err != nil {
-		return fmt.Errorf("error building CRLs: unable to parse revoked issuers: %v", err)
+		if !isDelta {
+			// Revoking an issuer forces us to rebuild our complete CRL,
+			// regardless of whether or not we've enabled auto rebuilding or
+			// delta CRLs. If we elide the above isDelta check, this results
+			// in a non-empty delta CRL, containing the serial of the
+			// now-revoked issuer, even though it was generated _after_ the
+			// complete CRL with the issuer on it. There's no reason to
+			// duplicate this serial number on the delta, hence the above
+			// guard for isDelta.
+			if err := augmentWithRevokedIssuers(issuerIDEntryMap, issuerIDCertMap, revokedCertsMap); err != nil {
+				return fmt.Errorf("error building CRLs: unable to parse revoked issuers: %v", err)
+			}
+		}
 	}
 
 	// Now we can call buildCRL once, on an arbitrary/representative issuer