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

API: Add timeouts for client requests to avoid DoS attack #204

Open
Tracked by #467
josecelano opened this issue Jun 15, 2023 · 11 comments
Open
Tracked by #467

API: Add timeouts for client requests to avoid DoS attack #204

josecelano opened this issue Jun 15, 2023 · 11 comments
Assignees
Labels
Security Publicly Connected to Security
Milestone

Comments

@josecelano
Copy link
Member

josecelano commented Jun 15, 2023

As described here, we need to set a timeout for requests to avoid a denial of service attack.

How to do it:

@josecelano josecelano added the Security Publicly Connected to Security label Jun 15, 2023
@josecelano
Copy link
Member Author

The tower::builder::ServiceBuilder::timeout it only fail requests that take longer than timeout.

If the next layer takes more than timeout to respond to a request, processing is terminated and an error is returned.

That's not behavior we want. We want the server to send a 408 response when the client opens a new HTTP connection and it does not use it to make any request.

HTTP 408 status code: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/408

ActixWeb implementation

Example using ActixWeb:

The HTTP service builder allows you to set the "client request timeout".

https://github.com/actix/actix-web/blob/1072d0dacf3a5a7a8e2d5fd7fe5bb47658198bf8/actix-http/src/builder.rs#L88-L100

/// Set client request timeout (for first request).
///
/// Defines a timeout for reading client request header. If the client does not transmit the
/// request head within this duration, the connection is terminated with a `408 Request Timeout`
/// response error.
///
/// A duration of zero disables the timeout.
///
/// By default, the client timeout is 5 seconds.
pub fn client_request_timeout(mut self, dur: Duration) -> Self {
    self.client_request_timeout = dur;
    self
}

You can also do it with the server:

https://github.com/actix/actix-web/blob/1072d0dacf3a5a7a8e2d5fd7fe5bb47658198bf8/actix-web/src/server.rs#L190-L201

/// Sets server client timeout for first request.
///
/// Defines a timeout for reading client request head. If a client does not transmit the entire
/// set headers within this time, the request is terminated with a 408 (Request Timeout) error.
///
/// To disable timeout set value to 0.
///
/// By default client timeout is set to 5000 milliseconds.
pub fn client_request_timeout(self, dur: Duration) -> Self {
    self.config.lock().unwrap().client_request_timeout = dur;
    self
}

Axum implementation

It seems Axum server also has a function to set the read timeout but I haven't been able to make it work:

let app = Router::new().route("/", get(entrypoint_handler)).layer(
    ServiceBuilder::new()
        .layer(HandleErrorLayer::new(handle_timeout_error))
        .layer(TimeoutLayer::new(Duration::from_secs(5)))
        .timeout(Duration::from_secs(5)),
);

let server = axum::Server::from_tcp(tcp_listener)
    .expect("a new server from the previously created tcp listener.")
    .http1_header_read_timeout(Duration::from_secs(5))
    .serve(app.into_make_service_with_connect_info::<SocketAddr>());

I've created a new repo to try different solutions: https://github.com/josecelano/axum-request-timeout/blob/main/src/app.rs

Axum uses the hyper server. And the hyper server has a method: http1_header_read_timeout

Axum Server implementation

axum-server is a hyper server implementation designed to be used with axum framework.

@programatik29 has proposed a solution for axum-server here.

Hyper

There is an open issue on the hyper repo: timeout while waiting for headers.

It seems "The http1_header_read_timeout method only starts the timer when the first line of the http1 header is sent."

Client Header Timeout vs Client Body Timeout

I'm not sure if we should use the client_header_timeout or the client_body_timeout.

As if it's described in the this issue, the client_header_timeout "defines a timeout for reading client request header. If a client does not transmit the entire header within this time, the request is terminated with the 408 (Request Time-out) error" while the client_body_timeout "defines a timeout for reading client request body. The timeout is set only for a period between two successive read operations, not for the transmission of the whole request body. If a client does not transmit anything within this time, the request is terminated with the 408 (Request Time-out) error."

Conclusion

It seems there is no official support for this in Axum. We could

  • Try to implement our solution like axum-server.
  • Migrate to the axum-server.
  • Keep the ActixWeb implementation until Axum supports this feature.
  • Enable the Axum implementation and let the user deal with this problem for the time being. We do not even support HTTPs yet, so users must use a proxy like Nginx. They can set up the timeouts there. See links.

What do you think @da2ce7?

Links

@programatik29
Copy link

Ideally hyper should support this feature rather than axum since supporting it in axum would require having its own server implementation. Hopefully this will be resolved with hyper-1.0 release.

@da2ce7
Copy link
Contributor

da2ce7 commented Jun 20, 2023 via email

@josecelano josecelano changed the title Axum API: add timeout layer to avoid DoS attack Add timeouts for client requests to avoid DoS attack Jun 20, 2023
@jmkng
Copy link

jmkng commented Nov 7, 2023

Hello all,

I see this issue is still open, I assume you are still having trouble with the timeout middleware? Maybe I can take a look.

@josecelano
Copy link
Member Author

Hello all,

I see this issue is still open, I assume you are still having trouble with the timeout middleware? Maybe I can take a look.

Hi @jmkng it seems the best solution would be to fix it in hyper 1.0, but it can take long until they release the version 1.0. Maybe in the mean time we could implement the solution proposed by @programatik29. It was fixed on the axum-server](programatik29/axum-server#39). Maybe we could do something similar and remove the patch once Hyper supports timeouts.

@cgbosse cgbosse added this to the v3.0.0 milestone Jan 16, 2024
@josecelano josecelano changed the title Add timeouts for client requests to avoid DoS attack API: Add timeouts for client requests to avoid DoS attack Feb 8, 2024
@josecelano
Copy link
Member Author

axum-server was supposed to include the feature to set up a timeout for the TLS handshake. We have moved to axum-server in the tracker. However, It seems that feature was removed. I've opened a new issue on the axum-server repo: programatik29/axum-server#116.

@josecelano
Copy link
Member Author

I've applied @programatik29 patch on the tracker. I will do the same here.

@programatik29 can we use your code? Is there any specific license we should include?

@josecelano josecelano self-assigned this May 16, 2024
@josecelano
Copy link
Member Author

josecelano commented May 16, 2024

I've just realized the patch on the tracker does not work when you enable TSL. I had to comment on the TimeoutAcceptor to make the TSL configuration work. See #584. After merging that PR this problem will be fixed only when TSL is disabled (HTTP) but I will not work for HTTPs.

    match tls {
        Some(tls) => custom_axum::from_tcp_rustls_with_timeouts(socket, tls)
            .handle(handle)
            //.acceptor(TimeoutAcceptor)
            .serve(router.into_make_service_with_connect_info::<std::net::SocketAddr>())
            .await
            .expect("API server should be running"),
        None => custom_axum::from_tcp_with_timeouts(socket)
            .handle(handle)
            .acceptor(TimeoutAcceptor)
            .serve(router.into_make_service_with_connect_info::<std::net::SocketAddr>())
            .await
            .expect("API server should be running"),
    };

We are not using TSL in production because we use Nginx as a reverse proxy. Certificates are handled with Nginx and certbot.

@josecelano
Copy link
Member Author

I've converted the discussion in the Axun repo into a issue: tokio-rs/axum#2741

@josecelano josecelano modified the milestones: v3.0.0, v3.1.0 May 27, 2024
@josecelano
Copy link
Member Author

A PR has been merged in the hyper repo. It changes the http1_header_read_timeout timeout.

image

This could fix this issue. We can try when this change is published in a new release.

@josecelano
Copy link
Member Author

A PR has been merged in the hyper repo. It changes the http1_header_read_timeout timeout.

image

This could fix this issue. We can try when this change is published in a new release.

hyper 1.4.0 has been released with server starting header read timeout immediately (#3185) (0eb1b6cf)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Security Publicly Connected to Security
Projects
Status: BUG & Security
Development

No branches or pull requests

5 participants