fix(authn/oauth2): prevent oauth2 redirect loops

[chris-h-phillips]

During setup of spinnaker authentication with oauth2 a common hurdle is a redirect loop.

For example:

spinnaker/spinnaker#5794
spinnaker/spinnaker#1630

Also, many threads in Slack discuss these problems. In fact this appears to be a common
pitfall for the spring-security-oauth2-autoconfigure library in general. A light refresher
on the ouath2 flow in play here seems worthwhile. The user is redirected from /login in gate
to the external auth provider (google, github, etc.) and after successfully authenticating
they are redirected back to the gate /login endpoint but this time with a code parameter
that is to be used to request an access token.

This request can fail for a variety of reasons, and if it does, the underlying spring library
triggers a redirect to the /error endpoint. What causes the redirect loop for gate in particular
(and for other users of the library in a similar fashion) is that the WebSecurityConfigurerAdapter
in play is treating /error as an authenticated path and so instead of just returning with a 401,
it re-redirects to /login and the redirect loop continues.

My thought is that instead of a redirect loop, simply allowing the 401 to be returned will be a stronger
more helpful signal as to what is going on. Hopefully it will save future first-time installers headaches.

Spinnaker docs have included several troubleshooting hints and tips for how where you terminate SSL
affects configuration etc. Even after following all of these and lots of spelunking through spinnaker
github issues and combing over threads in slack, I found myself still experiencing a redirect loop even
though I had applied all the combined wisdom that was applicable to my setup.

As it turns out, I had a bad copy/paste of my client secret in my configuration. So the request
to turn the code from google into an access token from google was failing with a 401. After much
debugging and deep diving into the spring security code I found that had I turned on DEBUG in gate
for these classes in gate-local.yml:

logging:
  level:
    org.springframework.security.web.authentication.SimpleUrlAuthenticationFailureHandler: DEBUG
    org.springframework.security.oauth2.client.filter.OAuth2ClientAuthenticationProcessingFilter: DEBUG

Then I would have seen in the logs that a 401 response was returned from google and perhaps it would have
caused me to look closer at my botched client secret configuration. I think perhaps we don't want to require
that all operators of spinnaker become spring-security-oauth2 experts. So I'm proposing adding /error to
the list of paths in gate that aren't treated as authenticated. Thus short-circuiting the redirect loop and
bringing to light helpful troubleshooting info that was previously more or less swallowed.

[spinnakerbot]

Please delete the pull request instructions from the body of your pull request message.

The instructions start with the line:

We prefer small, well tested pull requests.

You can reopen your pull request when this has been addressed.

[deverton]

What serves up the /error path? I had a quick skim through Gate and Spring Boot and couldn't find the request mapping for that path. The concern would be that the handler for that path could potentially leak more information than needed.

[chris-h-phillips]

The response is a simple json object that looks like this:

Oauth2ClientAuthenticationProcessingFilter is a AbstractAuthenticationProcessingFilter

When the exception from the getAccessToken bubbles up it triggers a call to unsuccessfulAuthentication which in turn invokes the default failureHandler (since gate hasn't configured a different one). The default handler is a SimpleUrlAuthenticationFailureHandler The call to response.sendError triggers the global spring boot BasicErrorController which is how the json response is constructed and where the /error comes into play.

[jasonmcintosh]

https://github.com/spinnaker/kork/blob/master/kork-web/src/main/java/com/netflix/spinnaker/kork/web/controllers/GenericErrorController.java

Think this is part of the headache too...

[chris-h-phillips]

Ah yes it is the GenericErrorController from kork can confirm thanks for pointing that out 👍

[dbyron-sf]

This implementation of getErrorAttributes looks like an attempt to keep unsavory info out of responses to /error. Is there something else we can/need to do there to make this safe(r)?

[deverton]

From all the excellent information provided here I think it seems reasonably safe to do this. At worst, it looks like there's a way to filter additional information if it turns out there is a vulnerability here.

[jasonmcintosh]

Looks good to me :)