Skip to content

#10227: RelayState max length not respected (fix) #10231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
evtr wants to merge 1 commit into from
Closed

#10227: RelayState max length not respected (fix) #10231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions server-spi-private/src/main/java/org/keycloak/broker/provider/util/IdentityBrokerState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
package org.keycloak.broker.provider.util;

import java.util.regex.Pattern;
import java.util.HashMap;
import java.util.Map;

/**
* Encapsulates parsing logic related to state passed to identity provider in "state" (or RelayState) parameter
Expand All @@ -29,11 +31,20 @@ public class IdentityBrokerState {

private static final Pattern DOT = Pattern.compile("\\.");

private static final int RELAY_STATE_MAX_LENGTH = 80;
private static final Map<String, String> clientIdDigests = new HashMap<>();

public static IdentityBrokerState decoded(String state, String clientId, String tabId) {
String encodedState = state + "." + tabId + "." + clientId;
String clientIdDigest = clientId;
String encodedState = buildEncodedState(state, clientIdDigest, tabId);

return new IdentityBrokerState(state, clientId, tabId, encodedState);
if(encodedState.length() > RELAY_STATE_MAX_LENGTH) {
// Try to compress the clientId
clientIdDigest = getClientIdDigest(clientIdDigest);
encodedState = buildEncodedState(state, clientIdDigest, tabId);
}

return new IdentityBrokerState(state, clientIdDigest, tabId, encodedState);
}


Expand All @@ -42,12 +53,45 @@ public static IdentityBrokerState encoded(String encodedState) {

String state =(decoded.length > 0) ? decoded[0] : null;
String tabId = (decoded.length > 1) ? decoded[1] : null;
String clientId = (decoded.length > 2) ? decoded[2] : null;

// The clientId may have been digested previously. In that case, replace it with the original clientId.
String clientIdFromEncodedState = (decoded.length > 2) ? decoded[2] : null;
String clientId = tryLookupClientIdFromDigest(clientIdFromEncodedState);

return new IdentityBrokerState(state, clientId, tabId, encodedState);
}


private static String buildEncodedState(String state, String clientId, String tabId) {
return state + "." + tabId + "." + clientId;
}

private static String getClientIdDigest(String clientId) {
// Note: The map may be accessed concurrently, but a key always gets the same value (since String.hashCode() is deterministic),
// so we don't need to worry about race conditions.
clientIdDigests.put(clientId, Integer.toString(clientId.hashCode()));
return clientIdDigests.get(clientId);
}

private static String tryLookupClientIdFromDigest(String clientIdDigest) {
String foundClientId = null;

for(String clientId : clientIdDigests.keySet()) {
String digest = clientIdDigests.get(clientId);
if(digest == null) {
throw new IllegalStateException(String.format("Digest for clientId %s was unexpectedly null!", clientId));
}

if(digest.equals(clientIdDigest)) {
if(foundClientId != null) {
throw new IllegalStateException(String.format("The clientId digest %s has multiple client id's mapped to it, impossible to reverse!", clientIdDigest));
}
foundClientId = clientId;
}
}

return foundClientId != null ? foundClientId : clientIdDigest;
}

private final String decodedState;
private final String clientId;
Expand Down
74 changes: 74 additions & 0 deletions ...-spi-private/src/test/java/org/keycloak/broker/provider/util/IdentityBrokerStateTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package org.keycloak.broker.provider.util;

import org.junit.Test;
import org.keycloak.authorization.identity.Identity;

@ahus1 ahus1 Apr 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused import, please remove the unused import.


import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class IdentityBrokerStateTest {
private static final int RELAY_STATE_MAX_LENGTH = 80;

@Test
public void decoded_withinBounds_preservesClientId() {
// Arrange
// The length of the concatenated strings is below 80 characters, the RelayState limit as per the SAML specification.
String state = "gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk";
String tabId = "vpISZLVDAc0";
String clientId = "foo";

// Act
IdentityBrokerState ibs = IdentityBrokerState.decoded(state, clientId, tabId);

// Assert
assertEquals(clientId, ibs.getClientId());
assertTrue(ibs.getEncoded().length() < RELAY_STATE_MAX_LENGTH);
}

@Test
public void decoded_outOfBounds_compressesClientId() {
// Arrange
// The length of the concatenated strings is above 80 characters, the RelayState limit as per the SAML specification.
String state = "gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk";
String tabId = "vpISZLVDAc0";
String clientId = "https://login.blablabla.com/auth/realms/broker";

// Act
IdentityBrokerState ibs = IdentityBrokerState.decoded(state, clientId, tabId);

// Assert
assertTrue(ibs.getClientId().length() < clientId.length());
assertTrue(ibs.getEncoded().length() < RELAY_STATE_MAX_LENGTH);
}

@Test
public void encoded_compressedClientId_decompressesClientId() {
// Arrange
String state = "gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk";
String tabId = "vpISZLVDAc0";
String clientId = "https://login.blablabla.com/auth/realms/broker";
String encodedState = IdentityBrokerState.decoded(state, clientId, tabId).getEncoded();

@ahus1 ahus1 Apr 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the "arrange" part is calling IdentityBrokerState, thereby depending on the tested class. There is also no test to verify the output of encodedState.
With that observation, I'd rather have the encodedState as a hardcoded string in here, so that it is visible in the code what is tested as the input, and that input doesn't change unexpectedly wehn decoded() or getEncoded() change.

Suggested change
String encodedState = IdentityBrokerState.decoded(state, clientId, tabId).getEncoded();
String encodedState = IdentityBrokerState.decoded(state, clientId, tabId).getEncoded();
assertEquals("gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk.vpISZLVDAc0.1997353014", encodedState);


// Act
IdentityBrokerState ibs = IdentityBrokerState.encoded(encodedState);

// Assert
assertEquals(clientId, ibs.getClientId());
}

@Test
public void encoded_uncompressedClientId_preservesClientId() {
// Arrange
String state = "gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk";
String tabId = "vpISZLVDAc0";
String clientId = "foo";
String encodedState = IdentityBrokerState.decoded(state, clientId, tabId).getEncoded();

@ahus1 ahus1 Apr 12, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

Suggested change
String encodedState = IdentityBrokerState.decoded(state, clientId, tabId).getEncoded();
String encodedState = IdentityBrokerState.decoded(state, clientId, tabId).getEncoded();
assertEquals("gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk.vpISZLVDAc0.foo", encodedState);


// Act
IdentityBrokerState ibs = IdentityBrokerState.encoded(encodedState);

// Assert
assertEquals(clientId, ibs.getClientId());
}
}