refactor: clarify features exposed by ironrdp-tls (#325)

crates/ironrdp-tls/Cargo.toml

@@ -16,12 +16,13 @@ doctest = false
 test = false
 
 [features]
-default = ["rustls"]
-rustls = ["dep:tokio-rustls"]
-native-tls = ["dep:tokio-native-tls"]
+default = [] # No default feature, the user must choose a TLS backend by enabling the appropriate feature.
+rustls = ["dep:tokio-rustls", "dep:x509-cert", "tokio/io-util"]
+native-tls = ["dep:tokio-native-tls", "dep:x509-cert", "tokio/io-util"]
+stub = []
 
 [dependencies]
-x509-cert = { version = "0.2", default-features = false, features = ["std"] }
-tokio = { version = "1.34", features = ["io-util"] }
+tokio = { version = "1.34" }
+x509-cert = { version = "0.2", default-features = false, features = ["std"], optional = true }
 tokio-native-tls = { version = "0.3", optional = true }
 tokio-rustls =  { version = "0.24", features = ["dangerous_configuration"], optional = true }

crates/ironrdp-tls/README.md

@@ -1,3 +1,59 @@
 # IronRDP TLS
 
 TLS boilerplate common with most IronRDP clients.
+
+This crate exposes three features for selecting the TLS backend:
+
+- `rustls`: use the rustls crate.
+- `native-tls`: use the native-tls crate.
+- `stub`: use a stubbed backend which fail at runtime when used.
+
+These features are mutually exclusive and only one may be enabled at a time.
+When more than one backend is enabled, a compile-time error is emitted.
+For this reason, no feature is enabled by default.
+
+The rationale is two-fold:
+
+- It makes deliberate the choice of the TLS backend.
+- It eliminates the risk of mistakenly enabling multiple backends at once.
+
+With this approach, it’s obvious which backend is enabled when looking at the dependency declaration:
+
+```toml
+# This:
+ironrdp-tls = { version = "x.y.z", features = ["rustls"] }
+
+# Instead of:
+ironrdp-tls = "x.y.z"
+```
+
+There is also no default feature to disable:
+
+```toml
+# This:
+ironrdp-tls = { version = "x.y.z", features = ["native-tls"] }
+
+# Instead of:
+ironrdp-tls = { version = "x.y.z", default-features = false, features = ["native-tls"] }
+```
+
+This is typically more convenient and less error-prone when re-exposing the features from another crate.
+
+```toml
+[features]
+rustls = ["ironrdp-tls/rustls"]
+native-tls = ["ironrdp-tls/native-tls"]
+stub-tls = ["ironrdp-tls/stub"]
+
+# This:
+[dependencies]
+ironrdp-tls = "x.y.z"
+
+# Instead of:
+[dependencies]
+ironrdp-tls = { version = "x.y.z", default-features = false }
+```
+
+(This is worse when the crate is exposing other default features which are typically not disabled by default.)
+
+The stubbed backend is provided as an easy way to make the code compiles with minimal dependencies if required.

crates/ironrdp-tls/src/lib.rs

@@ -1,86 +1,33 @@
-use std::io;
-
-use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt as _};
+#![doc = include_str!("../README.md")]
 
 #[cfg(feature = "rustls")]
-pub type TlsStream<S> = tokio_rustls::client::TlsStream<S>;
-
-#[cfg(all(feature = "native-tls", not(feature = "rustls")))]
-pub type TlsStream<S> = tokio_native_tls::TlsStream<S>;
-
-pub async fn upgrade<S>(stream: S, server_name: &str) -> io::Result<(TlsStream<S>, Vec<u8>)>
-where
-    S: Unpin + AsyncRead + AsyncWrite,
-{
-    #[cfg(feature = "rustls")]
-    let mut tls_stream = {
-        let mut config = tokio_rustls::rustls::client::ClientConfig::builder()
-            .with_safe_defaults()
-            .with_custom_certificate_verifier(std::sync::Arc::new(danger::NoCertificateVerification))
-            .with_no_client_auth();
-
-        // This adds support for the SSLKEYLOGFILE env variable (https://wiki.wireshark.org/TLS#using-the-pre-master-secret)
-        config.key_log = std::sync::Arc::new(tokio_rustls::rustls::KeyLogFile::new());
-
-        // Disable TLS resumption because it’s not supported by some services such as CredSSP.
-        //
-        // > The CredSSP Protocol does not extend the TLS wire protocol. TLS session resumption is not supported.
-        //
-        // source: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-cssp/385a7489-d46b-464c-b224-f7340e308a5c
-        config.resumption = tokio_rustls::rustls::client::Resumption::disabled();
-
-        let config = std::sync::Arc::new(config);
-
-        let server_name = server_name.try_into().unwrap();
-
-        tokio_rustls::TlsConnector::from(config)
-            .connect(server_name, stream)
-            .await?
-    };
+#[path = "rustls.rs"]
+mod impl_;
 
-    #[cfg(all(feature = "native-tls", not(feature = "rustls")))]
-    let mut tls_stream = {
-        let connector = tokio_native_tls::native_tls::TlsConnector::builder()
-            .danger_accept_invalid_certs(true)
-            .use_sni(false)
-            .build()
-            .map(tokio_native_tls::TlsConnector::from)
-            .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
+#[cfg(feature = "native-tls")]
+#[path = "native_tls.rs"]
+mod impl_;
 
-        connector
-            .connect(server_name, stream)
-            .await
-            .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?
-    };
+#[cfg(feature = "stub")]
+#[path = "stub.rs"]
+mod impl_;
 
-    tls_stream.flush().await?;
+#[cfg(any(
+    not(any(feature = "stub", feature = "native-tls", feature = "rustls")),
+    all(feature = "stub", feature = "native-tls"),
+    all(feature = "stub", feature = "rustls"),
+    all(feature = "rustls", feature = "native-tls"),
+))]
+compile_error!("a TLS backend must be selected by enabling a single feature out of: `rustls`, `native-tls`, `stub`");
 
-    #[cfg(feature = "rustls")]
-    let server_public_key = {
-        let cert = tls_stream
-            .get_ref()
-            .1
-            .peer_certificates()
-            .and_then(|certificates| certificates.first())
-            .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "peer certificate is missing"))?;
-        extract_tls_server_public_key(&cert.0)?
-    };
+// The whole public API of this crate.
+#[cfg(any(feature = "stub", feature = "native-tls", feature = "rustls"))]
+pub use impl_::{upgrade, TlsStream};
 
-    #[cfg(all(feature = "native-tls", not(feature = "rustls")))]
-    let server_public_key = {
-        let cert = tls_stream
-            .get_ref()
-            .peer_certificate()
-            .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?
-            .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "peer certificate is missing"))?;
-        let cert = cert.to_der().map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
-        extract_tls_server_public_key(&cert)?
-    };
+#[cfg(any(feature = "native-tls", feature = "rustls"))]
+pub(crate) fn extract_tls_server_public_key(cert: &[u8]) -> std::io::Result<Vec<u8>> {
+    use std::io;
 
-    Ok((tls_stream, server_public_key))
-}
-
-fn extract_tls_server_public_key(cert: &[u8]) -> io::Result<Vec<u8>> {
     use x509_cert::der::Decode as _;
 
     let cert = x509_cert::Certificate::from_der(cert).map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
@@ -95,27 +42,3 @@ fn extract_tls_server_public_key(cert: &[u8]) -> io::Result<Vec<u8>> {
 
     Ok(server_public_key)
 }
-
-#[cfg(feature = "rustls")]
-mod danger {
-    use std::time::SystemTime;
-
-    use tokio_rustls::rustls::client::ServerCertVerified;
-    use tokio_rustls::rustls::{Certificate, Error, ServerName};
-
-    pub(super) struct NoCertificateVerification;
-
-    impl tokio_rustls::rustls::client::ServerCertVerifier for NoCertificateVerification {
-        fn verify_server_cert(
-            &self,
-            _end_entity: &Certificate,
-            _intermediates: &[Certificate],
-            _server_name: &ServerName,
-            _scts: &mut dyn Iterator<Item = &[u8]>,
-            _ocsp_response: &[u8],
-            _now: SystemTime,
-        ) -> Result<ServerCertVerified, Error> {
-            Ok(tokio_rustls::rustls::client::ServerCertVerified::assertion())
-        }
-    }
-}

crates/ironrdp-tls/src/native_tls.rs

@@ -0,0 +1,38 @@
+use std::io;
+
+use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt as _};
+
+pub type TlsStream<S> = tokio_native_tls::TlsStream<S>;
+
+pub async fn upgrade<S>(stream: S, server_name: &str) -> io::Result<(TlsStream<S>, Vec<u8>)>
+where
+    S: Unpin + AsyncRead + AsyncWrite,
+{
+    let mut tls_stream = {
+        let connector = tokio_native_tls::native_tls::TlsConnector::builder()
+            .danger_accept_invalid_certs(true)
+            .use_sni(false)
+            .build()
+            .map(tokio_native_tls::TlsConnector::from)
+            .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
+
+        connector
+            .connect(server_name, stream)
+            .await
+            .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?
+    };
+
+    tls_stream.flush().await?;
+
+    let server_public_key = {
+        let cert = tls_stream
+            .get_ref()
+            .peer_certificate()
+            .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?
+            .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "peer certificate is missing"))?;
+        let cert = cert.to_der().map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
+        crate::extract_tls_server_public_key(&cert)?
+    };
+
+    Ok((tls_stream, server_public_key))
+}

crates/ironrdp-tls/src/rustls.rs

@@ -0,0 +1,72 @@
+use std::io;
+
+use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt as _};
+
+pub type TlsStream<S> = tokio_rustls::client::TlsStream<S>;
+
+pub async fn upgrade<S>(stream: S, server_name: &str) -> io::Result<(TlsStream<S>, Vec<u8>)>
+where
+    S: Unpin + AsyncRead + AsyncWrite,
+{
+    let mut tls_stream = {
+        let mut config = tokio_rustls::rustls::client::ClientConfig::builder()
+            .with_safe_defaults()
+            .with_custom_certificate_verifier(std::sync::Arc::new(danger::NoCertificateVerification))
+            .with_no_client_auth();
+
+        // This adds support for the SSLKEYLOGFILE env variable (https://wiki.wireshark.org/TLS#using-the-pre-master-secret)
+        config.key_log = std::sync::Arc::new(tokio_rustls::rustls::KeyLogFile::new());
+
+        // Disable TLS resumption because it’s not supported by some services such as CredSSP.
+        //
+        // > The CredSSP Protocol does not extend the TLS wire protocol. TLS session resumption is not supported.
+        //
+        // source: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-cssp/385a7489-d46b-464c-b224-f7340e308a5c
+        config.resumption = tokio_rustls::rustls::client::Resumption::disabled();
+
+        let config = std::sync::Arc::new(config);
+
+        let server_name = server_name.try_into().unwrap();
+
+        tokio_rustls::TlsConnector::from(config)
+            .connect(server_name, stream)
+            .await?
+    };
+
+    tls_stream.flush().await?;
+
+    let server_public_key = {
+        let cert = tls_stream
+            .get_ref()
+            .1
+            .peer_certificates()
+            .and_then(|certificates| certificates.first())
+            .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "peer certificate is missing"))?;
+        crate::extract_tls_server_public_key(&cert.0)?
+    };
+
+    Ok((tls_stream, server_public_key))
+}
+
+mod danger {
+    use std::time::SystemTime;
+
+    use tokio_rustls::rustls::client::ServerCertVerified;
+    use tokio_rustls::rustls::{Certificate, Error, ServerName};
+
+    pub(super) struct NoCertificateVerification;
+
+    impl tokio_rustls::rustls::client::ServerCertVerifier for NoCertificateVerification {
+        fn verify_server_cert(
+            &self,
+            _end_entity: &Certificate,
+            _intermediates: &[Certificate],
+            _server_name: &ServerName,
+            _scts: &mut dyn Iterator<Item = &[u8]>,
+            _ocsp_response: &[u8],
+            _now: SystemTime,
+        ) -> Result<ServerCertVerified, Error> {
+            Ok(tokio_rustls::rustls::client::ServerCertVerified::assertion())
+        }
+    }
+}

crates/ironrdp-tls/src/stub.rs

@@ -0,0 +1,39 @@
+use std::io;
+use std::marker::PhantomData;
+use std::task::{Context, Poll};
+
+use tokio::io::{AsyncRead, AsyncWrite, ReadBuf};
+
+#[derive(Debug)]
+pub struct TlsStream<S> {
+    _marker: PhantomData<S>,
+}
+
+impl<S> AsyncRead for TlsStream<S> {
+    fn poll_read(self: std::pin::Pin<&mut Self>, _: &mut Context<'_>, _: &mut ReadBuf<'_>) -> Poll<io::Result<()>> {
+        Poll::Ready(Ok(()))
+    }
+}
+
+impl<S> AsyncWrite for TlsStream<S> {
+    fn poll_write(self: std::pin::Pin<&mut Self>, _: &mut Context<'_>, _: &[u8]) -> Poll<Result<usize, io::Error>> {
+        Poll::Ready(Ok(0))
+    }
+
+    fn poll_flush(self: std::pin::Pin<&mut Self>, _: &mut Context<'_>) -> Poll<Result<(), io::Error>> {
+        Poll::Ready(Ok(()))
+    }
+
+    fn poll_shutdown(self: std::pin::Pin<&mut Self>, _: &mut Context<'_>) -> Poll<Result<(), io::Error>> {
+        Poll::Ready(Ok(()))
+    }
+}
+
+pub async fn upgrade<S>(stream: S, server_name: &str) -> io::Result<(TlsStream<S>, Vec<u8>)>
+where
+    S: Unpin + AsyncRead + AsyncWrite,
+{
+    // Do nothing and fail
+    let _ = (stream, server_name);
+    Err(io::Error::other("no TLS backend enabled for this build"))
+}