Support both rustls and native-tls #7

jacob-pro · 2022-02-20T17:55:46Z

Following up on #6

jacob-pro · 2022-02-20T17:56:48Z

src/lib.rs

+mod compile_time_checks {
+    #[cfg(not(any(feature = "rustls", feature = "native-tls")))]
+    compile_error!("tls-listener requires either the `rustls` or `native-tls` feature");
+
+    #[cfg(all(feature = "rustls", feature = "native-tls"))]
+    compile_error!("The `rustls` and `native-tls` features in tls-listener are mutually exclusive");
+}


These produce nice readable errors in the case that a user hasn't got their features setup right

jacob-pro · 2022-02-20T17:57:55Z

src/lib.rs

@@ -78,23 +92,39 @@ pin_project! {
 /// Builder for `TlsListener`.
 #[derive(Clone)]
 pub struct Builder {
-    server_config: Arc<ServerConfig>,
+    acceptor: TlsAcceptor,


Unfortunately native_tls doesn't have an infallible equivalent to ServerConfig, so for simplicity and consistency it made sense just to make the user provide a TlsAcceptor in both instances

It might be worth have a from_config function on the builder that accepts ServerConfig if the rustls option is active.

Although ServerConfig in rustls is somewhat equivalent to native_tls::TlsAcceptor, and it might make sense to keep the rustls behavior the same, and have the native-tls feature take a native_tls::TlsAcceptor instead of a tokio_native_tls::TlsAcceptor.

You're probably already aware, but changing this breaks backwards compatibility.

I have changed the functions to accept T: Into<TlsAcceptor>, such that a user can pass in any of native_tls::TlsAcceptor or tokio_native_tls::TlsAcceptor, tokio_rustls::TlsAcceptor or Arc<ServerConfig>.

It remains a very minor breaking change but it is documented in the CHANGELOG now.

jacob-pro · 2022-02-20T18:01:25Z

src/lib.rs

+/// Wraps errors from either the listener or the TLS Acceptor
+#[derive(Debug, Error)]
+pub enum Error<A: std::error::Error> {
+    /// An error that arose from the listener ([AsyncAccept::Error])
+    #[error("{0}")]
+    ListenerError(#[source] A),
+    /// An error that occurred during the TLS accept handshake
+    #[cfg(feature = "rustls")]
+    #[error("{0}")]
+    TlsAcceptError(#[source] std::io::Error),
+    /// An error that occurred during the TLS accept handshake
+    #[cfg(feature = "native-tls")]
+    #[error("{0}")]
+    TlsAcceptError(#[source] tokio_native_tls::native_tls::Error),
+}
+


Before we were relying on the fact that all implementations of AsyncAccept happened to return io::Error, and also that the TLS accept future also returned an io::Error, but we can't do that anymore.

This also gets rid of the slightly odd downcasting from below and replaces the error with a type parameter

jacob-pro · 2022-02-20T18:02:46Z

Cargo.toml

 [features]
 default = ["tokio-net"]
+rustls = ["tokio-rustls"]
+native-tls = ["tokio-native-tls"]


Let me know if you would like rustls to be in the defaults instead? I think this way is more consistent with the rust ecosystem from what I have seen of other libraries

tmccombs

There are several backwards incompatible changes here. I'm not entirely opposed to them, especially since as far as I know this crate isn't particularly widely used, but I think it is at least worth documenting them in the README, or CHANGELOG (which doesn't exist yet).

The backwards incompatible changes I was able to find are:

Changing the signature of TlsListener::new and builder to take tokio_rustls::TlsAcceptor instead of rustls::ServerConfig.
Changing the error type
Adding Send + Sync + 'static bound on the connection type (I'd like to avoid this if possible for rustls at least)

tmccombs · 2022-02-21T03:23:41Z

src/lib.rs

    }
 }

 impl<A> TlsListener<A>
 where
    A: AsyncAccept,
-    A::Connection: AsyncRead + AsyncWrite + Unpin,
-    A::Error: Into<Box<dyn Error + Send + Sync>>,
+    A::Connection: AsyncRead + AsyncWrite + Unpin + Sync + Send + 'static,


Are the Sync, Send, and 'static bounds required for tls-native? Could we only add those bounds if the tls-native feature is enabled?

I couldn't find a super easy way to do that, but I'd also prefer not to have to add these bounds if we don't need to. And adding these bounds is backwards incompatible.

The Send + Sync is my mistake, those are not actually needed, so I have removed them.
I will do some further investigating around the 'static constraint which is currently needed for the native-tls to compile:

error[E0310]: the associated type `<A as AsyncAccept>::Connection` may not live long enough --> src\lib.rs:161:29 | 161 | ... Box::pin(async move { tls.accept(conn).await }), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: consider adding an explicit lifetime bound `<A as AsyncAccept>::Connection: 'static`... = note: ...so that the type `impl futures_util::Future<Output = [async output]>` will meet its required lifetime bounds For more information about this error, try `rustc --explain E0310`.

tmccombs · 2022-02-22T07:58:26Z

src/lib.rs

@@ -78,23 +92,39 @@ pin_project! {
 /// Builder for `TlsListener`.
 #[derive(Clone)]
 pub struct Builder {
-    server_config: Arc<ServerConfig>,
+    acceptor: TlsAcceptor,


It might be worth have a from_config function on the builder that accepts ServerConfig if the rustls option is active.

Although ServerConfig in rustls is somewhat equivalent to native_tls::TlsAcceptor, and it might make sense to keep the rustls behavior the same, and have the native-tls feature take a native_tls::TlsAcceptor instead of a tokio_native_tls::TlsAcceptor.

You're probably already aware, but changing this breaks backwards compatibility.

jacob-pro · 2022-02-22T18:26:40Z

Cargo.toml

-features = ["server"]
-optional = true
+thiserror = "1.0.30"
+tokio = { version = "1.0", features = ["time"] }


There was a bug here such that if you try to use this crate in another project that doesn't itself also import tokio/time, then it will fail to compile.
You wouldn't have noticed this in the tests because of the weird way Cargo works, where it combines all the feature flag from different targets (i.e. the library + dev dependencies in this case) (something like this rust-lang/cargo#3620 )

tmccombs · 2022-02-23T09:01:14Z

I built on this to create this branch which uses generics so that it can support both rustls and native-tls features simultaneously. @jacob-pro I'd be curious to know what you think of that.

jacob-pro · 2022-02-23T14:13:52Z

Thanks @tmccombs that looks really good. To be honest I don't hugely mind how it is implemented, I thought I would have ago at doing it with conditional compilation just because that is how I had seen other crates do it, but the generics does actually allow us to get around those trait bounds nicely.

If you are happy to merge your branch - feel free to either ammend or close this PR

Support both rustls and native-tls

30d34cb

jacob-pro commented Feb 20, 2022

View reviewed changes

tmccombs requested changes Feb 22, 2022

View reviewed changes

Tweaks based on PR feedback

2656771

jacob-pro commented Feb 22, 2022

View reviewed changes

tmccombs merged commit 2656771 into tmccombs:main Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support both rustls and native-tls #7

Support both rustls and native-tls #7

jacob-pro commented Feb 20, 2022

jacob-pro Feb 20, 2022

jacob-pro Feb 20, 2022

tmccombs Feb 22, 2022

jacob-pro Feb 22, 2022

jacob-pro Feb 20, 2022

jacob-pro Feb 20, 2022 •

edited

Loading

tmccombs left a comment

tmccombs Feb 21, 2022

jacob-pro Feb 22, 2022

tmccombs Feb 22, 2022

jacob-pro Feb 22, 2022

tmccombs commented Feb 23, 2022

jacob-pro commented Feb 23, 2022

Support both rustls and native-tls #7

Support both rustls and native-tls #7

Conversation

jacob-pro commented Feb 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-pro Feb 20, 2022 • edited Loading

Choose a reason for hiding this comment

tmccombs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmccombs commented Feb 23, 2022

jacob-pro commented Feb 23, 2022

jacob-pro Feb 20, 2022 •

edited

Loading