Add support for rustls

[travispaul]

Building off the work started in /pull/212

[bradfier]

As a first pass I've noted a few bits of low hanging fruit in the PR.

As-is, this branch interleaves a bunch of commits that are already present in master, so I've gone ahead and fixed/rebased that in dev-rustls if you'd like to reset this PR onto that.

Aside from the stuff I've explicitly called out above, there are two more things I'd like to make sure we get right here:

• First is how we handle the XOR behaviour of the two SSL features. Cargo features are supposed to be additive, this could present problems because we are a library crate rather than a whole web framework, so we could feasibly get pulled in by two different dependents, each of which enable a different SSL feature. I'll look into how we make enabling both features a nice transparent compile-time error rather than exploding with a double definition.
• OpenSSL's handling of multithreaded contexts is arcane, to put it politely, and as such the Rust crate errs on the side of caution by marking SslStream as not thread safe. rustls::ServerConnection seems to implement Send + Sync, so if we can get away without the Mutex around the stream that might offer a permanent workaround for issues like Responding to secure requests gets stuck after some time #160 and Running ssl.rs example, no response in the browser. #167 (which appear to be caused by some browsers waiting for a response while they're concurrently streaming a request).

[bradfier]

@travispaul I've addressed a few of the points I mentioned above in 3e2ca09, including something I noticed while editing which is that the code as is would drop any extra certificates in a fullchain.pem type bundle.

[travispaul]

@bradfier thanks for review and updates, I've reset my branch and pulled in your changes.

First is how we handle the XOR behaviour of the two SSL features. Cargo features are supposed to be additive,

That is a good point, looking at the link do you think this approach is reasonable:

"When there is a conflict, choose one feature over another. The cfg-if package can help with writing more complex cfg expressions."

Possibly always falling back to OpenSSL since that has is currently what the library uses?

rustls::ServerConnection seems to implement Send + Sync, so if we can get away without the Mutex around the stream that might offer a permanent workaround for issues like #160 and #167 (which appear to be caused by some browsers waiting for a response while they're concurrently streaming a request).

Would that suggest removing for support for OpenSSL altogether? I've not observed that problem, I'll see if I can reproduce.

[bradfier]

"When there is a conflict, choose one feature over another. The cfg-if package can help with writing more complex cfg expressions."

This is one approach, I'd like to see what it looks like in practice.

I had hoped that something as simple as this would do.

diff --git a/src/lib.rs b/src/lib.rs
index 25c748f..75b3368 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -246,6 +246,8 @@ impl Server {
         };
 
         // building the SSL capabilities
+        #[cfg(all(feature = "ssl-openssl", feature = "ssl-rustls"))]
+        compile_error!("Features 'ssl-openssl' and 'ssl-rustls' must not be enabled at the same time");
         #[cfg(feature = "ssl-openssl")]
         type SslContext = openssl::ssl::SslContext;
         #[cfg(feature = "ssl-rustls")]

However when you try and compile with --all-features you still get a cacophony of unhelpful errors, with the useful one buried in the middle:

error[E0428]: the name `Https` is defined multiple times
  --> src/util/refined_tcp_stream.rs:21:5
   |
19 |     Https(Arc<Mutex<SslStream<TcpStream>>>),
   |     --------------------------------------- previous definition of the type `Https` here
20 |     #[cfg(feature = "ssl-rustls")]
21 |     Https(Arc<Mutex<rustls::StreamOwned<rustls::ServerConnection, TcpStream>>>),
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Https` redefined here
   |
   = note: `Https` must be defined only once in the type namespace of this enum

error[E0428]: the name `SslContext` is defined multiple times
   --> src/lib.rs:254:9
    |
252 |         type SslContext = openssl::ssl::SslContext;
    |         ------------------------------------------- previous definition of the type `SslContext` here
253 |         #[cfg(feature = "ssl-rustls")]
254 |         type SslContext = Arc<rustls::ServerConfig>;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `SslContext` redefined here
    |
    = note: `SslContext` must be defined only once in the type namespace of this block

error: Features 'ssl-openssl' and 'ssl-rustls' must not be enabled at the same time
   --> src/lib.rs:250:9
    |
250 |         compile_error!("Features 'ssl-openssl' and 'ssl-rustls' must not be enabled at the same time");
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0308]: mismatched types
  --> src/util/refined_tcp_stream.rs:43:43
   |
43 |         Stream::Https(Arc::new(Mutex::new(stream)))
   |                                           ^^^^^^ expected struct `SslStream`, found struct `StreamOwned`
   |
   = note: expected struct `SslStream<TcpStream>`
              found struct `StreamOwned<ServerConnection, TcpStream>`

error[E0609]: no field `sock` on type `MutexGuard<'_, SslStream<TcpStream>>`
  --> src/util/refined_tcp_stream.rs:93:69
   |
93 |             Stream::Https(ref mut stream) => stream.lock().unwrap().sock.peer_addr(),
   |                                                                     ^^^^ unknown field

error[E0609]: no field `sock` on type `MutexGuard<'_, SslStream<TcpStream>>`
   --> src/util/refined_tcp_stream.rs:107:73
    |
107 |                 Stream::Https(ref mut stream) => stream.lock().unwrap().sock.shutdown(Shutdown::Read).ok(),
    |                                                                         ^^^^ unknown field

error[E0609]: no field `sock` on type `MutexGuard<'_, SslStream<TcpStream>>`
   --> src/util/refined_tcp_stream.rs:118:73
    |
118 |                 Stream::Https(ref mut stream) => stream.lock().unwrap().sock.shutdown(Shutdown::Write).ok(),
    |                                                                         ^^^^ unknown field

error[E0308]: mismatched types
   --> src/lib.rs:325:22
    |
325 |                 Some(Arc::new(tls_conf))
    |                      ^^^^^^^^^^^^^^^^^^ expected struct `openssl::ssl::SslContext`, found struct `Arc`
    |
    = note: expected struct `openssl::ssl::SslContext`
               found struct `Arc<rustls::ServerConfig>`

error[E0308]: mismatched types
   --> src/lib.rs:369:73
    |
369 | ...                   match rustls::ServerConnection::new(tls_conf.clone()) {
    |                                                           ^^^^^^^^^^^^^^^^ expected struct `Arc`, found struct `openssl::ssl::SslContext`
    |
    = note: expected struct `Arc<rustls::ServerConfig>`
               found struct `openssl::ssl::SslContext`

Some errors have detailed explanations: E0308, E0428, E0609.
For more information about an error, try `rustc --explain E0308`.
error: could not compile `tiny_http` due to 9 previous errors

Ideally we could avoid this and emit just the warning about the mutually incompatible features, cfg-if might allow this.

We definitely shouldn't just fall back to OpenSSL as the library consumer might be relying on Rustls to only permit strong TLS methods, which OpenSSL won't do by default.

Would that suggest removing for support for OpenSSL altogether? I've not observed that problem, I'll see if I can reproduce.

Almost completely opposite to the above, I think it's important to retain OpenSSL support, as Rustls makes a big deal of not supporting "insecure" TLS modes, sadly in the real world enterprises still use them, so to remove OpenSSL support would actually be a huge breaking change.

[bradfier]

Thanks for the help here @travispaul, I've now merged Rustls support in a9823f2 🎉

Due to the refactor I've managed to get the error spam from enabling both features down to 3 errors, where the explicit warning is shown nice and early.

I was not able to find a way to avoid the use of a Mutex for the owned Rustls stream, but some future work could be done here to try and work around it.

I need to fix #222 for the OpenSSL implementation then I'll cut a 0.11 and Rustls support will be available!

[travispaul]

Thanks @bradfier and @3xmblzj5!
Just happy to help nudge things along!