diff --git a/src/cert.rs b/src/cert.rs index 67de10f0..173f3deb 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -15,7 +15,9 @@ use crate::der::{self, DerIterator, FromDer, Tag, CONSTRUCTED, CONTEXT_SPECIFIC}; use crate::error::{DerTypeId, Error}; use crate::signed_data::SignedData; +use crate::subject_name::{GeneralName, NameIterator, WildcardDnsNameRef}; use crate::x509::{remember_extension, set_extension_once, DistributionPointName, Extension}; +use crate::DnsNameRef; /// A parsed X509 certificate. pub struct Cert<'a> { @@ -136,6 +138,27 @@ impl<'a> Cert<'a> { self.subject.as_slice_less_safe() } + pub(crate) fn valid_dns_names(&self) -> impl Iterator { + NameIterator::new(Some(self.subject), self.subject_alt_name).filter_map(|result| { + let presented_id = match result.ok()? { + GeneralName::DnsName(presented) => presented, + _ => return None, + }; + + // if the name could be converted to a DNS name, return it; otherwise, + // keep going. + match DnsNameRef::try_from_ascii(presented_id.as_slice_less_safe()) { + Ok(dns_name) => Some(dns_name.as_str()), + Err(_) => { + match WildcardDnsNameRef::try_from_ascii(presented_id.as_slice_less_safe()) { + Ok(wildcard_dns_name) => Some(wildcard_dns_name.as_str()), + Err(_) => None, + } + } + } + }) + } + /// Returns an iterator over the certificate's cRLDistributionPoints extension values, if any. pub(crate) fn crl_distribution_points( &self, diff --git a/src/end_entity.rs b/src/end_entity.rs index 0f4dd777..b36bb736 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -16,8 +16,6 @@ use pki_types::{CertificateDer, SignatureVerificationAlgorithm, TrustAnchor, Uni use crate::crl::RevocationOptions; use crate::error::Error; -#[cfg(feature = "alloc")] -use crate::subject_name::GeneralDnsNameRef; use crate::subject_name::{self, SubjectNameRef}; use crate::verify_cert::{self, KeyUsage}; use crate::{cert, signed_data}; @@ -150,14 +148,13 @@ impl<'a> EndEntityCert<'a> { ) } - /// Returns a list of the DNS names provided in the subject alternative names extension + /// Returns a list of valid DNS names provided in the subject alternative names extension /// /// This function must not be used to implement custom DNS name verification. - /// Verification functions are already provided as `verify_is_valid_for_dns_name` - /// and `verify_is_valid_for_at_least_one_dns_name`. - #[cfg(feature = "alloc")] - pub fn dns_names(&'a self) -> Result>, Error> { - subject_name::list_cert_dns_names(self) + /// Checking that a certificate is valid for a given subject name should always be done with + /// [EndEntityCert::verify_is_valid_for_subject_name]. + pub fn dns_names(&'a self) -> impl Iterator { + self.inner.valid_dns_names() } } @@ -214,10 +211,8 @@ mod tests { let cert = EndEntityCert::try_from(der).expect("should parse end entity certificate correctly"); - let mut names = cert - .dns_names() - .expect("should get all DNS names correctly for end entity cert"); - assert_eq!(names.next().map(<&str>::from), Some(name)); - assert_eq!(names.next().map(<&str>::from), None); + let mut names = cert.dns_names(); + assert_eq!(names.next(), Some(name)); + assert_eq!(names.next(), None); } } diff --git a/src/lib.rs b/src/lib.rs index 6f2b8c35..d6ef3a05 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -82,7 +82,6 @@ pub use { pub use pki_types as types; -#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))] #[cfg(feature = "alloc")] pub use { crl::{OwnedCertRevocationList, OwnedRevokedCert}, diff --git a/src/signed_data.rs b/src/signed_data.rs index ab2ec444..a1800898 100644 --- a/src/signed_data.rs +++ b/src/signed_data.rs @@ -92,6 +92,7 @@ impl<'a> SignedData<'a> { /// signatureAlgorithm AlgorithmIdentifier, /// signatureValue BIT STRING /// } + /// ``` /// /// OCSP responses (RFC 6960) look like this: /// ```ASN.1 diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index bacdeed1..eb4c3554 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -63,29 +63,6 @@ impl AsRef for DnsName { #[derive(Clone, Copy, Eq, PartialEq, Hash)] pub struct DnsNameRef<'a>(pub(crate) &'a [u8]); -impl AsRef for DnsNameRef<'_> { - #[inline] - fn as_ref(&self) -> &str { - // The unwrap won't fail because DnsNameRef are guaranteed to be ASCII - // and ASCII is a subset of UTF-8. - core::str::from_utf8(self.0).unwrap() - } -} - -/// An error indicating that a `DnsNameRef` could not built because the input -/// is not a syntactically-valid DNS Name. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub struct InvalidDnsNameError; - -impl core::fmt::Display for InvalidDnsNameError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - write!(f, "{:?}", self) - } -} - -#[cfg(feature = "std")] -impl ::std::error::Error for InvalidDnsNameError {} - impl<'a> DnsNameRef<'a> { /// Constructs a `DnsNameRef` from the given input if the input is a /// syntactically-valid DNS name. @@ -93,7 +70,7 @@ impl<'a> DnsNameRef<'a> { if !is_valid_dns_id( untrusted::Input::from(dns_name), IdRole::Reference, - AllowWildcards::No, + Wildcards::Deny, ) { return Err(InvalidDnsNameError); } @@ -110,10 +87,15 @@ impl<'a> DnsNameRef<'a> { /// Constructs a `DnsName` from this `DnsNameRef` #[cfg(feature = "alloc")] pub fn to_owned(&self) -> DnsName { - // DnsNameRef is already guaranteed to be valid ASCII, which is a - // subset of UTF-8. - let s: &str = (*self).into(); - DnsName(s.to_ascii_lowercase()) + // DnsNameRef is already guaranteed to be valid ASCII, which is subset of UTF-8. + DnsName(self.as_str().to_ascii_lowercase()) + } + + /// Yields a reference to the DNS name as a `&str`. + pub fn as_str(&self) -> &'a str { + // The unwrap won't fail because `DnsNameRef` values are guaranteed to be ASCII and ASCII + // is a subset of UTF-8. + core::str::from_utf8(self.0).unwrap() } } @@ -132,33 +114,6 @@ impl core::fmt::Debug for DnsNameRef<'_> { } } -impl<'a> From> for &'a str { - fn from(DnsNameRef(d): DnsNameRef<'a>) -> Self { - // The unwrap won't fail because DnsNameRefs are guaranteed to be ASCII - // and ASCII is a subset of UTF-8. - core::str::from_utf8(d).unwrap() - } -} - -/// A DNS name that may be either a DNS name identifier presented by a server (which may include -/// wildcards), or a DNS name identifier referenced by a client for matching purposes (wildcards -/// not permitted). -pub enum GeneralDnsNameRef<'name> { - /// a reference to a DNS name that may be used for matching purposes. - DnsName(DnsNameRef<'name>), - /// a reference to a presented DNS name that may include a wildcard. - Wildcard(WildcardDnsNameRef<'name>), -} - -impl<'a> From> for &'a str { - fn from(d: GeneralDnsNameRef<'a>) -> Self { - match d { - GeneralDnsNameRef::DnsName(name) => name.into(), - GeneralDnsNameRef::Wildcard(name) => name.into(), - } - } -} - /// A reference to a DNS Name presented by a server that may include a wildcard. /// /// A `WildcardDnsNameRef` is guaranteed to be syntactically valid. The validity rules @@ -172,16 +127,16 @@ impl<'a> From> for &'a str { /// [RFC 5280 Section 7.2]: https://tools.ietf.org/html/rfc5280#section-7.2 /// [RFC 6125 Section 4.1]: https://www.rfc-editor.org/rfc/rfc6125#section-4.1 #[derive(Clone, Copy, Eq, PartialEq, Hash)] -pub struct WildcardDnsNameRef<'a>(&'a [u8]); +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 fn try_from_ascii(dns_name: &'a [u8]) -> Result { + pub(crate) fn try_from_ascii(dns_name: &'a [u8]) -> Result { if !is_valid_dns_id( untrusted::Input::from(dns_name), IdRole::Reference, - AllowWildcards::Yes, + Wildcards::Allow, ) { return Err(InvalidDnsNameError); } @@ -189,10 +144,11 @@ impl<'a> WildcardDnsNameRef<'a> { Ok(Self(dns_name)) } - /// Constructs a `WildcardDnsNameRef` from the given input if the input is a - /// syntactically-valid DNS name. - pub fn try_from_ascii_str(dns_name: &'a str) -> Result { - Self::try_from_ascii(dns_name.as_bytes()) + /// Yields a reference to the DNS name as a `&str`. + pub(crate) fn as_str(&self) -> &'a str { + // The unwrap won't fail because a `WildcardDnsNameRef` is guaranteed to be ASCII and + // ASCII is a subset of UTF-8. + core::str::from_utf8(self.0).unwrap() } } @@ -211,23 +167,20 @@ impl core::fmt::Debug for WildcardDnsNameRef<'_> { } } -impl<'a> From> for &'a str { - fn from(WildcardDnsNameRef(d): WildcardDnsNameRef<'a>) -> Self { - // The unwrap won't fail because WildcardDnsNameRef are guaranteed to be ASCII - // and ASCII is a subset of UTF-8. - core::str::from_utf8(d).unwrap() - } -} +/// An error indicating that a `DnsNameRef` could not built because the input +/// is not a syntactically-valid DNS Name. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct InvalidDnsNameError; -impl AsRef for WildcardDnsNameRef<'_> { - #[inline] - fn as_ref(&self) -> &str { - // The unwrap won't fail because WildcardDnsNameRef are guaranteed to be ASCII - // and ASCII is a subset of UTF-8. - core::str::from_utf8(self.0).unwrap() +impl core::fmt::Display for InvalidDnsNameError { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + write!(f, "{:?}", self) } } +#[cfg(feature = "std")] +impl ::std::error::Error for InvalidDnsNameError {} + pub(super) fn presented_id_matches_reference_id( presented_dns_id: untrusted::Input, reference_dns_id: untrusted::Input, @@ -371,11 +324,11 @@ fn presented_id_matches_reference_id_internal( reference_dns_id_role: IdRole, reference_dns_id: untrusted::Input, ) -> Result { - if !is_valid_dns_id(presented_dns_id, IdRole::Presented, AllowWildcards::Yes) { + if !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, AllowWildcards::No) { + if !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, @@ -506,9 +459,9 @@ fn ascii_lower(b: u8) -> u8 { } #[derive(Clone, Copy, PartialEq)] -enum AllowWildcards { - No, - Yes, +enum Wildcards { + Deny, + Allow, } #[derive(Clone, Copy, PartialEq)] @@ -531,7 +484,7 @@ enum IdRole { fn is_valid_dns_id( hostname: untrusted::Input, id_role: IdRole, - allow_wildcards: AllowWildcards, + allow_wildcards: Wildcards, ) -> bool { // https://blogs.msdn.microsoft.com/oldnewthing/20120412-00/?p=7873/ if hostname.len() > 253 { @@ -552,7 +505,7 @@ fn is_valid_dns_id( // Only presented IDs are allowed to have wildcard labels. And, like // Chromium, be stricter than RFC 6125 requires by insisting that a // wildcard label consist only of '*'. - let is_wildcard = allow_wildcards == AllowWildcards::Yes && input.peek(b'*'); + let is_wildcard = allow_wildcards == Wildcards::Allow && input.peek(b'*'); let mut is_first_byte = !is_wildcard; if is_wildcard { if input.read_byte() != Ok(b'*') || input.read_byte() != Ok(b'.') { diff --git a/src/subject_name/mod.rs b/src/subject_name/mod.rs index fbc915f9..c3abf4ea 100644 --- a/src/subject_name/mod.rs +++ b/src/subject_name/mod.rs @@ -13,11 +13,9 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. mod dns_name; -#[cfg(feature = "alloc")] -pub(crate) use dns_name::GeneralDnsNameRef; +pub(super) use dns_name::WildcardDnsNameRef; pub use dns_name::{DnsNameRef, InvalidDnsNameError}; -/// Requires the `alloc` feature. #[cfg(feature = "alloc")] pub use dns_name::DnsName; @@ -31,6 +29,6 @@ pub use ip_address::{AddrParseError, IpAddrRef}; pub use ip_address::IpAddr; mod verify; -#[cfg(feature = "alloc")] -pub(super) use verify::list_cert_dns_names; -pub(super) use verify::{check_name_constraints, verify_cert_subject_name, GeneralName}; +pub(super) use verify::{ + check_name_constraints, verify_cert_subject_name, GeneralName, NameIterator, +}; diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 660ebadc..518fd062 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -12,12 +12,7 @@ // 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::vec::Vec; - use super::dns_name::{self, DnsNameRef}; -#[cfg(feature = "alloc")] -use super::dns_name::{GeneralDnsNameRef, WildcardDnsNameRef}; use super::ip_address::{self, IpAddrRef}; use super::name::SubjectNameRef; use crate::der::{self, FromDer}; @@ -29,7 +24,7 @@ pub(crate) fn verify_cert_dns_name( dns_name: DnsNameRef, ) -> Result<(), Error> { let cert = cert.inner(); - let dns_name = untrusted::Input::from(dns_name.as_ref().as_bytes()); + let dns_name = untrusted::Input::from(dns_name.as_str().as_bytes()); NameIterator::new(Some(cert.subject), cert.subject_alt_name) .find_map(|result| { let name = match result { @@ -263,13 +258,13 @@ enum Subtrees { ExcludedSubtrees, } -struct NameIterator<'a> { +pub(crate) struct NameIterator<'a> { subject_alt_name: Option>, subject_directory_name: Option>, } impl<'a> NameIterator<'a> { - fn new( + pub(crate) fn new( subject: Option>, subject_alt_name: Option>, ) -> Self { @@ -317,47 +312,6 @@ impl<'a> Iterator for NameIterator<'a> { } } -#[cfg(feature = "alloc")] -pub(crate) fn list_cert_dns_names<'names>( - cert: &'names crate::EndEntityCert<'names>, -) -> Result>, Error> { - let cert = &cert.inner(); - let mut names = Vec::new(); - - let result = - NameIterator::new(Some(cert.subject), cert.subject_alt_name).find_map(&mut |result| { - let name = match result { - Ok(name) => name, - Err(err) => return Some(err), - }; - - let presented_id = match name { - GeneralName::DnsName(presented) => presented, - _ => return None, - }; - - let dns_name = DnsNameRef::try_from_ascii(presented_id.as_slice_less_safe()) - .map(GeneralDnsNameRef::DnsName) - .or_else(|_| { - WildcardDnsNameRef::try_from_ascii(presented_id.as_slice_less_safe()) - .map(GeneralDnsNameRef::Wildcard) - }); - - // if the name could be converted to a DNS name, add it; otherwise, - // keep going. - if let Ok(name) = dns_name { - names.push(name) - } - - None - }); - - match result { - Some(err) => Err(err), - _ => Ok(names.into_iter()), - } -} - // It is *not* valid to derive `Eq`, `PartialEq, etc. for this type. In // particular, for the types of `GeneralName`s that we don't understand, we // don't even store the value. Also, the meaning of a `GeneralName` in a name diff --git a/tests/integration.rs b/tests/integration.rs index 8234d1af..3b3094ea 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -39,7 +39,7 @@ static ALL_SIGALGS: &[&dyn SignatureVerificationAlgorithm] = &[ * because they're rooted at a Verisign v1 root. */ #[cfg(feature = "alloc")] #[test] -pub fn netflix() { +fn netflix() { let ee: &[u8] = include_bytes!("netflix/ee.der"); let inter = CertificateDer::from(&include_bytes!("netflix/inter.der")[..]); let ca = CertificateDer::from(&include_bytes!("netflix/ca.der")[..]); @@ -66,7 +66,7 @@ pub fn netflix() { /* This is notable because it is a popular use of IP address subjectAltNames. */ #[cfg(feature = "alloc")] #[test] -pub fn cloudflare_dns() { +fn cloudflare_dns() { let ee: &[u8] = include_bytes!("cloudflare_dns/ee.der"); let inter = CertificateDer::from(&include_bytes!("cloudflare_dns/inter.der")[..]); let ca = CertificateDer::from(&include_bytes!("cloudflare_dns/ca.der")[..]); @@ -123,7 +123,7 @@ pub fn cloudflare_dns() { #[cfg(feature = "alloc")] #[test] -pub fn wpt() { +fn wpt() { let ee = CertificateDer::from(&include_bytes!("wpt/ee.der")[..]); let ca = CertificateDer::from(&include_bytes!("wpt/ca.der")[..]); @@ -145,7 +145,7 @@ pub fn wpt() { } #[test] -pub fn ed25519() { +fn ed25519() { let ee = CertificateDer::from(&include_bytes!("ed25519/ee.der")[..]); let ca = CertificateDer::from(&include_bytes!("ed25519/ca.der")[..]); @@ -255,14 +255,11 @@ fn read_ee_with_large_pos_serial() { webpki::EndEntityCert::try_from(&ee).expect("should parse 20-octet positive serial number"); } -#[cfg(feature = "alloc")] #[test] -pub fn list_netflix_names() { - let ee = include_bytes!("netflix/ee.der"); - +fn list_netflix_names() { expect_cert_dns_names( - ee, - &[ + include_bytes!("netflix/ee.der"), + [ "account.netflix.com", "ca.netflix.com", "netflix.ca", @@ -279,16 +276,13 @@ pub fn list_netflix_names() { ); } -#[cfg(feature = "alloc")] #[test] -pub fn invalid_subject_alt_names() { - // same as netflix ee certificate, but with the last name in the list - // changed to 'www.netflix:com' - let data = include_bytes!("misc/invalid_subject_alternative_name.der"); - +fn invalid_subject_alt_names() { expect_cert_dns_names( - data, - &[ + // same as netflix ee certificate, but with the last name in the list + // changed to 'www.netflix:com' + include_bytes!("misc/invalid_subject_alternative_name.der"), + [ "account.netflix.com", "ca.netflix.com", "netflix.ca", @@ -305,16 +299,13 @@ pub fn invalid_subject_alt_names() { ); } -#[cfg(feature = "alloc")] #[test] -pub fn wildcard_subject_alternative_names() { - // same as netflix ee certificate, but with the last name in the list - // changed to 'ww*.netflix:com' - let data = include_bytes!("misc/dns_names_and_wildcards.der"); - +fn wildcard_subject_alternative_names() { expect_cert_dns_names( - data, - &[ + // same as netflix ee certificate, but with the last name in the list + // changed to 'ww*.netflix:com' + include_bytes!("misc/dns_names_and_wildcards.der"), + [ "account.netflix.com", "*.netflix.com", "netflix.ca", @@ -331,42 +322,18 @@ pub fn wildcard_subject_alternative_names() { ); } -#[cfg(feature = "alloc")] -fn expect_cert_dns_names(data: &[u8], expected_names: &[&str]) { - use std::collections::HashSet; - - let der = CertificateDer::from(data); - let cert = webpki::EndEntityCert::try_from(&der) - .expect("should parse end entity certificate correctly"); - - let expected_names: HashSet<_> = expected_names.iter().cloned().collect(); - - let mut actual_names = cert - .dns_names() - .expect("should get all DNS names correctly for end entity cert") - .collect::>(); - - // Ensure that converting the list to a set doesn't throw away - // any duplicates that aren't supposed to be there - assert_eq!(actual_names.len(), expected_names.len()); - - let actual_names: std::collections::HashSet<&str> = - actual_names.drain(..).map(|name| name.into()).collect(); - - assert_eq!(actual_names, expected_names); -} - -#[cfg(feature = "alloc")] #[test] -pub fn no_subject_alt_names() { - let ee = CertificateDer::from(&include_bytes!("misc/no_subject_alternative_name.der")[..]); +fn no_subject_alt_names() { + expect_cert_dns_names(include_bytes!("misc/no_subject_alternative_name.der"), []) +} - let cert = webpki::EndEntityCert::try_from(&ee) +fn expect_cert_dns_names<'name>( + cert_der: &[u8], + expected_names: impl IntoIterator, +) { + let der = CertificateDer::from(cert_der); + let cert = webpki::EndEntityCert::try_from(&der) .expect("should parse end entity certificate correctly"); - let names = cert - .dns_names() - .expect("we should get a result even without subjectAltNames"); - - assert!(names.collect::>().is_empty()); + assert!(cert.dns_names().eq(expected_names)) }