@mposolda: Please, look at this :-)

If you need more info on this PR, please let me know

@ahus1: Are there any chances for this pr to be accepted?

Hi @evtr - thanks for reaching out. I did some labeling on this PR as it addresses SAML. I'm no expert in SAML, therefore I don't feel confident enough to review it. Looking at the code it technically looks ok. I also see that you opened an issue before creating the PR, this is great.

The commit message deviates a little bit from the contributing guidelines. This is no blocker, maybe you could still update it and then force-push it again so that it follows the format:

A brief descriptive summary

Optionally, more details around how it was implemented

Closes #1234

As this is a community pull request, you could reach out to SAML experts in the community to have an opinion on this PR. This could include confirming that it aligns with the specification, and if people think it might break existing implementations. Please use your usual Keycloak channels including the keycloak-dev mailing list to find those community members that are interested in SAML. I also see that two people in the issue reacted with a "thumbs up", maybe they would volunteer for a review.

Once you found those people, please keep the discussion in this GitHub issue, so that we have it "on record" for a final approval.

The final approval and decision to merge it will be with a maintainer (I'm not a maintainer).

Please let me know if you find this approach / comment helpful.

In response to the SAML related stuff. I would like to refer to http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf (spedifically the section 3.4.3 RelayState)

@mposolda: You seem to have a lot of SAML knowledge. Can you help?

@evtr @ahus1 Thanks for the PR and review.

But I am not a fan of the fix based on the static map clientIdDigests on the IdentityBrokerState class. This approach may not work for example in the cluster environment when methods decoded and encoded are called on different cluster nodes (which can happen due the fact that they are called in different HTTP requests).

It can work just when the cache of clientIdDigests is "preloaded" at startup, but this approach has other downsides (especially in environments with thousands of clients).

I can see that possible approach is to replace clientId (the stuff returned by clientModel.getClientId() with the id (the ID returned by clientModel.getId() ). In the current store, the lengths of id is 32 and hence it is too long. However maybe it will work in the new store?
@ahus1 Do you know what is the length of id (the one returned by clientModel.getId() ) in the new store?

For the time being, I suggest to workaround by using shorter clientId for your clients until this is addressed.

@evtr / @mposolda - with the new store, clientModel.getId() will be a String as well, and usually a UUID (as before). A store might decide to use a different String. With dashes, it might even be 36 characters long. A properly compressed UUID could be as small as approx. 22 characters (but only if it is a UUID, and there is no guarantee for that at the moment).

@ahus1 Ok, Thanks. In that case I suggest that:

• We use id instead of clientId as the source for the data
• In case that ID is UUID, we can encode it into this 22 characters long string you mentioned (actually it can be even 23 characters to pass the length limit of 80 characters :-)
• In case that ID is not UUID, we can just leave ID as is. So this is also not perfect solution, but maybe ok for now. Hopefully the combination of "client-id-not-uuid" AND "client-id-longer-than-22-characters" AND "strict SAML identity broker which accepts only relayStates of 80 characters" won't be very often.

@ahus1 @evtr Does that work for you guys?

@evtr / @mposolda - basically yes. The encoding of the UUID can be any encoding, please see a snippet to encode it via Base64 below that creates 22 bytes. Any encoding that is more efficient would create even shorter strings, but might clash on characters.

The alphabet used by Base64 is not URL safe, I don't know if that is an issue with SAML: ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/= - I've seen people replacing the last three characters with other characters when putting these into a URL.

Here a code snippet to encode/decode. The padding with "=" is removed on encoding; Base64 won't need it on decode.

    @Test
    public void test() {
        UUID uuid = UUID.randomUUID();
        String encoded = Base64.getEncoder().encodeToString(asBytes(uuid)).replace("=", "");
        System.out.println(encoded);
        Assert.assertEquals(22, encoded.length());
        Assert.assertEquals(uuid, asUuid(Base64.getDecoder().decode(encoded)));
    }

    public static byte[] asBytes(UUID uuid) {
        ByteBuffer bb = ByteBuffer.wrap(new byte[16]);
        bb.putLong(uuid.getMostSignificantBits());
        bb.putLong(uuid.getLeastSignificantBits());
        return bb.array();
    }

    public static UUID asUuid(byte[] bytes) {
        ByteBuffer bb = ByteBuffer.wrap(bytes);
        long firstLong = bb.getLong();
        long secondLong = bb.getLong();
        return new UUID(firstLong, secondLong);
    }

The alphabet used by Base64 is not URL safe, I don't know if that is an issue with SAML: ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/= - I've seen people replacing the last three characters with other characters when putting these into a URL.

Here a code snippet to encode/decode. The padding with "=" is removed on encoding; Base64 won't need it on decode.

We have Base64Url class inside the codebase, which effectively does this. So hopefully using that class should be ok

It seem to me that the solution with using something more likely to be a uuid as id in this context would be an ok solution.
In the case of connecting to a SAML IdP the "entityid" is used as the clientid, I think.

This particular IdP has other constraints on the entityid (it must be a URL matching some of the other metadata), so in this case i cant just make up a shorter one, unfortunately, but I suppose that is ok for it to actually validate the relaystate according to the SAML specs.

When I export my realm, I can se that the exported identityprovider has these properties:

"identityProviders": [
{
"alias": "myidp",
......
"internalId": "01852927-7950-4c1c-89ca-d010e1f77c24",
"providerId": "saml",
......
"config": {
"entityId": "https://something.actually.matching.something.else.and.therefore.too.long.com/auth/realms/testrealm",
......
}
}

I guess you are proposing using 01852927-7950-4c1c-89ca-d010e1f77c24 as input to the Base64.getEncoder().encodeToString(asBytes(uuid)).replace("=", "")

I think it sound like a good solution.

What to do next?

@evtr - here's my suggestion where to start: The interface of IdentityBrokerState.encoded() and IdentityBrokerState.decoded() would need to change.

For IdentityBrokerState.decoded(relayState, authSession.getClient().getClientId(), authSession.getTabId()); one would also pass authSession.getClient().getId() as this would be the UUID that would be used to shorten it.

For IdentityBrokerState.encoded(stateParam) you'd need to pass something to lookup the client by the UUID. You could pass down session and realm, and then it would be able to call session.clients().getClientById(realm, id).

Unfortunately this would make IdentityBrokerState more complex for unit testing. One could pass down a function that will allow it to map an ID to a clientId, but keep that in the "facade". So it could change to something like this:

    public static IdentityBrokerState encoded(String encodedState, KeycloakSession session, RealmModel realm) {
        return IdentityBrokerState.encoded(encodedState, id -> {
            ClientModel client = session.clients().getClientById(realm, id);
            if (client != null) {
                return client.getClientId();
            }
            return null;
        });
    }

    protected static IdentityBrokerState encoded(String encodedState, Function<String, String> idToClientId) {
       // put old code from public static IdentityBrokerState encoded(String encodedState) here that then calls idToClientId when needed
    }

Let me know what you think. Can it be simplified?

For IdentityBrokerState.encoded(stateParam) you'd need to pass something to lookup the client by the UUID. You could pass down session and realm, and then it would be able to call session.clients().getClientById(realm, id).

There is the issue that when you call IdentityBrokerState.encoded(stateParam), the client is not yet known. The actual ClientModel needs exactly to be looked-up from the stateParam . So the stateParam is the parameter, which needs to hold the actual reference to the client. Or did I miss something in your proposal?

@evtr yes, you can change the name of the parameter of decode method from clientId to something like clientDBId, which is returned by clientModel.getId() . And this can be converted to the 22 long string by encode to Base64. Please use org.keycloak.common.util.Base64Url to encode/decode to avoid reinventing the wheel for convert into URL friendly format.

One note is, that it is not 100% sure that clientModel.getId() is the UUID based on what @ahus1 mentioned above. It can happen in some rare cases when custom client storage is used, which is likely not the usual setup in the typical Keycloak deployment, but it can happen. So in case that it is not the UUID, I would just pass the string as is (the method encoded also needs to count with this).

In this case, it may be possible that relayState will be more than 80 characters long, but hopefully most of the deployments will be fine with this.

There is the issue that when you call IdentityBrokerState.encoded(stateParam), the client is not yet known. The actual ClientModel needs exactly to be looked-up from the stateParam . So the stateParam is the parameter, which needs to hold the actual reference to the client. Or did I miss something in your proposal?

Hi @mposolda - the code IdentityBrokerState.encoded(stateParam) was referring to the old code that needs to be replaced. The new code should then call the method listed below with the signature public static IdentityBrokerState encoded(String encodedState, KeycloakSession session, RealmModel realm).

I was thinking to include the "resolve-method" in the clientid by prefix it with a "1" or a "0":
Prefixing with 0 meaning that the clientid should be resolved in the classic manner - interpreted as client.clientId()
Prefixing with 1 meaning that the clientid should be resolved in the new manner - interpreted as client.Id()

My tests look like this:
@test
public void testDecodedUuidAsClientId() {

	// Given
	String state = "state";
	String clientDBId = "ed49448c-14cf-471e-a83a-063d0dc3bc8c";
	String tabId = "tabid";
	
	// When
	IdentityBrokerState ibs = IdentityBrokerState.decoded(state, clientDBId, tabId); // overloaded to calling decoded with fales in the parameter if the id should be interpreted as a db id
	
	// Then
	Assert.assertNotNull(ibs);
	Assert.assertEquals("state.tabid.07UlEjBTPRx6oOgY9DcO8jA", ibs.getEncoded());
}

@Test
public void testDecodedUuidAsClientDbId() {
	
	// Given
	String state = "state";
	String clientDBId = "ed49448c-14cf-471e-a83a-063d0dc3bc8c";
	String tabId = "tabid";
	
	// When
	IdentityBrokerState ibs = IdentityBrokerState.decoded(state, clientDBId, true, tabId);
	
	// Then
	Assert.assertNotNull(ibs);
	Assert.assertEquals("state.tabid.17UlEjBTPRx6oOgY9DcO8jA", ibs.getEncoded());
}

I thought that this could be a fairly easy change to do, but even simple changes make a kazillion tests fail:

[ERROR] UsernameTemplateMapperTest.usernameShouldBeDerivedFromAliasAndIdpSubClaim:80->AbstractBaseBrokerTest.logInAsUserInIDP:226->AbstractBaseBrokerTest.logInWithBroker:245->AbstractBaseBrokerTest.logInWithIdp:254 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutNestedBrokering:433->AbstractNestedBrokerTest.logInAsUserInNestedIDPForFirstTime:38 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutNestedBrokeringDoNotRevokeOfflineSessions:546->AbstractNestedBrokerTest.logInAsUserInNestedIDPForFirstTime:38 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutNestedBrokeringDownstreamLogoutOfSubConsumerFails:469->AbstractNestedBrokerTest.logInAsUserInNestedIDPForFirstTime:38 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutNestedBrokeringRevokeOfflineSessions:506->AbstractNestedBrokerTest.logInAsUserInNestedIDPForFirstTime:38 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutNestedBrokeringRevokeOfflineSessionsWithoutActiveUserSession:585->AbstractNestedBrokerTest.logInAsUserInNestedIDPForFirstTime:38 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutOnDisabledClientReturnsNotImplemented:407->AbstractBaseBrokerTest.logInAsUserInIDPForFirstTime:259->AbstractBaseBrokerTest.logInAsUserInIDP:226->AbstractBaseBrokerTest.logInWithBroker:245->AbstractBaseBrokerTest.logInWithIdp:254 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutWithSessionId:136->AbstractBaseBrokerTest.logInAsUserInIDPForFirstTime:259->AbstractBaseBrokerTest.logInAsUserInIDP:226->AbstractBaseBrokerTest.logInWithBroker:245->AbstractBaseBrokerTest.logInWithIdp:254 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutWithSessionIdMultipleOpenSession:244->AbstractBaseBrokerTest.logInAsUserInIDPForFirstTime:259->AbstractBaseBrokerTest.logInAsUserInIDP:226->AbstractBaseBrokerTest.logInWithBroker:245->AbstractBaseBrokerTest.logInWithIdp:254 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutWithSessionIdMultipleOpenSessionDifferentIdentityProvider:326->AbstractBaseBrokerTest.logInAsUserInIDPForFirstTime:259->AbstractBaseBrokerTest.logInAsUserInIDP:226->AbstractBaseBrokerTest.logInWithBroker:245->AbstractBaseBrokerTest.logInWithIdp:254 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutWithoutSessionId:162->AbstractBaseBrokerTest.logInAsUserInIDPForFirstTime:259->AbstractBaseBrokerTest.logInAsUserInIDP:226->AbstractBaseBrokerTest.logInWithBroker:245->AbstractBaseBrokerTest.logInWithIdp:254 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutWithoutSessionIdMultipleOpenSession:285->AbstractBaseBrokerTest.logInAsUserInIDPForFirstTime:259->AbstractBaseBrokerTest.logInAsUserInIDP:226->AbstractBaseBrokerTest.logInWithBroker:245->AbstractBaseBrokerTest.logInWithIdp:254 » NoSuchElement
[ERROR] BackchannelLogoutTest.postBackchannelLogoutWithoutSessionIdMultipleOpenSessionDifferentIdentityProvider:368->AbstractBaseBrokerTest.logInAsUserInIDPForFirstTime:259->AbstractBaseBrokerTest.logInAsUserInIDP:226->AbstractBaseBrokerTest.logInWithBroker:245->AbstractBaseBrokerTest.logInWithIdp:254 » NoSuchElement
[INFO]
[ERROR] Tests run: 4256, Failures: 125, Errors: 337, Skipped: 196

@evtr Not exactly sure about the cause of this. But I suspect it can be due the reason that "clientDbId" is not neccessarily the UUID. You can try to enable some logging to see what is going on. Also make sure to handle the situation when the UUID is not used. Something like this (only pseudo-code for the illustration):

try {
    UUID uuid = UUID.fromString(clientDbId);
    // encode to 22-length string as we have UUID
} catch (IllegalArgumentException iae) {
    // ID is not UUID. Just add the "clientDbId" as done in current Keycloak main (and count with the fact that it can be longer than 22 characters in this case)
}

This is just a suspicion and it is possible that test failures are caused by something different. Hopefully some debugging/logging can help you to figure it.

My problem is actually more in the class SessionCodeChecks

Here the clientid from the IdentityBrokerState is used to resolve back to the Client.

I thought that it would be impractical to assume that it is the databaseid that is used, so I did this:
if (clientId != null) {
if (useDbId) {
client = realm.getClientById(clientId);
} else {
client = realm.getClientByClientId(clientId);
}
}
if (client != null) {
session.getContext().setClient(client);
}

So I think that it is necessary to somehow check if the clientid is the database id or the client id. Handling the uuid contra non uuid is easy enough:

public static IdentityBrokerState decoded(String state, String clientId, boolean clientIdDb, String tabId) {
	
	String clientIdToUse = clientId;
	try {
		UUID clientDbUuid = UUID.fromString(clientId);
        ByteBuffer bb = ByteBuffer.wrap(new byte[16]);
        bb.putLong(clientDbUuid.getMostSignificantBits());
        bb.putLong(clientDbUuid.getLeastSignificantBits());
		byte[] clientUuidBytes = bb.array();
		clientIdToUse = Base64.getEncoder().encodeToString(clientUuidBytes).replace("=", "");
	} catch (IllegalArgumentException e) {
		// Ignore...the clientid in the database was not in UUID format. Just use as is.
	}
	clientIdToUse = (clientIdDb ? "1" : "0")+ clientIdToUse;
    String encodedState = state + "." + tabId + "." +clientIdToUse;

    return new IdentityBrokerState(state, clientIdDb, clientIdToUse, tabId, encodedState);
}


public static IdentityBrokerState encoded(String encodedState) {
    String[] decoded = DOT.split(encodedState, 4);

    String state =(decoded.length > 0) ? decoded[0] : null;
    String tabId = (decoded.length > 1) ? decoded[1] : null;
    String clientId = (decoded.length > 2) ? decoded[2] : null;
    boolean clientIdDb = clientId.startsWith("1");
    clientId = clientId.substring(1);
    
    try {
        byte[] decodedClientId = Base64.getDecoder().decode(clientId);
        ByteBuffer bb = ByteBuffer.wrap(decodedClientId);
        long first = bb.getLong();
        long second = bb.getLong();
        UUID clientDbUuid = new UUID(first, second);
        clientId = clientDbUuid.toString();
    } catch (IllegalArgumentException | BufferUnderflowException e) {
		// Ignore...the clientid was not in encoded uuid format. Just use as it is.
	}

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