From 28aa1d730da8b4c396baae4797ad9d04406c8e94 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Thu, 1 Jun 2023 08:53:44 -0300 Subject: [PATCH] Verify holder of the device code (#21) Closes https://github.com/keycloak/security/issues/32 Co-authored-by: Stian Thorgersen Conflicts: services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java --- .../models/OAuth2DeviceCodeModel.java | 3 +- .../oidc/grants/ciba/CibaGrantType.java | 8 +-- .../oidc/grants/device/DeviceGrantType.java | 17 ++++- .../keycloak/testsuite/client/CIBATest.java | 46 ++++++++++++ .../OAuth2DeviceAuthorizationGrantTest.java | 70 +++++++++++++++++++ 5 files changed, 133 insertions(+), 11 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/models/OAuth2DeviceCodeModel.java b/server-spi-private/src/main/java/org/keycloak/models/OAuth2DeviceCodeModel.java index 34dd97371b..4ea8977c29 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/OAuth2DeviceCodeModel.java +++ b/server-spi-private/src/main/java/org/keycloak/models/OAuth2DeviceCodeModel.java @@ -193,6 +193,8 @@ public class OAuth2DeviceCodeModel { Map result = new HashMap<>(); result.put(REALM_ID, realm.getId()); + result.put(CLIENT_ID, clientId); + if (clientNotificationToken != null) { result.put(CLIENT_NOTIFICATION_TOKEN_NOTE, clientNotificationToken); } @@ -201,7 +203,6 @@ public class OAuth2DeviceCodeModel { } if (denied == null) { - result.put(CLIENT_ID, clientId); result.put(EXPIRATION_NOTE, String.valueOf(expiration)); result.put(POLLING_INTERVAL_NOTE, String.valueOf(pollingInterval)); result.put(SCOPE_NOTE, scope); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantType.java b/services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantType.java index 07d139e2c5..a7c866a8a5 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantType.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantType.java @@ -161,19 +161,13 @@ public class CibaGrantType { throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_GRANT, cpe.getErrorDetail(), Response.Status.BAD_REQUEST); } - OAuth2DeviceCodeModel deviceCode = DeviceGrantType.getDeviceByDeviceCode(session, realm, request.getId()); + OAuth2DeviceCodeModel deviceCode = DeviceGrantType.getDeviceByDeviceCode(session, realm, client, event, request.getId()); if (deviceCode == null) { // Auth Req ID has not put onto cache, no need to remove Auth Req ID. throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_GRANT, "Invalid " + AUTH_REQ_ID, Response.Status.BAD_REQUEST); } - if (!request.getIssuedFor().equals(client.getClientId())) { - logDebug("invalid client.", request); - // the client sending this Auth Req ID does not match the client to which keycloak had issued Auth Req ID. - throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_GRANT, "unauthorized client", Response.Status.BAD_REQUEST); - } - if (deviceCode.isExpired()) { logDebug("expired.", request); throw new CorsErrorResponseException(cors, OAuthErrorException.EXPIRED_TOKEN, "authentication timed out", Response.Status.BAD_REQUEST); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java b/services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java index c3ca0ee18d..c021971b65 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java @@ -48,6 +48,7 @@ import org.keycloak.protocol.oidc.grants.device.clientpolicy.context.DeviceToken import org.keycloak.protocol.oidc.grants.device.endpoints.DeviceEndpoint; import org.keycloak.protocol.oidc.utils.PkceUtils; import org.keycloak.services.CorsErrorResponseException; +import org.keycloak.services.ErrorResponseException; import org.keycloak.services.clientpolicy.ClientPolicyException; import org.keycloak.services.managers.AuthenticationManager; import org.keycloak.services.managers.UserSessionCrossDCManager; @@ -145,10 +146,20 @@ public class DeviceGrantType { return flow != null; } - public static OAuth2DeviceCodeModel getDeviceByDeviceCode(KeycloakSession session, RealmModel realm, String deviceCode) { + public static OAuth2DeviceCodeModel getDeviceByDeviceCode(KeycloakSession session, RealmModel realm, ClientModel client, EventBuilder event, String deviceCode) { SingleUseObjectProvider singleUseStore = session.singleUseObjects(); Map notes = singleUseStore.get(OAuth2DeviceCodeModel.createKey(deviceCode)); - return notes != null ? OAuth2DeviceCodeModel.fromCache(realm, deviceCode, notes) : null; + OAuth2DeviceCodeModel deviceCodeModel = notes != null ? OAuth2DeviceCodeModel.fromCache(realm, deviceCode, notes) : null; + + if (deviceCodeModel != null) { + if (!client.getClientId().equals(deviceCodeModel.getClientId())) { + event.error(Errors.INVALID_OAUTH2_DEVICE_CODE); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "unauthorized client", + Response.Status.BAD_REQUEST); + } + } + + return deviceCodeModel; } public static void removeDeviceByDeviceCode(KeycloakSession session, String deviceCode) { @@ -227,7 +238,7 @@ public class DeviceGrantType { "Missing parameter: " + OAuth2Constants.DEVICE_CODE, Response.Status.BAD_REQUEST); } - OAuth2DeviceCodeModel deviceCodeModel = getDeviceByDeviceCode(session, realm, deviceCode); + OAuth2DeviceCodeModel deviceCodeModel = getDeviceByDeviceCode(session, realm, client, event, deviceCode); if (deviceCodeModel == null) { event.error(Errors.INVALID_OAUTH2_DEVICE_CODE); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/CIBATest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/CIBATest.java index ec7c987a39..0664782c0a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/CIBATest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/CIBATest.java @@ -1104,6 +1104,52 @@ public class CIBATest extends AbstractClientPoliciesTest { } } + @Test + public void testVerifyHolderOfAuthenticationRequestRef() throws Exception { + ClientResource firstClientResource = null; + ClientResource secondClientResource = null; + ClientRepresentation firstClientRep = null; + ClientRepresentation secondClientRep = null; + try { + final String username = "nutzername-gelb"; + final String firstClientName = "test-app-scope"; // see testrealm.json + final String secondClientName = TEST_CLIENT_NAME; + final String firstClientPassword = "password"; + final String secondClientPassword = TEST_CLIENT_PASSWORD; + String firstClientAuthReqId = null; + + firstClientResource = ApiUtil.findClientByClientId(adminClient.realm(TEST_REALM_NAME), firstClientName); + assertThat(firstClientResource, notNullValue()); + + firstClientRep = firstClientResource.toRepresentation(); + prepareCIBASettings(firstClientResource, firstClientRep); + + secondClientResource = ApiUtil.findClientByClientId(adminClient.realm(TEST_REALM_NAME), secondClientName); + assertThat(secondClientResource, notNullValue()); + + secondClientRep = secondClientResource.toRepresentation(); + prepareCIBASettings(secondClientResource, secondClientRep); + + // first client Backchannel Authentication Request + AuthenticationRequestAcknowledgement response = doBackchannelAuthenticationRequest(firstClientName, firstClientPassword, username, "asdfghjkl"); + firstClientAuthReqId = response.getAuthReqId(); + + // first client Authentication Channel Request + TestAuthenticationChannelRequest firstClientAuthenticationChannelReq = doAuthenticationChannelRequest("asdfghjkl"); + + // first client Authentication Channel completed + doAuthenticationChannelCallback(firstClientAuthenticationChannelReq); + + // second client Token Request + OAuthClient.AccessTokenResponse tokenRes = oauth.doBackchannelAuthenticationTokenRequest(secondClientName, secondClientPassword, firstClientAuthReqId); + assertEquals(400, tokenRes.getStatusCode()); + assertEquals("unauthorized client", tokenRes.getErrorDescription()); + } finally { + revertCIBASettings(firstClientResource, firstClientRep); + revertCIBASettings(secondClientResource, secondClientRep); + } + } + @Test public void testRequestTokenBeforeAuthenticationNotCompleted() throws Exception { ClientResource clientResource = null; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuth2DeviceAuthorizationGrantTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuth2DeviceAuthorizationGrantTest.java index 783def737d..bcc8fa9411 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuth2DeviceAuthorizationGrantTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuth2DeviceAuthorizationGrantTest.java @@ -16,6 +16,7 @@ */ package org.keycloak.testsuite.oauth; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.keycloak.models.OAuth2DeviceConfig.DEFAULT_OAUTH2_DEVICE_CODE_LIFESPAN; @@ -59,11 +60,13 @@ import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.message.BasicNameValuePair; import org.apache.http.impl.client.CloseableHttpClient; import org.keycloak.util.BasicAuthHelper; +import org.openqa.selenium.Cookie; import java.util.List; import java.util.LinkedList; import java.io.UnsupportedEncodingException; +import java.util.concurrent.TimeUnit; /** * @author Hiroyuki Wada @@ -106,6 +109,7 @@ public class OAuth2DeviceAuthorizationGrantTest extends AbstractKeycloakTest { ClientRepresentation appPublic = ClientBuilder.create().id(KeycloakModelUtils.generateId()).publicClient() .clientId(DEVICE_APP_PUBLIC).attribute(OAuth2DeviceConfig.OAUTH2_DEVICE_AUTHORIZATION_GRANT_ENABLED, "true") + .redirectUris(OAuthClient.APP_ROOT + "/auth") .build(); realm.client(appPublic); @@ -243,6 +247,72 @@ public class OAuth2DeviceAuthorizationGrantTest extends AbstractKeycloakTest { } } + @Test + public void testVerifyHolderOfDeviceCode() throws Exception { + // Device Authorization Request from device + oauth.realm(REALM_NAME); + oauth.clientId(DEVICE_APP_PUBLIC); + OAuthClient.DeviceAuthorizationResponse response = oauth.doDeviceAuthorizationRequest(DEVICE_APP_PUBLIC, null); + + assertEquals(200, response.getStatusCode()); + assertNotNull(response.getDeviceCode()); + assertNotNull(response.getUserCode()); + assertNotNull(response.getVerificationUri()); + assertNotNull(response.getVerificationUriComplete()); + assertEquals(60, response.getExpiresIn()); + assertEquals(5, response.getInterval()); + + openVerificationPage(response.getVerificationUriComplete()); + + // Do Login + oauth.fillLoginForm("device-login", "password"); + + // Consent + grantPage.accept(); + + // Token request from device + OAuthClient.AccessTokenResponse tokenResponse = oauth.doDeviceTokenRequest(DEVICE_APP_PUBLIC, null, response.getDeviceCode()); + + assertEquals(200, tokenResponse.getStatusCode()); + + String tokenString = tokenResponse.getAccessToken(); + assertNotNull(tokenString); + AccessToken token = oauth.verifyToken(tokenString); + + assertNotNull(token); + + for (Cookie cookie : driver.manage().getCookies()) { + driver.manage().deleteCookie(cookie); + } + + oauth.openLoginForm(); + + oauth.realm(REALM_NAME); + oauth.clientId(DEVICE_APP_PUBLIC_CUSTOM_CONSENT); + + oauth.fillLoginForm("device-login", "password"); + + for (Cookie cookie : driver.manage().getCookies()) { + driver.manage().deleteCookie(cookie); + } + + oauth.openLoginForm(); + + response = oauth.doDeviceAuthorizationRequest(DEVICE_APP_PUBLIC_CUSTOM_CONSENT, null); + + openVerificationPage(response.getVerificationUriComplete()); + + // Consent + Assert.assertTrue(grantPage.getDisplayedGrants().contains("This is the custom consent screen text.")); + grantPage.accept(); + + // Token request from device + tokenResponse = oauth.doDeviceTokenRequest(DEVICE_APP_PUBLIC, null, response.getDeviceCode()); + + assertEquals(400, tokenResponse.getStatusCode()); + assertEquals("unauthorized client", tokenResponse.getErrorDescription()); + } + @Test public void testPublicClientOptionalScope() throws Exception { // Device Authorization Request from device - check giving optional scope phone