From 39828b2c944125dcfb8370edf79865048667203a Mon Sep 17 00:00:00 2001 From: Philipp Nowak Date: Mon, 11 Feb 2019 19:02:18 +0100 Subject: [PATCH] [KEYCLOAK-9539] Race condition SecurityContextHolder.setAuthentication() This is an issue with the Spring Security Keycloak Adapter relating to the way the Authentication is stored in the SecurityContext, causing a race condition in application code using that. It does not seem to affect actual Spring Security operation. We had a pretty strange race condition in our application. When many requests were incoming at the same time, occasionally the old unauthenticated Authentication provided to KeycloakAuthenticationProvider for performing the actual authentication would stay the current authentication, as returned by SecurityContextHolder.getContext().getAuthentication(). That resulted in authenticated users' JavaScript requests occasionally (~1/50 given a large request volume) returning a 403 because the 'old' token was still in the context, causing Spring Security to see them as unauthenticated. This PR resolves this issue by replacing the whole context, as suggested by a Spring Security contributor in jzheaux/spring-security-oauth2-resource-server#48. By default, SecurityContextHolder keeps the actual context object in a ThreadLocal, which should be safe from race-conditions. The actual Authentication object, however, is kept in a mere field, hence the reason for this PR. JIRA issue: https://issues.jboss.org/browse/KEYCLOAK-9539 --- .../SpringSecurityRequestAuthenticator.java | 6 +++++- .../KeycloakAuthenticationProcessingFilter.java | 14 ++++++++------ .../token/SpringSecurityTokenStore.java | 4 +++- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/authentication/SpringSecurityRequestAuthenticator.java b/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/authentication/SpringSecurityRequestAuthenticator.java index b6c8702298..e9b850d992 100755 --- a/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/authentication/SpringSecurityRequestAuthenticator.java +++ b/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/authentication/SpringSecurityRequestAuthenticator.java @@ -32,6 +32,7 @@ import org.keycloak.adapters.springsecurity.account.SimpleKeycloakAccount; import org.keycloak.adapters.springsecurity.token.KeycloakAuthenticationToken; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import javax.servlet.http.HttpServletRequest; @@ -94,7 +95,10 @@ public class SpringSecurityRequestAuthenticator extends RequestAuthenticator { logger.debug("Completing bearer authentication. Bearer roles: {} ",roles); - SecurityContextHolder.getContext().setAuthentication(new KeycloakAuthenticationToken(account, false)); + SecurityContext context = SecurityContextHolder.createEmptyContext(); + context.setAuthentication(new KeycloakAuthenticationToken(account, false)); + SecurityContextHolder.setContext(context); + request.setAttribute(KeycloakSecurityContext.class.getName(), securityContext); } diff --git a/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/filter/KeycloakAuthenticationProcessingFilter.java b/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/filter/KeycloakAuthenticationProcessingFilter.java index cda70b54af..09e6a513fd 100644 --- a/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/filter/KeycloakAuthenticationProcessingFilter.java +++ b/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/filter/KeycloakAuthenticationProcessingFilter.java @@ -49,6 +49,7 @@ import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter; import org.springframework.security.web.util.matcher.AntPathRequestMatcher; @@ -197,14 +198,15 @@ public class KeycloakAuthenticationProcessingFilter extends AbstractAuthenticati log.debug("Authentication success using bearer token/basic authentication. Updating SecurityContextHolder to contain: {}", authResult); } - SecurityContextHolder.getContext().setAuthentication(authResult); - - // Fire event - if (this.eventPublisher != null) { - eventPublisher.publishEvent(new InteractiveAuthenticationSuccessEvent(authResult, this.getClass())); - } + SecurityContext context = SecurityContextHolder.createEmptyContext(); + context.setAuthentication(authResult); + SecurityContextHolder.setContext(context); try { + // Fire event + if (this.eventPublisher != null) { + eventPublisher.publishEvent(new InteractiveAuthenticationSuccessEvent(authResult, this.getClass())); + } chain.doFilter(request, response); } finally { SecurityContextHolder.clearContext(); diff --git a/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/token/SpringSecurityTokenStore.java b/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/token/SpringSecurityTokenStore.java index ea2a1f65fc..a932dda6c3 100755 --- a/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/token/SpringSecurityTokenStore.java +++ b/adapters/oidc/spring-security/src/main/java/org/keycloak/adapters/springsecurity/token/SpringSecurityTokenStore.java @@ -105,7 +105,9 @@ public class SpringSecurityTokenStore implements AdapterTokenStore { } logger.debug("Saving account info {}", account); - SecurityContextHolder.getContext().setAuthentication(new KeycloakAuthenticationToken(account, true)); + SecurityContext context = SecurityContextHolder.createEmptyContext(); + context.setAuthentication(new KeycloakAuthenticationToken(account, true)); + SecurityContextHolder.setContext(context); } @Override