-
Notifications
You must be signed in to change notification settings - Fork 5k
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 a forward authentication option v2 #1030
Conversation
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.
Hey @vladshub, thanks a lot :)
I have few comments before merging:
- What format is
{}
used inlog.*
? - Could you add some documentation in
doc/toml.md
? - Could you squash your commits ?
/cc @containous/traefik @diegooliveira
|
||
data := make(map[string]interface{}) | ||
if err := json.Unmarshal(body, &data); err != nil { | ||
log.Debugf("Auth failed...") |
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.
You may log err
here ;)
default: | ||
byteValue, err := json.Marshal(object) | ||
if err != nil { | ||
log.Debugf("failed to Marshal object: {}", object) |
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.
You may log err
here ;)
8709482
to
b38cc35
Compare
Hi @emilevauge, |
b38cc35
to
57233a5
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.
Thanks a lot @vladshub
LGTM
/cc @containous/traefik
|
||
## Enable authentication forwarding | ||
|
||
Uses a query param from the request to check for authentication with an auth service. |
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.
Only a query param, what about a specific custom header?
Hey, thank you for your contribution! I was waiting for this :-) |
w.WriteHeader(http.StatusInternalServerError) | ||
return | ||
} | ||
forwardReq.Header.Add("Accept", "application/json") |
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.
Can we add the original Accept
header here? Otherwise an auth back end cannot know what was sent. Maybe just use X-Accept
? Not sure though as X
headers should not be used anymore :)
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.
@Toflar this Accept is to negotiate with the authentication end point. The objects lib (github.com/stretchr/stew/objects) that is used to extract information from the authentication end point and replay in the original request only knows how to work with Json. The original request might be in any format.
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.
@diegooliveira I see, I must have misunderstood the PR. I thought it forwards the original request as discussed in your PR.
# address = ":80" | ||
# [entryPoints.http.auth.forward] | ||
# address = "http://authserver.com/auth" | ||
# [entryPoints.http.auth.Forward.requestParameters.email] |
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.
Any reason why upper and lower case are mixed here? forward
and Forward
?
[entryPoints.http.auth.Forward.responseReplayFields.userName] | ||
path = "user.name" | ||
as = "" # No name transformation | ||
in = "parameter" |
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 didn't quite get it so I (tried) to read the source code an rewrote it a bit, what do you think of this (please also make sure it's correct what I'm saying :D):
Proposal:
Authentication forwarding is a mechanism to allow for arbitrary authentication back ends to be implemented. It works by forwarding any incoming request from traefik proxy to a specified endpoint (the setting key
entryPoints.http.auth.forward.address
). This endpoint address is called "authentication back end". The response of the authentication back end is then evaluated and depending on the HTTP status code the original request to traefik is either allowed or denied. The authentication back end has to send a response with an HTTP status code200
to allow access. Any other status code will be used as response status code by traefik to the original request (so if your back end sends a response with503
, traefik will also send a503
response).
In addition to this, traefik provides basic functionality to modify the original request's query parameters before they are sent to the authentication back end. This allows you to map a request parameter (in this case
token
) from e.g.traefik.com/secret?token=foobar
toauthserver.com/auth?theToken=foobar
. Use the keywordsname
andas
in theentryPoints.http.auth.Forward.requestParameters
setting key as shown in the example.
Moreover, you can also replay certain information from the back end authentication response back to the original request received by traefik. For this to work, your authentication back end must send a JSON response.
Now I'm lost a bit because I would like to add the information about replaying back end auth information and I understand the header
option but can you explain the parameter
option for me?
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 parameter option will take data from the auth server response JSON and at it as a query parameter to the backend server if authentication is successful. Header option will likewise take the data but send it as a header.
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.
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 see. I thought it forwards the original request which would be way more powerful. See the discussion in the other PR for use cases: #764
Do you think we could do that instead? That would also make the configuration easier if you ask me.
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.
+1 for request forwarding (headers only, not sure the body is necessary?), this way the auth service has all the info available and a whole load of config is avoided.
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.
In the other PR we discussed that this should be a config and not too big of a deal to implement. Why would you prevent users from forwarding the whole body if they need it?
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.
@Toflar the original plans was to make a second pull request to implement the "full body forward", I think it's a good plan.
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.
@Toflar I have implemented forwarding of headers, regarding "forwarding the full body" I agree with @diegooliveira it should be another PR I have started implementing it but there are some aspects that look strange for example:
- I would need to force the request to be JSON but what will happen if the original request was XML will the auth server know how to handle it?
- Sending random body (and I say random because there is no original route in the request) to the auth server will be without context regarding the destination of the request.
- What HTTP method should be used since currently we use GET to send the query params to send the body we would need to post it and back to point 2 if we don't use the same verb or provide this context in the request we will have a problem and the body will be useless.
- To overcome issues issues 2-3 we would have to do one of 2 things create some kind of generic message that will be posted to the auth server with route, method and body if such exists and implement in the auth server support for such a request or a less good solution is to forward the request to the auth server with the same method but loos the path along the way at which point your auth server would have to be able to get the query string params that we want to forward for authentication and read the body if one exists in addition support all http methods in one end point...
Those are the issues just from the first 5 minuets of the implementation. I really think it would be best to open a different PR for this.
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 would need to force the request to be JSON but what will happen if the original request was XML will the auth server know how to handle it?
That's exactly the point. If you forward the request just like it is coming in, you are not responsible for that. The auth back end has to decide what it wants to handle. This allows the back end to provide authentication in whatever format, on whatever conditions, no matter if it's headers, body content, json or xml etc. If it gets XML and can only handle JSON it seems like the request is wrong so it should probably send a non 200 response.
Sending random body (and I say random because there is no original route in the request) to the auth server will be without context regarding the destination of the request.
The original Host
has to be transferred, yes. We could use Traefik-Auth-Original-Host
for that (for example) or make it configurable in best case.
What HTTP method should be used since currently we use GET to send the query params to send the body we would need to post it and back to point 2 if we don't use the same verb or provide this context in the request we will have a problem and the body will be useless.
The same as the original request. If it was a POST, we POST to the auth back end. If it was a GET, we GET.
Don't get me wrong, I really appreciate your work on this matter! I'm just trying to point out my ideas. The current PR is nice but it's very limiting and I'm not sure if it's a good idea to merge this and change it again later on. It will cause a lot of different configuration options whereas if we implemented it in a generic way, we basically leave it all to the auth back end. The only thing you can configure is if you want to forward the body or just the headers to save bandwidth if you don't need the body.
Side note: This is very much inspired by the way nginx has been doing it for a long time with the ngx_http_auth_request_module. You can find the C source here.
So for me, to cover all use cases, the config should look like this and the rest should be left to the auth back end:
[entryPoints.http.auth.forward]
endpoint = "http://authserver.com/auth"
forwardBody = true # default: false
[entryPoints.http.auth.forward.headerMap]
"Host" = "Traefik-Auth-Original-Host"
Thanks for taking initiative on this, I've also been looking forward to it. I think it would be helpful to add more options for passing data to the auth server such as cookies and headers as well. I'm fairly new to Go but I added cookies and a few fixes/improvements in my fork as I needed them for my auth server. You're welcome to use it or I can create a new PR with improvements once this is merged in. |
if forwardResponse.StatusCode != 200 { | ||
log.Debugf("Remote error %s. StatusCode: %s", forward.Address, forwardResponse.StatusCode) | ||
w.WriteHeader(forwardResponse.StatusCode) | ||
w.Write(body) |
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.
Should probably return after this line, otherwise it continues trying to parse the JSON response even though there was an error.
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.
Thanks and done.
@vladshub there was a call to be allowed to replay the original request payload to the authentication end point. Do you plan to do that? |
086c28d
to
b6e7a9a
Compare
A forward authentication that extracts parameters from the request and then call a authenticate endpoint. If the authentication outcome is a 200 it allows to extract information from the response body and add to the proxy call.
fe48a08
to
313e24b
Compare
@drampelt I have implemented forward of all headers so you get the cookies as well 😄 |
As mentioned elsewhere, I'm not coding in Go yet. All I can contribute is what I already did here: review and comment and I'll do that again if it helps :-) |
Happy to help. |
@emilevauge I can help but there is one thing. |
Are we talking about a RFC process, something like this? I like the idea. |
@diegooliveira that feels as an overkill I was talking more about a general feature map/plan and explicit boundaries .... |
I was thinking that would should start by getting consensus on the features that should definitely be included for v1 but here isn't really the place to do that. I was just going to ask if there's an irc/slack channel but I found it (https://traefik.herokuapp.com/), joined as j0hnsmith. |
@vladshub @Toflar @drampelt @diegooliveira @j0hnsmith |
@emilevauge @vladshub @Toflar @drampelt @diegooliveira @j0hnsmith what about opening a shared Google doc to make a draft proposal, this way we may get to a better solution. If you agree, please send a message with your email address and I'll create the doc with write privilege. |
I guess the simplest solution for now is as follows: First step
Second step
Third step
|
Oh, that was just a little too late :) Maybe we can also just use a specific channel on traefik.slack.com? |
@diegooliveira I like Google Doc, but I would prefer a simpler solution (without invite needed). What about a simple gist ? Simple, revisions, comments, and on Github :) |
Sure, feel free to create one. I'm okay with whatever solution you choose :) |
Let's use Google Doc instead (revisions need git on gist, not that simple ;) ): Authentication middleware proposal |
@emilevauge I am now using Traefik as our API "gateway" and am very interested in the auth middleware, could help as well if wanted/needed. Could i get read access to the proposal to review? |
Is anyone working on implementation (i.e. is proposal finished)? |
Is the proposal locked down and ready for development? |
Currently we don't work on the feature, if you want to work on implementation and make a PR, we will be very pleased with this. |
But even the "Authentication middleware proposal" is not public... :(, and @vladshub's PR is rejected, just confused about about the plan.. |
@meggarr you should be able to ask for permissions once you try to open the proposal, and @emilevauge should afterwards grant it to you. |
Something that simulates nginx's auth_request would be highly appreciated. If implemented like in NGINX, one would be abel to use existing solutions like Authelia (LDAP + 2FA) which is a big plus. |
Hi everyone, back in January I ended up just forking the project and implementing a solution very specific to my needs at the time, and then unfortunately never got a chance to continue working on a standard solution. I've opened up #1972 with a simplified implementation that I think is a good starting point for this feature. @emilevauge @vladshub @Toflar @diegooliveira @j0hnsmith if any of you are still interested in this I'd love to hear your thoughts. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Hello, it's not the right place to ask a question, the community forum is the right place. So I will flag your comment as offtopic. |
It seems that there is no progress in #764 so I have re-based the work that was done there and added something that I have been waiting for #764 to be merged.
Please take a look
Now it is possible to delegate the authentication to a third party agent, like a OAuth endpoint. This authentication option extracts parameters from the original request, forward then to a authentication endpoint, if it succeed part of the response body can be added as json serialized headers.