-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support Traefik ForwardAuth without a 401 handler #1023
Support Traefik ForwardAuth without a 401 handler #1023
Conversation
bd4b8cb
to
8d8a874
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.
This is an interesting idea, left a couple of questions. I'm not hugely keen on this overloading of the authonly endpoint, it would be better if we could change the behaviour to actually redirect rather than rendering the page, is that possible?
8d8a874
to
148a82f
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 definitely prefer the redirect option as it's implemented now. Did you test this and it worked as expected?
@NickMeves what do you think to this approach to allow the flexibility for traefik?
Just thinking about this, have you tried to use a static upstream as was introduced in #265? The default behaviour for accessing any endpoint that is not authenticated is to return the sign in page, so I think you may be able to configure this for your needs |
redirect option makes sense as its handling the case at a HTTP level, however, it would also mean duplicating the code to keep track of the original request, before its redirected to https://oauth2-proxy-service.example.com/oauth2/(sign_in|start). If not, the original request is lost and the sign_in|start logic will only redirect back to https://oauth2-proxy-service.example.com once OAuth2 completes |
I might have misunderstood how to use the upstream functionality but I did try with
If the original service endpoint is also added to list of |
4284db4
to
bdb3201
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.
The example given on docs should be re-written with the File Dynamic Configuration Reference in mind.
The rules can be handled better as well.
docs/docs/configuration/overview.md
Outdated
```yaml | ||
a-service: | ||
routes: | ||
- kind: Rule | ||
match: Host(`a-service.example.com`) && PathPrefix(`/`) | ||
middlewares: | ||
- name: oauth-auth-redirect # redirects all unauthenticated to oauth2 signin | ||
services: | ||
- name: a-service-backend | ||
port: 80 | ||
- kind: Rule | ||
match: Host(`a-service.example.com`) && PathPrefix(`/no-auto-redirect`) | ||
middlewares: | ||
- name: oauth-auth-wo-redirect # unauthenticated session will return a 401 | ||
services: | ||
- name: a-service-backend | ||
port: 80 | ||
- kind: Rule | ||
match: Host(`a-service.example.com`) && PathPrefix(`/oauth2`) # Make oauth2-proxy handle all /oauth2 calls on a-service.example.com | ||
middlewares: | ||
- name: auth-headers | ||
services: | ||
- name: oauth-proxy-backend | ||
port: 80 | ||
b-service: | ||
routes: | ||
- kind: Rule | ||
match: Host(`b-service.example.com`) && PathPrefix(`/`) | ||
middlewares: | ||
- name: oauth-auth-redirect # redirects all unauthenticated to oauth2 signin | ||
services: | ||
- name: b-service-backend | ||
port: 80 | ||
- kind: Rule | ||
match: Host(`b-service.example.com`) && PathPrefix(`/oauth2`) # Make oauth2-proxy handle all /oauth2 calls on a-service.example.com | ||
middlewares: | ||
- name: auth-headers | ||
services: | ||
- name: oauth-proxy-backend | ||
port: 80 | ||
oauth-proxy: | ||
routes: | ||
- kind: Rule | ||
match: Host(`oauth-proxy.example.com`) && PathPrefix(`/`) | ||
middlewares: | ||
- name: auth-headers | ||
services: | ||
- name: oauth-proxy-backend | ||
port: 80 | ||
|
||
middlewares: | ||
oauth-auth-wo-redirect: | ||
forwardAuth: | ||
address: 'https://oauth-proxy.example.com/oauth2/auth' | ||
trustForwardHeader: true | ||
authResponseHeaders: | ||
- X-Auth-Request-Access-Token | ||
- Authorization | ||
oauth-auth-redirect: | ||
forwardAuth: | ||
address: 'https://oauth-proxy.example.com/oauth2/auth?redirect_signin=true' | ||
trustForwardHeader: true | ||
authResponseHeaders: | ||
- X-Auth-Request-Access-Token | ||
- Authorization | ||
auth-headers: | ||
headers: | ||
sslRedirect: true | ||
stsSeconds: 315360000 | ||
browserXssFilter: true | ||
contentTypeNosniff: true | ||
forceSTSHeader: true | ||
sslHost: example.com | ||
stsIncludeSubdomains: true | ||
stsPreload: true | ||
frameDeny: true | ||
``` |
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.
This example does not utilize the File Dynamic Configuration Reference which is shown on Traefik Docs: https://doc.traefik.io/traefik/reference/dynamic-configuration/file/
Different config styles documented for the same software target can create confusion.
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.
It was easier to use my existing Kubernetes provider configuration as the examples at the time, but have now updated to use Dynamic File Provider. Is it worth also keeping the Kubernetes Provider CRD example here or somewhere else as its not always straightforward to switch between different configuration providers easily?
Take a look at this writeup on the nginx ingress configuration of this: https://kubernetes.github.io/ingress-nginx/examples/auth/oauth-external-auth/ Particularly here:
Based on a failure case, could you set a parameter on the I lean toward making this is broad as possible so other systems or use cases could use it and not tailoring the solution purely to traefik ease of use since it likely only a small minority of the reverse proxies that sit in front of OAuth2 Proxy. |
If you mean to update Traefik ForwardAuth middleware address endpoint to automatically set the We are currently setting the middleware to below and configuring any service route path that needs to be protected by authentication with the below middleware configuration. Even though each service and its path can be configured with the middleware, due to the lack of dynamic variable support, its not possible to use and replace variables
|
I have digged around and have seen that Traefik doesn't pass through redirect responses of error middlewares correctly and mashes up the 302 response with a 401 with the "Found" page and with a Location header. This is likely a Traefik bug or an implementation choice. I have opened traefik/traefik#7872 to investigate. |
e1fa084
to
3111ce0
Compare
@NickMeves, @JoelSpeed appreciate any further thoughts/concerns on this please? Have resolved the points raised code wise and have also updated the tests. |
3d1caa1
to
8ad73bb
Compare
oauthproxy.go
Outdated
|
||
// Orignal request URI is captured and added using `rd` parameter in the redirect | ||
// to ensure a redirect to the original URL will still happen after OAuth/Signin process | ||
redirect, err := p.getAppRedirect(req) |
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.
getAppRedirect
has a lot of logic paths to figure out where the app redirect goes to -- In traefik will this come from the X-Forwarded-(Proto|Host|Uri)
headers?
If so and we then map them to rd
on the SignIn handler -- that is acceptable (or is traffic still sending the same X headers to the sign in..?)
FYI - rd
takes priority over everything else -- so this logic seems good to me. I just wanted to talk it out in case there's unintentional mixing of logic.
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.
Yes, Traefik uses the X-Forwarded-(Proto|Host|Uri)
headers to provide the request URL information. The getAppRedirect
seems to handle all the various cases nicely and the usage of rd
parameter seemed like the best case here to preserve the original request URL
The flow is:
- original unauthenticated request comes in to
https://a-service.example.com/
- Traefik ForwardAuth middleware forwards this request to
https://oauth2-proxy.example.com/oauth2/auth?redirect_signin=true
- As per new logic, the original request URL is captured in
rd=https://a-service.example.com/
and added as a QS to/oauth2/sign_in
redirect path with a 302 redirect returned to the client
oauthproxy.go
Outdated
|
||
// SignIn has the check on SkipProviderButton to do either SigninPage or OauthStart | ||
redirectPath := fmt.Sprintf("%s?rd=%s", p.SignInPath, redirect) | ||
http.Redirect(rw, req, redirectPath, http.StatusFound) |
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.
Do you need the full URL here?
E.g. -- what happens if this is used in a system hitting this from a subrequest auth check where the OAuth2-Proxy server is hosted on a different subdomain then the requested resource?
In that case, the redirect to the relative path will be at the application domain - not the separate oauth2-proxy server. (this separate central oauth2-proxy auth server hit via subrequests on a different subdomain is a common architecture deployment).
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.
And then how do we accurately know the appropriate Proto/Host
details to set? If this endpoint is hit via subrequests, it might not have X-Forwarded
details or be on different internal domain (and therefore different HTTP Host header).
For instance, when I use this architecture, I do subrequests to this endpoint over the Kubernetes service URL directly (e.g. deployment.namespace.svc.cluster.local
) to avoid the extra hop of going though the Ingress layer internally.
The views the /oauth2/auth
endpoint has of its requestutil.GetRequestHost(req)
for those requests are not what is accessible externally.
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.
Upon further reflection -- this problem will cause you to get misleading results for the p.getAppRedirect(req)
call above to try to set an rd
querystring as well 🤔
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 think we might need to revisit redirect_signin
not being a bool, but rather being an explicit URL to the sign in endpoint with the proto/host/path all set in your configuration.
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.
Apologies but I quite don't understand by what you mean by subrequests
but perhaps an example would help me. However, interesting that you mention bypassing ingress and the redirect logic depends upon ingress configuration.
For instance, when I use this architecture, I do subrequests to this endpoint over the Kubernetes service URL directly (e.g. deployment.namespace.svc.cluster.local) to avoid the extra hop of going though the Ingress layer internally
Trying to understand how this looks for real requests, an end to end example of what the initial request is and how you bypass the ingress and make the /oauth2/auth
request to the kubernetes internal endpoints would be useful. We do have to note that since redirect_signin
is a boolean flag (for now), you could still switch off this behaviour by setting it to false when using internal endpoints?
I think we might need to revisit
redirect_signin
not being a bool, but rather being an explicit URL to the sign in endpoint with the proto/host/path all set in your configuration.
I possibly didn't get your point correctly but any explicit configuration will not be possible to set/use when using ingress like Traefik
Ok cool, glad that it is working now! Can we get this PR updated to update the docs and drop the code changes as they are unnecessary right? |
12e83fb
to
2a0c438
Compare
Docs changes look good as I understand them, are these also relevant for the 7.0.x and 6.1.x docs branches? Do we need to backport them? You may want to add a changelog entry as well but I'll leave that up to you, |
I have just tested with v6.1.1 version and this solution does not seem to work. It works with v7.0.0 however. I'll update the changelog and push, thanks |
…2/auth' endpoint
- remove commented out alternative solutions and debug log statements
- Add doc on using static upstream with Traefik ForwardAuth middleware
Given that this works with v7, would you mind copying the changes into https://github.com/oauth2-proxy/oauth2-proxy/blob/master/docs/versioned_docs/version-7.0.x/configuration/overview.md which is for the v7.0.x release? Also, there seems to be a changelog conflict, might need a rebase? |
8c57e3d
to
409073d
Compare
Missed the versioned docs, all updated now |
Sorry, to keep the changes going forward, they need to be in the main docs folder (where you had it originally) and the versioned ones, could you re-add it to the other file please? |
ah, my mistake, didn't realise both places need to be updated. I have updated both the docs now |
sans: | ||
- "*.example.com" | ||
oauth: | ||
rule: "Host(`a-service.example.com`, `b-service.example.com`, `oauth.example.com`) && PathPrefix(`/oauth2/`)" |
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.
Where's oauth.example.com/
handled?
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.
🤦 , looks like I missed it during the refactor, added it now, thanks
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 think this is good, @linuxgemini were you happy with the changes?
@JoelSpeed LGTM as well |
* oauth2-proxyGH-1015 Adds support for Traefik to OauthStart on '/oauth2/auth' endpoint * Fix incorrect reference to signout path and point to signin path - remove commented out alternative solutions and debug log statements * Remove skip provider button check as SignIn method already does this * Updated traefik example to match existing file configuration reference, updated tests * Update doc and refactor nested conditional statements * Revert code changes as static upstream provides the same functionality - Add doc on using static upstream with Traefik ForwardAuth middleware * update changelog * Move the doc changes to 7.0.x versioned docs * Re-add traefik docs update in the main docs overview.md * add missing oauth2-proxy routing Co-authored-by: Praveen Chinthala <[email protected]>
Adds support for Traefik ForwardAuth middleware without the need to handle 401 unauthorized response using an
errors
middleware/oauth2/auth
endpoint with a QS boolean parameter to start Oauth2 process when required--skip-provider-button
is enabledFixes #1015
Description
Motivation and Context
How Has This Been Tested?
Checklist: