From 3bc028fe2d4aa2477d227365dd270a999bd50c44 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Wed, 29 Nov 2023 09:08:38 +0100 Subject: [PATCH] Remove lowercase for the hostname as recommended/advised by OAuth spec Closes https://github.com/keycloak/keycloak/issues/25001 Signed-off-by: rmartinc --- .../topics/keycloak/changes-23_0_2.adoc | 7 ++++ .../upgrading/topics/keycloak/changes.adoc | 4 +++ .../protocol/oidc/utils/RedirectUtils.java | 25 ++++++--------- .../oidc/utils/RedirectUtilsTest.java | 32 +++++++++++++++++++ .../testsuite/oauth/OAuthRedirectUriTest.java | 11 ++++--- 5 files changed, 59 insertions(+), 20 deletions(-) create mode 100644 docs/documentation/upgrading/topics/keycloak/changes-23_0_2.adoc diff --git a/docs/documentation/upgrading/topics/keycloak/changes-23_0_2.adoc b/docs/documentation/upgrading/topics/keycloak/changes-23_0_2.adoc new file mode 100644 index 0000000000..e7b1e79891 --- /dev/null +++ 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. \ No newline at end of file diff --git a/docs/documentation/upgrading/topics/keycloak/changes.adoc b/docs/documentation/upgrading/topics/keycloak/changes.adoc index d32cf08a98..2ceb0b05c9 100644 --- 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 index b1bc4e3aa8..81669f5377 100644 --- 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 class RedirectUtils { 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 class RedirectUtils { 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 class RedirectUtils { 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 @@ public class RedirectUtils { String redirectUri = null; if (uri != null) { redirectUri = uri.normalize().toString(); - redirectUri = lowerCaseHostname(redirectUri); } return redirectUri; } @@ -204,9 +205,11 @@ public class RedirectUtils { 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 @@ public class RedirectUtils { return KeycloakUriBuilder.fromUri(decodedRedirectUri, false).preserveDefaultPort() .replaceQuery(origQuery) .fragment(origFragment) + .userInfo(origUserInfo) .buildAsString(); } else { // Next attempt @@ -230,15 +234,6 @@ public class RedirectUtils { 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 index 4a5cd883c6..54a4caa04e 100644 --- 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 class RedirectUtilsTest { Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/path 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 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/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java index 2a407bfc55..842b7b5f2f 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java @@ -382,11 +382,11 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { 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 class OAuthRedirectUriTest extends AbstractKeycloakTest { 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