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

feat(h1): add server header read timeout #2675

Merged
merged 17 commits into from Nov 18, 2021
Merged

feat(h1): add server header read timeout #2675

merged 17 commits into from Nov 18, 2021

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Oct 26, 2021

Implements a timeout for reading the request header. The timer starts either when the connection is established (first request) or when the first byte is read from the header (subsequent requests reusing the same connection). The entire header must be read before the specified timeout is elapsed.

This is the first step towards protecting from Slowloris types of attacks

Supersedes #2661 by fixing #2661 (comment)

Partially fixes #2457

Closes #2661

@paolobarbolini paolobarbolini marked this pull request as ready for review October 27, 2021 04:10
@silence-code
Copy link

This is to start timing after parsing the request header of the first block. Clear the timing after parsing all the request headers?

@silence-code
Copy link

It is recommended that test cases cover these scenarios:

  1. First build a slow request header, but within our timeout, the request succeeds, and then send only one request that will trigger timeout
  2. Build multiple slow request headers within our timeout, and the requests are successful.

@paolobarbolini
Copy link
Contributor Author

paolobarbolini commented Oct 28, 2021

This is to start timing after parsing the request header of the first block. Clear the timing after parsing all the request headers?

Yup

It is recommended that test cases cover these scenarios:

  1. First build a slow request header, but within our timeout, the request succeeds, and then send only one request that will trigger timeout
  2. Build multiple slow request headers within our timeout, and the requests are successful.

Done in the last commit

@paolobarbolini
Copy link
Contributor Author

Resolved the merge conflicts after the last PR was merged.

Ping @seanmonstar. We're waiting for a code review 😃

@paolobarbolini paolobarbolini changed the title Implement http1 server header read timeout feat(h1): add server header read timeout Nov 5, 2021
@silence-coding
Copy link
Contributor

@paolobarbolini From the header_read_timeout_no_write test case, it seems that once the connection is established, the timeout mechanism is started. Is that correct? I think this implementation is lacking, which means that the client's connection pool cannot establish connections in advance, and the client's keepAlive configuration is broken and does not keep alive as expected.

@paolobarbolini
Copy link
Contributor Author

@paolobarbolini From the header_read_timeout_no_write test case, it seems that once the connection is established, the timeout mechanism is started. Is that correct? I think this implementation is lacking, which means that the client's connection pool cannot establish connections in advance, and the client's keepAlive configuration is broken and does not keep alive as expected.

Right, I didn't consider connection pools starting connections in advance. Fixed

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the lag, thanks for writing this! It's very thorough, and I especially appreciate that there's tests so we don't accidentally break it in the future ❤️

I left a few comments inline.

Cargo.toml Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/proto/h1/role.rs Outdated Show resolved Hide resolved
src/server/server.rs Show resolved Hide resolved
src/server/conn.rs Outdated Show resolved Hide resolved
src/server/server.rs Outdated Show resolved Hide resolved
paolobarbolini and others added 2 commits November 18, 2021 20:35
Co-authored-by: Sean McArthur <sean@seanmonstar.com>
Co-authored-by: Sean McArthur <sean@seanmonstar.com>
@seanmonstar seanmonstar merged commit 842c655 into hyperium:master Nov 18, 2021
@NeoLegends
Copy link

Great work here! This PR leaves me wondering though, won't hyper also need this for h2 and h3, or are there other mechanisms in place there that prevent slow loris attacks?

@DFINITYManu
Copy link

DFINITYManu commented Aug 5, 2023

Valiant effort, but still there is no way to set a timeout for clients that connect only to send no data (by experimentation I discovered that the timeout does not fire if I just telnet into the TCP port and hang on there -- it only fires if I send the actual headers too late). Which means it's trivial to gum up a hyper server just by sending a lot of connections and nothing else.

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

Successfully merging this pull request may close these issues.

Defends against slow HTTP attacks.
6 participants