`InMemory[Reactive]OAuth2AuthorizedClientService` does not support changes to the `ClientRegistration` at runtime #15511

kzander91 · 2024-08-03T11:52:39Z

Describe the bug
We're calling an API that requires JWTs for authentication. The JWTs are obtained from an authorization server using Client Credentials Grant.
The authorization server requires us to periodically rotate the client secret. We store the secret in a separate configuration service (where we can update it out-of-band) and we have implemented a custom ReactiveClientRegistrationRepository that retrieves the secret from there.

This setup works fine until the secret is changed: We noticed that our app keeps using the previous secret, even though our ReactiveClientRegistrationRepository is returning the new secret.

We have tracked this down to InMemoryReactiveOAuth2AuthorizedClientService#loadAuthorizedClient which internally uses our repository implementation:

spring-security/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/InMemoryReactiveOAuth2AuthorizedClientService.java

Lines 60 to 67 in 6adc3b3

    
           public <T extends OAuth2AuthorizedClient> Mono<T> loadAuthorizedClient(String clientRegistrationId, 
        
           		String principalName) { 
        
           	Assert.hasText(clientRegistrationId, "clientRegistrationId cannot be empty"); 
        
           	Assert.hasText(principalName, "principalName cannot be empty"); 
        
           	return (Mono<T>) this.clientRegistrationRepository.findByRegistrationId(clientRegistrationId) 
        
           		.map((clientRegistration) -> new OAuth2AuthorizedClientId(clientRegistrationId, principalName)) 
        
           		.flatMap((identifier) -> Mono.justOrEmpty(this.authorizedClients.get(identifier))); 
        
           }

Note how the ClientRegistration retrieved from our clientRegistrationRepository (containing the current secret) is effectively ignored in favor of the ClientRegistration cached in the authorizedClients map (as a member of OAuth2AuthorizedClient). This means that the client secret that was current at the time of the first token request is used forever, even if the ClientRegistration changes afterwards.
After we rotated the secret, the authorization server rejects the previous one, resulting in all token requests to fail shortly after a secret rotation.

Expected behavior
The InMemoryReactiveOAuth2AuthorizedClientService should consider the fact that the ClientRegistration may change at runtime and thus not ignore it after fetching it from the registration repository.

Workaround

First quick workaround was to reboot our app after every secret rotation.
Next, we implemented our own ReactiveOAuth2AuthorizedClientService that is basically a copy of InMemoryReactiveOAuth2AuthorizedClientService, but with the following modification to loadAuthorizedClient:

return this.clientRegistrationRepository.findByRegistrationId(clientRegistrationId)
  .mapNotNull(clientRegistration -> {
    OAuth2AuthorizedClientId id = new OAuth2AuthorizedClientId(clientRegistrationId, principalName);
    OAuth2AuthorizedClient cachedAuthorizedClient = this.authorizedClients.get(id);
    if (cachedAuthorizedClient == null) {
      return null;
    }
    // Use current registration here  vvvvvvvvvvvvvvvvvv
    return new OAuth2AuthorizedClient(clientRegistration, cachedAuthorizedClient.getPrincipalName(), cachedAuthorizedClient.getAccessToken(), cachedAuthorizedClient.getRefreshToken());
  });

Note that the non-reactive version has the same behaviour, so this isn't specific to the reactive implementation.

Is there a specific reason for why the service behaves that way? What would be the recommended way to implement an OAuth2 client with secrets that can change at runtime?

The R2DBC implementation for example does behave as expected and always returns the ClientRegistration from the repository, see here:

spring-security/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/R2dbcReactiveOAuth2AuthorizedClientService.java

Lines 150 to 156 in 6adc3b3

    
           private Mono<OAuth2AuthorizedClient> getAuthorizedClient(OAuth2AuthorizedClientHolder authorizedClientHolder) { 
        
           	return this.clientRegistrationRepository.findByRegistrationId(authorizedClientHolder.getClientRegistrationId()) 
        
           		.switchIfEmpty(Mono.error(dataRetrievalFailureException(authorizedClientHolder.getClientRegistrationId()))) 
        
           		.map((clientRegistration) -> new OAuth2AuthorizedClient(clientRegistration, 
        
           				authorizedClientHolder.getPrincipalName(), authorizedClientHolder.getAccessToken(), 
        
           				authorizedClientHolder.getRefreshToken())); 
        
           }

The text was updated successfully, but these errors were encountered:

sjohnr · 2024-11-13T23:03:26Z

Hi @kzander91, thanks for the report. Apologies on the delay, I'm just going through some issues that we haven't been able to get to until now.

We have tracked this down to InMemoryReactiveOAuth2AuthorizedClientService#loadAuthorizedClient which internally uses our repository implementation:

The InMemoryReactiveOAuth2AuthorizedClientService implementation (and in-memory implementations in general) is not recommended for production use. This is a general guideline, but I don't see it specifically stated anywhere in our documentation so it's understandable that you might not be aware.

Typically, you would want to use R2dbcReactiveOAuth2AuthorizedClientService (which doesn't have this issue) or a custom implementation anyway. Before going any further, I want to make sure there's not a specific reason you chose to keep using the in-memory one?

kzander91 · 2024-11-14T08:41:32Z

Hi @sjohnr, no worries!

I am aware that the in-memory implementations of various beans aren't recommended for production use, but my interpretation of the reason behind that was always that there's no mechanism to limit the size of the map, resulting in potential OOMEs. Are there other reasons for it not being recommended for production?

In my use case, the map will only ever contain a single mapping, because the user in the SecurityContext will always be the same, single user, and API calls will only ever be made to the same endpoint with bearer tokens obtained from the same IDP. Thus, the complexity of setting up a database for that (plus the additional latency for reading from/writing to the DB) just isn't worth it and the in-memory variant should be perfectly suitable for that use case.

sjohnr · 2024-11-14T18:30:44Z

Are there other reasons for it not being recommended for production?

The in-memory implementations are just not designed for production use. Any issues they have would be specific to their implementation.

Having said that, we still want to fix bugs that are obviously incorrect (and not directly related to being in memory), so I think the bug you mention here could be fixed since the r2dbc version doesn't have the same issue. Would you be interested in sending a PR?

kzander91 · 2024-11-16T09:14:52Z

@sjohnr sure I'll give it a shot! Would you say that the workaround I showed above was a suitable fix? Then I'll try submitting that as a PR.

This changes `InMemoryOAuth2AuthorizedClientService.loadAuthorizedClient` (and its reactive counterpart) to always return `OAuth2AuthorizedClient` instances containing the current `ClientRegistration` as obtained from the `ClientRegistrationRepository`. Before this change, the first `ClientRegistration` instance was cached, with the effect that any changes made in the `ClientRegistrationRepository` (such as a new client secret) would not have taken effect. Closes spring-projectsgh-15511

sjohnr · 2024-11-19T00:18:30Z

Would you say that the workaround I showed above was a suitable fix?

Yes, I think so! Though I would find it preferable to use a fluent return value by nesting flatMap:

// @formatter:off
return (Mono<T>) this.clientRegistrationRepository.findByRegistrationId(clientRegistrationId)
	.flatMap((clientRegistration) -> Mono.just(new OAuth2AuthorizedClientId(clientRegistrationId, principalName))
		.mapNotNull(this.authorizedClients::get)
		.map((authorizedClient) -> new OAuth2AuthorizedClient(clientRegistration,
			authorizedClient.getPrincipalName(),
			authorizedClient.getAccessToken(),
			authorizedClient.getRefreshToken()))
	);
// @formatter:on

This changes `InMemoryOAuth2AuthorizedClientService.loadAuthorizedClient` (and its reactive counterpart) to always return `OAuth2AuthorizedClient` instances containing the current `ClientRegistration` as obtained from the `ClientRegistrationRepository`. Before this change, the first `ClientRegistration` instance was cached, with the effect that any changes made in the `ClientRegistrationRepository` (such as a new client secret) would not have taken effect. Closes spring-projectsgh-15511

kzander91 · 2024-11-19T08:14:42Z

@sjohnr #16133 is ready for your consideration.

kzander91 added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Aug 3, 2024

sjohnr self-assigned this Nov 13, 2024

sjohnr added status: waiting-for-feedback We need additional information before we can continue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 13, 2024

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 14, 2024

kzander91 linked a pull request Nov 19, 2024 that will close this issue

Always return current ClientRegistration in loadAuthorizedClient #16133

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`InMemory[Reactive]OAuth2AuthorizedClientService` does not support changes to the `ClientRegistration` at runtime #15511

`InMemory[Reactive]OAuth2AuthorizedClientService` does not support changes to the `ClientRegistration` at runtime #15511

kzander91 commented Aug 3, 2024 •

edited

Loading

sjohnr commented Nov 13, 2024

kzander91 commented Nov 14, 2024 •

edited

Loading

sjohnr commented Nov 14, 2024

kzander91 commented Nov 16, 2024 •

edited

Loading

sjohnr commented Nov 19, 2024

kzander91 commented Nov 19, 2024

InMemory[Reactive]OAuth2AuthorizedClientService does not support changes to the ClientRegistration at runtime #15511

InMemory[Reactive]OAuth2AuthorizedClientService does not support changes to the ClientRegistration at runtime #15511

Comments

kzander91 commented Aug 3, 2024 • edited Loading

sjohnr commented Nov 13, 2024

kzander91 commented Nov 14, 2024 • edited Loading

sjohnr commented Nov 14, 2024

kzander91 commented Nov 16, 2024 • edited Loading

sjohnr commented Nov 19, 2024

kzander91 commented Nov 19, 2024

`InMemory[Reactive]OAuth2AuthorizedClientService` does not support changes to the `ClientRegistration` at runtime #15511

`InMemory[Reactive]OAuth2AuthorizedClientService` does not support changes to the `ClientRegistration` at runtime #15511

kzander91 commented Aug 3, 2024 •

edited

Loading

kzander91 commented Nov 14, 2024 •

edited

Loading

kzander91 commented Nov 16, 2024 •

edited

Loading