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

headers: warn on surprisingly large copied headers #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rjbs
Copy link
Member

@rjbs rjbs commented Jun 15, 2020

No description provided.

@wolfsage
Copy link
Contributor

few nits, seems right


# Chosen arbitrarily as "seems pretty large". -- rjbs, 2020-06-15
if (length $v > 1024 && ! NO_WARN) {
warn sprintf "proxying large (%ib) %s header", length $v, $k;
Copy link
Contributor

Choose a reason for hiding this comment

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

%ib? Huh

Copy link
Member Author

Choose a reason for hiding this comment

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

%i to format an integer. b is then the literal character b.

Choose a reason for hiding this comment

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

Just checking, this shows the length and header, but not the URL that generated the large header. Are the request url and the error correlated in the logs so we can see what crazy server it is that's returning the bogus header?

Also this doesn't actually drop or truncate the header, so the underlying problem of large headers causing nginx to block an upstream will still occur right?

Also, I think the problem isn't the size of an individual header, but the sum total of response headers.


http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffer_size

proxy_buffer_size size;

Sets the size of the buffer used for reading the first part of the response received from the proxied server. This part usually contains a small response header. By default, the buffer size is equal to one memory page. This is either 4K or 8K, depending on a platform. It can be made smaller, however.

And really, it's more than just headers there, it's the response line as well. So I wonder if the best thing to do here is:

  1. Have @COPY_RESPONSE_HEADERS in a specific order of "most important headers" (e.g. content-type) to "least important headers" (e.g. etag)
  2. Copy/add the headers hopscotch adds itself first, keeping track of the length
  3. Copy each header in @COPY_RESPONSE_HEADERS order, keeping track of the length
  4. Have a "maximum headers length" config option (default to 8k, which is what we have set in nginx-http.conf), and if the sum length of headers would exceed that length, just drop the headers

Hopscotch interacts with external real world servers, so it really does need to have a bit of a "defense in depth" approach to this stuff.

warn sprintf "proxying large (%ib) %s header", length $v, $k;
}

push @out_headers, lc $k, $v;
Copy link
Contributor

Choose a reason for hiding this comment

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

parents around $k would be nice to show lc is only operating on it

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do.

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.

3 participants