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

Update RemoteAddrRoutePredicateFactory to optionally respect the X-Forwarded-For header. Addresses #155. #156

Merged
merged 2 commits into from
Mar 19, 2018

Conversation

fitzoh
Copy link
Contributor

@fitzoh fitzoh commented Jan 14, 2018

Doesn't play nice with yaml config yet, haven't figured out how to parse additional arguments from Tuple yet.

@codecov-io
Copy link

codecov-io commented Jan 14, 2018

Codecov Report

Merging #156 into master will increase coverage by 1.09%.
The diff coverage is 85.71%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #156      +/-   ##
========================================
+ Coverage    70.9%    72%   +1.09%     
========================================
  Files          99     87      -12     
  Lines        2437   1879     -558     
  Branches      166    123      -43     
========================================
- Hits         1728   1353     -375     
+ Misses        617    460     -157     
+ Partials       92     66      -26
Impacted Files Coverage Δ
...ler/predicate/RemoteAddrRoutePredicateFactory.java 76.47% <100%> (-12.72%) ⬇️
...ay/handler/support/RoutePredicateFactoryUtils.java 80% <84.21%> (ø)
...ework/cloud/gateway/route/builder/BooleanSpec.java 21.05% <0%> (-47.37%) ⬇️
.../factory/SetRequestHeaderGatewayFilterFactory.java 60% <0%> (-40%) ⬇️
...oud/gateway/filter/ratelimit/RedisRateLimiter.java 17.24% <0%> (-29.19%) ⬇️
...tory/RemoveResponseHeaderGatewayFilterFactory.java 71.42% <0%> (-28.58%) ⬇️
...actory/RequestRateLimiterGatewayFilterFactory.java 60% <0%> (-22.61%) ⬇️
...cloud/gateway/route/builder/GatewayFilterSpec.java 33.33% <0%> (-12.72%) ⬇️
...org/springframework/cloud/gateway/route/Route.java 70.14% <0%> (-5.53%) ⬇️
... and 63 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91d93fa...f167bc3. Read the comment docs.

@fitzoh fitzoh force-pushed the master branch 2 times, most recently from 16d8500 to bb1c346 Compare January 14, 2018 05:00
@fitzoh
Copy link
Contributor Author

fitzoh commented Jan 14, 2018

Need to look into security implications as well, this is probably a good starting point cloudfoundry/gorouter#179

@spencergibb
Copy link
Member

I don't know how to do it with tuple. Seems like it's all or nothing. Maybe @ConfigurationProperties.

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

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

I think this looks alright. Maybe update PredicateSpec?

@fitzoh
Copy link
Contributor Author

fitzoh commented Feb 13, 2018

I'll start digging back into this one, should probably add a link to #125 as well.

@fitzoh fitzoh force-pushed the master branch 3 times, most recently from 5a11f8d to 14b1e21 Compare February 14, 2018 00:18
@fitzoh
Copy link
Contributor Author

fitzoh commented Feb 14, 2018

Did some rebasing to catch up with the transition from String IPs -> InetSocketAddress and made it easier to customize the way the client IP is discovered.

By default, the existing behavior of exchange.getRequest().getRemoteAddress() is used.
There are now two other options:
extractFinalRemoteAddressFromXForwardedHeader always takes the first entry in the X-Forwarded-For, which is easy, but vulnerable to spoofing as described in some of the earlier referenced issues.
extractRemoteAddressFromXForwardedHeaderByMaxTrustedIndex requires an index (maxTrustedIndex) which corresponds to the number of trusted layers of infrastructure running in front of your application.

For instance, if you're running with just NGINX in front of the gateway, the maxTrustedIndex would be 1.

If you're running in Cloud Foundry with behind HAPROXY, then the gorouter, the maxTrustedIndex would be two.

The index is 1 based to prevent a user accidentally introducing an off by 1 error and erring too far in the untrusted direction.

Future work might include a remoteAddressResolver which takes a list of trusted IP addresses and terminates at the first untrusted IP. This would provide more flexibility if the number of ingress hops is unknown, but less helpful if trusted ip's are unknown.

Regarding PredicateSpec, I'm tempted to leave off a convenience function to increase the odds of the user reading the documentation, and ensuring that they're aware of the security implications.


/**
* @author Spencer Gibb
* @author Andrew Fitzgerald
*/
public class RoutePredicateFactoryUtils {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right place for this. After looking at the class, I'm going to remove it and move the one static method to the single class where it is used.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a class called RemoteAddressResolver.

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

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

I think the logic is straight forward enough. Needs some cosmetic changes.

* @param maxTrustedIndex correlates to the number of trusted proxies expected in front spring cloud gateway
* (index starts at 1).
*/
public static InetSocketAddress extractRemoteAddressFromXForwardedHeaderByMaxTrustedIndex(ServerWebExchange exchange, int maxTrustedIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

These could possible attached to an enum and therefore used in normal configuration of RemoteAddrRoutePredicateFactory

…d for extracting client IP with is safer from spoofing of X-Forwarded-For header.
@fitzoh
Copy link
Contributor Author

fitzoh commented Mar 19, 2018

Rebased and moved things out of RoutePredicateFactoryUtils.

There's now an RemoteAddressResolver interface, and two impls, default which just gets the client IP from the request, and an X-Forwarded-For based version.

The X-Forwarded-For uses the same logic around trusted number of infrastructure hops, and has two static constructor methods.
One takes a trusted index, and the other is a no-arg trust-all constructor...

Might need another round of polish, but wanted to check in after that rebase+refactor.

@spencergibb spencergibb merged commit d795344 into spring-cloud:master Mar 19, 2018
bijukunjummen pushed a commit to bijukunjummen/spring-cloud-gateway that referenced this pull request Jun 8, 2018
…-Forwarded-For` header. Addresses spring-cloud#155. (spring-cloud#156)

* Update `RemoteAddrRoutePredicateFactory` to optionally respect the `X-Forwarded-For` header. Fixes spring-cloudgh-155.

* Make function to extract client IP configurable. Add additional method for extracting client IP with is safer from spoofing of X-Forwarded-For header.
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