fix deadlock in jwt realm

jakelandis · Sep 7, 2023 · 43a1a94 · 43a1a94
1 parent 2461cb1
commit 43a1a94
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 15 deletions.
diff --git a/...core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java b/...core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java
@@ -23,31 +23,33 @@
  */
 public class CacheIteratorHelper<K, V> {
     private final Cache<K, V> cache;
-    private final ReleasableLock updateLock;
-    private final ReleasableLock iteratorLock;
+    private final ReleasableLock readLock;
+    private final ReleasableLock writeLock;
 
     public CacheIteratorHelper(Cache<K, V> cache) {
         this.cache = cache;
         final ReadWriteLock lock = new ReentrantReadWriteLock();
         // the lock is used in an odd manner; when iterating over the cache we cannot have modifiers other than deletes using the
         // iterator but when not iterating we can modify the cache without external locking. When making normal modifications to the cache
-        // the read lock is obtained so that we can allow concurrent modifications; however when we need to iterate over the keys or values
+        // the read lock can obtained so that we can allow concurrent modifications; however when we need to iterate over the keys or values
         // of the cache the write lock must obtained to prevent any modifications.
-        updateLock = new ReleasableLock(lock.readLock());
-        iteratorLock = new ReleasableLock(lock.writeLock());
+        // Note - the write lock is needed for concurrent modifications across Cache#put and Cache#invalidateAll
+        // see https://github.com/elastic/elasticsearch/issues/99326 for additional information
+        readLock = new ReleasableLock(lock.readLock());
+        writeLock = new ReleasableLock(lock.writeLock());
     }
 
-    public ReleasableLock acquireUpdateLock() {
-        return updateLock.acquire();
+    public ReleasableLock acquireReadLock() {
+        return readLock.acquire();
     }
 
-    private ReleasableLock acquireForIterator() {
-        return iteratorLock.acquire();
+    public ReleasableLock acquireWriteLock() {
+        return writeLock.acquire();
     }
 
     public void removeKeysIf(Predicate<K> removeIf) {
         // the cache cannot be modified while doing this operation per the terms of the cache iterator
-        try (ReleasableLock ignored = this.acquireForIterator()) {
+        try (ReleasableLock ignored = this.acquireWriteLock()) {
             Iterator<K> iterator = cache.keys().iterator();
             while (iterator.hasNext()) {
                 K key = iterator.next();
@@ -60,7 +62,7 @@ public void removeKeysIf(Predicate<K> removeIf) {
 
     public void removeValuesIf(Predicate<V> removeIf) {
         // the cache cannot be modified while doing this operation per the terms of the cache iterator
-        try (ReleasableLock ignored = this.acquireForIterator()) {
+        try (ReleasableLock ignored = this.acquireWriteLock()) {
             Iterator<V> iterator = cache.values().iterator();
             while (iterator.hasNext()) {
                 V value = iterator.next();

diff --git a/...ck/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtRealm.java b/...ck/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtRealm.java
@@ -403,7 +403,7 @@ private void processValidatedJwt(
                     () -> format("Realm [%s] roles resolved [%s] for principal=[%s].", name(), join(",", user.roles()), principal)
                 );
                 if (isCacheEnabled()) {
-                    try (ReleasableLock ignored = jwtCacheHelper.acquireUpdateLock()) {
+                    try (ReleasableLock ignored = jwtCacheHelper.acquireWriteLock()) {
                         final long expWallClockMillis = claimsSet.getExpirationTime().getTime() + allowedClockSkew.getMillis();
                         jwtCache.put(jwtCacheKey, new ExpiringUser(result.getValue(), new Date(expWallClockMillis)));
                     }
@@ -449,7 +449,7 @@ private void invalidateJwtCache() {
         if (isCacheEnabled()) {
             try {
                 logger.trace("Invalidating JWT cache for realm [{}]", name());
-                try (ReleasableLock ignored = jwtCacheHelper.acquireUpdateLock()) {
+                try (ReleasableLock ignored = jwtCacheHelper.acquireWriteLock()) {
                     jwtCache.invalidateAll();
                 }
                 logger.debug("Invalidated JWT cache for realm [{}]", name());

diff --git a/...urity/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/...urity/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
@@ -327,7 +327,7 @@ private void buildThenMaybeCacheRole(
             restrictedIndices,
             listener.delegateFailureAndWrap((delegate, role) -> {
                 if (role != null && tryCache) {
-                    try (ReleasableLock ignored = roleCacheHelper.acquireUpdateLock()) {
+                    try (ReleasableLock ignored = roleCacheHelper.acquireReadLock()) {
                         /* this is kinda spooky. We use a read/write lock to ensure we don't modify the cache if we hold
                          * the write lock (fetching stats for instance - which is kinda overkill?) but since we fetching
                          * stuff in an async fashion we need to make sure that if the cache got invalidated since we
@@ -546,7 +546,7 @@ public static void buildRoleFromDescriptors(
     public void invalidateAll() {
         numInvalidation.incrementAndGet();
         negativeLookupCache.invalidateAll();
-        try (ReleasableLock ignored = roleCacheHelper.acquireUpdateLock()) {
+        try (ReleasableLock ignored = roleCacheHelper.acquireReadLock()) {
             roleCache.invalidateAll();
         }
         dlsBitsetCache.clear("role store invalidation");