Add context in name validation errors

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]
@@ -33,7 +40,7 @@ pub enum Error {
     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.
@@ -127,6 +134,12 @@ 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,
+
     /// A valid issuer for the certificate could not be found.
     UnknownIssuer,
 
@@ -216,7 +229,7 @@ impl Error {
         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,
@@ -225,6 +238,7 @@ impl Error {
             Self::PathLenConstraintViolated => 220,
             Self::CaUsedAsEndEntity | Self::EndEntityUsedAsCa => 210,
             Self::IssuerNotCrlSigner => 200,
+            Self::UnexpectedCertNameSimple => 280,
 
             // Errors related to supported features used in an invalid way.
             Self::InvalidCertValidity => 190,
@@ -300,6 +314,22 @@ impl fmt::Display for Error {
 #[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.
+    ///
+    /// Unlike `expected`, these names might contain wildcard labels, and can contain both DNS
+    /// and 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,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};
+use crate::{error::InvalidNameContext, 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)),
-            }
-        })
-        .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.

src/subject_name/ip_address.rs

@@ -12,35 +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 pki_types::IpAddr;
+#[cfg(feature = "alloc")]
+use pki_types::ServerName;
 
 use super::verify::{GeneralName, NameIterator};
-use crate::{Cert, Error};
+use crate::{error::InvalidNameContext, Cert, 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()),
     };
 
-    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,
-            }
-        })
-        .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:

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,148 @@ 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, mut skipping) = (true, false);
+                for group in self.0.chunks_exact(2) {
+                    match (first, group == [0, 0], skipping) {
+                        (true, _, _) => first = false,
+                        (false, false, false) => f.write_str(":")?,
+                        (false, true, _) => {
+                            skipping = true;
+                            continue;
+                        }
+                        (false, false, true) => {
+                            skipping = false;
+                            f.write_str("::")?;
+                        }
+                    }
+
+                    if group[0] != 0 {
+                        write!(f, "{:x}", group[0])?;
+                    }
+
+                    match group[0] {
+                        0 => write!(f, "{:x}", group[1])?,
+                        _ => write!(f, "{:02x}", 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::db8)"
+        );
+
+        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)"
+        );
+    }
+}