verify_cert: use enum for build chain error

The `loop_while_non_fatal_error` helper can return one of three things:

* success, when a validated chain to a trust anchor was built.
* a fatal error, e.g. when a `Budget` has been exceeded and no further
  path building should occur because we've exhausted a budget.
* a non-fatal error, when a candidate chain results in an error
  condition, but other paths could be considered if the options are not
  exhausted.

This commit attempts to express this in the type system, centralizing
a check for what is/isn't a fatal error and ensuring that downstream
callers to `loop_while_non_fatal_error` handle the fatal case
appropriately.

src/error.rs

@@ -13,6 +13,7 @@
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 use core::fmt;
+use std::ops::ControlFlow;
 
 /// An error that occurs during certificate validation or name validation.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
@@ -271,6 +272,17 @@ impl Error {
     }
 }
 
+impl From<Error> for ControlFlow<Error, Error> {
+    fn from(value: Error) -> Self {
+        match value {
+            // If an error is fatal, we've exhausted the potential for continued search.
+            err if err.is_fatal() => Self::Break(err),
+            // Otherwise we've rejected one candidate chain, but may continue to search for others.
+            err => Self::Continue(err),
+        }
+    }
+}
+
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         write!(f, "{:?}", self)

src/verify_cert.rs

@@ -13,6 +13,7 @@
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 use core::default::Default;
+use std::ops::ControlFlow;
 
 use pki_types::{CertificateDer, SignatureVerificationAlgorithm, TrustAnchor};
 
@@ -130,7 +131,10 @@ pub(crate) struct ChainOptions<'a> {
 }
 
 pub(crate) fn build_chain(opts: &ChainOptions, cert: &Cert, time: time::Time) -> Result<(), Error> {
-    build_chain_inner(opts, cert, time, 0, &mut Budget::default())
+    build_chain_inner(opts, cert, time, 0, &mut Budget::default()).map_err(|e| match e {
+        ControlFlow::Break(err) => err,
+        ControlFlow::Continue(err) => err,
+    })
 }
 
 fn build_chain_inner(
@@ -139,7 +143,7 @@ fn build_chain_inner(
     time: time::Time,
     sub_ca_count: usize,
     budget: &mut Budget,
-) -> Result<(), Error> {
+) -> Result<(), ControlFlow<Error, Error>> {
     let used_as_ca = used_as_ca(&cert.ee_or_ca);
 
     check_issuer_independent_properties(cert, time, used_as_ca, sub_ca_count, opts.eku.inner)?;
@@ -151,7 +155,7 @@ fn build_chain_inner(
             const MAX_SUB_CA_COUNT: usize = 6;
 
             if sub_ca_count >= MAX_SUB_CA_COUNT {
-                return Err(Error::MaximumPathDepthExceeded);
+                return Err(Error::MaximumPathDepthExceeded.into());
             }
         }
         UsedAsCa::No => {
@@ -179,7 +183,7 @@ fn build_chain_inner(
         |trust_anchor: &TrustAnchor| {
             let trust_anchor_subject = untrusted::Input::from(trust_anchor.subject.as_ref());
             if cert.issuer != trust_anchor_subject {
-                return Err(Error::UnknownIssuer);
+                return Err(Error::UnknownIssuer.into());
             }
 
             // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?;
@@ -206,26 +210,26 @@ fn build_chain_inner(
     let err = match result {
         Ok(()) => return Ok(()),
         // Fatal errors should halt further path building.
-        res @ Err(err) if err.is_fatal() => return res,
+        res @ Err(ControlFlow::Break(_)) => return res,
         // Non-fatal errors should be carried forward as the default_error for subsequent
         // loop_while_non_fatal_error processing and only returned once all other path-building
         // options have been exhausted.
-        Err(err) => err,
+        Err(ControlFlow::Continue(err)) => err,
     };
 
     loop_while_non_fatal_error(err, opts.intermediate_certs, |cert_der| {
         let potential_issuer =
             Cert::from_der(untrusted::Input::from(cert_der), EndEntityOrCa::Ca(cert))?;
 
         if potential_issuer.subject != cert.issuer {
-            return Err(Error::UnknownIssuer);
+            return Err(Error::UnknownIssuer.into());
         }
 
         // Prevent loops; see RFC 4158 section 5.2.
         let mut prev = cert;
         loop {
             if potential_issuer.spki == prev.spki && potential_issuer.subject == prev.subject {
-                return Err(Error::UnknownIssuer);
+                return Err(Error::UnknownIssuer.into());
             }
             match &prev.ee_or_ca {
                 EndEntityOrCa::EndEntity => {
@@ -253,7 +257,7 @@ fn check_signed_chain(
     trust_anchor: &TrustAnchor,
     revocation: Option<RevocationOptions>,
     budget: &mut Budget,
-) -> Result<(), Error> {
+) -> Result<(), ControlFlow<Error, Error>> {
     let mut spki_value = untrusted::Input::from(trust_anchor.subject_public_key_info.as_ref());
     let mut issuer_subject = untrusted::Input::from(trust_anchor.subject.as_ref());
     let mut issuer_key_usage = None; // TODO(XXX): Consider whether to track TrustAnchor KU.
@@ -294,7 +298,7 @@ fn check_signed_chain_name_constraints(
     trust_anchor: &TrustAnchor,
     subject_common_name_contents: subject_name::SubjectCommonNameContents,
     budget: &mut Budget,
-) -> Result<(), Error> {
+) -> Result<(), ControlFlow<Error, Error>> {
     let mut cert = cert_chain;
     let mut name_constraints = trust_anchor
         .name_constraints
@@ -742,8 +746,8 @@ impl KeyUsageMode {
 fn loop_while_non_fatal_error<V>(
     default_error: Error,
     values: V,
-    mut f: impl FnMut(V::Item) -> Result<(), Error>,
-) -> Result<(), Error>
+    mut f: impl FnMut(V::Item) -> Result<(), ControlFlow<Error, Error>>,
+) -> Result<(), ControlFlow<Error, Error>>
 where
     V: IntoIterator,
 {
@@ -752,13 +756,13 @@ where
         match f(v) {
             Ok(()) => return Ok(()),
             // Fatal errors should halt further looping.
-            res @ Err(err) if err.is_fatal() => return res,
+            res @ Err(ControlFlow::Break(_)) => return res,
             // Non-fatal errors should be ranked by specificity and only returned
             // once all other path-building options have been exhausted.
-            Err(new_error) => error = error.most_specific(new_error),
+            Err(ControlFlow::Continue(new_error)) => error = error.most_specific(new_error),
         }
     }
-    Err(error)
+    Err(error.into())
 }
 
 #[cfg(test)]
@@ -888,7 +892,7 @@ mod tests {
         intermediate_count: usize,
         trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer,
         budget: Option<Budget>,
-    ) -> Error {
+    ) -> ControlFlow<Error, Error> {
         let ca_cert = make_issuer("Bogus Subject", None);
         let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap());
 
@@ -917,16 +921,16 @@ mod tests {
     #[test]
     #[cfg(feature = "alloc")]
     fn test_too_many_signatures() {
-        assert_eq!(
+        assert!(matches!(
             build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes, None),
-            Error::MaximumSignatureChecksExceeded
-        );
+            ControlFlow::Break(Error::MaximumSignatureChecksExceeded)
+        ));
     }
 
     #[test]
     #[cfg(feature = "alloc")]
     fn test_too_many_path_calls() {
-        assert_eq!(
+        assert!(matches!(
             build_degenerate_chain(
                 10,
                 TrustAnchorIsActualIssuer::No,
@@ -938,12 +942,12 @@ mod tests {
                     ..Budget::default()
                 })
             ),
-            Error::MaximumPathBuildCallsExceeded
-        );
+            ControlFlow::Break(Error::MaximumPathBuildCallsExceeded)
+        ));
     }
 
     #[cfg(feature = "alloc")]
-    fn build_linear_chain(chain_length: usize) -> Result<(), Error> {
+    fn build_linear_chain(chain_length: usize) -> Result<(), ControlFlow<Error, Error>> {
         let ca_cert = make_issuer(format!("Bogus Subject {chain_length}"), None);
         let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap());
 
@@ -967,18 +971,21 @@ mod tests {
     #[test]
     #[cfg(feature = "alloc")]
     fn longest_allowed_path() {
-        assert_eq!(build_linear_chain(1), Ok(()));
-        assert_eq!(build_linear_chain(2), Ok(()));
-        assert_eq!(build_linear_chain(3), Ok(()));
-        assert_eq!(build_linear_chain(4), Ok(()));
-        assert_eq!(build_linear_chain(5), Ok(()));
-        assert_eq!(build_linear_chain(6), Ok(()));
+        assert!(build_linear_chain(1).is_ok());
+        assert!(build_linear_chain(2).is_ok());
+        assert!(build_linear_chain(3).is_ok());
+        assert!(build_linear_chain(4).is_ok());
+        assert!(build_linear_chain(5).is_ok());
+        assert!(build_linear_chain(6).is_ok());
     }
 
     #[test]
     #[cfg(feature = "alloc")]
     fn path_too_long() {
-        assert_eq!(build_linear_chain(7), Err(Error::MaximumPathDepthExceeded));
+        assert!(matches!(
+            build_linear_chain(7),
+            Err(ControlFlow::Continue(Error::MaximumPathDepthExceeded))
+        ));
     }
 
     #[test]
@@ -1028,13 +1035,13 @@ mod tests {
         // Validation should succeed with the name constraint comparison budget allocated above.
         // This shows that we're not consuming budget on unused intermediates: we didn't budget
         // enough comparisons for that to pass the overall chain building.
-        verify_chain(
+        assert!(verify_chain(
             &ca_cert_der,
             &intermediates_der,
             &ee_cert,
             Some(passing_budget),
         )
-        .unwrap();
+        .is_ok());
 
         let failing_budget = Budget {
             // See passing_budget: 2 comparisons is not sufficient.
@@ -1051,7 +1058,12 @@ mod tests {
             Some(failing_budget),
         );
 
-        assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded));
+        assert!(matches!(
+            result,
+            Err(ControlFlow::Break(
+                Error::MaximumNameConstraintComparisonsExceeded
+            ))
+        ));
     }
 
     #[cfg(feature = "alloc")]
@@ -1060,7 +1072,7 @@ mod tests {
         intermediates_der: &[Vec<u8>],
         ee_cert: &CertificateDer<'_>,
         budget: Option<Budget>,
-    ) -> Result<(), Error> {
+    ) -> Result<(), ControlFlow<Error, Error>> {
         use crate::{extract_trust_anchor, ECDSA_P256_SHA256};
         use crate::{EndEntityCert, Time};