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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

When using a Load Balancer, mTLS and a route service, I need my apps to receive the X-Forwarded-Client-Cert header #203

Closed
46bit opened this issue May 10, 2021 · 3 comments

Comments

@46bit
Copy link
Contributor

46bit commented May 10, 2021

馃憢馃徎 We're reporting an issue that we may want to contribute a PR to fix. Would like some feedback on our suggested fix.

Issue

Our platform terminates TLS before reaching Gorouter. We provide the X-Forwarded-Client-Cert header to Gorouter. We want Gorouter to trust the X-Forwarded-Client-Cert header it receives.

Our user needs to authenticate requests to their apps. They are using Mutual TLS. They also want to put a Route Service in front of their app.

Gorouter deletes the X-Forwarded-Client-Cert header before it reaches the app.

Affected Versions

Present in current main (6f3027928ec24bee0617231bf496f624b37c8f4a.)

Context

We have Gorouter configured in router.forwarded_client_cert: always_forward mode. This is supposed to:

Always forward the XFCC header in the request, regardless of whether the client connection is mTLS.
Use this value when your load balancer is forwarding the client certificate and requests are not forwarded to Gorouter over mTLS.

The relevant code is https://github.com/cloudfoundry/gorouter/blob/379860daa83a162ffe0b6039eafb7c8bfa1eaccf/handlers/clientcert.go#L57-L70 and https://github.com/cloudfoundry/gorouter/blob/379860daa83a162ffe0b6039eafb7c8bfa1eaccf/proxy/proxy.go#L216-L224.

The critical line is https://github.com/cloudfoundry/gorouter/blob/379860daa83a162ffe0b6039eafb7c8bfa1eaccf/proxy/proxy.go#L222. When a request comes from a Route Service, this code forces removal of X-Forwarded-Client-Cert even if Gorouter is in always_forward mode.

This code was introduced in 2017 in https://www.pivotaltracker.com/story/show/153524695/comments/188564963. There seems to have been non-specific concerns around non-HTTPS traffic that led to this implementation.

We believe this does not weaken any security protections. By using always_forward you already trust the client (i.e. the Load Balancer.) In this codepath there is also protection from verification of the Route Service (using X-Cf-Proxy-Signature.)

Traffic Diagram

We have three requests going on:

  1. The end user's request: End user -> NLB -> Haproxy -> Gorouter
  2. Gorouter making an external request to the route service: Gorouter -> NLB -> Haproxy -> Gorouter -> Route service app
  3. The route service making an external request to the app: Route service app -> NLB -> Haproxy -> Gorouter -> App

X-Forwarded-Client-Cert is being removed by Gorouter in request 3.

Steps to Reproduce

This is very hard to summarise. I hope the issue is clear from the code and description above.

Expected result

X-Forwarded-Client-Cert is received by the route service and the app.

Current result

X-Forwarded-Client-Cert is received by the route service, but not the app.

Suggested Fix

Our user's route service could copy the contents of X-Forwarded-Client-Cert into a custom header name that would reach their app.

We would rather fix this issue within Gorouter. We're interested in contributing a PR. I think the best solution is to alter that one line of code to allow always_forward.

Changing https://github.com/cloudfoundry/gorouter/blob/379860daa83a162ffe0b6039eafb7c8bfa1eaccf/proxy/proxy.go#L222 into:

return valid && forwardedClientCert != config.SANITIZE_SET && forwardedClientCert != config.ALWAYS_FORWARD, nil
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@46bit
Copy link
Contributor Author

46bit commented May 12, 2021

I have opened a PR that implements the change we're suggesting: cloudfoundry/gorouter#281

@46bit
Copy link
Contributor Author

46bit commented May 25, 2021

cloudfoundry/gorouter#281 has been merged, so this issue can be closed once that's shipped in routing-release

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

No branches or pull requests

2 participants