fix/header-timeout: adding ReadTimeout values

[patrickhuie19]

Adding large ReadTimeout values to address linting errors.

unblocks #38

[krehermann]

how do we determine if these are reasonable values?

what is the existing behavior?

[patrickhuie19]

how do we determine if these are reasonable values?

what is the existing behavior?

I have the same first question - I'll tag someone else in for a look to see if these could be detrimental. Based on my experience, these values are at the very high end of what an HTTP request should take before it's split up into a job polling pattern. (i.e. if that's the case here, we're going to need a bigger shovel!)

Existing behavior is no timeout, thus the potential for a slowloris attack.

[jmank88]

how do we determine if these are reasonable values?
what is the existing behavior?

I have the same first question - I'll tag someone else in for a look to see if these could be detrimental. Based on my experience, these values are at the very high end of what an HTTP request should take before it's split up into a job polling pattern. (i.e. if that's the case here, we're going to need a bigger shovel!)

Existing behavior is no timeout, thus the potential for a slowloris attack.

What about something more flexible that the callers can configure themselves, so they aren't stuck with defaults?

[patrickhuie19]

how do we determine if these are reasonable values?
what is the existing behavior?

I have the same first question - I'll tag someone else in for a look to see if these could be detrimental. Based on my experience, these values are at the very high end of what an HTTP request should take before it's split up into a job polling pattern. (i.e. if that's the case here, we're going to need a bigger shovel!)
Existing behavior is no timeout, thus the potential for a slowloris attack.

What about something more flexible that the callers can configure themselves, so they aren't stuck with defaults?

In the context of the threat vector, I see this change as something much more inflexible - i.e. while we could allow clients to configure timeout values that make sense for them, these are the upper bounds for a sensible HTTP request.

[jmank88]

In the context of the threat vector, I see this change as something much more inflexible - i.e. while we could allow clients to configure timeout values that make sense for them, these are the upper bounds for a sensible HTTP request.

But this code is not sanity checking an upper bound - it is imposing a single constant fixed value. Why is that necessary? What if they want a lower value?

[jkongie]

I don't see any problems setting a default value here, but as Jordan said it would be nice to be configurable with perhaps a sanity check which logs if the provided configuration value is over the default upper bound. It would be pretty easy to add configuration as a ServerOption (perhaps WithHTTPReadTimeout) that can be passed in during initialization.

For the value itself, it seems rather subjective, but the chosen values seem reasonable.

[jmank88]

It looks like the linter is concerned about ReadHeaderTimeout too.

[patrickhuie19]

After thinking about it, I think configurable is the way to go here.

But this code is not sanity checking an upper bound - it is imposing a single constant fixed value. Why is that necessary? What if they want a lower value?

You're right - whether larger or smaller we should leave it up to the library's customers/consumers to set. We can log a warning if it is not set.

For the value itself, it seems rather subjective, but the chosen values seem reasonable.

It's subjective , but we won't have to worry about artificially choosing a value in the next revision 🙂

It looks like the linter is concerned about ReadHeaderTimeout too.

I'll double check, but setting ReadTimeout should solve it. Package documentation:

// Because ReadTimeout does not let Handlers make per-request
// decisions on each request body's acceptable deadline or
// upload rate, most users will prefer to use
// ReadHeaderTimeout. It is valid to use them both.
ReadTimeout [time](https://pkg.go.dev/time).[Duration](https://pkg.go.dev/time#Duration)

// ReadHeaderTimeout is the amount of time allowed to read
// request headers. The connection's read deadline is reset
// after reading the headers and the Handler can decide what
// is considered too slow for the body. If ReadHeaderTimeout
// is zero, the value of ReadTimeout is used. If both are
// zero, there is no timeout.
ReadHeaderTimeout [time](https://pkg.go.dev/time).[Duration](https://pkg.go.dev/time#Duration)

stale