ZuulProxy returning 200 OK (default) HTTP status instead of custom (401) when OAuth2TokenRelayFilter throws BadCredentialsException

[miguelfgar]

Hi,

When using ZuulProxy as API & Security Gateway (handling oauth2 tokens and propagating to downstream services - Zuul annotated with @EnableOAuth2SSO) it is returning 200 OK (zuul default) http status when OAuth2TokenRelayFilter (which extends ZuulFilter) is throwing BadCredentialsException.

Zuul by default is handling HTTP returns the following way:

• The return of the downstream invoked service if it was able to pass the request downstream
• 200 OK status by default (if no custom error handling is implemented)

In cases where BadCredentialsExceptions is raised because OAuth2TokenRelayFilter was not able to obtain an OAuth2 access token from the authorization server the request is never passed downstream (Zuul is not passing it downstream to the service as it was not able to obtain an oauth2 token). In this case, Zuul is returning 200 OK (default) because no custom error handling was implemented with an empty response body.

So Zuul knows the status that needs to return in this case it needs a custom error handling. However, I was not able to see a custom error handler or exception handler for this so it "explains" zuul how to deal with the error (which status code to return). Might I be missing something? I guess not, but it is possible...

I solved my problem using the standard SendErrorFilter (extends ZuulFilter) that checks if a component has notified Zuul the status code to return by checking Zuul context (with an agreement on finding certain parameter ("error.status_code" in this case). For instance:

   ctx.set("error.status_code", HttpServletResponse.SC_UNAUTHORIZED);

Adding this line to OAuth2TokenRelayFilter in the proper section (when throwing BadCredentialsException) fixed my problem. However I would like to confirm that this is the proper solution or to know a better one if exists. Alternatives would not to modify OAuth2TokenRelayFilter and add another filter to handle the error or add a custom error handler for this. However, given that OAuth2TokenRelayFilter is a ZuulFilter (it extends this class) and uses the zuul context anyway, to add the "error.status_code" in OAuth2TokenRelayFilter directly is not intrussive.

The getAccessToken() method in OAuth2TokenRelayFilter would look like this:

    private String getAccessToken(RequestContext ctx) {
        String value = (String) ctx.get(ACCESS_TOKEN);
        if (restTemplate != null) {
            // In case it needs to be refreshed
            OAuth2Authentication auth = (OAuth2Authentication) SecurityContextHolder
                    .getContext().getAuthentication();
            if (restTemplate.getResource().getClientId()
                    .equals(auth.getOAuth2Request().getClientId())) {
                try {
                    value = restTemplate.getAccessToken().getValue();
                }
                catch (Exception e) {
                    // Quite possibly a UserRedirectRequiredException, but the caller
                    // probably doesn't know how to handle it, otherwise they wouldn't be
                    // using this filter, so we rethrow as an authentication exception

                                       // Line added so Zuul SendErrorFilter is "notified" that needs to return 401 - Not Authorized so Zuul is not returning default 200 OK in this case
                                       ctx.set("error.status_code", HttpServletResponse.SC_UNAUTHORIZED);

                    throw new BadCredentialsException("Cannot obtain valid access token");
                }
            }
        }
        return value;
    }

I would appreciate any feedback. In particular @dsyer: for sure you can be helpful as always :-) would you mind to tell what are your thoughts? Am I right or wrong and is there a better solution from you point of view? I did not send a pull request because I'm not sure this is the best solution yet.

Thanks and regards,

[spencergibb]

I made some changes in zuul yesterday that fix the empty 200 OK when there should be an error.

[miguelfgar]

Thanks @spencergibb for your quick response.

I gave a quick look and I saw instead of taking the error status from zuul context "error.status_code" entry now is taken from the status code of the ZuulException. Thanks for telling me about the changes you did yesterday.

However, so this works with your changes I guess some work needs to be done to align OAuth2TokenRealyFilter with them, right? I mean, OAuth2TokenRelayFilter is throwing BadCredentialsException, not a ZuulException. Even if a the exception is translated at some point into a ZuulException by Spring "magic" (not sure right now if it is just not translated or I miss where the 'magic' is at the moment :-) ) the BadCredentialException has no "statusCode" field (ZuulException has) so I guess nobody will be adding the proper status to be returned.. As I said, I might be missing something.

Are you able to confirm that some code needs to be added so OAuth2TokenRelayFilter is aligned with this changes?

Thanks and Regards,

Miguel

[spencergibb]

yes it probably does need to be updated.

[miguelfgar]

Thanks @spencergibb.
That was very helful. Specially because currently i'm still in spring-cloud BRIXTON-SR7 (not CANDEM yet).

@dsyer, @spencergibb What do you think would be the best implementation option to align OAuth2TokenRelayFilter with this changes performed by @spencergibb yesterday? Instead of using AccessDeniedException (which is generic and reused not just for Oauth2) should a new exception be used extending ZuulException (and so having a statusCode field to be informed as 401 in this case)? or should some 'magic' exists translating the AccessDeniedException into a ZuulException and adding the 401 status at some point? Or maybe none of this options as you might have a better idea in mind :-)

Also, will be this changes performed by @spencergibb applied to BRIXTON branch? or do you thing the solution I mentioned (adding the error.status_code in OAuth2TokenRelayFilter) is correct for BRIXTON if the changes mentioned by @spencergibb are not applied to BRIXTON work stream?

Thanks and Regards,

Miguel

[Bessonov]

@spencergibb @dsyer I ran in the same issue. Is there any update on this?

[miguelfgar]

Hi @Bessonov,

As I mentioned above, a workaround in the mentioned versions is to set the flag in the context that the Zuul sendError filter processes:

                ctx.set("error.status_code", HttpServletResponse.SC_UNAUTHORIZED);

This works (not sure is the best solution, though). However, for the new versions with the changes mentioned by @spencergibb it looks like spring-oauth2 artifact needs to be adapted accordingly (OAuth2TokenRelayFilter, for instance). Here it would be great if @dsyer could give his point of view or some guidance about the best way to implement it (adapt it).

Regards

[Bessonov]

thanks, guys!

[miguelfgar]

Yes, (even though the issue is closed) I just want to say thank you! :-)

[MrChrisPSimmons]

Perhaps I'm missing something but this doesn't seem to work any more. I'm seeing that the BadCredentialsException is being caught here at com.netflix.zuul.FilterProcessor.processZuulFilter(FilterProcessor.java:227) ~[zuul-core-1.3.0.jar:1.3.0] which throws a ZuulException with a 500 error code, resulting in the client seeing a 500 error. I'm using spring-cloud-security-1.2.0.RELEASE.jar.

If you look at https://github.com/spring-cloud/spring-cloud-netflix/commit/08fe5a89e9b6e13c72b0ec6e4c57ad11c26e1c07 its not paying any attention to error.status_code any more in SendErrorFilter.

[dsyer]

If there's an upstream I guess we need to fix that. Please open another issue (or send a PR).

[MrChrisPSimmons]

I've belatedly realized that what I'm seeing is the same as https://github.com/spring-cloud/spring-cloud-security/issues/119 which is still open, having come up with a very similar work around. Sorry for the noise.