diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 566f82cc..809b2286 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,7 +12,7 @@ permissions: contents: read env: - RUSTFLAGS: -D warnings -F unused_must_use + RUSTFLAGS: -D warnings jobs: clippy-build-std: diff --git a/Cargo.lock b/Cargo.lock index 9b033b55..92cd9aeb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -261,7 +261,7 @@ checksum = "917ce264624a4b4db1c364dcc35bfca9ded014d0a958cd47ad3e960e988ea51c" [[package]] name = "rustls-platform-verifier" -version = "0.5.3" +version = "0.6.0" dependencies = [ "android_logger", "base64", diff --git a/README.md b/README.md index 27e42a7d..9a0dd733 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,7 @@ let config = ClientConfig::builder_with_provider(arc_crypto_provider) .with_safe_default_protocol_versions() .unwrap() .with_platform_verifier() + .unwrap() .with_no_client_auth(); ``` diff --git a/rustls-platform-verifier/Cargo.toml b/rustls-platform-verifier/Cargo.toml index 72c11c4c..8d4df7e3 100644 --- a/rustls-platform-verifier/Cargo.toml +++ b/rustls-platform-verifier/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustls-platform-verifier" -version = "0.5.3" +version = "0.6.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"] @@ -33,13 +33,13 @@ rustls = { version = "0.23.27", default-features = false, features = ["std"] } log = { version = "0.4" } base64 = { version = "0.22", optional = true } # Only used when the `cert-logging` feature is enabled. jni = { version = "0.21", default-features = false, optional = true } # Only used during doc generation -once_cell = "1.9" [target.'cfg(all(unix, not(target_os = "android"), not(target_vendor = "apple"), not(target_arch = "wasm32")))'.dependencies] rustls-native-certs = "0.8" webpki = { package = "rustls-webpki", version = "0.103", default-features = false } [target.'cfg(target_os = "android")'.dependencies] +once_cell = "1.9" rustls-platform-verifier-android = { path = "../android-release-support", version = "0.1.0" } jni = { version = "0.21", default-features = false } webpki = { package = "rustls-webpki", version = "0.103", default-features = false } diff --git a/rustls-platform-verifier/src/lib.rs b/rustls-platform-verifier/src/lib.rs index f254d0d0..3c1da291 100644 --- a/rustls-platform-verifier/src/lib.rs +++ b/rustls-platform-verifier/src/lib.rs @@ -2,9 +2,14 @@ #![doc = include_str!("../README.md")] #![warn(missing_docs)] -use rustls::{client::WantsClientCert, ClientConfig, ConfigBuilder, WantsVerifier}; use std::sync::Arc; +#[cfg(feature = "dbg")] +use rustls::crypto::CryptoProvider; +#[cfg(feature = "dbg")] +use rustls::pki_types::CertificateDer; +use rustls::{client::WantsClientCert, ClientConfig, ConfigBuilder, WantsVerifier}; + mod verification; pub use verification::Verifier; @@ -26,66 +31,16 @@ mod tests; #[cfg_attr(feature = "ffi-testing", allow(unused_imports))] pub use tests::ffi::*; -/// Creates and returns a `rustls` configuration that verifies TLS -/// certificates in the best way for the underlying OS platform, using -/// safe defaults for the `rustls` configuration. -/// -/// # Example -/// -/// This example shows how to use the custom verifier with the `reqwest` crate: -/// ```ignore -/// # use reqwest::ClientBuilder; -/// #[tokio::main] -/// use rustls_platform_verifier::ConfigVerifierExt; -/// -/// async fn main() { -/// let client = ClientBuilder::new() -/// .use_preconfigured_tls(ClientConfig::with_platform_verifier()) -/// .build() -/// .expect("nothing should fail"); -/// -/// let _response = client.get("https://example.com").send().await; -/// } -/// ``` -/// -/// **Important:** You must ensure that your `reqwest` version is using the same Rustls -/// version as this crate or it will panic when downcasting the `&dyn Any` verifier. -/// -/// If you require more control over the rustls [`ClientConfig`], you can import the -/// [`BuilderVerifierExt`] trait and call `.with_platform_verifier()` on the [`ConfigBuilder`]. -/// -/// Refer to the crate level documentation to see what platforms -/// are currently supported. -#[deprecated(since = "0.4.0", note = "use the `ConfigVerifierExt` instead")] -pub fn tls_config() -> ClientConfig { - ClientConfig::with_platform_verifier() -} - -/// Attempts to construct a `rustls` configuration that verifies TLS certificates in the best way -/// for the underlying OS platform, using the provided -/// [`CryptoProvider`][rustls::crypto::CryptoProvider]. -/// -/// See [`tls_config`] for further documentation. -/// -/// # Errors -/// -/// Propagates any error returned by [`rustls::ConfigBuilder::with_safe_default_protocol_versions`]. -#[deprecated(since = "0.4.0", note = "use the `BuilderVerifierExt` instead")] -pub fn tls_config_with_provider( - provider: Arc, -) -> Result { - Ok(ClientConfig::builder_with_provider(provider) - .with_safe_default_protocol_versions()? - .with_platform_verifier() - .with_no_client_auth()) -} - /// Exposed for debugging certificate issues with standalone tools. /// -/// This is not intended for production use, you should use [tls_config] instead. +/// This is not intended for production use, you should use [`BuilderVerifierExt`] or +/// [`ConfigVerifierExt`] instead. #[cfg(feature = "dbg")] -pub fn verifier_for_dbg(root: &[u8]) -> Arc { - Arc::new(Verifier::new_with_fake_root(root)) +pub fn verifier_for_dbg( + root: CertificateDer<'static>, + crypto_provider: Arc, +) -> Arc { + Arc::new(Verifier::new_with_fake_root(root, crypto_provider)) } /// Extension trait to help configure [`ClientConfig`]s with the platform verifier. @@ -97,16 +52,22 @@ pub trait BuilderVerifierExt { /// use rustls_platform_verifier::BuilderVerifierExt; /// let config = ClientConfig::builder() /// .with_platform_verifier() + /// .unwrap() /// .with_no_client_auth(); /// ``` - fn with_platform_verifier(self) -> ConfigBuilder; + fn with_platform_verifier( + self, + ) -> Result, rustls::Error>; } impl BuilderVerifierExt for ConfigBuilder { - fn with_platform_verifier(self) -> ConfigBuilder { - let provider = self.crypto_provider().clone(); - self.dangerous() - .with_custom_certificate_verifier(Arc::new(Verifier::new().with_provider(provider))) + fn with_platform_verifier( + self, + ) -> Result, rustls::Error> { + let verifier = Verifier::new(self.crypto_provider().clone())?; + Ok(self + .dangerous() + .with_custom_certificate_verifier(Arc::new(verifier))) } } @@ -119,13 +80,13 @@ pub trait ConfigVerifierExt { /// use rustls_platform_verifier::ConfigVerifierExt; /// let config = ClientConfig::with_platform_verifier(); /// ``` - fn with_platform_verifier() -> ClientConfig; + fn with_platform_verifier() -> Result; } impl ConfigVerifierExt for ClientConfig { - fn with_platform_verifier() -> ClientConfig { - ClientConfig::builder() - .with_platform_verifier() - .with_no_client_auth() + fn with_platform_verifier() -> Result { + Ok(ClientConfig::builder() + .with_platform_verifier()? + .with_no_client_auth()) } } diff --git a/rustls-platform-verifier/src/tests/ffi.rs b/rustls-platform-verifier/src/tests/ffi.rs index 70ab5739..23fc54fa 100644 --- a/rustls-platform-verifier/src/tests/ffi.rs +++ b/rustls-platform-verifier/src/tests/ffi.rs @@ -47,7 +47,6 @@ mod android { .with_filter(log_filter), ); crate::android::init_with_env(env, cx).unwrap(); - crate::tests::ensure_global_state(); std::panic::set_hook(Box::new(|info| { let msg = if let Some(msg) = info.payload().downcast_ref::<&'static str>() { msg diff --git a/rustls-platform-verifier/src/tests/mod.rs b/rustls-platform-verifier/src/tests/mod.rs index bafaff23..9e5c7a9e 100644 --- a/rustls-platform-verifier/src/tests/mod.rs +++ b/rustls-platform-verifier/src/tests/mod.rs @@ -1,14 +1,18 @@ #[cfg(feature = "ffi-testing")] pub mod ffi; -use std::error::Error as StdError; use std::time::Duration; +use std::{error::Error as StdError, sync::Arc}; mod verification_real_world; mod verification_mock; -use rustls::{pki_types, CertificateError, Error as TlsError, Error::InvalidCertificate}; +use rustls::{ + crypto::CryptoProvider, + pki_types, CertificateError, + Error::{self as TlsError, InvalidCertificate}, +}; struct TestCase<'a, E: StdError> { /// The name of the server we're connecting to. @@ -62,6 +66,6 @@ pub(crate) fn verification_time() -> pki_types::UnixTime { pki_types::UnixTime::since_unix_epoch(Duration::from_secs(1_748_633_220)) } -fn ensure_global_state() { - let _ = rustls::crypto::ring::default_provider().install_default(); +fn test_provider() -> Arc { + Arc::new(rustls::crypto::ring::default_provider()) } diff --git a/rustls-platform-verifier/src/tests/verification_mock/mod.rs b/rustls-platform-verifier/src/tests/verification_mock/mod.rs index 2bb55d80..38a2686b 100644 --- a/rustls-platform-verifier/src/tests/verification_mock/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_mock/mod.rs @@ -34,7 +34,7 @@ use rustls::pki_types::{DnsName, ServerName}; use rustls::{CertificateError, Error as TlsError, OtherError}; use super::TestCase; -use crate::tests::{assert_cert_error_eq, ensure_global_state, verification_time}; +use crate::tests::{assert_cert_error_eq, test_provider, verification_time}; use crate::verification::{EkuError, Verifier}; macro_rules! mock_root_test_cases { @@ -80,7 +80,8 @@ macro_rules! no_error { }; } -const ROOT1: &[u8] = include_bytes!("root1.crt"); +const ROOT1: pki_types::CertificateDer<'static> = + pki_types::CertificateDer::from_slice(include_bytes!("root1.crt")); const ROOT1_INT1: &[u8] = include_bytes!("root1-int1.crt"); const ROOT1_INT1_EXAMPLE_COM_GOOD: &[u8] = include_bytes!("root1-int1-ee_example.com-good.crt"); const ROOT1_INT1_LOCALHOST_IPV4_GOOD: &[u8] = include_bytes!("root1-int1-ee_127.0.0.1-good.crt"); @@ -93,18 +94,21 @@ const LOCALHOST_IPV6: &str = "::1"; #[cfg(any(test, feature = "ffi-testing"))] #[cfg_attr(feature = "ffi-testing", allow(dead_code))] pub(super) fn verification_without_mock_root() { - ensure_global_state(); + let crypto_provider = test_provider(); + // 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-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_root_certs::TLS_SERVER_ROOT_CERTS.iter().cloned()) - .unwrap(); + let verifier = Verifier::new_with_extra_roots( + webpki_root_certs::TLS_SERVER_ROOT_CERTS.iter().cloned(), + crypto_provider, + ) + .unwrap(); #[cfg(not(target_os = "freebsd"))] - let verifier = Verifier::new(); + let verifier = Verifier::new(crypto_provider).unwrap(); let server_name = pki_types::ServerName::try_from(EXAMPLE_COM).unwrap(); let end_entity = pki_types::CertificateDer::from(ROOT1_INT1_EXAMPLE_COM_GOOD); @@ -334,13 +338,13 @@ fn test_with_mock_root( test_case: &TestCase, root_src: Roots, ) { - ensure_global_state(); log::info!("verifying {:?}", test_case.expected_result); + let provider = test_provider(); let verifier = match root_src { - Roots::OnlyExtra => Verifier::new_with_fake_root(ROOT1), // TODO: time + Roots::OnlyExtra => Verifier::new_with_fake_root(ROOT1, provider), // TODO: time #[cfg(not(target_os = "android"))] - Roots::ExtraAndPlatform => Verifier::new_with_extra_roots([ROOT1.into()]).unwrap(), + Roots::ExtraAndPlatform => Verifier::new_with_extra_roots([ROOT1], provider).unwrap(), }; let mut chain = test_case .chain 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 fcaf70d6..d78861a8 100644 --- a/rustls-platform-verifier/src/tests/verification_real_world/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_real_world/mod.rs @@ -42,7 +42,7 @@ use rustls::pki_types::{DnsName, ServerName}; use rustls::{CertificateError, Error as TlsError}; use super::TestCase; -use crate::tests::{assert_cert_error_eq, ensure_global_state, verification_time}; +use crate::tests::{assert_cert_error_eq, test_provider, verification_time}; use crate::Verifier; // This is the certificate chain presented by one server for @@ -120,22 +120,25 @@ macro_rules! no_error { } fn real_world_test(test_case: &TestCase) { - ensure_global_state(); log::info!( "verifying ref ID {:?} expected {:?}", test_case.reference_id, test_case.expected_result ); + let crypto_provider = test_provider(); + // On BSD systems openssl-probe fails to find the system CA bundle, // so we must provide extra roots from webpki-root-cert. #[cfg(target_os = "freebsd")] - let verifier = - Verifier::new_with_extra_roots(webpki_root_certs::TLS_SERVER_ROOT_CERTS.iter().cloned()) - .unwrap(); + let verifier = Verifier::new_with_extra_roots( + webpki_root_certs::TLS_SERVER_ROOT_CERTS.iter().cloned(), + crypto_provider, + ) + .unwrap(); #[cfg(not(target_os = "freebsd"))] - let verifier = Verifier::new(); + let verifier = Verifier::new(crypto_provider).unwrap(); let mut chain = test_case .chain diff --git a/rustls-platform-verifier/src/verification/android.rs b/rustls-platform-verifier/src/verification/android.rs index 2c145d8c..7f602e89 100644 --- a/rustls-platform-verifier/src/verification/android.rs +++ b/rustls-platform-verifier/src/verification/android.rs @@ -5,7 +5,6 @@ use jni::{ strings::JavaStr, JNIEnv, }; -use once_cell::sync::OnceCell; use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerifier}; use rustls::crypto::{verify_tls12_signature, verify_tls13_signature, CryptoProvider}; use rustls::pki_types; @@ -46,14 +45,8 @@ const AUTH_TYPE: &str = "RSA"; pub struct Verifier { /// Testing only: The root CA certificate to trust. #[cfg(any(test, feature = "ffi-testing"))] - test_only_root_ca_override: Option>, - pub(super) crypto_provider: OnceCell>, -} - -impl Default for Verifier { - fn default() -> Self { - Self::new() - } + test_only_root_ca_override: Option>, + crypto_provider: Arc, } #[cfg(any(test, feature = "ffi-testing"))] @@ -71,24 +64,23 @@ impl Drop for Verifier { impl Verifier { /// Creates a new instance of a TLS certificate verifier that utilizes the /// Android certificate facilities. - /// - /// A [`CryptoProvider`] must be set with - /// [`set_provider`][Verifier::set_provider]/[`with_provider`][Verifier::with_provider] or - /// [`CryptoProvider::install_default`] before the verifier can be used. - pub fn new() -> Self { - Self { + pub fn new(crypto_provider: Arc) -> Result { + Ok(Self { #[cfg(any(test, feature = "ffi-testing"))] test_only_root_ca_override: None, - crypto_provider: OnceCell::new(), - } + crypto_provider, + }) } /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert. #[cfg(any(test, feature = "ffi-testing"))] - pub(crate) fn new_with_fake_root(root: &[u8]) -> Self { + pub(crate) fn new_with_fake_root( + root: pki_types::CertificateDer<'static>, + crypto_provider: Arc, + ) -> Self { Self { - test_only_root_ca_override: Some(root.into()), - crypto_provider: OnceCell::new(), + test_only_root_ca_override: Some(root), + crypto_provider, } } @@ -314,7 +306,7 @@ impl ServerCertVerifier for Verifier { message, cert, dss, - &self.get_provider().signature_verification_algorithms, + &self.crypto_provider.signature_verification_algorithms, ) } @@ -328,12 +320,12 @@ impl ServerCertVerifier for Verifier { message, cert, dss, - &self.get_provider().signature_verification_algorithms, + &self.crypto_provider.signature_verification_algorithms, ) } fn supported_verify_schemes(&self) -> Vec { - self.get_provider() + self.crypto_provider .signature_verification_algorithms .supported_schemes() } diff --git a/rustls-platform-verifier/src/verification/apple.rs b/rustls-platform-verifier/src/verification/apple.rs index 03ea14d8..34c94502 100644 --- a/rustls-platform-verifier/src/verification/apple.rs +++ b/rustls-platform-verifier/src/verification/apple.rs @@ -1,10 +1,7 @@ use std::sync::Arc; -use super::log_server_cert; -use crate::verification::invalid_certificate; use core_foundation::date::CFDate; use core_foundation_sys::date::kCFAbsoluteTimeIntervalSince1970; -use once_cell::sync::OnceCell; use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerifier}; use rustls::crypto::{verify_tls12_signature, verify_tls13_signature, CryptoProvider}; use rustls::pki_types; @@ -16,6 +13,9 @@ use security_framework::{ trust::SecTrust, }; +use super::log_server_cert; +use crate::verification::invalid_certificate; + mod errors { pub(super) use security_framework_sys::base::{ errSecCertificateRevoked, errSecCreateChainFailed, errSecHostNameMismatch, @@ -49,23 +49,19 @@ pub struct Verifier { /// Testing only: The root CA certificate to trust. #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: Option, - pub(super) crypto_provider: OnceCell>, + crypto_provider: Arc, } impl Verifier { /// Creates a new instance of a TLS certificate verifier that utilizes the Apple certificate /// facilities. - /// - /// A [`CryptoProvider`] must be set with - /// [`set_provider`][Verifier::set_provider]/[`with_provider`][Verifier::with_provider] or - /// [`CryptoProvider::install_default`] before the verifier can be used. - pub fn new() -> Self { - Self { + pub fn new(crypto_provider: Arc) -> Result { + Ok(Self { extra_roots: Vec::new(), #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, - crypto_provider: OnceCell::new(), - } + crypto_provider, + }) } /// Creates a new instance of a TLS certificate verifier that utilizes the Apple certificate @@ -74,6 +70,7 @@ impl Verifier { /// See [Verifier::new] for the external requirements the verifier needs. pub fn new_with_extra_roots( roots: impl IntoIterator>, + crypto_provider: Arc, ) -> Result { let extra_roots = roots .into_iter() @@ -86,17 +83,20 @@ impl Verifier { extra_roots, #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, - crypto_provider: OnceCell::new(), + crypto_provider, }) } /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert. #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] - pub(crate) fn new_with_fake_root(root: &[u8]) -> Self { + pub(crate) fn new_with_fake_root( + root: pki_types::CertificateDer<'static>, + crypto_provider: Arc, + ) -> Self { Self { extra_roots: Vec::new(), - test_only_root_ca_override: Some(SecCertificate::from_der(root).unwrap()), - crypto_provider: OnceCell::new(), + test_only_root_ca_override: Some(SecCertificate::from_der(root.as_ref()).unwrap()), + crypto_provider, } } @@ -282,7 +282,7 @@ impl ServerCertVerifier for Verifier { message, cert, dss, - &self.get_provider().signature_verification_algorithms, + &self.crypto_provider.signature_verification_algorithms, ) } @@ -296,19 +296,13 @@ impl ServerCertVerifier for Verifier { message, cert, dss, - &self.get_provider().signature_verification_algorithms, + &self.crypto_provider.signature_verification_algorithms, ) } fn supported_verify_schemes(&self) -> Vec { - self.get_provider() + self.crypto_provider .signature_verification_algorithms .supported_schemes() } } - -impl Default for Verifier { - fn default() -> Self { - Self::new() - } -} diff --git a/rustls-platform-verifier/src/verification/mod.rs b/rustls-platform-verifier/src/verification/mod.rs index 8d329143..eba96ea9 100644 --- a/rustls-platform-verifier/src/verification/mod.rs +++ b/rustls-platform-verifier/src/verification/mod.rs @@ -1,4 +1,4 @@ -use rustls::crypto::CryptoProvider; +#[cfg(any(windows, target_vendor = "apple"))] use std::sync::Arc; #[cfg(all( @@ -84,30 +84,3 @@ const ALLOWED_EKUS: &[windows_sys::core::PCSTR] = &[windows_sys::Win32::Security::Cryptography::szOID_PKIX_KP_SERVER_AUTH]; #[cfg(target_os = "android")] pub const ALLOWED_EKUS: &[&str] = &["1.3.6.1.5.5.7.3.1"]; - -impl Verifier { - /// Chainable setter to configure the [`CryptoProvider`] for this `Verifier`. - /// - /// This will be used instead of the rustls process-default `CryptoProvider`, even if one has - /// been installed. - pub fn with_provider(mut self, crypto_provider: Arc) -> Self { - self.set_provider(crypto_provider); - self - } - - /// Configures the [`CryptoProvider`] for this `Verifier`. - /// - /// This will be used instead of the rustls process-default `CryptoProvider`, even if one has - /// been installed. - pub fn set_provider(&mut self, crypto_provider: Arc) { - self.crypto_provider = crypto_provider.into(); - } - - fn get_provider(&self) -> &Arc { - self.crypto_provider.get_or_init(|| { - CryptoProvider::get_default() - .expect("rustls default CryptoProvider not set") - .clone() - }) - } -} diff --git a/rustls-platform-verifier/src/verification/others.rs b/rustls-platform-verifier/src/verification/others.rs index f4b7e9ad..63cb3b8d 100644 --- a/rustls-platform-verifier/src/verification/others.rs +++ b/rustls-platform-verifier/src/verification/others.rs @@ -1,5 +1,6 @@ -use super::log_server_cert; -use once_cell::sync::OnceCell; +use std::fmt::Debug; +use std::sync::Arc; + use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}; use rustls::client::WebPkiServerVerifier; use rustls::pki_types; @@ -7,8 +8,8 @@ use rustls::{ crypto::CryptoProvider, CertificateError, DigitallySignedStruct, Error as TlsError, OtherError, SignatureScheme, }; -use std::fmt::Debug; -use std::sync::{Arc, Mutex}; + +use super::log_server_cert; /// A TLS certificate verifier that uses the system's root store and WebPKI. #[derive(Debug)] @@ -20,105 +21,67 @@ pub struct Verifier { // locking and unlocking the application will pull fresh root // certificates from disk, picking up on any changes // that might have been made since. - inner: OnceCell>, - - // Extra trust anchors to add to the verifier above and beyond those provided by the - // platform via rustls-native-certs. - extra_roots: Mutex>>, - - /// Testing only: an additional root CA certificate to trust. - #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] - test_only_root_ca_override: Option>, - - pub(super) crypto_provider: OnceCell>, + inner: Arc, } impl Verifier { /// Creates a new verifier whose certificate validation is provided by /// WebPKI, using root certificates provided by the platform. - /// - /// A [`CryptoProvider`] must be set with - /// [`set_provider`][Verifier::set_provider]/[`with_provider`][Verifier::with_provider] or - /// [`CryptoProvider::install_default`] before the verifier can be used. - pub fn new() -> Self { - Self { - inner: OnceCell::new(), - extra_roots: Vec::new().into(), - #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] - test_only_root_ca_override: None, - crypto_provider: OnceCell::new(), - } + pub fn new(crypto_provider: Arc) -> Result { + Self::new_inner([], None, crypto_provider) } /// 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>, + extra_roots: impl IntoIterator>, + crypto_provider: Arc, ) -> Result { - Ok(Self { - inner: OnceCell::new(), - 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(), - }) + Self::new_inner(extra_roots, None, crypto_provider) } /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert. #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] - pub(crate) fn new_with_fake_root(root: &[u8]) -> Self { - Self { - inner: OnceCell::new(), - extra_roots: Vec::new().into(), - test_only_root_ca_override: Some(root.into()), - crypto_provider: OnceCell::new(), - } - } - - fn get_or_init_verifier(&self) -> Result<&Arc, TlsError> { - self.inner.get_or_try_init(|| self.init_verifier()) + pub(crate) fn new_with_fake_root( + root: pki_types::CertificateDer<'static>, + crypto_provider: Arc, + ) -> Self { + Self::new_inner([], Some(root), crypto_provider) + .expect("failed to create verifier with fake root") } - // Attempt to load CA root certificates present on system, fallback to WebPKI roots if error - fn init_verifier(&self) -> Result, TlsError> { + /// 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. + fn new_inner( + extra_roots: impl IntoIterator>, + #[allow(unused)] // test_root is only used in tests + test_root: Option>, + crypto_provider: Arc, + ) -> Result { let mut root_store = rustls::RootCertStore::empty(); // For testing only: load fake root cert, instead of native/WebPKI roots #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] { - if let Some(test_root) = &self.test_only_root_ca_override { - let (added, ignored) = - root_store.add_parsable_certificates([pki_types::CertificateDer::from( - test_root.as_ref(), - )]); - if (added != 1) || (ignored != 0) { - panic!("Failed to insert fake, test-only root trust anchor"); - } - return Ok(WebPkiServerVerifier::builder_with_provider( - root_store.into(), - Arc::clone(self.get_provider()), - ) - .build() - .unwrap()); + if let Some(test_root) = test_root { + root_store.add(test_root)?; + return Ok(Self { + inner: WebPkiServerVerifier::builder_with_provider( + root_store.into(), + crypto_provider.clone(), + ) + .build() + .map_err(|e| TlsError::Other(OtherError(Arc::new(e))))?, + }); } } - // Safety: There's no way for the mutex to be locked multiple times, so this is - // an infallible operation. - let mut extra_roots = self.extra_roots.try_lock().unwrap(); - if !extra_roots.is_empty() { - let count = extra_roots.len(); - root_store.extend(extra_roots.drain(..)); - log::debug!( - "Loaded {count} extra CA certificates in addition to possible system roots", - ); + // While we ignore invalid certificates from the system, we forward errors from + // parsing the extra roots to the caller. + for cert in extra_roots { + root_store.add(cert)?; } #[cfg(all( @@ -130,8 +93,8 @@ impl Verifier { { let result = rustls_native_certs::load_native_certs(); let (added, ignored) = root_store.add_parsable_certificates(result.certs); - if ignored != 0 { - log::warn!("Some CA root certificates were ignored due to errors"); + if ignored > 0 { + log::warn!("{ignored} platform CA root certificates were ignored due to errors"); } for error in result.errors { @@ -145,7 +108,7 @@ impl Verifier { "No CA certificates were loaded from the system".to_owned(), )); } else { - log::debug!("Loaded {added} CA certificates from the system"); + log::debug!("Loaded {added} CA root certificates from the system"); } } @@ -156,12 +119,14 @@ impl Verifier { ); }; - WebPkiServerVerifier::builder_with_provider( - root_store.into(), - Arc::clone(self.get_provider()), - ) - .build() - .map_err(|e| TlsError::Other(OtherError(Arc::new(e)))) + Ok(Self { + inner: WebPkiServerVerifier::builder_with_provider( + root_store.into(), + crypto_provider.clone(), + ) + .build() + .map_err(|e| TlsError::Other(OtherError(Arc::new(e))))?, + }) } } @@ -176,7 +141,7 @@ impl ServerCertVerifier for Verifier { ) -> Result { log_server_cert(end_entity); - self.get_or_init_verifier()? + self.inner .verify_server_cert(end_entity, intermediates, server_name, ocsp_response, now) .map_err(map_webpki_errors) // This only contains information from the system or other public @@ -193,8 +158,7 @@ impl ServerCertVerifier for Verifier { cert: &pki_types::CertificateDer<'_>, dss: &DigitallySignedStruct, ) -> Result { - self.get_or_init_verifier()? - .verify_tls12_signature(message, cert, dss) + self.inner.verify_tls12_signature(message, cert, dss) } fn verify_tls13_signature( @@ -203,24 +167,11 @@ impl ServerCertVerifier for Verifier { cert: &pki_types::CertificateDer<'_>, dss: &DigitallySignedStruct, ) -> Result { - self.get_or_init_verifier()? - .verify_tls13_signature(message, cert, dss) + self.inner.verify_tls13_signature(message, cert, dss) } fn supported_verify_schemes(&self) -> Vec { - // XXX: Don't go through `self.verifier` here: It introduces extra failure - // cases and is strictly unneeded because `get_provider` is the same provider and - // set of algorithms passed into the wrapped `WebPkiServerVerifier`. Given this, - // the list of schemes are identical. - self.get_provider() - .signature_verification_algorithms - .supported_schemes() - } -} - -impl Default for Verifier { - fn default() -> Self { - Self::new() + self.inner.supported_verify_schemes() } } diff --git a/rustls-platform-verifier/src/verification/windows.rs b/rustls-platform-verifier/src/verification/windows.rs index f3db4207..685f841b 100644 --- a/rustls-platform-verifier/src/verification/windows.rs +++ b/rustls-platform-verifier/src/verification/windows.rs @@ -18,15 +18,6 @@ //! [Microsoft's Documentation]: //! [Microsoft's Example]: -use super::{log_server_cert, ALLOWED_EKUS}; -use once_cell::sync::OnceCell; -use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerifier}; -use rustls::crypto::{verify_tls12_signature, verify_tls13_signature, CryptoProvider}; -use rustls::pki_types; -use rustls::{ - CertificateError, DigitallySignedStruct, Error as TlsError, Error::InvalidCertificate, - SignatureScheme, -}; use std::{ convert::TryInto, mem::{self, MaybeUninit}, @@ -34,6 +25,14 @@ use std::{ ptr::{self, NonNull}, sync::Arc, }; + +use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerifier}; +use rustls::crypto::{verify_tls12_signature, verify_tls13_signature, CryptoProvider}; +use rustls::pki_types; +use rustls::{ + CertificateError, DigitallySignedStruct, Error as TlsError, Error::InvalidCertificate, + SignatureScheme, +}; use windows_sys::Win32::{ Foundation::{ BOOL, CERT_E_CN_NO_MATCH, CERT_E_EXPIRED, CERT_E_INVALID_NAME, CERT_E_UNTRUSTEDROOT, @@ -55,6 +54,8 @@ use windows_sys::Win32::{ }, }; +use super::{log_server_cert, ALLOWED_EKUS}; + // The `windows-sys` definition for `CERT_CHAIN_PARA` does not take old OS versions // into account so we define it ourselves for better (hypothetical) OS backwards compat. // In the future a compile-time size assertion can be added against the upstream type to help stay in sync. @@ -486,8 +487,8 @@ fn call_with_last_error Option>(mut call: F) -> Result>, - pub(super) crypto_provider: OnceCell>, + test_only_root_ca_override: Option>, + crypto_provider: Arc, /// Extra trust anchors to add to the verifier above and beyond those provided by /// the system-provided trust stores. extra_roots: Option, @@ -496,43 +497,39 @@ pub struct Verifier { impl Verifier { /// Creates a new instance of a TLS certificate verifier that utilizes the /// Windows certificate facilities. - /// - /// A [`CryptoProvider`] must be set with - /// [`set_provider`][Verifier::set_provider]/[`with_provider`][Verifier::with_provider] or - /// [`CryptoProvider::install_default`] before the verifier can be used. - pub fn new() -> Self { - Self { + pub fn new(crypto_provider: Arc) -> Result { + Ok(Self { #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, - crypto_provider: OnceCell::new(), + crypto_provider, extra_roots: None, - } + }) } /// Creates a new instance of a TLS certificate verifier that utilizes the /// Windows certificate facilities and augmented by the provided extra root certificates. - /// - /// A [`CryptoProvider`] must be set with - /// [`set_provider`][Verifier::set_provider]/[`with_provider`][Verifier::with_provider] or - /// [`CryptoProvider::install_default`] before the verifier can be used. pub fn new_with_extra_roots( roots: impl IntoIterator>, + crypto_provider: Arc, ) -> Result { let cert_engine = CertEngine::new_with_extra_roots(roots)?; Ok(Self { #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, - crypto_provider: OnceCell::new(), + crypto_provider, extra_roots: Some(cert_engine), }) } /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert. #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] - pub(crate) fn new_with_fake_root(root: &[u8]) -> Self { + pub(crate) fn new_with_fake_root( + root: pki_types::CertificateDer<'static>, + crypto_provider: Arc, + ) -> Self { Self { - test_only_root_ca_override: Some(root.into()), - crypto_provider: OnceCell::new(), + test_only_root_ca_override: Some(root), + crypto_provider, extra_roots: None, } } @@ -686,7 +683,7 @@ impl ServerCertVerifier for Verifier { message, cert, dss, - &self.get_provider().signature_verification_algorithms, + &self.crypto_provider.signature_verification_algorithms, ) } @@ -700,23 +697,17 @@ impl ServerCertVerifier for Verifier { message, cert, dss, - &self.get_provider().signature_verification_algorithms, + &self.crypto_provider.signature_verification_algorithms, ) } fn supported_verify_schemes(&self) -> Vec { - self.get_provider() + self.crypto_provider .signature_verification_algorithms .supported_schemes() } } -impl Default for Verifier { - fn default() -> Self { - Self::new() - } -} - /// A trait to represent an object that can be safely created with all zero values /// and have a size assigned to it. ///