Description
Before reporting an issue
- I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.
Area
storage
Describe the bug
we recently discovered a bug in Keycloak’s cache handling when caching users that are loaded from a user federation with a MAX_LIFESPAN setting. We've already briefly discussed this on the security mailing list and decided not to treat it as security related, and therefore we're opening it here.
We identified the problem as the following:
When a user is looked up in Keycloak via its username or email, Keycloak not only caches the user object, but also caches the ID of the user that matches the given username or email.
While the user object in the cache is correctly removed from the cache after the user federation’s MAX_LIFESPAN setting, the cached ID in the cached query is not evicted.
Version
26.2.4
Regression
- The issue is a regression
Expected behavior
- The user ‘john.doe’ logs in to any client, causing the user and its ID to be cached
- In an external system, the user’s username is changed to ‘jane.doe’
- After the MAX_LIFESPAN period has expired, the user logs in again to a client using their old email ‘john.doe’
- This login attempt should not succeed
Actual behavior
- The user ‘john.doe’ logs in to any client, causing the user and its ID to be cached
- In an external system, the user’s username is changed to ‘jane.doe’
- After the MAX_LIFESPAN period has expired, the user logs in again to a client using their old email ‘john.doe’
- This login attempt succeeds. Any tokens (or userinfo request) produced from that new login session will however contain the new username ‘jane.doe’, despite the user logging in successfully as ‘john.doe’
- Generally, the cached query will eventually disappear in a high load system by being displaced by other entries, however in a low load system, we have observed these cache entries lingering for hours or even days
- This behaviour is the same, regardless of the “email as username” or other login settings in the keycloak realm
How to Reproduce?
I've build a small reproducer for the problem which describes the bug in detail and allows easy reproduction
https://github.com/sonOfRa/keycloak-cache-invalidation
Anything else?
In our installation, we're currently running the following patch, which fixes the problem. However we're not sure if this is the desired solution for upstream, and would like to discuss possible alternatives.
diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java
index 86475fa81a..6b673a1c7f 100755
--- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java
+++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java
@@ -297,8 +297,14 @@ public class UserCacheSession implements UserCache, OnCreateComponent, OnUpdateC
return getDelegate().getUserByUsername(realm, username);
}
- logger.trace("return getUserById");
- return getUserById(realm, userId);
+
+ UserModel cachedUser = getUserById(realm, userId);
+ if (cachedUser.getUsername().equals(username)) {
+ logger.trace("return getUserById");
+ return cachedUser;
+ }
+ cache.invalidateObject(cacheKey);
+ return getDelegate().getUserByUsername(realm, username);
}
}
@@ -435,7 +441,12 @@ public class UserCacheSession implements UserCache, OnCreateComponent, OnUpdateC
return getDelegate().getUserByEmail(realm, email);
}
- return getUserById(realm, userId);
+ UserModel cachedUser = getUserById(realm, userId);
+ if (cachedUser.getEmail().equals(email)) {
+ return cachedUser;
+ }
+ cache.invalidateObject(cacheKey);
+ return getDelegate().getUserByEmail(realm, email);
}
}
What this patch does is simply check if the email or username for the cached ID still matches the user fetched from the federation, and if it does not, invalidate the cache entry and re-fetch it by username or email.
Note that we haven't tried this patch against 26.2.4, we're running it against 25.0.6 in our environment, so it might not apply cleanly on newer versions, but the concept could still be applied.