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

Add information about token expiry to events #28312

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahus1
Copy link
Contributor

@ahus1 ahus1 commented Mar 30, 2024

Closes #28311

What this PR adds:

  • Enrich the refresh event with additional information about the lifetime of the access token and when the refresh token was issued
  • Monitor the refresh event and log a message like the following if the age of the refresh token is smaller than 25% of the lifetime of the access token:

    In realm ... client .. requested a new access token when the refresh token was only ... seconds old which is ...% of the access token's lifetime. This indicates that that the previous access token was still valid for ... second. Change the client to ask for new access tokens only when they are about to expire to reduce the load on Keycloak. Warnings are suppressed for this client for ... seconds.

  • The warnings are logged only once per 60 seconds for a client to avoid logging too many messages

What a user can do:

  • Enable the event listener on selective realms and watch the event logs.
  • Write their own event listener to do different things with the information provided.
  • Enable recording the events to the database, and analyze the events afterwards.

Documentation and tests are is still missing.

Closes keycloak#28311

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1 ahus1 self-assigned this Mar 30, 2024
@ahus1
Copy link
Contributor Author

ahus1 commented Apr 2, 2024

@stianst - I remember I heard from first some time ago that client might refresh their access tokens too often, even if they are still valid for quite some time.

I want to add some additional information to the event to track this scenario, and also to help admins by deploying a ready-to-use event listener.

I'd be happy to hear your thoughts on this. Thanks!

cc: @mposolda @pedroigor @thomasdarimont

@thomasdarimont
Copy link
Contributor

thomasdarimont commented Apr 2, 2024

Thanks for the pointer! I am also familiar with this situation from many customer deployments.

I think these checks are very helpful in alerting teams to unfavorable client implementations at an early stage.

However, I wonder whether this should really be a configurable event listener or rather a built-in check during token refresh. A warning such as premature token renewal detected could actually always be helpful, right?

I only see a few situations where such warnings might be triggered despit being "legit":

  • Third-party products with inefficient OAuth implementations (refresh on every access...) where admins cannot change the software...
  • Clients with too eager token refresh strategies... (token must be valid for at least 2mins), e.g. because it is passed through multiple service calls.

So for some client's this might be acceptible behaviour for others it might not. Your implementation allows us to detect the general case, however can we also allow users to provide additional configuration to detect which client's refresh behaviour is legit and which one's is not?

@ahus1
Copy link
Contributor Author

ahus1 commented Apr 2, 2024

Thank you for the feedback. The event listener was the result of making it configurable via the UI (possibly per realm).

So while the data is collected all the time and you are free to analyze it as you need, the event listener is one initial way to use that data. Depending on feedback, it might be enabled by default.

A client that is too eager to refresh might benefit from a longer lifetime of the access tokens, and the warning will no longer be there.

Eventually we could have a client attribute which suppresses the message. Not sure about a good naming for that attribute here - I am also looking for a good name for the event listener. Ideas are welcome.

@ahus1
Copy link
Contributor Author

ahus1 commented Apr 2, 2024

Updated PR description to contain some documentation-in-a-nutshell.

@daviddelannoy
Copy link
Contributor

@ahus1 I have read your PR, for my use case for example, it will be useful to be able to enable a specific event listener by configuration on one realm only.
Enrich the REFRESH_TOKEN event like you did is enough for troubleshooting : I think that many users exploit keycloak events like we do in a log stack, so reporting on bad practise will be easy to do by using REFRESH_TOKEN events and producing new vizualizations and reports.

Like @thomasdarimont proposed, a client attribute (toggle) like Legit early access token renewal will permit to get rid of the warning if we already know that we can't change client calls from this specific client_id ...

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

Successfully merging this pull request may close these issues.

Detect clients which refresh their access tokens too early
3 participants