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

Introduce response header limits #453

Merged
merged 5 commits into from
Nov 18, 2024
Merged

Introduce response header limits #453

merged 5 commits into from
Nov 18, 2024

Conversation

maxmoehl
Copy link
Member

@maxmoehl maxmoehl commented Nov 7, 2024

Summary

  • Fix the header accounting on requests.
  • (internal) Rename the config field MaxHeaderBytes to MaxRequestHeaderBytes.
  • Add MaxRequestHeaderBytes and the corresponding logic.

A request with too large response headers will now look like this:

$ curl -i "https://net-tools.cf.local/headers?foobarbazz=$(python3 -c 'print("a" * 1000)')"
HTTP/2 502
content-type: text/plain; charset=utf-8
x-cf-routererror: endpoint_failure (net/http: HTTP/1.x transport connection broken: net/http: server response headers exceeded 1024 bytes; aborted)
x-content-type-options: nosniff
date: Thu, 07 Nov 2024 09:24:09 GMT
content-length: 67

502 Bad Gateway: Registered endpoint failed to handle the request.

Backward Compatibility

Breaking Change? No

@maxmoehl maxmoehl requested a review from a team as a code owner November 7, 2024 09:27
@maxmoehl
Copy link
Member Author

maxmoehl commented Nov 7, 2024

This is the first set of changes, next up is limiting the number of headers as outlined in the issue. (and of course tests)

@maxmoehl maxmoehl force-pushed the maxmoehl/header-limits branch from b937564 to 9c275e6 Compare November 8, 2024 19:26
@maxmoehl
Copy link
Member Author

I'm not sure whether we want to rename the gorouter yaml property from max_header_bytes to max_request_header_bytes or whether that's already considered breaking?

@maxmoehl maxmoehl force-pushed the maxmoehl/header-limits branch from f128f20 to 6fdf626 Compare November 11, 2024 09:57
@maxmoehl
Copy link
Member Author

Note to the reviewer: I've split the changes into review-able commits focusing on one aspect each so it's easier to review the individual changes :)

Copy link
Contributor

@peanball peanball left a comment

Choose a reason for hiding this comment

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

requesting to review some naming consistency and code comments for clarification (alignment with existing comment density)

config/config.go Show resolved Hide resolved
proxy/proxy.go Show resolved Hide resolved
proxy/round_tripper/proxy_round_tripper.go Show resolved Hide resolved
integration/header_test.go Outdated Show resolved Hide resolved
handlers/max_request_size_test.go Show resolved Hide resolved
When counting they bytes sent by the client in the request line and
headers, repeated header keys are not counted. This commit adjusts the
logic to account for repeated keys.
@maxmoehl maxmoehl force-pushed the maxmoehl/header-limits branch from f4d4095 to d3b8890 Compare November 12, 2024 12:02
peanball
peanball previously approved these changes Nov 12, 2024
@maxmoehl
Copy link
Member Author

Once the routing-release PR is ready as well I will merge both.

Rename the config property in preparation for the new response header
limit. The YAML property retains its name to stay compatible with any
existing configurations.
Add a new property to configure the response header limit of the
transport used to send requests to route services and backends.

Resolves: cloudfoundry/routing-release#309
This commit adds support to limit the amount of request and response
headers gorouter will accept and process.

Resolves: cloudfoundry/routing-release#309
peanball
peanball previously approved these changes Nov 18, 2024
@ameowlia ameowlia merged commit 2794143 into main Nov 18, 2024
1 check passed
@ameowlia ameowlia deleted the maxmoehl/header-limits branch November 18, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants