[gorouter] Add property to restrict header count and size

[maxmoehl]

We deploy gorouter behind HAProxy which limits the amount of headers and the size of each header for requests and responses. For requests towards CF this isn't much of an issue since the request is rejected with 400: Bad Request and it never reaches the application. However, response headers are processed by the gorouter and the request shows up as 200: OK but is later rejected by HAProxy and returned as a 502: Bad Gateway to the client.

To avoid this behaviour which is confusing for users, we would like to introduce two properties:

• max_headers: int to limit the amount of req/res headers
• max_header_size: int to limit the number of bytes a single header is allowed to occupy the request/response line and headers (without body) are allowed to occupy

This allows us to reject responses in gorouter and show the error message in the app logs which are visible to the user.

We can implement this feature but would like to first discuss if there are any concerns/questions. Leave a 👍 if you like this / don't see any issues with it.

[ameowlia]

max_headers

• Adding max_headers as described technically sounds good, as long as you provide a way to make it unlimited so it is not a breaking change.
• ❓ However, I don't understand the use case behind limiting the total number of headers instead of the total size. Can you explain the use case?

max_header_size

• The property router.max_header_kb already exists, which is very similar to what you want for max_header_size.
• However router.max_header_kb only applies to requests (I believe) and not responses.
• Maybe you could use that property and also add a property enforce_header_limit_on_response or something? It's not the cleanest, but I am always worried about causing breaking changes.
• Happy to hear other suggestions for how to keep these properties are clear as possible, while not causing a breaking change.

[maxmoehl]

@ameowlia thanks for the feedback!

First of all: yes, we always want to make sure to not break any existing setups, the properties would be disabled by default and only considered when explicitly enabled.

I should've provided a bit more detail in my initial issue: HAProxy has two main buffer sizes: tune.bufsize and tune.maxrewrite:

• tune.bufsize is the base buffer size
• tune.maxrewrite is the portion of tune.bufsize that is reserved for modification while processing the request
• the buffer size available for the request/response line + headers (without body) is then tune.bufsize - tune.maxrewrite ¹
• tune.http.maxhdr the maximum amount of headers on each response/request

max_headers

We want to be able to set some limit that applies to request and response headers. The main reason to add such a property is to fail in gorouter and show the failure in logs the user can access (+ the x_cf_routererror header to give more details).

max_header_size

Agreed, this covers our use-case. Adding a flag to enforce the limit on responses as well sounds good!

Footnotes

1. there is one mistake in my original issue: what I proposed as a per-header limit is actually the limit for all headers + request/response line (so it's the same as max_header_kb), I will update the issue accordingly. ↩

[ameowlia]

Sounds good to me; I look forward to the PR! (Also I learned about markdown footnotes 😮.)

[MarcPaquette]

Hey @maxmoehl & @ameowlia,
Is this issue still open? Is there any further work to do or can we close this out?

[plowin]

Hey @MarcPaquette, thx for checking in!
We will soon invest in this topic, it is still required work and should stay open.

[maxmoehl]

Looking into this I noticed that we are forced to do our own accounting as the request would otherwise be invisible in our logs. I've commented on golang/go#21548 as we really need a solution for this, this has become a recurring pattern for us by now (other examples include TLS handshake errors or connections which abort half-way through the headers).

As we have to work with the parsed representation of the request this entire accounting is error-prone. One thing I noticed is that repeated header keys are not accounted for, so repeating This-Is-A-Very-Long-Header-Key: a will allow you to work around the limit (I don't consider this a real risk as we still enforce a hard limit independent of of the configured one). I'm preparing a fix for this.

Another thing which is unclear to me is whether we should distinguish between HTTP/1.1 and HTTP/2. Given that HTTP/2 is a binary protocol with framing, properly counting the bytes on the wire will be impossible as there is no way of knowing how many frames were used to transmit the headers. So our best guess is to just treat them equally and always count according to the HTTP/1.1 representation?

Coming back to the original issue, what I wanted initially was to limit the amount of bytes read from the wire from the start of each request - independent of protocol etc. but without a change to the stdlib this won't be possible.

---

Limiting the received headers from the application looks much more straight forward. Setting the limit and then exceeding it will simply result in an error which can be handled.