Please bear(er) with me. I definitely don't want Spring to contain vendor specific logic. I just want to: - inform the Spring team about the difficulties with this particular integration (maybe some things could be improved, either on Spring side or on Keycloak side) - find out if there's currently a better way to set this up

I have not made a self-contained reproducer as that would be quite a bit of work, but I can definitely make one if that's required.

Keycloak supports backchannel logout, although the feature is not well documented. It all boils down to the following: When Keycloak receives a user logout, it can propagate this logout to all its client applications. It does so by sending a POST request to the client's /k_logout endpoint. Among other things, the request body (org.keycloak.representations.adapters.action.LogoutAction as JWS) contains the sessionId(s) that the client should invalidate.

To accomplish this, Keycloak first needs to know our session ids. Here we have two options: 1. the session id can be included in the POST body when requesting an access token 2. the session id can be included in the POST body when requesting a refresh token

The first option was easy to implement (using a custom OAuth2AuthorizationCodeGrantRequestEntityConverter, as demonstrated here). There was however one big problem: if we want to protect against session fixation attacks, we must change the sessionId after successful authentication, thus rendering the previous sessionId (the one we sent to keycloak) useless.

As such, I implemented the second option. I registered a custom SavedRequestAwareAuthenticationSuccessHandler, which, onAuthenticationSuccess(), sends a POST request to the refresh token endpoint, including the newly generated sessionId (this time using a custom OAuth2RefreshTokenGrantRequestEntityConverter). I could, however, not use the existing OAuth2AuthorizedClientManager, because RefreshTokenOAuth2AuthorizedClientProvider won't reauthorize if the accessToken is not expired yet. Therefore, I had to either: - write my own OAuth2AuthorizedClientProvider, which is basically a copy of RefreshTokenOAuth2AuthorizedClientProvider, except that it will not check if the current access token is expired; or - bypass OAuth2AuthorizedClientManager entirely, and work directly with DefaultRefreshTokenTokenResponseClient

Both approached seem to work, but the latter seems like a bad idea. In bypassing OAuth2AuthorizedClientManager, the newly fetched token is not persisted. If Keycloak decides to send us a new refresh token and revoke the original one, our OAuth2AuthorizedClient's refreshToken would no longer work.

By now, Keycloak can send us the sessionId to invalidate (/k_logout endpoint), so the only thing left for us to do is implement session invalidation. I believe invalidating a session by id in a servlet application is not supported by the servlet spec. Instead, I expire the corresponding org.springframework.security.core.session.SessionInformation object. A custom javax.servlet.http.HttpFilter invalidates the corresponding HttpSession on the next incoming request for that session, and passes the request downstream. This way, invalidated sessions & non-existing/timed-out sessions are both handled by ExceptionTranslationFilter using authenticationEntryPoint (after an AccessDeniedException is thrown). I couldn't use org.springframework.security.web.session.ConcurrentSessionFilter, because SessionInformationExpiredStrategy provides no access to the filter chain.

TL;DR To securely (i.e. with session fixation protection) integrate with Keycloak's backchannel logout, I had trouble with the following: - RefreshTokenOAuth2AuthorizedClientProvider won't reauthorize if the accessToken is not expired yet - ConcurrentSessionFilter does not allow me to pass the request downstream, which in unfortunate since ExceptionTranslationFilter contains the desired error handling (authenticationEntryPoint)

Is this the correct approach, and if so, could the spring team work on improving this?

Comment From: jgrandja

Thank you for the details @jvanheesch. This seems like a duplicate of #7845. I would prefer to log the details there and close this issue.

Unfortunately, #7845 is not scheduled for 5.4. However, if you're interested in submitting a PR for this then maybe we can get it into 5.4.

Comment From: jvanheesch

It's definitely not a duplicate, but it is related. If Keycloak supported OIDC back-channel logout, this issue would be obsolete, but as you noted in another related issue, Keycloak's back-channel logout mechanism is proprietary and not spec-compliant. Keycloak does have plans to support OIDC back-channel logout, but sadly these things take lots of time.

I believe the Spring team should try to ease the migration from Spring Security OAuth to Spring Security wherever possible, which includes the common case of replacing a custom authorization server with Keycloak. As such, I explained in detail the difficulties I came across when setting up the integration with Keycloak's back-channel logout, so that the Spring team can consider whether or not they can simplify this migration. For example, in this particular case, the Spring team could reconsider: - whether or not it makes sense for RefreshTokenOAuth2AuthorizedClientProvider to refuse re-authorization if the current access token is not expired - whether or not SessionInformationExpiredStrategy should be given access to the filter chain, such that we can propagate the request downstream, which enables us to reuse ExceptionTranslationFilter's sendStartAuthentication().

I do have one more question: has the team considered creating a spring-boot-starter-keycloak jar? Such jar could contain vendor-specific logic and shield developers like me from the differences between proprietary vs spec-compliant implementation. Keycloak does provide a keycloak-spring-boot-starter jar, but it doesn't properly work out-of-the-box and the Spring Boot version is outdated.

Comment From: jgrandja

@jvanheesch I reviewed the OpenID Connect Back-Channel Logout spec once again and re-read your initial comment and I do see the challenges of propagating the sid claim to Keycloak. It does not work in a custom OAuth2AuthorizationCodeGrantRequestEntityConverter since the SessionId changes after the OIDC authentication flow completes, as you have indicated. And a custom AuthenticationSuccessHandler would not work either if you're using the standard RefreshTokenOAuth2AuthorizedClientProvider since the access token is not expired.

So the 2 main issues you have are:

  1. RefreshTokenOAuth2AuthorizedClientProvider won't reauthorize if the accessToken is not expired yet
  2. ConcurrentSessionFilter does not allow me to pass the request downstream, which in unfortunate since ExceptionTranslationFilter contains the desired error handling (authenticationEntryPoint)

The 2nd option should be logged as a separate ticket and we can address it there.

As for the 1st option, the current implementation makes sense. It does not make sense to refresh a token that has not expired so I don't see a change happening here. As well, creating a similar implementation of RefreshTokenOAuth2AuthorizedClientProvider that simply delegates to DefaultRefreshTokenTokenResponseClient without any checks would be straight forward and then you could plug it in via OAuth2AuthorizedClientProviderBuilder.builder().provider(customProvider) or directly set it in the manager via setter.

Looking at the sid claim:

sid OPTIONAL. Session ID - String identifier for a Session. This represents a Session of a User Agent or device for a logged-in End-User at an RP. Different sid values are used to identify distinct sessions at an OP. The sid value need only be unique in the context of a particular issuer. Its contents are opaque to the RP. Its syntax is the same as an OAuth 2.0 Client Identifier.

It is optional. Is Keycloak requiring it? If Keycloak allows it to be optional then the following logic would need to be applied on the client.

A Logout Token MUST contain either a sub or a sid Claim, and MAY contain both. If a sid Claim is not present, the intent is that all sessions at the RP for the End-User identified by the iss and sub Claims be logged out.

has the team considered creating a spring-boot-starter-keycloak jar?

No we haven't and it's not really anything we would house in Spring Security. You could check with the Spring Boot team but this make more sense in the existing keycloak-spring-boot-starter.

Comment From: jvanheesch

@jgrandja Thank you for your in-depth response!

The 2nd option should be logged as a separate ticket and we can address it there.

All right, I'll create a ticket for this one of the next days.

As for the 1st option, the current implementation makes sense. It does not make sense to refresh a token that has not expired so I don't see a change happening here.

I looked at the other OAuth2AuthorizedClientProvider implementations and I really disagree. There are a few code smells here: - RefreshTokenOAuth2AuthorizedClientProvider, PasswordOAuth2AuthorizedClientProvider and ClientCredentialsOAuth2AuthorizedClientProvider all have the same !hasTokenExpired(authorizedClient.getAccessToken()) logic (including clockSkew) - I need the core behavior of RefreshTokenOAuth2AuthorizedClientProvider (i.e.: get a new token), but I cannot reuse this class due to this !hasTokenExpired(authorizedClient.getAccessToken()) logic

In my opinion, this common !hasTokenExpired(authorizedClient.getAccessToken()) logic should be extracted from the core behavior (for all 3 OAuth2AuthorizedClientProvider implementations listed above). I don't know the code base all too well, so I cannot say for sure where the code should be, but an obvious "fix" would be: create a new OAuth2AuthorizedClientProvider implementation that wraps N OAuth2AuthorizedClientProviders and only delegates to them if the access token is not expired (otherwise it returns null).

As well, creating a similar implementation of RefreshTokenOAuth2AuthorizedClientProvider that simply delegates to DefaultRefreshTokenTokenResponseClient without any checks would be straight forward and then you could plug it in via OAuth2AuthorizedClientProviderBuilder.builder().provider(customProvider) or directly set it in the manager via setter.

It is indeed straightforward for those in the know, but it's another (albeit small) obstacle for people setting up this integration.

Looking at the sid claim:

sid OPTIONAL. Session ID - String identifier for a Session. This represents a Session of a User Agent or device for a logged-in End-User at an RP. Different sid values are used to identify distinct sessions at an OP. The sid value need only be unique in the context of a particular issuer. Its contents are opaque to the RP. Its syntax is the same as an OAuth 2.0 Client Identifier.

To reiterate: Keycloak's (current) back-channel logout mechanism is entirely proprietary and not at all spec-compliant - there's no notion of an OIDC logout token. The back-channel logout request is a POST request from Keycloak to the sso client, where the body is a jws containing an object of type org.keycloak.representations.adapters.action.LogoutAction. This request is NOT sent to clients for which no sessionId was received, and the request body does not contain a sub claim analogue (i.e. no user information). As such, we need to send the sessionId to Keycloak if we want to enable back-channel logout. It is, however, good to know that OIDC provides an alternative with the sub claim! I agree with you that in that case, the client could just log out all sessions for that user.

Comment From: jgrandja

@jvanheesch

I looked at the other OAuth2AuthorizedClientProvider implementations and I really disagree.

You are definitely entitled to your opinion. However, the way I see it, there is no need for RefreshTokenOAuth2AuthorizedClientProvider to refresh a non-expired access token. This simply does not make sense and would be reported as a bug if we were to apply this change.

In my opinion, this common !hasTokenExpired(authorizedClient.getAccessToken()) logic should be extracted from the core behavior....but an obvious "fix" would be: create a new OAuth2AuthorizedClientProvider implementation that wraps N OAuth2AuthorizedClientProviders and only delegates to them if the access token is not expired (otherwise it returns null).

Creating a delegate that simply checks for access token expiry and delegates if expired seems a bit too much. If a user wanted to configure the PasswordOAuth2AuthorizedClientProvider only, then they would also have to ensure they configured the delegate as well, which is an extra bit of configuration and not ideal. Furthermore, conditional checks that work together should belong together. Separating the token expiry conditional check into a delegate doesn't make sense IMO. It would make more sense to push that logic into a base class that the OAuth2AuthorizedClientProvider implementations would extend from.

I need the core behavior of RefreshTokenOAuth2AuthorizedClientProvider (i.e.: get a new token), but I cannot reuse this class due to this !hasTokenExpired(authorizedClient.getAccessToken()) logic

Here is a custom implementation you can use:

@Bean
OAuth2AuthorizedClientManager authorizedClientManager(ClientRegistrationRepository clientRegistrationRepository,
                                                        OAuth2AuthorizedClientRepository authorizedClientRepository) {
    OAuth2AuthorizedClientProvider authorizedClientProvider =
            OAuth2AuthorizedClientProviderBuilder.builder()
                    .authorizationCode()
                    .provider(refreshTokenAuthorizedClientProvider())
                    .build();
    DefaultOAuth2AuthorizedClientManager authorizedClientManager = new DefaultOAuth2AuthorizedClientManager(
            clientRegistrationRepository, authorizedClientRepository);
    authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);

    return authorizedClientManager;
}

private OAuth2AuthorizedClientProvider refreshTokenAuthorizedClientProvider() {
    final OAuth2AccessTokenResponseClient<OAuth2RefreshTokenGrantRequest> accessTokenResponseClient =
            new DefaultRefreshTokenTokenResponseClient();

    return context -> {
        OAuth2AuthorizedClient authorizedClient = context.getAuthorizedClient();
        if (authorizedClient == null || authorizedClient.getRefreshToken() == null) {
            return null;
        }
        OAuth2RefreshTokenGrantRequest refreshTokenGrantRequest = new OAuth2RefreshTokenGrantRequest(
                authorizedClient.getClientRegistration(), authorizedClient.getAccessToken(),
                authorizedClient.getRefreshToken());

        OAuth2AccessTokenResponse tokenResponse;
        try {
            tokenResponse = accessTokenResponseClient.getTokenResponse(refreshTokenGrantRequest);
        } catch (OAuth2AuthorizationException ex) {
            throw new ClientAuthorizationException(ex.getError(),
                    authorizedClient.getClientRegistration().getRegistrationId(), ex);
        }

        return new OAuth2AuthorizedClient(context.getAuthorizedClient().getClientRegistration(),
                context.getPrincipal().getName(), tokenResponse.getAccessToken(),
                tokenResponse.getRefreshToken());
    };
}

As mentioned, please log a new issue for ConcurrentSessionFilter and the solution above will resolve your 2nd issue for RefreshTokenOAuth2AuthorizedClientProvider.

I'm going to close this as a duplicate of #7845. I would recommend to keep track of #7845 so when the work starts for OIDC Back-Channel Logout, you can provide further input to ensure the implementation works with Keycloak.

Comment From: mcejp

For anyone stumbling onto this issue months (years) later: OpenID Connect Back-Channel Logout was shipped in Keycloak 12: - https://www.keycloak.org/2020/12/keycloak-1200-released.html - https://web.archive.org/web/20200922183709/https://issues.redhat.com/browse/KEYCLOAK-2940 - https://github.com/keycloak/keycloak/pull/7272