fix(server): start header read timeout immediately

[seanmonstar]

The http1_header_read_timeout used to start once there was a single read of headers. This change makes it start the timer immediately, right when the connection is estabilished.

Closes #3178

cc @paolobarbolini @silence-coding @SylvainGarrigues

[jeromegn]

I ran into this Today. Anything blocking this?

[seanmonstar]

A comment on the linked issue gave me pause. I started looking in other libraries, and for instance in Go, they use a ReadHeaderTimeout and an IdleTimeout for the different stages of a connection. It might be more appropriate to do the same here. I'm not yet sure which is better.

cc @sfackler, you've asked often about idle and read timeouts.

[sfackler]

I would interpret the time before the first byte of the header is read as idle time rather than read-header time, yeah.

You can already make a wrapper around a Hyper Connection + handler service that tracks idle timeouts but it's a huge PITA. Having a built in feature for that would definitely be a good idea given that it's a pretty universal thing to want in servers.

[finnbear]

FWIW the documentation on http1_header_read_timeout implies to me that the timeout would start immediately and stop when the header was fully transferred. I would have expected to see "If a client starts but does not finish transmitting the entire header within this time, the connection is closed" to describe the current behavior.

I'm not yet sure which is better.

+1 to using the same timeout such that a longer time budget helps both idle connections and slow header connections but does not help malicious connections (which idle and then slowly send headers) last twice as long.

[jeromegn]

I forgot about this and made my own PR.

Without this change, hyper is vulnerable to malicious actors who may just open connections and never send anything.

I'd love a idle timeout functionality, but that's a lot more involved afaict.

To be honest, I was planning on just using this "fixed" header timeout behaviour in addition to a "idle timeout" in my http_body::Body implementation that would reset on successful reads w/ poll_data. That would be sufficient for our use case.

Reading headers is the very first stage of a new HTTP 1 connection and it makes sense to me that it would timeout if http1_header_read_timeout is set and the client does not send any data. If hyper were to add a idle timeout, it would also work, both would start at the very beginning of the connection. A sensible idle timeout might be 60 seconds, but that seems wrong for a header read timeout. I'd set our timeout for headers to ~5s.

[jeromegn]

Would a different config please everybody? Maybe http1_header_start_timeout?

[seanmonstar]

Would a different config please everybody? Maybe http1_header_start_timeout?

I think, from talking to several people, many find it clearer for there to be a separate timeout. So yea, something like http1_idle_timeout (or http1_keepalive_timeout, or something, naming is hard). If so, I could close this, and we could make a new issue describing it the feature.

[finnbear]

This is a restatement of #3185 (comment) with additional details.

think, from talking to several people, many find it clearer for there to be a separate timeout.

Clarity aside, isn't it the case that splitting one timeout into two timeouts could make DoS twice as effective?

For example, assume 99.99% of legitimate clients will send a header consisting of multiple TCP segments, and may occasionally experience up to 5000ms of latency/congestion e.g. due to being on a cellular data connection.

If there was one timeout, it could be 5000ms. As such, an adversary must transmit the entire header in 4750ms for a good chance keeping the connection open.

If there were two timeouts, both of them would have to be 5000ms, because the congestion could occur either before or during the header transmission. As such, an adversary could wait 4750ms before sending the first segment and then an additional 4750ms before sending the rest of the header, keeping the connection open for twice as long.

By Little's Law, doubling the lifetime of a connection means there will be double the number of active connections, per unit of potentially-firewall-limited adversary bandwidth. This could double the impact of a DoS attack on OS and server process resources.

(that said, having a timeout is infinitely better than not having one. the two-timeout approach is probably good enough for me)

[seanmonstar]

I've since decided to re-open this and merge. While some libraries do have a separate timeout options, none of them seem to use the idle timeout for the very first request. Any concern of a browser preconnect not working isn't a concern to any other implementation, and that makes sense: if a browser looks like a bad guy, it should stop that.

[RLuzi]

Hello everyone, sorry to bother you.
I'm having a problem that I can't explain with a simple server I implemented with hyper, setting 10 seconds as the value of the header_read_timeout() function in the Builder of the http1 server.

If I connect with nc and do nothing for 10 sec or if I start pressing enter without typing anything, the connection is closed after 10 sec.

If I send a request and, after receiving the reply, press enter again, the connection is closed after 10 sec.

But it will never be closed if I send a request and do nothing after receiving the reply, leaving the connection open and inactive.

Is this normal behaviour? Shouldn't it still be closed by the server after 10 sec?

Thanks in advance.

[finnbear]

I want to investigate this, because I rely on the header_read_timeout being applied to all requests.

@RLuzi please confirm which version of hyper you are using (you can find it in Cargo.lock as Cargo.toml is just a lower bound)

[seanmonstar]

Interesting. Could you open a new issue with a reproduction so we can look into it?

[jeromegn]

If I send a request and, after receiving the reply, press enter again, the connection is closed after 10 sec.

Maybe I'm understanding wrong, but I think this is working as designed.

If you press enter, then you're sending a new request and at that point the header read timeout starts ticking.

If you want connections to be closed after a request is done, then that's probably new code to handle that or different settings?

What if you send Connection: close in the first request and then do nothing? Does it close right after receiving the response?

[finnbear]

Ah, yes. I was confused (see #3743 (comment) for more). The current behavior only starts the header read timeout once a subsequent request has already been started. The linked comment shows a simple change that would change this.

This PR will, if merged, apply the h1 header timeout when waiting for subsequent requests, closing the gap between expectations and actual behavior here: #3828

Could you open a new issue with a reproduction so we can look into it?

This new test is a reproducible example: 1ed8ea0

[RLuzi]

Interesting. Could you open a new issue with a reproduction so we can look into it?

I think that #3828 describes perfectly the situation: "after the first request, the connection can remain open indefinitely". So if the change proposed in the PR solves this problem, we should be fine.

If you press enter, then you're sending a new request and at that point the header read timeout starts ticking.

I totally agree with what you said here.

If you want connections to be closed after a request is done, then that's probably new code to handle that or different settings?
What if you send Connection: close in the first request and then do nothing? Does it close right after receiving the response?

But the behaviour I would like is not to close the connection after each request. This can be achieved by deactivating keep_alive with the dedicated function of the server's Builder.

What I would like is for there to be a timeout that tells the server when to close the connection if it remains idle after a request. Which is exactly the behaviour described for what was called ‘idle_timeout’ in the quoted PR.

I apologise if I misspoke above, what I was wondering was whether it was normal for the timeout set with header_request_timeout() to fail during an idle period after a request has been sent.

@RLuzi please confirm which version of hyper you are using (you can find it in Cargo.lock as Cargo.toml is just a lower bound)

If it can still be useful, I'm working with hyper version 1.5.1.

Thank you all for your time.

[RLuzi]

Good morning! When do you think the pull request for this problem will be reviewed and possibly accepted?