crl: don't iterate revoked certs up-front.

To better get a sense of the cost of parsing the base CRL vs parsing and
iterating the revoked certs we need to stop parsing and iterating
revoked certs when we parse the base CRL. We did this previously so that
we could surface errors in the revoked certs up-front, but for the
purposes of benchmarking it obscures what we're trying to measure.

src/crl.rs

@@ -69,7 +69,7 @@ impl<'a> CertRevocationList<'a> {
             )
         })?;
 
-        let crl = tbs_cert_list.read_all(Error::BadDer, |tbs_cert_list| {
+        tbs_cert_list.read_all(Error::BadDer, |tbs_cert_list| {
             // RFC 5280 §5.1.2.1:
             //   This optional field describes the version of the encoded CRL.  When
             //   extensions are used, as required by this profile, this field MUST be
@@ -164,15 +164,7 @@ impl<'a> CertRevocationList<'a> {
             )?;
 
             Ok(crl)
-        })?;
-
-        // Iterate through the revoked certificate entries to ensure they are valid so we can
-        // yield an error up-front instead of on first iteration by the caller.
-        for cert_result in crl.into_iter() {
-            cert_result?;
-        }
-
-        Ok(crl)
+        })
     }
 
     fn remember_extension(&mut self, extension: &Extension<'a>) -> Result<(), Error> {
@@ -226,17 +218,17 @@ impl<'a> CertRevocationList<'a> {
         })
     }
 
-    /// Try to find a [`RevokedCert`] in the CRL that has a serial number matching `serial`. This
-    /// method will ignore any [`RevokedCert`] entries that do not parse successfully. To handle
-    /// parse errors use [`CertRevocationList`]'s [`IntoIterator`] trait.
-    pub fn find_serial(&self, serial: &[u8]) -> Option<RevokedCert<'_>> {
+    /// Try to find a [`RevokedCert`] in the CRL that has a serial number matching `serial`.
+    pub fn find_serial(&self, serial: &[u8]) -> Result<Option<RevokedCert<'_>>, Error> {
         // TODO(XXX): This linear scan is sub-optimal from a performance perspective, but avoids
         //            any allocation. It would be nice to offer a speedier alternative for
         //            when the alloc feature is enabled:
         //            https://github.com/rustls/webpki/issues/80
-        self.into_iter()
-            .filter_map(|parse_res| parse_res.ok())
-            .find(|revoked_cert| revoked_cert.serial_number.eq(serial))
+        Ok(self
+            .into_iter()
+            .collect::<Result<Vec<_>, _>>()?
+            .into_iter()
+            .find(|revoked_cert| revoked_cert.serial_number == serial))
     }
 
     /// Raw DER encoding of the issuer of the CRL.

src/verify_cert.rs

@@ -239,7 +239,7 @@ fn check_crl(
 
     // Try to find the cert serial in the verified CRL contents.
     let cert_serial = cert.serial.as_slice_less_safe();
-    match crl.find_serial(cert_serial) {
+    match crl.find_serial(cert_serial)? {
         None => Ok(Some(CertNotRevoked::assertion())),
         Some(_) => Err(Error::CertRevoked),
     }

tests/crl_tests.rs

@@ -21,7 +21,7 @@ fn parse_valid_crl() {
     assert_eq!(aki, expected_aki);
 
     // We should find the expected revoked certificate with the expected serial number.
-    assert!(crl.find_serial(REVOKED_SERIAL).is_some())
+    assert!(crl.find_serial(REVOKED_SERIAL).unwrap().is_some())
 }
 
 #[test]
@@ -100,9 +100,11 @@ fn parse_negative_crl_number_crl() {
 
 #[test]
 fn parse_entry_negative_serial_crl() {
-    // Parsing a CRL that includes a revoked entry with a negative serial number should error.
     let crl = include_bytes!("crls/crl.negative.serial.der");
-    let res = CertRevocationList::from_der(&crl[..]);
+    let crl = CertRevocationList::from_der(&crl[..]).unwrap();
+
+    // Searching a CRL that includes a revoked entry with a negative serial number should error.
+    let res = crl.find_serial(&[0xC0, 0xFF, 0xEE]);
     assert!(matches!(res, Err(Error::InvalidSerialNumber)));
 }
 
@@ -112,7 +114,7 @@ fn parse_entry_without_exts_crl() {
     let crl = include_bytes!("crls/crl.no.entry.exts.der");
     let crl = CertRevocationList::from_der(&crl[..]).expect("unexpected error parsing crl");
     // We should find the expected revoked certificate with the expected serial number.
-    assert!(crl.find_serial(REVOKED_SERIAL).is_some());
+    assert!(crl.find_serial(REVOKED_SERIAL).unwrap().is_some());
 }
 
 #[test]
@@ -125,17 +127,21 @@ fn parse_entry_with_empty_exts_seq() {
 
 #[test]
 fn parse_entry_unknown_crit_ext_crl() {
-    // Parsing a CRL that includes a revoked entry that has an unknown critical extension should error.
     let crl = include_bytes!("crls/crl.entry.unknown.crit.ext.der");
-    let res = CertRevocationList::from_der(&crl[..]);
+    let crl = CertRevocationList::from_der(&crl[..]).unwrap();
+
+    // Searching a CRL that includes a revoked entry that has an unknown critical extension should error.
+    let res = crl.find_serial(&[]);
     assert!(matches!(res, Err(Error::UnsupportedCriticalExtension)));
 }
 
 #[test]
 fn parse_entry_invalid_reason_crl() {
-    // Parsing a CRL that includes a revoked entry that has an unknown revocation reason should error.
     let crl = include_bytes!("crls/crl.entry.invalid.reason.der");
-    let res = CertRevocationList::from_der(&crl[..]);
+    let crl = CertRevocationList::from_der(&crl[..]).unwrap();
+
+    // Searching a CRL that includes a revoked entry that has an unknown revocation reason should error.
+    let res = crl.find_serial(&[0x0B, 0xAD]);
     assert!(matches!(res, Err(Error::UnsupportedRevocationReason)));
 }
 
@@ -149,15 +155,18 @@ fn parse_entry_invalidity_date_crl() {
     assert!(crl
         .find_serial(REVOKED_SERIAL)
         .unwrap()
+        .unwrap()
         .invalidity_date
         .is_some());
 }
 
 #[test]
 fn parse_entry_indirect_issuer_crl() {
-    // Parsing a CRL that includes a revoked entry that has a issuer certificate extension
-    // should error because this indicates the CRL is an "indirect" CRL that we do not support.
     let crl = include_bytes!("crls/crl.entry.issuer.ext.der");
-    let res = CertRevocationList::from_der(&crl[..]);
+    let crl = CertRevocationList::from_der(&crl[..]).unwrap();
+
+    // Searching a CRL that includes a revoked entry that has a issuer certificate extension
+    // should error because this indicates the CRL is an "indirect" CRL that we do not support.
+    let res = crl.find_serial(&[0xDE, 0xAD]);
     assert!(matches!(res, Err(Error::UnsupportedIndirectCrl)));
 }