spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.96k stars 40.65k forks source link

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

Closed simon-mitchell closed 2 years ago

simon-mitchell commented 2 years ago

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 )

wilkinsona commented 2 years ago

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()
       }
petergphillips commented 2 years ago

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 commented 2 years ago

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.

simon-mitchell commented 2 years ago

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 commented 2 years ago

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.

cmdjulian commented 2 years ago

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)

mbhave commented 2 years ago

@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 commented 2 years ago

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

mjustin commented 2 years ago

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 commented 2 years ago

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

mjustin commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

lgraf commented 2 years ago

@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 commented 2 years ago

@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.