libre.sh/keycloak-scim
3
0
Fork 0
Code Issues 15 Pull requests Releases Packages Activity Actions

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

Browse source 
Closes https://github.com/keycloak/keycloak/issues/25001

Signed-off-by: rmartinc <rmartinc@redhat.com>
This commit is contained in:
rmartinc 2023-11-29 09:08:38 +01:00 committed by Pedro Igor
parent b6cdcb3c27
commit 3bc028fe2d
5 changed files with 59 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
docs/documentation/upgrading/topics/keycloak/changes-23_0_2.adoc Normal file
View file

@ -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.

4
docs/documentation/upgrading/topics/keycloak/changes.adoc
View file

@ -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]

25
services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java
View file

@ -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);

32
services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java
View file

@ -142,4 +142,36 @@ public class RedirectUtilsTest {
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));
}
}

11
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java
View file

@ -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