From 4e2ef1b96422b3f5edaf8969f9417ca3926a3ef4 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 13 Sep 2023 18:31:40 -0400 Subject: [PATCH 01/22] subject_name: remove comment about required feature We switched to using `doc_auto_cfg` to automatically indicate in Rustdocs when an item requires a particular feature. This comment about the `dns_name::DnsName` re-export requiring alloc isn't necessary anymore. --- src/subject_name/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/subject_name/mod.rs b/src/subject_name/mod.rs index fbc915f9..e9234529 100644 --- a/src/subject_name/mod.rs +++ b/src/subject_name/mod.rs @@ -17,7 +17,6 @@ mod dns_name; pub(crate) use dns_name::GeneralDnsNameRef; pub use dns_name::{DnsNameRef, InvalidDnsNameError}; -/// Requires the `alloc` feature. #[cfg(feature = "alloc")] pub use dns_name::DnsName; From 839cd93c46b8a235d7868e24dd9d289db0f07bac Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 13 Sep 2023 18:50:22 -0400 Subject: [PATCH 02/22] lib: remove unneeded manual docsrs cfg_attr We switched to `doc_auto_cfg` and don't need to manually annotate `cfg(feature ...)` annotations for docsrs purposes anymore. --- src/lib.rs | 1 - 1 file changed, 1 deletion(-) 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}, From 0682cec3c6bfbd4d27bebe844683f16dc33beeeb Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 14 Sep 2023 09:38:56 -0400 Subject: [PATCH 03/22] tests: remove unnecessary pub visibility --- tests/integration.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 8234d1af..9a59c08b 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")[..]); @@ -257,7 +257,7 @@ fn read_ee_with_large_pos_serial() { #[cfg(feature = "alloc")] #[test] -pub fn list_netflix_names() { +fn list_netflix_names() { let ee = include_bytes!("netflix/ee.der"); expect_cert_dns_names( @@ -281,7 +281,7 @@ pub fn list_netflix_names() { #[cfg(feature = "alloc")] #[test] -pub fn invalid_subject_alt_names() { +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"); @@ -307,7 +307,7 @@ pub fn invalid_subject_alt_names() { #[cfg(feature = "alloc")] #[test] -pub fn wildcard_subject_alternative_names() { +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"); @@ -358,7 +358,7 @@ fn expect_cert_dns_names(data: &[u8], expected_names: &[&str]) { #[cfg(feature = "alloc")] #[test] -pub fn no_subject_alt_names() { +fn no_subject_alt_names() { let ee = CertificateDer::from(&include_bytes!("misc/no_subject_alternative_name.der")[..]); let cert = webpki::EndEntityCert::try_from(&ee) From 12cd0eadbe68b26289dd6cebbd937d92d101ae7e Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 14 Sep 2023 09:41:40 -0400 Subject: [PATCH 04/22] tests: rename data -> cert_der in expect_cert_dns_names --- tests/integration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 9a59c08b..ca255a5c 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -332,10 +332,10 @@ fn wildcard_subject_alternative_names() { } #[cfg(feature = "alloc")] -fn expect_cert_dns_names(data: &[u8], expected_names: &[&str]) { +fn expect_cert_dns_names(cert_der: &[u8], expected_names: &[&str]) { use std::collections::HashSet; - let der = CertificateDer::from(data); + let der = CertificateDer::from(cert_der); let cert = webpki::EndEntityCert::try_from(&der) .expect("should parse end entity certificate correctly"); From f5ace6d60b1af512bd9e96c136dcfc9cab89dc67 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 10:16:48 -0400 Subject: [PATCH 05/22] end_entity: rework dns_names to return iter of &str This is what most consumers of the API are interested in, and avoids needing to export the `GeneralDnsNameRef` and `WildcardDnsNameRef` types. --- src/end_entity.rs | 4 +--- src/subject_name/mod.rs | 2 -- src/subject_name/verify.rs | 4 ++-- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/end_entity.rs b/src/end_entity.rs index 0f4dd777..875df8cf 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}; @@ -156,7 +154,7 @@ impl<'a> EndEntityCert<'a> { /// 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> { + pub fn dns_names(&'a self) -> Result, Error> { subject_name::list_cert_dns_names(self) } } diff --git a/src/subject_name/mod.rs b/src/subject_name/mod.rs index e9234529..04ad102a 100644 --- a/src/subject_name/mod.rs +++ b/src/subject_name/mod.rs @@ -13,8 +13,6 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. mod dns_name; -#[cfg(feature = "alloc")] -pub(crate) use dns_name::GeneralDnsNameRef; pub use dns_name::{DnsNameRef, InvalidDnsNameError}; #[cfg(feature = "alloc")] diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 660ebadc..38cd79df 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -320,7 +320,7 @@ impl<'a> Iterator for NameIterator<'a> { #[cfg(feature = "alloc")] pub(crate) fn list_cert_dns_names<'names>( cert: &'names crate::EndEntityCert<'names>, -) -> Result>, Error> { +) -> Result, Error> { let cert = &cert.inner(); let mut names = Vec::new(); @@ -346,7 +346,7 @@ pub(crate) fn list_cert_dns_names<'names>( // if the name could be converted to a DNS name, add it; otherwise, // keep going. if let Ok(name) = dns_name { - names.push(name) + names.push(name.into()) } None From de8b2ec0f773f0014f8a9ce3641595800126a33c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 10:21:01 -0400 Subject: [PATCH 06/22] tests: simplify `expect_cert_dns_names` --- tests/integration.rs | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index ca255a5c..d54ac098 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -262,7 +262,7 @@ fn list_netflix_names() { expect_cert_dns_names( ee, - &[ + [ "account.netflix.com", "ca.netflix.com", "netflix.ca", @@ -288,7 +288,7 @@ fn invalid_subject_alt_names() { expect_cert_dns_names( data, - &[ + [ "account.netflix.com", "ca.netflix.com", "netflix.ca", @@ -314,7 +314,7 @@ fn wildcard_subject_alternative_names() { expect_cert_dns_names( data, - &[ + [ "account.netflix.com", "*.netflix.com", "netflix.ca", @@ -332,28 +332,15 @@ fn wildcard_subject_alternative_names() { } #[cfg(feature = "alloc")] -fn expect_cert_dns_names(cert_der: &[u8], expected_names: &[&str]) { - use std::collections::HashSet; - +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 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); + assert!(cert.dns_names().unwrap().eq(expected_names)) } #[cfg(feature = "alloc")] From 8d609504d180adb3befb013e4d3e290532126047 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 10:21:31 -0400 Subject: [PATCH 07/22] tests: move `no_subject_alt_names` test above helper --- tests/integration.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index d54ac098..bc6dd42d 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -331,18 +331,6 @@ fn wildcard_subject_alternative_names() { ); } -#[cfg(feature = "alloc")] -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"); - - assert!(cert.dns_names().unwrap().eq(expected_names)) -} - #[cfg(feature = "alloc")] #[test] fn no_subject_alt_names() { @@ -357,3 +345,15 @@ fn no_subject_alt_names() { assert!(names.collect::>().is_empty()); } + +#[cfg(feature = "alloc")] +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"); + + assert!(cert.dns_names().unwrap().eq(expected_names)) +} From 2350c2e61017e952a65f505380f5d52d17597451 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 10:21:58 -0400 Subject: [PATCH 08/22] tests: rework `no_subject_alt_names` test We can express this test with the `expect_cert_dns_names` helper. --- tests/integration.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index bc6dd42d..88d4e9cc 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -334,16 +334,7 @@ fn wildcard_subject_alternative_names() { #[cfg(feature = "alloc")] #[test] fn no_subject_alt_names() { - let ee = CertificateDer::from(&include_bytes!("misc/no_subject_alternative_name.der")[..]); - - let cert = webpki::EndEntityCert::try_from(&ee) - .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()); + expect_cert_dns_names(include_bytes!("misc/no_subject_alternative_name.der"), []) } #[cfg(feature = "alloc")] From 22cebce14aeb853f7070e858e88ac1b02be2d8c6 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 14 Sep 2023 09:51:15 -0400 Subject: [PATCH 09/22] tests: clean up unneeded bindings --- tests/integration.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 88d4e9cc..455672f6 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -258,10 +258,8 @@ fn read_ee_with_large_pos_serial() { #[cfg(feature = "alloc")] #[test] fn list_netflix_names() { - let ee = include_bytes!("netflix/ee.der"); - expect_cert_dns_names( - ee, + include_bytes!("netflix/ee.der"), [ "account.netflix.com", "ca.netflix.com", @@ -282,12 +280,10 @@ fn list_netflix_names() { #[cfg(feature = "alloc")] #[test] 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"); - 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", @@ -308,12 +304,10 @@ fn invalid_subject_alt_names() { #[cfg(feature = "alloc")] #[test] 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"); - 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", From 7ffbabef22d44d0c6caa562e849fac35c0b85573 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 14 Sep 2023 10:10:42 -0400 Subject: [PATCH 10/22] end_entity: fix `dns_names` rustdoc comment Prior to this commit the rustdoc comment on `EndEntityCert.dns_names` mentioned using `verify_is_valid_for_dns_name` and `verify_is_valid_for_at_least_one_dns_name`, but these functions don't exist anymore. This commit updates the comment to point to `EndEntityCert::verify_is_valid_for_subject_name`, and does so with a proper Rustdoc link so that future updates will be caught by `cargo doc` if we forget to fix this reference to match. --- src/end_entity.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/end_entity.rs b/src/end_entity.rs index 875df8cf..bc46c014 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -151,8 +151,8 @@ impl<'a> EndEntityCert<'a> { /// Returns a list of the 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`. + /// Checking that a certificate is valid for a given subject name should always be done with + /// [EndEntityCert::verify_is_valid_for_subject_name]. #[cfg(feature = "alloc")] pub fn dns_names(&'a self) -> Result, Error> { subject_name::list_cert_dns_names(self) From 8bf4fa9e9ef81d994ff53dbe99b2dd1c723d0677 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 11:08:55 -0400 Subject: [PATCH 11/22] end_entity: make `dns_names` infallible The purpose of the `dns_names` helper on an `EndEntityCert` is to provide users the opportunity to get information on the dNSName SAN values in a certificate for **non-validation** purposes. Checking that a certificate is valid for a particular name should always be done with `verify_is_valid_for_at_least_one_dns_name`. With that use-case in mind, we can make the `dns_names` helper easier for consumers to use by filtering out invalid general names, returning an `Iterator` unconditionally, instead of a `Result`. This better matches the updated name validation semantics where we ignore `MalformedDnsIdentifier` errors to continue to try to find a valid name to validate against. --- src/end_entity.rs | 8 +++--- src/subject_name/verify.rs | 52 +++++++++++++++++--------------------- tests/integration.rs | 2 +- 3 files changed, 27 insertions(+), 35 deletions(-) diff --git a/src/end_entity.rs b/src/end_entity.rs index bc46c014..214ef86d 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -148,13 +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. /// Checking that a certificate is valid for a given subject name should always be done with /// [EndEntityCert::verify_is_valid_for_subject_name]. #[cfg(feature = "alloc")] - pub fn dns_names(&'a self) -> Result, Error> { + pub fn dns_names(&'a self) -> impl Iterator { subject_name::list_cert_dns_names(self) } } @@ -212,9 +212,7 @@ 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"); + let mut names = cert.dns_names(); assert_eq!(names.next().map(<&str>::from), Some(name)); assert_eq!(names.next().map(<&str>::from), None); } diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 38cd79df..568c86e1 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -320,42 +320,36 @@ impl<'a> Iterator for NameIterator<'a> { #[cfg(feature = "alloc")] pub(crate) fn list_cert_dns_names<'names>( cert: &'names crate::EndEntityCert<'names>, -) -> Result, Error> { +) -> impl Iterator { 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), - }; + NameIterator::new(Some(cert.subject), cert.subject_alt_name).for_each(|result| { + let name = match result { + Ok(name) => name, + Err(_) => return, + }; - let presented_id = match name { - GeneralName::DnsName(presented) => presented, - _ => return None, - }; + let presented_id = match name { + GeneralName::DnsName(presented) => presented, + _ => return, + }; - 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.into()) - } + 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) + }); - None - }); + // if the name could be converted to a DNS name, add it; otherwise, + // keep going. + if let Ok(name) = dns_name { + names.push(name.into()) + } + }); - match result { - Some(err) => Err(err), - _ => Ok(names.into_iter()), - } + names.into_iter() } // It is *not* valid to derive `Eq`, `PartialEq, etc. for this type. In diff --git a/tests/integration.rs b/tests/integration.rs index 455672f6..78bd5e62 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -340,5 +340,5 @@ fn expect_cert_dns_names<'name>( let cert = webpki::EndEntityCert::try_from(&der) .expect("should parse end entity certificate correctly"); - assert!(cert.dns_names().unwrap().eq(expected_names)) + assert!(cert.dns_names().eq(expected_names)) } From 807df740e41f3697038ecfad4cce94bb991c57e7 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 11:20:50 -0400 Subject: [PATCH 12/22] lib: rm alloc req. for `dns_names` With the update to the `dns_names` function in the previous commit we can now make `EndEntity.dns_names` work without requiring `alloc`. --- src/end_entity.rs | 1 - src/subject_name/mod.rs | 6 +++--- src/subject_name/verify.rs | 28 ++++++++-------------------- tests/integration.rs | 5 ----- 4 files changed, 11 insertions(+), 29 deletions(-) diff --git a/src/end_entity.rs b/src/end_entity.rs index 214ef86d..d52d5ac2 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -153,7 +153,6 @@ impl<'a> EndEntityCert<'a> { /// This function must not be used to implement custom DNS name verification. /// Checking that a certificate is valid for a given subject name should always be done with /// [EndEntityCert::verify_is_valid_for_subject_name]. - #[cfg(feature = "alloc")] pub fn dns_names(&'a self) -> impl Iterator { subject_name::list_cert_dns_names(self) } diff --git a/src/subject_name/mod.rs b/src/subject_name/mod.rs index 04ad102a..fc90dc48 100644 --- a/src/subject_name/mod.rs +++ b/src/subject_name/mod.rs @@ -28,6 +28,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, list_cert_dns_names, verify_cert_subject_name, GeneralName, +}; diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 568c86e1..e9cd6351 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::dns_name::{self, DnsNameRef, GeneralDnsNameRef, WildcardDnsNameRef}; use super::ip_address::{self, IpAddrRef}; use super::name::SubjectNameRef; use crate::der::{self, FromDer}; @@ -317,22 +312,16 @@ impl<'a> Iterator for NameIterator<'a> { } } -#[cfg(feature = "alloc")] pub(crate) fn list_cert_dns_names<'names>( cert: &'names crate::EndEntityCert<'names>, ) -> impl Iterator { let cert = &cert.inner(); - let mut names = Vec::new(); - - NameIterator::new(Some(cert.subject), cert.subject_alt_name).for_each(|result| { - let name = match result { - Ok(name) => name, - Err(_) => return, - }; + NameIterator::new(Some(cert.subject), cert.subject_alt_name).filter_map(|result| { + let name = result.ok()?; let presented_id = match name { GeneralName::DnsName(presented) => presented, - _ => return, + _ => return None, }; let dns_name = DnsNameRef::try_from_ascii(presented_id.as_slice_less_safe()) @@ -344,12 +333,11 @@ pub(crate) fn list_cert_dns_names<'names>( // if the name could be converted to a DNS name, add it; otherwise, // keep going. - if let Ok(name) = dns_name { - names.push(name.into()) + match dns_name { + Ok(name) => Some(name.into()), + _ => None, } - }); - - names.into_iter() + }) } // It is *not* valid to derive `Eq`, `PartialEq, etc. for this type. In diff --git a/tests/integration.rs b/tests/integration.rs index 78bd5e62..3b3094ea 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -255,7 +255,6 @@ fn read_ee_with_large_pos_serial() { webpki::EndEntityCert::try_from(&ee).expect("should parse 20-octet positive serial number"); } -#[cfg(feature = "alloc")] #[test] fn list_netflix_names() { expect_cert_dns_names( @@ -277,7 +276,6 @@ fn list_netflix_names() { ); } -#[cfg(feature = "alloc")] #[test] fn invalid_subject_alt_names() { expect_cert_dns_names( @@ -301,7 +299,6 @@ fn invalid_subject_alt_names() { ); } -#[cfg(feature = "alloc")] #[test] fn wildcard_subject_alternative_names() { expect_cert_dns_names( @@ -325,13 +322,11 @@ fn wildcard_subject_alternative_names() { ); } -#[cfg(feature = "alloc")] #[test] fn no_subject_alt_names() { expect_cert_dns_names(include_bytes!("misc/no_subject_alternative_name.der"), []) } -#[cfg(feature = "alloc")] fn expect_cert_dns_names<'name>( cert_der: &[u8], expected_names: impl IntoIterator, From be4dba60f95f6ece29371c5d0205178f31d9f112 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 11:25:56 -0400 Subject: [PATCH 13/22] subject_name: tidy up `list_cert_dns_names` Avoid combinator chaining, use explicit `match`. --- src/subject_name/verify.rs | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index e9cd6351..f926ee84 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -12,7 +12,7 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -use super::dns_name::{self, DnsNameRef, GeneralDnsNameRef, WildcardDnsNameRef}; +use super::dns_name::{self, DnsNameRef, WildcardDnsNameRef}; use super::ip_address::{self, IpAddrRef}; use super::name::SubjectNameRef; use crate::der::{self, FromDer}; @@ -317,25 +317,19 @@ pub(crate) fn list_cert_dns_names<'names>( ) -> impl Iterator { let cert = &cert.inner(); NameIterator::new(Some(cert.subject), cert.subject_alt_name).filter_map(|result| { - let name = result.ok()?; - - let presented_id = match name { + let presented_id = match result.ok()? { 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, + // if the name could be converted to a DNS name, return it; otherwise, // keep going. - match dns_name { - Ok(name) => Some(name.into()), - _ => None, + match DnsNameRef::try_from_ascii(presented_id.as_slice_less_safe()) { + Ok(dns_name) => Some(dns_name.into()), + Err(_) => match WildcardDnsNameRef::try_from_ascii(presented_id.as_slice_less_safe()) { + Ok(wildcard_dns_name) => Some(wildcard_dns_name.into()), + Err(_) => None, + }, } }) } From 4332b8c6b2f89bafda512c5d00e46fcd59c09e84 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 12:13:23 -0400 Subject: [PATCH 14/22] signed_data: fix code block missing close marker --- src/signed_data.rs | 1 + 1 file changed, 1 insertion(+) 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 From 415c3872ec86aea5549ec350fbfe545c433c86c3 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 20 Sep 2023 10:32:15 +0200 Subject: [PATCH 15/22] Move InvalidDnsNameError definition down below usage --- src/subject_name/dns_name.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index bacdeed1..239e21d8 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -72,20 +72,6 @@ impl AsRef for DnsNameRef<'_> { } } -/// 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. @@ -228,6 +214,20 @@ impl AsRef for WildcardDnsNameRef<'_> { } } +/// 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 {} + pub(super) fn presented_id_matches_reference_id( presented_dns_id: untrusted::Input, reference_dns_id: untrusted::Input, From 7aa0f8d61fafa3c6097cfa6459e166977be654fe Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 20 Sep 2023 10:32:42 +0200 Subject: [PATCH 16/22] Move GeneralDnsNameRef definition to the top --- src/subject_name/dns_name.rs | 38 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index 239e21d8..2d562f12 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -18,6 +18,25 @@ use core::fmt::Write; use crate::Error; +/// 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 DNS Name suitable for use in the TLS Server Name Indication (SNI) /// extension and/or for use as the reference hostname for which to verify a /// certificate. @@ -126,25 +145,6 @@ impl<'a> From> for &'a str { } } -/// 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 From d0522c000b33e898aa26ee4094dee27a31cb49a1 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 20 Sep 2023 10:34:02 +0200 Subject: [PATCH 17/22] Refactor faux-bool enum definition for AllowWildcards --- src/subject_name/dns_name.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index 2d562f12..f39ccffe 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -98,7 +98,7 @@ impl<'a> DnsNameRef<'a> { if !is_valid_dns_id( untrusted::Input::from(dns_name), IdRole::Reference, - AllowWildcards::No, + Wildcards::Deny, ) { return Err(InvalidDnsNameError); } @@ -167,7 +167,7 @@ impl<'a> WildcardDnsNameRef<'a> { if !is_valid_dns_id( untrusted::Input::from(dns_name), IdRole::Reference, - AllowWildcards::Yes, + Wildcards::Allow, ) { return Err(InvalidDnsNameError); } @@ -371,11 +371,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 +506,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 +531,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 +552,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'.') { From b11327ba8a55e6f1afa2890f8e2fdb6e0fdc30af Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 20 Sep 2023 10:42:22 +0200 Subject: [PATCH 18/22] Use as_str() method to reference DnsNameRef contents The From impl feels a little unidiomatic because the DnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`. --- src/subject_name/dns_name.rs | 32 ++++++++++---------------------- src/subject_name/verify.rs | 2 +- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index f39ccffe..bd3757a3 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -31,7 +31,7 @@ pub enum GeneralDnsNameRef<'name> { impl<'a> From> for &'a str { fn from(d: GeneralDnsNameRef<'a>) -> Self { match d { - GeneralDnsNameRef::DnsName(name) => name.into(), + GeneralDnsNameRef::DnsName(name) => name.as_str(), GeneralDnsNameRef::Wildcard(name) => name.into(), } } @@ -82,15 +82,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() - } -} - impl<'a> DnsNameRef<'a> { /// Constructs a `DnsNameRef` from the given input if the input is a /// syntactically-valid DNS name. @@ -115,10 +106,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() } } @@ -137,14 +133,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 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 diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index f926ee84..cd5a2474 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -24,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 { From abf970c9d7f3afc22067829a12ee0abd3ac1caf5 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 20 Sep 2023 10:46:02 +0200 Subject: [PATCH 19/22] Use as_str() method to reference WildcardDnsNameRef contents The From impl feels a little unidiomatic because the WildcardDnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`. --- src/subject_name/dns_name.rs | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index bd3757a3..a9a9ec0e 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -32,7 +32,7 @@ impl<'a> From> for &'a str { fn from(d: GeneralDnsNameRef<'a>) -> Self { match d { GeneralDnsNameRef::DnsName(name) => name.as_str(), - GeneralDnsNameRef::Wildcard(name) => name.into(), + GeneralDnsNameRef::Wildcard(name) => name.as_str(), } } } @@ -168,6 +168,13 @@ impl<'a> WildcardDnsNameRef<'a> { 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 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() + } } impl core::fmt::Debug for WildcardDnsNameRef<'_> { @@ -185,23 +192,6 @@ 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() - } -} - -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() - } -} - /// 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)] From f470c07bebb0e6da215b1b6033faef464dd52f5d Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 20 Sep 2023 10:50:21 +0200 Subject: [PATCH 20/22] Use as_str() method to reference GeneralDnsNameRef contents The From impl feels a little unidiomatic because the GeneralDnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`. --- src/end_entity.rs | 4 ++-- src/subject_name/dns_name.rs | 11 ++++++----- src/subject_name/verify.rs | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/end_entity.rs b/src/end_entity.rs index d52d5ac2..76ff96f0 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -212,7 +212,7 @@ mod tests { EndEntityCert::try_from(der).expect("should parse end entity certificate correctly"); let mut names = cert.dns_names(); - assert_eq!(names.next().map(<&str>::from), Some(name)); - assert_eq!(names.next().map(<&str>::from), None); + assert_eq!(names.next(), Some(name)); + assert_eq!(names.next(), None); } } diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index a9a9ec0e..20a862fe 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -28,11 +28,12 @@ pub enum GeneralDnsNameRef<'name> { Wildcard(WildcardDnsNameRef<'name>), } -impl<'a> From> for &'a str { - fn from(d: GeneralDnsNameRef<'a>) -> Self { - match d { - GeneralDnsNameRef::DnsName(name) => name.as_str(), - GeneralDnsNameRef::Wildcard(name) => name.as_str(), +impl<'a> GeneralDnsNameRef<'a> { + /// Yields the DNS name as a `&str`. + pub fn as_str(&self) -> &'a str { + match self { + Self::DnsName(name) => name.as_str(), + Self::Wildcard(name) => name.as_str(), } } } diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index cd5a2474..ad2ec5d0 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -325,9 +325,9 @@ pub(crate) fn list_cert_dns_names<'names>( // 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.into()), + 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.into()), + Ok(wildcard_dns_name) => Some(wildcard_dns_name.as_str()), Err(_) => None, }, } From 1e92abff36b7df2daac713a2e228db8db71340cc Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 20 Sep 2023 11:17:31 +0200 Subject: [PATCH 21/22] Remove API that is now unused --- src/subject_name/dns_name.rs | 32 +++----------------------------- 1 file changed, 3 insertions(+), 29 deletions(-) diff --git a/src/subject_name/dns_name.rs b/src/subject_name/dns_name.rs index 20a862fe..eb4c3554 100644 --- a/src/subject_name/dns_name.rs +++ b/src/subject_name/dns_name.rs @@ -18,26 +18,6 @@ use core::fmt::Write; use crate::Error; -/// 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> GeneralDnsNameRef<'a> { - /// Yields the DNS name as a `&str`. - pub fn as_str(&self) -> &'a str { - match self { - Self::DnsName(name) => name.as_str(), - Self::Wildcard(name) => name.as_str(), - } - } -} - /// A DNS Name suitable for use in the TLS Server Name Indication (SNI) /// extension and/or for use as the reference hostname for which to verify a /// certificate. @@ -147,12 +127,12 @@ impl core::fmt::Debug for DnsNameRef<'_> { /// [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, @@ -164,14 +144,8 @@ 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 fn as_str(&self) -> &'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() From 64fb1b7f3b8c271d65052e34cbcee2ffc2212ef4 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 20 Sep 2023 11:15:21 -0400 Subject: [PATCH 22/22] cert: move `list_cert_dns_names` to `Cert` This commit lifts the free-standing `list_cert_dns_names` helper from the `subject_name` module to be associated with a `Cert`. Doing so also requires making the `subject_name::NameIterator` and `subject_name::WildcardDnsNameRef` `pub(crate)` visible. --- src/cert.rs | 23 +++++++++++++++++++++++ src/end_entity.rs | 2 +- src/subject_name/mod.rs | 3 ++- src/subject_name/verify.rs | 28 +++------------------------- 4 files changed, 29 insertions(+), 27 deletions(-) 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 76ff96f0..b36bb736 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -154,7 +154,7 @@ impl<'a> EndEntityCert<'a> { /// 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 { - subject_name::list_cert_dns_names(self) + self.inner.valid_dns_names() } } diff --git a/src/subject_name/mod.rs b/src/subject_name/mod.rs index fc90dc48..c3abf4ea 100644 --- a/src/subject_name/mod.rs +++ b/src/subject_name/mod.rs @@ -13,6 +13,7 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. mod dns_name; +pub(super) use dns_name::WildcardDnsNameRef; pub use dns_name::{DnsNameRef, InvalidDnsNameError}; #[cfg(feature = "alloc")] @@ -29,5 +30,5 @@ pub use ip_address::IpAddr; mod verify; pub(super) use verify::{ - check_name_constraints, list_cert_dns_names, verify_cert_subject_name, GeneralName, + check_name_constraints, verify_cert_subject_name, GeneralName, NameIterator, }; diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index ad2ec5d0..518fd062 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -12,7 +12,7 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -use super::dns_name::{self, DnsNameRef, WildcardDnsNameRef}; +use super::dns_name::{self, DnsNameRef}; use super::ip_address::{self, IpAddrRef}; use super::name::SubjectNameRef; use crate::der::{self, FromDer}; @@ -258,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 { @@ -312,28 +312,6 @@ impl<'a> Iterator for NameIterator<'a> { } } -pub(crate) fn list_cert_dns_names<'names>( - cert: &'names crate::EndEntityCert<'names>, -) -> impl Iterator { - let cert = &cert.inner(); - NameIterator::new(Some(cert.subject), cert.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, - }, - } - }) -} - // 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