Remove lowercase for the hostname as recommended/advised by OAuth spec

Closes #25001 Signed-off-by: rmartinc <[email protected]>
keycloak · Dec 5, 2023 · 942aa38 · 942aa38
1 parent 3b2b6e4
commit 942aa38
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 20 deletions.
diff --git a/docs/documentation/upgrading/topics/keycloak/changes-23_0_2.adoc b/docs/documentation/upgrading/topics/keycloak/changes-23_0_2.adoc
@@ -0,0 +1,7 @@
+= Valid redirect URIs for clients are always compared with exact string matching
+
+Version 1.8.0 introduced a lower-case for the hostname and scheme when comparing a redirect URI with the specified valid redirects for a client. Unfortunately it did not fully work in all the protocols, and, for example, the host was lower-cased for `http` but not for `https`. As https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#name-protecting-redirect-based-f[OAuth 2.0 Security Best Current Practice] advises to compare URIs using exact string matching, {project_name} will follow the recommendation and for now on valid redirects are compared with exact case even for the hostname and scheme.
+
+For realms relying on the old behavior, the valid redirect URIs for their clients should now hold separate entries for each URI that should be recognized by the server.
+
+Although it introduces more steps and verbosity when configuring clients, the new behavior enables more secure deployments as pattern-based checks are frequently the cause of security issues. Not only due to how they are implemented but also how they are configured.
diff --git a/docs/documentation/upgrading/topics/keycloak/changes.adoc b/docs/documentation/upgrading/topics/keycloak/changes.adoc
@@ -1,5 +1,9 @@
 == Migration Changes
 
+=== Migrating to 23.0.2
+
+include::changes-23_0_2.adoc[leveloffset=3]
+
 === Migrating to 23.0.0
 
 include::changes-23_0_0.adoc[leveloffset=3]

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
@@ -72,10 +72,8 @@ public static Set<String> resolveValidRedirects(KeycloakSession session, String
             if (validRedirect.startsWith("/")) {
                 validRedirect = relativeToAbsoluteURI(session, rootUrl, validRedirect);
                 logger.debugv("replacing relative valid redirect with: {0}", validRedirect);
-                resolveValidRedirects.add(validRedirect);
-            } else {
-                resolveValidRedirects.add(validRedirect);
             }
+            resolveValidRedirects.add(validRedirect);
         }
         return resolveValidRedirects;
     }
@@ -148,7 +146,11 @@ public static String verifyRedirectUri(KeycloakSession session, String rootUrl,
                 valid = matchesRedirects(resolveValidRedirects, redirectUri, false);
             }
 
-            if (valid != null && redirectUri.startsWith("/")) {
+            if (valid != null && !originalRedirect.isAbsolute()) {
+                // return absolute if the original URI is relative
+                if (!redirectUri.startsWith("/")) {
+                    redirectUri = "/" + redirectUri;
+                }
                 redirectUri = relativeToAbsoluteURI(session, rootUrl, redirectUri);
             }
 
@@ -174,7 +176,7 @@ public static String verifyRedirectUri(KeycloakSession session, String rootUrl,
     private static URI toUri(String redirectUri) {
         URI uri = null;
         if (redirectUri != null) {
-        try {
+            try {
                 uri = URI.create(redirectUri);
             } catch (IllegalArgumentException cause) {
                 logger.debug("Invalid redirect uri", cause);
@@ -189,7 +191,6 @@ private static String getNormalizedRedirectUri(URI uri) {
         String redirectUri = null;
         if (uri != null) {
             redirectUri = uri.normalize().toString();
-            redirectUri = lowerCaseHostname(redirectUri);
         }
         return redirectUri;
     }
@@ -204,9 +205,11 @@ private static String decodeRedirectUri(String redirectUri) {
             KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(redirectUri, false).preserveDefaultPort();
             String origQuery = uriBuilder.getQuery();
             String origFragment = uriBuilder.getFragment();
+            String origUserInfo = uriBuilder.getUserInfo();
             String encodedRedirectUri = uriBuilder
                     .replaceQuery(null)
                     .fragment(null)
+                    .userInfo(null)
                     .buildAsString();
             String decodedRedirectUri = null;
 
@@ -217,6 +220,7 @@ private static String decodeRedirectUri(String redirectUri) {
                     return KeycloakUriBuilder.fromUri(decodedRedirectUri, false).preserveDefaultPort()
                             .replaceQuery(origQuery)
                             .fragment(origFragment)
+                            .userInfo(origUserInfo)
                             .buildAsString();
                 } else {
                     // Next attempt
@@ -230,15 +234,6 @@ private static String decodeRedirectUri(String redirectUri) {
         return null;
     }
 
-    private static String lowerCaseHostname(String redirectUri) {
-        int n = redirectUri.indexOf('/', 7);
-        if (n == -1) {
-            return redirectUri.toLowerCase();
-        } else {
-            return redirectUri.substring(0, n).toLowerCase() + redirectUri.substring(n);
-        }
-    }
-
     private static String relativeToAbsoluteURI(KeycloakSession session, String rootUrl, String relative) {
         if (rootUrl != null) {
             rootUrl = ResolveRelative.resolveRootUrl(session, rootUrl);

diff --git a/services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java b/services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java
@@ -142,4 +142,36 @@ public void testverifyInvalidRedirectUri() {
         Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/path<less/", set, false));
         Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/path/index.jsp?param=v1 v2", set, false));
     }
+
+    @Test
+    // https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#name-protecting-redirect-based-f
+    // OAuth recommends/advises exact matching string comparison for URIs
+    public void testverifyCaseIsSensitive() {
+        Set<String> set = Stream.of(
+                "https://keycloak.org/*",
+                "http://KeyCloak.org/*",
+                "no.host.Name.App:/Test"
+        ).collect(Collectors.toSet());
+
+        Assert.assertEquals("https://keycloak.org/index.html", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/index.html", set, false));
+        Assert.assertEquals("http://KeyCloak.org/index.html", RedirectUtils.verifyRedirectUri(session, null, "http://KeyCloak.org/index.html", set, false));
+        Assert.assertEquals("no.host.Name.App:/Test", RedirectUtils.verifyRedirectUri(session, null, "no.host.Name.App:/Test", set, false));
+
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://KeyCloak.org/index.html", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://keycloak.org/index.html", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "HTTPS://keycloak.org/index.html", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "no.host.Name.app:/Test", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "no.host.Name.App:/test", set, false));
+    }
+
+    @Test
+    public void testRelativeRedirectUri() {
+        Set<String> set = Stream.of(
+                "*"
+        ).collect(Collectors.toSet());
+
+        Assert.assertEquals("https://keycloak.org/path", RedirectUtils.verifyRedirectUri(session, "https://keycloak.org", "/path", set, false));
+        Assert.assertEquals("https://keycloak.org/path", RedirectUtils.verifyRedirectUri(session, "https://keycloak.org", "path", set, false));
+    }
+
 }
diff --git a/...rquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java b/...rquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java
@@ -382,11 +382,11 @@ public void testDifferentCaseInHostname() throws IOException {
         oauth.clientId("test-dash");
 
         checkRedirectUri("http://with-dash.example.local", true);
-        checkRedirectUri("http://wiTh-dAsh.example.local", true);
+        checkRedirectUri("http://wiTh-dAsh.example.local", false);
         checkRedirectUri("http://with-dash.example.local/foo", true);
-        checkRedirectUri("http://wiTh-dAsh.example.local/foo", true);
+        checkRedirectUri("http://wiTh-dAsh.example.local/foo", false);
         checkRedirectUri("http://with-dash.example.local/foo", true);
-        checkRedirectUri("http://wiTh-dAsh.example.local/foo", true);
+        checkRedirectUri("http://wiTh-dAsh.example.local/foo", false);
         checkRedirectUri("http://wiTh-dAsh.example.local/Foo", false);
         checkRedirectUri("http://wiTh-dAsh.example.local/foO", false);
     }
@@ -395,8 +395,9 @@ public void testDifferentCaseInHostname() throws IOException {
     public void testDifferentCaseInScheme() throws IOException {
         oauth.clientId("test-dash");
 
-        checkRedirectUri("HTTP://with-dash.example.local", true);
-        checkRedirectUri("Http://wiTh-dAsh.example.local", true);
+        checkRedirectUri("http://with-dash.example.local", true);
+        checkRedirectUri("HTTP://with-dash.example.local", false);
+        checkRedirectUri("Http://wiTh-dAsh.example.local", false);
     }
 
     @Test