From 2c9435594bbb94073da871fb6df6915117d0e038 Mon Sep 17 00:00:00 2001 From: Pavel Mikhalkevich Date: Sun, 18 Aug 2024 14:14:16 +0400 Subject: [PATCH] Rm 'verify_server_cert' option from WorkerBuilder --- Cargo.toml | 1 - src/tls/native_tls.rs | 18 ---------- src/tls/rustls.rs | 77 ++++------------------------------------- src/worker/builder.rs | 35 +++---------------- tests/tls/native_tls.rs | 33 ------------------ tests/tls/rustls.rs | 37 ++------------------ 6 files changed, 13 insertions(+), 188 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ede25b43..db34e9d1 100644 --- 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 index a85a97fd..16bf3eae 100644 --- a/src/tls/native_tls.rs +++ b/src/tls/native_tls.rs @@ -62,24 +62,6 @@ impl TlsStream { .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 { - 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 index c130d9d1..3496669e 100644 --- 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 { /// 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 { + /// 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 { 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); - -impl ServerCertVerifier for NoCertVerification { - fn verify_server_cert( - &self, - _: &CertificateDer<'_>, - _: &[CertificateDer<'_>], - _: &ServerName<'_>, - _: &[u8], - _: UnixTime, - ) -> Result { - Ok(ServerCertVerified::assertion()) - } - - fn verify_tls12_signature( - &self, - message: &[u8], - cert: &rustls_pki_types::CertificateDer<'_>, - dss: &DigitallySignedStruct, - ) -> Result { - self.0.verify_tls12_signature(message, cert, dss) - } - - fn verify_tls13_signature( - &self, - message: &[u8], - cert: &CertificateDer<'_>, - dss: &DigitallySignedStruct, - ) -> Result { - self.0.verify_tls13_signature(message, cert, dss) - } - - fn supported_verify_schemes(&self) -> Vec { - self.0.supported_verify_schemes() - } -} - #[async_trait::async_trait] impl Reconnect for BufStream> { async fn reconnect(&mut self) -> io::Result { diff --git a/src/worker/builder.rs b/src/worker/builder.rs index 10d616e2..a947eb03 100644 --- a/src/worker/builder.rs +++ b/src/worker/builder.rs @@ -32,9 +32,6 @@ pub struct WorkerBuilder { shutdown_timeout: Option, shutdown_signal: Option, 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 Default for WorkerBuilder { @@ -56,9 +53,6 @@ impl Default for WorkerBuilder { 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 WorkerBuilder { /// _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 WorkerBuilder { /// 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 WorkerBuilder { 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 WorkerBuilder { } #[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 index 54af2a58..c50ebb8d 100644 --- 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 index e7e9b4c6..41f605a8 100644 --- 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;