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

Add X-Skipper-Redirect-Base-Uri override headers for oauthgrant filter redirect_uri #3228

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

czhou-brex
Copy link
Contributor

@czhou-brex czhou-brex commented Sep 16, 2024

Add an override request header for the base URL of the redirect_uri param. Fallback to the req.Host by default.

Testing:

  • oauthGrant without the override header
  • oauthGrant with the override header

Issue:
#3229

@czhou-brex czhou-brex marked this pull request as ready for review September 16, 2024 17:16
@szuecs
Copy link
Member

szuecs commented Sep 16, 2024

The problem with override by header is that it can easily lead to a security issue (example).
I think we need a different implementation if that's needed. A trigger from a client can not be the way we want to have it.

@AlexanderYastrebov
Copy link
Member

Please describe the problem it intends to solve (maybe raise a ticket).

@czhou-brex
Copy link
Contributor Author

The problem with override by header is that it can easily lead to a security issue (example). I think we need a different implementation if that's needed. A trigger from a client can not be the way we want to have it.

It is true the client can now pass an arbitrary value, but ultimately the IDP will decide which redirect_uri values can be or cannot be allowed.

@kliakhovskii-brex
Copy link

Please describe the problem it intends to solve (maybe raise a ticket).

Will prob follow up on @czhou-brex behalf for now as we're trying to use for oidc/auth-grant authN for our services.

The problems they're running into is having another proxy in front of Skipper. In our case it's a CloudFront that caches some resources, so the routing kind of looks like the following:

  • user loads a browser page
  • it goes to... let's say cloudfront.brex.com
  • then it has Skipper endpoint as origin, let's say skipper.brex.com
  • if authGrant needs to redirect users to identity provider, it generates the redirect_uri out of the current host (which is skipper.brex.com) but this is not visible host to the user.
  • Ideally what we want is to have a redirect_uri that will go back to smth like cloudfront.brex.com/oauth/callback

So it's basically how to redirect to the top-most proxy in the chain when Skipper host isn't what directly visible to users due to smth like CDN.

PS Another potential use case we're trying to figure out is localhost experience for things like webpack dev server, technically it is an acting proxy in front of Skipper too, and it can even do smth like localhost:3000/oauth/callback redirect, and set a cookie for localhost too. But this is secondary right now (though curious to hear your thoughts on this).

@AlexanderYastrebov
Copy link
Member

  • it goes to... let's say cloudfront.brex.com
  • then it has Skipper endpoint as origin, let's say skipper.brex.com

Is it possible to configure first proxy to pass "cloudfront.brex.com" as a Host header?

The filter will substitute the base URL of redirect_uri, if "X-Skipper-Redirect-Base-Uri" header is passed in the request.

I am not sure I understand who is supposed to send this header? Likely its not browser but first proxy, or?

@kliakhovskii-brex
Copy link

Is it possible to configure first proxy to pass "cloudfront.brex.com" as a Host header?

Yes it's doable, but then Skipper has to have routing rules for each external host. For things like localhost (or any local custom hosts e.g. dev.example.com that points to your localhost) i find it somewhat weird.

I am not sure I understand who is supposed to send this header? Likely its not browser but first proxy, or?

Yes, I would expect a first proxy to pass it somewhat.

What's interesting, @mmichels-brex was testing it with both authGrant & oidc filters, and the latter seem to be going back to the right host. This is one of the reasons I believe we will likely stick to oidc & just finalize filters for logout (as they practically don't exist on Skipper today).

@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented Sep 19, 2024

I think the "standard" way is to use X-Forwarded-Host header.

In case of oauthGrant its not enough to patch redirect URI because request domain is also used to configure token cookie (see uses of extractDomainFromHost).

If we move towards passing host from upstream proxy then I think we should think about using X-Forwarded-Host to override incoming request.Host via middleware before routing. And also investigate how other proxies solve the issue.

What's interesting, @mmichels-brex was testing it with both authGrant & oidc filters, and the latter seem to be going back to the right host.

oid seems to also consider "host" header (see #1016) but I think this is not correct because go HTTP server removes Host header and puts its value to the request.Host:

// For incoming requests, the Host header is promoted to the
// Request.Host field and removed from the Header map.

So maybe if oidc route has setRequestHeader("Host", "cloudfront.brex.com") then it works (I don't know how your route that works looks like).

See also recent Host header related discussion #2038

@AlexanderYastrebov
Copy link
Member

Yes it's doable, but then Skipper has to have routing rules for each external host. For things like localhost (or any local custom hosts e.g. dev.example.com that points to your localhost) i find it somewhat weird.

Why? Maybe you could provide your example routes for better understanding?
By default oauthGrant and oidc filters will derive domain from req.Host which comes from Host header.
So if upstream proxy sets proper Host header everything should work out of the box.

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.

4 participants