Skip to content
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

ResponseStatusException no longer returning response body in 2.6.1 using spring security #28953

Closed
SimonMitchellMOJ opened this issue Dec 9, 2021 · 15 comments
Labels
type: regression A regression from a previous release
Milestone

Comments

@SimonMitchellMOJ
Copy link

we found that after upgrading to spring boot 2.6.1 that the response body from the ResponseStatusException is no longer being populated even for authenticated users

It looks like the spring boot forwards onto the /error page but the BearerTokenAuthenticationFIlter which extends the OncePerRequestFilter doesn't add the necessary authentication to the spring security context when in error state.
This means that we then hit #26356 and the body is empty.

an example project is https://github.com/ministryofjustice/hmpps-spring-boot-2.6-bug

if you run ./gradlew test then it will fail

the branch https://github.com/ministryofjustice/hmpps-spring-boot-2.6-bug/tree/previous-working-version shows it working in the previous version of spring boot 2.5.6 alternately allowlisting /error fixes it too ( but we don't want to allowlist /error )

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 9, 2021
@wilkinsona
Copy link
Member

Thanks for the sample.

The behaviour you have observed is intended with Spring Boot 2.6. Your security configuration requires authentication to access everything but /favicon.ico, /csrf, /health/**, /info, /webjars/**, and /v2/api-docs. This means that you have secured Spring Boot's error page at /error and, thanks to the changes in #26356, an unauthenticated user no longer receives a full error response.

If you want any user, authenticated or not, to receive Spring Boot's default error response, you should permit access to /error. Applying this diff to your application fixes the failing test:

diff --git a/src/main/kotlin/uk/gov/justice/digital/hmpps/token/config/ResourceServerConfiguration.kt b/src/main/kotlin/uk/gov/justice/digital/hmpps/token/config/ResourceServerConfiguration.kt
index aa77ab2..6a4d0a6 100644
--- a/src/main/kotlin/uk/gov/justice/digital/hmpps/token/config/ResourceServerConfiguration.kt
+++ b/src/main/kotlin/uk/gov/justice/digital/hmpps/token/config/ResourceServerConfiguration.kt
@@ -25,7 +25,7 @@ open class ResourceServerConfiguration : WebSecurityConfigurerAdapter() {
       .authorizeRequests { auth ->
         auth.antMatchers(
           "/favicon.ico", "/csrf", "/health/**", "/info",
-          "/webjars/**", "/v2/api-docs"
+          "/webjars/**", "/v2/api-docs", "/error"
         )
           .permitAll().anyRequest().authenticated()
       }

@wilkinsona wilkinsona added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 9, 2021
@petergphillips
Copy link

I think the important behavioural change here in 2.6 is that authenticated users no longer receive a full error response if the application relies on the default error handler. It would be good to document this change and provide the recommended solution.

It does feel a bit strange though that due to Error page is accessible when no credentials are provided we have now ended up with Error page is not accessible when credentials are provided and the recommended solution to that problem is to allow the error page to be accessible to everyone!

It does sound therefore that the default error handler should not be used. Instead I guess the application could catch the exception and return a ResponseEntity directly or alternatively throw a custom exception and then implement a @ExceptionHandler that returns a ResponseEntity.

@wilkinsona
Copy link
Member

I think the important behavioural change here in 2.6 is that authenticated users no longer receive a full error response if the application relies on the default error handler.

That shouldn't be the case.

the recommended solution to that problem is to allow the error page top be accessible to everyone

That is not the recommendation. If a user is authenticated, they should received a body in an error response without permitting all to access the error page. If that's not happening, then we need to investigate further. It could be that there's a faulty filter involved (as I believe was the case in the second issue to which you linked above), you're hitting a limitation of Spring Security, or something else.

@SimonMitchellMOJ
Copy link
Author

The user in the sample project above is fully authenticated but they are not receiving a body in the error response.

It looks like the spring boot forwards onto the /error page but the BearerTokenAuthenticationFIlter which extends the OncePerRequestFilter doesn't add the necessary authentication to the spring security context when in error state.

Should I therefore raise a bug against the BearerTokenAuthenticationFIlter? (I presume other filters will be affected as well)

@wilkinsona
Copy link
Member

wilkinsona commented Dec 10, 2021

Apologies, @SimonMitchellMOJ, I had misunderstood the problem that the sample was demonstrating.

The problem is due to Spring Security's SecurityContextPersistenceFilter behaving differently depending on the authentication method that is being used. When using basic authentication, the authentication from the initial request is reinstated when the request fails and is forwarded to Boot's error page:

2021-12-10 10:11:08.830 DEBUG 5568 --- [o-auto-1-exec-1] o.s.security.web.FilterChainProxy        : Securing GET /fail
2021-12-10 10:11:08.835 DEBUG 5568 --- [o-auto-1-exec-1] s.s.w.c.SecurityContextPersistenceFilter : Set SecurityContextHolder to empty SecurityContext
2021-12-10 10:11:09.002 DEBUG 5568 --- [o-auto-1-exec-1] o.s.s.a.dao.DaoAuthenticationProvider    : Authenticated user
2021-12-10 10:11:09.003 DEBUG 5568 --- [o-auto-1-exec-1] o.s.s.w.a.www.BasicAuthenticationFilter  : Set SecurityContextHolder to UsernamePasswordAuthenticationToken [Principal=org.springframework.security.core.userdetails.User [Username=username, Password=[PROTECTED], Enabled=true, AccountNonExpired=true, credentialsNonExpired=true, AccountNonLocked=true, Granted Authorities=[]], Credentials=[PROTECTED], Authenticated=true, Details=WebAuthenticationDetails [RemoteIpAddress=127.0.0.1, SessionId=null], Granted Authorities=[]]
2021-12-10 10:11:09.023 DEBUG 5568 --- [o-auto-1-exec-1] w.c.HttpSessionSecurityContextRepository : Created HttpSession as SecurityContext is non-default
2021-12-10 10:11:09.023 DEBUG 5568 --- [o-auto-1-exec-1] w.c.HttpSessionSecurityContextRepository : Stored SecurityContextImpl [Authentication=UsernamePasswordAuthenticationToken [Principal=org.springframework.security.core.userdetails.User [Username=username, Password=[PROTECTED], Enabled=true, AccountNonExpired=true, credentialsNonExpired=true, AccountNonLocked=true, Granted Authorities=[]], Credentials=[PROTECTED], Authenticated=true, Details=WebAuthenticationDetails [RemoteIpAddress=127.0.0.1, SessionId=null], Granted Authorities=[]]] to HttpSession [org.apache.catalina.session.StandardSessionFacade@651af14a]
2021-12-10 10:11:09.028 DEBUG 5568 --- [o-auto-1-exec-1] o.s.s.w.a.i.FilterSecurityInterceptor    : Authorized filter invocation [GET /fail] with attributes [fullyAuthenticated]
2021-12-10 10:11:09.029 DEBUG 5568 --- [o-auto-1-exec-1] o.s.security.web.FilterChainProxy        : Secured GET /fail
2021-12-10 10:11:09.048 DEBUG 5568 --- [o-auto-1-exec-1] w.c.HttpSessionSecurityContextRepository : Stored SecurityContextImpl [Authentication=UsernamePasswordAuthenticationToken [Principal=org.springframework.security.core.userdetails.User [Username=username, Password=[PROTECTED], Enabled=true, AccountNonExpired=true, credentialsNonExpired=true, AccountNonLocked=true, Granted Authorities=[]], Credentials=[PROTECTED], Authenticated=true, Details=WebAuthenticationDetails [RemoteIpAddress=127.0.0.1, SessionId=null], Granted Authorities=[]]] to HttpSession [org.apache.catalina.session.StandardSessionFacade@651af14a]
2021-12-10 10:11:09.048 DEBUG 5568 --- [o-auto-1-exec-1] s.s.w.c.SecurityContextPersistenceFilter : Cleared SecurityContextHolder to complete request
2021-12-10 10:11:09.059 DEBUG 5568 --- [o-auto-1-exec-1] o.s.security.web.FilterChainProxy        : Securing GET /error
2021-12-10 10:11:09.059 DEBUG 5568 --- [o-auto-1-exec-1] w.c.HttpSessionSecurityContextRepository : Retrieved SecurityContextImpl [Authentication=UsernamePasswordAuthenticationToken [Principal=org.springframework.security.core.userdetails.User [Username=username, Password=[PROTECTED], Enabled=true, AccountNonExpired=true, credentialsNonExpired=true, AccountNonLocked=true, Granted Authorities=[]], Credentials=[PROTECTED], Authenticated=true, Details=WebAuthenticationDetails [RemoteIpAddress=127.0.0.1, SessionId=null], Granted Authorities=[]]]
2021-12-10 10:11:09.059 DEBUG 5568 --- [o-auto-1-exec-1] s.s.w.c.SecurityContextPersistenceFilter : Set SecurityContextHolder to SecurityContextImpl [Authentication=UsernamePasswordAuthenticationToken [Principal=org.springframework.security.core.userdetails.User [Username=username, Password=[PROTECTED], Enabled=true, AccountNonExpired=true, credentialsNonExpired=true, AccountNonLocked=true, Granted Authorities=[]], Credentials=[PROTECTED], Authenticated=true, Details=WebAuthenticationDetails [RemoteIpAddress=127.0.0.1, SessionId=null], Granted Authorities=[]]]
2021-12-10 10:11:09.059 DEBUG 5568 --- [o-auto-1-exec-1] o.s.security.web.FilterChainProxy        : Secured GET /error
2021-12-10 10:11:09.120 DEBUG 5568 --- [o-auto-1-exec-1] s.s.w.c.SecurityContextPersistenceFilter : Cleared SecurityContextHolder to complete request

By contrast, when using a bearer token, the forward to /error is done with an anonymous authentication rather than the AuthAwareAuthenticationToken that was initially established:

2021-12-10 10:09:46.611 DEBUG 5524 --- [o-auto-1-exec-1] o.s.security.web.FilterChainProxy        : Securing POST /token/verify
2021-12-10 10:09:46.615 DEBUG 5524 --- [o-auto-1-exec-1] s.s.w.c.SecurityContextPersistenceFilter : Set SecurityContextHolder to empty SecurityContext
2021-12-10 10:09:46.645 DEBUG 5524 --- [o-auto-1-exec-1] o.s.s.o.s.r.a.JwtAuthenticationProvider  : Authenticated token
2021-12-10 10:09:46.646 DEBUG 5524 --- [o-auto-1-exec-1] .o.s.r.w.BearerTokenAuthenticationFilter : Set SecurityContextHolder to AuthAwareAuthenticationToken [Principal=token-verification-api-client, Credentials=[PROTECTED], Authenticated=true, Details=WebAuthenticationDetails [RemoteIpAddress=127.0.0.1, SessionId=null], Granted Authorities=[SCOPE_read, SCOPE_write]]
2021-12-10 10:09:46.651 DEBUG 5524 --- [o-auto-1-exec-1] o.s.s.w.a.i.FilterSecurityInterceptor    : Authorized filter invocation [POST /token/verify] with attributes [authenticated]
2021-12-10 10:09:46.652 DEBUG 5524 --- [o-auto-1-exec-1] o.s.security.web.FilterChainProxy        : Secured POST /token/verify
2021-12-10 10:09:46.707 DEBUG 5524 --- [o-auto-1-exec-1] s.s.w.c.SecurityContextPersistenceFilter : Cleared SecurityContextHolder to complete request
2021-12-10 10:09:46.711 DEBUG 5524 --- [o-auto-1-exec-1] o.s.security.web.FilterChainProxy        : Securing POST /error
2021-12-10 10:09:46.711 DEBUG 5524 --- [o-auto-1-exec-1] s.s.w.c.SecurityContextPersistenceFilter : Set SecurityContextHolder to empty SecurityContext
2021-12-10 10:09:46.712 DEBUG 5524 --- [o-auto-1-exec-1] o.s.s.w.a.AnonymousAuthenticationFilter  : Set SecurityContextHolder to anonymous SecurityContext
2021-12-10 10:09:46.712 DEBUG 5524 --- [o-auto-1-exec-1] o.s.security.web.FilterChainProxy        : Secured POST /error

The newly introduced ErrorPageSecurityFilter is built on the assumption that Spring Security will behave consistently in terms of the authentication that's available when a request is forwarded. The above shows that the assumption doesn't hold true when using a bearer token. We'll need to discuss this with the Security team to figure out the best course of action.

@wilkinsona wilkinsona reopened this Dec 10, 2021
@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue for: team-meeting An issue we'd like to discuss as a team to make progress and removed status: invalid An issue that we don't feel is valid labels Dec 10, 2021
@cmdjulian
Copy link

cmdjulian commented Dec 13, 2021

I'm seeing the same problem in our applications. We wrote our own custom Authentication. We're also not seeing response bodys on our requests anymore. For 2.5.7 it was working fine.
It boils down to the ErrorController.error(HttpServletRequest request) not getting called in 2.6.1 from what I found out.
I tried the current snapshot of 2.6.2 with the same outcome, no response body. I tried adding /error to the list of permitted paths but this also didn't bring back the response body.
Its relay unfortunate because this is an update blocker for us. Especially regarding the new log4j cve it would be really nice to be able to upgrade as soon as 2.6.2 is out (of course we temporally fixed it by forcing log4j to be version 2.15.0 in our gradle file)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 13, 2021
@mbhave
Copy link
Contributor

mbhave commented Dec 13, 2021

@cmdjulian Can you provide a minimal sample that we can use to replicate the issue, please? You're most likely running into the same issue but I want to be sure since you mentioned custom Authentication and the original issue was caused due to the Authentication not being available to the error dispatch due to a session policy of STATELESS.

@cmdjulian
Copy link

cmdjulian commented Dec 14, 2021

We use SessionCreationPolicy.STATELESS as well. We created a custom Authentication implementation to enrich the SecurityContext with more data. Under the hood it just extracts a JWT present in the Authentication header of a given request.
The code can be pictures as following:

public class JwtAuthorizationFilter extends OncePerRequestFilter {

    public static final String BEARER_TOKEN_PREFIX = "Bearer ";

    /**
     * Put token into Spring Security Context, so that {@link JwtAuthenticationProvider}
     * can verify the tokens validity.
     */
    @Override
    protected void doFilterInternal(@NonNull HttpServletRequest req, @NonNull HttpServletResponse res, FilterChain chain)
        throws IOException, ServletException {

        getJwtFromRequest(req).ifPresent(jwtCredentials -> {
            JwtAuthenticationToken jwtAuthenticationToken = JwtAuthenticationToken.builder()
                .token(jwtCredentials)
                .details(new WebAuthenticationDetailsSource().buildDetails(req))
                .authenticated(false)
                .build();

            var context = SecurityContextHolder.createEmptyContext();
            context.setAuthentication(jwtAuthenticationToken);
            SecurityContextHolder.setContext(context);
        });

        chain.doFilter(req, res);
    }

    /**
     * Extract JWT value from Request header.
     */
    private Optional<String> getJwtFromRequest(HttpServletRequest request) {
        String token = request.getHeader(HttpHeaders.AUTHORIZATION);

        return StringUtils.startsWith(token, BEARER_TOKEN_PREFIX)
            ? Optional.of(token.substring(BEARER_TOKEN_PREFIX.length()))
            : Optional.empty();
    }

}
@RequiredArgsConstructor
public class JwtAuthenticationProvider implements AuthenticationProvider {

    private final JwtManager manager;

    private final UserDetailsService userDetailsService;

    @Override
    public Authentication authenticate(Authentication authentication) {
        final VerifiedUserToken verifiedToken = manager.verifyToken(authentication.getCredentials().toString());

        return authenticateUser(verifiedToken, authentication.getDetails());
    }

    private Authentication authenticateUser(VerifiedUserToken token, Object details) {
        UserDetails user = userDetailsService.loadUserByUsername(token.getUsername());

        return constructAuthentication(token.getRawToken(), user, details);
    }

    private Authentication constructAuthentication(String token, UserDetails user, Object details) {
        return JwtAuthenticationToken.builder()
            .token(token)
            .user(user)
            .details(details)
            .authorities(user.getAuthorities())
            .authenticated(true)
            .build();
    }

    @Override
    public boolean supports(Class<?> authentication) {
        return authentication.equals(JwtAuthenticationToken.class);
    }

}

The JwtAuthenticationToken just implements Authentication without any extra buzz. If you still want me to submit a complete example let me know.
Except for not using Spring OAuth2 its the same as in the example provided example

@philwebb philwebb added this to the 2.6.2 milestone Dec 15, 2021
@philwebb philwebb added type: regression A regression from a previous release and removed status: feedback-provided Feedback has been provided for: team-meeting An issue we'd like to discuss as a team to make progress labels Dec 15, 2021
@mjustin
Copy link

mjustin commented Dec 17, 2021

Is there a possible (temporary) workaround within an app being upgraded from 2.5.x to 2.6.1, or are we pretty much stuck waiting for 2.6.2?

@mbhave
Copy link
Contributor

mbhave commented Dec 17, 2021

@mjustin A workaround would be to remove the ErrorPageSecurityFilter as described in this comment.

@mjustin
Copy link

mjustin commented Dec 17, 2021

@mjustin A workaround would be to remove the ErrorPageSecurityFilter as described in this comment.

Thanks. Won't that have the side effect of also enabling error pages when the user is not logged authenticated, though?

@datagitlies
Copy link

@mbhave I just updated to 2.6.2 and the behavior is the same... I'm not sure d9d161c actually fixed the issue completely. The problem (for me) seems to be ErrorPageSecurityFilter.java#L80 - snippet below:

private boolean isAllowed(HttpServletRequest request, Integer errorCode) {
	Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
	if (isUnauthenticated(authentication) && isNotAuthenticationError(errorCode)) {
		return true;
	}
	return getPrivilegeEvaluator().isAllowed(request.getRequestURI(), authentication);
}

The authentication even for an authenticated user is always an instance of AnonymousAuthenticationToken - In my scenario the user is trying to access a resource they are not allowed to see. Which, I'm expecting an HTTP 403 response but I would also expect to see the error body (because the user is authenticated). Even setting .antMatchers("/error").permitAll() does not fix the issue. Why is the authentication from the SecurityContextHolder being wiped out?

@mjustin
Copy link

mjustin commented Jan 3, 2022

I will say this fixed the specific issue I was having, so it looks to be at least a partial fix.

@lgraf
Copy link

lgraf commented Jan 4, 2022

@mbhave I just updated to 2.6.2 and the behavior is the same... I'm not sure d9d161c actually fixed the issue completely. The problem (for me) seems to be ErrorPageSecurityFilter.java#L80 - snippet below:

private boolean isAllowed(HttpServletRequest request, Integer errorCode) {
	Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
	if (isUnauthenticated(authentication) && isNotAuthenticationError(errorCode)) {
		return true;
	}
	return getPrivilegeEvaluator().isAllowed(request.getRequestURI(), authentication);
}

The authentication even for an authenticated user is always an instance of AnonymousAuthenticationToken - In my scenario the user is trying to access a resource they are not allowed to see. Which, I'm expecting a HTTP 403 response but I would also expect to see the error body (because the user is authenticated). Even setting .antMatchers("/error").permitAll() does not fix the issue. Why is the authentication from the SecurityContextHolder being wiped out?

@mbhave @datagitlies We have the same scenario: Show a (custom) spring boot error page if a user has no access (403) to a resource. As far i can tell the current ErrorPageSecurityFilter implementation does not support this case.

The implementation call WebInvocationPrivilegeEvaluator.isAllowed(uri, authentication) in case of an HTTP 401/403.

During debugging i observed the following:

  1. The ErrorPageSecurityFilter use HttpServletRequest.getRequestURI() as uri parameter for the WebInvocationPrivilegeEvaluator.isAllowed(uri, authentication) method, which contains the context path. The JavaDoc notes that the context-path should be excluded in the uri parameter

This seems to be the reason that a matcher like .antMatchers("/error").permitAll() will not match in this case. Instead the matcher must additionally include the context-path to match.

  1. Like already described by @datagitlies without access to the actual authentication (in case of @Transient authentication or STATELESS session policy) matchers like .antMatchers("/error").authenticated() will not work.

Are these known limitations, or is the described case not meant to be implemented by using spring error pages?

@philwebb
Copy link
Member

philwebb commented Jan 4, 2022

@lgraf I believe that the .antMatcher(...) always needs the context path in the pattern. You might want to try mvcMatcher(...) instead. If you're still having problems that you think are caused by a bug can you please open a new issue and attach a sample project that we can run and debug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

10 participants