Open lukedirtwalker opened 1 year ago
That's interesting. Thanks for opening an issue about this.
Giving this some thought, I think I agree: auth plugins should be invoked in a sandbox, of sorts. We have a caddyhttp.ResponseRecorder
type to make this easier, but it's not required to use it.
So yeah, maybe the algorithm should be like what you said: run all the auth plugins until one succeeds; if so, go with that, discard the others that didn't succeed. If none matched, I'm not sure what the best answer is... but I like your line of thought so far.
(PS. Is there a use case where the user would want all the auth plugins to pass before continuing? Maybe that needs to be an option! Otherwise it's not clear whether the list of auth modules is OR'ed or AND'ed... and it's not clear that one logic is inherently better than the other.)
Would be happy to review a PR. It might need a few iterations while we figure this out though.
@mholt I created a PR here: https://github.com/caddyserver/caddy/pull/5192
I think at the very least, we should update the godoc comment to explain the current behaviour.
I have a setup where I have multiple authentication providers configured:
The
authorizer
is from the caddy-security plugin, it handles the OAuth login etc. If the user is not logged in the user will be redirected.The expectation of this configuration is that I can either use OAuth (via
authorizer
provider) or Basic Auth (viahttp_basic
provider) to login and to pass theauthentication
handler. The problem is that the plugins have side effects on the response. For example when I use basic auth I get back the some of the Header and the status code of theauthorizer
plugin but also the expected content from a handler that is after theauthentication
handler.Looking at the code it becomes clear why this happens: https://github.com/caddyserver/caddy/blob/6efd1b3bb1217841269e67930cba33992fb96930/modules/caddyhttp/caddyauth/caddyauth.go#L71-L76 We pass the same
http.ResponseWriter
to all authentication providers. In my case theauthorizer
plugin sets the status code to 302 and sets the location to redirect in the header. Essentially callinghttp.ResponseWriter.WriteHeader
seals the header from being written by any other plugin. The order of the provisioners, depends on the map order which is random.Potential solution: I think we should pass separate
http.ResponseWriter
instances to each provider, if any provider succeeds all of them can be discarded.The question is what should if none of the providers successfully authenticates/authorizes the user. Some of the providers may rely on redirects, if there are multiple I think there needs to be an explicit priority, if only one wants to redirect, the redirect could take precedence over the other statuses.
I would be happy to provide a PR to fix this, if there is agreement on the approach on how to handle this.