snapview / tokio-tungstenite

Future-based Tungstenite for Tokio. Lightweight stream-based WebSocket implementation
MIT License
1.88k stars 236 forks source link

Public `encryption` module or separate crate for encrypting TcpStream #285

Open twitu opened 1 year ago

twitu commented 1 year ago

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 -

Is this something you can consider?

daniel-abramov commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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! :pray: