Cleanup changes around issuer revocation (#16874)

* Refactor CRL tests to use /sys/mounts

Thanks Steve for the approach! This also address nits from Kit.

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

* Skip CRL building steps when disabled

This skips a number of steps during CRL build when it is disabled (and
forceNew is not set). In particular, we avoid fetching issuers, we avoid
associating issuers with revocation entries (and building that in-memory
mapping), making CRL building more efficient.

This means that there'll again be very little overhead on clusters with
the CRL disabled.

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

* Prevent revoking roots from appearing on own CRLs

This change ensures that when marking a root as revoked, it no longer
appears on its own CRL. Very few clients support this event (as
generally only leaves/intermediates are checked for presence on a
parent's CRL) and it is technically undefined behavior (if the root is
revoked, its own CRL should be untrusted and thus including it on its
own CRL isn't a safe/correct distribution channel).

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

* 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 <alex.scheel@hashicorp.com>

* Add changelog entry

This is only for the last commit.

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

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

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)