From 21364963583e6255f6806534719d9af434c3106a Mon Sep 17 00:00:00 2001 From: stormshield-gt <143998166+stormshield-gt@users.noreply.github.com.> Date: Tue, 27 Aug 2024 11:37:24 +0200 Subject: [PATCH 1/5] Add extra roots on macos and ios Co-authored-by: ComplexSpaces --- .../src/verification/apple.rs | 70 +++++++++++++++---- 1 file changed, 58 insertions(+), 12 deletions(-) diff --git a/rustls-platform-verifier/src/verification/apple.rs b/rustls-platform-verifier/src/verification/apple.rs index 40de6eb8..5dc3a22e 100644 --- a/rustls-platform-verifier/src/verification/apple.rs +++ b/rustls-platform-verifier/src/verification/apple.rs @@ -43,6 +43,9 @@ fn system_time_to_cfdate(time: pki_types::UnixTime) -> Result /// A TLS certificate verifier that utilizes the Apple platform certificate facilities. #[derive(Debug)] pub struct Verifier { + /// Extra trust anchors to add to the verifier above and beyond those provided by + /// the system-provided trust stores. + extra_roots: Vec>, /// Testing only: The root CA certificate to trust. #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: Option>, @@ -50,7 +53,7 @@ pub struct Verifier { } impl Verifier { - /// Creates a new instance of a TLS certificate verifier that utilizes the macOS certificate + /// Creates a new instance of a TLS certificate verifier that utilizes the Apple certificate /// facilities. /// /// A [`CryptoProvider`] must be set with @@ -58,6 +61,20 @@ impl Verifier { /// [`CryptoProvider::install_default`] before the verifier can be used. pub fn new() -> Self { Self { + extra_roots: Vec::new(), + #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] + test_only_root_ca_override: None, + crypto_provider: OnceCell::new(), + } + } + + /// Creates a new instance of a TLS certificate verifier that utilizes the Apple certificate + /// facilities with the addition of extra root certificates to trust. + /// + /// See [Verifier::new] for the external requirements the verifier needs. + pub fn new_with_extra_roots(roots: Vec>) -> Self { + Self { + extra_roots: roots, #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, crypto_provider: OnceCell::new(), @@ -68,6 +85,7 @@ impl Verifier { #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] pub(crate) fn new_with_fake_root(root: &[u8]) -> Self { Self { + extra_roots: Vec::new(), test_only_root_ca_override: Some(root.into()), crypto_provider: OnceCell::new(), } @@ -123,21 +141,49 @@ impl Verifier { .map_err(|e| invalid_certificate(e.to_string()))?; } - // When testing, support using fake roots and ignoring values present on the system. + let raw_extra_roots = self.extra_roots.iter(); + + #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] + let extra_root = self + .test_only_root_ca_override + .as_ref() + .map(|root| pki_types::CertificateDer::from_slice(root)); + + #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] + let raw_extra_roots = raw_extra_roots.chain(&extra_root).to_owned(); + + let extra_roots = raw_extra_roots + .map(|root| { + SecCertificate::from_der(root) + .map_err(|_| TlsError::InvalidCertificate(CertificateError::BadEncoding)) + }) + .collect::, _>>()?; + + // If any extra roots were provided by the user (or tests), provide them to the trust + // evaluation regardless of their system trust settings or status. + if !extra_roots.is_empty() { + trust_evaluation + .set_anchor_certificates(&extra_roots) + .map_err(|e| TlsError::Other(OtherError(Arc::new(e))))?; + + // We want to trust both the system-installed and the extra roots. This must be set + // since calling `SecTrustSetAnchorCertificates` "disables the trusting of any + // anchors other than the ones specified by this function call" by default. + trust_evaluation + .set_trust_anchor_certificates_only(false) + .map_err(|e| TlsError::Other(OtherError(Arc::new(e))))?; + } + + // When testing, support using fake roots and ignoring default roots present on the system for + // consistency/reproducibility reasons. // // XXX: This does not currently limit revocation from fetching information online, or prevent // the downloading of root CAs. #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] { - // If these panicked, it would be a programmer bug in the tests. - if let Some(test_root) = &self.test_only_root_ca_override { - let test_root = - SecCertificate::from_der(test_root).expect("failed to parse test root"); - - // Supply the custom root, which will be the only one trusted during evaluation. - trust_evaluation - .set_anchor_certificates(&[test_root]) - .expect("failed to set anchors"); + if self.test_only_root_ca_override.is_some() { + // XXX: The test root was already provided to the trust evaluation as an extra root. + // We only need to stop use of the default system-installed roots. // As per [Apple's docs], building and verifying a certificate chain will // search through the system and keychain to find certificates that it @@ -174,7 +220,7 @@ impl Verifier { CertificateError::UnknownIssuer, )), errors::errSecInvalidExtendedKeyUsage => Ok(TlsError::InvalidCertificate( - CertificateError::Other(OtherError(std::sync::Arc::new(super::EkuError))), + CertificateError::Other(OtherError(Arc::new(super::EkuError))), )), errors::errSecCertificateRevoked => { Ok(TlsError::InvalidCertificate(CertificateError::Revoked)) From 61ca4cc4b28f99fb57a887f386e6f1e7fdb06d43 Mon Sep 17 00:00:00 2001 From: stormshield-gt <143998166+stormshield-gt@users.noreply.github.com.> Date: Tue, 27 Aug 2024 11:38:29 +0200 Subject: [PATCH 2/5] Link RFC 5280 AIA section to certificate instead of CRL --- rustls-platform-verifier/src/tests/verification_mock/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rustls-platform-verifier/src/tests/verification_mock/mod.rs b/rustls-platform-verifier/src/tests/verification_mock/mod.rs index 9509bcf2..8d487440 100644 --- a/rustls-platform-verifier/src/tests/verification_mock/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_mock/mod.rs @@ -221,7 +221,7 @@ mock_root_test_cases! { // with AIA because there's no AIA issuer field in the certificate). // (AIA is an extension that allows downloading of missing data, // like missing certificates, during validation; see - // https://datatracker.ietf.org/doc/html/rfc5280#section-5.2.7). + // https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.2.1). ee_only_dns [ any(windows, unix) ] => TestCase { reference_id: EXAMPLE_COM, chain: &[ROOT1_INT1_EXAMPLE_COM_GOOD], From 36cc3f54d814949dbc4ead7470b71776c08e9329 Mon Sep 17 00:00:00 2001 From: stormshield-gt <143998166+stormshield-gt@users.noreply.github.com.> Date: Sat, 24 Aug 2024 12:26:03 +0200 Subject: [PATCH 3/5] Uniformize new_with_extra_roots --- rustls-platform-verifier/Cargo.toml | 6 +++--- .../src/tests/verification_mock/mod.rs | 5 +++-- .../src/tests/verification_real_world/mod.rs | 5 +++-- .../src/verification/others.rs | 18 +++++++++++------- 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/rustls-platform-verifier/Cargo.toml b/rustls-platform-verifier/Cargo.toml index e91e5b7a..0e63ac28 100644 --- a/rustls-platform-verifier/Cargo.toml +++ b/rustls-platform-verifier/Cargo.toml @@ -46,12 +46,12 @@ webpki = { package = "rustls-webpki", version = "0.102", default-features = fals android_logger = { version = "0.13", optional = true } # Only used during testing. [target.'cfg(target_arch = "wasm32")'.dependencies] -webpki-roots = "0.26" webpki = { package = "rustls-webpki", version = "0.102", default-features = false } +webpki-root-certs = "0.26" -# BSD targets require webpki-roots for the real-world verification tests. +# BSD targets require webpki-roots-certs for the real-world verification tests. [target.'cfg(target_os = "freebsd")'.dev-dependencies] -webpki-roots = "0.26" +webpki-root-certs = "0.26" [target.'cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos"))'.dependencies] core-foundation = "0.9" diff --git a/rustls-platform-verifier/src/tests/verification_mock/mod.rs b/rustls-platform-verifier/src/tests/verification_mock/mod.rs index 8d487440..87d02944 100644 --- a/rustls-platform-verifier/src/tests/verification_mock/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_mock/mod.rs @@ -86,10 +86,11 @@ pub(super) fn verification_without_mock_root() { ensure_global_state(); // Since Rustls 0.22 constructing a webpki verifier (like the one backing Verifier on unix // systems) without any roots produces `OtherError(NoRootAnchors)` - since our FreeBSD CI - // runner fails to find any roots with openssl-probe we need to provide webpki-roots here + // runner fails to find any roots with openssl-probe we need to provide webpki-root-certs here // or the test will fail with the `OtherError` instead of the expected `CertificateError`. #[cfg(target_os = "freebsd")] - let verifier = Verifier::new_with_extra_roots(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); + let verifier = + Verifier::new_with_extra_roots(webpki_root_certs::TLS_SERVER_ROOT_CERTS.iter().cloned()); #[cfg(not(target_os = "freebsd"))] let verifier = Verifier::new(); diff --git a/rustls-platform-verifier/src/tests/verification_real_world/mod.rs b/rustls-platform-verifier/src/tests/verification_real_world/mod.rs index 7d941202..cc111b83 100644 --- a/rustls-platform-verifier/src/tests/verification_real_world/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_real_world/mod.rs @@ -126,9 +126,10 @@ fn real_world_test(test_case: &TestCase) { ); // On BSD systems openssl-probe fails to find the system CA bundle, - // so we must provide extra roots from webpki-roots. + // so we must provide extra roots from webpki-root-cert. #[cfg(target_os = "freebsd")] - let verifier = Verifier::new_with_extra_roots(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); + let verifier = + Verifier::new_with_extra_roots(webpki_root_certs::TLS_SERVER_ROOT_CERTS.iter().cloned()); #[cfg(not(target_os = "freebsd"))] let verifier = Verifier::new(); diff --git a/rustls-platform-verifier/src/verification/others.rs b/rustls-platform-verifier/src/verification/others.rs index 670be4e6..a1450312 100644 --- a/rustls-platform-verifier/src/verification/others.rs +++ b/rustls-platform-verifier/src/verification/others.rs @@ -53,12 +53,16 @@ impl Verifier { /// Creates a new verifier whose certificate validation is provided by /// WebPKI, using root certificates provided by the platform and augmented by /// the provided extra root certificates. - pub fn new_with_extra_roots( - roots: impl IntoIterator>, - ) -> Self { + pub fn new_with_extra_roots(roots: Vec>) -> Self { Self { inner: OnceCell::new(), - extra_roots: roots.into_iter().collect::>().into(), + extra_roots: roots + .into_iter() + .flat_map(|root| { + webpki::anchor_from_trusted_cert(&root).map(|anchor| anchor.to_owned()) + }) + .collect::>() + .into(), #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, crypto_provider: OnceCell::new(), @@ -154,9 +158,9 @@ impl Verifier { #[cfg(target_arch = "wasm32")] { - root_store - .roots - .extend_from_slice(webpki_roots::TLS_SERVER_ROOTS); + root_store.add_parsable_certificates( + webpki_root_certs::TLS_SERVER_ROOT_CERTS.iter().cloned(), + ); }; WebPkiServerVerifier::builder_with_provider( From beef74a38110e79841d201abbf59d1ea84e2df02 Mon Sep 17 00:00:00 2001 From: stormshield-gt <143998166+stormshield-gt@users.noreply.github.com.> Date: Mon, 26 Aug 2024 09:15:27 +0200 Subject: [PATCH 4/5] Add test for new_with_extra_roots --- Cargo.lock | 19 ++++--- rustls-platform-verifier/Cargo.toml | 4 +- .../src/tests/verification_mock/mod.rs | 49 +++++++++++++++++-- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8abfc98d..83d3e86e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -196,6 +196,12 @@ version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf" +[[package]] +name = "paste" +version = "1.0.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" + [[package]] name = "proc-macro2" version = "1.0.86" @@ -297,9 +303,9 @@ dependencies = [ [[package]] name = "rustls-pki-types" -version = "1.7.0" +version = "1.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "976295e77ce332211c0d24d92c0e83e50f5c5f046d11082cea19f3df13a3562d" +checksum = "fc0a2ce646f8655401bb81e7927b812614bd5d91dbc968696be50603510fcaf0" [[package]] name = "rustls-platform-verifier" @@ -312,13 +318,14 @@ dependencies = [ "jni", "log", "once_cell", + "paste", "rustls", "rustls-native-certs", "rustls-platform-verifier-android", "rustls-webpki", "security-framework", "security-framework-sys", - "webpki-roots", + "webpki-root-certs", "windows-sys", ] @@ -451,10 +458,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9c8d87e72b64a3b4db28d11ce29237c246188f4f51057d65a7eab63b7987e423" [[package]] -name = "webpki-roots" -version = "0.26.3" +name = "webpki-root-certs" +version = "0.26.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bd7c23921eeb1713a4e851530e9b9756e4fb0e89978582942612524cf09f01cd" +checksum = "d7a7077dce97d094d1a558b1dad6a3baf1c5c3ba5b65ecb18c493ae00b955f9e" dependencies = [ "rustls-pki-types", ] diff --git a/rustls-platform-verifier/Cargo.toml b/rustls-platform-verifier/Cargo.toml index 0e63ac28..45495bf1 100644 --- a/rustls-platform-verifier/Cargo.toml +++ b/rustls-platform-verifier/Cargo.toml @@ -19,7 +19,7 @@ crate-type = ["cdylib", "rlib"] # Enables a C interface to use for testing where `cargo` can't be used. # This feature is not stable, nor is the interface exported when it is enabled. # Do not rely on this or use it in production. -ffi-testing = ["android_logger", "rustls/ring"] +ffi-testing = ["android_logger", "rustls/ring", "paste"] # Enables APIs that expose lower-level verifier types for debugging purposes. dbg = [] # Enables `log::debug` base64-encoded logging of all end-entity certificates processed @@ -34,6 +34,7 @@ log = { version = "0.4" } base64 = { version = "0.22", optional = true } # Only used when the `cert-logging` feature is enabled. jni = { version = "0.19", default-features = false, optional = true } # Only used during doc generation once_cell = "1.9" +paste = { version = "1.0", default-features = false, optional = true } # Only used when `ffi-testing` feature is enabled [target.'cfg(all(unix, not(target_os = "android"), not(target_os = "macos"), not(target_os = "ios"), not(target_os = "tvos"), not(target_arch = "wasm32")))'.dependencies] rustls-native-certs = "0.7" @@ -64,6 +65,7 @@ windows-sys = { version = "0.52", default-features = false, features = ["Win32_F [dev-dependencies] rustls = { version = "0.23", default-features = false, features = ["ring"] } +paste = { version = "1.0", default-features = false } # Only used when `ffi-testing` feature is enabled [package.metadata.docs.rs] rustdoc-args = ["--cfg", "docsrs"] diff --git a/rustls-platform-verifier/src/tests/verification_mock/mod.rs b/rustls-platform-verifier/src/tests/verification_mock/mod.rs index 87d02944..25d5039c 100644 --- a/rustls-platform-verifier/src/tests/verification_mock/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_mock/mod.rs @@ -41,6 +41,14 @@ macro_rules! mock_root_test_cases { pub fn $name() { super::$name() } + + paste::paste!{ + #[cfg(all($target, not(windows), not(target_os = "android")))] + #[test] + pub fn [<$name _extra>](){ + super::[<$name _extra>]() + } + } )+ } @@ -49,8 +57,15 @@ macro_rules! mock_root_test_cases { pub static ALL_TEST_CASES: &'static [fn()] = &[ $( #[cfg($target)] - $name + $name, + + paste::paste!{ + #[cfg(all($target, not(windows), not(target_os = "android")))] + [<$name _extra>] + } + ),+ + ]; }; @@ -58,7 +73,14 @@ macro_rules! mock_root_test_cases { $( #[cfg($target)] pub(super) fn $name() { - test_with_mock_root(&$test_case); + test_with_mock_root(&$test_case, Roots::OnlyExtra); + } + + paste::paste!{ + #[cfg(all($target, not(windows), not(target_os = "android")))] + pub(super) fn [<$name _extra>]() { + test_with_mock_root(&$test_case, Roots::ExtraAndPlatform); + } } )+ }; @@ -301,11 +323,18 @@ mock_root_test_cases! { }, } -fn test_with_mock_root(test_case: &TestCase) { +fn test_with_mock_root( + test_case: &TestCase, + root_src: Roots, +) { ensure_global_state(); log::info!("verifying {:?}", test_case.expected_result); - let verifier = Verifier::new_with_fake_root(ROOT1); // TODO: time + let verifier = match root_src { + Roots::OnlyExtra => Verifier::new_with_fake_root(ROOT1), // TODO: time + #[cfg(all(unix, not(target_os = "android")))] + Roots::ExtraAndPlatform => Verifier::new_with_extra_roots(vec![ROOT1.into()]), + }; let mut chain = test_case .chain .iter() @@ -337,3 +366,15 @@ fn test_with_mock_root(test_case: &T ); // TODO: get into specifics of errors returned when it fails. } + +enum Roots { + /// Test with only extra roots, without loading the platform trust store. + /// + /// We want to keep things reproducible given the background-managed nature of trust roots on platforms. + OnlyExtra, + /// Test with loading the extra roots and the platform trust store. + /// + /// Right now, not all platforms are supported. + #[cfg(all(unix, not(target_os = "android")))] + ExtraAndPlatform, +} From 094341ed7c8de588fdeceabdadd397be9cc371c2 Mon Sep 17 00:00:00 2001 From: stormshield-gt <143998166+stormshield-gt@users.noreply.github.com.> Date: Thu, 29 Aug 2024 12:30:18 +0200 Subject: [PATCH 5/5] Cargo: version 0.3.4 -> 0.4.0 The `new_with_extra_roots` function for other platforms now take a `CertificateDer` instead of a `TrustAnchor` which is a breaking change --- Cargo.lock | 2 +- rustls-platform-verifier/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 83d3e86e..78eea86a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -309,7 +309,7 @@ checksum = "fc0a2ce646f8655401bb81e7927b812614bd5d91dbc968696be50603510fcaf0" [[package]] name = "rustls-platform-verifier" -version = "0.3.4" +version = "0.4.0" dependencies = [ "android_logger", "base64", diff --git a/rustls-platform-verifier/Cargo.toml b/rustls-platform-verifier/Cargo.toml index 45495bf1..002a52d3 100644 --- a/rustls-platform-verifier/Cargo.toml +++ b/rustls-platform-verifier/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustls-platform-verifier" -version = "0.3.4" +version = "0.4.0" authors = ["ComplexSpaces ", "1Password"] description = "rustls-platform-verifier supports verifying TLS certificates in rustls with the operating system verifier" keywords = ["tls", "certificate", "verification", "os", "native"]