Public encryption module or separate crate for encrypting TcpStream

[twitu]

I've been using this fantastic crate for a websocket client. However I also need a raw Tcp client that can be encrypted with Tls if needed. I noticed that that the encryption implementation in this crate is fantastic and covers all the typical corner cases that make Tls difficult. However the encryption module is not public, which makes it impossible to reuse the logic.

Specifically the below function is very general and specifically if there could be a way to reuse the logic before the last line. It'll be a big help to anybody wanting to work with encrypted TcpStreams. It's almost like there's a mini library waiting to be unburied.

// not public
mod encryption {
   // various tls implementations
}

pub async fn client_async_tls_with_config<R, S>(
    request: R,
    stream: S,
    config: Option<WebSocketConfig>,
    connector: Option<Connector>,
) -> Result<(WebSocketStream<MaybeTlsStream<S>>, Response), Error>
where
    R: IntoClientRequest + Unpin,
    S: 'static + AsyncRead + AsyncWrite + Send + Unpin,
    MaybeTlsStream<S>: Unpin,
{
   // implementation details
   // encrypt tcp stream with tls if required
   // REQUEST: This logic can be made into a separate public function
   // This function will return an encrypted TcpStream
   ....

    // wrap tcp stream into a websocket
    client_async_with_config(request, stream, config).await
}

Here are a few ideas in order of effort -

• make encryption a public module (least effort)
• Refactor a couple of functions so they can be re-used just with TcpStreams. For e.g. in connect this portion could be factored out into separate function (moderate effort)

      let domain = domain(&request)?;
   let port = request
       .uri()
       .port_u16()
       .or_else(|| match request.uri().scheme_str() {
           Some("wss") => Some(443),
           Some("ws") => Some(80),
           _ => None,
       })
       .ok_or(Error::Url(UrlError::UnsupportedUrlScheme))?;
  
   let addr = format!("{domain}:{port}");
   let socket = TcpStream::connect(addr).await.map_err(Error::Io)?;
  
   if disable_nagle {
       socket.set_nodelay(true)?;
   }

• Factor out separate crate that contains all the Tcp encryption logic (high effort)

Is this something you can consider?

[daniel-abramov]

If it is a common case that is also used in other libraries, then making a separate crate might make sense!

Initially, we added TLS as dependencies just because it's a very common case in an actual production code, so to spare a common boilerplate, it made its way into a library. However, if the code is generic enough (or could be made generic), it could be extracted into a separate crate (that's even better for us, less code to deal with non-WS spec 🙂).

That being said, I have not checked if someone has created a similar crate already and/or if it's really generic enough to be used in other use cases.

[agalakhov]

This totally makes sense. This logic is very generic and the only reason why it was implemented in such a way, we needed some encryption at the very beginning and had to deal with it. Now it's time to refactor.

[twitu]

It's great to hear such a positive response. Happy to help in anyway for now I'm using a fork of this repo with some modifications to access the encryption logic.

[twitu]

I made it work for my use case with a few minor changes. Here's a patch for the changes. The main difference is creating a tcp_tls function that just wraps the underlying stream with MaybeTLS. This function is used in client_async_tls_with_config to get the wrapped stream and make it into a websocket stream but it can also be used independently, just to wrap a tcp stream with tls.

diff --git a/nautilus_core/network/tokio-tungstenite/src/tls.rs b/nautilus_core/network/tokio-tungstenite/src/tls.rs
index c9015a65e..5653f2654 100644
--- a/nautilus_core/network/tokio-tungstenite/src/tls.rs
+++ b/nautilus_core/network/tokio-tungstenite/src/tls.rs
@@ -2,7 +2,11 @@
 use tokio::io::{AsyncRead, AsyncWrite};
 
 use tungstenite::{
-    client::uri_mode, error::Error, handshake::client::Response, protocol::WebSocketConfig,
+    client::uri_mode,
+    error::Error,
+    handshake::client::{Request, Response},
+    protocol::WebSocketConfig,
+    stream::Mode,
 };
 
 use crate::{client_async_with_config, IntoClientRequest, WebSocketStream};
@@ -25,7 +29,9 @@ pub enum Connector {
     Rustls(std::sync::Arc<rustls::ClientConfig>),
 }
 
-mod encryption {
+/// Encrypt a stream usin Tls
+pub mod encryption {
+    /// Use native-tls implementaiton to encrypt
     #[cfg(feature = "native-tls")]
     pub mod native_tls {
         use native_tls_crate::TlsConnector;
@@ -174,31 +185,21 @@ where
     client_async_tls_with_config(request, stream, None, None).await
 }
 
-/// The same as `client_async_tls()` but the one can specify a websocket configuration,
-/// and an optional connector. If no connector is specified, a default one will
-/// be created.
-///
-/// Please refer to `client_async_tls()` for more details.
-pub async fn client_async_tls_with_config<R, S>(
-    request: R,
+/// Given a domain and mode
+pub async fn tcp_tls<S>(
+    request: &Request,
+    mode: Mode,
     stream: S,
-    config: Option<WebSocketConfig>,
     connector: Option<Connector>,
-) -> Result<(WebSocketStream<MaybeTlsStream<S>>, Response), Error>
+) -> Result<MaybeTlsStream<S>, Error>
 where
-    R: IntoClientRequest + Unpin,
     S: 'static + AsyncRead + AsyncWrite + Send + Unpin,
     MaybeTlsStream<S>: Unpin,
 {
-    let request = request.into_client_request()?;
-
     #[cfg(any(feature = "native-tls", feature = "__rustls-tls"))]
-    let domain = crate::domain(&request)?;
-
-    // Make sure we check domain and mode first. URL must be valid.
-    let mode = uri_mode(request.uri())?;
+    let domain = crate::domain(request)?;
 
-    let stream = match connector {
+    match connector {
         Some(conn) => match conn {
             #[cfg(feature = "native-tls")]
             Connector::NativeTls(conn) => {
@@ -224,7 +225,30 @@ where
                 self::encryption::plain::wrap_stream(stream, mode).await
             }
         }
-    }?;
+    }
+}
+
+/// The same as `client_async_tls()` but the one can specify a websocket configuration,
+/// and an optional connector. If no connector is specified, a default one will
+/// be created.
+///
+/// Please refer to `client_async_tls()` for more details.
+pub async fn client_async_tls_with_config<R, S>(
+    request: R,
+    stream: S,
+    config: Option<WebSocketConfig>,
+    connector: Option<Connector>,
+) -> Result<(WebSocketStream<MaybeTlsStream<S>>, Response), Error>
+where
+    R: IntoClientRequest + Unpin,
+    S: 'static + AsyncRead + AsyncWrite + Send + Unpin,
+    MaybeTlsStream<S>: Unpin,
+{
+    let request = request.into_client_request()?;
+
+    // Make sure we check domain and mode first. URL must be valid.
+    let mode = uri_mode(request.uri())?;
 
+    let stream = tcp_tls(&request, mode, stream, connector).await?;
     client_async_with_config(request, stream, config).await
 }

[cjdsellers]

Just checking in on this issue.

I think @twitu makes a great suggestion, I had a go at extracting the necessary pieces into a separate encryption library and its fairly close to being possible save for some private access (such as the WebSocketStream new function).

Would this proposal be considered at all on the development roadmap for this year?

[daniel-abramov]

Would this proposal be considered at all on the development roadmap for this year?

To be fully transparent, there is no clearly defined development roadmap for this year as the development of this library is stalled (I seem to be the only "active" maintainer at the moment judging by GitHub stats from 2017-2023; and most of the greatest contributions in the recent years have been done by the awesome community). So if someone creates a PR, I'd be glad to carefully review and verify it to the best of my knowledge, but there is no roadmap for the changes.

As for the "roadmap" - if there is anything I'd personally put on a roadmap for myself to tackle, I'd say it would be addressing the concerns listed in this comment.

[cjdsellers]

Hi @daniel-abramov

This all makes sense, and I appreciate the transparency. Most likely I won't have bandwidth myself to make such a PR - as is often the way with open source. I appreciate your ongoing work as project maintainer though, many thanks! 🙏