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.