Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
finnbear opened this issue Mar 10, 2022 · 3 comments
Closed

Comments

@finnbear
Copy link
Contributor

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 🎉). 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?
finnbear added a commit to finnbear/axum-server that referenced this issue Mar 11, 2022
@programatik29
Copy link
Owner

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
Copy link
Contributor Author

finnbear commented Mar 31, 2022

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants