From 28aa2ea4a958a43905f7f8985fd16b3f99e30d31 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 15 Sep 2023 10:16:36 -0400 Subject: [PATCH 1/9] test_utils: alloc feature gate at module level Presently all of the helpers in this module require the `alloc` feature to be present. Rather than sprinkling the feature requirement on all members let's just put at the module level. If we add helpers that don't require `alloc` we can revisit. --- src/test_utils.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test_utils.rs b/src/test_utils.rs index b30c12d7..69fad1d2 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -1,13 +1,12 @@ -#[cfg(feature = "alloc")] +#![cfg(feature = "alloc")] + use crate::types::CertificateDer; /// Signature algorithm used by certificates generated using `make_issuer` and /// `make_end_entity`. This is exported as a constant so that tests can use the /// same algorithm when generating certificates using `rcgen`. -#[cfg(feature = "alloc")] pub(crate) static RCGEN_SIGNATURE_ALG: &rcgen::SignatureAlgorithm = &rcgen::PKCS_ECDSA_P256_SHA256; -#[cfg(feature = "alloc")] pub(crate) fn make_issuer( org_name: impl Into, name_constraints: Option, @@ -27,7 +26,6 @@ pub(crate) fn make_issuer( rcgen::Certificate::from_params(ca_params).unwrap() } -#[cfg(feature = "alloc")] pub(crate) fn make_end_entity(issuer: &rcgen::Certificate) -> CertificateDer<'static> { let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]); ee_params.is_ca = rcgen::IsCa::ExplicitNoCa; From c8374b90f45c5f723667beceb3403fef1ae3c728 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 15 Sep 2023 10:22:33 -0400 Subject: [PATCH 2/9] test_utils: break out issuer CertificateParams Rather than having `make_issuer` support all possible parameters as arguments we should have a helper that provides a base `CertificateParams` with defaults that are sensible for an issuer, and then customize it as required. This commit adds `issuer_params` for generating the base `rcgen::CertificateParams` for an issuer, and then updates `make_issuer` to use it. --- src/test_utils.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/test_utils.rs b/src/test_utils.rs index 69fad1d2..802089ef 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -11,6 +11,15 @@ pub(crate) fn make_issuer( org_name: impl Into, name_constraints: Option, ) -> rcgen::Certificate { + let mut params = issuer_params(org_name); + params.name_constraints = name_constraints; + rcgen::Certificate::from_params(params).unwrap() +} + +/// Populate a [CertificateParams] that describes an unconstrained issuer certificate capable +/// of signing other certificates and CRLs, with the given `org_name` as an organization distinguished +/// subject name. +pub(crate) fn issuer_params(org_name: impl Into) -> rcgen::CertificateParams { let mut ca_params = rcgen::CertificateParams::new(Vec::new()); ca_params .distinguished_name @@ -22,8 +31,7 @@ pub(crate) fn make_issuer( rcgen::KeyUsagePurpose::CrlSign, ]; ca_params.alg = RCGEN_SIGNATURE_ALG; - ca_params.name_constraints = name_constraints; - rcgen::Certificate::from_params(ca_params).unwrap() + ca_params } pub(crate) fn make_end_entity(issuer: &rcgen::Certificate) -> CertificateDer<'static> { From 83cfa2ba1a4b1c579804c086f7160c640401b87c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 15 Sep 2023 10:30:22 -0400 Subject: [PATCH 3/9] test_utils: rm make_issuer name constraint arg This was only used by one test, and now that there's a convenient way to get a `rcgen::CertificateParams` that describes sensible issuer defaults we can use that to customize the name constraints in only that one test, avoiding passing `None` from many others. --- src/end_entity.rs | 2 +- src/test_utils.rs | 9 ++------- src/verify_cert.rs | 25 ++++++++++++------------- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/src/end_entity.rs b/src/end_entity.rs index 0f4dd777..4aad6669 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -174,7 +174,7 @@ mod tests { fn printable_string_common_name() { const DNS_NAME: &str = "test.example.com"; - let issuer = test_utils::make_issuer("Test", None); + let issuer = test_utils::make_issuer("Test"); let ee_cert_der = { let mut params = rcgen::CertificateParams::new(vec![DNS_NAME.to_string()]); diff --git a/src/test_utils.rs b/src/test_utils.rs index 802089ef..e974d012 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -7,13 +7,8 @@ use crate::types::CertificateDer; /// same algorithm when generating certificates using `rcgen`. pub(crate) static RCGEN_SIGNATURE_ALG: &rcgen::SignatureAlgorithm = &rcgen::PKCS_ECDSA_P256_SHA256; -pub(crate) fn make_issuer( - org_name: impl Into, - name_constraints: Option, -) -> rcgen::Certificate { - let mut params = issuer_params(org_name); - params.name_constraints = name_constraints; - rcgen::Certificate::from_params(params).unwrap() +pub(crate) fn make_issuer(org_name: impl Into) -> rcgen::Certificate { + rcgen::Certificate::from_params(issuer_params(org_name)).unwrap() } /// Populate a [CertificateParams] that describes an unconstrained issuer certificate capable diff --git a/src/verify_cert.rs b/src/verify_cert.rs index c4240ef0..2f25c51c 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -499,7 +499,7 @@ enum Role { mod tests { use super::*; #[cfg(feature = "alloc")] - use crate::test_utils::{make_end_entity, make_issuer}; + use crate::test_utils::{issuer_params, make_end_entity, make_issuer}; #[test] fn eku_key_purpose_id() { @@ -519,13 +519,13 @@ mod tests { trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, budget: Option, ) -> ControlFlow { - let ca_cert = make_issuer("Bogus Subject", None); + let ca_cert = make_issuer("Bogus Subject"); let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap()); let mut intermediates = Vec::with_capacity(intermediate_count); let mut issuer = ca_cert; for _ in 0..intermediate_count { - let intermediate = make_issuer("Bogus Subject", None); + let intermediate = make_issuer("Bogus Subject"); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); issuer = intermediate; @@ -578,13 +578,13 @@ mod tests { #[cfg(feature = "alloc")] fn build_linear_chain(chain_length: usize) -> Result<(), ControlFlow> { - let ca_cert = make_issuer(format!("Bogus Subject {chain_length}"), None); + let ca_cert = make_issuer(format!("Bogus Subject {chain_length}")); let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap()); let mut intermediates = Vec::with_capacity(chain_length); let mut issuer = ca_cert; for i in 0..chain_length { - let intermediate = make_issuer(format!("Bogus Subject {i}"), None); + let intermediate = make_issuer(format!("Bogus Subject {i}")); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); issuer = intermediate; @@ -623,13 +623,12 @@ mod tests { fn name_constraint_budget() { // Issue a trust anchor that imposes name constraints. The constraint should match // the end entity certificate SAN. - let ca_cert = make_issuer( - "Constrained Root", - Some(rcgen::NameConstraints { - permitted_subtrees: vec![rcgen::GeneralSubtree::DnsName(".com".into())], - excluded_subtrees: vec![], - }), - ); + let mut ca_cert_params = issuer_params("Constrained Root"); + ca_cert_params.name_constraints = Some(rcgen::NameConstraints { + permitted_subtrees: vec![rcgen::GeneralSubtree::DnsName(".com".into())], + excluded_subtrees: vec![], + }); + let ca_cert = rcgen::Certificate::from_params(ca_cert_params).unwrap(); let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap()); // Create a series of intermediate issuers. We'll only use one in the actual built path, @@ -638,7 +637,7 @@ mod tests { const NUM_INTERMEDIATES: usize = 5; let mut intermediates = Vec::with_capacity(NUM_INTERMEDIATES); for i in 0..NUM_INTERMEDIATES { - intermediates.push(make_issuer(format!("Intermediate {i}"), None)); + intermediates.push(make_issuer(format!("Intermediate {i}"))); } // Each intermediate should be issued by the trust anchor. From 11c4d09d29f7d4d67a382d609c82687988cd2852 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 15 Sep 2023 10:39:59 -0400 Subject: [PATCH 4/9] test_utils: break out end entity `CertificateParams` This commit updates `make_end_entity` similar to `make_issuer`: there's a new `end_entity_params` helper that returns sensible default `rcgen::CertificateParams` for an end-entity certificate when further customization is helpful, and leaves `make_end_entity` for when any old `rcgen::Certificate` for an end-entity issued by a specific issuer is required. --- src/test_utils.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/test_utils.rs b/src/test_utils.rs index e974d012..c4308f77 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -30,13 +30,17 @@ pub(crate) fn issuer_params(org_name: impl Into) -> rcgen::CertificatePa } pub(crate) fn make_end_entity(issuer: &rcgen::Certificate) -> CertificateDer<'static> { - let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]); - ee_params.is_ca = rcgen::IsCa::ExplicitNoCa; - ee_params.alg = RCGEN_SIGNATURE_ALG; CertificateDer::from( - rcgen::Certificate::from_params(ee_params) + rcgen::Certificate::from_params(end_entity_params(vec!["example.com".into()])) .unwrap() .serialize_der_with_signer(issuer) .unwrap(), ) } + +pub(crate) fn end_entity_params(subject_alt_names: Vec) -> rcgen::CertificateParams { + let mut ee_params = rcgen::CertificateParams::new(subject_alt_names); + ee_params.is_ca = rcgen::IsCa::ExplicitNoCa; + ee_params.alg = RCGEN_SIGNATURE_ALG; + ee_params +} From 83100d9cec72353bfe149c6a2762db14549ced52 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 15 Sep 2023 10:41:24 -0400 Subject: [PATCH 5/9] end_entity: use `test_utils::end_entity_params` The `printable_string_common_name` unit test only needs to customize the end entity distinguished name, so let's use the new `test_utils::end_entity_params` fn and only provide that customization. --- src/end_entity.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/end_entity.rs b/src/end_entity.rs index 4aad6669..42613f49 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -177,15 +177,13 @@ mod tests { let issuer = test_utils::make_issuer("Test"); let ee_cert_der = { - let mut params = rcgen::CertificateParams::new(vec![DNS_NAME.to_string()]); + let mut params = test_utils::end_entity_params(vec![DNS_NAME.to_string()]); // construct a certificate that uses `PrintableString` as the // common name value, rather than `UTF8String`. params.distinguished_name.push( rcgen::DnType::CommonName, rcgen::DnValue::PrintableString("example.com".to_string()), ); - params.is_ca = rcgen::IsCa::ExplicitNoCa; - params.alg = test_utils::RCGEN_SIGNATURE_ALG; let cert = rcgen::Certificate::from_params(params) .expect("failed to make ee cert (this is a test bug)"); let bytes = cert From 7b1c0676e3f921a94a8c5aecc27761abd6f49035 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 15 Sep 2023 10:44:11 -0400 Subject: [PATCH 6/9] test_utils: unexport RCGEN_SIGNATURE_ALG There aren't any consumers outside of the `test_utils` module and now that we're exposing `rcgen::CertificateParams` it's unlikely there will be new usages for creating params from scratch. For other use-cases it's possible to dig the value out of `rcgen::Certificate` and `rcgen::CertificateParams` instances. --- src/test_utils.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test_utils.rs b/src/test_utils.rs index c4308f77..70b931b9 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -2,10 +2,8 @@ use crate::types::CertificateDer; -/// Signature algorithm used by certificates generated using `make_issuer` and -/// `make_end_entity`. This is exported as a constant so that tests can use the -/// same algorithm when generating certificates using `rcgen`. -pub(crate) static RCGEN_SIGNATURE_ALG: &rcgen::SignatureAlgorithm = &rcgen::PKCS_ECDSA_P256_SHA256; +/// Signature algorithm used by certificates and parameters generated using the test utils helpers. +static RCGEN_SIGNATURE_ALG: &rcgen::SignatureAlgorithm = &rcgen::PKCS_ECDSA_P256_SHA256; pub(crate) fn make_issuer(org_name: impl Into) -> rcgen::Certificate { rcgen::Certificate::from_params(issuer_params(org_name)).unwrap() From 4213bba1745974430afea4c4f9efa3e1b94a9794 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 15 Sep 2023 11:47:16 -0400 Subject: [PATCH 7/9] verify_cert: fix build_degenerate_chain This commit updates the `build_degenerate_chain` helper to properly reproduce the path building complexity budget issue that was reported upstream in briansmith/webpki. The previous implementation only reproduced the issue when the signature validation budget was artificially inflated. The new formulation is able to reproduce the issue with the default signature validation budget, and default build chain call budget (and completes in reasonable time). This better demonstrates the both limits are needed as its possible to make pathological certificate chains that avoid the one limit and are caught by the other. --- src/verify_cert.rs | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 2f25c51c..743cc8e8 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -516,13 +516,17 @@ mod tests { #[cfg(feature = "alloc")] fn build_degenerate_chain( intermediate_count: usize, - trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, + trust_anchor: TrustAnchorIsActualIssuer, budget: Option, ) -> ControlFlow { let ca_cert = make_issuer("Bogus Subject"); let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap()); - let mut intermediates = Vec::with_capacity(intermediate_count); + let mut intermediates = Vec::with_capacity(intermediate_count + 1); + if let TrustAnchorIsActualIssuer::No = trust_anchor { + intermediates.push(ca_cert_der.to_vec()); + } + let mut issuer = ca_cert; for _ in 0..intermediate_count { let intermediate = make_issuer("Bogus Subject"); @@ -531,12 +535,16 @@ mod tests { issuer = intermediate; } - if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer { - intermediates.pop(); - } + let trust_anchor = match trust_anchor { + TrustAnchorIsActualIssuer::No => { + let unused_anchor = make_issuer("Bogus Trust Anchor"); + CertificateDer::from(unused_anchor.serialize_der().unwrap()) + } + TrustAnchorIsActualIssuer::Yes => ca_cert_der, + }; verify_chain( - &ca_cert_der, + &trust_anchor, &intermediates, &make_end_entity(&issuer), budget, @@ -555,23 +563,13 @@ mod tests { #[test] #[cfg(feature = "alloc")] - fn test_too_many_path_calls_mini() { + fn test_too_many_path_calls() { assert!(matches!( - build_degenerate_chain( + dbg!(build_degenerate_chain( 10, TrustAnchorIsActualIssuer::No, - Some(Budget { - // Crafting a chain that will expend the build chain calls budget without - // first expending the signature checks budget is tricky, so we artificially - // inflate the signature limit to make this test easier to write. - signatures: usize::MAX, - // We don't use the default build chain budget here. Doing so makes this test - // run slowly (due to testing quadratic runtime up to the limit) without adding - // much value from a testing perspective over using a smaller non-default limit. - build_chain_calls: 100, - ..Budget::default() - }) - ), + None + )), ControlFlow::Break(Error::MaximumPathBuildCallsExceeded) )); } From 0de17059371602e3e6de541dbcd98a7eba6085ca Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 18 Sep 2023 09:30:41 -0400 Subject: [PATCH 8/9] verify_cert: rename TrustAnchorIsActualIssuer --- src/verify_cert.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 743cc8e8..14619460 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -508,22 +508,22 @@ mod tests { } #[cfg(feature = "alloc")] - enum TrustAnchorIsActualIssuer { - Yes, - No, + enum ChainTrustAnchor { + NotInChain, + InChain, } #[cfg(feature = "alloc")] fn build_degenerate_chain( intermediate_count: usize, - trust_anchor: TrustAnchorIsActualIssuer, + trust_anchor: ChainTrustAnchor, budget: Option, ) -> ControlFlow { let ca_cert = make_issuer("Bogus Subject"); let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap()); let mut intermediates = Vec::with_capacity(intermediate_count + 1); - if let TrustAnchorIsActualIssuer::No = trust_anchor { + if let ChainTrustAnchor::InChain = trust_anchor { intermediates.push(ca_cert_der.to_vec()); } @@ -536,11 +536,11 @@ mod tests { } let trust_anchor = match trust_anchor { - TrustAnchorIsActualIssuer::No => { + ChainTrustAnchor::InChain => { let unused_anchor = make_issuer("Bogus Trust Anchor"); CertificateDer::from(unused_anchor.serialize_der().unwrap()) } - TrustAnchorIsActualIssuer::Yes => ca_cert_der, + ChainTrustAnchor::NotInChain => ca_cert_der, }; verify_chain( @@ -556,7 +556,7 @@ mod tests { #[cfg(feature = "alloc")] fn test_too_many_signatures() { assert!(matches!( - build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes, None), + build_degenerate_chain(5, ChainTrustAnchor::NotInChain, None), ControlFlow::Break(Error::MaximumSignatureChecksExceeded) )); } @@ -565,11 +565,7 @@ mod tests { #[cfg(feature = "alloc")] fn test_too_many_path_calls() { assert!(matches!( - dbg!(build_degenerate_chain( - 10, - TrustAnchorIsActualIssuer::No, - None - )), + dbg!(build_degenerate_chain(10, ChainTrustAnchor::InChain, None)), ControlFlow::Break(Error::MaximumPathBuildCallsExceeded) )); } From 5575fa2a343f1b1a88db7a40270b0e4df13a0621 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 15 Sep 2023 13:49:37 -0400 Subject: [PATCH 9/9] verify_cert: remove unused build_degenerate_chain arg The budget argument for `build_degenerate_chain` is no longer necessary. We always use the default budget with this helper. --- src/verify_cert.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 14619460..da563f32 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -517,7 +517,6 @@ mod tests { fn build_degenerate_chain( intermediate_count: usize, trust_anchor: ChainTrustAnchor, - budget: Option, ) -> ControlFlow { let ca_cert = make_issuer("Bogus Subject"); let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap()); @@ -547,7 +546,7 @@ mod tests { &trust_anchor, &intermediates, &make_end_entity(&issuer), - budget, + None, ) .unwrap_err() } @@ -556,7 +555,7 @@ mod tests { #[cfg(feature = "alloc")] fn test_too_many_signatures() { assert!(matches!( - build_degenerate_chain(5, ChainTrustAnchor::NotInChain, None), + build_degenerate_chain(5, ChainTrustAnchor::NotInChain), ControlFlow::Break(Error::MaximumSignatureChecksExceeded) )); } @@ -565,7 +564,7 @@ mod tests { #[cfg(feature = "alloc")] fn test_too_many_path_calls() { assert!(matches!( - dbg!(build_degenerate_chain(10, ChainTrustAnchor::InChain, None)), + dbg!(build_degenerate_chain(10, ChainTrustAnchor::InChain)), ControlFlow::Break(Error::MaximumPathBuildCallsExceeded) )); }