From d45305ba3e6a7f0104f2483a441c963b318ee2ea Mon Sep 17 00:00:00 2001
From: Daniel McCarney <daniel@binaryparadox.net>
Date: Wed, 25 Oct 2023 11:41:30 -0400
Subject: [PATCH] crl: make verify_signature crate-internal, use budget

Previously the `verify_signature` fn was part of the
`CertRevocationList` trait, and so part of the public API. This meant we
couldn't accept an `untrusted::Input` argument, or more importantly,
a `Budget` to use to indicate we've consumed a signature validation
operation.

Now that the `CertRevocationList` type is an enum we can easily make
this fn crate-internal. It doesn't seem especially valuable to external
users, especially given that it's somewhat nuanced/cumbersome to build
the right SPKI representation to use for validation outside of webpki.

This commit makes the fn crate internal, adds the `Budget` argument,
reworks the SPKI to be an `untrusted::Input` and folds-in the
`signed_data` crate-internal fn since it wasn't being used anywhere
except in `verify_signature`.

The net result is that it's not impossible to verify a CRL signature
without providing a budget to use. In the path building context in
`verify_cert.rs` we pass through the budget we use for the overall
accounting during path building.
---
 src/crl/mod.rs   |  6 +-----
 src/crl/types.rs | 51 +++++++++++++++++++++---------------------------
 2 files changed, 23 insertions(+), 34 deletions(-)

diff --git a/src/crl/mod.rs b/src/crl/mod.rs
index c54b243e..2ac9241d 100644
--- a/src/crl/mod.rs
+++ b/src/crl/mod.rs
@@ -132,11 +132,7 @@ impl<'a> RevocationOptions<'a> {
         // TODO(XXX): consider whether we can refactor so this happens once up-front, instead
         //            of per-lookup.
         //            https://github.com/rustls/webpki/issues/81
-        // Note: The `verify_signature` method is part of a public trait in the exported API.
-        //       We can't add a budget argument to that fn in a semver compatible way and so must
-        //       consume signature budget here before calling verify_signature.
-        budget.consume_signature()?;
-        crl.verify_signature(supported_sig_algs, issuer_spki.as_slice_less_safe())
+        crl.verify_signature(supported_sig_algs, issuer_spki, budget)
             .map_err(crl_signature_err)?;
 
         // Verify that if the issuer has a KeyUsage bitstring it asserts cRLSign.
diff --git a/src/crl/types.rs b/src/crl/types.rs
index b4fecc82..fd844336 100644
--- a/src/crl/types.rs
+++ b/src/crl/types.rs
@@ -74,26 +74,6 @@ impl<'a> CertRevocationList<'a> {
         }
     }
 
-    /// Verify the CRL signature using the issuer certificate and a list of supported signature
-    /// verification algorithms.
-    ///
-    /// This is appropriate for one-off validation of CRL signatures and shouldn't be used in
-    /// a context where the overall number of signature validation operations is budgeted (e.g.
-    /// during path building).
-    pub fn verify_signature(
-        &self,
-        supported_sig_algs: &[&dyn SignatureVerificationAlgorithm],
-        issuer_spki: &[u8],
-    ) -> Result<(), Error> {
-        signed_data::verify_signed_data(
-            supported_sig_algs,
-            untrusted::Input::from(issuer_spki),
-            &self.signed_data(),
-            &mut Budget::default(),
-        )
-        .map_err(crl_signature_err)
-    }
-
     /// Returns true if the CRL can be considered authoritative for the given certificate.
     ///
     /// A CRL is considered authoritative for a certificate when:
@@ -136,16 +116,29 @@ impl<'a> CertRevocationList<'a> {
         crl_idp.authoritative_for(path)
     }
 
-    pub(crate) fn signed_data(&self) -> SignedData {
-        match self {
-            #[cfg(feature = "alloc")]
-            CertRevocationList::Owned(crl) => crl.signed_data.borrow(),
-            CertRevocationList::Borrowed(crl) => SignedData {
-                data: crl.signed_data.data,
-                algorithm: crl.signed_data.algorithm,
-                signature: crl.signed_data.signature,
+    /// Verify the CRL signature using the issuer certificate and a list of supported signature
+    /// verification algorithms, consuming signature operations from the [`Budget`].
+    pub(crate) fn verify_signature(
+        &self,
+        supported_sig_algs: &[&dyn SignatureVerificationAlgorithm],
+        issuer_spki: untrusted::Input,
+        budget: &mut Budget,
+    ) -> Result<(), Error> {
+        signed_data::verify_signed_data(
+            supported_sig_algs,
+            issuer_spki,
+            &match self {
+                #[cfg(feature = "alloc")]
+                CertRevocationList::Owned(crl) => crl.signed_data.borrow(),
+                CertRevocationList::Borrowed(crl) => SignedData {
+                    data: crl.signed_data.data,
+                    algorithm: crl.signed_data.algorithm,
+                    signature: crl.signed_data.signature,
+                },
             },
-        }
+            budget,
+        )
+        .map_err(crl_signature_err)
     }
 }