Replace iterate_names() with a NameIterator

src/subject_name/verify.rs

@@ -14,6 +14,7 @@
 
 #[cfg(feature = "alloc")]
 use alloc::vec::Vec;
+use core::mem;
 
 use super::dns_name::{self, DnsNameRef};
 #[cfg(feature = "alloc")]
@@ -30,24 +31,29 @@ pub(crate) fn verify_cert_dns_name(
 ) -> Result<(), Error> {
     let cert = cert.inner();
     let dns_name = untrusted::Input::from(dns_name.as_ref().as_bytes());
-    iterate_names(
+    NameIterator::new(
         Some(cert.subject),
         cert.subject_alt_name,
         SubjectCommonNameContents::Ignore,
-        Err(Error::CertNotValidForName),
-        &mut |name| {
-            let presented_id = match name {
-                GeneralName::DnsName(presented) => presented,
-                _ => return None,
-            };
-
-            match dns_name::presented_id_matches_reference_id(presented_id, dns_name) {
-                Ok(true) => Some(Ok(())),
-                Ok(false) | Err(Error::MalformedDnsIdentifier) => None,
-                Err(e) => Some(Err(e)),
-            }
-        },
     )
+    .find_map(|result| {
+        let name = match result {
+            Ok(name) => name,
+            Err(err) => return Some(Err(err)),
+        };
+
+        let presented_id = match name {
+            GeneralName::DnsName(presented) => presented,
+            _ => return None,
+        };
+
+        match dns_name::presented_id_matches_reference_id(presented_id, dns_name) {
+            Ok(true) => Some(Ok(())),
+            Ok(false) | Err(Error::MalformedDnsIdentifier) => None,
+            Err(e) => Some(Err(e)),
+        }
+    })
+    .unwrap_or(Err(Error::CertNotValidForName))
 }
 
 pub(crate) fn verify_cert_subject_name(
@@ -64,25 +70,28 @@ pub(crate) fn verify_cert_subject_name(
         }
     };
 
-    iterate_names(
-        // IP addresses are not compared against the subject field;
-        // only against Subject Alternative Names.
+    NameIterator::new(
         None,
         cert.inner().subject_alt_name,
         SubjectCommonNameContents::Ignore,
-        Err(Error::CertNotValidForName),
-        &mut |name| {
-            let presented_id = match name {
-                GeneralName::IpAddress(presented) => presented,
-                _ => return None,
-            };
-
-            match ip_address::presented_id_matches_reference_id(presented_id, ip_address) {
-                true => Some(Ok(())),
-                false => None,
-            }
-        },
     )
+    .find_map(|result| {
+        let name = match result {
+            Ok(name) => name,
+            Err(err) => return Some(Err(err)),
+        };
+
+        let presented_id = match name {
+            GeneralName::IpAddress(presented) => presented,
+            _ => return None,
+        };
+
+        match ip_address::presented_id_matches_reference_id(presented_id, ip_address) {
+            true => Some(Ok(())),
+            false => None,
+        }
+    })
+    .unwrap_or(Err(Error::CertNotValidForName))
 }
 
 // https://tools.ietf.org/html/rfc5280#section-4.2.1.10
@@ -111,19 +120,23 @@ pub(crate) fn check_name_constraints(
 
     let mut child = subordinate_certs;
     loop {
-        iterate_names(
+        let result = NameIterator::new(
             Some(child.subject),
             child.subject_alt_name,
             subject_common_name_contents,
-            Ok(()),
-            &mut |name| {
-                check_presented_id_conforms_to_constraints(
-                    name,
-                    permitted_subtrees,
-                    excluded_subtrees,
-                )
-            },
-        )?;
+        )
+        .find_map(|result| {
+            let name = match result {
+                Ok(name) => name,
+                Err(err) => return Some(Err(err)),
+            };
+
+            check_presented_id_conforms_to_constraints(name, permitted_subtrees, excluded_subtrees)
+        });
+
+        if let Some(Err(err)) = result {
+            return Err(err);
+        }
 
         child = match child.ee_or_ca {
             EndEntityOrCa::Ca(child_cert) => child_cert,
@@ -265,87 +278,159 @@ pub(crate) enum SubjectCommonNameContents {
     Ignore,
 }
 
-fn iterate_names<'names>(
-    subject: Option<untrusted::Input<'names>>,
-    subject_alt_name: Option<untrusted::Input<'names>>,
+struct NameIterator<'a> {
+    state: NameIteratorState<'a>,
     subject_common_name_contents: SubjectCommonNameContents,
-    result_if_never_stopped_early: Result<(), Error>,
-    f: &mut impl FnMut(GeneralName<'names>) -> Option<Result<(), Error>>,
-) -> Result<(), Error> {
-    if let Some(subject_alt_name) = subject_alt_name {
-        let mut subject_alt_name = untrusted::Reader::new(subject_alt_name);
-        // https://bugzilla.mozilla.org/show_bug.cgi?id=1143085: An empty
-        // subjectAltName is not legal, but some certificates have an empty
-        // subjectAltName. Since we don't support CN-IDs, the certificate
-        // will be rejected either way, but checking `at_end` before
-        // attempting to parse the first entry allows us to return a better
-        // error code.
-        while !subject_alt_name.at_end() {
-            let name = GeneralName::from_der(&mut subject_alt_name)?;
-            if let Some(result) = f(name) {
-                return result;
-            }
-        }
-    }
+}
 
-    let subject = match subject {
-        Some(subject) => subject,
-        None => return result_if_never_stopped_early,
-    };
+impl<'a> NameIterator<'a> {
+    fn new(
+        subject: Option<untrusted::Input<'a>>,
+        subject_alt_name: Option<untrusted::Input<'a>>,
+        subject_common_name_contents: SubjectCommonNameContents,
+    ) -> Self {
+        let state = match (subject_alt_name, subject) {
+            (Some(subject_alt_name), _) => NameIteratorState::SubjectAltName {
+                subject_alt_name: untrusted::Reader::new(subject_alt_name),
+                subject,
+            },
+            (None, Some(subject)) => NameIteratorState::SubjectDirectoryName { subject },
+            (None, None) => NameIteratorState::None,
+        };
 
-    if let Some(result) = f(GeneralName::DirectoryName(subject)) {
-        return result;
+        NameIterator {
+            state,
+            subject_common_name_contents,
+        }
     }
+}
 
-    if let SubjectCommonNameContents::Ignore = subject_common_name_contents {
-        return result_if_never_stopped_early;
-    }
+impl<'a> Iterator for NameIterator<'a> {
+    type Item = Result<GeneralName<'a>, Error>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let item;
+        self.state = match mem::replace(&mut self.state, NameIteratorState::None) {
+            NameIteratorState::SubjectAltName {
+                mut subject_alt_name,
+                subject,
+            } => {
+                // https://bugzilla.mozilla.org/show_bug.cgi?id=1143085: An empty
+                // subjectAltName is not legal, but some certificates have an empty
+                // subjectAltName. Since we don't support CN-IDs, the certificate
+                // will be rejected either way, but checking `at_end` before
+                // attempting to parse the first entry allows us to return a better
+                // error code.
+
+                item = match GeneralName::from_der(&mut subject_alt_name) {
+                    Ok(name) => Some(Ok(name)),
+                    Err(err) => {
+                        self.state = NameIteratorState::None;
+                        return Some(Err(err));
+                    }
+                };
+
+                match subject_alt_name.at_end() {
+                    true => match subject {
+                        Some(subject) => NameIteratorState::SubjectDirectoryName { subject },
+                        None => NameIteratorState::None,
+                    },
+                    false => NameIteratorState::SubjectAltName {
+                        subject_alt_name,
+                        subject,
+                    },
+                }
+            }
+            NameIteratorState::SubjectDirectoryName { subject } => {
+                item = Some(Ok(GeneralName::DirectoryName(subject)));
+                match self.subject_common_name_contents {
+                    SubjectCommonNameContents::DnsName => {
+                        NameIteratorState::SubjectCommonName { subject }
+                    }
+                    SubjectCommonNameContents::Ignore => NameIteratorState::None,
+                }
+            }
+            NameIteratorState::SubjectCommonName { subject } => {
+                item = match common_name(subject) {
+                    Ok(Some(cn)) => match self.subject_common_name_contents {
+                        SubjectCommonNameContents::DnsName => Some(Ok(GeneralName::DnsName(cn))),
+                        SubjectCommonNameContents::Ignore => None,
+                    },
+                    Ok(None) => None,
+                    Err(err) => {
+                        self.state = NameIteratorState::None;
+                        return Some(Err(err));
+                    }
+                };
+                NameIteratorState::None
+            }
+            NameIteratorState::None => {
+                item = None;
+                NameIteratorState::None
+            }
+        };
 
-    match common_name(subject) {
-        Ok(Some(cn)) => match f(GeneralName::DnsName(cn)) {
-            Some(result) => result,
-            None => result_if_never_stopped_early,
-        },
-        Ok(None) => result_if_never_stopped_early,
-        Err(err) => Err(err),
+        item
     }
 }
 
+enum NameIteratorState<'a> {
+    SubjectAltName {
+        subject_alt_name: untrusted::Reader<'a>,
+        subject: Option<untrusted::Input<'a>>,
+    },
+    SubjectDirectoryName {
+        subject: untrusted::Input<'a>,
+    },
+    SubjectCommonName {
+        subject: untrusted::Input<'a>,
+    },
+    None,
+}
+
 #[cfg(feature = "alloc")]
 pub(crate) fn list_cert_dns_names<'names>(
     cert: &'names crate::EndEntityCert<'names>,
 ) -> Result<impl Iterator<Item = GeneralDnsNameRef<'names>>, Error> {
     let cert = &cert.inner();
     let mut names = Vec::new();
 
-    iterate_names(
+    let result = NameIterator::new(
         Some(cert.subject),
         cert.subject_alt_name,
         SubjectCommonNameContents::DnsName,
-        Ok(()),
-        &mut |name| {
-            let presented_id = match name {
-                GeneralName::DnsName(presented) => presented,
-                _ => return None,
-            };
+    )
+    .find_map(&mut |result| {
+        let name = match result {
+            Ok(name) => name,
+            Err(err) => return Some(err),
+        };
 
-            let dns_name = DnsNameRef::try_from_ascii(presented_id.as_slice_less_safe())
-                .map(GeneralDnsNameRef::DnsName)
-                .or_else(|_| {
-                    WildcardDnsNameRef::try_from_ascii(presented_id.as_slice_less_safe())
-                        .map(GeneralDnsNameRef::Wildcard)
-                });
-
-            // if the name could be converted to a DNS name, add it; otherwise,
-            // keep going.
-            if let Ok(name) = dns_name {
-                names.push(name)
-            }
+        let presented_id = match name {
+            GeneralName::DnsName(presented) => presented,
+            _ => return None,
+        };
 
-            None
-        },
-    )
-    .map(|_| names.into_iter())
+        let dns_name = DnsNameRef::try_from_ascii(presented_id.as_slice_less_safe())
+            .map(GeneralDnsNameRef::DnsName)
+            .or_else(|_| {
+                WildcardDnsNameRef::try_from_ascii(presented_id.as_slice_less_safe())
+                    .map(GeneralDnsNameRef::Wildcard)
+            });
+
+        // if the name could be converted to a DNS name, add it; otherwise,
+        // keep going.
+        if let Ok(name) = dns_name {
+            names.push(name)
+        }
+
+        None
+    });
+
+    match result {
+        Some(err) => Err(err),
+        _ => Ok(names.into_iter()),
+    }
 }
 
 // It is *not* valid to derive `Eq`, `PartialEq, etc. for this type. In