Eagerly build root store on miscellaneous Unix platforms

Author: djc

.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:

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();
 ```
 

rustls-platform-verifier/Cargo.toml

@@ -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 }

rustls-platform-verifier/src/lib.rs

@@ -52,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<ClientConfig, WantsClientCert>;
+    fn with_platform_verifier(
+        self,
+    ) -> Result<ConfigBuilder<ClientConfig, WantsClientCert>, rustls::Error>;
 }
 
 impl BuilderVerifierExt for ConfigBuilder<ClientConfig, WantsVerifier> {
-    fn with_platform_verifier(self) -> ConfigBuilder<ClientConfig, WantsClientCert> {
-        let provider = self.crypto_provider().clone();
-        self.dangerous()
-            .with_custom_certificate_verifier(Arc::new(Verifier::new(provider)))
+    fn with_platform_verifier(
+        self,
+    ) -> Result<ConfigBuilder<ClientConfig, WantsClientCert>, rustls::Error> {
+        let verifier = Verifier::new(self.crypto_provider().clone())?;
+        Ok(self
+            .dangerous()
+            .with_custom_certificate_verifier(Arc::new(verifier)))
     }
 }
 
@@ -74,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<ClientConfig, rustls::Error>;
 }
 
 impl ConfigVerifierExt for ClientConfig {
-    fn with_platform_verifier() -> ClientConfig {
-        ClientConfig::builder()
-            .with_platform_verifier()
-            .with_no_client_auth()
+    fn with_platform_verifier() -> Result<ClientConfig, rustls::Error> {
+        Ok(ClientConfig::builder()
+            .with_platform_verifier()?
+            .with_no_client_auth())
     }
 }

rustls-platform-verifier/src/tests/verification_mock/mod.rs

@@ -108,7 +108,7 @@ pub(super) fn verification_without_mock_root() {
     .unwrap();
 
     #[cfg(not(target_os = "freebsd"))]
-    let verifier = Verifier::new(crypto_provider);
+    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);

rustls-platform-verifier/src/tests/verification_real_world/mod.rs

@@ -138,7 +138,7 @@ fn real_world_test<E: std::error::Error>(test_case: &TestCase<E>) {
     .unwrap();
 
     #[cfg(not(target_os = "freebsd"))]
-    let verifier = Verifier::new(crypto_provider);
+    let verifier = Verifier::new(crypto_provider).unwrap();
 
     let mut chain = test_case
         .chain

rustls-platform-verifier/src/verification/android.rs

@@ -64,12 +64,12 @@ impl Drop for Verifier {
 impl Verifier {
     /// Creates a new instance of a TLS certificate verifier that utilizes the
     /// Android certificate facilities.
-    pub fn new(crypto_provider: Arc<CryptoProvider>) -> Self {
-        Self {
+    pub fn new(crypto_provider: Arc<CryptoProvider>) -> Result<Self, TlsError> {
+        Ok(Self {
             #[cfg(any(test, feature = "ffi-testing"))]
             test_only_root_ca_override: None,
             crypto_provider,
-        }
+        })
     }
 
     /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert.

rustls-platform-verifier/src/verification/apple.rs

@@ -55,13 +55,13 @@ pub struct Verifier {
 impl Verifier {
     /// Creates a new instance of a TLS certificate verifier that utilizes the Apple certificate
     /// facilities.
-    pub fn new(crypto_provider: Arc<CryptoProvider>) -> Self {
-        Self {
+    pub fn new(crypto_provider: Arc<CryptoProvider>) -> Result<Self, TlsError> {
+        Ok(Self {
             extra_roots: Vec::new(),
             #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))]
             test_only_root_ca_override: None,
             crypto_provider,
-        }
+        })
     }
 
     /// Creates a new instance of a TLS certificate verifier that utilizes the Apple certificate

rustls-platform-verifier/src/verification/others.rs

@@ -1,13 +1,15 @@
-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;
 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)]
@@ -19,52 +21,24 @@ 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<Arc<WebPkiServerVerifier>>,
-
-    // Extra trust anchors to add to the verifier above and beyond those provided by the
-    // platform via rustls-native-certs.
-    extra_roots: Mutex<Vec<pki_types::TrustAnchor<'static>>>,
-
-    /// Testing only: an additional root CA certificate to trust.
-    #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))]
-    test_only_root_ca_override: Option<pki_types::CertificateDer<'static>>,
-
-    crypto_provider: Arc<CryptoProvider>,
+    inner: Arc<WebPkiServerVerifier>,
 }
 
 impl Verifier {
     /// Creates a new verifier whose certificate validation is provided by
     /// WebPKI, using root certificates provided by the platform.
-    pub fn new(crypto_provider: Arc<CryptoProvider>) -> 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,
-        }
+    pub fn new(crypto_provider: Arc<CryptoProvider>) -> Result<Self, TlsError> {
+        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<Item = pki_types::CertificateDer<'static>>,
+        extra_roots: impl IntoIterator<Item = pki_types::CertificateDer<'static>>,
         crypto_provider: Arc<CryptoProvider>,
     ) -> Result<Self, TlsError> {
-        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::<Vec<_>>()
-                .into(),
-            #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))]
-            test_only_root_ca_override: None,
-            crypto_provider,
-        })
+        Self::new_inner(extra_roots, None, crypto_provider)
     }
 
     /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert.
@@ -73,51 +47,49 @@ impl Verifier {
         root: pki_types::CertificateDer<'static>,
         crypto_provider: Arc<CryptoProvider>,
     ) -> Self {
-        Self {
-            inner: OnceCell::new(),
-            extra_roots: Vec::new().into(),
-            test_only_root_ca_override: Some(root),
-            crypto_provider,
-        }
+        Self::new_inner([], Some(root), crypto_provider)
+            .expect("failed to create verifier with fake root")
     }
 
-    fn get_or_init_verifier(&self) -> Result<&Arc<WebPkiServerVerifier>, TlsError> {
-        self.inner.get_or_try_init(|| self.init_verifier())
-    }
-
-    // Attempt to load CA root certificates present on system, fallback to WebPKI roots if error
-    fn init_verifier(&self) -> Result<Arc<WebPkiServerVerifier>, 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<Item = pki_types::CertificateDer<'static>>,
+        #[allow(unused)] // test_root is only used in tests
+        test_root: Option<pki_types::CertificateDer<'static>>,
+        crypto_provider: Arc<CryptoProvider>,
+    ) -> Result<Self, TlsError> {
         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(),
-                    self.crypto_provider.clone(),
-                )
-                .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",
-            );
+        let mut num_extra_roots = 0;
+        let (added, ignored) =
+            root_store.add_parsable_certificates(extra_roots.into_iter().inspect(|_| {
+                num_extra_roots += 1;
+            }));
+
+        if num_extra_roots > 0 {
+            if ignored > 0 {
+                log::warn!("{ignored} extra CA root certificates were ignored due to errors");
+            } else {
+                log::debug!("Loaded {added} CA root certificates from extra roots");
+            }
         }
 
         #[cfg(all(
@@ -129,8 +101,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 {
@@ -144,7 +116,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");
             }
         }
 
@@ -155,9 +127,14 @@ impl Verifier {
             );
         };
 
-        WebPkiServerVerifier::builder_with_provider(root_store.into(), self.crypto_provider.clone())
+        Ok(Self {
+            inner: WebPkiServerVerifier::builder_with_provider(
+                root_store.into(),
+                crypto_provider.clone(),
+            )
             .build()
-            .map_err(|e| TlsError::Other(OtherError(Arc::new(e))))
+            .map_err(|e| TlsError::Other(OtherError(Arc::new(e))))?,
+        })
     }
 }
 
@@ -172,7 +149,7 @@ impl ServerCertVerifier for Verifier {
     ) -> Result<ServerCertVerified, TlsError> {
         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
@@ -189,8 +166,7 @@ impl ServerCertVerifier for Verifier {
         cert: &pki_types::CertificateDer<'_>,
         dss: &DigitallySignedStruct,
     ) -> Result<HandshakeSignatureValid, TlsError> {
-        self.get_or_init_verifier()?
-            .verify_tls12_signature(message, cert, dss)
+        self.inner.verify_tls12_signature(message, cert, dss)
     }
 
     fn verify_tls13_signature(
@@ -199,18 +175,11 @@ impl ServerCertVerifier for Verifier {
         cert: &pki_types::CertificateDer<'_>,
         dss: &DigitallySignedStruct,
     ) -> Result<HandshakeSignatureValid, TlsError> {
-        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<SignatureScheme> {
-        // 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.crypto_provider
-            .signature_verification_algorithms
-            .supported_schemes()
+        self.inner.supported_verify_schemes()
     }
 }
 

rustls-platform-verifier/src/verification/windows.rs

@@ -497,13 +497,13 @@ pub struct Verifier {
 impl Verifier {
     /// Creates a new instance of a TLS certificate verifier that utilizes the
     /// Windows certificate facilities.
-    pub fn new(crypto_provider: Arc<CryptoProvider>) -> Self {
-        Self {
+    pub fn new(crypto_provider: Arc<CryptoProvider>) -> Result<Self, TlsError> {
+        Ok(Self {
             #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))]
             test_only_root_ca_override: None,
             crypto_provider,
             extra_roots: None,
-        }
+        })
     }
 
     /// Creates a new instance of a TLS certificate verifier that utilizes the