Add context in name validation errors

src/der.rs

@@ -111,9 +111,10 @@ pub(crate) fn nested_limited<'a, R>(
     decoder: impl FnOnce(&mut untrusted::Reader<'a>) -> Result<R, Error>,
     size_limit: usize,
 ) -> Result<R, Error> {
-    expect_tag_and_get_value_limited(input, tag, size_limit)
-        .map_err(|_| error)?
-        .read_all(error, decoder)
+    match expect_tag_and_get_value_limited(input, tag, size_limit) {
+        Ok(value) => value.read_all(error, decoder),
+        Err(_) => Err(error),
+    }
 }
 
 // TODO: investigate taking decoder as a reference to reduce generated code
@@ -312,9 +313,9 @@ pub(crate) fn nested_of_mut<'a>(
     error: Error,
     mut decoder: impl FnMut(&mut untrusted::Reader<'a>) -> Result<(), Error>,
 ) -> Result<(), Error> {
-    nested(input, outer_tag, error, |outer| {
+    nested(input, outer_tag, error.clone(), |outer| {
         loop {
-            nested(outer, inner_tag, error, |inner| decoder(inner))?;
+            nested(outer, inner_tag, error.clone(), |inner| decoder(inner))?;
             if outer.at_end() {
                 break;
             }
@@ -703,11 +704,11 @@ mod tests {
             let mut bytes = untrusted::Reader::new(untrusted::Input::from(tc.input));
 
             let res = read_tag_and_get_value_limited(&mut bytes, tc.limit);
-            match tc.err {
+            match &tc.err {
                 None => assert!(res.is_ok()),
                 Some(e) => {
                     let actual = res.unwrap_err();
-                    assert_eq!(actual, e)
+                    assert_eq!(&actual, e)
                 }
             }
         }

src/end_entity.rs

@@ -20,7 +20,7 @@ use pki_types::{
 
 use crate::crl::RevocationOptions;
 use crate::error::Error;
-use crate::subject_name::{verify_dns_names, verify_ip_address_names, NameIterator};
+use crate::subject_name::{verify_dns_names, verify_ip_address_names};
 use crate::verify_cert::{self, KeyUsage, VerifiedPath};
 use crate::{cert, signed_data};
 
@@ -125,16 +125,10 @@ impl EndEntityCert<'_> {
         server_name: &ServerName<'_>,
     ) -> Result<(), Error> {
         match server_name {
-            ServerName::DnsName(dns_name) => verify_dns_names(
-                dns_name,
-                NameIterator::new(Some(self.inner.subject), self.inner.subject_alt_name),
-            ),
+            ServerName::DnsName(dns_name) => verify_dns_names(dns_name, &self.inner),
             // IP addresses are not compared against the subject field;
             // only against Subject Alternative Names.
-            ServerName::IpAddress(ip_address) => verify_ip_address_names(
-                ip_address,
-                NameIterator::new(None, self.inner.subject_alt_name),
-            ),
+            ServerName::IpAddress(ip_address) => verify_ip_address_names(ip_address, &self.inner),
             _ => Err(Error::UnsupportedNameType),
         }
     }

src/error.rs

@@ -12,11 +12,18 @@
 // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
+#[cfg(feature = "alloc")]
+use alloc::string::String;
+#[cfg(feature = "alloc")]
+use alloc::vec::Vec;
 use core::fmt;
 use core::ops::ControlFlow;
 
+#[cfg(feature = "alloc")]
+use pki_types::ServerName;
+
 /// An error that occurs during certificate validation or name validation.
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 #[non_exhaustive]
 pub enum Error {
     /// The encoding of some ASN.1 DER-encoded item is invalid.
@@ -33,7 +40,7 @@
     CertExpired,
 
     /// The certificate is not valid for the name it is being validated for.
-    CertNotValidForName,
+    CertNotValidForName(InvalidNameContext),
 
     /// The certificate is not valid yet; i.e. the time it is being validated
     /// for is earlier than the certificate's notBefore time.
@@ -216,7 +223,7 @@
         match &self {
             // Errors related to certificate validity
             Self::CertNotValidYet | Self::CertExpired => 290,
-            Self::CertNotValidForName => 280,
+            Self::CertNotValidForName(_) => 280,
             Self::CertRevoked | Self::UnknownRevocationStatus | Self::CrlExpired => 270,
             Self::InvalidCrlSignatureForPublicKey | Self::InvalidSignatureForPublicKey => 260,
             Self::SignatureAlgorithmMismatch => 250,
@@ -300,6 +307,22 @@
 #[cfg(feature = "std")]
 impl ::std::error::Error for Error {}
 
+/// Additional context for the `CertNotValidForName` error variant.
+///
+/// The contents of this type depend on whether the `alloc` feature is enabled.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct InvalidNameContext {
+    /// Expected server name.
+    #[cfg(feature = "alloc")]
+    pub expected: ServerName<'static>,
+    /// The names presented in the end entity certificate.
+    ///
+    /// These are the subject names as present in the leaf certificate and may contain DNS names
+    /// with or without a wildcard label as well as IP address names.
+    #[cfg(feature = "alloc")]
+    pub presented: Vec<String>,
+}
+
 /// Trailing data was found while parsing DER-encoded input for the named type.
 #[allow(missing_docs)]
 #[non_exhaustive]

src/lib.rs

@@ -79,7 +79,7 @@ pub use {
         UnknownStatusPolicy,
     },
     end_entity::EndEntityCert,
-    error::{DerTypeId, Error},
+    error::{DerTypeId, Error, InvalidNameContext},
     rpk_entity::RawPublicKeyEntity,
     signed_data::alg_id,
     trust_anchor::anchor_from_trusted_cert,

src/subject_name/dns_name.rs

@@ -12,37 +12,57 @@
 // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
+#[cfg(feature = "alloc")]
+use alloc::format;
 use core::fmt::Write;
 
+#[cfg(feature = "alloc")]
+use pki_types::ServerName;
 use pki_types::{DnsName, InvalidDnsNameError};
 
 use super::verify::{GeneralName, NameIterator};
-use crate::Error;
+use crate::cert::Cert;
+use crate::error::{Error, InvalidNameContext};
 
-pub(crate) fn verify_dns_names(
-    reference: &DnsName<'_>,
-    mut names: NameIterator<'_>,
-) -> Result<(), Error> {
+pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Result<(), Error> {
     let dns_name = untrusted::Input::from(reference.as_ref().as_bytes());
-    names
-        .find_map(|result| {
-            let name = match result {
-                Ok(name) => name,
-                Err(err) => return Some(Err(err)),
-            };
+    let result = NameIterator::new(Some(cert.subject), cert.subject_alt_name).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,
-            };
+        let presented_id = match name {
+            GeneralName::DnsName(presented) => presented,
+            _ => return None,
+        };
 
-            match presented_id_matches_reference_id(presented_id, IdRole::Reference, dns_name) {
-                Ok(true) => Some(Ok(())),
-                Ok(false) | Err(Error::MalformedDnsIdentifier) => None,
-                Err(e) => Some(Err(e)),
-            }
-        })
-        .unwrap_or(Err(Error::CertNotValidForName))
+        match presented_id_matches_reference_id(presented_id, IdRole::Reference, dns_name) {
+            Ok(true) => Some(Ok(())),
+            Ok(false) | Err(Error::MalformedDnsIdentifier) => None,
+            Err(e) => Some(Err(e)),
+        }
+    });
+
+    match result {
+        Some(result) => return result,
+        #[cfg(feature = "alloc")]
+        None => {}
+        #[cfg(not(feature = "alloc"))]
+        None => Err(Error::CertNotValidForName(InvalidNameContext {})),
+    }
+
+    // Try to yield a more useful error. To avoid allocating on the happy path,
+    // we reconstruct the same `NameIterator` and replay it.
+    #[cfg(feature = "alloc")]
+    {
+        Err(Error::CertNotValidForName(InvalidNameContext {
+            expected: ServerName::DnsName(reference.to_owned()),
+            presented: NameIterator::new(Some(cert.subject), cert.subject_alt_name)
+                .filter_map(|result| Some(format!("{:?}", result.ok()?)))
+                .collect(),
+        }))
+    }
 }
 
 /// A reference to a DNS Name presented by a server that may include a wildcard.
@@ -868,14 +888,14 @@
 
     #[test]
     fn presented_matches_reference_test() {
-        for &(presented, reference, expected_result) in PRESENTED_MATCHES_REFERENCE {
+        for (presented, reference, expected_result) in PRESENTED_MATCHES_REFERENCE {
             let actual_result = presented_id_matches_reference_id(
                 untrusted::Input::from(presented),
                 IdRole::Reference,
                 untrusted::Input::from(reference),
             );
             assert_eq!(
-                actual_result, expected_result,
+                &actual_result, expected_result,
                 "presented_id_matches_reference_id(\"{:?}\", \"{:?}\")",
                 presented, reference
             );
@@ -945,14 +965,14 @@
 
     #[test]
     fn presented_matches_constraint_test() {
-        for &(presented, constraint, expected_result) in PRESENTED_MATCHES_CONSTRAINT {
+        for (presented, constraint, expected_result) in PRESENTED_MATCHES_CONSTRAINT {
             let actual_result = presented_id_matches_reference_id(
                 untrusted::Input::from(presented),
                 IdRole::NameConstraint,
                 untrusted::Input::from(constraint),
             );
             assert_eq!(
-                actual_result, expected_result,
+                &actual_result, expected_result,
                 "presented_id_matches_constraint(\"{:?}\", \"{:?}\")",
                 presented, constraint,
             );

src/subject_name/ip_address.rs

@@ -12,38 +12,57 @@
 // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
+#[cfg(feature = "alloc")]
+use alloc::format;
+
 use pki_types::IpAddr;
+#[cfg(feature = "alloc")]
+use pki_types::ServerName;
 
 use super::verify::{GeneralName, NameIterator};
-use crate::Error;
+use crate::cert::Cert;
+use crate::error::{Error, InvalidNameContext};
 
-pub(crate) fn verify_ip_address_names(
-    reference: &IpAddr,
-    mut names: NameIterator<'_>,
-) -> Result<(), Error> {
+pub(crate) fn verify_ip_address_names(reference: &IpAddr, cert: &Cert<'_>) -> Result<(), Error> {
     let ip_address = match reference {
         IpAddr::V4(ip) => untrusted::Input::from(ip.as_ref()),
         IpAddr::V6(ip) => untrusted::Input::from(ip.as_ref()),
     };
 
-    names
-        .find_map(|result| {
-            let name = match result {
-                Ok(name) => name,
-                Err(err) => return Some(Err(err)),
-            };
+    let result = NameIterator::new(None, cert.subject_alt_name).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,
-            };
+        let presented_id = match name {
+            GeneralName::IpAddress(presented) => presented,
+            _ => return None,
+        };
 
-            match presented_id_matches_reference_id(presented_id, ip_address) {
-                true => Some(Ok(())),
-                false => None,
-            }
-        })
-        .unwrap_or(Err(Error::CertNotValidForName))
+        match presented_id_matches_reference_id(presented_id, ip_address) {
+            true => Some(Ok(())),
+            false => None,
+        }
+    });
+
+    match result {
+        Some(result) => return result,
+        #[cfg(feature = "alloc")]
+        None => {}
+        #[cfg(not(feature = "alloc"))]
+        None => Err(Error::CertNotValidForName(InvalidNameContext {})),
+    }
+
+    #[cfg(feature = "alloc")]
+    {
+        Err(Error::CertNotValidForName(InvalidNameContext {
+            expected: ServerName::from(*reference),
+            presented: NameIterator::new(None, cert.subject_alt_name)
+                .filter_map(|result| Some(format!("{:?}", result.ok()?)))
+                .collect(),
+        }))
+    }
 }
 
 // https://tools.ietf.org/html/rfc5280#section-4.2.1.6 says:
@@ -659,7 +678,7 @@
         use std::boxed::Box;
         use std::net::IpAddr;
 
-        for &(presented, constraint_address, constraint_mask, expected_result) in
+        for (presented, constraint_address, constraint_mask, expected_result) in
             PRESENTED_MATCHES_CONSTRAINT
         {
             let presented_bytes: Box<[u8]> = match presented.parse::<IpAddr>().unwrap() {
@@ -680,7 +699,7 @@
                 untrusted::Input::from(&constraint_bytes),
             );
             assert_eq!(
-                actual_result, expected_result,
+                &actual_result, expected_result,
                 "presented_id_matches_constraint(\"{:?}\", \"{:?}\")",
                 presented_bytes, constraint_bytes
             );