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
network: Use peer address after proxy fix for app rate limiter if available #5848
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5848 +/- ##
===========================================
- Coverage 55.83% 29.16% -26.67%
===========================================
Files 477 477
Lines 67256 67269 +13
===========================================
- Hits 37552 19621 -17931
- Misses 27171 45590 +18419
+ Partials 2533 2058 -475 ☔ View full report in Codecov by Sentry. |
437e683
to
87efa11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a lot more to read, but need to stop early for the day.
41515b7
to
5e238b3
Compare
Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>
e243753
to
9b70c0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy to follow, feedback incorporated well. Agree with decision to apply the last in list rule solely X-Forwarded-For.
Summary
peer.RoutingAddr
used to useconn.Addr
to get remote IP. Changed to useremoteAddress
in order to accommodate possible reverse proxy hiding nodes behind.Additionally, changed
X-Forwarded-For
handing to allow the single reverse proxy use case (handle potentially adversarial "X-Forwared-For" headers from clients).Before: did not use any values if there is a list of values in the first
X-Forwarded-For
header.After: use the last value in the list "A, B, C" in the last
X-Forwarded-For
header.Test Plan
Added a test ws proxy implementation and a unit test checking the addr is taken from a proxy-provided header.
Added unit tests for
X-Forwarded-For
andRoutingAddr
changes.