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

Refresh token rotation with multiple tabs #14122

Closed
luisalves00 opened this issue Aug 31, 2022 · 38 comments · Fixed by #29966
Closed

Refresh token rotation with multiple tabs #14122

luisalves00 opened this issue Aug 31, 2022 · 38 comments · Fixed by #29966
Assignees
Labels
area/oidc Indicates an issue on OIDC area help wanted kind/bug Categorizes a PR related to a bug priority/important Must be worked on very soon release/25.0.0 status/bumped-by-bot team/core-clients

Comments

@luisalves00
Copy link

Describe the bug

Following this discussion, we are experience the problem.

Keycloak has single session and with for example 3 tabs opened, using sessionStorage as proposed by angular-oauth2-oidc, when we have token rotation enabled it doesn't work, because we can only use the lastest RT.

Version

18.0.2

Expected behavior

Keycloak should do a rotation per initial delivered token, so each tab can use their own RT (table instead of single record)

Actual behavior

Only one of the tabs work as expected (not sure if first opened or the last one)

How to Reproduce?

create spa that uses sessionStorage to keep the RT and open multiple tabs

Anything else?

No response

@luisalves00 luisalves00 added kind/bug Categorizes a PR related to a bug status/triage labels Aug 31, 2022
@stianst
Copy link
Contributor

stianst commented Sep 16, 2022

If you are using sessionStorage I would imagine you are actually sharing the initial refresh token across the tabs though?

@stianst stianst added the area/oidc Indicates an issue on OIDC area label Sep 16, 2022
@luisalves00
Copy link
Author

I'm not a front end engineer (will add some colleagues to the thread), but for what I understood it can't done (https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage).
The behavior I've seen is that first tab do login and create a session on Keycloak. Then it stores the refresh_token on the tab sessionStorage.
Now if you open a new tab, keycloak session already exist, so no login window is prompted. You get new refresh_token on the sessionStorage for that tab.
Now that we have rotation set to ZERO and they are maintained per keycloak session we have 1 tab that eventually logs out the user as if you reuse a refresh_token as a security measure the session is terminated (correct behavior).

I'm not sure if it's possible, but I believe you could maintain a table of current refresh_tokens per tab and then the problem might be mitigated.
When second tab gets a refresh_token it doesn't send a refresh_token, therefore I believe you can add this new generated refresh_token to a table that is linked to the session. Than when receiving the refresh_tokens you just have to look on table to check if exist and replace by the one you send to the client. Now each tab can own their refresh_token.

@marsbear
Copy link

marsbear commented Sep 16, 2022

Hey, I can fill in some information here.

If you are using sessionStorage I would imagine you are actually sharing the initial refresh token across the tabs though?

No, each tab gets its own refresh token. Thus the second tab invalidates the first tab's refresh token upon receiving his own. When the refresh cycle starts after a couple of minutes, the first tab starts, has an outdated token, redirects to login, gets a new one. Then the second tab starts to refresh with a now outdated refresh token and so on.

@stianst
Copy link
Contributor

stianst commented Sep 16, 2022

Of course, been a while since I've done any frontend stuff. Since it's per-tab storage basically the refresh tokens shouldn't be invalidated.

@stianst
Copy link
Contributor

stianst commented Sep 16, 2022

@mposolda any insight?

@mposolda
Copy link
Contributor

mposolda commented Sep 19, 2022

@stianst Looks like a valid bug.

With Revoke Refresh Tokens enabled, we track refresh tokens per client session. There is always single tracked refresh token per client session. Hence with multiple browser tabs for same client, the refresh doesn't work when each tab has it's own refresh token.

Possibilities to fix:

  1. Update clientSession to track refresh tokens from multiple tabs. Will require model change (for example methods on AuthenticatedClientSessionModel like get/setCurrentRefreshToken and get/setCurrentRefreshTokenUseCount will require update to contain also tabId as a parameter or something similar can be done)

  2. Make each tab to have it's own clientSession - will require much more changes and have impact on memory etc.

  3. Track used refresh tokens instead of tracking "current" refresh token. So used refresh token, which was used at least refresh token max reuse count times will be saved on server and won't be possible to use anymore for refresh. This has some more side-effects like:

  • Used refresh tokens need to be tracked for long period of time (problem especially for offline tokens)
  • In case that "Refresh Token Max Reuse" is bigger than 1, it can happen that attacker will reply request with some valid refresh token. For example assume refresh token rf1 and Refresh Token Max Reuse is 2.
    -- USer refreshed token rf1 and received new refresh token rf2
    -- Attacker replies request and refreshes also with rf1, which he managed to steal somehow, and receives refreshed token rf3. Now user can continue using refresh token rf2 and attacker can continue using rf3. User will never recognize that attacker steals his session and has his own refresh token -> KO
  1. Anything else?

In shortuct: I don't see any easy way to fix this. My favourite from those is option 1

@mposolda
Copy link
Contributor

@stianst And I would personally triage this with Help wanted as this is not a regression and I don't see much capacity within the team to fix this.

@luisalves00
Copy link
Author

@mposolda for option 1, the client lib like angular-oauth2-oidc will need to add logic to generate that tabId or it can be managed by Keycloak?

@marsbear
Copy link

I would assume that it should be possible to track refresh tokens by the token they were initially created for. So whenever a new token is created inside a session (without a refresh token being used), a new "branch" is opened within the session. Refresh token usage is then tracked per branch-id. That sounds pretty much like @mposolda 's tab-id, right? :)

@mposolda
Copy link
Contributor

@marsbear Pretty much yes. We can track refresh tokens by "tab ID" or "branch ID" or even by the ID of refresh token itself. Always when refresh token is issued due the "code to token" grant (or some other grant different than the "refresh token"), we know that this is kind of "new login" rather than "refresh of the existing login", so we can add new ID to track to the list of existing IDs. On the other hand, during refresh, we don't add new ID, but rather replace the existing one.

@luisalves00
Copy link
Author

If tabId is managed in server side I would say is a great solution. Regarding option 3, we plan to have Refresh Token Max Reuse = 0. In which scenario it makes sense to allow more than 1 use?

@mposolda
Copy link
Contributor

@luisalves00 I recall more than 1 use is helpful for the environments with unstable network (or other similar infrastructure issues).

For example if you trigger refresh with refresh token "rf1" and refresh is successful on the server, but for some reason, the application failed to consume the new refresh token "rf2". In other words, HTTP request is processed by Keycloak server and sent the HTTP response back, but the HTTP response never arrives back to the application. In this case, application can retry the refresh again with "rf1" . But in case when there is "max reuse = 0" , the refresh token rf1 is not valid anymore. Application is stuck at this point and will never be able to refresh anymore. So user will need to login again.

@luisalves00
Copy link
Author

@mposolda thanks for the explanation.

@wawyed
Copy link

wawyed commented Oct 31, 2022

@marsbear Pretty much yes. We can track refresh tokens by "tab ID" or "branch ID" or even by the ID of refresh token itself. Always when refresh token is issued due the "code to token" grant (or some other grant different than the "refresh token"), we know that this is kind of "new login" rather than "refresh of the existing login", so we can add new ID to track to the list of existing IDs. On the other hand, during refresh, we don't add new ID, but rather replace the existing one.

Do you have any idea when this will be implemented or looked at? I just came across the same issue trying to implement keycloak in my frontend

@wawyed
Copy link

wawyed commented Nov 18, 2022

@stianst @mposolda any updates please?

@mposolda
Copy link
Contributor

@wawyed I've triagwed this with "Help wanted". Keycloak team won't have capacity to look at this in the near future, so it would need to be community contribution. See comments above for approach how to address this. Just a note, that it may be non-trivial effort to properly implement this as it may need to be done for both old and new storage.

@wawyed
Copy link

wawyed commented Nov 21, 2022

@mposolda thanks for the update. I could have a look but it would be very useful if I could have some pointers on where to start. I'd go for option 1 and that seem the most reasonable one.

What did you mean the old and new storage?

@mposolda
Copy link
Contributor

@wawyed There are more implementations of the UserSessionProvider as we have old storage (based on infinispan in case of userSession, or based on DB for some other things like realms and users etc) and new storage (based on map providers). The new storage is going to support zero-downtime upgrade and AFAIK this particular change mentioned in this PR also would need to be done in new storage in a way to support zero-downtime upgrade.

I don't know exactly how to do it for the new store. It seems that more deep-dive into sources would be needed to implement this use-case.

@jbman
Copy link

jbman commented Nov 22, 2022

Like @marsbear and the orginal author of the issue, I think that a client should get multiple different refresh tokens, while each of the tokens can not be reused, but used only once:

Expected behavior
Keycloak should do a rotation per initial delivered token, so each tab can use their own RT (table instead of single record)

Compared to max reuse = 1 (or 2 or 3), dealing with network errors could better be handled by by an additional grace time in which the refresh token is allowed to be reused. This needs an expiry timestamp at the refresh token internally, which would be set as soon as the refresh token is used and a regular clean-up of these expired refresh tokens.
We may also want to have a limit for the refresh tokens per client (with default e.g. 10) so that a user could work in ten tabs without risking intentional or unintentional resource consumption by many SSO auth code grants.

@cm253
Copy link

cm253 commented Dec 13, 2022

Why not share one refresh and access token in multiple browser tabs on the client-side? If I would implement a desktop or mobile app using multiple processes/threads I wouldn't fetch multiple refresh and access tokens, but would share one refresh and access token and use it in all processes. As browser tabs are not much different (each browser tab has its own process with its own address space, but can share data for example the cookies and sessionStorage and also make IPC communication using the BroadcastChannel JS API) the tokens can be shared and used from multiple tabs.

Sounds easier to me than to have a refresh token per tab that must also be managed in the backend. If this is done in the Keycloak JavaScript adapter all web framework wrappers that use this adapter would have this multi-tab feature without changing a single line of code.

I don't know the internals of the Keycloak JavaScript adapter, but perhaps it makes sense to evaluate it at least.

@wawyed
Copy link

wawyed commented Dec 13, 2022

@cm253 what you are saying is not as easy as it sounds or reliable. Here's why:

  • You'd need to have a concept of main tab vs secondary tabs.
  • But what happens when the main tab is closed unexpectedly. For whatever reason the tab/window is forced closed and your app doesn't have enough time to assign a new tab, your app now is in an invalid state.
  • What happens when the main tab is minimised, depending on the OS the app could be sent to a sleep state where js code is not running in the background so even though the main tab is still there is not renewing tokens.

I do believe that 2 separate refresh tokens should be treated independently and not invalidate each other. Regardless if both refresh tokens came from the same keycloak session

@cm253
Copy link

cm253 commented Dec 13, 2022

Have you also evaluated if a service worker can act as a proxy that is responsible of refreshing/handling the tokens? With a service worker there would not be the concept of a main and secondary tab, but all tabs would use the same service worker that handles the refreshes. If one of the tabs tries to fetch an access token, and there is already a valid access token available in the service worker, this one would be returned to all tabs. If the access token gets invalid, the service worker would fetch a new access/refresh token and pass it to all tabs that requests a new one. Every browser context (tab) would use the same service worker (singleton), so sharing data is a no brainer.

@wawyed
Copy link

wawyed commented Dec 13, 2022

@cm253 I think you are asking for a lot of logic to be implement for every possible openid/oauth client out there to handle something that should be working correctly. I don't see how having two separate refresh tokens need to have connection to each other and invalidate each other when referesh token rotation is enabled. That seems like a bug to me.

@Badisi
Copy link

Badisi commented Dec 20, 2022

I had the exact same problem and opened another issue with a few more details here: #16081

I agree with @wawyed that this is clearly a bug and other AS are implementing it well already.

@cm253's solution is not viable as this issue is beyond a simple "multiple tabs" issue. It also exists between web browsers (ex: same app opened in Safari and Firefox) or even between devices (ex: same app opened in Safari mobile and Chrome desktop).

@keycloak-github-bot
Copy link

Due to the amount of issues reported by the community we are not able to prioritise resolving this issue at the moment.

If you are affected by this issue, upvote it by adding a 👍 to the description. We would also welcome a contribution to fix the issue.

@jonkoops
Copy link
Contributor

jonkoops commented Apr 3, 2024

To those upvoting the bot comment, that won't work, you'll have to upvote the PR description itself.

@opdt
Copy link
Contributor

opdt commented May 13, 2024

FYI: We implemented a fix for this inside our Cassandra-Datastore (since we cannot change the model interface, the implementation is not really straightforward though): https://github.com/opdt/keycloak-cassandra-extension/blob/main/core/src/main/java/de/arbeitsagentur/opdt/keycloak/cassandra/userSession/CassandraAuthenticatedClientSessionAdapter.java#L76

This uses one of the strategies discussed in this thread: Track Reuse per Token-ID and allow reuses during a configurable "grace period". The feature is only active, if "max token reuses" is equal to 0, since token-theft ("fork") would be otherwise undetectable using this strategy.

We would be glad to help implementing this in Keycloak itself since our solution is obviously rather hacky. We would just need a "go" from you, using one of the strategies discussed here.

@graziang graziang self-assigned this May 17, 2024
graziang added a commit to graziang/keycloak that referenced this issue Jun 6, 2024
Closes keycloak#14122

Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
graziang added a commit to graziang/keycloak that referenced this issue Jun 6, 2024
Closes keycloak#14122

Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
graziang added a commit to graziang/keycloak that referenced this issue Jun 7, 2024
Closes keycloak#14122

Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
graziang added a commit to graziang/keycloak that referenced this issue Jun 7, 2024
Closes keycloak#14122

Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
mposolda pushed a commit that referenced this issue Jun 7, 2024
Closes #14122

Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc Indicates an issue on OIDC area help wanted kind/bug Categorizes a PR related to a bug priority/important Must be worked on very soon release/25.0.0 status/bumped-by-bot team/core-clients
Projects
None yet
Development

Successfully merging a pull request may close this issue.