KEYCLOAK-14019 Improvements for request_uri parameter

(cherry picked from commit da38b36297a5bd9890f7df031696b516268d6cff)
keycloak · Jan 18, 2021 · eac3329 · eac3329
1 parent acfea8e
commit eac3329
Show file tree

Hide file tree

Showing 15 changed files with 214 additions and 4 deletions.
diff --git a/...main/java/org/keycloak/protocol/oidc/representations/OIDCConfigurationRepresentation.java b/...main/java/org/keycloak/protocol/oidc/representations/OIDCConfigurationRepresentation.java
@@ -109,6 +109,9 @@ public class OIDCConfigurationRepresentation {
     @JsonProperty("request_uri_parameter_supported")
     private Boolean requestUriParameterSupported;
 
+    @JsonProperty("require_request_uri_registration")
+    private Boolean requireRequestUriRegistration;
+
     // KEYCLOAK-7451 OAuth Authorization Server Metadata for Proof Key for Code Exchange
     @JsonProperty("code_challenge_methods_supported")
     private List<String> codeChallengeMethodsSupported;
@@ -343,6 +346,14 @@ public void setRequestUriParameterSupported(Boolean requestUriParameterSupported
         this.requestUriParameterSupported = requestUriParameterSupported;
     }
 
+    public Boolean getRequireRequestUriRegistration() {
+        return requireRequestUriRegistration;
+    }
+
+    public void setRequireRequestUriRegistration(Boolean requireRequestUriRegistration) {
+        this.requireRequestUriRegistration = requireRequestUriRegistration;
+    }
+
     // KEYCLOAK-7451 OAuth Authorization Server Metadata for Proof Key for Code Exchange
     public List<String> getCodeChallengeMethodsSupported() {
         return codeChallengeMethodsSupported;

diff --git a/services/src/main/java/org/keycloak/protocol/oidc/OIDCAdvancedConfigWrapper.java b/services/src/main/java/org/keycloak/protocol/oidc/OIDCAdvancedConfigWrapper.java
@@ -20,9 +20,13 @@
 import org.keycloak.authentication.authenticators.client.X509ClientAuthenticator;
 import org.keycloak.jose.jws.Algorithm;
 import org.keycloak.models.ClientModel;
+import org.keycloak.models.Constants;
 import org.keycloak.representations.idm.ClientRepresentation;
 
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 
 /**
  * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
@@ -79,6 +83,14 @@ public void setRequestObjectRequired(String requestObjectRequired) {
         setAttribute(OIDCConfigAttributes.REQUEST_OBJECT_REQUIRED, requestObjectRequired);
     }
 
+    public List<String> getRequestUris() {
+        return getAttributeMultivalued(OIDCConfigAttributes.REQUEST_URIS);
+    }
+
+    public void setRequestUris(List<String> requestUris) {
+        setAttributeMultivalued(OIDCConfigAttributes.REQUEST_URIS, requestUris);
+    }
+
     public boolean isUseJwksUrl() {
         String useJwksUrl = getAttribute(OIDCConfigAttributes.USE_JWKS_URL);
         return Boolean.parseBoolean(useJwksUrl);
@@ -244,4 +256,20 @@ private void setAttribute(String attrKey, String attrValue) {
             }
         }
     }
+
+    private List<String> getAttributeMultivalued(String attrKey) {
+        String attrValue = getAttribute(attrKey);
+        if (attrValue == null) return Collections.emptyList();
+        return Arrays.asList(Constants.CFG_DELIMITER_PATTERN.split(attrValue));
+    }
+
+    private void setAttributeMultivalued(String attrKey, List<String> attrValues) {
+        if (attrValues == null || attrValues.size() == 0) {
+            // Remove attribute
+            setAttribute(attrKey, null);
+        } else {
+            String attrValueFull = String.join(Constants.CFG_DELIMITER, attrValues);
+            setAttribute(attrKey, attrValueFull);
+        }
+    }
 }
diff --git a/services/src/main/java/org/keycloak/protocol/oidc/OIDCConfigAttributes.java b/services/src/main/java/org/keycloak/protocol/oidc/OIDCConfigAttributes.java
@@ -27,6 +27,8 @@ public final class OIDCConfigAttributes {
     public static final String REQUEST_OBJECT_REQUIRED_REQUEST = "request only";
     public static final String REQUEST_OBJECT_REQUIRED_REQUEST_URI = "request_uri only";
 
+    public static final String REQUEST_URIS = "request.uris";
+
     public static final String JWKS_URL = "jwks.url";
 
     public static final String USE_JWKS_URL = "use.jwks.url";

diff --git a/services/src/main/java/org/keycloak/protocol/oidc/OIDCWellKnownProvider.java b/services/src/main/java/org/keycloak/protocol/oidc/OIDCWellKnownProvider.java
@@ -133,6 +133,7 @@ public Object getConfig() {
 
         config.setRequestParameterSupported(true);
         config.setRequestUriParameterSupported(true);
+        config.setRequireRequestUriRegistration(true);
 
         // KEYCLOAK-7451 OAuth Authorization Server Metadata for Proof Key for Code Exchange
         config.setCodeChallengeMethodsSupported(DEFAULT_CODE_CHALLENGE_METHODS_SUPPORTED);

diff --git a/...keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequestParserProcessor.java b/...keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequestParserProcessor.java
@@ -26,13 +26,15 @@
 import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper;
 import org.keycloak.protocol.oidc.OIDCConfigAttributes;
 import org.keycloak.protocol.oidc.OIDCLoginProtocol;
+import org.keycloak.protocol.oidc.utils.RedirectUtils;
 import org.keycloak.services.ErrorPageException;
 import org.keycloak.services.ServicesLogger;
 import org.keycloak.services.messages.Messages;
 
 import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
 import java.io.InputStream;
+import java.util.HashSet;
 import java.util.List;
 
 /**
@@ -75,7 +77,14 @@ public static AuthorizationEndpointRequest parseRequest(EventBuilder event, Keyc
             if (requestParam != null) {
                 new AuthzEndpointRequestObjectParser(session, requestParam, client).parseRequest(request);
             } else if (requestUriParam != null) {
-                try (InputStream is = session.getProvider(HttpClientProvider.class).get(requestUriParam)) {
+                // Validate "requestUriParam" with allowed requestUris
+                List<String> requestUris = OIDCAdvancedConfigWrapper.fromClientModel(client).getRequestUris();
+                String requestUri = RedirectUtils.verifyRedirectUri(session, client.getRootUrl(), requestUriParam, new HashSet<>(requestUris), false);
+                if (requestUri == null) {
+                    throw new RuntimeException("Specified 'request_uri' not allowed for this client.");
+                }
+
+                try (InputStream is = session.getProvider(HttpClientProvider.class).get(requestUri)) {
                     String retrievedRequest = StreamUtil.readString(is);
                     new AuthzEndpointRequestObjectParser(session, retrievedRequest, client).parseRequest(request);
                 }

diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java
@@ -78,7 +78,7 @@ private static Set<String> getValidateRedirectUris(KeycloakSession session) {
                 .collect(Collectors.toSet());
     }
 
-    private static String verifyRedirectUri(KeycloakSession session, String rootUrl, String redirectUri, Set<String> validRedirects, boolean requireRedirectUri) {
+    public static String verifyRedirectUri(KeycloakSession session, String rootUrl, String redirectUri, Set<String> validRedirects, boolean requireRedirectUri) {
         KeycloakUriInfo uriInfo = session.getContext().getUri();
         RealmModel realm = session.getContext().getRealm();
 

diff --git a/...ces/src/main/java/org/keycloak/services/clientregistration/oidc/DescriptionConverter.java b/...ces/src/main/java/org/keycloak/services/clientregistration/oidc/DescriptionConverter.java
@@ -142,6 +142,10 @@ public static ClientRepresentation toInternal(KeycloakSession session, OIDCClien
             configWrapper.setIdTokenEncryptedResponseEnc(clientOIDC.getIdTokenEncryptedResponseEnc());
         }
 
+        if (clientOIDC.getRequestUris() != null) {
+            configWrapper.setRequestUris(clientOIDC.getRequestUris());
+        }
+
         configWrapper.setTokenEndpointAuthSigningAlg(clientOIDC.getTokenEndpointAuthSigningAlg());
 
         configWrapper.setBackchannelLogoutUrl(clientOIDC.getBackchannelLogoutUri());
@@ -253,6 +257,9 @@ public static OIDCClientRepresentation toExternalResponse(KeycloakSession sessio
         if (config.getIdTokenEncryptedResponseEnc() != null) {
             response.setIdTokenEncryptedResponseEnc(config.getIdTokenEncryptedResponseEnc());
         }
+        if (config.getRequestUris() != null) {
+            response.setRequestUris(config.getRequestUris());
+        }
         if (config.getTokenEndpointAuthSigningAlg() != null) {
             response.setTokenEndpointAuthSigningAlg(config.getTokenEndpointAuthSigningAlg());
         }

diff --git a/...illian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPolicyBasicsTest.java b/...illian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPolicyBasicsTest.java
@@ -721,6 +721,7 @@ public void testSecureRequestObjectExecutor() throws ClientRegistrationException
         String clientSecret = "secret";
         String cid = createClientByAdmin(clientId, (ClientRepresentation clientRep) -> {
             clientRep.setSecret(clientSecret);
+            OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).setRequestUris(Arrays.asList(TestApplicationResourceUrls.clientRequestUri()));
         });
         adminClient.realm(REALM_NAME).clients().get(cid).roles().create(RoleBuilder.create().name("sample-client-role").build());
 

diff --git a/...an/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCClientRegistrationTest.java b/...an/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCClientRegistrationTest.java
@@ -531,4 +531,21 @@ public void testClientWithoutScope() throws ClientRegistrationException {
         assertTrue(realmDefaultClientScopes.equals(new HashSet<>(registeredDefaultClientScopes)));
 
     }
+
+    @Test
+    public void testRequestUris() throws Exception {
+        OIDCClientRepresentation clientRep = null;
+        OIDCClientRepresentation response = null;
+
+        clientRep = createRep();
+        clientRep.setRequestUris(Arrays.asList("http://host/foo", "https://host2/bar"));
+
+        response = reg.oidc().create(clientRep);
+        Assert.assertNames(response.getRequestUris(), "http://host/foo", "https://host2/bar");
+
+        // Test Keycloak representation
+        ClientRepresentation kcClient = getClient(response.getClientId());
+        OIDCAdvancedConfigWrapper config = OIDCAdvancedConfigWrapper.fromClientRepresentation(kcClient);
+        Assert.assertNames(config.getRequestUris(), "http://host/foo", "https://host2/bar");
+    }
 }
diff --git a/...n/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCAdvancedRequestParamsTest.java b/...n/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCAdvancedRequestParamsTest.java
@@ -30,6 +30,7 @@
 import org.keycloak.authentication.authenticators.client.JWTClientAuthenticator;
 import org.keycloak.common.Profile;
 import org.keycloak.common.util.Time;
+import org.keycloak.common.util.UriUtils;
 import org.keycloak.events.Details;
 import org.keycloak.events.EventType;
 import org.keycloak.jose.jws.Algorithm;
@@ -121,7 +122,10 @@ public void configureTestRealm(RealmRepresentation testRealm) {
 
     @Before
     public void clientConfiguration() {
-        ClientManager.realm(adminClient.realm("test")).clientId("test-app").directAccessGrant(true);
+        ClientManager.realm(adminClient.realm("test")).clientId("test-app")
+                .directAccessGrant(true)
+                .setRequestUris(TestApplicationResourceUrls.clientRequestUri());
+
         /*
          * Configure the default client ID. Seems like OAuthClient is keeping the state of clientID
          * For example: If some test case configure oauth.clientId("sample-public-client"), other tests
@@ -786,6 +790,72 @@ public void requestUriParamUnsigned() throws Exception {
         assertTrue(appPage.isCurrent());
     }
 
+    @Test
+    public void requestUriParamWithAllowedRequestUris() throws Exception {
+        oauth.stateParamHardcoded("mystate1");
+        String validRedirectUri = oauth.getRedirectUri();
+        TestOIDCEndpointsApplicationResource oidcClientEndpointsResource = testingClient.testApp().oidcClientEndpoints();
+        oidcClientEndpointsResource.setOIDCRequest("test", "test-app", validRedirectUri, "10", Algorithm.none.toString());
+        ClientManager.ClientManagerBuilder clientMgrBuilder = ClientManager.realm(adminClient.realm("test")).clientId("test-app");
+
+        oauth.requestUri(TestApplicationResourceUrls.clientRequestUri());
+
+        // Test with the relative allowed request_uri - should pass
+        String absoluteRequestUri = TestApplicationResourceUrls.clientRequestUri();
+        String requestUri = absoluteRequestUri.substring(UriUtils.getOrigin(absoluteRequestUri).length());
+        clientMgrBuilder.setRequestUris(requestUri);
+
+        oauth.openLoginForm();
+        Assert.assertFalse(errorPage.isCurrent());
+        loginPage.assertCurrent();
+
+        // Test with the relative and star at the end - should pass
+        requestUri = requestUri.replace("/get-oidc-request", "/*");
+        clientMgrBuilder.setRequestUris(requestUri);
+
+        oauth.openLoginForm();
+        Assert.assertFalse(errorPage.isCurrent());
+        loginPage.assertCurrent();
+
+        // Test absolute and wildcard at the end - should pass
+        requestUri = absoluteRequestUri.replace("/get-oidc-request", "/*");
+        clientMgrBuilder.setRequestUris(requestUri);
+
+        oauth.openLoginForm();
+        Assert.assertFalse(errorPage.isCurrent());
+        loginPage.assertCurrent();
+
+        // Test star only as wildcard - should pass
+        clientMgrBuilder.setRequestUris("*");
+
+        oauth.openLoginForm();
+        Assert.assertFalse(errorPage.isCurrent());
+        loginPage.assertCurrent();
+
+        // Test with multiple request_uris - should pass
+        clientMgrBuilder.setRequestUris("/foo", requestUri);
+
+        oauth.openLoginForm();
+        Assert.assertFalse(errorPage.isCurrent());
+        loginPage.assertCurrent();
+
+        // Test invalid request_uris - should fail
+        clientMgrBuilder.setRequestUris("/foo", requestUri.replace("/*", "/foo"));
+
+        oauth.openLoginForm();
+        errorPage.assertCurrent();
+
+        // Test with no request_uri set at all - should fail
+        clientMgrBuilder.setRequestUris();
+
+        oauth.openLoginForm();
+        errorPage.assertCurrent();
+
+        // Revert
+        clientMgrBuilder.setRequestUris(TestApplicationResourceUrls.clientRequestUri());
+
+    }
+
     @Test
     public void requestUriParamSigned() throws Exception {
         oauth.stateParamHardcoded("mystate3");

diff --git a/...llian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCWellKnownProviderTest.java b/...llian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCWellKnownProviderTest.java
@@ -152,6 +152,7 @@ public void testDiscovery() {
             // Request and Request_Uri
             Assert.assertTrue(oidcConfig.getRequestParameterSupported());
             Assert.assertTrue(oidcConfig.getRequestUriParameterSupported());
+            Assert.assertTrue(oidcConfig.getRequireRequestUriRegistration());
 
             // KEYCLOAK-7451 OAuth Authorization Server Metadata for Proof Key for Code Exchange
             // PKCE support

diff --git a/...ration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientManager.java b/...ration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientManager.java
@@ -2,11 +2,14 @@
 
 import org.keycloak.admin.client.resource.ClientResource;
 import org.keycloak.admin.client.resource.RealmResource;
+import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper;
+import org.keycloak.protocol.oidc.OIDCConfigAttributes;
 import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.ProtocolMapperRepresentation;
 import org.keycloak.representations.idm.RoleRepresentation;
 import org.keycloak.representations.idm.UserRepresentation;
 
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.LinkedList;
@@ -68,10 +71,11 @@ public void updateAttribute(String attribute, String value) {
             clientResource.update(app);
         }
 
-        public void directAccessGrant(Boolean enable) {
+        public ClientManagerBuilder directAccessGrant(Boolean enable) {
             ClientRepresentation app = clientResource.toRepresentation();
             app.setDirectAccessGrantsEnabled(enable);
             clientResource.update(app);
+            return this;
         }
 
         public ClientManagerBuilder standardFlow(Boolean enable) {
@@ -158,6 +162,13 @@ public void removeRedirectUris(String... redirectUris) {
             clientResource.update(app);
         }
 
+        // Set valid values of "request_uri" parameter
+        public void setRequestUris(String... requestUris) {
+            ClientRepresentation app = clientResource.toRepresentation();
+            OIDCAdvancedConfigWrapper.fromClientRepresentation(app).setRequestUris(Arrays.asList(requestUris));
+            clientResource.update(app);
+        }
+
         public UserRepresentation getServiceAccountUser() {
             return clientResource.getServiceAccountUser();
         }

diff --git a/themes/src/main/resources/theme/base/admin/messages/admin-messages_en.properties b/themes/src/main/resources/theme/base/admin/messages/admin-messages_en.properties
@@ -387,6 +387,8 @@ request-object-signature-alg=Request Object Signature Algorithm
 request-object-signature-alg.tooltip=JWA algorithm, which client needs to use when sending OIDC request object specified by 'request' or 'request_uri' parameters. If set to 'any', Request object can be signed by any algorithm (including 'none' ).
 request-object-required=Request Object Required
 request-object-required.tooltip=Specifies if the client needs to provide a request object with their authorization requests, and what method they can use for this. If set to "not required", providing a request object is optional. In all other cases, providing a request object is mandatory. If set to "request", the request object must be provided by value. If set to "request_uri", the request object must be provided by reference. If set to "request or request_uri", either method can be used.
+request-uris=Valid Request URIs
+request-uris.tooltip=List of valid URIs, which can be used as values of 'request_uri' parameter during OpenID Connect authentication request. There is support for the same capabilities like for Valid Redirect URIs. For example wildcards or relative paths.
 fine-saml-endpoint-conf=Fine Grain SAML Endpoint Configuration
 fine-saml-endpoint-conf.tooltip=Expand this section to configure exact URLs for Assertion Consumer and Single Logout Service.
 assertion-consumer-post-binding-url=Assertion Consumer Service POST Binding URL