From 7b227ae47c7b9fa3bd9db40a7d4eb70614b574d8 Mon Sep 17 00:00:00 2001 From: Takashi Norimatsu Date: Wed, 7 Apr 2021 13:25:58 +0900 Subject: [PATCH] KEYCLOAK-17666 Client Policy - Executor : Limiting available period of Request Object --- .../executor/SecureRequestObjectExecutor.java | 19 +++++++++++++++ .../client/AbstractClientPoliciesTest.java | 2 +- .../testsuite/client/ClientPoliciesTest.java | 24 +++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRequestObjectExecutor.java b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRequestObjectExecutor.java index e0370fa713..dbf49ed2fc 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRequestObjectExecutor.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRequestObjectExecutor.java @@ -125,6 +125,25 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Request Expired"); } + // check whether "nbf" claim exists + if (requestObject.get("nbf") == null) { + logger.trace("nbf claim not incuded."); + throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter : nbf"); + } + + // check whether request object not yet being processed + long nbf = requestObject.get("nbf").asLong(); + if (Time.currentTime() < nbf) { // TODO: Time.currentTime() is int while nbf is long... + logger.trace("request object not yet being processed."); + throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Request not yet being processed"); + } + + // check whether request object's available period is short + if (exp - nbf > 3600) { + logger.trace("request object's available period is long."); + throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Request's available period is long"); + } + // check whether "aud" claim exists List aud = new ArrayList(); JsonNode audience = requestObject.get("aud"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java index ef360d1cdf..b68bd601bd 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java @@ -520,7 +520,7 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { requestObject.id(KeycloakModelUtils.generateId()); requestObject.iat(Long.valueOf(Time.currentTime())); requestObject.exp(requestObject.getIat() + Long.valueOf(300)); - requestObject.nbf(Long.valueOf(0)); + requestObject.nbf(requestObject.getIat()); requestObject.setClientId(clientId); requestObject.setResponseType("code"); requestObject.setRedirectUriParam(oauth.getRedirectUri()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java index aa59f57b63..052f6175ef 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java @@ -982,6 +982,30 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Request Expired", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); + // check whether "nbf" claim exists + requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); + requestObject.nbf(null); + registerRequestObject(requestObject, clientId, Algorithm.ES256, false); + oauth.openLoginForm(); + assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals("Missing parameter : nbf", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); + + // check whether request object not yet being processed + requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); + requestObject.nbf(requestObject.getNbf() + 600); + registerRequestObject(requestObject, clientId, Algorithm.ES256, false); + oauth.openLoginForm(); + assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals("Request not yet being processed", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); + + // check whether request object's available period is short + requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); + requestObject.exp(requestObject.getNbf() + 3601); + registerRequestObject(requestObject, clientId, Algorithm.ES256, false); + oauth.openLoginForm(); + assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals("Request's available period is long", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); + // check whether "aud" claim exists requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); requestObject.audience((String)null);