Implement X-Forwarded-For header processing
#7359
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Turbo87
added
C-internal 🔧
This adjusts the `process_xff_headers()` to our current deployment reality of the app being reachable either directly on Heroku or with CloudFront in front. It also adds a work around for some broken Heroku behavior.
…eader values This is supposed to eventually replace the `X-Real-IP` header usage once production traffic has shown that our implementation is correct.
Turbo87
commented
Oct 25, 2023
There was a problem hiding this comment.
Mainly, two major comments:
- String comparisons on a flat vec for something per-request is very slow, it'd be beneficial to use
cidrin an appropriate integer-sized hashmap for lookups. - Should we even allow direct calls via heroku, or should we just start banishing all requests not using a CDN?
|
Since this is purely additive and the performance concerns have been addressed, I'll go ahead and merge this to test it out on staging and then deploy it to production. |
Turbo87
commented
Oct 26, 2023
Comment on lines
+168
to
+169
| // The Heroku router has a bug where it currently (2023-10-25) appends | ||
| // the connecting IP to the **first** header instead of the last. |
There was a problem hiding this comment.
for context: heroku/roadmap#238
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR essentially implements http://nginx.org/en/docs/http/ngx_http_realip_module.html inside of our axum server, as another step towards removing the nginx wrapper.
… but there is a twist. Heroku is somewhat broken. Their broken handles the header incorrectly in case the original request contains multiple
X-Forwarded-Forheaders. Instead of adding the connecting IP to the last header, or merging the headers, it adds the IP to the first header 😱I've reported this to Heroku in the form of a support ticket, but while that is being processed we will unfortunately have to use a workaround. Luckily CloudFront merges these headers, so if we get multiple headers we can know for certain that the request did not come from CloudFront.
On the bright side, we only use the value for logging at the moment. The PR adds another field to the logs with the IP addressed that was parsed from the
X-Forwarded-Forheaders. It also adds a log marker in case the IP is different from the one that nginx supplies us in theX-Real-IPheader. This will allow us to see if there are differences to the nginx algorithm once production traffic hits this code. Spoiler alert: yes, the Heroku workaround is a difference, but for regular, non-malicious traffic this should not matter.