From b1c4ecf0380c7b7060fd6693e139d03446dac227 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 23 Jun 2023 09:25:52 -0400 Subject: [PATCH] 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 | 26 +++++++++----------------- src/verify_cert.rs | 2 +- tests/crl_tests.rs | 31 ++++++++++++++++++++----------- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/crl.rs b/src/crl.rs index f51d0343..44dbce45 100644 --- a/src/crl.rs +++ b/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> { + /// Try to find a [`RevokedCert`] in the CRL that has a serial number matching `serial`. + pub fn find_serial(&self, serial: &[u8]) -> Result>, 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::, _>>()? + .into_iter() + .find(|revoked_cert| revoked_cert.serial_number == serial)) } /// Raw DER encoding of the issuer of the CRL. diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 47135e09..660b1a0c 100644 --- a/src/verify_cert.rs +++ b/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), } diff --git a/tests/crl_tests.rs b/tests/crl_tests.rs index 81cf270e..8eddf410 100644 --- a/tests/crl_tests.rs +++ b/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))); }