search

DuendeSoftware / IdentityServer

The most flexible and standards-compliant OpenID Connect and OAuth 2.x framework for ASP.NET Core
https://duendesoftware.com/products/identityserver
Other
1.4k stars 317 forks source link

InMemoryPersistentGrantStore not cleaning up expired tokens #649

Closed Tornhoof closed 2 years ago

Tornhoof commented 2 years ago

Which version of Duende IdentityServer are you using? 6.0 (this also applies to previous versions of IdentityServer) Which version of .NET are you using? 6.0 Describe the bug A customer reports a system which gets gradually slower over a few days, after a restart it is fast again. A memory dump shows a rather large InMemoryPersistentGrantStore (yes this is inmemory, we don't really care if the system is restarted and the grants are lost), roughly 250k items (after a few days) The InMemoryPersistentGrantStore is a ConcurrentDictionary which stores the grant data in memory and the values is primarily removed if the token is checked as expired.

        if (token.CreationTime.HasExceeded(token.Lifetime, _clock.UtcNow.UtcDateTime))
        {
            LogError("Token expired.");

            await _referenceTokenStore.RemoveReferenceTokenAsync(tokenHandle);
            return Invalid(OidcConstants.ProtectedResourceErrors.ExpiredToken);

This (as far as I understand) means, that the ref tokens are only removed if they are used after they are expired. There is no timer which checks for expired values.

To Reproduce

Use InMemoryPersistentGrantStore and create a lot of tokens (in this case ref tokens).

Expected behavior

InMemoryPersistentGrantStore gets cleaned up, i.e. by using a memory cache with timeouts or similar mechanism.

If the InMemoryPersistentGrantStore is only designed for testing purposes, then it should be more clear (there was a question in the ID4 repo about that, without any answer).

leastprivilege commented 2 years ago

Well - the InMemoryPersistentGrantStore is only designed for testing purposes. We even emit a warning at startup saying that...

Tornhoof commented 2 years ago

Yes, your info log level is: https://github.com/DuendeSoftware/IdentityServer/blob/ffa4da997b44dcb32103bb26a919fb14cd1cf0f5/src/IdentityServer/Configuration/IdentityServerApplicationBuilderExtensions.cs#L81

https://github.com/DuendeSoftware/IdentityServer/blob/ffa4da997b44dcb32103bb26a919fb14cd1cf0f5/src/IdentityServer/Configuration/DependencyInjection/IdentityServerServiceCollectionExtensions.cs#L51-L52

I was not aware that this means that it is a no-go on production (because it worked fine, if you ignore the leaking).

Since you clarified it, i'll close this.