Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Token revocations: cannot revoke and out of memory exception #2226

Open
1 task done
dxnxk-0 opened this issue Nov 14, 2024 · 5 comments
Open
1 task done

Token revocations: cannot revoke and out of memory exception #2226

dxnxk-0 opened this issue Nov 14, 2024 · 5 comments

Comments

@dxnxk-0
Copy link

dxnxk-0 commented Nov 14, 2024

Confirm you've already contributed to this project or that you sponsor it

  • I confirm I'm a sponsor or a contributor

Version

5.8.0

Question

We have recently implemented some features regarding the logout, so that we revoke all tokens for a given subject. Since this release, we now notice we have thousands of revocation failures (and associated logs) due to TryRevokeAsync returning false. This is the code revoking the user tokens:

await foreach (var token in _openIddictTokenManager.FindBySubjectAsync(subjectId))
            if (!await _openIddictTokenManager.TryRevokeAsync(token))
                _logger.LogError("Failed to revoke token for subject {SubjectId} ({Token})", subjectId,
                    token.ToString());

The logout code seems to trigger some out of memory exceptions in some cases (not always):
Capture d’écran 2024-11-14 à 15 55 07

When this memory exception has occured, note that we had 52 successful token revocation and 58 revocation failures logs.

@kevinchalet
Copy link
Member

Thanks for the report, @dxnxk-0.

Does that help if you force the tokens to be buffered? (e.g by adding .ToListAsync() to _openIddictTokenManager.FindBySubjectAsync(subjectId))

This API isn't very efficient by design, but getting an OOM exception is a bit harsh. Is your environment highly RAM-constrained?

OpenIddict 6.0 will ship with much more efficient RevokeAsync() and RevokeBySubjectAsync() API: please give the latest preview a try when you have a moment to see if the same error exists with these APIs.

Cheers.

@dxnxk-0
Copy link
Author

dxnxk-0 commented Nov 14, 2024

We made some progress on the investigation. We have discovered we have a lot of tokens in our database which are not cleaned up by the Qwartz job. We have noticed the job does not delete any token older than 14 days, the default configuration. In our case, as we delete tokens one by one on logout, and that this configuration might accumulate a lot of tokens for a given subject, the logout becomes a costly process and visibly consume a lot of memory and cpu. This problem is connected to a bug in one of our client which was refreshing its tokens much to frequently, accumulating for a given user more than 200_000 tokens. This user, when logging out, has caused the whole server to use way too much memory and cpu during hours.

Measures we will take right now:

  • update the Qwartz option to prune tokens older than 10 minutes
  • using an Openiddict version supporting revoking subject tokens in bulk

@dxnxk-0
Copy link
Author

dxnxk-0 commented Nov 14, 2024

@kevinchalet thanks for your response. Do you see a problem if we set Qwart options like this in order to clean our tokens as we go:

options.UseQuartz()    
  .SetMinimumTokenLifespan(TimeSpan.FromHours(1))
  .SetMinimumAuthorizationLifespan(TimeSpan.FromHours(1));

@dxnxk-0
Copy link
Author

dxnxk-0 commented Nov 14, 2024

@kevinchalet we thought that until the V6 is out, instead of revoking the tokens one by one, we could retrieve all authorizations for a subject and then revoke the authorization. Don't you think this approach would be far more efficient ?

await foreach (var auth in _openIddictAuthorizationManager.FindBySubjectAsync(subjectId))    
  if (!await _openIddictAuthorizationManager.TryRevokeAsync(auth))        
    _logger.LogError("Failed to revoke authorization for subject {SubjectId} ({Authorization})", subjectId,  auth.ToString());

@kevinchalet
Copy link
Member

Do you see a problem if we set Qwart options like this in order to clean our tokens as we go:

In general, there are two main issues when opting for an ultra aggressive cleanup strategy:

  • Losing some history: it's harder to track all the tokens issued to a specific client/for a specific subject.
  • Identity tokens used as id_token_hints can no longer be validated since the token entry is missing. It's generally not a huge deal, but something to keep in mind if you use that (e.g to bypass sign-out screens when the client sends a valid id_token_hint).

Don't you think this approach would be far more efficient ?

Definitely. It's less granular, but in this case, it's not a problem, since you're revoking all the tokens associated to a user 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants