From 5b0de78399ea6d0cca8f29d015a26a456a98d62f Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 20 Jul 2021 15:35:30 +1000 Subject: [PATCH 1/3] Fix the usage of CacheIteratorHelper for service account. CacheIteratorHelper requires lock acquisition for any mutation to the underlying cache. This means it is incorrect to manipulate the cache without invocation of CacheIteratorHelper#acquireUpdateLock. This is OK for caches of simple values, but feels excessive for caches of ListenableFuture. This PR update the cache invalidation code to use cache.forEach instead of CacheInvalidator. It simplifies the code by removing any explicit lockings. The tradeoff is that it needs to build a list of keys to delete in memory. Overall it is a better tradeoff since no explicit locking is required and better leverage of Cache's own methods. --- .../CachingServiceAccountTokenStore.java | 42 +++++++++++++++---- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java index dcaa3be5a6dd2..7e97695b5fbaa 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java @@ -23,9 +23,15 @@ import org.elasticsearch.xpack.core.security.support.CacheIteratorHelper; import org.elasticsearch.xpack.security.support.CacheInvalidatorRegistry; +import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; +import java.util.Iterator; +import java.util.List; +import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Predicate; public abstract class CachingServiceAccountTokenStore implements ServiceAccountTokenStore, CacheInvalidatorRegistry.CacheInvalidator { @@ -125,17 +131,35 @@ private void authenticateWithCache(ServiceAccountToken token, ActionListener qualifiedTokenNames) { - if (cache != null) { - logger.trace("invalidating cache for service token [{}]", - Strings.collectionToCommaDelimitedString(qualifiedTokenNames)); - for (String qualifiedTokenName : qualifiedTokenNames) { - if (qualifiedTokenName.endsWith("/")) { - // Wildcard case of invalidating all tokens for a service account, e.g. "elastic/fleet-server/" - cacheIteratorHelper.removeKeysIf(key -> key.startsWith(qualifiedTokenName)); - } else { - cache.invalidate(qualifiedTokenName); + if (cache != null && false == qualifiedTokenNames.isEmpty()) { + logger.trace("invalidating cache for service token [{}]", Strings.collectionToCommaDelimitedString(qualifiedTokenNames)); + final Set exacts = new HashSet<>(qualifiedTokenNames); + final Set prefixes = new HashSet<>(); + final Iterator it = exacts.iterator(); + while (it.hasNext()) { + final String name = it.next(); + if (name.endsWith("/")) { + prefixes.add(name); + it.remove(); } } + + final Predicate predicate; + if (exacts.isEmpty()) { + predicate = k -> prefixes.stream().anyMatch(k::startsWith); + } else if (prefixes.isEmpty()) { + predicate = exacts::contains; + } else { + predicate = k -> exacts.contains(k) || prefixes.stream().anyMatch(k::startsWith); + } + + final List keys = new ArrayList<>(); + cache.forEach((k, v) -> { + if (predicate.test(k)) { + keys.add(k); + } + }); + keys.forEach(cache::invalidate); } } From d3400d788e265fa26a53ce35e5ddddfa2b8a4279 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 20 Jul 2021 15:51:00 +1000 Subject: [PATCH 2/3] remove dead code --- .../authc/service/CachingServiceAccountTokenStore.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java index 7e97695b5fbaa..c6fe78bdd7453 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java @@ -20,7 +20,6 @@ import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.xpack.core.security.action.service.TokenInfo.TokenSource; import org.elasticsearch.xpack.core.security.authc.support.Hasher; -import org.elasticsearch.xpack.core.security.support.CacheIteratorHelper; import org.elasticsearch.xpack.security.support.CacheInvalidatorRegistry; import java.util.ArrayList; @@ -48,7 +47,6 @@ public abstract class CachingServiceAccountTokenStore implements ServiceAccountT private final Settings settings; private final ThreadPool threadPool; private final Cache> cache; - private CacheIteratorHelper> cacheIteratorHelper; private final Hasher hasher; CachingServiceAccountTokenStore(Settings settings, ThreadPool threadPool) { @@ -60,10 +58,8 @@ public abstract class CachingServiceAccountTokenStore implements ServiceAccountT .setExpireAfterWrite(ttl) .setMaximumWeight(CACHE_MAX_TOKENS_SETTING.get(settings)) .build(); - cacheIteratorHelper = new CacheIteratorHelper<>(cache); } else { cache = null; - cacheIteratorHelper = null; } hasher = Hasher.resolve(CACHE_HASH_ALGO_SETTING.get(settings)); } From bf2f3c1c4d984d4a33ad0917afe67b14b3b482a5 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 21 Jul 2021 08:36:39 +1000 Subject: [PATCH 3/3] simplification --- .../CachingServiceAccountTokenStore.java | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java index c6fe78bdd7453..2a9bd40d48925 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStore.java @@ -127,7 +127,7 @@ private void authenticateWithCache(ServiceAccountToken token, ActionListener qualifiedTokenNames) { - if (cache != null && false == qualifiedTokenNames.isEmpty()) { + if (cache != null) { logger.trace("invalidating cache for service token [{}]", Strings.collectionToCommaDelimitedString(qualifiedTokenNames)); final Set exacts = new HashSet<>(qualifiedTokenNames); final Set prefixes = new HashSet<>(); @@ -140,22 +140,17 @@ public final void invalidate(Collection qualifiedTokenNames) { } } - final Predicate predicate; - if (exacts.isEmpty()) { - predicate = k -> prefixes.stream().anyMatch(k::startsWith); - } else if (prefixes.isEmpty()) { - predicate = exacts::contains; - } else { - predicate = k -> exacts.contains(k) || prefixes.stream().anyMatch(k::startsWith); + exacts.forEach(cache::invalidate); + if (false == prefixes.isEmpty()) { + final Predicate predicate = k -> prefixes.stream().anyMatch(k::startsWith); + final List keys = new ArrayList<>(); + cache.forEach((k, v) -> { + if (predicate.test(k)) { + keys.add(k); + } + }); + keys.forEach(cache::invalidate); } - - final List keys = new ArrayList<>(); - cache.forEach((k, v) -> { - if (predicate.test(k)) { - keys.add(k); - } - }); - keys.forEach(cache::invalidate); } }