Add forward authentication option #1972

drampelt · 2017-08-19T04:36:02Z

Description

This implements forward authentication, much like in ngx_http_auth_request_module.

Based on the discussion in #1030 and Proposal Doc, it's clear we needed to start with something simple and then build on it later. This implementation is a simplified version of @vladshub's implementation in #1030, removing the request and response body header/query parameter manipulation.

When forward auth is enabled, the request is first forwarded to the specified auth server. If the auth server responds with a 2XX status, the original request is performed. Otherwise, the response from the auth server is returned.

Next steps

If everyone agrees this is a good starting point, I think the next step would be either copying or renaming response headers from the auth server as request headers sent to the backend.

From what I can tell that seems to be the main use-case, the auth server provides authenticated user information in response headers and then the backend application uses that information to automatically authenticate the user. See oauth2_proxy with nginx as an example of this. I believe this would bring it to feature parity with ngx_http_auth_request_module.

See this commit for a potential implementation of header mapping.

Of course I'm open to discussion about how to proceed and interested in hearing other ideas.

emilevauge · 2017-08-21T21:41:05Z

Wow, great job @drampelt !
Design LGTM :)

StevenACoffman

Nice improvement!

emilevauge

Hey @drampelt, great job!
I made few comments though.

emilevauge · 2017-08-22T19:58:57Z

auth/forward.go

+		return
+	}
+
+	defer forwardResponse.Body.Close()


I would move this line after the block:

if readError != nil { ... } defer forwardResponse.Body.Close()

Indeed, if readError is not nil, maybe the body will be empty, then the defered Close() would lead to a panic.

emilevauge · 2017-08-22T20:00:49Z

types/types.go

@@ -314,6 +315,11 @@ type Digest struct {
 	UsersFile string
 }

+// Forward authentication
+type Forward struct {
+	Address string


I think it would be great to add the possibility to connect in TLS (with an self-signed cert).

I'm agree with @emilevauge for the TLS connection.
Indeed, for example, I tried to test the PR by connecting Traefik to a docker_auth server but this server allows only HTTPS connections...

Yes I agree that would be useful. Unfortunately I'm not too familiar with Go yet and not really sure how to go about doing that - would anyone else be interested in implementing that?

No problem @drampelt.
We can do the modifications for your 😉

ldez

A quick review.

ldez · 2017-08-22T08:16:56Z

auth/forward.go

+func Forward(forward *types.Forward, w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
+	client := http.Client{}
+
+	forwardReq, err := http.NewRequest("GET", forward.Address, nil)


"GET" -> http.methodGet

ldez · 2017-08-22T10:42:31Z

auth/forward.go

+		return
+	}
+
+	if forwardResponse.StatusCode < 200 || forwardResponse.StatusCode >= 300 {


Could you replace:

200 by http.StatusOK

300 by http.StatusMultipleChoices

maybe the limit can be 400 instead of 300?

I chose 300 to allow the auth server to redirect to a login page if necessary. For example if doing Google sign in, the auth server would need to redirect the user to Google by returning a 302.

ldez · 2017-08-22T10:44:46Z

middlewares/authenticator.go

-	"github.com/abbot/go-http-auth"
+	goauth "github.com/abbot/go-http-auth"
+
+	"github.com/containous/traefik/auth"


could you sort imports?

Sorry, could you clarify what order they should be in? I'm not too familiar with Go yet and gofmt seems to think that is fine.

emilevauge · 2017-08-23T16:12:24Z

@drampelt The code freeze for 1.4 will happen in 2 days (August 25th). Do you think you will be able to fix your PR in the meantime ?

drampelt · 2017-08-24T02:20:57Z

Thanks for the reviews, I have a fairly busy week but I'll see if I can get it fixed up by tomorrow.

drampelt · 2017-08-25T02:18:20Z

Fixed most of the issues I knew how to.

timoreimann

Marathon part LGTM.

nmengin

Many thanks for this really useful PR.
LGTM 👏 👏

emilevauge

Well done @drampelt and @nmengin 👏
LGTM

ldez

LGTM

…ndencies.

RC1140 · 2017-09-18T07:52:35Z

Hi
Any idea how to use this, I grabbed the latest RC and made the config changes as per the doc update but I never get redirected.

I used github as an example to test

defaultEntryPoints = ["http"]

[entryPoints]
    [entryPoints.http]
    address = ":80"
      [entryPoints.http.auth.forward]
      address = "http://github.com/login/oauth/authorize"

[web]
address = ":8081"

[file]

[backends]
  [backends.backend1]
    [backends.backend1.servers.server1]
       url = "http://localhost:8000"

[frontends]
  [frontends.frontend1]
      backend = "backend1"
      passHostHeader = true
      [frontends.frontend1.routes.example]
          rule = "Host:localhost"

If I try to hit localhost i simply hit my local server on port 8000 with no auth request.

ldez · 2017-09-18T08:43:07Z

@RC1140 Thanks for your interest in Traefik 😃

Please discuss this in :

Stack Overflow using the "traefik" tag
the Traefik community Slack channel

traefiker added area/middleware size/M status/0-needs-triage labels Aug 19, 2017

drampelt mentioned this pull request Aug 19, 2017

Add a forward authentication option v2 #1030

Closed

ldez added area/authentication status/1-needs-design-review and removed status/0-needs-triage labels Aug 19, 2017

emilevauge added status/2-needs-review and removed status/1-needs-design-review labels Aug 21, 2017

emilevauge added this to the 1.4 milestone Aug 21, 2017

StevenACoffman approved these changes Aug 22, 2017

View reviewed changes

emilevauge requested changes Aug 22, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Aug 22, 2017

ldez suggested changes Aug 22, 2017

View reviewed changes

drampelt force-pushed the forward-auth branch from 1b2d603 to 9556e33 Compare August 25, 2017 02:13

nmengin force-pushed the forward-auth branch from 9556e33 to e108a4d Compare August 25, 2017 14:30

drampelt requested review from a team as code owners August 25, 2017 14:30

timoreimann approved these changes Aug 25, 2017

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Aug 25, 2017

nmengin approved these changes Aug 25, 2017

View reviewed changes

emilevauge approved these changes Aug 25, 2017

View reviewed changes

ldez approved these changes Aug 25, 2017

View reviewed changes

ldez added status/3-needs-merge and removed status/2-needs-review labels Aug 25, 2017

traefiker added the status/4-merge-in-progress label Aug 25, 2017

drampelt and others added 4 commits August 25, 2017 15:42

Add forward authentication option

77a21f6

Adding TLS configuration to authentication forward

4aaa011

Move TLSConfig from provider to types in the way to avoid cyclic depe…

8a5707f

…ndencies.

Fix formatting

26f7121

traefiker force-pushed the forward-auth branch from d25a773 to 26f7121 Compare August 25, 2017 15:42

traefiker removed the status/4-merge-in-progress label Aug 25, 2017

traefiker merged commit 52b69fb into traefik:master Aug 25, 2017

traefiker removed the status/3-needs-merge label Aug 25, 2017

ldez added the kind/enhancement a new or improved feature. label Aug 28, 2017

traefik locked and limited conversation to collaborators Sep 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add forward authentication option #1972

Add forward authentication option #1972

drampelt commented Aug 19, 2017 •

edited

Loading

emilevauge commented Aug 21, 2017

StevenACoffman left a comment

emilevauge left a comment

emilevauge Aug 22, 2017

emilevauge Aug 22, 2017

nmengin Aug 23, 2017 •

edited

Loading

drampelt Aug 25, 2017

nmengin Aug 25, 2017

ldez left a comment

ldez Aug 22, 2017

ldez Aug 22, 2017

ldez Aug 22, 2017

drampelt Aug 25, 2017

ldez Aug 22, 2017

drampelt Aug 25, 2017

emilevauge commented Aug 23, 2017

drampelt commented Aug 24, 2017 •

edited

Loading

drampelt commented Aug 25, 2017

timoreimann left a comment

nmengin left a comment

emilevauge left a comment

ldez left a comment

RC1140 commented Sep 18, 2017 •

edited by ldez

Loading

ldez commented Sep 18, 2017

Add forward authentication option #1972

Add forward authentication option #1972

Conversation

drampelt commented Aug 19, 2017 • edited Loading

Description

Next steps

emilevauge commented Aug 21, 2017

StevenACoffman left a comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmengin Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge commented Aug 23, 2017

drampelt commented Aug 24, 2017 • edited Loading

drampelt commented Aug 25, 2017

timoreimann left a comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

RC1140 commented Sep 18, 2017 • edited by ldez Loading

ldez commented Sep 18, 2017

drampelt commented Aug 19, 2017 •

edited

Loading

nmengin Aug 23, 2017 •

edited

Loading

drampelt commented Aug 24, 2017 •

edited

Loading

RC1140 commented Sep 18, 2017 •

edited by ldez

Loading