From 1f5ee9bf808b3239d2b6d5a0974d1ce6ce1e8f4b Mon Sep 17 00:00:00 2001 From: Takashi Norimatsu Date: Sun, 26 Nov 2023 11:02:25 +0900 Subject: [PATCH] NPE in checkAndBindMtlsHoKToken on Token Refresh when using SuppressRefreshTokenRotationExecutor and Certificate Bound Token closes #25022 Signed-off-by: Takashi Norimatsu --- .../oidc/endpoints/TokenEndpoint.java | 4 +- .../client/policies/ClientPoliciesTest.java | 62 ++++++++++++++++++- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java index e867a054e8..914304b8cc 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java @@ -566,11 +566,11 @@ public class TokenEndpoint { // KEYCLOAK-6771 Certificate Bound Token TokenManager.AccessTokenResponseBuilder responseBuilder = tokenManager.refreshAccessToken(session, session.getContext().getUri(), clientConnection, realm, client, refreshToken, event, headers, request); - session.clientPolicy().triggerOnEvent(new TokenRefreshResponseContext(formParams, responseBuilder)); - checkAndBindMtlsHoKToken(responseBuilder, clientConfig.isUseRefreshToken()); checkAndBindDPoPToken(responseBuilder, clientConfig.isUseRefreshToken() && (client.isPublicClient() || client.isBearerOnly()), Profile.isFeatureEnabled(Profile.Feature.DPOP)); + session.clientPolicy().triggerOnEvent(new TokenRefreshResponseContext(formParams, responseBuilder)); + res = responseBuilder.build(); if (!responseBuilder.isOfflineToken()) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesTest.java index ff76004038..1ce8da09e5 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesTest.java @@ -25,6 +25,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.keycloak.testsuite.admin.AbstractAdminTest.loadJson; @@ -44,6 +45,7 @@ import static org.keycloak.testsuite.util.ClientPoliciesUtil.createSecureClientA import static org.keycloak.testsuite.util.ClientPoliciesUtil.createSecureSigningAlgorithmForSignedJwtEnforceExecutorConfig; import static org.keycloak.testsuite.util.ClientPoliciesUtil.createTestRaiseExeptionConditionConfig; +import java.io.IOException; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -53,7 +55,7 @@ import java.util.Map; import jakarta.ws.rs.core.Response; -import org.hamcrest.Matchers; +import org.apache.http.impl.client.CloseableHttpClient; import org.jboss.logging.Logger; import org.junit.Assert; import org.junit.Assume; @@ -111,11 +113,13 @@ import org.keycloak.services.clientpolicy.executor.RejectResourceOwnerPasswordCr import org.keycloak.services.clientpolicy.executor.SecureClientAuthenticatorExecutorFactory; import org.keycloak.services.clientpolicy.executor.SecureSessionEnforceExecutorFactory; import org.keycloak.services.clientpolicy.executor.SecureSigningAlgorithmForSignedJwtExecutorFactory; +import org.keycloak.services.clientpolicy.executor.SuppressRefreshTokenRotationExecutorFactory; import org.keycloak.testsuite.arquillian.annotation.EnableFeature; import org.keycloak.testsuite.client.resources.TestApplicationResourceUrls; import org.keycloak.testsuite.services.clientpolicy.condition.TestRaiseExceptionConditionFactory; import org.keycloak.testsuite.updaters.ClientAttributeUpdater; import org.keycloak.testsuite.util.ClientBuilder; +import org.keycloak.testsuite.util.MutualTLSUtils; import org.keycloak.testsuite.util.ClientPoliciesUtil.ClientPoliciesBuilder; import org.keycloak.testsuite.util.ClientPoliciesUtil.ClientPolicyBuilder; import org.keycloak.testsuite.util.ClientPoliciesUtil.ClientProfileBuilder; @@ -574,6 +578,62 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { } } + @Test + public void testSuppressRefreshTokenRotationWithHolderOfKeyToken() throws Exception { + Assume.assumeTrue("This test must be executed with enabled TLS.", ServerURLs.AUTH_SERVER_SSL_REQUIRED); + + // register profiles + String json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "Le Premier Profil") + .addExecutor(SuppressRefreshTokenRotationExecutorFactory.PROVIDER_ID, null) + .addExecutor(HolderOfKeyEnforcerExecutorFactory.PROVIDER_ID, + createHolderOfKeyEnforceExecutorConfig(Boolean.TRUE)) + .toRepresentation() + ).toString(); + updateProfiles(json); + + // register policies + json = (new ClientPoliciesBuilder()).addPolicy( + (new ClientPolicyBuilder()).createPolicy(POLICY_NAME, "Den Forste Politikken", Boolean.TRUE) + .addCondition(ClientRolesConditionFactory.PROVIDER_ID, + createClientRolesConditionConfig(Arrays.asList(SAMPLE_CLIENT_ROLE))) + .addProfile(PROFILE_NAME) + .toRepresentation() + ).toString(); + updatePolicies(json); + + try (ClientAttributeUpdater cau = ClientAttributeUpdater.forClient(adminClient, REALM_NAME, TEST_CLIENT)) { + ClientRepresentation clientRep = cau.getResource().toRepresentation(); + Assert.assertNotNull(clientRep); + OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).setUseMtlsHoKToken(true); + cau.update(); + // Check login. + OAuthClient.AuthorizationEndpointResponse loginResponse = oauth.doLogin(TEST_USER_NAME, TEST_USER_PASSWORD); + Assert.assertNull(loginResponse.getError()); + + String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + + // Check token obtaining. + OAuthClient.AccessTokenResponse accessTokenResponse; + try (CloseableHttpClient client = MutualTLSUtils.newCloseableHttpClientWithDefaultKeyStoreAndTrustStore()) { + accessTokenResponse = oauth.doAccessTokenRequest(code, TEST_CLIENT_SECRET, client); + } catch (IOException ioe) { + throw new RuntimeException(ioe); + } + assertEquals(200, accessTokenResponse.getStatusCode()); + + // Check token refresh. + OAuthClient.AccessTokenResponse accessTokenResponseRefreshed; + try (CloseableHttpClient client = MutualTLSUtils.newCloseableHttpClientWithDefaultKeyStoreAndTrustStore()) { + accessTokenResponseRefreshed = oauth.doRefreshTokenRequest(accessTokenResponse.getRefreshToken(), TEST_CLIENT_SECRET, client); + } catch (IOException ioe) { + throw new RuntimeException(ioe); + } + assertEquals(200, accessTokenResponseRefreshed.getStatusCode()); + assertNull(accessTokenResponseRefreshed.getRefreshToken()); + } + } + @Test public void testNegativeLogicCondition() throws Exception { // register profiles