From c56b3e091ab22ee6718b314137c6e863372394d1 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 27 Aug 2026 14:49:57 +0100 Subject: [PATCH 1/8] Improve errors from DNS name validation --- src/error.rs | 20 ++++++++++++++++ src/subject_name/dns_name.rs | 44 ++++++++++++++++++------------------ 2 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/error.rs b/src/error.rs index e228dd1c..a61cd3b2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -430,3 +430,23 @@ pub enum DerTypeId { SubjectUniqueId, KeyUsageExtension, } + +/// Errors possible when parsing a DNS name. +/// +/// This applies when parsing presented names, asserted names, or name constraints. +#[expect(missing_docs)] +#[non_exhaustive] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum DnsNameError { + IllegalCharacter(u8), + LabelMustNotBeEmpty, + LabelMustNotEndWithHyphen, + LabelMustNotStartWithHyphen, + LabelTooLong, + LastLabelMustNotBeAllNumeric, + NameMustBeRelative, + NameTooLong, + Truncated, + WildcardAsteriskMustBeAloneInLabel, + WildcardMustPrecedeTwoLabels, +} diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index 816649bd..4cb01947 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -22,7 +22,7 @@ use pki_types::{DnsName, InvalidDnsNameError}; use super::{GeneralName, NameIterator}; use crate::cert::Cert; -use crate::error::{Error, InvalidNameContext}; +use crate::error::{DnsNameError, Error, InvalidNameContext}; use crate::subject_name::Subtrees; pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Result<(), Error> { @@ -85,7 +85,7 @@ impl<'a> WildcardDnsNameRef<'a> { /// Constructs a `WildcardDnsNameRef` from the given input if the input is a /// syntactically-valid DNS name. pub(crate) fn try_from_ascii(dns_name: &'a [u8]) -> Result { - if !is_valid_dns_id( + if let Err(_) = is_valid_dns_id( untrusted::Input::from(dns_name), IdRole::Reference, Wildcards::Allow, @@ -240,11 +240,11 @@ pub(super) fn presented_id_matches_reference_id( reference_dns_id_role: IdRole, reference_dns_id: untrusted::Input<'_>, ) -> Result { - if !is_valid_dns_id(presented_dns_id, IdRole::Presented, Wildcards::Allow) { + if let Err(_) = is_valid_dns_id(presented_dns_id, IdRole::Presented, Wildcards::Allow) { return Err(Error::MalformedDnsIdentifier); } - if !is_valid_dns_id(reference_dns_id, reference_dns_id_role, Wildcards::Deny) { + if let Err(_) = is_valid_dns_id(reference_dns_id, reference_dns_id_role, Wildcards::Deny) { return Err(match reference_dns_id_role { IdRole::NameConstraint(_) => Error::MalformedNameConstraint, _ => Error::MalformedDnsIdentifier, @@ -401,16 +401,16 @@ fn is_valid_dns_id( hostname: untrusted::Input<'_>, id_role: IdRole, allow_wildcards: Wildcards, -) -> bool { +) -> Result<(), DnsNameError> { // https://blogs.msdn.microsoft.com/oldnewthing/20120412-00/?p=7873/ if hostname.len() > 253 { - return false; + return Err(DnsNameError::NameTooLong); } let mut input = untrusted::Reader::new(hostname); if matches!(id_role, IdRole::NameConstraint(_)) && input.at_end() { - return true; + return Ok(()); } let mut dot_count = 0; @@ -425,7 +425,7 @@ fn is_valid_dns_id( let mut is_first_byte = !is_wildcard; if is_wildcard { if input.read_byte() != Ok(b'*') || input.read_byte() != Ok(b'.') { - return false; + return Err(DnsNameError::WildcardAsteriskMustBeAloneInLabel); } dot_count += 1; } @@ -436,13 +436,13 @@ fn is_valid_dns_id( match input.read_byte() { Ok(b'-') => { if label_length == 0 { - return false; // Labels must not start with a hyphen. + return Err(DnsNameError::LabelMustNotStartWithHyphen); } label_is_all_numeric = false; label_ends_with_hyphen = true; label_length += 1; if label_length > MAX_LABEL_LENGTH { - return false; + return Err(DnsNameError::LabelTooLong); } } @@ -453,7 +453,7 @@ fn is_valid_dns_id( label_ends_with_hyphen = false; label_length += 1; if label_length > MAX_LABEL_LENGTH { - return false; + return Err(DnsNameError::LabelTooLong); } } @@ -462,7 +462,7 @@ fn is_valid_dns_id( label_ends_with_hyphen = false; label_length += 1; if label_length > MAX_LABEL_LENGTH { - return false; + return Err(DnsNameError::LabelTooLong); } } @@ -470,17 +470,17 @@ fn is_valid_dns_id( dot_count += 1; let name_constrained = matches!(id_role, IdRole::NameConstraint(_)); if label_length == 0 && (!name_constrained || !is_first_byte) { - return false; + return Err(DnsNameError::LabelMustNotBeEmpty); } if label_ends_with_hyphen { - return false; // Labels must not end with a hyphen. + return Err(DnsNameError::LabelMustNotEndWithHyphen); } label_length = 0; } - _ => { - return false; - } + Ok(x) => return Err(DnsNameError::IllegalCharacter(x)), + + Err(_) => return Err(DnsNameError::Truncated), } is_first_byte = false; @@ -492,15 +492,15 @@ fn is_valid_dns_id( // Only reference IDs, not presented IDs or name constraints, may be // absolute. if label_length == 0 && id_role != IdRole::Reference { - return false; + return Err(DnsNameError::NameMustBeRelative); } if label_ends_with_hyphen { - return false; // Labels must not end with a hyphen. + return Err(DnsNameError::LabelMustNotEndWithHyphen); } if label_is_all_numeric { - return false; // Last label must not be all numeric. + return Err(DnsNameError::LastLabelMustNotBeAllNumeric); } if is_wildcard { @@ -516,11 +516,11 @@ fn is_valid_dns_id( // similar to Chromium. Even then, it might be better to still enforce // that there are at least two labels after the wildcard. if label_count < 3 { - return false; + return Err(DnsNameError::WildcardMustPrecedeTwoLabels); } } - true + Ok(()) } #[cfg(test)] From ffddca440d8d77696bd6da48fe00696c6e97fef5 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 27 Aug 2026 14:50:24 +0100 Subject: [PATCH 2/8] Expose errors from DNS identifier validation --- src/error.rs | 8 +- src/lib.rs | 2 +- src/subject_name/dns_name.rs | 275 ++++++++++++++++++++++++----------- 3 files changed, 195 insertions(+), 90 deletions(-) diff --git a/src/error.rs b/src/error.rs index a61cd3b2..140c0af3 100644 --- a/src/error.rs +++ b/src/error.rs @@ -138,7 +138,7 @@ pub enum Error { /// A presented or reference DNS identifier was malformed, potentially /// containing invalid characters or invalid labels. - MalformedDnsIdentifier, + MalformedDnsIdentifier(DnsNameError), /// The certificate extensions are malformed. /// @@ -149,7 +149,7 @@ pub enum Error { /// A name constraint was malformed, potentially containing invalid characters or /// invalid labels. - MalformedNameConstraint, + MalformedNameConstraint(DnsNameError), /// The maximum number of name constraint comparisons has been reached. MaximumNameConstraintComparisonsExceeded, @@ -302,8 +302,8 @@ impl Error { Self::MaximumPathDepthExceeded => 61, // Errors related to malformed data. - Self::MalformedDnsIdentifier => 60, - Self::MalformedNameConstraint => 50, + Self::MalformedDnsIdentifier(_) => 60, + Self::MalformedNameConstraint(_) => 50, Self::MalformedExtensions | Self::TrailingData(_) => 40, Self::ExtensionValueInvalid => 30, diff --git a/src/lib.rs b/src/lib.rs index 3501218a..be3a104b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -77,7 +77,7 @@ pub use crl::{OwnedCertRevocationList, OwnedRevokedCert}; pub use der::DerIterator; pub use end_entity::EndEntityCert; pub use error::{ - DerTypeId, Error, InvalidNameContext, UnsupportedSignatureAlgorithmContext, + DerTypeId, DnsNameError, Error, InvalidNameContext, UnsupportedSignatureAlgorithmContext, UnsupportedSignatureAlgorithmForPublicKeyContext, }; pub use rpk_entity::RawPublicKeyEntity; diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index 4cb01947..eeb1f83e 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -16,9 +16,9 @@ use alloc::format; use core::fmt::Write; +use pki_types::DnsName; #[cfg(feature = "alloc")] use pki_types::ServerName; -use pki_types::{DnsName, InvalidDnsNameError}; use super::{GeneralName, NameIterator}; use crate::cert::Cert; @@ -39,7 +39,7 @@ pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Resu match presented_id_matches_reference_id(presented_id, IdRole::Reference, dns_name) { Ok(true) => Some(Ok(())), - Ok(false) | Err(Error::MalformedDnsIdentifier) => None, + Ok(false) | Err(Error::MalformedDnsIdentifier(_)) => None, Err(e) => Some(Err(e)), } }); @@ -84,14 +84,12 @@ pub(crate) struct WildcardDnsNameRef<'a>(&'a [u8]); impl<'a> WildcardDnsNameRef<'a> { /// Constructs a `WildcardDnsNameRef` from the given input if the input is a /// syntactically-valid DNS name. - pub(crate) fn try_from_ascii(dns_name: &'a [u8]) -> Result { - if let Err(_) = is_valid_dns_id( + pub(crate) fn try_from_ascii(dns_name: &'a [u8]) -> Result { + is_valid_dns_id( untrusted::Input::from(dns_name), IdRole::Reference, Wildcards::Allow, - ) { - return Err(InvalidDnsNameError); - } + )?; Ok(Self(dns_name)) } @@ -240,14 +238,14 @@ pub(super) fn presented_id_matches_reference_id( reference_dns_id_role: IdRole, reference_dns_id: untrusted::Input<'_>, ) -> Result { - if let Err(_) = is_valid_dns_id(presented_dns_id, IdRole::Presented, Wildcards::Allow) { - return Err(Error::MalformedDnsIdentifier); + if let Err(e) = is_valid_dns_id(presented_dns_id, IdRole::Presented, Wildcards::Allow) { + return Err(Error::MalformedDnsIdentifier(e)); } - if let Err(_) = is_valid_dns_id(reference_dns_id, reference_dns_id_role, Wildcards::Deny) { + if let Err(e) = is_valid_dns_id(reference_dns_id, reference_dns_id_role, Wildcards::Deny) { return Err(match reference_dns_id_role { - IdRole::NameConstraint(_) => Error::MalformedNameConstraint, - _ => Error::MalformedDnsIdentifier, + IdRole::NameConstraint(_) => Error::MalformedNameConstraint(e), + _ => Error::MalformedDnsIdentifier(e), }); } @@ -346,7 +344,9 @@ pub(super) fn presented_id_matches_reference_id( if presented.at_end() { // Don't allow presented IDs to be absolute. if presented_byte == b'.' { - return Err(Error::MalformedDnsIdentifier); + return Err(Error::MalformedDnsIdentifier( + DnsNameError::NameMustBeRelative, + )); } break; } @@ -526,10 +526,11 @@ fn is_valid_dns_id( #[cfg(test)] mod tests { use super::*; + use crate::error::DnsNameError::*; #[expect(clippy::type_complexity)] const PRESENTED_MATCHES_REFERENCE: &[(&[u8], &[u8], Result)] = &[ - (b"", b"a", Err(Error::MalformedDnsIdentifier)), + (b"", b"a", Err(Error::MalformedDnsIdentifier(Truncated))), (b"a", b"a", Ok(true)), (b"b", b"a", Ok(false)), (b"*.b.a", b"c.b.a", Ok(true)), @@ -537,9 +538,21 @@ mod tests { (b"*.b.a", b"b.a.", Ok(false)), // Wildcard not in leftmost label (b"d.c.b.a", b"d.c.b.a", Ok(true)), - (b"d.*.b.a", b"d.c.b.a", Err(Error::MalformedDnsIdentifier)), - (b"d.c*.b.a", b"d.c.b.a", Err(Error::MalformedDnsIdentifier)), - (b"d.c*.b.a", b"d.cc.b.a", Err(Error::MalformedDnsIdentifier)), + ( + b"d.*.b.a", + b"d.c.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), + ( + b"d.c*.b.a", + b"d.c.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), + ( + b"d.c*.b.a", + b"d.cc.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), // case sensitivity ( b"abcdefghijklmnopqrstuvwxyz", @@ -557,70 +570,106 @@ mod tests { // A trailing dot indicates an absolute name, and absolute names can match // relative names, and vice-versa. (b"example", b"example", Ok(true)), - (b"example.", b"example.", Err(Error::MalformedDnsIdentifier)), + ( + b"example.", + b"example.", + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), + ), (b"example", b"example.", Ok(true)), - (b"example.", b"example", Err(Error::MalformedDnsIdentifier)), + ( + b"example.", + b"example", + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), + ), (b"example.com", b"example.com", Ok(true)), ( b"example.com.", b"example.com.", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), ), (b"example.com", b"example.com.", Ok(true)), ( b"example.com.", b"example.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), ), ( b"example.com..", b"example.com.", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(LabelMustNotBeEmpty)), ), ( b"example.com..", b"example.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(LabelMustNotBeEmpty)), ), ( b"example.com...", b"example.com.", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(LabelMustNotBeEmpty)), ), // xn-- IDN prefix - (b"x*.b.a", b"xa.b.a", Err(Error::MalformedDnsIdentifier)), - (b"x*.b.a", b"xna.b.a", Err(Error::MalformedDnsIdentifier)), - (b"x*.b.a", b"xn-a.b.a", Err(Error::MalformedDnsIdentifier)), - (b"x*.b.a", b"xn--a.b.a", Err(Error::MalformedDnsIdentifier)), - (b"xn*.b.a", b"xn--a.b.a", Err(Error::MalformedDnsIdentifier)), + ( + b"x*.b.a", + b"xa.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), + ( + b"x*.b.a", + b"xna.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), + ( + b"x*.b.a", + b"xn-a.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), + ( + b"x*.b.a", + b"xn--a.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), + ( + b"xn*.b.a", + b"xn--a.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), ( b"xn-*.b.a", b"xn--a.b.a", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"xn--*.b.a", b"xn--a.b.a", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), + ( + b"xn*.b.a", + b"xn--a.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), - (b"xn*.b.a", b"xn--a.b.a", Err(Error::MalformedDnsIdentifier)), ( b"xn-*.b.a", b"xn--a.b.a", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"xn--*.b.a", b"xn--a.b.a", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"xn---*.b.a", b"xn--a.b.a", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), // "*" cannot expand to nothing. - (b"c*.b.a", b"c.b.a", Err(Error::MalformedDnsIdentifier)), + ( + b"c*.b.a", + b"c.b.a", + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), + ), // -------------------------------------------------------------------------- // The rest of these are test cases adapted from Chromium's // x509_certificate_unittest.cc. The parameter order is the opposite in @@ -633,66 +682,76 @@ mod tests { (b"*.foo.com", b"bar.foo.com", Ok(true)), (b"*.test.fr", b"www.test.fr", Ok(true)), (b"*.test.FR", b"wwW.tESt.fr", Ok(true)), - (b".uk", b"f.uk", Err(Error::MalformedDnsIdentifier)), + ( + b".uk", + b"f.uk", + Err(Error::MalformedDnsIdentifier(LabelMustNotBeEmpty)), + ), ( b"?.bar.foo.com", b"w.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'?'))), ), ( b"(www|ftp).foo.com", b"www.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'('))), ), // regex! ( b"www.foo.com\0", b"www.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(0))), ), ( b"www.foo.com\0*.foo.com", b"www.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(0))), ), (b"ww.house.example", b"www.house.example", Ok(false)), (b"www.test.org", b"test.org", Ok(false)), (b"*.test.org", b"test.org", Ok(false)), - (b"*.org", b"test.org", Err(Error::MalformedDnsIdentifier)), + ( + b"*.org", + b"test.org", + Err(Error::MalformedDnsIdentifier(WildcardMustPrecedeTwoLabels)), + ), // '*' must be the only character in the wildcard label ( b"w*.bar.foo.com", b"w.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"ww*ww.bar.foo.com", b"www.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"ww*ww.bar.foo.com", b"wwww.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"w*w.bar.foo.com", b"wwww.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"w*w.bar.foo.c0m", b"wwww.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"wa*.bar.foo.com", b"WALLY.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"*Ly.bar.foo.com", b"wally.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier( + WildcardAsteriskMustBeAloneInLabel, + )), ), // Chromium does URL decoding of the reference ID, but we don't, and we also // require that the reference ID is valid, so we can't test these two. @@ -702,20 +761,20 @@ mod tests { ( b"*.jp", b"www.test.co.jp", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(WildcardMustPrecedeTwoLabels)), ), (b"www.test.co.uk", b"www.test.co.jp", Ok(false)), ( b"www.*.co.jp", b"www.test.co.jp", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), (b"www.bar.foo.com", b"www.bar.foo.com", Ok(true)), (b"*.foo.com", b"www.bar.foo.com", Ok(false)), ( b"*.*.foo.com", b"www.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), // Our matcher requires the reference ID to be a valid DNS name, so we cannot // test this case. @@ -746,17 +805,19 @@ mod tests { ( b"xn--poema-*.com.br", b"xn--poema-9qae5a.com.br", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"xn--*-9qae5a.com.br", b"xn--poema-9qae5a.com.br", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"*--poema-9qae5a.com.br", b"xn--poema-9qae5a.com.br", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier( + WildcardAsteriskMustBeAloneInLabel, + )), ), // The following are adapted from the examples quoted from // http://tools.ietf.org/html/rfc6125#section-6.4.3 @@ -768,17 +829,19 @@ mod tests { ( b"baz*.example.net", b"baz1.example.net", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"*baz.example.net", b"foobaz.example.net", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier( + WildcardAsteriskMustBeAloneInLabel, + )), ), ( b"b*z.example.net", b"buzz.example.net", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), // Wildcards should not be valid for public registry controlled domains, // and unknown/unrecognized domains, at least three domain components must @@ -789,15 +852,29 @@ mod tests { ( b"*.example", b"test.example", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(WildcardMustPrecedeTwoLabels)), ), // The result is different than Chromium, because Chromium takes into account // the additional knowledge it has that "co.uk" is a TLD. mozilla::pkix does // not know that. (b"*.co.uk", b"example.co.uk", Ok(true)), - (b"*.com", b"foo.com", Err(Error::MalformedDnsIdentifier)), - (b"*.us", b"foo.us", Err(Error::MalformedDnsIdentifier)), - (b"*", b"foo", Err(Error::MalformedDnsIdentifier)), + ( + b"*.com", + b"foo.com", + Err(Error::MalformedDnsIdentifier(WildcardMustPrecedeTwoLabels)), + ), + ( + b"*.us", + b"foo.us", + Err(Error::MalformedDnsIdentifier(WildcardMustPrecedeTwoLabels)), + ), + ( + b"*", + b"foo", + Err(Error::MalformedDnsIdentifier( + WildcardAsteriskMustBeAloneInLabel, + )), + ), // IDN variants of wildcards and registry controlled domains. ( b"*.xn--poema-9qae5a.com.br", @@ -815,7 +892,7 @@ mod tests { ( b"*.xn--mgbaam7a8h", b"example.xn--mgbaam7a8h", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(WildcardMustPrecedeTwoLabels)), ), // Wildcards should be permissible for 'private' registry-controlled // domains. (In mozilla::pkix, we do not know if it is a private registry- @@ -826,33 +903,49 @@ mod tests { ( b"*.*.com", b"foo.example.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), ( b"*.bar.*.com", b"foo.bar.example.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(IllegalCharacter(b'*'))), ), // Absolute vs relative DNS name tests. Although not explicitly specified // in RFC 6125, absolute reference names (those ending in a .) should // match either absolute or relative presented names. // TODO: File errata against RFC 6125 about this. - (b"foo.com.", b"foo.com", Err(Error::MalformedDnsIdentifier)), + ( + b"foo.com.", + b"foo.com", + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), + ), (b"foo.com", b"foo.com.", Ok(true)), - (b"foo.com.", b"foo.com.", Err(Error::MalformedDnsIdentifier)), - (b"f.", b"f", Err(Error::MalformedDnsIdentifier)), + ( + b"foo.com.", + b"foo.com.", + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), + ), + ( + b"f.", + b"f", + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), + ), (b"f", b"f.", Ok(true)), - (b"f.", b"f.", Err(Error::MalformedDnsIdentifier)), + ( + b"f.", + b"f.", + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), + ), ( b"*.bar.foo.com.", b"www-3.bar.foo.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), ), (b"*.bar.foo.com", b"www-3.bar.foo.com.", Ok(true)), ( b"*.bar.foo.com.", b"www-3.bar.foo.com.", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), ), // We require the reference ID to be a valid DNS name, so we cannot test this // case. @@ -860,31 +953,35 @@ mod tests { ( b"*.com.", b"example.com", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), ), ( b"*.com", b"example.com.", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(WildcardMustPrecedeTwoLabels)), ), ( b"*.com.", b"example.com.", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), + ), + ( + b"*.", + b"foo.", + Err(Error::MalformedDnsIdentifier(Truncated)), ), - (b"*.", b"foo.", Err(Error::MalformedDnsIdentifier)), - (b"*.", b"foo", Err(Error::MalformedDnsIdentifier)), + (b"*.", b"foo", Err(Error::MalformedDnsIdentifier(Truncated))), // The result is different than Chromium because we don't know that co.uk is // a TLD. ( b"*.co.uk.", b"foo.co.uk", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), ), ( b"*.co.uk.", b"foo.co.uk.", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), ), ]; @@ -907,32 +1004,40 @@ mod tests { #[expect(clippy::type_complexity)] const PRESENTED_MATCHES_CONSTRAINT: &[(&[u8], &[u8], Result)] = &[ // No absolute presented IDs allowed - (b".", b"", Err(Error::MalformedDnsIdentifier)), - (b"www.example.com.", b"", Err(Error::MalformedDnsIdentifier)), + ( + b".", + b"", + Err(Error::MalformedDnsIdentifier(LabelMustNotBeEmpty)), + ), + ( + b"www.example.com.", + b"", + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), + ), ( b"www.example.com.", b"www.example.com.", - Err(Error::MalformedDnsIdentifier), + Err(Error::MalformedDnsIdentifier(NameMustBeRelative)), ), // No absolute constraints allowed ( b"www.example.com", b".", - Err(Error::MalformedNameConstraint), + Err(Error::MalformedNameConstraint(NameMustBeRelative)), ), ( b"www.example.com", b"www.example.com.", - Err(Error::MalformedNameConstraint), + Err(Error::MalformedNameConstraint(NameMustBeRelative)), ), // No wildcard in constraints allowed ( b"www.example.com", b"*.example.com", - Err(Error::MalformedNameConstraint), + Err(Error::MalformedNameConstraint(IllegalCharacter(b'*'))), ), // No empty presented IDs allowed - (b"", b"", Err(Error::MalformedDnsIdentifier)), + (b"", b"", Err(Error::MalformedDnsIdentifier(Truncated))), // Empty constraints match everything allowed (b"example.com", b"", Ok(true)), (b"*.example.com", b"", Ok(true)), From 6648849652a4f68079c8ca4ee719afdd38b343cd Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 27 Aug 2026 16:53:10 +0100 Subject: [PATCH 3/8] tls_server_certs: top-down reordering --- tests/tls_server_certs.rs | 134 +++++++++++++++++++------------------- 1 file changed, 67 insertions(+), 67 deletions(-) diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index 4f8288bf..b5e499c6 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -26,48 +26,6 @@ use webpki::{ExtendedKeyUsage, InvalidNameContext, PathBuilder, anchor_from_trus mod common; use common::issuer_params; -#[track_caller] -fn check_cert( - ee: &[u8], - ca: &[u8], - valid_names: &[&str], - invalid_names: &[&str], - presented_names: &[&str], -) -> Result<(), webpki::Error> { - let ca_cert_der = CertificateDer::from(ca); - let anchors = [anchor_from_trusted_cert(&ca_cert_der).unwrap()]; - let builder = PathBuilder::new( - &[], - None, - &ExtendedKeyUsage::SERVER_AUTH, - rustls_aws_lc_rs::ALL_VERIFICATION_ALGS, - &anchors, - ); - - let ee_der = CertificateDer::from(ee); - let time = UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d)); - let cert = webpki::EndEntityCert::try_from(&ee_der).unwrap(); - builder.build(&cert, time)?; - - for valid in valid_names { - let name = ServerName::try_from(*valid).unwrap(); - assert_eq!(cert.verify_is_valid_for_subject_name(&name), Ok(())); - } - - for invalid in invalid_names { - let name = ServerName::try_from(*invalid).unwrap(); - assert_eq!( - cert.verify_is_valid_for_subject_name(&name), - Err(webpki::Error::CertNotValidForName(InvalidNameContext { - expected: name.to_owned(), - presented: presented_names.iter().map(|n| n.to_string()).collect(), - })) - ); - } - - Ok(()) -} - #[test] fn no_name_constraints() { let issuer = make_issuer(None); @@ -742,6 +700,33 @@ fn invalid_dns_name_matching() { ); } +#[test] +fn presented_names_escape_control_characters() { + // `InvalidNameContext::presented` is public API built by formatting the SAN + // entries, and a certificate can carry anything there. Whatever a caller + // does with those strings, they should not contain raw control characters. + let issuer = make_issuer(None); + let ee = generate_cert_with_names( + None, + None, + vec![SanType::DnsName( + "a\r\nInjected: header\u{1b}[31m".try_into().unwrap(), + )], + &issuer, + ); + + assert_eq!( + check_cert( + ee.der(), + issuer.der(), + &[], + &["real.example.com"], + &[r#"DnsName("a\r\nInjected: header\u{1b}[31m")"#], + ), + Ok(()) + ); +} + fn generate_cert(sans: Vec, issuer: &CertifiedIssuer<'_, KeyPair>) -> Certificate { generate_cert_with_names(None, None, sans, issuer) } @@ -785,32 +770,47 @@ fn make_issuer(name_constraints: Option) -> CertifiedIssuer<'st CertifiedIssuer::self_signed(ca_params, ca_key).expect("failed to generate CA cert") } -// OID for emailAddress in subject DN (pkcs9-emailAddress) -const OID_EMAIL_ADDRESS: &[u64] = &[1, 2, 840, 113549, 1, 9, 1]; - -#[test] -fn presented_names_escape_control_characters() { - // `InvalidNameContext::presented` is public API built by formatting the SAN - // entries, and a certificate can carry anything there. Whatever a caller - // does with those strings, they should not contain raw control characters. - let issuer = make_issuer(None); - let ee = generate_cert_with_names( - None, +#[track_caller] +fn check_cert( + ee: &[u8], + ca: &[u8], + valid_names: &[&str], + invalid_names: &[&str], + presented_names: &[&str], +) -> Result<(), webpki::Error> { + let ca_cert_der = CertificateDer::from(ca); + let anchors = [anchor_from_trusted_cert(&ca_cert_der).unwrap()]; + let builder = PathBuilder::new( + &[], None, - vec![SanType::DnsName( - "a\r\nInjected: header\u{1b}[31m".try_into().unwrap(), - )], - &issuer, + &ExtendedKeyUsage::SERVER_AUTH, + rustls_aws_lc_rs::ALL_VERIFICATION_ALGS, + &anchors, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &[], - &["real.example.com"], - &[r#"DnsName("a\r\nInjected: header\u{1b}[31m")"#], - ), - Ok(()) - ); + let ee_der = CertificateDer::from(ee); + let time = UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d)); + let cert = webpki::EndEntityCert::try_from(&ee_der).unwrap(); + builder.build(&cert, time)?; + + for valid in valid_names { + let name = ServerName::try_from(*valid).unwrap(); + assert_eq!(cert.verify_is_valid_for_subject_name(&name), Ok(())); + } + + for invalid in invalid_names { + let name = ServerName::try_from(*invalid).unwrap(); + assert_eq!( + cert.verify_is_valid_for_subject_name(&name), + Err(webpki::Error::CertNotValidForName(InvalidNameContext { + expected: name.to_owned(), + presented: presented_names.iter().map(|n| n.to_string()).collect(), + })) + ); + } + + Ok(()) } + +// OID for emailAddress in subject DN (pkcs9-emailAddress) +const OID_EMAIL_ADDRESS: &[u64] = &[1, 2, 840, 113549, 1, 9, 1]; From d93e81d70e07cc9426234f2e2fcb6904228c7f3e Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 27 Aug 2026 16:57:36 +0100 Subject: [PATCH 4/8] tls_server_certs: inline `uri_name_constraints` wrappers --- tests/tls_server_certs.rs | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index b5e499c6..250a5431 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -575,11 +575,10 @@ fn ip46_mixed_address_san_allowed() { fn uri_san_rejected_against_uri_permitted_subtree() { let ca_key = KeyPair::generate().unwrap(); let mut ca_params = issuer_params("issuer.example.com").unwrap(); - ca_params - .custom_extensions - .push(uri_permitted_name_constraints( - b"https://allowed.example.com", - )); + ca_params.custom_extensions.push(uri_name_constraints( + b"https://allowed.example.com", + SubtreesTag::PermittedSubtrees, + )); let issuer = CertifiedIssuer::self_signed(ca_params, ca_key).expect("failed to generate CA"); let ee = generate_cert( @@ -597,9 +596,10 @@ fn uri_san_rejected_against_uri_permitted_subtree() { fn uri_san_rejected_against_uri_excluded_subtree() { let ca_key = KeyPair::generate().unwrap(); let mut ca_params = issuer_params("issuer.example.com").unwrap(); - ca_params - .custom_extensions - .push(uri_excluded_name_constraints(b"https://evil.example.com")); + ca_params.custom_extensions.push(uri_name_constraints( + b"https://evil.example.com", + SubtreesTag::ExcludedSubtrees, + )); let issuer = CertifiedIssuer::self_signed(ca_params, ca_key).expect("failed to generate CA"); let ee = generate_cert( @@ -612,20 +612,7 @@ fn uri_san_rejected_against_uri_excluded_subtree() { ); } -// Hand-encode a NameConstraints extension (OID 2.5.29.30) with a single -// permittedSubtree containing a URI GeneralName. rcgen's GeneralSubtree enum -// doesn't expose a URI variant, so we emit the DER directly. -fn uri_permitted_name_constraints(uri: &[u8]) -> CustomExtension { - uri_name_constraints(uri, 0xa0) // permittedSubtrees [0] IMPLICIT -} - -// Hand-encode a NameConstraints extension (OID 2.5.29.30) with a single -// excludedSubtree containing a URI GeneralName. -fn uri_excluded_name_constraints(uri: &[u8]) -> CustomExtension { - uri_name_constraints(uri, 0xa1) // excludedSubtrees [1] IMPLICIT -} - -fn uri_name_constraints(uri: &[u8], subtrees_tag: u8) -> CustomExtension { +fn uri_name_constraints(uri: &[u8], subtrees_tag: SubtreesTag) -> CustomExtension { assert!(uri.len() < 128); // URI GeneralName: [6] IMPLICIT IA5String let mut uri_gn = vec![0x86, uri.len() as u8]; @@ -634,7 +621,7 @@ fn uri_name_constraints(uri: &[u8], subtrees_tag: u8) -> CustomExtension { let mut subtree = vec![0x30, uri_gn.len() as u8]; subtree.extend_from_slice(&uri_gn); // permittedSubtrees [0] or excludedSubtrees [1] IMPLICIT GeneralSubtrees - let mut subtrees = vec![subtrees_tag, subtree.len() as u8]; + let mut subtrees = vec![subtrees_tag as u8, subtree.len() as u8]; subtrees.extend_from_slice(&subtree); // NameConstraints SEQUENCE let mut nc = vec![0x30, subtrees.len() as u8]; @@ -645,6 +632,12 @@ fn uri_name_constraints(uri: &[u8], subtrees_tag: u8) -> CustomExtension { ext } +#[repr(u8)] +enum SubtreesTag { + PermittedSubtrees = 0xa0, + ExcludedSubtrees = 0xa1, +} + #[test] fn permit_directory_name_not_implemented() { let mut dn = DistinguishedName::new(); From 1877635bc260c8ad59592a2c48fdf75613df8d86 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 27 Aug 2026 17:34:22 +0100 Subject: [PATCH 5/8] tls_server_certs: rework `check_cert` helper --- tests/tls_server_certs.rs | 378 +++++++++++++++----------------------- 1 file changed, 147 insertions(+), 231 deletions(-) diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index 250a5431..beed6379 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -21,7 +21,10 @@ use rcgen::{ DistinguishedName, DnType, GeneralSubtree, IsCa, KeyPair, NameConstraints, SanType, date_time_ymd, }; -use webpki::{ExtendedKeyUsage, InvalidNameContext, PathBuilder, anchor_from_trusted_cert}; +use webpki::{ + EndEntityCert, Error, ExtendedKeyUsage, InvalidNameContext, PathBuilder, + anchor_from_trusted_cert, +}; mod common; use common::issuer_params; @@ -35,16 +38,10 @@ fn no_name_constraints() { vec![SanType::DnsName("dns.example.com".try_into().unwrap())], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["dns.example.com"], - &["subject.example.com"], - &["DnsName(\"dns.example.com\")"] - ), - Ok(()) - ); + check_cert(ee.der(), issuer.der(), &["DnsName(\"dns.example.com\")"]) + .unwrap() + .assert_valid_name("dns.example.com") + .assert_invalid_name("subject.example.com"); } #[test] @@ -62,19 +59,18 @@ fn additional_dns_labels() { ], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["host1.example.com", "host2.example.com"], - &["subject.example.com"], - &[ - "DnsName(\"host1.example.com\")", - "DnsName(\"host2.example.com\")" - ] - ), - Ok(()) - ); + check_cert( + ee.der(), + issuer.der(), + &[ + "DnsName(\"host1.example.com\")", + "DnsName(\"host2.example.com\")", + ], + ) + .unwrap() + .assert_valid_name("host1.example.com") + .assert_valid_name("host2.example.com") + .assert_invalid_name("subject.example.com"); } #[test] @@ -95,11 +91,9 @@ fn disallow_dns_san() { check_cert( ee.der(), issuer.der(), - &[], - &[], &["DnsName(\"disallowed.example.com\")"] ), - Err(webpki::Error::NameConstraintViolation) + Err(Error::NameConstraintViolation) ); } @@ -110,10 +104,9 @@ fn allow_subject_common_name() { excluded_subtrees: vec![], })); let ee = generate_cert_with_names(Some("allowed.example.com"), None, vec![], &issuer); - assert_eq!( - check_cert(ee.der(), issuer.der(), &[], &["allowed.example.com"], &[]), - Ok(()) - ); + check_cert(ee.der(), issuer.der(), &[]) + .unwrap() + .assert_invalid_name("allowed.example.com"); } #[test] @@ -126,16 +119,13 @@ fn allow_dns_san() { vec![SanType::DnsName("allowed.example.com".try_into().unwrap())], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["allowed.example.com"], - &[], - &["DnsName(\"allowed.example.com\")"] - ), - Ok(()) - ); + check_cert( + ee.der(), + issuer.der(), + &["DnsName(\"allowed.example.com\")"], + ) + .unwrap() + .assert_valid_name("allowed.example.com"); } #[test] @@ -155,16 +145,14 @@ fn allow_dns_san_and_subject_common_name() { )], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["allowed-san.example.com"], - &["allowed-cn.example.com"], - &["DnsName(\"allowed-san.example.com\")"] - ), - Ok(()) - ); + check_cert( + ee.der(), + issuer.der(), + &["DnsName(\"allowed-san.example.com\")"], + ) + .unwrap() + .assert_valid_name("allowed-san.example.com") + .assert_invalid_name("allowed-cn.example.com"); } #[test] @@ -191,14 +179,12 @@ fn disallow_dns_san_and_allow_subject_common_name() { check_cert( ee.der(), issuer.der(), - &[], - &[], &[ "DnsName(\"allowed-san.example.com\")", "DnsName(\"disallowed-san.example.com\")" ] ), - Err(webpki::Error::NameConstraintViolation) + Err(Error::NameConstraintViolation) ); } @@ -211,7 +197,7 @@ fn we_incorrectly_ignore_name_constraints_on_name_in_subject() { let ee = generate_cert_with_names(None, Some("test@example.com"), vec![], &issuer); // webpki incorrectly ignores name constraints on email addresses in the subject DN // The email in subject should be checked against constraints, but it isn't - assert_eq!(check_cert(ee.der(), issuer.der(), &[], &[], &[]), Ok(())); + check_cert(ee.der(), issuer.der(), &[]).unwrap(); } #[test] @@ -225,8 +211,8 @@ fn reject_constraints_on_unimplemented_names() { &issuer, ); assert_eq!( - check_cert(ee.der(), issuer.der(), &[], &[], &[]), - Err(webpki::Error::NameConstraintViolation) + check_cert(ee.der(), issuer.der(), &[]), + Err(Error::NameConstraintViolation) ); } @@ -240,16 +226,10 @@ fn we_ignore_constraints_on_names_that_do_not_appear_in_cert() { vec![SanType::DnsName("notexample.com".try_into().unwrap())], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["notexample.com"], - &["example.com"], - &["DnsName(\"notexample.com\")"] - ), - Ok(()) - ); + check_cert(ee.der(), issuer.der(), &["DnsName(\"notexample.com\")"]) + .unwrap() + .assert_valid_name("notexample.com") + .assert_invalid_name("example.com"); } #[test] @@ -262,16 +242,12 @@ fn wildcard_san_accepted_if_in_subtree() { vec![SanType::DnsName("*.example.com".try_into().unwrap())], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["bob.example.com", "jane.example.com"], - &["example.com", "uh.oh.example.com"], - &["DnsName(\"*.example.com\")"] - ), - Ok(()) - ); + check_cert(ee.der(), issuer.der(), &["DnsName(\"*.example.com\")"]) + .unwrap() + .assert_valid_name("bob.example.com") + .assert_valid_name("jane.example.com") + .assert_invalid_name("example.com") + .assert_invalid_name("uh.oh.example.com"); } #[test] @@ -285,14 +261,8 @@ fn wildcard_san_rejected_if_in_excluded_subtree() { &issuer, ); assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &[], - &[], - &["DnsName(\"*.example.com\")"] - ), - Err(webpki::Error::NameConstraintViolation) + check_cert(ee.der(), issuer.der(), &["DnsName(\"*.example.com\")"]), + Err(Error::NameConstraintViolation) ); } @@ -311,14 +281,8 @@ fn wildcard_san_rejected_if_could_match_excluded_subtree() { &issuer, ); assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &[], - &[], - &["DnsName(\"*.example.com\")"] - ), - Err(webpki::Error::NameConstraintViolation) + check_cert(ee.der(), issuer.der(), &["DnsName(\"*.example.com\")"]), + Err(Error::NameConstraintViolation) ); } @@ -337,14 +301,8 @@ fn wildcard_san_rejected_if_could_match_name_outside_permitted_subtree() { &issuer, ); assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &[], - &[], - &["DnsName(\"*.example.com\")"] - ), - Err(webpki::Error::NameConstraintViolation) + check_cert(ee.der(), issuer.der(), &["DnsName(\"*.example.com\")"]), + Err(Error::NameConstraintViolation) ); } @@ -362,14 +320,8 @@ fn ip4_address_san_rejected_if_in_excluded_subtree() { &issuer, ); assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &[], - &[], - &["IpAddress(12.34.56.78)"] - ), - Err(webpki::Error::NameConstraintViolation) + check_cert(ee.der(), issuer.der(), &["IpAddress(12.34.56.78)"]), + Err(Error::NameConstraintViolation) ); } @@ -386,16 +338,9 @@ fn ip4_address_san_allowed_if_outside_excluded_subtree() { vec![SanType::IpAddress("12.34.56.78".parse().unwrap())], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["12.34.56.78"], - &[], - &["IpAddress(12.34.56.78)"] - ), - Ok(()) - ); + check_cert(ee.der(), issuer.der(), &["IpAddress(12.34.56.78)"]) + .unwrap() + .assert_valid_name("12.34.56.78"); } #[test] @@ -412,14 +357,8 @@ fn ip4_address_san_rejected_if_excluded_is_sparse_cidr_mask() { &issuer, ); assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &[], - &[], - &["IpAddress(12.34.56.79)"] - ), - Err(webpki::Error::InvalidNetworkMaskConstraint) + check_cert(ee.der(), issuer.der(), &["IpAddress(12.34.56.79)"]), + Err(Error::InvalidNetworkMaskConstraint) ); } @@ -436,20 +375,12 @@ fn ip4_address_san_allowed() { vec![SanType::IpAddress("12.34.56.78".parse().unwrap())], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["12.34.56.78"], - &[ - "12.34.56.77", - "12.34.56.79", - "0000:0000:0000:0000:0000:ffff:0c22:384e" - ], - &["IpAddress(12.34.56.78)"] - ), - Ok(()) - ); + check_cert(ee.der(), issuer.der(), &["IpAddress(12.34.56.78)"]) + .unwrap() + .assert_valid_name("12.34.56.78") + .assert_invalid_name("12.34.56.77") + .assert_invalid_name("12.34.56.79") + .assert_invalid_name("0000:0000:0000:0000:0000:ffff:0c22:384e"); } #[test] @@ -468,14 +399,8 @@ fn ip6_address_san_rejected_if_in_excluded_subtree() { &issuer, ); assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &[], - &[], - &["IpAddress(2001:db8::1)"] - ), - Err(webpki::Error::NameConstraintViolation) + check_cert(ee.der(), issuer.der(), &["IpAddress(2001:db8::1)"]), + Err(Error::NameConstraintViolation) ); } @@ -494,16 +419,9 @@ fn ip6_address_san_allowed_if_outside_excluded_subtree() { vec![SanType::IpAddress("2001:db9::1".parse().unwrap())], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["2001:0db9:0000:0000:0000:0000:0000:0001"], - &[], - &["IpAddress(2001:db9::1)"] - ), - Ok(()) - ); + check_cert(ee.der(), issuer.der(), &["IpAddress(2001:db9::1)"]) + .unwrap() + .assert_valid_name("2001:0db9:0000:0000:0000:0000:0000:0001"); } #[test] @@ -521,16 +439,10 @@ fn ip6_address_san_allowed() { vec![SanType::IpAddress("2001:db9::1".parse().unwrap())], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["2001:0db9:0000:0000:0000:0000:0000:0001"], - &["12.34.56.78"], - &["IpAddress(2001:db9::1)"] - ), - Ok(()) - ); + check_cert(ee.der(), issuer.der(), &["IpAddress(2001:db9::1)"]) + .unwrap() + .assert_valid_name("2001:0db9:0000:0000:0000:0000:0000:0001") + .assert_invalid_name("12.34.56.78"); } #[test] @@ -554,20 +466,17 @@ fn ip46_mixed_address_san_allowed() { ], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["12.34.56.78", "2001:0db9:0000:0000:0000:0000:0000:0001"], - &[ - "12.34.56.77", - "12.34.56.79", - "0000:0000:0000:0000:0000:ffff:0c22:384e" - ], - &["IpAddress(12.34.56.78)", "IpAddress(2001:db9::1)"] - ), - Ok(()) - ); + check_cert( + ee.der(), + issuer.der(), + &["IpAddress(12.34.56.78)", "IpAddress(2001:db9::1)"], + ) + .unwrap() + .assert_valid_name("12.34.56.78") + .assert_valid_name("2001:0db9:0000:0000:0000:0000:0000:0001") + .assert_invalid_name("12.34.56.77") + .assert_invalid_name("12.34.56.79") + .assert_invalid_name("0000:0000:0000:0000:0000:ffff:0c22:384e"); } /// Since we don't have real constraint matching implemented for URI names, fail closed. @@ -586,8 +495,8 @@ fn uri_san_rejected_against_uri_permitted_subtree() { &issuer, ); assert_eq!( - check_cert(ee.der(), issuer.der(), &[], &[], &[]), - Err(webpki::Error::NameConstraintViolation), + check_cert(ee.der(), issuer.der(), &[]), + Err(Error::NameConstraintViolation), ); } @@ -607,8 +516,8 @@ fn uri_san_rejected_against_uri_excluded_subtree() { &issuer, ); assert_eq!( - check_cert(ee.der(), issuer.der(), &[], &[], &[]), - Err(webpki::Error::NameConstraintViolation), + check_cert(ee.der(), issuer.der(), &[]), + Err(Error::NameConstraintViolation), ); } @@ -648,8 +557,8 @@ fn permit_directory_name_not_implemented() { })); let ee = generate_cert(vec![], &issuer); assert_eq!( - check_cert(ee.der(), issuer.der(), &[], &[], &[]), - Err(webpki::Error::NameConstraintViolation) + check_cert(ee.der(), issuer.der(), &[]), + Err(Error::NameConstraintViolation) ); } @@ -663,8 +572,8 @@ fn exclude_directory_name_not_implemented() { })); let ee = generate_cert(vec![], &issuer); assert_eq!( - check_cert(ee.der(), issuer.der(), &[], &[], &[]), - Err(webpki::Error::NameConstraintViolation) + check_cert(ee.der(), issuer.der(), &[]), + Err(Error::NameConstraintViolation) ); } @@ -678,19 +587,16 @@ fn invalid_dns_name_matching() { ], &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &["dns.example.com"], - &[], - &[ - "DnsName(\"{invalid}.example.com\")", - "DnsName(\"dns.example.com\")" - ] - ), - Ok(()) - ); + check_cert( + ee.der(), + issuer.der(), + &[ + "DnsName(\"{invalid}.example.com\")", + "DnsName(\"dns.example.com\")", + ], + ) + .unwrap() + .assert_valid_name("dns.example.com"); } #[test] @@ -708,16 +614,13 @@ fn presented_names_escape_control_characters() { &issuer, ); - assert_eq!( - check_cert( - ee.der(), - issuer.der(), - &[], - &["real.example.com"], - &[r#"DnsName("a\r\nInjected: header\u{1b}[31m")"#], - ), - Ok(()) - ); + check_cert( + ee.der(), + issuer.der(), + &[r#"DnsName("a\r\nInjected: header\u{1b}[31m")"#], + ) + .unwrap() + .assert_invalid_name("real.example.com"); } fn generate_cert(sans: Vec, issuer: &CertifiedIssuer<'_, KeyPair>) -> Certificate { @@ -764,13 +667,7 @@ fn make_issuer(name_constraints: Option) -> CertifiedIssuer<'st } #[track_caller] -fn check_cert( - ee: &[u8], - ca: &[u8], - valid_names: &[&str], - invalid_names: &[&str], - presented_names: &[&str], -) -> Result<(), webpki::Error> { +fn check_cert(ee: &[u8], ca: &[u8], presented_names: &[&str]) -> Result { let ca_cert_der = CertificateDer::from(ca); let anchors = [anchor_from_trusted_cert(&ca_cert_der).unwrap()]; let builder = PathBuilder::new( @@ -783,26 +680,45 @@ fn check_cert( let ee_der = CertificateDer::from(ee); let time = UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d)); - let cert = webpki::EndEntityCert::try_from(&ee_der).unwrap(); + let cert = EndEntityCert::try_from(&ee_der).unwrap(); builder.build(&cert, time)?; - for valid in valid_names { - let name = ServerName::try_from(*valid).unwrap(); - assert_eq!(cert.verify_is_valid_for_subject_name(&name), Ok(())); + Ok(NameChecker { + cert_der: ee_der.into_owned(), + expected_presented_names: presented_names.iter().map(|n| n.to_string()).collect(), + }) +} + +#[derive(Debug, PartialEq)] +struct NameChecker { + cert_der: CertificateDer<'static>, + expected_presented_names: Vec, +} + +impl NameChecker { + #[track_caller] + fn assert_valid_name(&self, name: &str) -> &Self { + let name = ServerName::try_from(name).unwrap(); + assert_eq!(self.cert().verify_is_valid_for_subject_name(&name), Ok(())); + self } - for invalid in invalid_names { - let name = ServerName::try_from(*invalid).unwrap(); + #[track_caller] + fn assert_invalid_name(&self, name: &str) -> &Self { + let name = ServerName::try_from(name).unwrap(); assert_eq!( - cert.verify_is_valid_for_subject_name(&name), - Err(webpki::Error::CertNotValidForName(InvalidNameContext { + self.cert().verify_is_valid_for_subject_name(&name), + Err(Error::CertNotValidForName(InvalidNameContext { expected: name.to_owned(), - presented: presented_names.iter().map(|n| n.to_string()).collect(), + presented: self.expected_presented_names.clone(), })) ); + self } - Ok(()) + fn cert(&self) -> EndEntityCert<'_> { + EndEntityCert::try_from(&self.cert_der).unwrap() + } } // OID for emailAddress in subject DN (pkcs9-emailAddress) From be6f013ed8bd6dfb9c2b7215072b449e6bbf5e56 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 27 Aug 2026 16:38:48 +0100 Subject: [PATCH 6/8] Yield a more specific error than `CertNotValidForName` If we saw a `MalformedDnsIdentifier` error when considering names, this is a more proximate cause of what was previously reported as `CertNotValidForName`. --- src/subject_name/dns_name.rs | 44 ++++++++++++++---------- tests/tls_server_certs.rs | 66 +++++++++++++++++++++++++++++------- 2 files changed, 79 insertions(+), 31 deletions(-) diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index eeb1f83e..eeab1eb7 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -27,33 +27,37 @@ use crate::subject_name::Subtrees; pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Result<(), Error> { let dns_name = untrusted::Input::from(reference.as_ref().as_bytes()); - let result = NameIterator::new(cert.subject_alt_name).find_map(|result| { - let name = match result { - Ok(name) => name, - Err(err) => return Some(Err(err)), + let mut specific_error = None; + let mut iter = NameIterator::new(cert.subject_alt_name); + let result = loop { + let name = match iter.next() { + Some(Ok(name)) => name, + Some(Err(err)) => return Err(err), + None => break None, }; let GeneralName::DnsName(presented_id) = name else { - return None; + continue; }; 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)), + Ok(true) => break Some(Ok(())), + Ok(false) => {} + Err(err @ Error::MalformedDnsIdentifier(_)) => specific_error = Some(err), + Err(e) => return Err(e), } - }); + }; - match result { - #[cfg_attr(not(feature = "alloc"), expect(clippy::needless_return))] - Some(result) => return result, - #[cfg(feature = "alloc")] - None => {} - #[cfg(not(feature = "alloc"))] - None => Err(Error::CertNotValidForName(InvalidNameContext {})), - } + if let Some(result) = result { + return result; + }; + + // If we have a more specific error, we yield that. + if let Some(specific) = specific_error { + return Err(specific); + }; - // Try to yield a more useful error. To avoid allocating on the happy path, + // Otherwise, construct a useful error. To avoid allocating on the happy path, // we reconstruct the same `NameIterator` and replay it. #[cfg(feature = "alloc")] { @@ -64,6 +68,10 @@ pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Resu .collect(), })) } + #[cfg(not(feature = "alloc"))] + { + Err(Error::CertNotValidForName(InvalidNameContext {})) + } } /// A reference to a DNS Name presented by a server that may include a wildcard. diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index beed6379..728bd6da 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -22,7 +22,7 @@ use rcgen::{ date_time_ymd, }; use webpki::{ - EndEntityCert, Error, ExtendedKeyUsage, InvalidNameContext, PathBuilder, + DnsNameError, EndEntityCert, Error, ExtendedKeyUsage, InvalidNameContext, PathBuilder, anchor_from_trusted_cert, }; @@ -614,13 +614,50 @@ fn presented_names_escape_control_characters() { &issuer, ); - check_cert( - ee.der(), - issuer.der(), - &[r#"DnsName("a\r\nInjected: header\u{1b}[31m")"#], - ) - .unwrap() - .assert_invalid_name("real.example.com"); + assert_eq!( + check_cert( + ee.der(), + issuer.der(), + &[r#"DnsName("a\r\nInjected: header\u{1b}[31m")"#], + ) + .unwrap() + .check_name("real.example.com"), + Err(Error::MalformedDnsIdentifier( + DnsNameError::IllegalCharacter(b'\r') + )) + ); +} + +/// See +#[test] +fn good_error_with_invalid_wildcard() { + let issuer = make_issuer(None); + let ee = generate_cert_with_names( + None, + None, + vec![ + SanType::DnsName("localhost".try_into().unwrap()), + SanType::DnsName("*.localhost".try_into().unwrap()), + ], + &issuer, + ); + + let ee_der = CertificateDer::from(ee); + let cert = EndEntityCert::try_from(&ee_der).unwrap(); + + // normal behaviour for matching name + assert_eq!( + cert.verify_is_valid_for_subject_name(&"localhost".try_into().unwrap()), + Ok(()) + ); + + // unmatching name gets a more specific error + assert_eq!( + cert.verify_is_valid_for_subject_name(&"test.localhost".try_into().unwrap()), + Err(Error::MalformedDnsIdentifier( + DnsNameError::WildcardMustPrecedeTwoLabels + )) + ); } fn generate_cert(sans: Vec, issuer: &CertifiedIssuer<'_, KeyPair>) -> Certificate { @@ -698,24 +735,27 @@ struct NameChecker { impl NameChecker { #[track_caller] fn assert_valid_name(&self, name: &str) -> &Self { - let name = ServerName::try_from(name).unwrap(); - assert_eq!(self.cert().verify_is_valid_for_subject_name(&name), Ok(())); + self.check_name(name).unwrap(); self } #[track_caller] fn assert_invalid_name(&self, name: &str) -> &Self { - let name = ServerName::try_from(name).unwrap(); assert_eq!( - self.cert().verify_is_valid_for_subject_name(&name), + self.check_name(name), Err(Error::CertNotValidForName(InvalidNameContext { - expected: name.to_owned(), + expected: ServerName::try_from(name).unwrap().to_owned(), presented: self.expected_presented_names.clone(), })) ); self } + fn check_name(&self, name: &str) -> Result<(), Error> { + self.cert() + .verify_is_valid_for_subject_name(&ServerName::try_from(name).unwrap()) + } + fn cert(&self) -> EndEntityCert<'_> { EndEntityCert::try_from(&self.cert_der).unwrap() } From f36ede66e9d3119be8d9e9b4a7fbfd585080caa3 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 27 Aug 2026 21:51:34 +0100 Subject: [PATCH 7/8] Test what happens if `valid_dns_names()` sees invalid name --- tests/tls_server_certs.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index 728bd6da..84c6ff12 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -621,6 +621,7 @@ fn presented_names_escape_control_characters() { &[r#"DnsName("a\r\nInjected: header\u{1b}[31m")"#], ) .unwrap() + .assert_dns_names(&[]) .check_name("real.example.com"), Err(Error::MalformedDnsIdentifier( DnsNameError::IllegalCharacter(b'\r') @@ -751,6 +752,12 @@ impl NameChecker { self } + #[track_caller] + fn assert_dns_names(&self, names: &[&str]) -> &Self { + assert_eq!(self.cert().valid_dns_names().collect::>(), names); + self + } + fn check_name(&self, name: &str) -> Result<(), Error> { self.cert() .verify_is_valid_for_subject_name(&ServerName::try_from(name).unwrap()) From c2ce8a1eb6a179f00834f3eeff6e8a956df45931 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 27 Aug 2026 22:03:12 +0100 Subject: [PATCH 8/8] Improve testing of DNS name parsing --- src/subject_name/dns_name.rs | 60 ++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index eeab1eb7..5a9e6506 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -1008,6 +1008,66 @@ mod tests { } } + #[test] + fn dns_name_cases() { + WildcardDnsNameRef::try_from_ascii(b"abc").unwrap(); + + assert_eq!( + WildcardDnsNameRef::try_from_ascii(b"-abc"), + Err(LabelMustNotStartWithHyphen) + ); + assert_eq!( + WildcardDnsNameRef::try_from_ascii(b"abc-"), + Err(LabelMustNotEndWithHyphen) + ); + assert_eq!( + WildcardDnsNameRef::try_from_ascii(b"abc-."), + Err(LabelMustNotEndWithHyphen) + ); + + assert_eq!( + WildcardDnsNameRef::try_from_ascii(b"123"), + Err(LastLabelMustNotBeAllNumeric) + ); + + assert_eq!( + WildcardDnsNameRef::try_from_ascii(&[b'a'; 254]), + Err(NameTooLong) + ); + assert_eq!( + WildcardDnsNameRef::try_from_ascii(&[b'a'; 253]), + Err(LabelTooLong) + ); + assert_eq!( + WildcardDnsNameRef::try_from_ascii( + b"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0" + ), + Err(LabelTooLong) + ); + assert_eq!( + WildcardDnsNameRef::try_from_ascii( + b"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-" + ), + Err(LabelTooLong) + ); + assert_eq!( + WildcardDnsNameRef::try_from_ascii( + b"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaZ" + ), + Err(LabelTooLong) + ); + + assert_eq!( + WildcardDnsNameRef::try_from_ascii(b"*.hello"), + Err(WildcardMustPrecedeTwoLabels) + ); + assert_eq!( + WildcardDnsNameRef::try_from_ascii(b"*.hello."), + Err(WildcardMustPrecedeTwoLabels) + ); + WildcardDnsNameRef::try_from_ascii(b"*.hello.world").unwrap(); + } + // (presented_name, constraint, expected_matches) #[expect(clippy::type_complexity)] const PRESENTED_MATCHES_CONSTRAINT: &[(&[u8], &[u8], Result)] = &[