Forward Authentication: add X-Forwarded-Uri #2398
Conversation
juliens
changed the title
juliens
added
kind/enhancement
ldez
changed the title
| XForwardedHost = "X-Forwarded-Host" | ||
| XForwardedPort = "X-Forwarded-Port" | ||
| XForwardedServer = "X-Forwarded-Server" | ||
| XForwardedURI = "X-Forwarded-Uri" |
There was a problem hiding this comment.
It's not authorized to manually change files in /vendor.
Please move this change into middlewares/auth/forward.go
middlewares/auth/forward.go
Outdated
| @@ -122,4 +122,8 @@ func writeHeader(req *http.Request, forwardReq *http.Request, trustForwardHeader | |||
| } else { | |||
| forwardReq.Header.Del(forward.XForwardedHost) | |||
| } | |||
|
|
|||
| if forwardURI { | |||
There was a problem hiding this comment.
Could you use the same process as others header?
And remove the forwardURI option
if xfURI := req.Header.Get(XForwardedURI); xfURI != "" && trustForwardHeader {
forwardReq.Header.Set(XForwardedURI, xfURI)
} else if req.URL.RequestURI() != "" {
forwardReq.Header.Set(XForwardedURI, req.URL.RequestURI())
} else {
forwardReq.Header.Del(XForwardedURI)
}
types/types.go
Outdated
| @@ -353,6 +353,7 @@ type Forward struct { | |||
| Address string `description:"Authentication server address"` | |||
| TLS *ClientTLS `description:"Enable TLS support" export:"true"` | |||
| TrustForwardHeader bool `description:"Trust X-Forwarded-* headers" export:"true"` | |||
| ForwardURI bool `description:"Forward requested URI to authenticator"` | |||
There was a problem hiding this comment.
Could you remove this option? see my previous comment.
middlewares/auth/forward_test.go
Outdated
| @@ -162,6 +162,7 @@ func Test_writeHeader(t *testing.T) { | |||
| name string | |||
| headers map[string]string | |||
| trustForwardHeader bool | |||
| forwardUri bool | |||
There was a problem hiding this comment.
Could you remove this? see my previous comment.
docs/configuration/entrypoints.md
Outdated
| # Optional | ||
| # Default: false | ||
| # | ||
| forwardUri = true |
There was a problem hiding this comment.
Could you remove this option? see my next comment.
middlewares/auth/forward_test.go
Outdated
|
|
||
| writeHeader(req, forwardReq, test.trustForwardHeader) | ||
|
|
There was a problem hiding this comment.
Of course. Sorry for this.
Can you tell me which tool will handle such formatting? go fmt does not. Or was it just manual checking? Thanks.
There was a problem hiding this comment.
It's gofmt on our CI: https://semaphoreci.com/containous/traefik/branches/pull-request-2398/builds/3
In local you can do make validate.
There was a problem hiding this comment.
Thank you. I see and will check all those validation tests you integrated. Sadly validate-gofmt doesn't producde the error when the blank line is missing (Go 1.9.2).
I will check those validation results on GitHub next time.
middlewares/auth/forward.go
Outdated
| @@ -12,6 +12,10 @@ import ( | |||
| "github.com/vulcand/oxy/utils" | |||
| ) | |||
|
|
|||
| const ( | |||
| XForwardedURI = "X-Forwarded-Uri" | |||
There was a problem hiding this comment.
Could you rename to xForwardedURI?
|
@SebastianBauer Could you please rebase your PR on master ? |
ldez
added
contributor/waiting-for-corrections
bot/no-merge
and removed
contributor/waiting-for-corrections
bot/no-merge
labels
ldez
added
status/3-needs-merge
and removed
bot/no-merge
contributor/waiting-for-corrections
labels
What does this PR do?
Added requested URI to auth server for Forward Authentication.
Motivation
Missing feature for authentication decision based on the URI (frontend).
More
Added implementation, docs and test.
Additional Notes
Could be good enough for #2162 as well.