Support distributed apps with refresh tokens invalidated after first use

[pascalberger]

Which version of Duende.AccessTokenManagement are you using? 2.1.2

Which version of .NET are you using? 8

Describe the bug

In a distributed application outdated refresh token is used in UserAccessAccessTokenManagementService.RefreshUserAccessTokenAsync when refresh token has been updated, while waiting for the synchronization in UserAccessAccessTokenManagementService.GetAccessTokenAsync.

To Reproduce

• Use Duende.AccessTokenManagement against an OIDC IDP in a load balanced setup with at least 2 replicas and a distributed locking implementation for IUserTokenRequestSynchronization
• Wait until refresh token is expired
• Send two request at the same time to both replicas

First replica will succeed. The replica waiting for the synchronization will call IUserTokenEndpointService.RefreshAccessTokenAsync with the old refresh token (which was cached because of a previous call to GetTokenAsync to in UserAccessAccessTokenManagementService.RefreshUserAccessTokenAsync), instead of the refresh token which was issued by the other replica, which can result in the call failing if the used IDP invalidates refresh token after first use.

Expected behavior

Happy to hear any inputs what's the best way to handle this scenario.

I see an option by implementing my own UserAccessAccessTokenManagementService and AuthenticationSessionUserAccessTokenStore which takes care of invalidating the cache, but I would like to avoid if possible to duplicate the code.

Also happy to provide a PR if you would accept an enhancement to support this scenario.

Additional context

• An application with a BFF implementation which handles authentication against an OIDC IDP
• IDP invalidates refresh tokens after first use
• BFF runs with 2 replicas
• Custom IUserTokenRequestSynchronization implementation which implements a distributed lock

[josephdecock]

Instead of this IUserTokenRequestSynchronization customization, I would allow the BFF's refresh token to be reused. While rotation historically was recommended (and in the past was the IdentityServer default), it turns out that a persistent attacker won't be defeated by rotation, but rotation does cause reliability problems - both simple things like the token response getting dropped or more subtle things like this concurrency issue you've found. Given that, we decided to change the default of IdentityServer to allow refresh token reuse. We've blogged about this change and also have more documentation available. But again, it is safe to reuse your BFF's refresh tokens, because the token is bound to the BFF's client secret. An attacker can't use the refresh token without compromising the BFF to the point of being able to authenticate as the BFF client. Another good resource on this topic is OAuth 2.0 Security Best Current Practice, section 4.14.

The IUserTokenRequestSynchronization service is actually intended for synchronization within a particular host. It doesn't solve the concurrent update problem that you're dealing with, and we don't use it in a way that a distributed lock implementation would help you because by the time we're trying to synchronize, the user's session (and therefore the refresh token) has already been read.

So to sum up, my two recommendations are:

• Remove your custom IUserTokenRequestSynchronization implementation
• Allow the BFF to reuse refresh tokens

[josephdecock]

@AndersAbel - maybe we should improve the docs of the synchronization service to avoid confusion like this in future. Maybe just at the level of xmldoc and not in the documentation website, as this service is not really intended as an extensibility point.

[josephdecock]

Is anything further needed on this issue, or should we close it?

[pascalberger]

@josephdecock Thank you for your explanation 👍 Issue can be closed.

[AndersAbel]

@AndersAbel - maybe we should improve the docs of the synchronization service to avoid confusion like this in future. Maybe just at the level of xmldoc and not in the documentation website, as this service is not really intended as an extensibility point.

I think it would make sense to document both in XML doc and in the documentation site. We should be happy if people read one of them.