From acfbdf6b0e311db6f7b6f5e4e4621abbb1f964a7 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Mon, 9 Mar 2020 10:54:19 -0300 Subject: [PATCH] [KEYCLOAK-13187] - Concurrency issue when refreshing tokens and updating security context state --- .../RefreshableKeycloakSecurityContext.java | 98 +++++++++++-------- .../adapter/page/TokenRefreshPage.java | 40 ++++++++ .../servlet/DemoServletsAdapterTest.java | 73 ++++++++++++++ .../resources/adapter-test/demorealm.json | 10 ++ .../token-refresh/META-INF/context.xml | 20 ++++ .../token-refresh/WEB-INF/jetty-web.xml | 46 +++++++++ .../token-refresh/WEB-INF/keycloak.json | 9 ++ .../token-refresh/WEB-INF/web.xml | 93 ++++++++++++++++++ 8 files changed, 346 insertions(+), 43 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/TokenRefreshPage.java create mode 100644 testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/META-INF/context.xml create mode 100644 testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/jetty-web.xml create mode 100644 testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/keycloak.json create mode 100644 testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/web.xml diff --git a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/RefreshableKeycloakSecurityContext.java b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/RefreshableKeycloakSecurityContext.java index 1a3851d1eb..3d4ad9cc5c 100755 --- a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/RefreshableKeycloakSecurityContext.java +++ b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/RefreshableKeycloakSecurityContext.java @@ -114,51 +114,63 @@ public class RefreshableKeycloakSecurityContext extends KeycloakSecurityContext if (log.isTraceEnabled()) { log.trace("Doing refresh"); } - AccessTokenResponse response = null; - try { - response = ServerRequest.invokeRefresh(deployment, refreshToken); - } catch (IOException e) { - log.error("Refresh token failure", e); - return false; - } catch (ServerRequest.HttpFailure httpFailure) { - log.error("Refresh token failure status: " + httpFailure.getStatus() + " " + httpFailure.getError()); - return false; - } - if (log.isTraceEnabled()) { - log.trace("received refresh response"); - } - String tokenString = response.getToken(); - AccessToken token = null; - try { - AdapterTokenVerifier.VerifiedTokens tokens = AdapterTokenVerifier.verifyTokens(tokenString, response.getIdToken(), deployment); - token = tokens.getAccessToken(); - log.debug("Token Verification succeeded!"); - } catch (VerificationException e) { - log.error("failed verification of token"); - return false; - } - - // If the TTL is greater-or-equal to the expire time on the refreshed token, have to abort or go into an infinite refresh loop - if (!isTokenTimeToLiveSufficient(token)) { - log.error("failed to refresh the token with a longer time-to-live than the minimum"); - return false; - } - - if (response.getNotBeforePolicy() > deployment.getNotBefore()) { - deployment.updateNotBefore(response.getNotBeforePolicy()); - } - - this.token = token; - if (response.getRefreshToken() != null) { - if (log.isTraceEnabled()) { - log.trace("Setup new refresh token to the security context"); + + // block requests if the refresh token herein stored is already being used to refresh the token so that subsequent requests + // can use the last refresh token issued by the server. Note that this will only work for deployments using the session store + // and, when running in a cluster, sticky sessions must be used. + // + synchronized (this) { + if (checkActive) { + log.trace("Checking whether token has been refreshed in another thread already."); + if (isActive() && isTokenTimeToLiveSufficient(this.token)) return true; + } + AccessTokenResponse response; + try { + response = ServerRequest.invokeRefresh(deployment, refreshToken); + } catch (IOException e) { + log.error("Refresh token failure", e); + return false; + } catch (ServerRequest.HttpFailure httpFailure) { + log.error("Refresh token failure status: " + httpFailure.getStatus() + " " + httpFailure.getError()); + return false; + } + if (log.isTraceEnabled()) { + log.trace("received refresh response"); + } + String tokenString = response.getToken(); + AccessToken token = null; + try { + AdapterTokenVerifier.VerifiedTokens tokens = AdapterTokenVerifier.verifyTokens(tokenString, response.getIdToken(), deployment); + token = tokens.getAccessToken(); + log.debug("Token Verification succeeded!"); + } catch (VerificationException e) { + log.error("failed verification of token"); + return false; + } + + // If the TTL is greater-or-equal to the expire time on the refreshed token, have to abort or go into an infinite refresh loop + if (!isTokenTimeToLiveSufficient(token)) { + log.error("failed to refresh the token with a longer time-to-live than the minimum"); + return false; + } + + if (response.getNotBeforePolicy() > deployment.getNotBefore()) { + deployment.updateNotBefore(response.getNotBeforePolicy()); + } + + this.token = token; + if (response.getRefreshToken() != null) { + if (log.isTraceEnabled()) { + log.trace("Setup new refresh token to the security context"); + } + this.refreshToken = response.getRefreshToken(); + } + this.tokenString = tokenString; + if (tokenStore != null) { + tokenStore.refreshCallback(this); } - this.refreshToken = response.getRefreshToken(); - } - this.tokenString = tokenString; - if (tokenStore != null) { - tokenStore.refreshCallback(this); } + return true; } diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/TokenRefreshPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/TokenRefreshPage.java new file mode 100644 index 0000000000..9ff7aa37b6 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/TokenRefreshPage.java @@ -0,0 +1,40 @@ +/* + * Copyright 2016 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.testsuite.adapter.page; + +import org.jboss.arquillian.container.test.api.OperateOnDeployment; +import org.jboss.arquillian.test.api.ArquillianResource; + +import java.net.URL; + +/** + * @author Marek Posolda + */ +public class TokenRefreshPage extends AbstractShowTokensPage { + + public static final String DEPLOYMENT_NAME = "token-refresh"; + + @ArquillianResource + @OperateOnDeployment(DEPLOYMENT_NAME) + private URL url; + + @Override + public URL getInjectedUrl() { + return url; + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/DemoServletsAdapterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/DemoServletsAdapterTest.java index bba4e7670f..c7da6e4c9e 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/DemoServletsAdapterTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/DemoServletsAdapterTest.java @@ -17,7 +17,15 @@ package org.keycloak.testsuite.adapter.servlet; import org.apache.commons.io.FileUtils; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.client.utils.URLEncodedUtils; +import org.apache.http.impl.client.BasicCookieStore; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.impl.cookie.BasicClientCookie; +import org.apache.http.util.EntityUtils; import org.jboss.arquillian.container.test.api.Deployment; import org.jboss.arquillian.drone.api.annotation.Drone; import org.jboss.arquillian.graphene.page.Page; @@ -30,6 +38,7 @@ import org.junit.Test; import org.keycloak.OAuth2Constants; import org.keycloak.adapters.OIDCAuthenticationError; import org.keycloak.admin.client.resource.ClientResource; +import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.common.util.Time; import org.keycloak.constants.AdapterConstants; import org.keycloak.events.Details; @@ -64,6 +73,7 @@ import org.keycloak.testsuite.adapter.page.SecurePortal; import org.keycloak.testsuite.adapter.page.SecurePortalRewriteRedirectUri; import org.keycloak.testsuite.adapter.page.SecurePortalWithCustomSessionConfig; import org.keycloak.testsuite.adapter.page.TokenMinTTLPage; +import org.keycloak.testsuite.adapter.page.TokenRefreshPage; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.arquillian.annotation.AppServerContainer; import org.keycloak.testsuite.utils.arquillian.ContainerConstants; @@ -96,10 +106,14 @@ import java.io.File; import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -171,6 +185,8 @@ public class DemoServletsAdapterTest extends AbstractServletsAdapterTest { @Page private TokenMinTTLPage tokenMinTTLPage; @Page + private TokenRefreshPage tokenRefreshPage; + @Page private OAuthGrant oAuthGrantPage; @Page protected Applications applicationsPage; @@ -262,6 +278,11 @@ public class DemoServletsAdapterTest extends AbstractServletsAdapterTest { return servletDeployment(TokenMinTTLPage.DEPLOYMENT_NAME, AdapterActionsFilter.class, AbstractShowTokensServlet.class, TokenMinTTLServlet.class, ErrorServlet.class); } + @Deployment(name = TokenRefreshPage.DEPLOYMENT_NAME) + protected static WebArchive tokenRefresh() { + return servletDeployment(TokenRefreshPage.DEPLOYMENT_NAME, AdapterActionsFilter.class, AbstractShowTokensServlet.class, TokenMinTTLServlet.class, ErrorServlet.class); + } + @Deployment(name = BasicAuth.DEPLOYMENT_NAME) protected static WebArchive basicAuth() { return servletDeployment(BasicAuth.DEPLOYMENT_NAME, BasicAuthServlet.class); @@ -755,6 +776,58 @@ public class DemoServletsAdapterTest extends AbstractServletsAdapterTest { setAdapterAndServerTimeOffset(0, tokenMinTTLPage.toString()); } + @Test + public void testTokenConcurrentRefresh() { + RealmResource demoRealm = adminClient.realm("demo"); + RealmRepresentation demo = demoRealm.toRepresentation(); + + demo.setAccessTokenLifespan(2); + demo.setRevokeRefreshToken(true); + demo.setRefreshTokenMaxReuse(0); + + demoRealm.update(demo); + + // Login + tokenRefreshPage.navigateTo(); + assertTrue(testRealmLoginPage.form().isUsernamePresent()); + assertCurrentUrlStartsWithLoginUrlOf(testRealmPage); + testRealmLoginPage.form().login("bburke@redhat.com", "password"); + assertCurrentUrlEquals(tokenRefreshPage); + + // Revert times + setAdapterAndServerTimeOffset(5, tokenRefreshPage.toString()); + + BasicCookieStore cookieStore = new BasicCookieStore(); + BasicClientCookie jsessionid = new BasicClientCookie("JSESSIONID", driver.manage().getCookieNamed("JSESSIONID").getValue()); + + jsessionid.setDomain("localhost"); + jsessionid.setPath("/"); + cookieStore.addCookie(jsessionid); + + ExecutorService executor = Executors.newWorkStealingPool(); + CompletableFuture future = CompletableFuture.completedFuture(null); + + try { + for (int i = 0; i < 5; i++) { + future = CompletableFuture.allOf(future, CompletableFuture.runAsync(() -> { + try (CloseableHttpClient client = HttpClientBuilder.create().setDefaultCookieStore(cookieStore) + .build()) { + HttpUriRequest request = new HttpGet(tokenRefreshPage.getInjectedUrl().toString()); + try (CloseableHttpResponse httpResponse = client.execute(request)) { + assertTrue("Token not refreshed", EntityUtils.toString(httpResponse.getEntity()).contains("accessToken")); + } + } catch (Exception e) { + throw new RuntimeException(e); + } + }, executor)); + } + + future.join(); + } finally { + executor.shutdownNow(); + } + } + // Tests forwarding of parameters like "prompt" @Test public void testOIDCParamsForwarding() { diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/demorealm.json b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/demorealm.json index c5b335894f..6af989d114 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/demorealm.json +++ b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/demorealm.json @@ -323,6 +323,16 @@ ], "secret": "password" }, + { + "clientId": "token-refresh", + "enabled": true, + "adminUrl": "/token-refresh", + "baseUrl": "/token-refresh", + "redirectUris": [ + "/token-refresh/*" + ], + "secret": "password" + }, { "clientId": "third-party", "enabled": true, diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/META-INF/context.xml b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/META-INF/context.xml new file mode 100644 index 0000000000..8050b11f39 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/META-INF/context.xml @@ -0,0 +1,20 @@ + + + + + \ No newline at end of file diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/jetty-web.xml b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/jetty-web.xml new file mode 100644 index 0000000000..8c59313878 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/jetty-web.xml @@ -0,0 +1,46 @@ + + + + + + + + + + + + + \ No newline at end of file diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/keycloak.json b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/keycloak.json new file mode 100644 index 0000000000..8b50173f8f --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/keycloak.json @@ -0,0 +1,9 @@ +{ + "realm": "demo", + "resource": "token-refresh", + "auth-server-url": "http://localhost:8180/auth", + "ssl-required" : "external", + "credentials": { + "secret": "password" + } +} \ No newline at end of file diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/web.xml b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/web.xml new file mode 100644 index 0000000000..dbe6ef9850 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/token-refresh/WEB-INF/web.xml @@ -0,0 +1,93 @@ + + + + + + token-refresh + + + AdapterActionsFilter + org.keycloak.testsuite.adapter.filter.AdapterActionsFilter + + + + Servlet + org.keycloak.testsuite.adapter.servlet.TokenMinTTLServlet + + + + Error Servlet + org.keycloak.testsuite.adapter.servlet.ErrorServlet + + + + AdapterActionsFilter + /* + + + + Servlet + /* + + + + Error Servlet + /error.html + + + + + Users + /* + + + user + + + + + Errors + /error.html + + + + + Unsecured + /unsecured/* + + + + + KEYCLOAK + test + + /error.html + /error.html + + + + + admin + + + user + +