server example error handling is incorrect + documentation is insufficient

[EverlastingBugstopper]

steps to reproduce:

first, verify the server example works:

$ cargo run --example server
Starting to serve on https://127.0.0.1:1337.

---

$ curl -X POST --data "POST-it note" https://127.0.0.1:1337/echo --insecure
POST-it note

---

so far so good! lets try curling with http to see if the server gracefully drops the packet

---

$ curl -X POST --data "POST-it note" http://127.0.0.1:1337/echo 
curl: (52) Empty reply from server

that part seems right, but wait! my server has crashed!

[!] Voluntary server halt due to client-connection error...
FAILED: error accepting connection: TLS Error: Custom { kind: InvalidData, error: CorruptMessage }

ok - lets look at the example code. it seems to say that i can uncomment the error and just return Ok(None) and that should take care of things: uncomment this line and comment this line so it looks like this:

---

let incoming_tls_stream = tcp
  .incoming()
  .map_err(|e| error(format!("Incoming failed: {:?}", e)))
  .and_then(move |s| {
    tls_acceptor.accept(s).map_err(|e| Ok(None))
  })
  .boxed();

---

unfortunately when i run the server it fails to compile with a rather verbose error message:

$ cargo run --example server
   Compiling hyper-rustls v0.21.0 (/Users/averyharnish/Documents/work/hyper-rustls)
error[E0271]: type mismatch resolving `<[closure@examples/server.rs:67:44: 72:14] as std::ops::FnOnce<(std::io::Error,)>>::Output == std::io::Error`
  --> examples/server.rs:66:10
   |
66 |         .and_then(move |s| {
   |          ^^^^^^^^ expected enum `std::result::Result`, found struct `std::io::Error`
   |
   = note: expected enum `std::result::Result<std::option::Option<_>, _>`
            found struct `std::io::Error`
   = note: required because of the requirements on the impl of `futures_util::fns::FnOnce1<std::io::Error>` for `[closure@examples/server.rs:67:44: 72:14]`
   = note: required because of the requirements on the impl of `std::future::Future` for `futures_util::future::future::map::Map<futures_util::future::try_future::into_future::IntoFuture<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>>, futures_util::fns::MapErrFn<[closure@examples/server.rs:67:44: 72:14]>>`
   = note: required because of the requirements on the impl of `futures_core::future::TryFuture` for `futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>`

error[E0599]: no method named `boxed` found for struct `futures_util::stream::try_stream::and_then::AndThen<futures_util::stream::try_stream::MapErr<tokio::net::tcp::incoming::Incoming<'_>, [closure@examples/server.rs:65:18: 65:64]>, futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>, [closure@examples/server.rs:66:19: 73:10 tls_acceptor:_]>` in the current scope
  --> examples/server.rs:74:10
   |
74 |         .boxed();
   |          ^^^^^ method not found in `futures_util::stream::try_stream::and_then::AndThen<futures_util::stream::try_stream::MapErr<tokio::net::tcp::incoming::Incoming<'_>, [closure@examples/server.rs:65:18: 65:64]>, futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>, [closure@examples/server.rs:66:19: 73:10 tls_acceptor:_]>`
   | 
  ::: /Users/averyharnish/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-util-0.3.5/src/stream/try_stream/and_then.rs:13:1
   |
13 | pub struct AndThen<St, Fut, F> {
   | ------------------------------
   | |
   | doesn't satisfy `_: futures_core::stream::Stream`
   | doesn't satisfy `_: futures_util::stream::stream::StreamExt`
   |
   = note: the method `boxed` exists but the following trait bounds were not satisfied:
           `futures_util::stream::try_stream::and_then::AndThen<futures_util::stream::try_stream::MapErr<tokio::net::tcp::incoming::Incoming<'_>, [closure@examples/server.rs:65:18: 65:64]>, futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>, [closure@examples/server.rs:66:19: 73:10 tls_acceptor:_]>: futures_core::stream::Stream`
           which is required by `futures_util::stream::try_stream::and_then::AndThen<futures_util::stream::try_stream::MapErr<tokio::net::tcp::incoming::Incoming<'_>, [closure@examples/server.rs:65:18: 65:64]>, futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>, [closure@examples/server.rs:66:19: 73:10 tls_acceptor:_]>: futures_util::stream::stream::StreamExt`
           `&futures_util::stream::try_stream::and_then::AndThen<futures_util::stream::try_stream::MapErr<tokio::net::tcp::incoming::Incoming<'_>, [closure@examples/server.rs:65:18: 65:64]>, futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>, [closure@examples/server.rs:66:19: 73:10 tls_acceptor:_]>: futures_core::stream::Stream`
           which is required by `&futures_util::stream::try_stream::and_then::AndThen<futures_util::stream::try_stream::MapErr<tokio::net::tcp::incoming::Incoming<'_>, [closure@examples/server.rs:65:18: 65:64]>, futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>, [closure@examples/server.rs:66:19: 73:10 tls_acceptor:_]>: futures_util::stream::stream::StreamExt`
           `&mut futures_util::stream::try_stream::and_then::AndThen<futures_util::stream::try_stream::MapErr<tokio::net::tcp::incoming::Incoming<'_>, [closure@examples/server.rs:65:18: 65:64]>, futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>, [closure@examples/server.rs:66:19: 73:10 tls_acceptor:_]>: futures_core::stream::Stream`
           which is required by `&mut futures_util::stream::try_stream::and_then::AndThen<futures_util::stream::try_stream::MapErr<tokio::net::tcp::incoming::Incoming<'_>, [closure@examples/server.rs:65:18: 65:64]>, futures_util::future::try_future::MapErr<tokio_rustls::Accept<tokio::net::tcp::stream::TcpStream>, [closure@examples/server.rs:67:44: 72:14]>, [closure@examples/server.rs:66:19: 73:10 tls_acceptor:_]>: futures_util::stream::stream::StreamExt`

warning: unused import: `StreamExt`
  --> examples/server.rs:10:22
   |
10 |     stream::{Stream, StreamExt, TryStreamExt},
   |                      ^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

error: aborting due to 2 previous errors; 1 warning emitted

Some errors have detailed explanations: E0271, E0599.
For more information about an error, try `rustc --explain E0271`.
error: could not compile `hyper-rustls`.

To learn more, run the command again with --verbose.

I tried looking into the documentation and I found into_failable which seems like maybe something that could be useful to me? But really I'm not sure where to go from here. Any help with this is greatly appreciated and I'd be happy to update the server example with something that works once I understand what's going on.

[jspspike]

This is a possible solution so that the server doesn't error out on a failed tls connection. I'll try to add some for of this to the example to help others trying to do the same thing.

  let tls_acceptor = &TlsAcceptor::from(tls_cfg);
    // Prepare a long-running future stream to accept and serve cients.
    let incoming_tls_stream = tcp
        .incoming()
        .filter_map(move |s| async move {
            let client = match s {
                Ok(x) => x,
                Err(e) => {
                    println!("Failed to accept a client, should probably back off");
                    return Some(Err(e));
                }
            };
            match tls_acceptor.accept(client).await {
                Ok(x) => Some(Ok(x)),
                Err(e) => {
                    println!("[!] Voluntary server halt due to client-connection error...: {}", e);
                    None
                }
            }
        })
        .boxed();

[pickfire]

Not just error handling, the server example can only accept one connection at a time, if you have a resolver that requires some time to complete and you have two connections, the second one will get stuck, essentially this means we cannot do tls verification with acme, I have solve this by using the TlsAcceptor in warp.

[mikedilger]

I have solve this by using the TlsAcceptor in warp.

FYI: While the TlsAcceptor and TlsStream in warp do avoid both of these issues, they are designed in a way that presents another issue. At the time a handler future is created, the warp TlsStream wrapper is still in Handshaking state. So your handler cannot get access to any SSL info (sni hostname, client certificates, etc). It goes to Streaming via AsyncRead/AsyncWrite events.

[Parth]

@ctz would you be the right person to nudge regarding this?

[Parth]

@mikedilger in what situation would I want that info?

I read a bit and it seems to be for instances where multiple dns entries point to the same application. So I could, within my application serve a different certificate based on what the client is trying to do.

And I would only be interested in client certificates if I was trying to do mTLS.

Is my understanding correct? If I'm not trying to do either of those things, am I free to use warp for in-process tls processing?

[mikedilger]

@Parth Using a warp filter, you can get the host Authority, which it gets either from the Host header or the target URI. It doesn't look at the SNI hostname (afaik). The SNI hostname is for SSL establishment, certificate selection, and not really needed outside of SSL generally. But it would be interesting if the SNI hostname didn't match the HTTP Authority, in which case I would like to detect that and refuse service. I was also more interested in low-level SSL details for curiosity sake and testing of clients to see what they negotiated. For that I ended up using hyper directly and coded the asynchronous handling manually rather than using Service and MakeService traits. Under that paradigm I get everything I want and I understand it a lot better.

[Parth]

@mikedilger is there a way I could study your implementation?

[mikedilger]

@Parth Sure. Here is a self-contained compiling example. Maybe this could be massaged into the example this issue is asking for. It's just that I'm not using hyper-rustls to do it.

https://gist.github.com/mikedilger/589f616a2ca607ad1daed278042c1bb8

[djc]

FYI, I think #147 fixes this issue.

[Vagelis-Prokopiou]

+1 from me.

I tried to create a server both by trying to port the example server from the repo to an actual bin project and by trying to reproduce the example server from the docs.
In both cases I had errors that I did not manage to resolve. Some of them are related to the various crate versions for sure, but overall creating a server from the docs seems quite unfeasible.

[djc]

Please be more concrete about the issues you ran into (ideally with specific errors).

[Vagelis-Prokopiou]

I managed to make the docs example server run. The problems were crate versions related. I managed to make it run with the following crate versions:

[dependencies]
hyper = { version = "0.14.27", features = ["full"] }
hyper-rustls = "0.24.2"
rustls = { version = "0.21.10", features = ["default"] }
tokio = { version = "1.35.0", features = ["full"] }
rustls-pemfile = { version = "1.0.4", features = [] }

Maybe the crate versions that the current example needs should be added as a comment at the top of the code example?
I believe it would be very helpful.

PS: The example server from the GitHub repo gives errors like:

error[E0308]: arguments to this method are incorrect
  --> src/main.rs:94:10
   |
94 |         .with_single_cert(certs, key)
   |          ^^^^^^^^^^^^^^^^        --- expected `PrivateKey`, found `PrivateKeyDer<'_>`
   |
note: expected `Vec<Certificate>`, found `Vec<CertificateDer<'_>>`
  --> src/main.rs:94:27
   |
94 |         .with_single_cert(certs, key)
   |                           ^^^^^
   = note: expected struct `Vec<rustls::key::Certificate>`
              found struct `Vec<CertificateDer<'_>>`

I believe they are crate versions related too. For example, the docs example needs rustls-pemfile 1.0.4, whereas the GitHub example needs rustls-pemfile 2.

Overall, I think that the dependencies that each example needs should be stated somehow clearly, to avoid errors and confusion.