Support traefik forwardAuth #762

czunker · 2020-09-07T11:25:44Z

Add support for traefiks X-Forwarded headers:

X-Forwarded-Proto
X-Forwarded-Host
X-Forwarded-Uri
Use them to construct a redirect uri, when X-Auth-Request-Redirect
is not present or valid.

Use one endpoint for auth and sign_in, which is the way
thomseddon/traefik-forward-auth works.
This also allows to skip the auth provider button.

Fixes #46

Description

Motivation and Context

How Has This Been Tested?

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

czunker · 2020-09-07T11:33:47Z

This PR is missing tests and docs. I'll have a look at it the coming days.

Beanow · 2020-09-07T17:05:26Z

Glad to see this :]
In particular, having the Traefik header support is a big deal.

Seems like you're adding the absolute URL, have you tried if that allows for the "auth host mode" style?

Small detail on the new endpoint. It's called auth or start, but doesn't behave like start.
Rather it's most similar to a proxy of static://202, or of /oauth2/sign_in.
So possibly auth_or_sign_in is closer to existing endpoints.

An alternative approach I can think of is to add a config option so we can modify /oauth2/auth's behavior.
Something like --auth-endpoint-sign-in bool default false.

Render the login page when a request to /oauth2/auth is not authorized.

czunker · 2020-09-08T12:24:58Z

How I understand your comment in the issue, oauth2_proxy handles the requests in an "auth host mode".

From a clients perspective:

query to service url (service.example.com)
redirect to login provider (login.example.com)
redirect to auth (auth.example.com => this is the oauth2_proxy)
redirect to service (service.example.com)

Ornias1993

Besides two small notes it looks okey codewise.

That being said:
I prefer one word endpoints personally.

So I think @Beanow's suggestion would be cleaner:
An alternative approach I can think of is to add a config option so we can modify /oauth2/auth's behavior.

Something like --auth-endpoint-sign-in bool default false.

Ornias1993 · 2020-09-15T18:28:07Z

oauthproxy.go

@@ -658,6 +668,9 @@ func (p *OAuthProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		p.AuthenticateOnly(rw, req)
 	case path == p.UserInfoPath:
 		p.UserInfo(rw, req)
+	// originally contributed but not merged by: https://github.com/oauth2-proxy/oauth2-proxy/compare/master...maxlaverse:support_traefik?expand=1


This should not be in-code comment, but a PR comment

Ornias1993 · 2020-09-15T18:28:18Z

oauthproxy.go

@@ -840,6 +853,38 @@ func (p *OAuthProxy) SkipAuthProxy(rw http.ResponseWriter, req *http.Request) {
 	p.serveMux.ServeHTTP(rw, req)
 }

+// originally contributed but not merged by: https://github.com/oauth2-proxy/oauth2-proxy/compare/master...maxlaverse:support_traefik?expand=1


This should not be in-code comment, but a PR comment

NickMeves · 2020-09-19T16:53:51Z

oauthproxy.go

+	if !p.IsValidRedirect(redirect) {
+		redirect = req.Header.Get("X-Forwarded-Proto") + "://" + req.Header.Get("X-Forwarded-Host") + req.Header.Get("X-Forwarded-Uri")
+		logger.Printf("X-Forwarded-* headers combined: %s", redirect)
+	}
 	if req.Form.Get("rd") != "" {


Is Traefik not able to explicitly configure the redirect URL in the rd param? For instance, this is what my nginx ingress sign-in annotation looks like:

Internal:
nginx.ingress.kubernetes.io/auth-signin: https://<REDACTED_DOMAIN>/oauth2/start?rd=https%3A%2F%2F$host$escaped_request_uri

Behind Proxy w/X-Forwarded-Host:
nginx.ingress.kubernetes.io/auth-signin: https://<REDACTED_DOMAIN>/oauth2/start?rd=https%3A%2F%2F$http_x_forwarded_host$escaped_request_uri

It might with some custom config (just like we are able to do a lot of what this PR adds with custom traefik configs right now), but the whole goal of this PR is to simplify the process of using forward auth. having to manually set explicit url's really defeats that purpose.

AFAIK this isn't possible in Traefik's config. (Rather nginx's config is closer to a scripting language so I'm sure we'll be back here once Caddy ships a forward auth middleware.)

There are various feature requests and future plans that might allow this, but there's also a backwards compatibility issue. Supporting X-Forwarded-* would add compatibility for Traefik all the way down to v1.4+ (2017) if I'm not mistaken.

having to manually set explicit url's really defeats that purpose.

Do you mean statically configuring the rb= parameter? Sure, we need to set forwardauth.address anyway and could include ?rb=. But unlike nginx, we don't have variables available to inject URLs dynamically per-request.

That might work for the root of a static setup, but even if the domain and path prefixes would remain static, it wouldn't support different paths. If a client requested /api/version and instead ends up at / after auth because of the static config, I wouldn't consider that fully supported. Especially because this isn't the expected behavior when we're using X-Auth-Request-Redirect / ?rb= in other setups.

I was saying the proposed idea was a bad one, so i'm not really into debating how it could be done anyway.

Agreed @Ornias1993, sorry for preaching to the choir. I wanted make sure I understood you correctly, and to include some more specifics for anyone else to follow along in case they're less familiar with Traefik.

NickMeves · 2020-09-19T17:00:19Z

oauthproxy.go

+	logger.Printf("X-Auth-Request-Redirect: %s", redirect)
+	// traefik does not send X-Auth-Request-Redirect, so give the traefik headers a try in case X-Auth-Request-Redirect is not set
+	if !p.IsValidRedirect(redirect) {
+		redirect = req.Header.Get("X-Forwarded-Proto") + "://" + req.Header.Get("X-Forwarded-Host") + req.Header.Get("X-Forwarded-Uri")


Take a look at this PR, it added a unified way for X-Forwarded-Host vs Host usage. Let's use that helper here

Take a look at this PR

What PR are you talking about?

Is there a link missing? Not sure what helper you're pointing to.

Link is definitely missing 😅

#729

util.GetRequestHost is the function we pretty much aligned on everywhere instead of req.Host

NickMeves · 2020-09-19T17:26:50Z

oauthproxy.go

+		OAuthCallbackPath:       fmt.Sprintf("%s/callback", opts.ProxyPrefix),
+		AuthOnlyPath:            fmt.Sprintf("%s/auth", opts.ProxyPrefix),
+		UserInfoPath:            fmt.Sprintf("%s/userinfo", opts.ProxyPrefix),
+		AuthenticateOrStartPath: fmt.Sprintf("%s/auth_or_start", opts.ProxyPrefix), // originally contributed but not merged by: https://github.com/oauth2-proxy/oauth2-proxy/compare/master...maxlaverse:support_traefik?expand=1


This path structure doesn't feel elegant. I feel like a querystring parameter that triggers a fallback to the OAuthStart handler instead of a 401 Unauthorized in the failure case on /auth is more elegant.

E.e /oauth2/auth?start=true

Seems pretty clean as well. Though again technically it isn't like start which immediately redirects, it's more like sign_in which either shows the various providers available or redirects when --skip-provider-button=true.

One downside to this approach vs having a new endpoint or new config option I can imagine, is that if someone assumes this functionality is available but is running an older version where it isn't yet, you wouldn't get any warnings/errors on startup. It would silently ignore the parameter and have unexpected behavior during actual requests that need authenticating.

In that regard I think either a config option or new endpoint is more predictable.

Config option for the auth endpoint would be "the best of both worlds" in this case, imho.
It would keep the endpoints elegant and keeps everything nice and tidy...

Is the intent to fallback to sign-in or start page? I'm tempted to lean toward start to align with recommendations for other subrequest recommended architectures - SignInPage kills existing OAuth2-Proxy session cookies.

But I could be swayed, I see the utility if people like the sign-in page (I hate it, get me to my IdP 😄 )

Could be chosen based on the option querystring param proposal? ?unauthorized=(?:start|sign_in)

One downside to this approach vs having a new endpoint or new config option I can imagine, is that if someone assumes this functionality is available but is running an older version where it isn't yet, you wouldn't get any warnings/errors on startup. It would silently ignore the parameter and have unexpected behavior during actual requests that need authenticating.

This is a known issue in many areas we hope to address with restructuring our docs. With the current docs representing master, we frequently get users confused by following the for configuration options that have merges but aren't in a release yet.

So I wouldn't let this be concern for our design decision.

Config option for the auth endpoint would be "the best of both worlds" in this case, imho.
It would keep the endpoints elegant and keeps everything nice and tidy...

Didn't read this closely enough -- Agreed! Config option is best to give an alternative to /oauth2/auth failure behavior

NickMeves · 2020-09-19T17:31:40Z

oauthproxy.go

@@ -490,6 +493,12 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error)
 	}

 	redirect = req.Header.Get("X-Auth-Request-Redirect")
+	logger.Printf("X-Auth-Request-Redirect: %s", redirect)
+	// traefik does not send X-Auth-Request-Redirect, so give the traefik headers a try in case X-Auth-Request-Redirect is not set
+	if !p.IsValidRedirect(redirect) {


This should be after the rd querystring parameter codepath since that is the more standard and currently adopted path. This ordering will lead to confusing log messages for K8s nginx ingress users.

@NickMeves K8s NGINX ingress with traefik is afaik not actually being the adviced way of running traefik anymore.
(as in: It is still formally "supported", but pretty much depricated)

I meant for other non-traefik users -- If I use K8s nginx ingress with this PR using rd, this leads to a lot of superfluous and confusing log messages for me that aren't applicable.

I'd be tempted to say we just log what the final redirect URL is at the end once we settle on a mechanism to chose it.

Why can traefik not inject this header? Surely it should be able to configure this? If not, can it not inject a query string parameter?

Add support for traefiks X-Forwarded headers: - X-Forwarded-Proto - X-Forwarded-Host - X-Forwarded-Uri Use them to construct a redirect uri, when X-Auth-Request-Redirect is not present or valid. Use one endpoint for auth and sign_in, which is the way thomseddon/traefik-forward-auth works. This also allows to skip the auth provider button. Fixes oauth2-proxy#46

czunker · 2020-09-23T11:18:01Z

Hi,

thanks for the review. I think I've integrated all feedback. Please have a look.

linuxgemini · 2020-09-28T14:18:36Z

Are there any updates on this? I've been pushing away my Traefik v2 deployment due to the missing support for quite some time.

JoelSpeed

As far as I'm aware users of Traefik have managed to get this configured before without needing changes to OAuth2 Proxy, I left a couple of questions in place based on this

JoelSpeed · 2020-10-05T16:57:27Z

docs/configuration/configuration.md

@@ -24,6 +24,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
 | ------ | ---- | ----------- | ------- |
 | `--acr-values` | string | optional, see [docs](https://openid.net/specs/openid-connect-eap-acr-values-1_0.html#acrValues) | `""` |
 | `--approval-prompt` | string | OAuth approval_prompt | `"force"` |
+| `--auth-endpoint-sign-in` | bool | Show sign in page, when request to /oauth2/auth is not authorized | `false` |


What is the difference between having this and a static endpoint configured? I believe that was why the static upstream option was implemented in the first place

JoelSpeed · 2020-10-05T16:58:42Z

oauthproxy.go

@@ -490,6 +493,12 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error)
 	}

 	redirect = req.Header.Get("X-Auth-Request-Redirect")
+	logger.Printf("X-Auth-Request-Redirect: %s", redirect)
+	// traefik does not send X-Auth-Request-Redirect, so give the traefik headers a try in case X-Auth-Request-Redirect is not set
+	if !p.IsValidRedirect(redirect) {


Why can traefik not inject this header? Surely it should be able to configure this? If not, can it not inject a query string parameter?

linuxgemini · 2020-10-05T17:10:06Z

Why can traefik not inject this header? Surely it should be able to configure this? If not, can it not inject a query string parameter?

@JoelSpeed can't reply to your second review but to reply:

Traefik doesn't have dynamic variables to be used in the configuration, it is proposed last year (traefik/traefik#5036) but no change happened since then.

Although you can inject headers, you can't really dynamically change it.

github-actions · 2020-12-05T00:15:24Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

linuxgemini · 2020-12-05T09:57:33Z

Are there any updates on this?

/unstale

linuxgemini · 2020-12-16T17:50:02Z

The PR is stale for more than 3 months and during this time I've used this patched build. If a new release is near, I'll probably have to migrate to Cloudflare Access, which doesn't support GitLab Groups, or stay on the patched build.

Traefik upstream hasn't gotten any further on dynamic headers and although there's a plugin for X-Request-Start; this is not suitable for oauth2-proxy anyway.

NickMeves · 2020-12-16T18:16:28Z

We haven't heard responses to PR feedback and it needs a rebase.

None of the maintainers are Traefik users, so we don't have subject matter expertise to push this over the edge ourselves.

Truthfully, a detailed writeup of the problem and architecture with supporting documentation would be very helpful. For me personally, it's hard to judge the merit of some of these structural changes without understanding why Traefik needs them.

The new header behavior guiding redirects makes sense. But I don't understand what is driving implementing a new endpoint/functionality under /oauth2/*

linuxgemini · 2020-12-16T18:23:07Z

Understandable, I think I'll make a PR with just the header logic and provide a better write-up.

linuxgemini · 2020-12-17T15:11:57Z

#957 is created.

NickMeves · 2021-01-09T03:38:13Z

#957 merged which captured most of this logic.

pcneo83 · 2021-01-27T15:23:22Z

Is there any chance on reconsidering the missing functionality that hasn't been captured in #957 please? While X-Forwarded-* headers are now supported to make the redirects capture the path correctly, this does not work on the initial unauthenticated requests with Traefik. The dependency on errors middleware to capture 401 status and then initiate /oauth2/sign_in breaks the signin redirect in the browser (when --skip-provider-button is true).

The --auth-endpoint-sign-in option suggested in #762 (comment) and implemented in this PR is the only missing functionality and would greatly help in using Traefik FowardAuth using a single endpoint oauth2/auth and will also fix

--skip-provider-button redirect failure in the browser
missing unauthenticated request URI path in the redirect

NickMeves · 2021-01-29T23:06:24Z

@pcneo83 Can you make a feature request issue with what we are missing so we can track any future improvements that might be needed. Otherwise, this writeup you've generated will likely get lost in this closed PR.

czunker requested a review from a team as a code owner September 7, 2020 11:25

czunker marked this pull request as draft September 8, 2020 11:59

czunker marked this pull request as ready for review September 9, 2020 07:53

Ornias1993 reviewed Sep 15, 2020

View reviewed changes

NickMeves reviewed Sep 19, 2020

View reviewed changes

Christian Zunker added 2 commits September 23, 2020 13:16

Add docs

e908ca5

czunker force-pushed the support_traefik branch from 9651678 to 716a2a8 Compare September 23, 2020 11:16

czunker force-pushed the support_traefik branch 2 times, most recently from 67b3782 to 940b1c1 Compare September 23, 2020 11:37

Add tests

0674c95

czunker force-pushed the support_traefik branch from 940b1c1 to 0674c95 Compare September 23, 2020 12:02

JoelSpeed reviewed Oct 5, 2020

View reviewed changes

JoelSpeed mentioned this pull request Oct 12, 2020

Redirect to /oauth2/auth despite OAUTH2_PROXY_PROXY_PREFIX being set #815

Closed

github-actions bot added the Stale label Dec 5, 2020

JoelSpeed removed the Stale label Dec 5, 2020

linuxgemini mentioned this pull request Dec 16, 2020

Use X-Forwarded-{Proto,Host} on redirect as last resort #957

Merged

3 tasks

NickMeves closed this Jan 9, 2021

linuxgemini mentioned this pull request Jan 30, 2021

[Feature Request] Allow auth endpoint to serve sign_in pages as well #1015

Closed

DanielHabenicht mentioned this pull request Jun 25, 2021

Traefik Forward and OAuth2-Proxy with skip-provider-button=true do not redirect to initial page #1255

Closed

Support traefik forwardAuth #762

Support traefik forwardAuth #762

Conversation

czunker commented Sep 7, 2020 • edited

Description

Motivation and Context

How Has This Been Tested?

Checklist:

czunker commented Sep 7, 2020

Beanow commented Sep 7, 2020

czunker commented Sep 8, 2020

Ornias1993 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beanow Sep 20, 2020 • edited

Choose a reason for hiding this comment

Beanow Sep 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMeves Sep 20, 2020 • edited

Choose a reason for hiding this comment

NickMeves Sep 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ornias1993 Sep 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czunker commented Sep 23, 2020

linuxgemini commented Sep 28, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linuxgemini commented Oct 5, 2020 • edited

github-actions bot commented Dec 5, 2020

linuxgemini commented Dec 5, 2020

linuxgemini commented Dec 16, 2020

NickMeves commented Dec 16, 2020

linuxgemini commented Dec 16, 2020

linuxgemini commented Dec 17, 2020

NickMeves commented Jan 9, 2021

pcneo83 commented Jan 27, 2021

NickMeves commented Jan 29, 2021

czunker commented Sep 7, 2020 •

edited

Beanow Sep 20, 2020 •

edited

Beanow Sep 20, 2020 •

edited

NickMeves Sep 20, 2020 •

edited

NickMeves Sep 19, 2020 •

edited

Ornias1993 Sep 20, 2020 •

edited

linuxgemini commented Oct 5, 2020 •

edited