programatik29 / axum-server

High level server designed to be used with axum framework.
MIT License
168 stars 56 forks source link

Does this guard against TCP connection leaks during SSL handshake? #37

Closed finnbear closed 2 years ago

finnbear commented 2 years ago

I recently decided to port my application to the axum framework, and intend for my Rust binary to do its own SSL. This crate seems like the best way to achieve that (especially since it supports hot-swapping certificates :tada:). However, one of the issues I had with a previous web framework was severe, persistent TCP connection leaks, due in part to a lack of a timeout on accepting SSL connections (and also during http2 upgrade, but that's outside the scope of this crate). When I took a look at the source code, I couldn't help but notice that no timeouts or sleeps were incorporated into the relevant Futures. This leads me to believe that, were I to deploy my application in production, I would see TCP connection counts steadily climb as real clients have a habit of silently hanging up their connection at the worst time.

So my questions are:

  1. Does this (or hyper, or axum) impose any timeout for SSL handshake? (or otherwise prevent this form of TCP connection leakage?)
  2. If not, is there a workaround (such as implementing a custom acceptor)?
  3. Would you prefer if I write a short test case to demonstrate an issue or lack there of?
programatik29 commented 2 years ago

Hey, sorry for the wait. I had no access to technology for a month.

  1. This crate uses tokio-rustls crate to handle TLS and doesn't do much else. AFAIK low-level hyper API doesn't have timeouts.
  2. Yes. You can implement axum_server::accept::Accept and use your custom acceptor. If you return an error on accept fn, connection will be dropped.
  3. That would be really good.
finnbear commented 2 years ago

Hey, sorry for the wait. I had no access to technology for a month.

No problem at all! I've just been using my PR in production in lieu of it being merged.

  1. That would be really good.

Copy and past the following into src/tls_rustls/mod.rs as a test.

    #[tokio::test]
    async fn tls_timeout() {
        let (handle, _server_task, addr) = start_server().await;
        assert_eq!(handle.connection_count(), 0);

        let stream = TcpStream::connect(addr).await.unwrap();

        // We intentionally avoid driving a TLS handshake to completion.
        std::mem::forget(stream);

        for i in 0..120 {
            tokio::time::sleep(Duration::from_secs(1)).await;
            if handle.connection_count() == 0 {
                println!("took {} seconds to cancel", i);
                break;
            }
        }

        assert_eq!(handle.connection_count(), 0, "two minutes is way too long");
    }

main branch:

$ cargo test --features tls-rustls -- --nocapture
test tls_rustls::tests::tls_timeout ... FAILED

failures:

---- tls_rustls::tests::tls_timeout stdout ----
thread 'tls_rustls::tests::tls_timeout' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: two minutes is way too long', src/tls_rustls/mod.rs:319:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

With my PR:

$ cargo test --features tls-rustls -- --nocapture
test tls_rustls::tests::tls_timeout ... ok
took 1 seconds to cancel

@programatik29

programatik29 commented 2 years ago

This(#39) pull request should solve this issue.