Add context in name validation errors

Author: djc

src/error.rs

@@ -12,9 +12,16 @@
 // 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, Debug, PartialEq, Eq)]
 #[non_exhaustive]
@@ -32,9 +39,6 @@ pub enum Error {
     /// later than the certificate's notAfter time.
     CertExpired,
 
-    /// The certificate is not valid for the name it is being validated for.
-    CertNotValidForName,
-
     /// The certificate is not valid yet; i.e. the time it is being validated
     /// for is earlier than the certificate's notBefore time.
     CertNotValidYet,
@@ -127,6 +131,26 @@ pub enum Error {
     /// Trailing data was found while parsing DER-encoded input for the named type.
     TrailingData(DerTypeId),
 
+    /// The certificate is not valid for the name it is being validated for.
+    ///
+    /// Variant without context for use in no-`alloc` environments. See `UnexpectedCertName` for
+    /// the variant that is used in `alloc` environments.
+    UnexpectedCertNameSimple,
+
+    /// The certificate is not valid for the name it is being validated for.
+    ///
+    /// Variant with context for use in `alloc` environments. See `UnexpectedCertNameSimple`
+    /// for the variant that is used in no-`alloc` environments.
+    #[cfg(feature = "alloc")]
+    UnexpectedCertName {
+        /// Expected server name.
+        expected: ServerName<'static>,
+        /// The names presented in the end entity certificate.
+        ///
+        /// Unlike `expected`, these names might contain wildcard labels.
+        presented: Vec<String>,
+    },
+
     /// A valid issuer for the certificate could not be found.
     UnknownIssuer,
 
@@ -216,7 +240,6 @@ impl Error {
         match &self {
             // Errors related to certificate validity
             Self::CertNotValidYet | Self::CertExpired => 290,
-            Self::CertNotValidForName => 280,
             Self::CertRevoked | Self::UnknownRevocationStatus | Self::CrlExpired => 270,
             Self::InvalidCrlSignatureForPublicKey | Self::InvalidSignatureForPublicKey => 260,
             Self::SignatureAlgorithmMismatch => 250,
@@ -225,6 +248,9 @@ impl Error {
             Self::PathLenConstraintViolated => 220,
             Self::CaUsedAsEndEntity | Self::EndEntityUsedAsCa => 210,
             Self::IssuerNotCrlSigner => 200,
+            Self::UnexpectedCertNameSimple => 280,
+            #[cfg(feature = "alloc")]
+            Self::UnexpectedCertName { .. } => 280,
 
             // Errors related to supported features used in an invalid way.
             Self::InvalidCertValidity => 190,

src/subject_name/dns_name.rs

@@ -12,34 +12,56 @@
 // 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::{Cert, Error};
 
 pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Result<(), Error> {
     let dns_name = untrusted::Input::from(reference.as_ref().as_bytes());
-    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 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)),
-            }
+        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 => return Err(Error::UnexpectedCertNameSimple),
+    }
+
+    // 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::UnexpectedCertName {
+            expected: ServerName::DnsName(reference.to_owned()),
+            presented: NameIterator::new(Some(cert.subject), cert.subject_alt_name)
+                .filter_map(|result| Some(format!("{:?}", result.ok()?)))
+                .collect(),
         })
-        .unwrap_or(Err(Error::CertNotValidForName))
+    }
 }
 
 /// A reference to a DNS Name presented by a server that may include a wildcard.

src/subject_name/ip_address.rs

@@ -12,7 +12,12 @@
 // 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::{Cert, Error};
@@ -23,24 +28,40 @@ pub(crate) fn verify_ip_address_names(reference: &IpAddr, cert: &Cert<'_>) -> Re
         IpAddr::V6(ip) => untrusted::Input::from(ip.as_ref()),
     };
 
-    NameIterator::new(None, cert.subject_alt_name)
-        .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,
-            }
+        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::UnexpectedCertNameSimple),
+    }
+
+    #[cfg(feature = "alloc")]
+    {
+        Err(Error::UnexpectedCertName {
+            expected: ServerName::from(*reference),
+            presented: NameIterator::new(None, cert.subject_alt_name)
+                .filter_map(|result| Some(format!("{:?}", result.ok()?)))
+                .collect(),
         })
-        .unwrap_or(Err(Error::CertNotValidForName))
+    }
 }
 
 // https://tools.ietf.org/html/rfc5280#section-4.2.1.6 says:

src/subject_name/verify.rs

@@ -12,6 +12,11 @@
 // 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 core::fmt;
+
 use super::dns_name::{self, IdRole};
 use super::ip_address;
 use crate::der::{self, FromDer};
@@ -295,3 +300,136 @@ impl<'a> FromDer<'a> for GeneralName<'a> {
 
     const TYPE_ID: DerTypeId = DerTypeId::GeneralName;
 }
+
+#[cfg(feature = "alloc")]
+impl fmt::Debug for GeneralName<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            GeneralName::DnsName(name) => write!(
+                f,
+                "DnsName(\"{}\")",
+                String::from_utf8_lossy(name.as_slice_less_safe())
+            ),
+            GeneralName::DirectoryName => write!(f, "DirectoryName"),
+            GeneralName::IpAddress(ip) => {
+                write!(f, "IpAddress({:?})", IpAddrSlice(ip.as_slice_less_safe()))
+            }
+            GeneralName::UniformResourceIdentifier(uri) => write!(
+                f,
+                "UniformResourceIdentifier(\"{}\")",
+                String::from_utf8_lossy(uri.as_slice_less_safe())
+            ),
+            GeneralName::Unsupported(tag) => write!(f, "Unsupported({})", tag),
+        }
+    }
+}
+
+#[cfg(feature = "alloc")]
+struct IpAddrSlice<'a>(&'a [u8]);
+
+#[cfg(feature = "alloc")]
+impl fmt::Debug for IpAddrSlice<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self.0.len() {
+            4 => {
+                let mut first = true;
+                for byte in self.0 {
+                    match first {
+                        true => first = false,
+                        false => f.write_str(".")?,
+                    }
+
+                    write!(f, "{}", byte)?;
+                }
+
+                Ok(())
+            }
+            16 => {
+                let mut first = true;
+                for group in self.0.chunks_exact(2) {
+                    match first {
+                        true => first = false,
+                        false => f.write_str(":")?,
+                    }
+
+                    match group {
+                        [0, 0] => f.write_str("0")?,
+                        _ => write!(f, "{:02x}{:02x}", group[0], group[1])?,
+                    }
+                }
+                Ok(())
+            }
+            _ => {
+                f.write_str("[invalid: ")?;
+                let mut first = true;
+                for byte in self.0 {
+                    match first {
+                        true => first = false,
+                        false => f.write_str(", ")?,
+                    }
+                    write!(f, "{:02x}", byte)?;
+                }
+                f.write_str("]")
+            }
+        }
+    }
+}
+
+#[cfg(all(test, feature = "alloc"))]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn debug_names() {
+        assert_eq!(
+            format!(
+                "{:?}",
+                GeneralName::DnsName(untrusted::Input::from(b"example.com"))
+            ),
+            "DnsName(\"example.com\")"
+        );
+
+        assert_eq!(format!("{:?}", GeneralName::DirectoryName), "DirectoryName");
+
+        assert_eq!(
+            format!(
+                "{:?}",
+                GeneralName::IpAddress(untrusted::Input::from(&[192, 0, 2, 1][..]))
+            ),
+            "IpAddress(192.0.2.1)"
+        );
+
+        assert_eq!(
+            format!(
+                "{:?}",
+                GeneralName::IpAddress(untrusted::Input::from(
+                    &[0x20, 0x01, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x0d, 0xb8][..]
+                ))
+            ),
+            "IpAddress(2001:0:0:0:0:0:0:0db8)"
+        );
+
+        assert_eq!(
+            format!(
+                "{:?}",
+                GeneralName::IpAddress(untrusted::Input::from(&[1, 2, 3, 4, 5, 6][..]))
+            ),
+            "IpAddress([invalid: 01, 02, 03, 04, 05, 06])"
+        );
+
+        assert_eq!(
+            format!(
+                "{:?}",
+                GeneralName::UniformResourceIdentifier(untrusted::Input::from(
+                    b"https://example.com"
+                ))
+            ),
+            "UniformResourceIdentifier(\"https://example.com\")"
+        );
+
+        assert_eq!(
+            format!("{:?}", GeneralName::Unsupported(0x42)),
+            "Unsupported(66)"
+        );
+    }
+}

tests/generate.py

@@ -286,6 +286,16 @@ def generate_tls_server_cert_test(
 
     valid_names_str: str = ", ".join('"' + name + '"' for name in valid_names)
     invalid_names_str: str = ", ".join('"' + name + '"' for name in invalid_names)
+    presented_names_str: str = ""
+    for san in sans if sans is not None else []:
+        if presented_names_str:
+            presented_names_str += ", "
+        if isinstance(san, x509.DNSName):
+            presented_names_str += f'"DnsName(\\"{san.value}\\")"'
+        elif isinstance(san, x509.IPAddress):
+            presented_names_str += f'"IpAddress({san.value})"'
+        else:
+            raise ValueError(f"Unsupported SAN type: {type(san)}")
 
     print(
         """
@@ -294,7 +304,7 @@ def generate_tls_server_cert_test(
     let ee = include_bytes!("%(ee_cert_path)s");
     let ca = include_bytes!("%(ca_cert_path)s");
     assert_eq!(
-        check_cert(ee, ca, &[%(valid_names_str)s], &[%(invalid_names_str)s]),
+        check_cert(ee, ca, &[%(valid_names_str)s], &[%(invalid_names_str)s], &[%(presented_names_str)s)]),
         %(expected)s
     );
 }"""