Rm 'verify_server_cert' option from WorkerBuilder

jonhoo · Aug 18, 2024 · 2c94355 · 2c94355
1 parent 0826522
commit 2c94355
Show file tree

Hide file tree

Showing 6 changed files with 13 additions and 188 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -53,7 +53,6 @@ tokio = { version = "1.35.1", features = [
 tokio-native-tls = { version = "0.3.1", optional = true }
 tokio-rustls = { version = "0.25.0", optional = true }
 rustls-native-certs = { version = "0.7.1", optional = true }
-rustls-pki-types = { version = "1.0.1", optional = true }
 tracing = "0.1"
 url = "2"
 semver = { version = "1.0.23", features = ["serde"] }

diff --git a/src/tls/native_tls.rs b/src/tls/native_tls.rs
@@ -62,24 +62,6 @@ impl TlsStream<TokioTcpStream> {
         .await
     }
 
-    /// Create a new TLS connection over TCP **dangerously** skipping TLS verification.
-    ///
-    /// Similar to [`TlsStream::connect`], but accepting invalid server certificates and
-    /// invalid hostnames.
-    pub async fn connect_dangerously_skipping_verification(
-        url: Option<&str>,
-    ) -> Result<Self, Error> {
-        TlsStream::with_connector(
-            TlsConnector::builder()
-                .danger_accept_invalid_certs(true)
-                .danger_accept_invalid_hostnames(true)
-                .build()
-                .map_err(error::Stream::NativeTls)?,
-            url,
-        )
-        .await
-    }
-
     /// Create a new TLS connection over TCP using a non-default TLS configuration.
     ///
     /// See `connect` for details about the `url` parameter.

diff --git a/src/tls/rustls.rs b/src/tls/rustls.rs
@@ -3,20 +3,13 @@ use crate::{Client, WorkerBuilder};
 
 use crate::proto::{self, utils};
 use crate::{Error, Reconnect};
-use rustls_pki_types::{CertificateDer, ServerName, UnixTime};
 use std::io;
 use std::ops::{Deref, DerefMut};
 use std::sync::Arc;
 use tokio::io::{AsyncRead, AsyncWrite, BufStream};
 use tokio::net::TcpStream as TokioTcpStream;
 use tokio_rustls::client::TlsStream as RustlsStream;
-use tokio_rustls::rustls::client::danger::{
-    HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier,
-};
-use tokio_rustls::rustls::{
-    client::WebPkiServerVerifier, ClientConfig, DigitallySignedStruct, Error as RustlsError,
-    RootCertStore, SignatureScheme,
-};
+use tokio_rustls::rustls::{ClientConfig, RootCertStore};
 use tokio_rustls::TlsConnector;
 
 /// A reconnectable stream encrypted with TLS.
@@ -75,34 +68,16 @@ impl TlsStream<TokioTcpStream> {
 
     /// Create a new TLS connection over TCP using native certificates.
     ///
-    /// Unlike [`TlsStream::connect`], creates a root certificates store populated with the certificates
-    /// loaded from a platform-native certificate store. Thos method also allows to **dangerously**
-    /// skip server certificates verification.
-    pub async fn connect_with_native_certs(
-        url: Option<&str>,
-        dangerously_skip_verify: bool,
-    ) -> Result<Self, Error> {
+    /// Unlike [`TlsStream::connect`], creates a root certificates store populated
+    /// with the certificates loaded from a platform-native certificate store.
+    pub async fn connect_with_native_certs(url: Option<&str>) -> Result<Self, Error> {
         let mut store = RootCertStore::empty();
         for cert in rustls_native_certs::load_native_certs()? {
             store.add(cert).map_err(io::Error::other)?;
         }
-
-        let config = if dangerously_skip_verify {
-            let cert_verifier = WebPkiServerVerifier::builder(Arc::new(store.clone()))
-                .build()
-                .expect("can construct standard verifier");
-            let mut config = ClientConfig::builder()
-                .with_root_certificates(store)
-                .with_no_client_auth();
-            config
-                .dangerous()
-                .set_certificate_verifier(Arc::new(NoCertVerification(cert_verifier)));
-            config
-        } else {
-            ClientConfig::builder()
-                .with_root_certificates(store)
-                .with_no_client_auth()
-        };
+        let config = ClientConfig::builder()
+            .with_root_certificates(store)
+            .with_no_client_auth();
         TlsStream::with_connector(TlsConnector::from(Arc::new(config)), url).await
     }
 
@@ -165,44 +140,6 @@ where
     }
 }
 
-#[derive(Debug)]
-struct NoCertVerification(Arc<WebPkiServerVerifier>);
-
-impl ServerCertVerifier for NoCertVerification {
-    fn verify_server_cert(
-        &self,
-        _: &CertificateDer<'_>,
-        _: &[CertificateDer<'_>],
-        _: &ServerName<'_>,
-        _: &[u8],
-        _: UnixTime,
-    ) -> Result<ServerCertVerified, RustlsError> {
-        Ok(ServerCertVerified::assertion())
-    }
-
-    fn verify_tls12_signature(
-        &self,
-        message: &[u8],
-        cert: &rustls_pki_types::CertificateDer<'_>,
-        dss: &DigitallySignedStruct,
-    ) -> Result<HandshakeSignatureValid, RustlsError> {
-        self.0.verify_tls12_signature(message, cert, dss)
-    }
-
-    fn verify_tls13_signature(
-        &self,
-        message: &[u8],
-        cert: &CertificateDer<'_>,
-        dss: &DigitallySignedStruct,
-    ) -> Result<HandshakeSignatureValid, RustlsError> {
-        self.0.verify_tls13_signature(message, cert, dss)
-    }
-
-    fn supported_verify_schemes(&self) -> Vec<SignatureScheme> {
-        self.0.supported_verify_schemes()
-    }
-}
-
 #[async_trait::async_trait]
 impl Reconnect for BufStream<TlsStream<tokio::net::TcpStream>> {
     async fn reconnect(&mut self) -> io::Result<proto::BoxedConnection> {

diff --git a/src/worker/builder.rs b/src/worker/builder.rs
@@ -32,9 +32,6 @@ pub struct WorkerBuilder<E> {
     shutdown_timeout: Option<Duration>,
     shutdown_signal: Option<ShutdownSignal>,
     tls_kind: TlsKind,
-    #[cfg(any(feature = "native_tls", feature = "rustls"))]
-    #[cfg_attr(docsrs, doc(cfg(any(feature = "native_tls", feature = "rustls"))))]
-    verify_server_cert: bool,
 }
 
 impl<E> Default for WorkerBuilder<E> {
@@ -56,9 +53,6 @@ impl<E> Default for WorkerBuilder<E> {
             shutdown_timeout: None,
             shutdown_signal: None,
             tls_kind: TlsKind::None,
-            #[cfg(any(feature = "native_tls", feature = "rustls"))]
-            #[cfg_attr(docsrs, doc(cfg(any(feature = "native_tls", feature = "rustls"))))]
-            verify_server_cert: true,
         }
     }
 }
@@ -258,9 +252,7 @@ impl<E: 'static> WorkerBuilder<E> {
     /// _SecureTransport_ on OSX, and _OpenSSL_ on other platforms.
     ///
     /// Internally, will use [`TlsStream::connect`](crate::native_tls::TlsStream::connect) to establish
-    /// a TLS stream to the Faktory server. If [`WorkerBuilder::dangerously_without_cert_verification`]
-    /// has been called on this builder, [`TlsStream::connect_dangerously_skipping_verification`](crate::native_tls::TlsStream::connect_dangerously_skipping_verification)
-    /// will be used.
+    /// a TLS stream to the Faktory server.
     ///
     /// Note that if you use this method on the builder, but eventually use [`WorkerBuilder::connect_with`]
     /// (rather than [`WorkerBuilder::connect`]) to create an instance of [`Worker`], this worker
@@ -275,9 +267,7 @@ impl<E: 'static> WorkerBuilder<E> {
     /// Make the traffic between this worker and Faktory encrypted with [`rustls`](https://github.com/rustls/rustls).
     ///
     /// Internally, will use [`TlsStream::connect_with_native_certs`](crate::rustls::TlsStream::connect_with_native_certs)
-    /// to establish a TLS stream to the Faktory server. If [`WorkerBuilder::dangerously_without_cert_verification`]
-    /// has been called on this builder, a `true` will provided to [`TlsStream::connect_with_native_certs`](crate::rustls::TlsStream::connect_with_native_certs)
-    /// as an argument for `dangerously_skip_verify`.
+    /// to establish a TLS stream to the Faktory server.
     ///
     /// Note that if you use this method on the builder, but eventually use [`WorkerBuilder::connect_with`]
     /// (rather than [`WorkerBuilder::connect`]) to create an instance of [`Worker`], this worker
@@ -289,14 +279,6 @@ impl<E: 'static> WorkerBuilder<E> {
         self
     }
 
-    /// Do not verify the server certificate.
-    #[cfg(any(feature = "native_tls", feature = "rustls"))]
-    #[cfg_attr(docsrs, doc(cfg(any(feature = "native_tls", feature = "rustls"))))]
-    pub fn dangerously_without_cert_verification(mut self) -> Self {
-        self.verify_server_cert = false;
-        self
-    }
-
     /// Connect to a Faktory server with a non-standard stream.
     ///
     /// Iternally, the `stream` will be buffered. In case you've got a `stream` that is _already_
@@ -360,21 +342,12 @@ impl<E: 'static> WorkerBuilder<E> {
             }
             #[cfg(feature = "rustls")]
             TlsKind::Rust => {
-                let stream = crate::rustls::TlsStream::connect_with_native_certs(
-                    url,
-                    !self.verify_server_cert,
-                )
-                .await?;
+                let stream = crate::rustls::TlsStream::connect_with_native_certs(url).await?;
                 self.connect_with(stream, password).await
             }
             #[cfg(feature = "native_tls")]
             TlsKind::Native => {
-                let stream = if self.verify_server_cert {
-                    crate::native_tls::TlsStream::connect(url).await?
-                } else {
-                    crate::native_tls::TlsStream::connect_dangerously_skipping_verification(url)
-                        .await?
-                };
+                let stream = crate::native_tls::TlsStream::connect(url).await?;
                 self.connect_with(stream, password).await
             }
         }

diff --git a/tests/tls/native_tls.rs b/tests/tls/native_tls.rs
@@ -61,39 +61,6 @@ async fn roundtrip_tls() {
     assert_eq!(job.args(), &[Value::from("z")]);
 }
 
-#[tokio::test(flavor = "multi_thread")]
-async fn roundtrip_tls_with_worker_builder() {
-    if env::var_os("FAKTORY_URL_SECURE").is_none() {
-        return;
-    }
-
-    let local = "roundtrip_tls_with_worker_builder";
-    let (tx, rx) = sync::mpsc::channel();
-
-    let mut worker = Worker::builder()
-        .register(local, fixtures::JobHandler::new(tx))
-        .with_native_tls()
-        .dangerously_without_cert_verification()
-        .connect(Some(&env::var("FAKTORY_URL_SECURE").unwrap()))
-        .await
-        .unwrap();
-
-    // "one-shot" producer
-    Client::connect(Some(&env::var("FAKTORY_URL").unwrap()))
-        .await
-        .unwrap()
-        .enqueue(Job::new(local, vec!["z"]).on_queue(local))
-        .await
-        .unwrap();
-
-    worker.run_one(0, &[local]).await.unwrap();
-
-    let job = rx.recv().unwrap();
-    assert_eq!(job.queue, local);
-    assert_eq!(job.kind(), local);
-    assert_eq!(job.args(), &[Value::from("z")]);
-}
-
 mod fixtures {
     pub use handler::JobHandler;
 

diff --git a/tests/tls/rustls.rs b/tests/tls/rustls.rs
@@ -1,5 +1,5 @@
 use faktory::rustls::TlsStream;
-use faktory::{Client, Job, Worker, WorkerBuilder, WorkerId};
+use faktory::{Client, Job, Worker, WorkerId};
 use serde_json::Value;
 use std::{
     env,
@@ -52,7 +52,7 @@ async fn roundtrip_tls() {
         .password()
         .map(|p| p.to_string());
 
-    let mut worker = WorkerBuilder::default()
+    let mut worker = Worker::builder()
         .hostname("tester".to_string())
         .wid(WorkerId::new(local))
         .register(local, fixtures::JobHandler::new(tx))
@@ -76,39 +76,6 @@ async fn roundtrip_tls() {
     assert_eq!(job.args(), &[Value::from("z")]);
 }
 
-#[tokio::test(flavor = "multi_thread")]
-async fn roundtrip_tls_with_worker_builder() {
-    if env::var_os("FAKTORY_URL_SECURE").is_none() {
-        return;
-    }
-
-    let local = "roundtrip_tls_with_worker_builder";
-    let (tx, rx) = sync::mpsc::channel();
-
-    let mut worker = Worker::builder()
-        .register(local, fixtures::JobHandler::new(tx))
-        .with_rustls()
-        .dangerously_without_cert_verification()
-        .connect(Some(&env::var("FAKTORY_URL_SECURE").unwrap()))
-        .await
-        .unwrap();
-
-    // "one-shot" producer
-    Client::connect(Some(&env::var("FAKTORY_URL").unwrap()))
-        .await
-        .unwrap()
-        .enqueue(Job::new(local, vec!["z"]).on_queue(local))
-        .await
-        .unwrap();
-
-    worker.run_one(0, &[local]).await.unwrap();
-
-    let job = rx.recv().unwrap();
-    assert_eq!(job.queue, local);
-    assert_eq!(job.kind(), local);
-    assert_eq!(job.args(), &[Value::from("z")]);
-}
-
 mod fixtures {
     pub use handler::JobHandler;
     pub use tls::TestServerCertVerifier;