From b72d0d3de19b0d6c2c650401201fa648ddb1f6be Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 27 Apr 2020 15:19:31 +1000 Subject: [PATCH 01/45] WIP --- .../authz/store/CompositeRolesStore.java | 3 +- .../authz/store/NativePrivilegeStore.java | 123 ++++++++++++++++++ 2 files changed, 125 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java index 377d29d01267c..c022db555dc03 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java @@ -422,7 +422,8 @@ public static void buildRoleFromDescriptors(Collection roleDescr final Set applicationPrivilegeNames = applicationPrivilegesMap.values().stream() .flatMap(Collection::stream) .collect(Collectors.toSet()); - privilegeStore.getPrivileges(applicationNames, applicationPrivilegeNames, ActionListener.wrap(appPrivileges -> { + // Role itself is cached, so skipping caching for application privileges + privilegeStore.getPrivilegesWithoutCaching(applicationNames, applicationPrivilegeNames, ActionListener.wrap(appPrivileges -> { applicationPrivilegesMap.forEach((key, names) -> ApplicationPrivilege.get(key.v1(), names, appPrivileges) .forEach(priv -> builder.addApplicationPrivilege(priv, key.v2()))); listener.onResponse(builder.build()); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 7142f6243d952..e9b3d9f8ed700 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -22,10 +22,14 @@ import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.cache.Cache; +import org.elasticsearch.common.cache.CacheBuilder; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.CollectionUtils; +import org.elasticsearch.common.util.concurrent.ReleasableLock; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.util.iterable.Iterables; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; @@ -45,18 +49,24 @@ import org.elasticsearch.xpack.core.security.action.role.ClearRolesCacheRequest; import org.elasticsearch.xpack.core.security.action.role.ClearRolesCacheResponse; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; +import org.elasticsearch.xpack.core.security.support.CacheIteratorHelper; import org.elasticsearch.xpack.security.support.SecurityIndexManager; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; +import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collector; import java.util.stream.Collectors; +import java.util.stream.Stream; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING; @@ -72,6 +82,12 @@ */ public class NativePrivilegeStore { + private static final Setting DESCRIPTOR_CACHE_SIZE_SETTING = + Setting.intSetting("xpack.security.authz.store.privileges.cache.max_size", 10_000, Setting.Property.NodeScope); + + private static final Setting APPLICATION_NAME_CACHE_SIZE_SETTING = + Setting.intSetting("xpack.security.authz.store.privileges.application_name.cache.max_size", 10_000, Setting.Property.NodeScope); + private static final Collector, ?, Map>> TUPLES_TO_MAP = Collectors.toMap( Tuple::v1, t -> CollectionUtils.newSingletonArrayList(t.v2()), (a, b) -> { @@ -83,15 +99,95 @@ public class NativePrivilegeStore { private final Settings settings; private final Client client; private final SecurityIndexManager securityIndexManager; + private final Cache> descriptorsCache; + private final CacheIteratorHelper> descriptorsCacheHelper; + private final Cache, Set> applicationNamesCache; + private final CacheIteratorHelper, Set> applicationNamesCacheHelper; public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManager securityIndexManager) { this.settings = settings; this.client = client; this.securityIndexManager = securityIndexManager; + CacheBuilder> builder = CacheBuilder.builder(); + final int cacheSize = DESCRIPTOR_CACHE_SIZE_SETTING.get(settings); + if (cacheSize >= 0) { + builder.setMaximumWeight(cacheSize); + builder.weigher((k, v) -> v.size()); + } + descriptorsCache = builder.build(); + descriptorsCacheHelper = new CacheIteratorHelper<>(descriptorsCache); + + CacheBuilder, Set> nameCacheBuilder = CacheBuilder.builder(); + final int nameCacheSize = APPLICATION_NAME_CACHE_SIZE_SETTING.get(settings); + if (nameCacheSize >= 0) { + nameCacheBuilder.setMaximumWeight(nameCacheSize); + nameCacheBuilder.weigher((k, v) -> k.size() + v.size()); + } + applicationNamesCache = nameCacheBuilder.build(); + applicationNamesCacheHelper = new CacheIteratorHelper<>(applicationNamesCache); } public void getPrivileges(Collection applications, Collection names, ActionListener> listener) { + + final Set applicationNamesCacheKey = (isEmpty(applications) || applications.contains("*")) ? + Collections.singleton("*") : Set.copyOf(applications); + + // Always fetch for the concrete application names even when the passed in application names do not + // contain any wildcard. This serves as a negative lookup. + final Set concreteApplicationNames = applicationNamesCache.get(applicationNamesCacheKey); + + if (concreteApplicationNames != null && concreteApplicationNames.size() == 0) { + listener.onResponse(Collections.emptySet()); + + } else if (concreteApplicationNames != null) { + final Tuple, Map>> cacheStatus = + cacheStatusForApplicationNames(concreteApplicationNames); + if (cacheStatus.v1().size() == 0) { + // everything is found in cache + final Set cachedDescriptors = + cacheStatus.v2().values().stream().flatMap(Collection::stream).collect(Collectors.toUnmodifiableSet()); + listener.onResponse(filterDescriptorsForNames(cachedDescriptors, names)); + } else { + // Some of the applications is not cached, need retrieval. Always fetch all privileges for an application + getPrivilegesWithoutCaching(cacheStatus.v1(), Collections.emptySet(), ActionListener.wrap(fetchedDescriptors -> { + final Map> mapOfFetchedDescriptors = + fetchedDescriptors.stream().collect( + Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); + try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { + for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { + descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> entry.getValue()); + } + } + final Set allDescriptors = + Stream.concat(cacheStatus.v2().values().stream().flatMap(Collection::stream), fetchedDescriptors.stream()) + .collect(Collectors.toUnmodifiableSet()); + listener.onResponse(filterDescriptorsForNames(allDescriptors, names)); + }, listener::onFailure)); + } + } else { + // Always fetch all privileges of an application for caching purpose + getPrivilegesWithoutCaching(applications, Collections.emptySet(), ActionListener.wrap(fetchedDescriptors -> { + // Populate caches + final Map> mapOfFetchedDescriptors = + fetchedDescriptors.stream().collect( + Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); + try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { + applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> Set.copyOf(mapOfFetchedDescriptors.keySet())); + } + try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { + for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { + descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> entry.getValue()); + } + } + listener.onResponse(filterDescriptorsForNames(fetchedDescriptors, names)); + }, listener::onFailure)); + } + } + + public void getPrivilegesWithoutCaching(Collection applications, Collection names, + ActionListener> listener) { + final SecurityIndexManager frozenSecurityIndex = securityIndexManager.freeze(); if (frozenSecurityIndex.indexExists() == false) { listener.onResponse(Collections.emptyList()); @@ -142,6 +238,33 @@ public void getPrivileges(Collection applications, Collection na } } + private Tuple, Map>> cacheStatusForApplicationNames( + Set concreteApplicationNames) { + + final Set uncachedApplicationNames = new HashSet<>(); + final Map> cachedDescriptors = new HashMap<>(); + + for (String concreteApplicationName: concreteApplicationNames) { + final Set descriptors = descriptorsCache.get(concreteApplicationName); + if (descriptors == null) { + uncachedApplicationNames.add(concreteApplicationName); + } else { + cachedDescriptors.put(concreteApplicationName, descriptors); + } + } + return Tuple.tuple(uncachedApplicationNames, cachedDescriptors); + } + + private Collection filterDescriptorsForNames( + Collection descriptors, Collection names) { + // empty set of names equals to retrieve everything + if (isEmpty(names)) { + return descriptors; + } + final Set uniqueNameSuffix = new HashSet<>(names); + return descriptors.stream().filter(d -> uniqueNameSuffix.contains(d.getName())).collect(Collectors.toUnmodifiableSet()); + } + private boolean isSinglePrivilegeMatch(Collection applications, Collection names) { return applications != null && applications.size() == 1 && hasWildcard(applications) == false && names != null && names.size() == 1; } From a02aba35ea0e04c5717e6240a71d0625d82e129a Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 27 Apr 2020 16:42:26 +1000 Subject: [PATCH 02/45] Add tests --- .../authz/store/NativePrivilegeStore.java | 75 +++++++++------- .../store/NativePrivilegeStoreTests.java | 88 +++++++++++++++++++ 2 files changed, 130 insertions(+), 33 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index e9b3d9f8ed700..b8ac1f3a4f902 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -83,10 +83,12 @@ public class NativePrivilegeStore { private static final Setting DESCRIPTOR_CACHE_SIZE_SETTING = - Setting.intSetting("xpack.security.authz.store.privileges.cache.max_size", 10_000, Setting.Property.NodeScope); + Setting.intSetting("xpack.security.authz.store.privileges.cache.max_size", + 10_000, Setting.Property.NodeScope); private static final Setting APPLICATION_NAME_CACHE_SIZE_SETTING = - Setting.intSetting("xpack.security.authz.store.privileges.application_name.cache.max_size", 10_000, Setting.Property.NodeScope); + Setting.intSetting("xpack.security.authz.store.privileges.application_name.cache.max_size", + 10_000, Setting.Property.NodeScope); private static final Collector, ?, Map>> TUPLES_TO_MAP = Collectors.toMap( Tuple::v1, @@ -135,25 +137,35 @@ public void getPrivileges(Collection applications, Collection na // Always fetch for the concrete application names even when the passed in application names do not // contain any wildcard. This serves as a negative lookup. - final Set concreteApplicationNames = applicationNamesCache.get(applicationNamesCacheKey); + Set concreteApplicationNames = applicationNamesCache.get(applicationNamesCacheKey); if (concreteApplicationNames != null && concreteApplicationNames.size() == 0) { listener.onResponse(Collections.emptySet()); - } else if (concreteApplicationNames != null) { - final Tuple, Map>> cacheStatus = - cacheStatusForApplicationNames(concreteApplicationNames); + } else { + final Tuple, Map>> cacheStatus; + if (concreteApplicationNames == null) { + cacheStatus = cacheStatusForApplicationNames(applicationNamesCacheKey); + } else { + cacheStatus = cacheStatusForApplicationNames(concreteApplicationNames); + } + if (cacheStatus.v1().size() == 0) { // everything is found in cache final Set cachedDescriptors = cacheStatus.v2().values().stream().flatMap(Collection::stream).collect(Collectors.toUnmodifiableSet()); listener.onResponse(filterDescriptorsForNames(cachedDescriptors, names)); } else { - // Some of the applications is not cached, need retrieval. Always fetch all privileges for an application + // Always fetch all privileges of an application for caching purpose getPrivilegesWithoutCaching(cacheStatus.v1(), Collections.emptySet(), ActionListener.wrap(fetchedDescriptors -> { - final Map> mapOfFetchedDescriptors = - fetchedDescriptors.stream().collect( - Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); + final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() + .collect(Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); + try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { + final Set allApplicationNames = + Stream.concat(cacheStatus.v2().keySet().stream(), mapOfFetchedDescriptors.keySet().stream()) + .collect(Collectors.toUnmodifiableSet()); + applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> allApplicationNames); + } try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> entry.getValue()); @@ -165,23 +177,6 @@ public void getPrivileges(Collection applications, Collection na listener.onResponse(filterDescriptorsForNames(allDescriptors, names)); }, listener::onFailure)); } - } else { - // Always fetch all privileges of an application for caching purpose - getPrivilegesWithoutCaching(applications, Collections.emptySet(), ActionListener.wrap(fetchedDescriptors -> { - // Populate caches - final Map> mapOfFetchedDescriptors = - fetchedDescriptors.stream().collect( - Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); - try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { - applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> Set.copyOf(mapOfFetchedDescriptors.keySet())); - } - try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { - for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { - descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> entry.getValue()); - } - } - listener.onResponse(filterDescriptorsForNames(fetchedDescriptors, names)); - }, listener::onFailure)); } } @@ -239,17 +234,21 @@ public void getPrivilegesWithoutCaching(Collection applications, Collect } private Tuple, Map>> cacheStatusForApplicationNames( - Set concreteApplicationNames) { + Set applicationNames) { final Set uncachedApplicationNames = new HashSet<>(); final Map> cachedDescriptors = new HashMap<>(); - for (String concreteApplicationName: concreteApplicationNames) { - final Set descriptors = descriptorsCache.get(concreteApplicationName); - if (descriptors == null) { - uncachedApplicationNames.add(concreteApplicationName); + for (String applicationName: applicationNames) { + if (applicationName.endsWith("*")) { + uncachedApplicationNames.add(applicationName); } else { - cachedDescriptors.put(concreteApplicationName, descriptors); + final Set descriptors = descriptorsCache.get(applicationName); + if (descriptors == null) { + uncachedApplicationNames.add(applicationName); + } else { + cachedDescriptors.put(applicationName, descriptors); + } } } return Tuple.tuple(uncachedApplicationNames, cachedDescriptors); @@ -469,4 +468,14 @@ private static String toDocId(String application, String name) { return DOC_TYPE_VALUE + "_" + application + ":" + name; } + // Package private for tests + Cache, Set> getApplicationNamesCache() { + return applicationNamesCache; + } + + // Package private for tests + Cache> getDescriptorsCache() { + return descriptorsCache; + } + } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index f0cfe173a083a..81972705095ee 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -47,14 +47,20 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; +import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; +import static java.util.Collections.emptySet; +import static java.util.Collections.singleton; +import static java.util.Collections.singletonList; import static org.elasticsearch.common.util.set.Sets.newHashSet; import static org.elasticsearch.index.seqno.SequenceNumbers.UNASSIGNED_SEQ_NO; import static org.hamcrest.Matchers.arrayContaining; @@ -245,6 +251,88 @@ public void testGetAllPrivileges() throws Exception { assertResult(sourcePrivileges, future); } + public void testGetPrivilegesCacheByApplicationNames() throws Exception { + final List sourcePrivileges = Arrays.asList( + new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()), + new ApplicationPrivilegeDescriptor("myapp", "user", newHashSet("action:login", "data:read/*"), emptyMap()), + new ApplicationPrivilegeDescriptor("myapp", "author", newHashSet("action:login", "data:read/*", "data:write/*"), emptyMap()) + ); + + final PlainActionFuture> future = new PlainActionFuture<>(); + store.getPrivileges(Arrays.asList("myapp", "yourapp"), null, future); + + final SearchHit[] hits = buildHits(sourcePrivileges); + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null)); + + assertEquals(Set.of("myapp"), store.getApplicationNamesCache().get(Set.of("myapp", "yourapp"))); + assertEquals(Set.copyOf(sourcePrivileges), store.getDescriptorsCache().get("myapp")); + assertResult(sourcePrivileges, future); + + // The 2nd call should use cache and success + final PlainActionFuture> future2 = new PlainActionFuture<>(); + store.getPrivileges(Arrays.asList("myapp", "yourapp"), null, future2); + listener.get().onResponse(null); + assertResult(sourcePrivileges, future2); + + // The 3rd call should use cache when the application name is part of the original query + final PlainActionFuture> future3 = new PlainActionFuture<>(); + store.getPrivileges(Arrays.asList("myapp"), null, future3); + listener.get().onResponse(null); + // Does not cache the name expansion if descriptors of the literal name is already cached + assertNull(store.getApplicationNamesCache().get(Set.of("myapp"))); + assertResult(sourcePrivileges, future3); + } + + public void testGetPrivilegesCacheWithNonExistentApplicationName() throws Exception { + final PlainActionFuture> future = new PlainActionFuture<>(); + store.getPrivileges(Collections.singletonList("*"), null, future); + final SearchHit[] hits = buildHits(emptyList()); + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null) ); + + assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("*"))); + assertResult(emptyList(), future); + + // The 2nd call should use cache + final PlainActionFuture> future2 = new PlainActionFuture<>(); + store.getPrivileges(Collections.singletonList("*"), null, future2); + listener.get().onResponse(null); + assertResult(emptyList(), future2); + } + + public void testGetPrivilegesCacheWithApplicationAndPrivilegeName() throws Exception { + final List sourcePrivileges = Arrays.asList( + new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()), + new ApplicationPrivilegeDescriptor("myapp", "user", newHashSet("action:login", "data:read/*"), emptyMap()), + new ApplicationPrivilegeDescriptor("myapp", "author", newHashSet("action:login", "data:read/*", "data:write/*"), emptyMap()) + ); + + final PlainActionFuture> future = new PlainActionFuture<>(); + store.getPrivileges(Collections.singletonList("myapp"), singletonList("user"), future); + + final SearchHit[] hits = buildHits(sourcePrivileges); + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null)); + + assertEquals(singleton("myapp"), store.getApplicationNamesCache().get(singleton("myapp"))); + // All privileges are cached + assertEquals(Set.copyOf(sourcePrivileges), store.getDescriptorsCache().get("myapp")); + assertResult(sourcePrivileges.subList(1, 2), future); + + // 2nd call with more privilege names can still use the cache + final PlainActionFuture> future2 = new PlainActionFuture<>(); + store.getPrivileges(Collections.singletonList("myapp"), Arrays.asList("user", "author"), future2); + listener.get().onResponse(null); + assertResult(sourcePrivileges.subList(1, 3), future2); + } + public void testPutPrivileges() throws Exception { final List putPrivileges = Arrays.asList( new ApplicationPrivilegeDescriptor("app1", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()), From 6d9fa0aca50fc709c47963d892720c487be02e37 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 27 Apr 2020 18:30:40 +1000 Subject: [PATCH 03/45] Add invalidation --- .../security/support/CacheIteratorHelper.java | 14 +++++ .../authz/store/NativePrivilegeStore.java | 27 +++++++-- .../store/NativePrivilegeStoreTests.java | 60 +++++++++++++------ 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java index 0dfdab26815a5..34aa585f1129e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.util.concurrent.ReleasableLock; import java.util.Iterator; +import java.util.Map; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Predicate; @@ -56,4 +57,17 @@ public void removeKeysIf(Predicate removeIf) { } } } + + public void removeValuesIf(Predicate removeIf) { + // the cache cannot be modified while doing this operation per the terms of the cache iterator + try (ReleasableLock ignored = this.acquireForIterator()) { + Iterator iterator = cache.keys().iterator(); + while (iterator.hasNext()) { + K key = iterator.next(); + if (removeIf.test(cache.get(key))) { + iterator.remove(); + } + } + } + } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index b8ac1f3a4f902..fa76d6faf1e0e 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -32,6 +32,7 @@ import org.elasticsearch.common.util.concurrent.ReleasableLock; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.util.iterable.Iterables; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -62,6 +63,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collector; @@ -105,6 +107,7 @@ public class NativePrivilegeStore { private final CacheIteratorHelper> descriptorsCacheHelper; private final Cache, Set> applicationNamesCache; private final CacheIteratorHelper, Set> applicationNamesCacheHelper; + private final AtomicLong numInvalidation = new AtomicLong(); public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManager securityIndexManager) { this.settings = settings; @@ -160,11 +163,13 @@ public void getPrivileges(Collection applications, Collection na getPrivilegesWithoutCaching(cacheStatus.v1(), Collections.emptySet(), ActionListener.wrap(fetchedDescriptors -> { final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() .collect(Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); - try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { - final Set allApplicationNames = - Stream.concat(cacheStatus.v2().keySet().stream(), mapOfFetchedDescriptors.keySet().stream()) - .collect(Collectors.toUnmodifiableSet()); - applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> allApplicationNames); + final Set allApplicationNames = + Stream.concat(cacheStatus.v2().keySet().stream(), mapOfFetchedDescriptors.keySet().stream()) + .collect(Collectors.toUnmodifiableSet()); + if (allApplicationNames.equals(applicationNamesCacheKey) == false) { + try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { + applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> allApplicationNames); + } } try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { @@ -180,6 +185,18 @@ public void getPrivileges(Collection applications, Collection na } } + public void invalidate(Set updatedApplicationNames) { + numInvalidation.incrementAndGet(); + applicationNamesCacheHelper.removeValuesIf(names -> Sets.intersection(names, updatedApplicationNames).isEmpty() == false); + descriptorsCacheHelper.removeKeysIf(updatedApplicationNames::contains); + } + + public void invalidateAll() { + numInvalidation.incrementAndGet(); + applicationNamesCache.invalidateAll(); + descriptorsCache.invalidateAll(); + } + public void getPrivilegesWithoutCaching(Collection applications, Collection names, ActionListener> listener) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index 81972705095ee..0494725cfd79b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -286,7 +286,36 @@ public void testGetPrivilegesCacheByApplicationNames() throws Exception { assertResult(sourcePrivileges, future3); } - public void testGetPrivilegesCacheWithNonExistentApplicationName() throws Exception { + public void testGetPrivilegesCacheWithApplicationAndPrivilegeName() throws Exception { + final List sourcePrivileges = Arrays.asList( + new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()), + new ApplicationPrivilegeDescriptor("myapp", "user", newHashSet("action:login", "data:read/*"), emptyMap()), + new ApplicationPrivilegeDescriptor("myapp", "author", newHashSet("action:login", "data:read/*", "data:write/*"), emptyMap()) + ); + + final PlainActionFuture> future = new PlainActionFuture<>(); + store.getPrivileges(Collections.singletonList("myapp"), singletonList("user"), future); + + final SearchHit[] hits = buildHits(sourcePrivileges); + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null)); + + // Not caching names with no wildcard + assertNull(store.getApplicationNamesCache().get(singleton("myapp"))); + // All privileges are cached + assertEquals(Set.copyOf(sourcePrivileges), store.getDescriptorsCache().get("myapp")); + assertResult(sourcePrivileges.subList(1, 2), future); + + // 2nd call with more privilege names can still use the cache + final PlainActionFuture> future2 = new PlainActionFuture<>(); + store.getPrivileges(Collections.singletonList("myapp"), Arrays.asList("user", "author"), future2); + listener.get().onResponse(null); + assertResult(sourcePrivileges.subList(1, 3), future2); + } + + public void testGetPrivilegesCacheWithNonExistentApplicationWildcard() throws Exception { final PlainActionFuture> future = new PlainActionFuture<>(); store.getPrivileges(Collections.singletonList("*"), null, future); final SearchHit[] hits = buildHits(emptyList()); @@ -296,6 +325,7 @@ public void testGetPrivilegesCacheWithNonExistentApplicationName() throws Except "_scrollId1", 1, 1, 0, 1, null, null) ); assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("*"))); + assertEquals(0, store.getDescriptorsCache().count()); assertResult(emptyList(), future); // The 2nd call should use cache @@ -305,32 +335,24 @@ public void testGetPrivilegesCacheWithNonExistentApplicationName() throws Except assertResult(emptyList(), future2); } - public void testGetPrivilegesCacheWithApplicationAndPrivilegeName() throws Exception { - final List sourcePrivileges = Arrays.asList( - new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()), - new ApplicationPrivilegeDescriptor("myapp", "user", newHashSet("action:login", "data:read/*"), emptyMap()), - new ApplicationPrivilegeDescriptor("myapp", "author", newHashSet("action:login", "data:read/*", "data:write/*"), emptyMap()) - ); - + public void testGetPrivilegesCacheWithNonExistentApplicationName() throws Exception { final PlainActionFuture> future = new PlainActionFuture<>(); - store.getPrivileges(Collections.singletonList("myapp"), singletonList("user"), future); - - final SearchHit[] hits = buildHits(sourcePrivileges); + store.getPrivileges(Collections.singletonList("no-such-app"), null, future); + final SearchHit[] hits = buildHits(emptyList()); listener.get().onResponse(new SearchResponse(new SearchResponseSections( new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), null, null, false, false, null, 1), - "_scrollId1", 1, 1, 0, 1, null, null)); + "_scrollId1", 1, 1, 0, 1, null, null) ); - assertEquals(singleton("myapp"), store.getApplicationNamesCache().get(singleton("myapp"))); - // All privileges are cached - assertEquals(Set.copyOf(sourcePrivileges), store.getDescriptorsCache().get("myapp")); - assertResult(sourcePrivileges.subList(1, 2), future); + assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("no-such-app"))); + assertEquals(0, store.getDescriptorsCache().count()); + assertResult(emptyList(), future); - // 2nd call with more privilege names can still use the cache + // The 2nd call should use cache final PlainActionFuture> future2 = new PlainActionFuture<>(); - store.getPrivileges(Collections.singletonList("myapp"), Arrays.asList("user", "author"), future2); + store.getPrivileges(Collections.singletonList("no-such-app"), null, future2); listener.get().onResponse(null); - assertResult(sourcePrivileges.subList(1, 3), future2); + assertResult(emptyList(), future2); } public void testPutPrivileges() throws Exception { From ee33fffc1ed432cd7baee23db54222947ac1c69b Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 27 Apr 2020 18:51:28 +1000 Subject: [PATCH 04/45] Add some corrurrency liveness protection --- .../authz/store/NativePrivilegeStore.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index fa76d6faf1e0e..feaf25543e6ab 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -159,6 +159,7 @@ public void getPrivileges(Collection applications, Collection na cacheStatus.v2().values().stream().flatMap(Collection::stream).collect(Collectors.toUnmodifiableSet()); listener.onResponse(filterDescriptorsForNames(cachedDescriptors, names)); } else { + final long invalidationCounter = numInvalidation.get(); // Always fetch all privileges of an application for caching purpose getPrivilegesWithoutCaching(cacheStatus.v1(), Collections.emptySet(), ActionListener.wrap(fetchedDescriptors -> { final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() @@ -166,14 +167,19 @@ public void getPrivileges(Collection applications, Collection na final Set allApplicationNames = Stream.concat(cacheStatus.v2().keySet().stream(), mapOfFetchedDescriptors.keySet().stream()) .collect(Collectors.toUnmodifiableSet()); - if (allApplicationNames.equals(applicationNamesCacheKey) == false) { - try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { - applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> allApplicationNames); + // Avoid caching potential stale results. + // TODO: But it is still possible that cache gets invalidated immediately after the if check + if (invalidationCounter == numInvalidation.get()) { + // Do not cache the names if expansion has no effect + if (allApplicationNames.equals(applicationNamesCacheKey) == false) { + try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { + applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> allApplicationNames); + } } - } - try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { - for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { - descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> entry.getValue()); + try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { + for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { + descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> entry.getValue()); + } } } final Set allDescriptors = From 4bbd16d80b1279f7bd981eed82409fd9830d3df4 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 27 Apr 2020 22:37:58 +1000 Subject: [PATCH 05/45] Add API for both rest and transport --- .../privilege/ClearPrivilegesCacheAction.java | 19 +++++ .../ClearPrivilegesCacheRequest.java | 66 +++++++++++++++ .../ClearPrivilegesCacheResponse.java | 64 +++++++++++++++ .../xpack/security/Security.java | 5 ++ .../TransportClearPrivilegesCacheAction.java | 82 +++++++++++++++++++ .../authz/store/NativePrivilegeStore.java | 53 ++++++------ .../privilege/RestClearPrivilegesAction.java | 47 +++++++++++ .../store/NativePrivilegeStoreTests.java | 34 ++++++-- 8 files changed, 339 insertions(+), 31 deletions(-) create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheAction.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheResponse.java create mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java create mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheAction.java new file mode 100644 index 0000000000000..2fbe2e0639112 --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheAction.java @@ -0,0 +1,19 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.core.security.action.privilege; + +import org.elasticsearch.action.ActionType; + +public class ClearPrivilegesCacheAction extends ActionType { + + public static final ClearPrivilegesCacheAction INSTANCE = new ClearPrivilegesCacheAction(); + public static final String NAME = "cluster:admin/xpack/security/privilege/cache/clear"; + + protected ClearPrivilegesCacheAction() { + super(NAME, ClearPrivilegesCacheResponse::new); + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java new file mode 100644 index 0000000000000..be72fc8671ddf --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java @@ -0,0 +1,66 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.core.security.action.privilege; + +import org.elasticsearch.action.support.nodes.BaseNodesRequest; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.transport.TransportRequest; + +import java.io.IOException; + +public class ClearPrivilegesCacheRequest extends BaseNodesRequest { + + String[] applicationNames; + + public ClearPrivilegesCacheRequest() { + super((String[]) null); + } + + public ClearPrivilegesCacheRequest(StreamInput in) throws IOException { + super(in); + applicationNames = in.readOptionalStringArray(); + } + + public ClearPrivilegesCacheRequest names(String... applicationNames) { + this.applicationNames = applicationNames; + return this; + } + + public String[] names() { + return applicationNames; + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + super.writeTo(out); + out.writeOptionalStringArray(applicationNames); + } + + public static class Node extends TransportRequest { + private String[] names; + + public Node(StreamInput in) throws IOException { + super(in); + names = in.readOptionalStringArray(); + } + + public Node(ClearPrivilegesCacheRequest request) { + this.names = request.names(); + } + + public String[] getNames() { + return names; + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + super.writeTo(out); + out.writeOptionalStringArray(names); + } + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheResponse.java new file mode 100644 index 0000000000000..85dcc3b94650f --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheResponse.java @@ -0,0 +1,64 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.core.security.action.privilege; + +import org.elasticsearch.action.FailedNodeException; +import org.elasticsearch.action.support.nodes.BaseNodeResponse; +import org.elasticsearch.action.support.nodes.BaseNodesResponse; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.xcontent.ToXContentFragment; +import org.elasticsearch.common.xcontent.XContentBuilder; + +import java.io.IOException; +import java.util.List; + +public class ClearPrivilegesCacheResponse extends BaseNodesResponse + implements ToXContentFragment { + + public ClearPrivilegesCacheResponse(StreamInput in) throws IOException { + super(in); + } + + public ClearPrivilegesCacheResponse(ClusterName clusterName, List nodes, List failures) { + super(clusterName, nodes, failures); + } + + @Override + protected List readNodesFrom(StreamInput in) throws IOException { + return in.readList(Node::new); + } + + @Override + protected void writeNodesTo(StreamOutput out, List nodes) throws IOException { + out.writeList(nodes); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject("nodes"); + for (Node node : getNodes()) { + builder.startObject(node.getNode().getId()); + builder.field("name", node.getNode().getName()); + builder.endObject(); + } + builder.endObject(); + return builder; + } + + public static class Node extends BaseNodeResponse { + public Node(StreamInput in) throws IOException { + super(in); + } + + public Node(DiscoveryNode node) { + super(node); + } + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index a62a6ef37ec73..f403d7f7bd158 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -88,6 +88,7 @@ import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectAuthenticateAction; import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectLogoutAction; import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectPrepareAuthenticationAction; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; import org.elasticsearch.xpack.core.security.action.privilege.DeletePrivilegesAction; import org.elasticsearch.xpack.core.security.action.privilege.GetBuiltinPrivilegesAction; import org.elasticsearch.xpack.core.security.action.privilege.GetPrivilegesAction; @@ -151,6 +152,7 @@ import org.elasticsearch.xpack.security.action.oidc.TransportOpenIdConnectAuthenticateAction; import org.elasticsearch.xpack.security.action.oidc.TransportOpenIdConnectLogoutAction; import org.elasticsearch.xpack.security.action.oidc.TransportOpenIdConnectPrepareAuthenticationAction; +import org.elasticsearch.xpack.security.action.privilege.TransportClearPrivilegesCacheAction; import org.elasticsearch.xpack.security.action.privilege.TransportDeletePrivilegesAction; import org.elasticsearch.xpack.security.action.privilege.TransportGetBuiltinPrivilegesAction; import org.elasticsearch.xpack.security.action.privilege.TransportGetPrivilegesAction; @@ -217,6 +219,7 @@ import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectAuthenticateAction; import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectLogoutAction; import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectPrepareAuthenticationAction; +import org.elasticsearch.xpack.security.rest.action.privilege.RestClearPrivilegesAction; import org.elasticsearch.xpack.security.rest.action.privilege.RestDeletePrivilegesAction; import org.elasticsearch.xpack.security.rest.action.privilege.RestGetBuiltinPrivilegesAction; import org.elasticsearch.xpack.security.rest.action.privilege.RestGetPrivilegesAction; @@ -727,6 +730,7 @@ public void onIndexModule(IndexModule module) { return Arrays.asList( new ActionHandler<>(ClearRealmCacheAction.INSTANCE, TransportClearRealmCacheAction.class), new ActionHandler<>(ClearRolesCacheAction.INSTANCE, TransportClearRolesCacheAction.class), + new ActionHandler<>(ClearPrivilegesCacheAction.INSTANCE, TransportClearPrivilegesCacheAction.class), new ActionHandler<>(GetUsersAction.INSTANCE, TransportGetUsersAction.class), new ActionHandler<>(PutUserAction.INSTANCE, TransportPutUserAction.class), new ActionHandler<>(DeleteUserAction.INSTANCE, TransportDeleteUserAction.class), @@ -786,6 +790,7 @@ public List getRestHandlers(Settings settings, RestController restC new RestAuthenticateAction(settings, securityContext.get(), getLicenseState()), new RestClearRealmCacheAction(settings, getLicenseState()), new RestClearRolesCacheAction(settings, getLicenseState()), + new RestClearPrivilegesAction(settings, getLicenseState()), new RestGetUsersAction(settings, getLicenseState()), new RestPutUserAction(settings, getLicenseState()), new RestDeleteUserAction(settings, getLicenseState()), diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java new file mode 100644 index 0000000000000..986c73f35b4dc --- /dev/null +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -0,0 +1,82 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.security.action.privilege; + +import org.elasticsearch.action.FailedNodeException; +import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.action.support.nodes.TransportNodesAction; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.tasks.Task; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.TransportService; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheResponse; +import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; +import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore; + +import java.io.IOException; +import java.util.Arrays; +import java.util.List; + +public class TransportClearPrivilegesCacheAction + extends TransportNodesAction { + + private final NativePrivilegeStore privilegesStore; + private final CompositeRolesStore rolesStore; + + @Inject + public TransportClearPrivilegesCacheAction( + ThreadPool threadPool, + ClusterService clusterService, + TransportService transportService, + ActionFilters actionFilters, + NativePrivilegeStore privilegesStore, + CompositeRolesStore rolesStore) { + super( + ClearPrivilegesCacheAction.NAME, + threadPool, + clusterService, + transportService, + actionFilters, + ClearPrivilegesCacheRequest::new, + ClearPrivilegesCacheRequest.Node::new, + ThreadPool.Names.MANAGEMENT, + ClearPrivilegesCacheResponse.Node.class); + this.privilegesStore = privilegesStore; + this.rolesStore = rolesStore; + } + + @Override + protected ClearPrivilegesCacheResponse newResponse( + ClearPrivilegesCacheRequest request, List nodes, List failures) { + return new ClearPrivilegesCacheResponse(clusterService.getClusterName(), nodes, failures); + } + + @Override + protected ClearPrivilegesCacheRequest.Node newNodeRequest(ClearPrivilegesCacheRequest request) { + return new ClearPrivilegesCacheRequest.Node(request); + } + + @Override + protected ClearPrivilegesCacheResponse.Node newNodeResponse(StreamInput in) throws IOException { + return new ClearPrivilegesCacheResponse.Node(in); + } + + @Override + protected ClearPrivilegesCacheResponse.Node nodeOperation(ClearPrivilegesCacheRequest.Node request, Task task) { + if (request.getNames() == null || request.getNames().length == 0) { + privilegesStore.invalidateAll(); + } else { + privilegesStore.invalidate(Arrays.asList(request.getNames())); + } + rolesStore.invalidateAll(); + return new ClearPrivilegesCacheResponse.Node(clusterService.localNode()); + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index feaf25543e6ab..72ceaffc863d9 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -46,9 +46,9 @@ import org.elasticsearch.index.query.TermsQueryBuilder; import org.elasticsearch.xpack.core.ClientHelper; import org.elasticsearch.xpack.core.security.ScrollHelper; -import org.elasticsearch.xpack.core.security.action.role.ClearRolesCacheAction; -import org.elasticsearch.xpack.core.security.action.role.ClearRolesCacheRequest; -import org.elasticsearch.xpack.core.security.action.role.ClearRolesCacheResponse; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheResponse; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; import org.elasticsearch.xpack.core.security.support.CacheIteratorHelper; import org.elasticsearch.xpack.security.support.SecurityIndexManager; @@ -64,7 +64,6 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; -import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collector; import java.util.stream.Collectors; @@ -191,18 +190,6 @@ public void getPrivileges(Collection applications, Collection na } } - public void invalidate(Set updatedApplicationNames) { - numInvalidation.incrementAndGet(); - applicationNamesCacheHelper.removeValuesIf(names -> Sets.intersection(names, updatedApplicationNames).isEmpty() == false); - descriptorsCacheHelper.removeKeysIf(updatedApplicationNames::contains); - } - - public void invalidateAll() { - numInvalidation.incrementAndGet(); - applicationNamesCache.invalidateAll(); - descriptorsCache.invalidateAll(); - } - public void getPrivilegesWithoutCaching(Collection applications, Collection names, ActionListener> listener) { @@ -256,6 +243,20 @@ public void getPrivilegesWithoutCaching(Collection applications, Collect } } + public void invalidate(Collection updatedApplicationNames) { + numInvalidation.incrementAndGet(); + final Set uniqueUpdatedApplicationNames = Set.copyOf(updatedApplicationNames); + // Always completely invalidate application names cache due to wildcard + applicationNamesCache.invalidateAll(); + descriptorsCacheHelper.removeKeysIf(updatedApplicationNames::contains); + } + + public void invalidateAll() { + numInvalidation.incrementAndGet(); + applicationNamesCache.invalidateAll(); + descriptorsCache.invalidateAll(); + } + private Tuple, Map>> cacheStatusForApplicationNames( Set applicationNames) { @@ -382,7 +383,9 @@ public void putPrivileges(Collection privileges, .map(r -> r.getId()) .map(NativePrivilegeStore::nameFromDocId) .collect(TUPLES_TO_MAP); - clearRolesCache(listener, createdNames); + clearCaches(listener, + privileges.stream().map(ApplicationPrivilegeDescriptor::getApplication).collect(Collectors.toList()), + createdNames); }, listener::onFailure), privileges.size()); for (ApplicationPrivilegeDescriptor privilege : privileges) { innerPutPrivilege(privilege, refreshPolicy, groupListener); @@ -404,7 +407,6 @@ private void innerPutPrivilege(ApplicationPrivilegeDescriptor privilege, WriteRe logger.warn("Failed to put privilege {} - {}", Strings.toString(privilege), e.toString()); listener.onFailure(e); } - } public void deletePrivileges(String application, Collection names, WriteRequest.RefreshPolicy refreshPolicy, @@ -423,7 +425,7 @@ public void deletePrivileges(String application, Collection names, Write .map(r -> r.getId()) .map(NativePrivilegeStore::nameFromDocId) .collect(TUPLES_TO_MAP); - clearRolesCache(listener, deletedNames); + clearCaches(listener, Collections.singletonList(application), deletedNames); }, listener::onFailure), names.size()); for (String name : names) { ClientHelper.executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, @@ -435,21 +437,22 @@ public void deletePrivileges(String application, Collection names, Write } } - private void clearRolesCache(ActionListener listener, T value) { + private void clearCaches(ActionListener listener, List applicationNames, T value) { // This currently clears _all_ roles, but could be improved to clear only those roles that reference the affected application - ClearRolesCacheRequest request = new ClearRolesCacheRequest(); - executeAsyncWithOrigin(client, SECURITY_ORIGIN, ClearRolesCacheAction.INSTANCE, request, + final ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest().names(applicationNames.toArray(String[]::new)); + executeAsyncWithOrigin(client, SECURITY_ORIGIN, ClearPrivilegesCacheAction.INSTANCE, request, new ActionListener<>() { @Override - public void onResponse(ClearRolesCacheResponse nodes) { + public void onResponse(ClearPrivilegesCacheResponse nodes) { listener.onResponse(value); } @Override public void onFailure(Exception e) { - logger.error("unable to clear role cache", e); + logger.error("unable to clear application privileges and role cache", e); listener.onFailure( - new ElasticsearchException("clearing the role cache failed. please clear the role cache manually", e)); + new ElasticsearchException("clearing the application privileges and role cache failed. " + + "please clear the caches manually", e)); } }); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java new file mode 100644 index 0000000000000..6dfb516c1a09d --- /dev/null +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java @@ -0,0 +1,47 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.security.rest.action.privilege; + +import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.license.XPackLicenseState; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.rest.action.RestActions.NodesResponseRestListener; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; +import org.elasticsearch.xpack.security.rest.action.saml.SamlBaseRestHandler; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; + +import static org.elasticsearch.rest.RestRequest.Method.POST; + +public class RestClearPrivilegesAction extends SamlBaseRestHandler { + + public RestClearPrivilegesAction(Settings settings, XPackLicenseState licenseState) { + super(settings, licenseState); + } + + @Override + public String getName() { + return "security_clear_privileges_cache_action"; + } + + @Override + public List routes() { + return Collections.singletonList(new Route(POST, "/_security/privilege/{application}/_clear_cache")); + } + + @Override + protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException { + String[] applicationNames = request.paramAsStringArrayOrEmptyIfAll("application"); + final ClearPrivilegesCacheRequest req = new ClearPrivilegesCacheRequest().names(applicationNames); + return channel -> client.execute(ClearPrivilegesCacheAction.INSTANCE, req, new NodesResponseRestListener<>(channel)); + } + +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index 0494725cfd79b..c25851ad375d0 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -35,7 +35,7 @@ import org.elasticsearch.search.SearchHits; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.client.NoOpClient; -import org.elasticsearch.xpack.core.security.action.role.ClearRolesCacheRequest; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; import org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames; import org.elasticsearch.xpack.security.support.SecurityIndexManager; @@ -362,8 +362,8 @@ public void testPutPrivileges() throws Exception { new ApplicationPrivilegeDescriptor("app2", "all", newHashSet("*"), emptyMap()) ); - final PlainActionFuture>> future = new PlainActionFuture<>(); - store.putPrivileges(putPrivileges, WriteRequest.RefreshPolicy.IMMEDIATE, future); + final PlainActionFuture>> putPrivilegeFuture = new PlainActionFuture<>(); + store.putPrivileges(putPrivileges, WriteRequest.RefreshPolicy.IMMEDIATE, putPrivilegeFuture); assertThat(requests, iterableWithSize(putPrivileges.size())); assertThat(requests, everyItem(instanceOf(IndexRequest.class))); @@ -392,10 +392,10 @@ public void testPutPrivileges() throws Exception { assertBusy(() -> assertFalse(requests.isEmpty()), 1, TimeUnit.SECONDS); assertThat(requests, iterableWithSize(1)); - assertThat(requests.get(0), instanceOf(ClearRolesCacheRequest.class)); + assertThat(requests.get(0), instanceOf(ClearPrivilegesCacheRequest.class)); listener.get().onResponse(null); - final Map> map = future.actionGet(); + final Map> map = putPrivilegeFuture.actionGet(); assertThat(map.entrySet(), iterableWithSize(2)); assertThat(map.get("app1"), iterableWithSize(1)); assertThat(map.get("app2"), iterableWithSize(1)); @@ -432,7 +432,7 @@ public void testDeletePrivileges() throws Exception { assertBusy(() -> assertFalse(requests.isEmpty()), 1, TimeUnit.SECONDS); assertThat(requests, iterableWithSize(1)); - assertThat(requests.get(0), instanceOf(ClearRolesCacheRequest.class)); + assertThat(requests.get(0), instanceOf(ClearPrivilegesCacheRequest.class)); listener.get().onResponse(null); final Map> map = future.actionGet(); @@ -441,6 +441,28 @@ public void testDeletePrivileges() throws Exception { assertThat(map.get("app1"), containsInAnyOrder("p1", "p3")); } + public void testInvalidate() { + store.getApplicationNamesCache().put(singleton("*"), Set.of()); + store.getDescriptorsCache().put("app-1", + singleton(new ApplicationPrivilegeDescriptor("app-1", "read", emptySet(), emptyMap()))); + store.getDescriptorsCache().put("app-2", + singleton(new ApplicationPrivilegeDescriptor("app-2", "read", emptySet(), emptyMap()))); + store.invalidate(singletonList("app-1")); + assertEquals(0, store.getApplicationNamesCache().count()); + assertEquals(1, store.getDescriptorsCache().count()); + } + + public void testInvalidateAll() { + store.getApplicationNamesCache().put(singleton("*"), Set.of()); + store.getDescriptorsCache().put("app-1", + singleton(new ApplicationPrivilegeDescriptor("app-1", "read", emptySet(), emptyMap()))); + store.getDescriptorsCache().put("app-2", + singleton(new ApplicationPrivilegeDescriptor("app-2", "read", emptySet(), emptyMap()))); + store.invalidateAll(); + assertEquals(0, store.getApplicationNamesCache().count()); + assertEquals(0, store.getDescriptorsCache().count()); + } + private SearchHit[] buildHits(List sourcePrivileges) { final SearchHit[] hits = new SearchHit[sourcePrivileges.size()]; for (int i = 0; i < hits.length; i++) { From df28676568c9ac872ebcb07c31da7449b313d2f0 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 01:54:25 +1000 Subject: [PATCH 06/45] Simplify NativePrivilegeStore --- .../authz/store/CompositeRolesStore.java | 2 +- .../authz/store/NativePrivilegeStore.java | 74 +++++++++---------- .../store/NativeRolesStoreIntegTests.java | 73 ++++++++++++++++++ 3 files changed, 108 insertions(+), 41 deletions(-) create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java index c022db555dc03..54ed260fb2fb7 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java @@ -423,7 +423,7 @@ public static void buildRoleFromDescriptors(Collection roleDescr .flatMap(Collection::stream) .collect(Collectors.toSet()); // Role itself is cached, so skipping caching for application privileges - privilegeStore.getPrivilegesWithoutCaching(applicationNames, applicationPrivilegeNames, ActionListener.wrap(appPrivileges -> { + privilegeStore.getPrivileges(applicationNames, applicationPrivilegeNames, ActionListener.wrap(appPrivileges -> { applicationPrivilegesMap.forEach((key, names) -> ApplicationPrivilege.get(key.v1(), names, appPrivileges) .forEach(priv -> builder.addApplicationPrivilege(priv, key.v2()))); listener.onResponse(builder.build()); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 72ceaffc863d9..9bfdab88543e1 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -31,8 +31,6 @@ import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.util.concurrent.ReleasableLock; import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.common.util.iterable.Iterables; -import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -121,19 +119,20 @@ public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManag descriptorsCache = builder.build(); descriptorsCacheHelper = new CacheIteratorHelper<>(descriptorsCache); - CacheBuilder, Set> nameCacheBuilder = CacheBuilder.builder(); + CacheBuilder, Set> applicationNamesCacheBuilder = CacheBuilder.builder(); final int nameCacheSize = APPLICATION_NAME_CACHE_SIZE_SETTING.get(settings); if (nameCacheSize >= 0) { - nameCacheBuilder.setMaximumWeight(nameCacheSize); - nameCacheBuilder.weigher((k, v) -> k.size() + v.size()); + applicationNamesCacheBuilder.setMaximumWeight(nameCacheSize); + applicationNamesCacheBuilder.weigher((k, v) -> k.size() + v.size()); } - applicationNamesCache = nameCacheBuilder.build(); + applicationNamesCache = applicationNamesCacheBuilder.build(); applicationNamesCacheHelper = new CacheIteratorHelper<>(applicationNamesCache); } public void getPrivileges(Collection applications, Collection names, ActionListener> listener) { + // TODO: We should have a way to express true Zero applications final Set applicationNamesCacheKey = (isEmpty(applications) || applications.contains("*")) ? Collections.singleton("*") : Set.copyOf(applications); @@ -142,6 +141,7 @@ public void getPrivileges(Collection applications, Collection na Set concreteApplicationNames = applicationNamesCache.get(applicationNamesCacheKey); if (concreteApplicationNames != null && concreteApplicationNames.size() == 0) { + logger.debug("returning empty application privileges as application names result in empty list"); listener.onResponse(Collections.emptySet()); } else { @@ -152,32 +152,39 @@ public void getPrivileges(Collection applications, Collection na cacheStatus = cacheStatusForApplicationNames(concreteApplicationNames); } - if (cacheStatus.v1().size() == 0) { - // everything is found in cache + if (cacheStatus.v1().isEmpty()) { + logger.debug("All application privileges found in cache"); final Set cachedDescriptors = cacheStatus.v2().values().stream().flatMap(Collection::stream).collect(Collectors.toUnmodifiableSet()); listener.onResponse(filterDescriptorsForNames(cachedDescriptors, names)); } else { final long invalidationCounter = numInvalidation.get(); // Always fetch all privileges of an application for caching purpose - getPrivilegesWithoutCaching(cacheStatus.v1(), Collections.emptySet(), ActionListener.wrap(fetchedDescriptors -> { + logger.debug("Fetching application privilege documents for: {}", cacheStatus.v1()); + innerGetPrivileges(cacheStatus.v1(), ActionListener.wrap(fetchedDescriptors -> { final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() .collect(Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); final Set allApplicationNames = Stream.concat(cacheStatus.v2().keySet().stream(), mapOfFetchedDescriptors.keySet().stream()) .collect(Collectors.toUnmodifiableSet()); // Avoid caching potential stale results. - // TODO: But it is still possible that cache gets invalidated immediately after the if check + // TODO: It is still possible that cache gets invalidated immediately after the if check if (invalidationCounter == numInvalidation.get()) { // Do not cache the names if expansion has no effect if (allApplicationNames.equals(applicationNamesCacheKey) == false) { try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { - applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> allApplicationNames); + applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> { + logger.debug("Caching application names query: {} = {}", k, allApplicationNames); + return allApplicationNames; + }); } } try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { - descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> entry.getValue()); + descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> { + logger.debug("Caching descriptors for application: {}", k); + return entry.getValue(); + }); } } } @@ -190,41 +197,23 @@ public void getPrivileges(Collection applications, Collection na } } - public void getPrivilegesWithoutCaching(Collection applications, Collection names, + private void innerGetPrivileges(Collection applications, ActionListener> listener) { + assert Objects.requireNonNull(applications).isEmpty() == false; final SecurityIndexManager frozenSecurityIndex = securityIndexManager.freeze(); if (frozenSecurityIndex.indexExists() == false) { listener.onResponse(Collections.emptyList()); } else if (frozenSecurityIndex.isAvailable() == false) { listener.onFailure(frozenSecurityIndex.getUnavailableReason()); - } else if (isSinglePrivilegeMatch(applications, names)) { - getPrivilege(Objects.requireNonNull(Iterables.get(applications, 0)), Objects.requireNonNull(Iterables.get(names, 0)), - ActionListener.wrap(privilege -> - listener.onResponse(privilege == null ? Collections.emptyList() : Collections.singletonList(privilege)), - listener::onFailure)); } else { securityIndexManager.checkIndexVersionThenExecute(listener::onFailure, () -> { - final QueryBuilder query; + final TermQueryBuilder typeQuery = QueryBuilders .termQuery(ApplicationPrivilegeDescriptor.Fields.TYPE.getPreferredName(), DOC_TYPE_VALUE); - if (isEmpty(applications) && isEmpty(names)) { - query = typeQuery; - } else if (isEmpty(names)) { - query = QueryBuilders.boolQuery().filter(typeQuery).filter(getApplicationNameQuery(applications)); - } else if (isEmpty(applications)) { - query = QueryBuilders.boolQuery().filter(typeQuery) - .filter(getPrivilegeNameQuery(names)); - } else if (hasWildcard(applications)) { - query = QueryBuilders.boolQuery().filter(typeQuery) - .filter(getApplicationNameQuery(applications)) - .filter(getPrivilegeNameQuery(names)); - } else { - final String[] docIds = applications.stream() - .flatMap(a -> names.stream().map(n -> toDocId(a, n))) - .toArray(String[]::new); - query = QueryBuilders.boolQuery().filter(typeQuery).filter(QueryBuilders.idsQuery().addIds(docIds)); - } + final QueryBuilder query = QueryBuilders.boolQuery().filter(typeQuery) + .filter(getApplicationNameQuery(applications)); + final Supplier supplier = client.threadPool().getThreadContext().newRestorableContext(false); try (ThreadContext.StoredContext ignore = client.threadPool().getThreadContext().stashWithOrigin(SECURITY_ORIGIN)) { SearchRequest request = client.prepareSearch(SECURITY_MAIN_ALIAS) @@ -234,7 +223,8 @@ public void getPrivilegesWithoutCaching(Collection applications, Collect .setFetchSource(true) .request(); logger.trace(() -> - new ParameterizedMessage("Searching for privileges [{}] with query [{}]", names, Strings.toString(query))); + new ParameterizedMessage("Searching for [{}] privileges with query [{}]", + applications, Strings.toString(query))); request.indicesOptions().ignoreUnavailable(); ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), hit -> buildPrivilege(hit.getId(), hit.getSourceRef())); @@ -244,14 +234,15 @@ public void getPrivilegesWithoutCaching(Collection applications, Collect } public void invalidate(Collection updatedApplicationNames) { + logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); numInvalidation.incrementAndGet(); - final Set uniqueUpdatedApplicationNames = Set.copyOf(updatedApplicationNames); // Always completely invalidate application names cache due to wildcard applicationNamesCache.invalidateAll(); descriptorsCacheHelper.removeKeysIf(updatedApplicationNames::contains); } public void invalidateAll() { + logger.debug("Invalidating all application privileges caches"); numInvalidation.incrementAndGet(); applicationNamesCache.invalidateAll(); descriptorsCache.invalidateAll(); @@ -275,6 +266,9 @@ private Tuple, Map>> cac } } } + if (cachedDescriptors.isEmpty() == false) { + logger.debug("Application privileges found in cache: {}", cachedDescriptors.keySet()); + } return Tuple.tuple(uncachedApplicationNames, cachedDescriptors); } @@ -288,8 +282,8 @@ private Collection filterDescriptorsForNames( return descriptors.stream().filter(d -> uniqueNameSuffix.contains(d.getName())).collect(Collectors.toUnmodifiableSet()); } - private boolean isSinglePrivilegeMatch(Collection applications, Collection names) { - return applications != null && applications.size() == 1 && hasWildcard(applications) == false && names != null && names.size() == 1; + private boolean isSinglePrivilegeMatch(Collection applications) { + return applications != null && applications.size() == 1 && hasWildcard(applications) == false; } private boolean hasWildcard(Collection applications) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java new file mode 100644 index 0000000000000..1f9f1ea706426 --- /dev/null +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java @@ -0,0 +1,73 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.security.authz.store; + +import org.elasticsearch.action.ActionFuture; +import org.elasticsearch.test.SecurityIntegTestCase; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheResponse; +import org.elasticsearch.xpack.core.security.action.privilege.GetPrivilegesRequestBuilder; +import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesAction; +import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesRequest; +import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesResponse; +import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; +import org.junit.Before; + +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.concurrent.ExecutionException; + +import static java.util.Collections.emptyMap; + +public class NativeRolesStoreIntegTests extends SecurityIntegTestCase { + + @Before + public void configureApplicationPrivileges() { + final List applicationPrivilegeDescriptors = Arrays.asList( + new ApplicationPrivilegeDescriptor("app-1", "read", Set.of("a:b:c", "x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-1", "write", Set.of("a:b:c", "x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-1", "admin", Set.of("a:b:c", "x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "read", Set.of("e:f:g", "t:u:v"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "write", Set.of("e:f:g", "t:u:v"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "admin", Set.of("e:f:g", "t:u:v"), emptyMap())); + + final PutPrivilegesRequest putPrivilegesRequest = new PutPrivilegesRequest(); + putPrivilegesRequest.setPrivileges(applicationPrivilegeDescriptors); + final ActionFuture future = + client().execute(PutPrivilegesAction.INSTANCE, putPrivilegesRequest); + + final PutPrivilegesResponse putPrivilegesResponse = future.actionGet(); + assertEquals(2, putPrivilegesResponse.created().size()); + assertEquals(6, putPrivilegesResponse.created().values().stream().mapToInt(List::size).sum()); + } + + public void testCache() { + + ApplicationPrivilegeDescriptor[] privileges = new GetPrivilegesRequestBuilder(client()) + .application("app-2").privileges("write").execute().actionGet().privileges(); + + assertNotNull(privileges); + assertEquals(1, privileges.length); + assertEquals("app-2", privileges[0].getApplication()); + assertEquals("write", privileges[0].getName()); + + // Invalidate cache + final ClearPrivilegesCacheResponse clearPrivilegesCacheResponse = + client().execute(ClearPrivilegesCacheAction.INSTANCE, new ClearPrivilegesCacheRequest()).actionGet(); + assertEquals(cluster().size(), clearPrivilegesCacheResponse.getNodes().size()); + assertFalse(clearPrivilegesCacheResponse.hasFailures()); + + privileges = new GetPrivilegesRequestBuilder(client()) + .application("app-2").privileges("read").execute().actionGet().privileges(); + assertNotNull(privileges); + assertEquals(1, privileges.length); + assertEquals("app-2", privileges[0].getApplication()); + assertEquals("read", privileges[0].getName()); + } +} From 4a63a16a5ee3c17670219c087d157d77761d2cf5 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 01:56:50 +1000 Subject: [PATCH 07/45] Minor --- .../xpack/security/authz/store/NativeRolesStoreIntegTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java index 1f9f1ea706426..39424b875a5d9 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java @@ -21,7 +21,6 @@ import java.util.Arrays; import java.util.List; import java.util.Set; -import java.util.concurrent.ExecutionException; import static java.util.Collections.emptyMap; From a1d177cbcf5d99d7cb0f9bf9e2593fed5c27eaa8 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 02:10:30 +1000 Subject: [PATCH 08/45] More tweak --- .../privilege/ClearPrivilegesCacheRequest.java | 16 ++++++++-------- .../TransportClearPrivilegesCacheAction.java | 4 ++-- .../authz/store/NativePrivilegeStore.java | 12 +++++++----- .../privilege/RestClearPrivilegesAction.java | 2 +- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java index be72fc8671ddf..c369e054f998b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java @@ -26,12 +26,12 @@ public ClearPrivilegesCacheRequest(StreamInput in) throws IOException { applicationNames = in.readOptionalStringArray(); } - public ClearPrivilegesCacheRequest names(String... applicationNames) { + public ClearPrivilegesCacheRequest applicationNames(String... applicationNames) { this.applicationNames = applicationNames; return this; } - public String[] names() { + public String[] applicationNames() { return applicationNames; } @@ -42,25 +42,25 @@ public void writeTo(StreamOutput out) throws IOException { } public static class Node extends TransportRequest { - private String[] names; + private String[] applicationNames; public Node(StreamInput in) throws IOException { super(in); - names = in.readOptionalStringArray(); + applicationNames = in.readOptionalStringArray(); } public Node(ClearPrivilegesCacheRequest request) { - this.names = request.names(); + this.applicationNames = request.applicationNames(); } - public String[] getNames() { - return names; + public String[] getApplicationNames() { + return applicationNames; } @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); - out.writeOptionalStringArray(names); + out.writeOptionalStringArray(applicationNames); } } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index 986c73f35b4dc..2232069fc13c1 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -71,10 +71,10 @@ protected ClearPrivilegesCacheResponse.Node newNodeResponse(StreamInput in) thro @Override protected ClearPrivilegesCacheResponse.Node nodeOperation(ClearPrivilegesCacheRequest.Node request, Task task) { - if (request.getNames() == null || request.getNames().length == 0) { + if (request.getApplicationNames() == null || request.getApplicationNames().length == 0) { privilegesStore.invalidateAll(); } else { - privilegesStore.invalidate(Arrays.asList(request.getNames())); + privilegesStore.invalidate(Arrays.asList(request.getApplicationNames())); } rolesStore.invalidateAll(); return new ClearPrivilegesCacheResponse.Node(clusterService.localNode()); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 9bfdab88543e1..67f14ee151b2d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -226,6 +226,7 @@ private void innerGetPrivileges(Collection applications, new ParameterizedMessage("Searching for [{}] privileges with query [{}]", applications, Strings.toString(query))); request.indicesOptions().ignoreUnavailable(); + // TODO: not parsing source of cached entries? ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), hit -> buildPrivilege(hit.getId(), hit.getSourceRef())); } @@ -238,7 +239,8 @@ public void invalidate(Collection updatedApplicationNames) { numInvalidation.incrementAndGet(); // Always completely invalidate application names cache due to wildcard applicationNamesCache.invalidateAll(); - descriptorsCacheHelper.removeKeysIf(updatedApplicationNames::contains); + final Set uniqueNames = Set.copyOf(updatedApplicationNames); + descriptorsCacheHelper.removeKeysIf(uniqueNames::contains); } public void invalidateAll() { @@ -378,7 +380,7 @@ public void putPrivileges(Collection privileges, .map(NativePrivilegeStore::nameFromDocId) .collect(TUPLES_TO_MAP); clearCaches(listener, - privileges.stream().map(ApplicationPrivilegeDescriptor::getApplication).collect(Collectors.toList()), + privileges.stream().map(ApplicationPrivilegeDescriptor::getApplication).collect(Collectors.toUnmodifiableSet()), createdNames); }, listener::onFailure), privileges.size()); for (ApplicationPrivilegeDescriptor privilege : privileges) { @@ -419,7 +421,7 @@ public void deletePrivileges(String application, Collection names, Write .map(r -> r.getId()) .map(NativePrivilegeStore::nameFromDocId) .collect(TUPLES_TO_MAP); - clearCaches(listener, Collections.singletonList(application), deletedNames); + clearCaches(listener, Collections.singleton(application), deletedNames); }, listener::onFailure), names.size()); for (String name : names) { ClientHelper.executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, @@ -431,9 +433,9 @@ public void deletePrivileges(String application, Collection names, Write } } - private void clearCaches(ActionListener listener, List applicationNames, T value) { + private void clearCaches(ActionListener listener, Set applicationNames, T value) { // This currently clears _all_ roles, but could be improved to clear only those roles that reference the affected application - final ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest().names(applicationNames.toArray(String[]::new)); + final ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest().applicationNames(applicationNames.toArray(String[]::new)); executeAsyncWithOrigin(client, SECURITY_ORIGIN, ClearPrivilegesCacheAction.INSTANCE, request, new ActionListener<>() { @Override diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java index 6dfb516c1a09d..00ad706108c29 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java @@ -40,7 +40,7 @@ public List routes() { @Override protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException { String[] applicationNames = request.paramAsStringArrayOrEmptyIfAll("application"); - final ClearPrivilegesCacheRequest req = new ClearPrivilegesCacheRequest().names(applicationNames); + final ClearPrivilegesCacheRequest req = new ClearPrivilegesCacheRequest().applicationNames(applicationNames); return channel -> client.execute(ClearPrivilegesCacheAction.INSTANCE, req, new NodesResponseRestListener<>(channel)); } From 05061f972d863e577d838ac9ee721b9e23b4f68a Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 12:49:36 +1000 Subject: [PATCH 09/45] Add single node tests Rest tests Documents Other tweaks and optimizations --- .../privilege/RestClearPrivilegesAction.java | 4 +- .../store/NativeRolesStoreCacheTests.java | 249 ++++++++++++++++++ .../store/NativeRolesStoreIntegTests.java | 72 ----- ...y.clear_cached_application_privileges.json | 26 ++ 4 files changed, 277 insertions(+), 74 deletions(-) create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreCacheTests.java delete mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java create mode 100644 x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_application_privileges.json diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java index 00ad706108c29..7e46930b36f1a 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java @@ -13,7 +13,7 @@ import org.elasticsearch.rest.action.RestActions.NodesResponseRestListener; import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; -import org.elasticsearch.xpack.security.rest.action.saml.SamlBaseRestHandler; +import org.elasticsearch.xpack.security.rest.action.SecurityBaseRestHandler; import java.io.IOException; import java.util.Collections; @@ -21,7 +21,7 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; -public class RestClearPrivilegesAction extends SamlBaseRestHandler { +public class RestClearPrivilegesAction extends SecurityBaseRestHandler { public RestClearPrivilegesAction(Settings settings, XPackLicenseState licenseState) { super(settings, licenseState); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreCacheTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreCacheTests.java new file mode 100644 index 0000000000000..599f8a438df0d --- /dev/null +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreCacheTests.java @@ -0,0 +1,249 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.security.authz.store; + +import org.elasticsearch.action.ActionFuture; +import org.elasticsearch.action.delete.DeleteRequest; +import org.elasticsearch.client.Client; +import org.elasticsearch.rest.RestStatus; +import org.elasticsearch.test.SecuritySingleNodeTestCase; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; +import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheResponse; +import org.elasticsearch.xpack.core.security.action.privilege.DeletePrivilegesRequestBuilder; +import org.elasticsearch.xpack.core.security.action.privilege.GetPrivilegesRequestBuilder; +import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesAction; +import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesRequest; +import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesResponse; +import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesAction; +import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesRequest; +import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; +import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; +import org.junit.Before; + +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Base64; +import java.util.Collections; +import java.util.List; +import java.util.Set; + +import static java.util.Collections.emptyMap; +import static java.util.Collections.singleton; +import static java.util.Collections.singletonMap; +import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; +import static org.elasticsearch.test.SecuritySettingsSource.TEST_PASSWORD_HASHED; +import static org.elasticsearch.test.SecuritySettingsSource.TEST_ROLE; +import static org.elasticsearch.test.SecuritySettingsSourceField.TEST_PASSWORD; +import static org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor.DOC_TYPE_VALUE; +import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_MAIN_ALIAS; + +public class NativeRolesStoreCacheTests extends SecuritySingleNodeTestCase { + + private static final String APP_USER_NAME = "app_user"; + + @Override + protected String configUsers() { + return super.configUsers() + + APP_USER_NAME + ":" + TEST_PASSWORD_HASHED; + } + + @Override + protected String configRoles() { + return super.configRoles() + + "app_role:\n" + + " cluster: ['monitor']\n" + + " indices:\n" + + " - names: ['*']\n" + + " privileges: ['read']\n" + + " applications:\n" + + " - application: 'app-1'\n" + + " privileges: ['read', 'check']\n" + + " resources: ['foo']\n" + + " - application: 'app-2'\n" + + " privileges: ['check']\n" + + " resources: ['foo']\n"; + } + + @Override + protected String configUsersRoles() { + return super.configUsersRoles() + + "app_role:" + APP_USER_NAME + "\n" + + TEST_ROLE + ":" + APP_USER_NAME + "\n"; + } + + @Before + public void configureApplicationPrivileges() { + final List applicationPrivilegeDescriptors = Arrays.asList( + new ApplicationPrivilegeDescriptor("app-1", "read", Set.of("a:b:c", "x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-1", "write", Set.of("a:b:c", "x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-1", "admin", Set.of("a:b:c", "x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "read", Set.of("e:f:g", "t:u:v"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "write", Set.of("e:f:g", "t:u:v"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "admin", Set.of("e:f:g", "t:u:v"), emptyMap())); + + final PutPrivilegesRequest putPrivilegesRequest = new PutPrivilegesRequest(); + putPrivilegesRequest.setPrivileges(applicationPrivilegeDescriptors); + final ActionFuture future = + client().execute(PutPrivilegesAction.INSTANCE, putPrivilegesRequest); + + final PutPrivilegesResponse putPrivilegesResponse = future.actionGet(); + assertEquals(2, putPrivilegesResponse.created().size()); + assertEquals(6, putPrivilegesResponse.created().values().stream().mapToInt(List::size).sum()); + } + + public void testCacheBehaviourWithGetPrivileges() { + final Client client = client(); + + ApplicationPrivilegeDescriptor[] privileges = new GetPrivilegesRequestBuilder(client) + .application("app-2").privileges("write").execute().actionGet().privileges(); + + assertEquals(1, privileges.length); + assertEquals("app-2", privileges[0].getApplication()); + assertEquals("write", privileges[0].getName()); + + // A hacky way to test cache is populated and used by deleting the backing documents. + // The test will fail if the cache is not in place + assertFalse(client.prepareBulk() + .add(new DeleteRequest(SECURITY_MAIN_ALIAS, DOC_TYPE_VALUE + "_app-2:read")) + .add(new DeleteRequest(SECURITY_MAIN_ALIAS, DOC_TYPE_VALUE + "_app-2:write")) + .add(new DeleteRequest(SECURITY_MAIN_ALIAS, DOC_TYPE_VALUE + "_app-2:admin")) + .setRefreshPolicy(IMMEDIATE).execute().actionGet().hasFailures()); + + // We can still get the privileges because it is cached + privileges = new GetPrivilegesRequestBuilder(client) + .application("app-2").privileges("read").execute().actionGet().privileges(); + + assertEquals(1, privileges.length); + + // We can get all app-2 privileges because cache is keyed by application + privileges = new GetPrivilegesRequestBuilder(client) + .application("app-2").execute().actionGet().privileges(); + + assertEquals(3, privileges.length); + + // Now properly invalidate the cache + final ClearPrivilegesCacheResponse clearPrivilegesCacheResponse = + client.execute(ClearPrivilegesCacheAction.INSTANCE, new ClearPrivilegesCacheRequest()).actionGet(); + assertFalse(clearPrivilegesCacheResponse.hasFailures()); + + // app-2 is no longer found + privileges = new GetPrivilegesRequestBuilder(client) + .application("app-2").privileges("read").execute().actionGet().privileges(); + assertEquals(0, privileges.length); + } + + public void testCacheBehaviourWithWildcard() { + final Client client = client(); + + ApplicationPrivilegeDescriptor[] privileges = new GetPrivilegesRequestBuilder(client) + .execute().actionGet().privileges(); + + assertEquals(6, privileges.length); + + // Delete a privilege properly + deleteApplicationPrivilege("app-2", "read"); + + // A direct read should also get nothing + assertEquals(0, new GetPrivilegesRequestBuilder(client) + .application("app-2").privileges("read").execute().actionGet().privileges().length); + + // The wildcard expression expansion should be invalidated + assertEquals(5, new GetPrivilegesRequestBuilder(client).execute().actionGet().privileges().length); + + // Now put it back and wild expression expansion should be invalidated again + addApplicationPrivilege("app-2", "read", "e:f:g", "t:u:v"); + + assertEquals(6, new GetPrivilegesRequestBuilder(client).execute().actionGet().privileges().length); + + // Delete the privilege again which invalidate the wildcard expansion + deleteApplicationPrivilege("app-2", "read"); + + // The descriptors cache is keyed by application name hence removal of a app-2 privilege only affects + // app-2, but not app-1. The cache hit/miss is tested by removing the backing documents + assertFalse(client.prepareBulk() + .add(new DeleteRequest(SECURITY_MAIN_ALIAS, DOC_TYPE_VALUE + "_app-1:write")) + .add(new DeleteRequest(SECURITY_MAIN_ALIAS, DOC_TYPE_VALUE + "_app-2:write")) + .setRefreshPolicy(IMMEDIATE).execute().actionGet().hasFailures()); + + // app-2 write privilege will not be found since cache is invalidated and backing document is gone + assertEquals(0, new GetPrivilegesRequestBuilder(client) + .application("app-2").privileges("write").execute().actionGet().privileges().length); + + // app-1 write privilege is still found since it is cached even when the backing document is gone + assertEquals(1, new GetPrivilegesRequestBuilder(client) + .application("app-1").privileges("write").execute().actionGet().privileges().length); + } + + public void testCacheBehaviourWithSuffixWildcard() { + final Client client = client(); + + // Populate the cache with suffix wildcard + assertEquals(6, new GetPrivilegesRequestBuilder(client).application("app-*").execute().actionGet().privileges().length); + + // Delete a backing document + assertEquals(RestStatus.OK, client.prepareDelete(SECURITY_MAIN_ALIAS, DOC_TYPE_VALUE + "_app-1:read") + .setRefreshPolicy(IMMEDIATE).execute().actionGet().status()); + + // A direct get privilege with no wildcard should still hit the cache without needing it to be in the names cache + assertEquals(1, new GetPrivilegesRequestBuilder(client).application("app-1") + .privileges("read").execute().actionGet().privileges().length); + } + + public void testCacheBehaviourWithHasPrivileges() { + + assertTrue(checkPrivilege("app-1", "read").getApplicationPrivileges() + .get("app-1").stream().findFirst().orElseThrow().getPrivileges().get("read")); + + assertFalse(checkPrivilege("app-1", "check").getApplicationPrivileges() + .get("app-1").stream().findFirst().orElseThrow().getPrivileges().get("check")); + + // Add the app-1 check privilege and it should be picked up + addApplicationPrivilege("app-1", "check", "a:b:c"); + assertTrue(checkPrivilege("app-1", "check").getApplicationPrivileges() + .get("app-1").stream().findFirst().orElseThrow().getPrivileges().get("check")); + + // Delete the app-1 read privilege and it should be picked up as well + deleteApplicationPrivilege("app-1", "read"); + assertFalse(checkPrivilege("app-1", "read").getApplicationPrivileges() + .get("app-1").stream().findFirst().orElseThrow().getPrivileges().get("read")); + + // TODO: This is a bug + assertTrue(checkPrivilege("app-2", "check").getApplicationPrivileges() + .get("app-2").stream().findFirst().orElseThrow().getPrivileges().get("check")); + } + + private HasPrivilegesResponse checkPrivilege(String applicationName, String privilegeName) { + final Client client = client().filterWithHeader(singletonMap("Authorization", + "Basic " + Base64.getEncoder().encodeToString(("app_user:" + TEST_PASSWORD).getBytes(StandardCharsets.UTF_8)))); + + // Has privileges always loads all privileges for an application + final HasPrivilegesRequest hasPrivilegesRequest = new HasPrivilegesRequest(); + hasPrivilegesRequest.username(APP_USER_NAME); + hasPrivilegesRequest.applicationPrivileges( + RoleDescriptor.ApplicationResourcePrivileges.builder() + .application(applicationName).privileges(privilegeName).resources("foo").build() + ); + hasPrivilegesRequest.clusterPrivileges("monitor"); + hasPrivilegesRequest.indexPrivileges(RoleDescriptor.IndicesPrivileges.builder().indices("*").privileges("read").build()); + return client.execute(HasPrivilegesAction.INSTANCE, hasPrivilegesRequest).actionGet(); + } + + private void addApplicationPrivilege(String applicationName, String privilegeName, String... actions) { + final List applicationPrivilegeDescriptors = Collections.singletonList( + new ApplicationPrivilegeDescriptor(applicationName, privilegeName, Set.of(actions), emptyMap())); + final PutPrivilegesRequest putPrivilegesRequest = new PutPrivilegesRequest(); + putPrivilegesRequest.setPrivileges(applicationPrivilegeDescriptors); + assertEquals(1, client().execute(PutPrivilegesAction.INSTANCE, putPrivilegesRequest).actionGet().created().keySet().size()); + } + + private void deleteApplicationPrivilege(String applicationName, String privilegeName) { + assertEquals(singleton(privilegeName), new DeletePrivilegesRequestBuilder(client()) + .application(applicationName).privileges(new String[] { privilegeName }).execute().actionGet().found()); + } +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java deleted file mode 100644 index 39424b875a5d9..0000000000000 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreIntegTests.java +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -package org.elasticsearch.xpack.security.authz.store; - -import org.elasticsearch.action.ActionFuture; -import org.elasticsearch.test.SecurityIntegTestCase; -import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; -import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; -import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheResponse; -import org.elasticsearch.xpack.core.security.action.privilege.GetPrivilegesRequestBuilder; -import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesAction; -import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesRequest; -import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesResponse; -import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; -import org.junit.Before; - -import java.util.Arrays; -import java.util.List; -import java.util.Set; - -import static java.util.Collections.emptyMap; - -public class NativeRolesStoreIntegTests extends SecurityIntegTestCase { - - @Before - public void configureApplicationPrivileges() { - final List applicationPrivilegeDescriptors = Arrays.asList( - new ApplicationPrivilegeDescriptor("app-1", "read", Set.of("a:b:c", "x:y:z"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-1", "write", Set.of("a:b:c", "x:y:z"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-1", "admin", Set.of("a:b:c", "x:y:z"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-2", "read", Set.of("e:f:g", "t:u:v"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-2", "write", Set.of("e:f:g", "t:u:v"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-2", "admin", Set.of("e:f:g", "t:u:v"), emptyMap())); - - final PutPrivilegesRequest putPrivilegesRequest = new PutPrivilegesRequest(); - putPrivilegesRequest.setPrivileges(applicationPrivilegeDescriptors); - final ActionFuture future = - client().execute(PutPrivilegesAction.INSTANCE, putPrivilegesRequest); - - final PutPrivilegesResponse putPrivilegesResponse = future.actionGet(); - assertEquals(2, putPrivilegesResponse.created().size()); - assertEquals(6, putPrivilegesResponse.created().values().stream().mapToInt(List::size).sum()); - } - - public void testCache() { - - ApplicationPrivilegeDescriptor[] privileges = new GetPrivilegesRequestBuilder(client()) - .application("app-2").privileges("write").execute().actionGet().privileges(); - - assertNotNull(privileges); - assertEquals(1, privileges.length); - assertEquals("app-2", privileges[0].getApplication()); - assertEquals("write", privileges[0].getName()); - - // Invalidate cache - final ClearPrivilegesCacheResponse clearPrivilegesCacheResponse = - client().execute(ClearPrivilegesCacheAction.INSTANCE, new ClearPrivilegesCacheRequest()).actionGet(); - assertEquals(cluster().size(), clearPrivilegesCacheResponse.getNodes().size()); - assertFalse(clearPrivilegesCacheResponse.hasFailures()); - - privileges = new GetPrivilegesRequestBuilder(client()) - .application("app-2").privileges("read").execute().actionGet().privileges(); - assertNotNull(privileges); - assertEquals(1, privileges.length); - assertEquals("app-2", privileges[0].getApplication()); - assertEquals("read", privileges[0].getName()); - } -} diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_application_privileges.json b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_application_privileges.json new file mode 100644 index 0000000000000..11bde32ff239b --- /dev/null +++ b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_application_privileges.json @@ -0,0 +1,26 @@ +{ + "security.clear_cached_roles":{ + "documentation":{ + "url":"https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-clear-role-cache.html", + "description":"Evicts privileges from the native application privileges cache." + }, + "stability":"stable", + "url":{ + "paths":[ + { + "path":"/_security/privilege/{application}/_clear_cache", + "methods":[ + "POST" + ], + "parts":{ + "name":{ + "type":"list", + "description":"application name" + } + } + } + ] + }, + "params":{} + } +} From daafb80a97c35c1f5d1bc3a138409cb016160e73 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 13:29:08 +1000 Subject: [PATCH 10/45] checkstyle --- .../xpack/core/security/support/CacheIteratorHelper.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java index 34aa585f1129e..d564db530c664 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java @@ -10,7 +10,6 @@ import org.elasticsearch.common.util.concurrent.ReleasableLock; import java.util.Iterator; -import java.util.Map; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Predicate; From 545e27c3aef42158a6e296bb6077a9981e4d3589 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 13:39:29 +1000 Subject: [PATCH 11/45] More checkstyle --- .../action/privilege/TransportClearPrivilegesCacheAction.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index 2232069fc13c1..efba362333386 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -25,8 +25,8 @@ import java.util.Arrays; import java.util.List; -public class TransportClearPrivilegesCacheAction - extends TransportNodesAction { +public class TransportClearPrivilegesCacheAction extends TransportNodesAction { private final NativePrivilegeStore privilegesStore; private final CompositeRolesStore rolesStore; From 5987b3bf823e09fe685211395cdb8257de45a4c6 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 14:07:53 +1000 Subject: [PATCH 12/45] checkstyle again --- .../xpack/security/authz/store/NativePrivilegeStore.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 67f14ee151b2d..1a880745c478f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -435,7 +435,8 @@ public void deletePrivileges(String application, Collection names, Write private void clearCaches(ActionListener listener, Set applicationNames, T value) { // This currently clears _all_ roles, but could be improved to clear only those roles that reference the affected application - final ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest().applicationNames(applicationNames.toArray(String[]::new)); + final ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest() + .applicationNames(applicationNames.toArray(String[]::new)); executeAsyncWithOrigin(client, SECURITY_ORIGIN, ClearPrivilegesCacheAction.INSTANCE, request, new ActionListener<>() { @Override From 60941785ac57b06e5ed9e305acd120db48566d32 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 14:09:59 +1000 Subject: [PATCH 13/45] Fix json API file --- ...on_privileges.json => security.clear_cached_privileges.json} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename x-pack/plugin/src/test/resources/rest-api-spec/api/{security.clear_cached_application_privileges.json => security.clear_cached_privileges.json} (93%) diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_application_privileges.json b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json similarity index 93% rename from x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_application_privileges.json rename to x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json index 11bde32ff239b..c866e186d242c 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_application_privileges.json +++ b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json @@ -1,5 +1,5 @@ { - "security.clear_cached_roles":{ + "security.clear_cached_privileges":{ "documentation":{ "url":"https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-clear-role-cache.html", "description":"Evicts privileges from the native application privileges cache." From 1c58309b87b38718e10825eb15b9344ae9b38912 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 28 Apr 2020 14:27:51 +1000 Subject: [PATCH 14/45] Update API json file --- .../rest-api-spec/api/security.clear_cached_privileges.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json index c866e186d242c..2b21692bb978d 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json +++ b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json @@ -13,9 +13,9 @@ "POST" ], "parts":{ - "name":{ - "type":"list", - "description":"application name" + "application":{ + "type":"string", + "description":"Application name" } } } From 9100c24ceca158023ef2059ef0a16fea9c57f91c Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 30 Apr 2020 12:39:32 +1000 Subject: [PATCH 15/45] WIP --- .../xpack/security/authz/store/NativePrivilegeStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 1a880745c478f..11623bc92ee71 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -134,7 +134,7 @@ public void getPrivileges(Collection applications, Collection na // TODO: We should have a way to express true Zero applications final Set applicationNamesCacheKey = (isEmpty(applications) || applications.contains("*")) ? - Collections.singleton("*") : Set.copyOf(applications); + Set.of("*") : Set.copyOf(applications); // Always fetch for the concrete application names even when the passed in application names do not // contain any wildcard. This serves as a negative lookup. From 3f9c5b7688845a6b301d683967d3d77207da35cb Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 30 Apr 2020 16:24:10 +1000 Subject: [PATCH 16/45] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java Co-Authored-By: Tim Vernum --- .../xpack/security/authz/store/NativePrivilegeStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 1a880745c478f..92d3430f8ea86 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -240,7 +240,7 @@ public void invalidate(Collection updatedApplicationNames) { // Always completely invalidate application names cache due to wildcard applicationNamesCache.invalidateAll(); final Set uniqueNames = Set.copyOf(updatedApplicationNames); - descriptorsCacheHelper.removeKeysIf(uniqueNames::contains); + uniqueNames.forEach(descriptorsCache::invalidate); } public void invalidateAll() { From b9538a8692484fe98164a5ee465ffa99076a5ecd Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 30 Apr 2020 16:32:18 +1000 Subject: [PATCH 17/45] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java Co-Authored-By: Tim Vernum --- .../action/privilege/TransportClearPrivilegesCacheAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index efba362333386..89ecdea8942a1 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -74,7 +74,7 @@ protected ClearPrivilegesCacheResponse.Node nodeOperation(ClearPrivilegesCacheRe if (request.getApplicationNames() == null || request.getApplicationNames().length == 0) { privilegesStore.invalidateAll(); } else { - privilegesStore.invalidate(Arrays.asList(request.getApplicationNames())); + privilegesStore.invalidate(List.of(request.getApplicationNames())); } rolesStore.invalidateAll(); return new ClearPrivilegesCacheResponse.Node(clusterService.localNode()); From f5b166c9d734f2d62fa0a485a776d9bdc742d0bb Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 30 Apr 2020 16:40:25 +1000 Subject: [PATCH 18/45] WIP --- .../core/security/support/CacheIteratorHelper.java | 13 ------------- .../TransportClearPrivilegesCacheAction.java | 1 - .../security/authz/store/NativePrivilegeStore.java | 3 +-- 3 files changed, 1 insertion(+), 16 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java index d564db530c664..0dfdab26815a5 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java @@ -56,17 +56,4 @@ public void removeKeysIf(Predicate removeIf) { } } } - - public void removeValuesIf(Predicate removeIf) { - // the cache cannot be modified while doing this operation per the terms of the cache iterator - try (ReleasableLock ignored = this.acquireForIterator()) { - Iterator iterator = cache.keys().iterator(); - while (iterator.hasNext()) { - K key = iterator.next(); - if (removeIf.test(cache.get(key))) { - iterator.remove(); - } - } - } - } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index 89ecdea8942a1..8f1d7f6d3a0a7 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -22,7 +22,6 @@ import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore; import java.io.IOException; -import java.util.Arrays; import java.util.List; public class TransportClearPrivilegesCacheAction extends TransportNodesAction applications, Collection na if (concreteApplicationNames != null && concreteApplicationNames.size() == 0) { logger.debug("returning empty application privileges as application names result in empty list"); listener.onResponse(Collections.emptySet()); - } else { final Tuple, Map>> cacheStatus; if (concreteApplicationNames == null) { @@ -240,7 +239,7 @@ public void invalidate(Collection updatedApplicationNames) { // Always completely invalidate application names cache due to wildcard applicationNamesCache.invalidateAll(); final Set uniqueNames = Set.copyOf(updatedApplicationNames); - uniqueNames.forEach(descriptorsCache::invalidate); + uniqueNames.forEach(descriptorsCache::invalidate); } public void invalidateAll() { From f1f3e728644dc5e07eaccbef2f341fb6d368bb7d Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 30 Apr 2020 22:26:16 +1000 Subject: [PATCH 19/45] Address feedback --- .../authz/store/NativePrivilegeStore.java | 43 ++++++++----------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 37f77fca1d883..f556f43767a36 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -101,9 +101,7 @@ public class NativePrivilegeStore { private final Client client; private final SecurityIndexManager securityIndexManager; private final Cache> descriptorsCache; - private final CacheIteratorHelper> descriptorsCacheHelper; private final Cache, Set> applicationNamesCache; - private final CacheIteratorHelper, Set> applicationNamesCacheHelper; private final AtomicLong numInvalidation = new AtomicLong(); public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManager securityIndexManager) { @@ -117,7 +115,6 @@ public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManag builder.weigher((k, v) -> v.size()); } descriptorsCache = builder.build(); - descriptorsCacheHelper = new CacheIteratorHelper<>(descriptorsCache); CacheBuilder, Set> applicationNamesCacheBuilder = CacheBuilder.builder(); final int nameCacheSize = APPLICATION_NAME_CACHE_SIZE_SETTING.get(settings); @@ -126,7 +123,6 @@ public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManag applicationNamesCacheBuilder.weigher((k, v) -> k.size() + v.size()); } applicationNamesCache = applicationNamesCacheBuilder.build(); - applicationNamesCacheHelper = new CacheIteratorHelper<>(applicationNamesCache); } public void getPrivileges(Collection applications, Collection names, @@ -150,45 +146,42 @@ public void getPrivileges(Collection applications, Collection na } else { cacheStatus = cacheStatusForApplicationNames(concreteApplicationNames); } + Set uncachedApplicationNames = cacheStatus.v1(); + Map> cachedDescriptors = cacheStatus.v2(); - if (cacheStatus.v1().isEmpty()) { + if (uncachedApplicationNames.isEmpty()) { logger.debug("All application privileges found in cache"); - final Set cachedDescriptors = - cacheStatus.v2().values().stream().flatMap(Collection::stream).collect(Collectors.toUnmodifiableSet()); - listener.onResponse(filterDescriptorsForNames(cachedDescriptors, names)); + listener.onResponse(filterDescriptorsForNames( + cachedDescriptors.values().stream().flatMap(Collection::stream).collect(Collectors.toUnmodifiableSet()), names)); } else { final long invalidationCounter = numInvalidation.get(); // Always fetch all privileges of an application for caching purpose - logger.debug("Fetching application privilege documents for: {}", cacheStatus.v1()); - innerGetPrivileges(cacheStatus.v1(), ActionListener.wrap(fetchedDescriptors -> { + logger.debug("Fetching application privilege documents for: {}", uncachedApplicationNames); + innerGetPrivileges(uncachedApplicationNames, ActionListener.wrap(fetchedDescriptors -> { final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() .collect(Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); final Set allApplicationNames = - Stream.concat(cacheStatus.v2().keySet().stream(), mapOfFetchedDescriptors.keySet().stream()) + Stream.concat(cachedDescriptors.keySet().stream(), mapOfFetchedDescriptors.keySet().stream()) .collect(Collectors.toUnmodifiableSet()); // Avoid caching potential stale results. // TODO: It is still possible that cache gets invalidated immediately after the if check if (invalidationCounter == numInvalidation.get()) { // Do not cache the names if expansion has no effect if (allApplicationNames.equals(applicationNamesCacheKey) == false) { - try (ReleasableLock ignored = applicationNamesCacheHelper.acquireUpdateLock()) { - applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> { - logger.debug("Caching application names query: {} = {}", k, allApplicationNames); - return allApplicationNames; - }); - } + applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> { + logger.debug("Caching application names query: {} = {}", k, allApplicationNames); + return allApplicationNames; + }); } - try (ReleasableLock ignored = descriptorsCacheHelper.acquireUpdateLock()) { - for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { - descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> { - logger.debug("Caching descriptors for application: {}", k); - return entry.getValue(); - }); - } + for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { + descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> { + logger.debug("Caching descriptors for application: {}", k); + return entry.getValue(); + }); } } final Set allDescriptors = - Stream.concat(cacheStatus.v2().values().stream().flatMap(Collection::stream), fetchedDescriptors.stream()) + Stream.concat(cachedDescriptors.values().stream().flatMap(Collection::stream), fetchedDescriptors.stream()) .collect(Collectors.toUnmodifiableSet()); listener.onResponse(filterDescriptorsForNames(allDescriptors, names)); }, listener::onFailure)); From a7b5b409e6e9fb4a4a88a4ebbbb1d6f1f49a877a Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 30 Apr 2020 22:36:57 +1000 Subject: [PATCH 20/45] Checkstyle --- .../xpack/security/authz/store/NativePrivilegeStore.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index f556f43767a36..60bab65f6b43d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -29,7 +29,6 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.CollectionUtils; -import org.elasticsearch.common.util.concurrent.ReleasableLock; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -48,7 +47,6 @@ import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest; import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheResponse; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; -import org.elasticsearch.xpack.core.security.support.CacheIteratorHelper; import org.elasticsearch.xpack.security.support.SecurityIndexManager; import java.io.IOException; From 5306bc13d3f368cf7ed26a4c4a12b3bac09a9eab Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 4 May 2020 11:37:54 +1000 Subject: [PATCH 21/45] Address feedback --- .../security/clear-privileges-cache.asciidoc | 42 ++++++ .../security/delete-app-privileges.asciidoc | 8 +- .../ClearPrivilegesCacheRequest.java | 22 ++- .../TransportClearPrivilegesCacheAction.java | 4 +- .../authz/store/NativePrivilegeStore.java | 142 +++++------------- ...va => NativePrivilegeStoreCacheTests.java} | 68 ++++++++- .../store/NativePrivilegeStoreTests.java | 104 ++++++------- .../api/security.clear_cached_privileges.json | 4 +- .../test/privileges/10_basic.yml | 10 ++ 9 files changed, 230 insertions(+), 174 deletions(-) create mode 100644 x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc rename x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/{NativeRolesStoreCacheTests.java => NativePrivilegeStoreCacheTests.java} (78%) diff --git a/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc b/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc new file mode 100644 index 0000000000000..579596c6415d7 --- /dev/null +++ b/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc @@ -0,0 +1,42 @@ +[role="xpack"] +[[security-api-clear-privilege-cache]] +=== Clear application privileges cache API +++++ +Clear privileges cache +++++ + +Evicts application privileges from the native application privilege cache. + +[[security-api-clear-privilege-cache-request]] +==== {api-request-title} + +`POST /_security/privilege//_clear_cache` + +[[security-api-clear-privilege-cache-prereqs]] +==== {api-prereq-title} + +* To use this API, you must have at least the `manage_security` cluster +privilege. + +[[security-api-clear-privilege-cache-desc]] +==== {api-description-title} + +For more information about the native realm, see +<> and <>. + +[[security-api-clear-privilege-cache-path-params]] +==== {api-path-parms-title} + +`application`:: + (string) The name of the application. If omitted, all entries are evicted from the cache. + +[[security-api-clear-privilege-cache-example]] +==== {api-examples-title} + +The clear privileges cache API evicts application privileges from the native application privilege cache. +For example, to clear the cache for `myapp`: + +[source,console] +-------------------------------------------------- +POST /_security/privilege/myapp/_clear_cache +-------------------------------------------------- diff --git a/x-pack/docs/en/rest-api/security/delete-app-privileges.asciidoc b/x-pack/docs/en/rest-api/security/delete-app-privileges.asciidoc index 0ff3ecc8b4aec..39ac1706c6dc2 100644 --- a/x-pack/docs/en/rest-api/security/delete-app-privileges.asciidoc +++ b/x-pack/docs/en/rest-api/security/delete-app-privileges.asciidoc @@ -10,7 +10,7 @@ Removes <>. [[security-api-delete-privilege-request]] ==== {api-request-title} -`DELETE /_security/privilege//` +`DELETE /_security/privilege//` [[security-api-delete-privilege-prereqs]] ==== {api-prereq-title} @@ -34,16 +34,16 @@ To use this API, you must have either: [[security-api-delete-privilege-example]] ==== {api-examples-title} -The following example deletes the `read` application privilege from the +The following example deletes the `read` application privilege from the `myapp` application: [source,console] -------------------------------------------------- DELETE /_security/privilege/myapp/read -------------------------------------------------- -// TEST[setup:app0102_privileges] +// TEST[setup:app0102_privileges] -If the role is successfully deleted, the request returns `{"found": true}`. +If the privilege is successfully deleted, the request returns `{"found": true}`. Otherwise, `found` is set to false. [source,console-result] diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java index c369e054f998b..96e6cafffe403 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java @@ -15,7 +15,8 @@ public class ClearPrivilegesCacheRequest extends BaseNodesRequest { - String[] applicationNames; + private String[] applicationNames; + private boolean clearRolesCache = false; public ClearPrivilegesCacheRequest() { super((String[]) null); @@ -24,6 +25,7 @@ public ClearPrivilegesCacheRequest() { public ClearPrivilegesCacheRequest(StreamInput in) throws IOException { super(in); applicationNames = in.readOptionalStringArray(); + clearRolesCache = in.readBoolean(); } public ClearPrivilegesCacheRequest applicationNames(String... applicationNames) { @@ -31,36 +33,54 @@ public ClearPrivilegesCacheRequest applicationNames(String... applicationNames) return this; } + public ClearPrivilegesCacheRequest clearRolesCache(boolean clearRolesCache) { + this.clearRolesCache = clearRolesCache; + return this; + } + public String[] applicationNames() { return applicationNames; } + public boolean isClearRolesCache() { + return clearRolesCache; + } + @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); out.writeOptionalStringArray(applicationNames); + out.writeBoolean(clearRolesCache); } public static class Node extends TransportRequest { private String[] applicationNames; + private boolean clearRolesCache; public Node(StreamInput in) throws IOException { super(in); applicationNames = in.readOptionalStringArray(); + clearRolesCache = in.readBoolean(); } public Node(ClearPrivilegesCacheRequest request) { this.applicationNames = request.applicationNames(); + this.clearRolesCache = request.clearRolesCache; } public String[] getApplicationNames() { return applicationNames; } + public boolean isClearRolesCache() { + return clearRolesCache; + } + @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); out.writeOptionalStringArray(applicationNames); + out.writeBoolean(clearRolesCache); } } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index 8f1d7f6d3a0a7..db89c9f40f462 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -75,7 +75,9 @@ protected ClearPrivilegesCacheResponse.Node nodeOperation(ClearPrivilegesCacheRe } else { privilegesStore.invalidate(List.of(request.getApplicationNames())); } - rolesStore.invalidateAll(); + if (request.isClearRolesCache()) { + rolesStore.invalidateAll(); + } return new ClearPrivilegesCacheResponse.Node(clusterService.localNode()); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 60bab65f6b43d..abe54e9a06fa2 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -12,12 +12,10 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.DocWriteResponse; import org.elasticsearch.action.delete.DeleteResponse; -import org.elasticsearch.action.get.GetResponse; import org.elasticsearch.action.index.IndexResponse; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.support.ContextPreservingActionListener; import org.elasticsearch.action.support.GroupedActionListener; -import org.elasticsearch.action.support.TransportActions; import org.elasticsearch.action.support.WriteRequest; import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; @@ -53,8 +51,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -63,7 +61,6 @@ import java.util.function.Supplier; import java.util.stream.Collector; import java.util.stream.Collectors; -import java.util.stream.Stream; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING; @@ -128,60 +125,43 @@ public void getPrivileges(Collection applications, Collection na // TODO: We should have a way to express true Zero applications final Set applicationNamesCacheKey = (isEmpty(applications) || applications.contains("*")) ? - Set.of("*") : Set.copyOf(applications); + Set.of("*") : new LinkedHashSet<>(applications); - // Always fetch for the concrete application names even when the passed in application names do not - // contain any wildcard. This serves as a negative lookup. + // Always fetch for the concrete application names even when the passed-in application names has no wildcard. + // This serves as a negative lookup, i.e. when a passed-in non-wildcard application does not exist. Set concreteApplicationNames = applicationNamesCache.get(applicationNamesCacheKey); if (concreteApplicationNames != null && concreteApplicationNames.size() == 0) { logger.debug("returning empty application privileges as application names result in empty list"); listener.onResponse(Collections.emptySet()); } else { - final Tuple, Map>> cacheStatus; - if (concreteApplicationNames == null) { - cacheStatus = cacheStatusForApplicationNames(applicationNamesCacheKey); - } else { - cacheStatus = cacheStatusForApplicationNames(concreteApplicationNames); - } - Set uncachedApplicationNames = cacheStatus.v1(); - Map> cachedDescriptors = cacheStatus.v2(); - - if (uncachedApplicationNames.isEmpty()) { + final Set cachedDescriptors = cachedDescriptorsForApplicationNames( + concreteApplicationNames != null ? concreteApplicationNames : applicationNamesCacheKey); + if (cachedDescriptors != null) { logger.debug("All application privileges found in cache"); - listener.onResponse(filterDescriptorsForNames( - cachedDescriptors.values().stream().flatMap(Collection::stream).collect(Collectors.toUnmodifiableSet()), names)); + listener.onResponse(filterDescriptorsForPrivilegeNames(cachedDescriptors, names)); } else { final long invalidationCounter = numInvalidation.get(); // Always fetch all privileges of an application for caching purpose - logger.debug("Fetching application privilege documents for: {}", uncachedApplicationNames); - innerGetPrivileges(uncachedApplicationNames, ActionListener.wrap(fetchedDescriptors -> { + logger.debug("Fetching application privilege documents for: {}", applicationNamesCacheKey); + innerGetPrivileges(applicationNamesCacheKey, ActionListener.wrap(fetchedDescriptors -> { final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() .collect(Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); - final Set allApplicationNames = - Stream.concat(cachedDescriptors.keySet().stream(), mapOfFetchedDescriptors.keySet().stream()) - .collect(Collectors.toUnmodifiableSet()); // Avoid caching potential stale results. // TODO: It is still possible that cache gets invalidated immediately after the if check if (invalidationCounter == numInvalidation.get()) { + final Set fetchedApplicationNames = Collections.unmodifiableSet(mapOfFetchedDescriptors.keySet()); // Do not cache the names if expansion has no effect - if (allApplicationNames.equals(applicationNamesCacheKey) == false) { - applicationNamesCache.computeIfAbsent(applicationNamesCacheKey, (k) -> { - logger.debug("Caching application names query: {} = {}", k, allApplicationNames); - return allApplicationNames; - }); + if (fetchedApplicationNames.equals(applicationNamesCacheKey) == false) { + logger.debug("Caching application names query: {} = {}", applicationNamesCacheKey, fetchedApplicationNames); + applicationNamesCache.put(applicationNamesCacheKey, fetchedApplicationNames); } for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { - descriptorsCache.computeIfAbsent(entry.getKey(), (k) -> { - logger.debug("Caching descriptors for application: {}", k); - return entry.getValue(); - }); + logger.debug("Caching descriptors for application: {}", entry.getKey()); + descriptorsCache.put(entry.getKey(), entry.getValue()); } } - final Set allDescriptors = - Stream.concat(cachedDescriptors.values().stream().flatMap(Collection::stream), fetchedDescriptors.stream()) - .collect(Collectors.toUnmodifiableSet()); - listener.onResponse(filterDescriptorsForNames(allDescriptors, names)); + listener.onResponse(filterDescriptorsForPrivilegeNames(fetchedDescriptors, names)); }, listener::onFailure)); } } @@ -240,50 +220,41 @@ public void invalidateAll() { descriptorsCache.invalidateAll(); } - private Tuple, Map>> cacheStatusForApplicationNames( - Set applicationNames) { - - final Set uncachedApplicationNames = new HashSet<>(); - final Map> cachedDescriptors = new HashMap<>(); + /** + * Try resolve all privileges for given application names from the cache. + * It returns non-null result only when privileges of ALL applications are + * found in the cache, i.e. it returns null if any of application name is + * NOT found in the cache. Since the cached is keyed by concrete application + * name, this means any wildcard will result in null. + */ + private Set cachedDescriptorsForApplicationNames(Set applicationNames) { + final Set cachedDescriptors = new HashSet<>(); for (String applicationName: applicationNames) { if (applicationName.endsWith("*")) { - uncachedApplicationNames.add(applicationName); + return null; } else { final Set descriptors = descriptorsCache.get(applicationName); if (descriptors == null) { - uncachedApplicationNames.add(applicationName); + return null; } else { - cachedDescriptors.put(applicationName, descriptors); + cachedDescriptors.addAll(descriptors); } } } - if (cachedDescriptors.isEmpty() == false) { - logger.debug("Application privileges found in cache: {}", cachedDescriptors.keySet()); - } - return Tuple.tuple(uncachedApplicationNames, cachedDescriptors); + return Collections.unmodifiableSet(cachedDescriptors); } - private Collection filterDescriptorsForNames( - Collection descriptors, Collection names) { + /** + * Filter to get all privilege descriptors that have any of the given privilege names. + */ + private Collection filterDescriptorsForPrivilegeNames( + Collection descriptors, Collection privilegeNames) { // empty set of names equals to retrieve everything - if (isEmpty(names)) { + if (isEmpty(privilegeNames)) { return descriptors; } - final Set uniqueNameSuffix = new HashSet<>(names); - return descriptors.stream().filter(d -> uniqueNameSuffix.contains(d.getName())).collect(Collectors.toUnmodifiableSet()); - } - - private boolean isSinglePrivilegeMatch(Collection applications) { - return applications != null && applications.size() == 1 && hasWildcard(applications) == false; - } - - private boolean hasWildcard(Collection applications) { - return applications.stream().anyMatch(n -> n.endsWith("*")); - } - - private QueryBuilder getPrivilegeNameQuery(Collection names) { - return QueryBuilders.termsQuery(ApplicationPrivilegeDescriptor.Fields.NAME.getPreferredName(), names); + return descriptors.stream().filter(d -> privilegeNames.contains(d.getName())).collect(Collectors.toUnmodifiableSet()); } private QueryBuilder getApplicationNameQuery(Collection applications) { @@ -322,43 +293,6 @@ private static boolean isEmpty(Collection collection) { return collection == null || collection.isEmpty(); } - void getPrivilege(String application, String name, ActionListener listener) { - final SecurityIndexManager frozenSecurityIndex = securityIndexManager.freeze(); - if (frozenSecurityIndex.isAvailable() == false) { - logger.warn(new ParameterizedMessage("failed to load privilege [{}] index not available", name), - frozenSecurityIndex.getUnavailableReason()); - listener.onResponse(null); - } else { - securityIndexManager.checkIndexVersionThenExecute(listener::onFailure, - () -> executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, - client.prepareGet(SECURITY_MAIN_ALIAS, toDocId(application, name)) - .request(), - new ActionListener() { - @Override - public void onResponse(GetResponse response) { - if (response.isExists()) { - listener.onResponse(buildPrivilege(response.getId(), response.getSourceAsBytesRef())); - } else { - listener.onResponse(null); - } - } - - @Override - public void onFailure(Exception e) { - // if the index or the shard is not there / available we just claim the privilege is not there - if (TransportActions.isShardNotAvailableException(e)) { - logger.warn(new ParameterizedMessage("failed to load privilege [{}] index not available", name), e); - listener.onResponse(null); - } else { - logger.error(new ParameterizedMessage("failed to load privilege [{}]", name), e); - listener.onFailure(e); - } - } - }, - client::get)); - } - } - public void putPrivileges(Collection privileges, WriteRequest.RefreshPolicy refreshPolicy, ActionListener>> listener) { securityIndexManager.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { @@ -426,7 +360,7 @@ public void deletePrivileges(String application, Collection names, Write private void clearCaches(ActionListener listener, Set applicationNames, T value) { // This currently clears _all_ roles, but could be improved to clear only those roles that reference the affected application final ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest() - .applicationNames(applicationNames.toArray(String[]::new)); + .applicationNames(applicationNames.toArray(String[]::new)).clearRolesCache(true); executeAsyncWithOrigin(client, SECURITY_ORIGIN, ClearPrivilegesCacheAction.INSTANCE, request, new ActionListener<>() { @Override diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreCacheTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java similarity index 78% rename from x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreCacheTests.java rename to x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java index 599f8a438df0d..99cf505a41b58 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreCacheTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java @@ -6,9 +6,15 @@ package org.elasticsearch.xpack.security.authz.store; +import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.ActionFuture; +import org.elasticsearch.action.DocWriteResponse; +import org.elasticsearch.action.admin.cluster.health.ClusterHealthAction; +import org.elasticsearch.action.admin.cluster.health.ClusterHealthRequestBuilder; import org.elasticsearch.action.delete.DeleteRequest; +import org.elasticsearch.action.delete.DeleteResponse; import org.elasticsearch.client.Client; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.SecuritySingleNodeTestCase; import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; @@ -19,9 +25,14 @@ import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesAction; import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesRequest; import org.elasticsearch.xpack.core.security.action.privilege.PutPrivilegesResponse; +import org.elasticsearch.xpack.core.security.action.role.PutRoleRequestBuilder; +import org.elasticsearch.xpack.core.security.action.role.PutRoleResponse; import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesAction; import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesRequest; import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse; +import org.elasticsearch.xpack.core.security.action.user.PutUserRequestBuilder; +import org.elasticsearch.xpack.core.security.action.user.PutUserResponse; +import org.elasticsearch.xpack.core.security.authc.support.Hasher; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; import org.junit.Before; @@ -43,14 +54,14 @@ import static org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor.DOC_TYPE_VALUE; import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_MAIN_ALIAS; -public class NativeRolesStoreCacheTests extends SecuritySingleNodeTestCase { +public class NativePrivilegeStoreCacheTests extends SecuritySingleNodeTestCase { private static final String APP_USER_NAME = "app_user"; @Override protected String configUsers() { return super.configUsers() - + APP_USER_NAME + ":" + TEST_PASSWORD_HASHED; + + APP_USER_NAME + ":" + TEST_PASSWORD_HASHED + "\n"; } @Override @@ -97,7 +108,7 @@ public void configureApplicationPrivileges() { assertEquals(6, putPrivilegesResponse.created().values().stream().mapToInt(List::size).sum()); } - public void testCacheBehaviourWithGetPrivileges() { + public void testGetPrivileges() { final Client client = client(); ApplicationPrivilegeDescriptor[] privileges = new GetPrivilegesRequestBuilder(client) @@ -138,7 +149,7 @@ public void testCacheBehaviourWithGetPrivileges() { assertEquals(0, privileges.length); } - public void testCacheBehaviourWithWildcard() { + public void testWildcard() { final Client client = client(); ApplicationPrivilegeDescriptor[] privileges = new GetPrivilegesRequestBuilder(client) @@ -180,7 +191,7 @@ public void testCacheBehaviourWithWildcard() { .application("app-1").privileges("write").execute().actionGet().privileges().length); } - public void testCacheBehaviourWithSuffixWildcard() { + public void testSuffixWildcard() { final Client client = client(); // Populate the cache with suffix wildcard @@ -195,8 +206,7 @@ public void testCacheBehaviourWithSuffixWildcard() { .privileges("read").execute().actionGet().privileges().length); } - public void testCacheBehaviourWithHasPrivileges() { - + public void testHasPrivileges() { assertTrue(checkPrivilege("app-1", "read").getApplicationPrivileges() .get("app-1").stream().findFirst().orElseThrow().getPrivileges().get("read")); @@ -218,6 +228,50 @@ public void testCacheBehaviourWithHasPrivileges() { .get("app-2").stream().findFirst().orElseThrow().getPrivileges().get("check")); } + public void testRolesCacheIsClearedWhenPrivilegesIsChanged() { + final Client client = client(); + + // Add a new user and role so they do not interfere existing tests + final String testRole = "test_role_cache_role"; + final String testRoleCacheUser = "test_role_cache_user"; + final PutRoleResponse putRoleResponse = new PutRoleRequestBuilder(client).name(testRole). + cluster("all") + .addIndices(new String[] { "*" }, new String[] { "read" }, null, null, null, false) + .get(); + assertTrue(putRoleResponse.isCreated()); + + final PutUserResponse putUserResponse = new PutUserRequestBuilder(client) + .username(testRoleCacheUser) + .roles(testRole) + .password(new SecureString("password".toCharArray()), + Hasher.resolve(randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt9", "bcrypt8", "bcrypt"))) + .get(); + assertTrue(putUserResponse.created()); + + // The created user can access cluster health because its role grants access + final Client testRoleCacheUserClient = client.filterWithHeader(singletonMap("Authorization", + "Basic " + Base64.getEncoder().encodeToString((testRoleCacheUser + ":password").getBytes(StandardCharsets.UTF_8)))); + new ClusterHealthRequestBuilder(testRoleCacheUserClient, ClusterHealthAction.INSTANCE).get(); + + // Directly deleted the role document + final DeleteResponse deleteResponse = client.prepareDelete(SECURITY_MAIN_ALIAS, "role-" + testRole).get(); + assertEquals(DocWriteResponse.Result.DELETED, deleteResponse.getResult()); + + // The cluster health action can still success since the role is cached + new ClusterHealthRequestBuilder(testRoleCacheUserClient, ClusterHealthAction.INSTANCE).get(); + + // Change an application privilege which triggers role cache invalidation as well + if (randomBoolean()) { + deleteApplicationPrivilege("app-1", "read"); + } else { + addApplicationPrivilege("app-3", "read", "t:u:v"); + } + // Since role cache is cleared, the cluster health action is no longer authorized + expectThrows(ElasticsearchSecurityException.class, + () -> new ClusterHealthRequestBuilder(testRoleCacheUserClient, ClusterHealthAction.INSTANCE).get()); + + } + private HasPrivilegesResponse checkPrivilege(String applicationName, String privilegeName) { final Client client = client().filterWithHeader(singletonMap("Authorization", "Basic " + Base64.getEncoder().encodeToString(("app_user:" + TEST_PASSWORD).getBytes(StandardCharsets.UTF_8)))); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index c25851ad375d0..17ee6008c3951 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -12,8 +12,6 @@ import org.elasticsearch.action.ActionType; import org.elasticsearch.action.delete.DeleteRequest; import org.elasticsearch.action.delete.DeleteResponse; -import org.elasticsearch.action.get.GetRequest; -import org.elasticsearch.action.get.GetResponse; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.index.IndexResponse; import org.elasticsearch.action.search.SearchRequest; @@ -29,7 +27,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentType; -import org.elasticsearch.index.get.GetResult; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.SearchHits; @@ -39,7 +36,6 @@ import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; import org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames; import org.elasticsearch.xpack.security.support.SecurityIndexManager; -import org.hamcrest.Matchers; import org.junit.After; import org.junit.Before; import org.mockito.Mockito; @@ -62,7 +58,6 @@ import static java.util.Collections.singleton; import static java.util.Collections.singletonList; import static org.elasticsearch.common.util.set.Sets.newHashSet; -import static org.elasticsearch.index.seqno.SequenceNumbers.UNASSIGNED_SEQ_NO; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -120,42 +115,27 @@ public void cleanup() { } public void testGetSinglePrivilegeByName() throws Exception { - final ApplicationPrivilegeDescriptor sourcePrivilege = new ApplicationPrivilegeDescriptor("myapp", "admin", - newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap() - ); - - final PlainActionFuture future = new PlainActionFuture<>(); - store.getPrivilege("myapp", "admin", future); - assertThat(requests, iterableWithSize(1)); - assertThat(requests.get(0), instanceOf(GetRequest.class)); - GetRequest request = (GetRequest) requests.get(0); - assertThat(request.index(), equalTo(RestrictedIndicesNames.SECURITY_MAIN_ALIAS)); - assertThat(request.id(), equalTo("application-privilege_myapp:admin")); - - final String docSource = Strings.toString(sourcePrivilege); - listener.get().onResponse(new GetResponse( - new GetResult(request.index(), request.id(), 0, 1, 1L, true, - new BytesArray(docSource), emptyMap(), emptyMap()) + final List sourcePrivileges = List.of( + new ApplicationPrivilegeDescriptor("myapp", "admin", + newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap() )); - final ApplicationPrivilegeDescriptor getPrivilege = future.get(1, TimeUnit.SECONDS); - assertThat(getPrivilege, equalTo(sourcePrivilege)); - } - public void testGetMissingPrivilege() throws Exception { - final PlainActionFuture future = new PlainActionFuture<>(); - store.getPrivilege("myapp", "admin", future); + final PlainActionFuture> future = new PlainActionFuture<>(); + store.getPrivileges(List.of("myapp"), List.of("admin"), future); assertThat(requests, iterableWithSize(1)); - assertThat(requests.get(0), instanceOf(GetRequest.class)); - GetRequest request = (GetRequest) requests.get(0); - assertThat(request.index(), equalTo(RestrictedIndicesNames.SECURITY_MAIN_ALIAS)); - assertThat(request.id(), equalTo("application-privilege_myapp:admin")); - - listener.get().onResponse(new GetResponse( - new GetResult(request.index(), request.id(), UNASSIGNED_SEQ_NO, 0, -1, - false, null, emptyMap(), emptyMap()) - )); - final ApplicationPrivilegeDescriptor getPrivilege = future.get(1, TimeUnit.SECONDS); - assertThat(getPrivilege, Matchers.nullValue()); + assertThat(requests.get(0), instanceOf(SearchRequest.class)); + SearchRequest request = (SearchRequest) requests.get(0); + final String query = Strings.toString(request.source().query()); + assertThat(query, containsString("{\"terms\":{\"application\":[\"myapp\"]")); + assertThat(query, containsString("{\"term\":{\"type\":{\"value\":\"application-privilege\"")); + + final SearchHit[] hits = buildHits(sourcePrivileges); + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null)); + + assertResult(sourcePrivileges, future); } public void testGetPrivilegesByApplicationName() throws Exception { @@ -259,7 +239,7 @@ public void testGetPrivilegesCacheByApplicationNames() throws Exception { ); final PlainActionFuture> future = new PlainActionFuture<>(); - store.getPrivileges(Arrays.asList("myapp", "yourapp"), null, future); + store.getPrivileges(List.of("myapp", "yourapp"), null, future); final SearchHit[] hits = buildHits(sourcePrivileges); listener.get().onResponse(new SearchResponse(new SearchResponseSections( @@ -273,13 +253,13 @@ public void testGetPrivilegesCacheByApplicationNames() throws Exception { // The 2nd call should use cache and success final PlainActionFuture> future2 = new PlainActionFuture<>(); - store.getPrivileges(Arrays.asList("myapp", "yourapp"), null, future2); + store.getPrivileges(List.of("myapp", "yourapp"), null, future2); listener.get().onResponse(null); assertResult(sourcePrivileges, future2); // The 3rd call should use cache when the application name is part of the original query final PlainActionFuture> future3 = new PlainActionFuture<>(); - store.getPrivileges(Arrays.asList("myapp"), null, future3); + store.getPrivileges(List.of("myapp"), null, future3); listener.get().onResponse(null); // Does not cache the name expansion if descriptors of the literal name is already cached assertNull(store.getApplicationNamesCache().get(Set.of("myapp"))); @@ -287,7 +267,7 @@ public void testGetPrivilegesCacheByApplicationNames() throws Exception { } public void testGetPrivilegesCacheWithApplicationAndPrivilegeName() throws Exception { - final List sourcePrivileges = Arrays.asList( + final List sourcePrivileges = List.of( new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()), new ApplicationPrivilegeDescriptor("myapp", "user", newHashSet("action:login", "data:read/*"), emptyMap()), new ApplicationPrivilegeDescriptor("myapp", "author", newHashSet("action:login", "data:read/*", "data:write/*"), emptyMap()) @@ -310,49 +290,63 @@ public void testGetPrivilegesCacheWithApplicationAndPrivilegeName() throws Excep // 2nd call with more privilege names can still use the cache final PlainActionFuture> future2 = new PlainActionFuture<>(); - store.getPrivileges(Collections.singletonList("myapp"), Arrays.asList("user", "author"), future2); + store.getPrivileges(Collections.singletonList("myapp"), List.of("user", "author"), future2); listener.get().onResponse(null); assertResult(sourcePrivileges.subList(1, 3), future2); } - public void testGetPrivilegesCacheWithNonExistentApplicationWildcard() throws Exception { + public void testGetPrivilegesCacheWithNonExistentApplicationName() throws Exception { final PlainActionFuture> future = new PlainActionFuture<>(); - store.getPrivileges(Collections.singletonList("*"), null, future); + store.getPrivileges(Collections.singletonList("no-such-app"), null, future); final SearchHit[] hits = buildHits(emptyList()); listener.get().onResponse(new SearchResponse(new SearchResponseSections( new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), null, null, false, false, null, 1), "_scrollId1", 1, 1, 0, 1, null, null) ); - assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("*"))); + assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("no-such-app"))); assertEquals(0, store.getDescriptorsCache().count()); assertResult(emptyList(), future); // The 2nd call should use cache final PlainActionFuture> future2 = new PlainActionFuture<>(); - store.getPrivileges(Collections.singletonList("*"), null, future2); + store.getPrivileges(Collections.singletonList("no-such-app"), null, future2); listener.get().onResponse(null); assertResult(emptyList(), future2); } - public void testGetPrivilegesCacheWithNonExistentApplicationName() throws Exception { + public void testGetPrivilegesCacheWithDifferentMatchAllApplicationNames() throws Exception { final PlainActionFuture> future = new PlainActionFuture<>(); - store.getPrivileges(Collections.singletonList("no-such-app"), null, future); + store.getPrivileges(emptyList(), null, future); final SearchHit[] hits = buildHits(emptyList()); listener.get().onResponse(new SearchResponse(new SearchResponseSections( new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), null, null, false, false, null, 1), "_scrollId1", 1, 1, 0, 1, null, null) ); - - assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("no-such-app"))); - assertEquals(0, store.getDescriptorsCache().count()); + assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("*"))); + assertEquals(1, store.getApplicationNamesCache().count()); assertResult(emptyList(), future); - // The 2nd call should use cache + // The 2nd call should use cache should translated to match all since it has a "*" final PlainActionFuture> future2 = new PlainActionFuture<>(); - store.getPrivileges(Collections.singletonList("no-such-app"), null, future2); - listener.get().onResponse(null); + store.getPrivileges(List.of("a", "b", "*", "c"), null, future2); + assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("*"))); + assertEquals(1, store.getApplicationNamesCache().count()); assertResult(emptyList(), future2); + + // The 3rd call also translated to match all + final PlainActionFuture> future3 = new PlainActionFuture<>(); + store.getPrivileges(null, null, future3); + assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("*"))); + assertEquals(1, store.getApplicationNamesCache().count()); + assertResult(emptyList(), future3); + + // The 4th call is also match all + final PlainActionFuture> future4 = new PlainActionFuture<>(); + store.getPrivileges(List.of("*"), null, future4); + assertEquals(emptySet(), store.getApplicationNamesCache().get(singleton("*"))); + assertEquals(1, store.getApplicationNamesCache().count()); + assertResult(emptyList(), future4); } public void testPutPrivileges() throws Exception { diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json index 2b21692bb978d..4817f1b001106 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json +++ b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json @@ -1,8 +1,8 @@ { "security.clear_cached_privileges":{ "documentation":{ - "url":"https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-clear-role-cache.html", - "description":"Evicts privileges from the native application privileges cache." + "url":"https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-clear-privilege-cache.html", + "description":"Evicts application privileges from the native application privileges cache." }, "stability":"stable", "url":{ diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml index e003dba2c2185..f17ba5d5e6744 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml @@ -233,6 +233,11 @@ teardown: } } + # Invalidate the cache for "app" + - do: + security.clear_cached_privileges: + application: "app" + --- "Test put and delete privileges": # Store some privileges @@ -324,3 +329,8 @@ teardown: "metadata": { } } } + + # Invalidate the cache for all applications + - do: + security.clear_cached_privileges: + application: "" From 03680bc9891557553be743288523860c71fa26be Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 4 May 2020 14:26:45 +1000 Subject: [PATCH 22/45] Skip yaml tests for 7.x --- .../resources/rest-api-spec/test/privileges/10_basic.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml index f17ba5d5e6744..13679d8a12a90 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml @@ -234,6 +234,9 @@ teardown: } # Invalidate the cache for "app" + - skip: + version: " - 7.99.99" + reason: "Backport pending" - do: security.clear_cached_privileges: application: "app" @@ -331,6 +334,9 @@ teardown: } # Invalidate the cache for all applications + - skip: + version: " - 7.99.99" + reason: "Backport pending" - do: security.clear_cached_privileges: application: "" From 435993df091f77e2f25a5460d7854003a482a5aa Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 4 May 2020 14:47:09 +1000 Subject: [PATCH 23/45] Fix yaml tests --- .../rest-api-spec/test/privileges/10_basic.yml | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml index 13679d8a12a90..3996901bd7548 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml @@ -233,14 +233,6 @@ teardown: } } - # Invalidate the cache for "app" - - skip: - version: " - 7.99.99" - reason: "Backport pending" - - do: - security.clear_cached_privileges: - application: "app" - --- "Test put and delete privileges": # Store some privileges @@ -333,10 +325,14 @@ teardown: } } - # Invalidate the cache for all applications +--- +"Test clear privileges cache": - skip: version: " - 7.99.99" reason: "Backport pending" + - do: + security.clear_cached_privileges: + application: "app" - do: security.clear_cached_privileges: application: "" From 20be80d5e9ecc72936533b6797a79406ba415213 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 4 May 2020 15:28:02 +1000 Subject: [PATCH 24/45] Add HLRC relevant files --- .../elasticsearch/client/SecurityClient.java | 35 +++++++++- .../client/SecurityRequestConverters.java | 10 +++ .../security/ClearPrivilegesCacheRequest.java | 65 +++++++++++++++++++ .../ClearPrivilegesCacheResponse.java | 50 ++++++++++++++ .../SecurityDocumentationIT.java | 48 ++++++++++++++ .../security/clear-privileges-cache.asciidoc | 32 +++++++++ x-pack/docs/en/rest-api/security.asciidoc | 7 +- .../en/rest-api/security/clear-cache.asciidoc | 4 +- 8 files changed, 246 insertions(+), 5 deletions(-) create mode 100644 client/rest-high-level/src/main/java/org/elasticsearch/client/security/ClearPrivilegesCacheRequest.java create mode 100644 client/rest-high-level/src/main/java/org/elasticsearch/client/security/ClearPrivilegesCacheResponse.java create mode 100644 docs/java-rest/high-level/security/clear-privileges-cache.asciidoc diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java index fba8c6ed2bd14..012093e0299b9 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java @@ -23,6 +23,8 @@ import org.elasticsearch.client.security.AuthenticateRequest; import org.elasticsearch.client.security.AuthenticateResponse; import org.elasticsearch.client.security.ChangePasswordRequest; +import org.elasticsearch.client.security.ClearPrivilegesCacheRequest; +import org.elasticsearch.client.security.ClearPrivilegesCacheResponse; import org.elasticsearch.client.security.ClearRealmCacheRequest; import org.elasticsearch.client.security.ClearRealmCacheResponse; import org.elasticsearch.client.security.ClearRolesCacheRequest; @@ -505,11 +507,42 @@ public ClearRolesCacheResponse clearRolesCache(ClearRolesCacheRequest request, R * @return cancellable that may be used to cancel the request */ public Cancellable clearRolesCacheAsync(ClearRolesCacheRequest request, RequestOptions options, - ActionListener listener) { + ActionListener listener) { return restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::clearRolesCache, options, ClearRolesCacheResponse::fromXContent, listener, emptySet()); } + /** + * Clears the privileges cache for a set of privileges. + * See + * the docs for more. + * + * @param request the request with the privileges for which the cache should be cleared. + * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized + * @return the response from the clear privileges cache call + * @throws IOException in case there is a problem sending the request or parsing back the response + */ + public ClearPrivilegesCacheResponse clearPrivilegesCache(ClearPrivilegesCacheRequest request, RequestOptions options) throws IOException { + return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::clearPrivilegesCache, options, + ClearPrivilegesCacheResponse::fromXContent, emptySet()); + } + + /** + * Clears the privileges cache for a set of privileges asynchronously. + * See + * the docs for more. + * + * @param request the request with the privileges for which the cache should be cleared. + * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized + * @param listener the listener to be notified upon request completion + * @return cancellable that may be used to cancel the request + */ + public Cancellable clearPrivilegesCacheAsync(ClearPrivilegesCacheRequest request, RequestOptions options, + ActionListener listener) { + return restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::clearPrivilegesCache, options, + ClearPrivilegesCacheResponse::fromXContent, listener, emptySet()); + } + /** * Synchronously retrieve the X.509 certificates that are used to encrypt communications in an Elasticsearch cluster. * See diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityRequestConverters.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityRequestConverters.java index c88d1d180fcc8..55301aed30752 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityRequestConverters.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityRequestConverters.java @@ -24,6 +24,7 @@ import org.apache.http.client.methods.HttpPost; import org.apache.http.client.methods.HttpPut; import org.elasticsearch.client.security.ChangePasswordRequest; +import org.elasticsearch.client.security.ClearPrivilegesCacheRequest; import org.elasticsearch.client.security.ClearRealmCacheRequest; import org.elasticsearch.client.security.ClearRolesCacheRequest; import org.elasticsearch.client.security.CreateApiKeyRequest; @@ -183,6 +184,15 @@ static Request clearRolesCache(ClearRolesCacheRequest disableCacheRequest) { return new Request(HttpPost.METHOD_NAME, endpoint); } + static Request clearPrivilegesCache(ClearPrivilegesCacheRequest disableCacheRequest) { + String endpoint = new RequestConverters.EndpointBuilder() + .addPathPartAsIs("_security/privilege") + .addCommaSeparatedPathParts(disableCacheRequest.applications()) + .addPathPart("_clear_cache") + .build(); + return new Request(HttpPost.METHOD_NAME, endpoint); + } + static Request deleteRoleMapping(DeleteRoleMappingRequest deleteRoleMappingRequest) { final String endpoint = new RequestConverters.EndpointBuilder() .addPathPartAsIs("_security/role_mapping") diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/ClearPrivilegesCacheRequest.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/ClearPrivilegesCacheRequest.java new file mode 100644 index 0000000000000..db6b2283e63b1 --- /dev/null +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/ClearPrivilegesCacheRequest.java @@ -0,0 +1,65 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.client.security; + +import org.elasticsearch.client.Validatable; + +import java.util.Arrays; + +/** + * The request used to clear the cache for native application privileges stored in an index. + */ +public final class ClearPrivilegesCacheRequest implements Validatable { + + private final String[] applications; + + /** + * Sets the applications for which caches will be evicted. When not set all privileges will be evicted from the cache. + * + * @param applications The application names + */ + public ClearPrivilegesCacheRequest(String... applications) { + this.applications = applications; + } + + /** + * @return an array of application names that will have the cache evicted or null if all + */ + public String[] applications() { + return applications; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ClearPrivilegesCacheRequest that = (ClearPrivilegesCacheRequest) o; + return Arrays.equals(applications, that.applications); + } + + @Override + public int hashCode() { + return Arrays.hashCode(applications); + } +} diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/ClearPrivilegesCacheResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/ClearPrivilegesCacheResponse.java new file mode 100644 index 0000000000000..d6b62123b6595 --- /dev/null +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/ClearPrivilegesCacheResponse.java @@ -0,0 +1,50 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.client.security; + +import org.elasticsearch.client.NodesResponseHeader; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.XContentParser; + +import java.io.IOException; +import java.util.List; + +/** + * The response object that will be returned when clearing the privileges cache + */ +public final class ClearPrivilegesCacheResponse extends SecurityNodesResponse { + + @SuppressWarnings("unchecked") + private static final ConstructingObjectParser PARSER = + new ConstructingObjectParser<>("clear_privileges_cache_response", false, + args -> new ClearPrivilegesCacheResponse((List)args[0], (NodesResponseHeader) args[1], (String) args[2])); + + static { + SecurityNodesResponse.declareCommonNodesResponseParsing(PARSER); + } + + public ClearPrivilegesCacheResponse(List nodes, NodesResponseHeader header, String clusterName) { + super(nodes, header, clusterName); + } + + public static ClearPrivilegesCacheResponse fromXContent(XContentParser parser) throws IOException { + return PARSER.parse(parser, null); + } +} diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java index c5ba7ba6011f3..bc5567e5370df 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java @@ -30,6 +30,8 @@ import org.elasticsearch.client.security.AuthenticateResponse; import org.elasticsearch.client.security.AuthenticateResponse.RealmInfo; import org.elasticsearch.client.security.ChangePasswordRequest; +import org.elasticsearch.client.security.ClearPrivilegesCacheRequest; +import org.elasticsearch.client.security.ClearPrivilegesCacheResponse; import org.elasticsearch.client.security.ClearRealmCacheRequest; import org.elasticsearch.client.security.ClearRealmCacheResponse; import org.elasticsearch.client.security.ClearRolesCacheRequest; @@ -1003,6 +1005,52 @@ public void onFailure(Exception e) { } } + public void testClearPrivilegesCache() throws Exception { + RestHighLevelClient client = highLevelClient(); + { + //tag::clear-privileges-cache-request + ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest("my_app"); + //end::clear-privileges-cache-request + //tag::clear-privileges-cache-execute + ClearPrivilegesCacheResponse response = client.security().clearPrivilegesCache(request, RequestOptions.DEFAULT); + //end::clear-privileges-cache-execute + + assertNotNull(response); + assertThat(response.getNodes(), not(empty())); + + //tag::clear-privileges-cache-response + List nodes = response.getNodes(); // <1> + //end::clear-privileges-cache-response + } + + { + //tag::clear-privileges-cache-execute-listener + ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest("my_app"); + ActionListener listener = new ActionListener<>() { + @Override + public void onResponse(ClearPrivilegesCacheResponse clearPrivilegesCacheResponse) { + // <1> + } + + @Override + public void onFailure(Exception e) { + // <2> + } + }; + //end::clear-privileges-cache-execute-listener + + // Replace the empty listener by a blocking listener in test + final CountDownLatch latch = new CountDownLatch(1); + listener = new LatchedActionListener<>(listener, latch); + + // tag::clear-privileges-cache-execute-async + client.security().clearPrivilegesCacheAsync(request, RequestOptions.DEFAULT, listener); // <1> + // end::clear-privileges-cache-execute-async + + assertTrue(latch.await(30L, TimeUnit.SECONDS)); + } + } + public void testGetSslCertificates() throws Exception { RestHighLevelClient client = highLevelClient(); { diff --git a/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc b/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc new file mode 100644 index 0000000000000..7f8c1a95e9f52 --- /dev/null +++ b/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc @@ -0,0 +1,32 @@ + +-- +:api: clear-privileges-cache +:request: ClearPrivilegesCacheRequest +:response: ClearPrivilegesCacheResponse +-- +[role="xpack"] +[id="{upid}-{api}"] +=== Clear Privileges Cache API + +[id="{upid}-{api}-request"] +==== Clear Privileges Cache Request + +A +{request}+ supports defining the name of applications that the cache should be cleared for. + +["source","java",subs="attributes,callouts,macros"] +-------------------------------------------------- +include-tagged::{doc-tests-file}[{api}-request] +-------------------------------------------------- + +include::../execution.asciidoc[] + +[id="{upid}-{api}-response"] +==== Clear Privileges Cache Response + +The returned +{response}+ allows to retrieve information about where the cache was cleared. + +["source","java",subs="attributes,callouts,macros"] +-------------------------------------------------- +include-tagged::{doc-tests-file}[{api}-response] +-------------------------------------------------- +<1> the list of nodes that the cache was cleared on diff --git a/x-pack/docs/en/rest-api/security.asciidoc b/x-pack/docs/en/rest-api/security.asciidoc index 215ad2d1c25a9..504986b658d34 100644 --- a/x-pack/docs/en/rest-api/security.asciidoc +++ b/x-pack/docs/en/rest-api/security.asciidoc @@ -15,10 +15,11 @@ You can use the following APIs to perform security activities. [[security-api-app-privileges]] === Application privileges -You can use the following APIs to add, update, retrieve, and remove application +You can use the following APIs to add, update, retrieve, and remove application privileges: -* <> +* <> +* <> * <> * <> @@ -28,7 +29,7 @@ privileges: You can use the following APIs to add, remove, update, and retrieve role mappings: -* <> +* <> * <> * <> diff --git a/x-pack/docs/en/rest-api/security/clear-cache.asciidoc b/x-pack/docs/en/rest-api/security/clear-cache.asciidoc index 2a1a227163da0..fcae76cb99779 100644 --- a/x-pack/docs/en/rest-api/security/clear-cache.asciidoc +++ b/x-pack/docs/en/rest-api/security/clear-cache.asciidoc @@ -25,8 +25,10 @@ There are realm settings that you can use to configure the user cache. For more information, see <>. -To evict roles from the role cache, see the +To evict roles from the role cache, see the <>. +To evict privileges from the privilege cache, see the +<>. [[security-api-clear-path-params]] ==== {api-path-parms-title} From d2f2f3eeeb122b29b4d12defd8f9f4f0e5500b57 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 4 May 2020 15:38:39 +1000 Subject: [PATCH 25/45] check style --- .../main/java/org/elasticsearch/client/SecurityClient.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java index 012093e0299b9..4fdf8907f5572 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java @@ -507,7 +507,7 @@ public ClearRolesCacheResponse clearRolesCache(ClearRolesCacheRequest request, R * @return cancellable that may be used to cancel the request */ public Cancellable clearRolesCacheAsync(ClearRolesCacheRequest request, RequestOptions options, - ActionListener listener) { + ActionListener listener) { return restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::clearRolesCache, options, ClearRolesCacheResponse::fromXContent, listener, emptySet()); } @@ -522,7 +522,8 @@ public Cancellable clearRolesCacheAsync(ClearRolesCacheRequest request, RequestO * @return the response from the clear privileges cache call * @throws IOException in case there is a problem sending the request or parsing back the response */ - public ClearPrivilegesCacheResponse clearPrivilegesCache(ClearPrivilegesCacheRequest request, RequestOptions options) throws IOException { + public ClearPrivilegesCacheResponse clearPrivilegesCache(ClearPrivilegesCacheRequest request, + RequestOptions options) throws IOException { return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::clearPrivilegesCache, options, ClearPrivilegesCacheResponse::fromXContent, emptySet()); } @@ -538,7 +539,7 @@ public ClearPrivilegesCacheResponse clearPrivilegesCache(ClearPrivilegesCacheReq * @return cancellable that may be used to cancel the request */ public Cancellable clearPrivilegesCacheAsync(ClearPrivilegesCacheRequest request, RequestOptions options, - ActionListener listener) { + ActionListener listener) { return restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::clearPrivilegesCache, options, ClearPrivilegesCacheResponse::fromXContent, listener, emptySet()); } From f1f7035fb45ceeefeda9d006ca7ae96ae7549d50 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 4 May 2020 16:32:53 +1000 Subject: [PATCH 26/45] Fix broken docs --- docs/java-rest/high-level/supported-apis.asciidoc | 2 ++ x-pack/docs/en/rest-api/security.asciidoc | 1 + .../en/rest-api/security/clear-privileges-cache.asciidoc | 6 +++--- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/java-rest/high-level/supported-apis.asciidoc b/docs/java-rest/high-level/supported-apis.asciidoc index e982dbbf75ae4..c7d4a0e2367ed 100644 --- a/docs/java-rest/high-level/supported-apis.asciidoc +++ b/docs/java-rest/high-level/supported-apis.asciidoc @@ -448,6 +448,7 @@ The Java High Level REST Client supports the following Security APIs: * <<{upid}-get-roles>> * <> * <<{upid}-clear-roles-cache>> +* <<{upid}-clear-privileges-cache>> * <<{upid}-clear-realm-cache>> * <<{upid}-authenticate>> * <<{upid}-has-privileges>> @@ -479,6 +480,7 @@ include::security/delete-privileges.asciidoc[] include::security/get-builtin-privileges.asciidoc[] include::security/get-privileges.asciidoc[] include::security/clear-roles-cache.asciidoc[] +include::security/clear-privileges-cache.asciidoc[] include::security/clear-realm-cache.asciidoc[] include::security/authenticate.asciidoc[] include::security/has-privileges.asciidoc[] diff --git a/x-pack/docs/en/rest-api/security.asciidoc b/x-pack/docs/en/rest-api/security.asciidoc index 504986b658d34..5a81fc8d2c7bb 100644 --- a/x-pack/docs/en/rest-api/security.asciidoc +++ b/x-pack/docs/en/rest-api/security.asciidoc @@ -107,6 +107,7 @@ include::security/authenticate.asciidoc[] include::security/change-password.asciidoc[] include::security/clear-cache.asciidoc[] include::security/clear-roles-cache.asciidoc[] +include::security/clear-privileges-cache.asciidoc[] include::security/create-api-keys.asciidoc[] include::security/put-app-privileges.asciidoc[] include::security/create-role-mappings.asciidoc[] diff --git a/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc b/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc index 579596c6415d7..b4722534118ae 100644 --- a/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc +++ b/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc @@ -1,11 +1,11 @@ [role="xpack"] [[security-api-clear-privilege-cache]] -=== Clear application privileges cache API +=== Clear privileges cache API ++++ Clear privileges cache ++++ -Evicts application privileges from the native application privilege cache. +Evicts privileges from the native application privilege cache. [[security-api-clear-privilege-cache-request]] ==== {api-request-title} @@ -33,7 +33,7 @@ For more information about the native realm, see [[security-api-clear-privilege-cache-example]] ==== {api-examples-title} -The clear privileges cache API evicts application privileges from the native application privilege cache. +The clear privileges cache API evicts privileges from the native application privilege cache. For example, to clear the cache for `myapp`: [source,console] From db6020eef6900835ea5ed094f76967db28b8302e Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 5 May 2020 09:11:47 +1000 Subject: [PATCH 27/45] Add bwc for the new transport action --- .../TransportClearPrivilegesCacheAction.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index db89c9f40f462..6f8e663e81b91 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -6,9 +6,12 @@ package org.elasticsearch.xpack.security.action.privilege; +import org.elasticsearch.Version; import org.elasticsearch.action.FailedNodeException; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.nodes.TransportNodesAction; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.StreamInput; @@ -22,6 +25,7 @@ import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore; import java.io.IOException; +import java.util.Arrays; import java.util.List; public class TransportClearPrivilegesCacheAction extends TransportNodesAction node.getVersion().onOrAfter(Version.V_8_0_0)).toArray(DiscoveryNode[]::new)); + } } From 73dda1fecc2d32e45faf34b6aefafd866e65fdc3 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 5 May 2020 12:06:17 +1000 Subject: [PATCH 28/45] Add working rolling upgrade mixed-cluster tests for privilege cache clearing --- .../xpack/security/Security.java | 4 ++-- ...va => RestClearPrivilegesCacheAction.java} | 4 ++-- .../test/privileges/10_basic.yml | 6 +++++- x-pack/qa/rolling-upgrade/build.gradle | 3 ++- .../110_application_privileges.yml | 19 +++++++++++++++++++ 5 files changed, 30 insertions(+), 6 deletions(-) rename x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/{RestClearPrivilegesAction.java => RestClearPrivilegesCacheAction.java} (91%) create mode 100644 x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/110_application_privileges.yml diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index f403d7f7bd158..b29249d158a54 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -219,7 +219,7 @@ import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectAuthenticateAction; import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectLogoutAction; import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectPrepareAuthenticationAction; -import org.elasticsearch.xpack.security.rest.action.privilege.RestClearPrivilegesAction; +import org.elasticsearch.xpack.security.rest.action.privilege.RestClearPrivilegesCacheAction; import org.elasticsearch.xpack.security.rest.action.privilege.RestDeletePrivilegesAction; import org.elasticsearch.xpack.security.rest.action.privilege.RestGetBuiltinPrivilegesAction; import org.elasticsearch.xpack.security.rest.action.privilege.RestGetPrivilegesAction; @@ -790,7 +790,7 @@ public List getRestHandlers(Settings settings, RestController restC new RestAuthenticateAction(settings, securityContext.get(), getLicenseState()), new RestClearRealmCacheAction(settings, getLicenseState()), new RestClearRolesCacheAction(settings, getLicenseState()), - new RestClearPrivilegesAction(settings, getLicenseState()), + new RestClearPrivilegesCacheAction(settings, getLicenseState()), new RestGetUsersAction(settings, getLicenseState()), new RestPutUserAction(settings, getLicenseState()), new RestDeleteUserAction(settings, getLicenseState()), diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesCacheAction.java similarity index 91% rename from x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java rename to x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesCacheAction.java index 7e46930b36f1a..205234c297de4 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/privilege/RestClearPrivilegesCacheAction.java @@ -21,9 +21,9 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; -public class RestClearPrivilegesAction extends SecurityBaseRestHandler { +public class RestClearPrivilegesCacheAction extends SecurityBaseRestHandler { - public RestClearPrivilegesAction(Settings settings, XPackLicenseState licenseState) { + public RestClearPrivilegesCacheAction(Settings settings, XPackLicenseState licenseState) { super(settings, licenseState); } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml index 3996901bd7548..572cf2fd48f7a 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/10_basic.yml @@ -329,10 +329,14 @@ teardown: "Test clear privileges cache": - skip: version: " - 7.99.99" - reason: "Backport pending" + reason: "backport pending https://github.com/elastic/elasticsearch/pull/55836" + - do: security.clear_cached_privileges: application: "app" + - match: { _nodes.failed: 0 } + - do: security.clear_cached_privileges: application: "" + - match: { _nodes.failed: 0 } diff --git a/x-pack/qa/rolling-upgrade/build.gradle b/x-pack/qa/rolling-upgrade/build.gradle index c05e0c9a1275c..b07153ed284f7 100644 --- a/x-pack/qa/rolling-upgrade/build.gradle +++ b/x-pack/qa/rolling-upgrade/build.gradle @@ -116,7 +116,8 @@ for (Version bwcVersion : bwcVersions.wireCompatible) { 'mixed_cluster/40_ml_datafeed_crud/Put job and datafeed with aggs in mixed cluster', 'mixed_cluster/80_transform_jobs_crud/Test put batch transform on mixed cluster', 'mixed_cluster/80_transform_jobs_crud/Test put continuous transform on mixed cluster', - 'mixed_cluster/90_ml_data_frame_analytics_crud/Put an outlier_detection job on the mixed cluster' + 'mixed_cluster/90_ml_data_frame_analytics_crud/Put an outlier_detection job on the mixed cluster', + 'mixed_cluster/110_application_privileges/Test clear privileges cache' ].join(',') } diff --git a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/110_application_privileges.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/110_application_privileges.yml new file mode 100644 index 0000000000000..fdbf8ddf55a68 --- /dev/null +++ b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/110_application_privileges.yml @@ -0,0 +1,19 @@ +--- +"Test clear privileges cache": + - skip: + features: "node_selector" + + - do: + node_selector: + version: "8.0.0 - " + security.clear_cached_privileges: + application: "app" + - match: { _nodes.failed: 0 } + + - do: + node_selector: + version: "8.0.0 - " + security.clear_cached_privileges: + application: "" + - match: { _nodes.failed: 0 } + From 65f9524627fcaaadd7ec6350a36fbf20738980d5 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 5 May 2020 13:56:23 +1000 Subject: [PATCH 29/45] Remove bwc handling code and simply report no handler --- .../TransportClearPrivilegesCacheAction.java | 9 --------- x-pack/qa/rolling-upgrade/build.gradle | 3 +-- .../110_application_privileges.yml | 19 ------------------- 3 files changed, 1 insertion(+), 30 deletions(-) delete mode 100644 x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/110_application_privileges.yml diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index 6f8e663e81b91..254ae6dbe3fdc 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -84,13 +84,4 @@ protected ClearPrivilegesCacheResponse.Node nodeOperation(ClearPrivilegesCacheRe } return new ClearPrivilegesCacheResponse.Node(clusterService.localNode()); } - - @Override - protected void resolveRequest(ClearPrivilegesCacheRequest request, ClusterState clusterState) { - assert request.concreteNodes() == null : "request concreteNodes shouldn't be set"; - String[] nodesIds = clusterState.nodes().resolveNodes(request.nodesIds()); - // TODO: version needs to be updated once 7.x backport is in place - request.setConcreteNodes(Arrays.stream(nodesIds).map(clusterState.nodes()::get) - .filter(node -> node.getVersion().onOrAfter(Version.V_8_0_0)).toArray(DiscoveryNode[]::new)); - } } diff --git a/x-pack/qa/rolling-upgrade/build.gradle b/x-pack/qa/rolling-upgrade/build.gradle index b07153ed284f7..c05e0c9a1275c 100644 --- a/x-pack/qa/rolling-upgrade/build.gradle +++ b/x-pack/qa/rolling-upgrade/build.gradle @@ -116,8 +116,7 @@ for (Version bwcVersion : bwcVersions.wireCompatible) { 'mixed_cluster/40_ml_datafeed_crud/Put job and datafeed with aggs in mixed cluster', 'mixed_cluster/80_transform_jobs_crud/Test put batch transform on mixed cluster', 'mixed_cluster/80_transform_jobs_crud/Test put continuous transform on mixed cluster', - 'mixed_cluster/90_ml_data_frame_analytics_crud/Put an outlier_detection job on the mixed cluster', - 'mixed_cluster/110_application_privileges/Test clear privileges cache' + 'mixed_cluster/90_ml_data_frame_analytics_crud/Put an outlier_detection job on the mixed cluster' ].join(',') } diff --git a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/110_application_privileges.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/110_application_privileges.yml deleted file mode 100644 index fdbf8ddf55a68..0000000000000 --- a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/110_application_privileges.yml +++ /dev/null @@ -1,19 +0,0 @@ ---- -"Test clear privileges cache": - - skip: - features: "node_selector" - - - do: - node_selector: - version: "8.0.0 - " - security.clear_cached_privileges: - application: "app" - - match: { _nodes.failed: 0 } - - - do: - node_selector: - version: "8.0.0 - " - security.clear_cached_privileges: - application: "" - - match: { _nodes.failed: 0 } - From 04bb476648b14a9390e392d367d5f33d59e9e49c Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 5 May 2020 14:02:07 +1000 Subject: [PATCH 30/45] Checkstyle --- .../action/privilege/TransportClearPrivilegesCacheAction.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index 254ae6dbe3fdc..db89c9f40f462 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -6,12 +6,9 @@ package org.elasticsearch.xpack.security.action.privilege; -import org.elasticsearch.Version; import org.elasticsearch.action.FailedNodeException; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.nodes.TransportNodesAction; -import org.elasticsearch.cluster.ClusterState; -import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.StreamInput; @@ -25,7 +22,6 @@ import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore; import java.io.IOException; -import java.util.Arrays; import java.util.List; public class TransportClearPrivilegesCacheAction extends TransportNodesAction Date: Fri, 15 May 2020 11:12:05 +1000 Subject: [PATCH 31/45] Clear privilege cache on security index changes --- .../xpack/security/Security.java | 1 + .../authz/store/NativePrivilegeStore.java | 13 +++++++ .../store/NativePrivilegeStoreTests.java | 39 +++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index b29249d158a54..ddb1a9c9bed66 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -428,6 +428,7 @@ Collection createComponents(Client client, ThreadPool threadPool, Cluste final NativePrivilegeStore privilegeStore = new NativePrivilegeStore(settings, client, securityIndex.get()); components.add(privilegeStore); + securityIndex.get().addIndexStateListener(privilegeStore::onSecurityIndexStateChange); dlsBitsetCache.set(new DocumentSubsetBitsetCache(settings, threadPool)); final FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(settings); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index abe54e9a06fa2..9c2a693ccd596 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -69,6 +69,8 @@ import static org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor.DOC_TYPE_VALUE; import static org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor.Fields.APPLICATION; import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_MAIN_ALIAS; +import static org.elasticsearch.xpack.security.support.SecurityIndexManager.isIndexDeleted; +import static org.elasticsearch.xpack.security.support.SecurityIndexManager.isMoveFromRedToNonRed; /** * {@code NativePrivilegeStore} is a store that reads/writes {@link ApplicationPrivilegeDescriptor} objects, @@ -204,6 +206,13 @@ private void innerGetPrivileges(Collection applications, } } + public void onSecurityIndexStateChange(SecurityIndexManager.State previousState, SecurityIndexManager.State currentState) { + if (isMoveFromRedToNonRed(previousState, currentState) || isIndexDeleted(previousState, currentState) + || previousState.isIndexUpToDate != currentState.isIndexUpToDate) { + invalidateAll(); + } + } + public void invalidate(Collection updatedApplicationNames) { logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); numInvalidation.incrementAndGet(); @@ -425,4 +434,8 @@ Cache> getDescriptorsCache() { return descriptorsCache; } + // Package private for tests + AtomicLong getNumInvalidation() { + return numInvalidation; + } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index 17ee6008c3951..df4330e3e1352 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -20,6 +20,8 @@ import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.action.support.WriteRequest; import org.elasticsearch.client.Client; +import org.elasticsearch.cluster.health.ClusterHealthStatus; +import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.common.Strings; import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.bytes.BytesArray; @@ -40,6 +42,7 @@ import org.junit.Before; import org.mockito.Mockito; +import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -457,6 +460,42 @@ public void testInvalidateAll() { assertEquals(0, store.getDescriptorsCache().count()); } + public void testCacheClearOnIndexHealthChange() { + final String securityIndexName = randomFrom( + RestrictedIndicesNames.INTERNAL_SECURITY_MAIN_INDEX_6, RestrictedIndicesNames.INTERNAL_SECURITY_MAIN_INDEX_7); + + long count = store.getNumInvalidation().get(); + + // Cache should be cleared when security is back to green + store.onSecurityIndexStateChange( + dummyState(securityIndexName, true, randomFrom((ClusterHealthStatus) null, ClusterHealthStatus.RED)), + dummyState(securityIndexName, true, randomFrom(ClusterHealthStatus.GREEN, ClusterHealthStatus.YELLOW))); + assertEquals(++count, store.getNumInvalidation().get()); + + // Cache should be cleared when security is deleted + store.onSecurityIndexStateChange( + dummyState(securityIndexName, true, randomFrom(ClusterHealthStatus.values())), + dummyState(securityIndexName, true, null)); + assertEquals(++count, store.getNumInvalidation().get()); + + // Cache should be cleared if indexUpToDate changed + final boolean isIndexUpToDate = randomBoolean(); + final ArrayList allPossibleHealthStatus = new ArrayList<>(Arrays.asList(ClusterHealthStatus.values())); + allPossibleHealthStatus.add(null); + store.onSecurityIndexStateChange( + dummyState(securityIndexName, isIndexUpToDate, randomFrom(allPossibleHealthStatus)), + dummyState(securityIndexName, !isIndexUpToDate, randomFrom(allPossibleHealthStatus))); + assertEquals(++count, store.getNumInvalidation().get()); + } + + private SecurityIndexManager.State dummyState( + String concreteSecurityIndexName, boolean isIndexUpToDate, ClusterHealthStatus healthStatus) { + return new SecurityIndexManager.State( + Instant.now(), isIndexUpToDate, true, true, null, + concreteSecurityIndexName, healthStatus, IndexMetadata.State.OPEN + ); + } + private SearchHit[] buildHits(List sourcePrivileges) { final SearchHit[] hits = new SearchHit[sourcePrivileges.size()]; for (int i = 0; i < hits.length; i++) { From e87d8974b248a20d5947facd397d7b2b9bb206bb Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 29 May 2020 16:58:14 +1000 Subject: [PATCH 32/45] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java Co-authored-by: Tim Vernum --- .../xpack/security/authz/store/NativePrivilegeStore.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 9c2a693ccd596..f49a4318c10d0 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -134,7 +134,8 @@ public void getPrivileges(Collection applications, Collection na Set concreteApplicationNames = applicationNamesCache.get(applicationNamesCacheKey); if (concreteApplicationNames != null && concreteApplicationNames.size() == 0) { - logger.debug("returning empty application privileges as application names result in empty list"); + logger.debug("returning empty application privileges for [{}] as application names result in empty list", + applicationNamesCacheKey); listener.onResponse(Collections.emptySet()); } else { final Set cachedDescriptors = cachedDescriptorsForApplicationNames( From 5acc7ec3738dc134013ff5f7ab955ee95b330779 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 29 May 2020 16:58:42 +1000 Subject: [PATCH 33/45] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java Co-authored-by: Tim Vernum --- .../xpack/security/authz/store/NativePrivilegeStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index f49a4318c10d0..cd27f7db7f9f2 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -141,7 +141,7 @@ public void getPrivileges(Collection applications, Collection na final Set cachedDescriptors = cachedDescriptorsForApplicationNames( concreteApplicationNames != null ? concreteApplicationNames : applicationNamesCacheKey); if (cachedDescriptors != null) { - logger.debug("All application privileges found in cache"); + logger.debug("All application privileges for [{}] found in cache", applicationNamesCacheKey); listener.onResponse(filterDescriptorsForPrivilegeNames(cachedDescriptors, names)); } else { final long invalidationCounter = numInvalidation.get(); From 5e3174a6b8a3ecf6591e3c68bc51195d8d177a06 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 29 May 2020 23:15:02 +1000 Subject: [PATCH 34/45] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java Co-authored-by: Tim Vernum --- .../xpack/security/authz/store/NativePrivilegeStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index cd27f7db7f9f2..0caf7e535b232 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -172,7 +172,7 @@ public void getPrivileges(Collection applications, Collection na private void innerGetPrivileges(Collection applications, ActionListener> listener) { - assert Objects.requireNonNull(applications).isEmpty() == false; + assert applications != null && applications.size() > 0 : "Application names are required (found " + applications + ")"; final SecurityIndexManager frozenSecurityIndex = securityIndexManager.freeze(); if (frozenSecurityIndex.indexExists() == false) { From 385fb124c868d55fe903db2d732e875364135231 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Sat, 30 May 2020 00:23:54 +1000 Subject: [PATCH 35/45] Update x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java Co-authored-by: Tim Vernum --- .../security/authz/store/NativePrivilegeStoreCacheTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java index 99cf505a41b58..c9a482bf0e98e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java @@ -108,7 +108,7 @@ public void configureApplicationPrivileges() { assertEquals(6, putPrivilegesResponse.created().values().stream().mapToInt(List::size).sum()); } - public void testGetPrivileges() { + public void testGetPrivilegesUsesCache() { final Client client = client(); ApplicationPrivilegeDescriptor[] privileges = new GetPrivilegesRequestBuilder(client) From e411dc9254178c9b7a389d5c90a5922abdf7cdbd Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Sat, 30 May 2020 00:24:21 +1000 Subject: [PATCH 36/45] Update x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java Co-authored-by: Tim Vernum --- .../security/authz/store/NativePrivilegeStoreCacheTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java index c9a482bf0e98e..14cd84df772f1 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java @@ -149,7 +149,7 @@ public void testGetPrivilegesUsesCache() { assertEquals(0, privileges.length); } - public void testWildcard() { + public void testPopulationOfCacheWhenLoadingPrivilegesForAllApplications() { final Client client = client(); ApplicationPrivilegeDescriptor[] privileges = new GetPrivilegesRequestBuilder(client) From 58b3942a138c135a4ed9caeeaf4c83e38e59336e Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Sat, 30 May 2020 00:35:27 +1000 Subject: [PATCH 37/45] Address feedback --- .../client/documentation/SecurityDocumentationIT.java | 4 ++-- .../security/clear-privileges-cache.asciidoc | 3 ++- .../rest-api/security/clear-privileges-cache.asciidoc | 1 + .../action/privilege/ClearPrivilegesCacheRequest.java | 4 ++-- .../TransportClearPrivilegesCacheAction.java | 2 +- .../security/authz/store/CompositeRolesStore.java | 1 - .../security/authz/store/NativePrivilegeStore.java | 11 ++++++----- .../authz/store/NativePrivilegeStoreTests.java | 5 ++++- .../api/security.clear_cached_privileges.json | 4 ++-- 9 files changed, 20 insertions(+), 15 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java index bc5567e5370df..65936f64607fa 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java @@ -1009,7 +1009,7 @@ public void testClearPrivilegesCache() throws Exception { RestHighLevelClient client = highLevelClient(); { //tag::clear-privileges-cache-request - ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest("my_app"); + ClearPrivilegesCacheRequest request = new ClearPrivilegesCacheRequest("my_app"); // <1> //end::clear-privileges-cache-request //tag::clear-privileges-cache-execute ClearPrivilegesCacheResponse response = client.security().clearPrivilegesCache(request, RequestOptions.DEFAULT); @@ -1019,7 +1019,7 @@ public void testClearPrivilegesCache() throws Exception { assertThat(response.getNodes(), not(empty())); //tag::clear-privileges-cache-response - List nodes = response.getNodes(); // <1> + List nodes = response.getNodes(); // <2> //end::clear-privileges-cache-response } diff --git a/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc b/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc index 7f8c1a95e9f52..ba9df960726e2 100644 --- a/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc +++ b/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc @@ -29,4 +29,5 @@ The returned +{response}+ allows to retrieve information about where the cache w -------------------------------------------------- include-tagged::{doc-tests-file}[{api}-response] -------------------------------------------------- -<1> the list of nodes that the cache was cleared on +<1> the name of the application(s) for which the cache should be cleared +<2> the list of nodes that the cache was cleared on diff --git a/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc b/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc index b4722534118ae..71dbeebcb3d4a 100644 --- a/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc +++ b/x-pack/docs/en/rest-api/security/clear-privileges-cache.asciidoc @@ -6,6 +6,7 @@ ++++ Evicts privileges from the native application privilege cache. +The cache is also automatically cleared for applications that have their privileges updated. [[security-api-clear-privilege-cache-request]] ==== {api-request-title} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java index 96e6cafffe403..04acc372dfb56 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/privilege/ClearPrivilegesCacheRequest.java @@ -42,7 +42,7 @@ public String[] applicationNames() { return applicationNames; } - public boolean isClearRolesCache() { + public boolean clearRolesCache() { return clearRolesCache; } @@ -72,7 +72,7 @@ public String[] getApplicationNames() { return applicationNames; } - public boolean isClearRolesCache() { + public boolean clearRolesCache() { return clearRolesCache; } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java index db89c9f40f462..563c4b27ede91 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/privilege/TransportClearPrivilegesCacheAction.java @@ -75,7 +75,7 @@ protected ClearPrivilegesCacheResponse.Node nodeOperation(ClearPrivilegesCacheRe } else { privilegesStore.invalidate(List.of(request.getApplicationNames())); } - if (request.isClearRolesCache()) { + if (request.clearRolesCache()) { rolesStore.invalidateAll(); } return new ClearPrivilegesCacheResponse.Node(clusterService.localNode()); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java index 92ecf5c01731d..7875b33e7f862 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java @@ -410,7 +410,6 @@ public static void buildRoleFromDescriptors(Collection roleDescr final Set applicationPrivilegeNames = applicationPrivilegesMap.values().stream() .flatMap(Collection::stream) .collect(Collectors.toSet()); - // Role itself is cached, so skipping caching for application privileges privilegeStore.getPrivileges(applicationNames, applicationPrivilegeNames, ActionListener.wrap(appPrivileges -> { applicationPrivilegesMap.forEach((key, names) -> ApplicationPrivilege.get(key.v1(), names, appPrivileges) .forEach(priv -> builder.addApplicationPrivilege(priv, key.v2()))); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 0caf7e535b232..3665e34573368 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -61,6 +62,8 @@ import java.util.function.Supplier; import java.util.stream.Collector; import java.util.stream.Collectors; +import java.util.stream.Stream; +import java.util.stream.StreamSupport; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING; @@ -127,7 +130,7 @@ public void getPrivileges(Collection applications, Collection na // TODO: We should have a way to express true Zero applications final Set applicationNamesCacheKey = (isEmpty(applications) || applications.contains("*")) ? - Set.of("*") : new LinkedHashSet<>(applications); + Set.of("*") : Set.copyOf(applications); // Always fetch for the concrete application names even when the passed-in application names has no wildcard. // This serves as a negative lookup, i.e. when a passed-in non-wildcard application does not exist. @@ -170,8 +173,7 @@ public void getPrivileges(Collection applications, Collection na } } - private void innerGetPrivileges(Collection applications, - ActionListener> listener) { + private void innerGetPrivileges(Collection applications, ActionListener> listener) { assert applications != null && applications.size() > 0 : "Application names are required (found " + applications + ")"; final SecurityIndexManager frozenSecurityIndex = securityIndexManager.freeze(); @@ -199,7 +201,6 @@ private void innerGetPrivileges(Collection applications, new ParameterizedMessage("Searching for [{}] privileges with query [{}]", applications, Strings.toString(query))); request.indicesOptions().ignoreUnavailable(); - // TODO: not parsing source of cached entries? ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener), hit -> buildPrivilege(hit.getId(), hit.getSourceRef())); } @@ -217,9 +218,9 @@ public void onSecurityIndexStateChange(SecurityIndexManager.State previousState, public void invalidate(Collection updatedApplicationNames) { logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); numInvalidation.incrementAndGet(); + final Set uniqueNames = Set.copyOf(updatedApplicationNames); // Always completely invalidate application names cache due to wildcard applicationNamesCache.invalidateAll(); - final Set uniqueNames = Set.copyOf(updatedApplicationNames); uniqueNames.forEach(descriptorsCache::invalidate); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index df4330e3e1352..276abd39896b7 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -61,6 +61,7 @@ import static java.util.Collections.singleton; import static java.util.Collections.singletonList; import static org.elasticsearch.common.util.set.Sets.newHashSet; +import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -156,7 +157,9 @@ public void testGetPrivilegesByApplicationName() throws Exception { assertThat(request.indices(), arrayContaining(RestrictedIndicesNames.SECURITY_MAIN_ALIAS)); final String query = Strings.toString(request.source().query()); - assertThat(query, containsString("{\"terms\":{\"application\":[\"myapp\",\"yourapp\"]")); + assertThat(query, anyOf( + containsString("{\"terms\":{\"application\":[\"myapp\",\"yourapp\"]"), + containsString("{\"terms\":{\"application\":[\"yourapp\",\"myapp\"]"))); assertThat(query, containsString("{\"term\":{\"type\":{\"value\":\"application-privilege\"")); final SearchHit[] hits = buildHits(sourcePrivileges); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json index 4817f1b001106..e2c66a32fa27b 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json +++ b/x-pack/plugin/src/test/resources/rest-api-spec/api/security.clear_cached_privileges.json @@ -14,8 +14,8 @@ ], "parts":{ "application":{ - "type":"string", - "description":"Application name" + "type":"list", + "description":"A comma-separated list of application names" } } } From 9a763c37817121dadf2878891660e7b602ebbd67 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Sat, 30 May 2020 00:45:49 +1000 Subject: [PATCH 38/45] Address feedback --- .../authz/store/NativePrivilegeStore.java | 5 ----- .../store/NativePrivilegeStoreCacheTests.java | 18 +++++++++--------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 3665e34573368..96c8f8a17b2e1 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -28,7 +28,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -53,17 +52,13 @@ import java.util.Collection; import java.util.Collections; import java.util.HashSet; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Supplier; import java.util.stream.Collector; import java.util.stream.Collectors; -import java.util.stream.Stream; -import java.util.stream.StreamSupport; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java index 14cd84df772f1..fe71467f8ef65 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java @@ -91,12 +91,12 @@ protected String configUsersRoles() { @Before public void configureApplicationPrivileges() { final List applicationPrivilegeDescriptors = Arrays.asList( - new ApplicationPrivilegeDescriptor("app-1", "read", Set.of("a:b:c", "x:y:z"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-1", "write", Set.of("a:b:c", "x:y:z"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-1", "admin", Set.of("a:b:c", "x:y:z"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-2", "read", Set.of("e:f:g", "t:u:v"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-2", "write", Set.of("e:f:g", "t:u:v"), emptyMap()), - new ApplicationPrivilegeDescriptor("app-2", "admin", Set.of("e:f:g", "t:u:v"), emptyMap())); + new ApplicationPrivilegeDescriptor("app-1", "read", Set.of("r:a:b:c", "r:x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-1", "write", Set.of("w:a:b:c", "w:x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-1", "admin", Set.of("a:a:b:c", "a:x:y:z"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "read", Set.of("r:e:f:g", "r:t:u:v"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "write", Set.of("w:e:f:g", "w:t:u:v"), emptyMap()), + new ApplicationPrivilegeDescriptor("app-2", "admin", Set.of("a:e:f:g", "a:t:u:v"), emptyMap())); final PutPrivilegesRequest putPrivilegesRequest = new PutPrivilegesRequest(); putPrivilegesRequest.setPrivileges(applicationPrivilegeDescriptors); @@ -168,7 +168,7 @@ public void testPopulationOfCacheWhenLoadingPrivilegesForAllApplications() { assertEquals(5, new GetPrivilegesRequestBuilder(client).execute().actionGet().privileges().length); // Now put it back and wild expression expansion should be invalidated again - addApplicationPrivilege("app-2", "read", "e:f:g", "t:u:v"); + addApplicationPrivilege("app-2", "read", "r:e:f:g", "r:t:u:v"); assertEquals(6, new GetPrivilegesRequestBuilder(client).execute().actionGet().privileges().length); @@ -214,7 +214,7 @@ public void testHasPrivileges() { .get("app-1").stream().findFirst().orElseThrow().getPrivileges().get("check")); // Add the app-1 check privilege and it should be picked up - addApplicationPrivilege("app-1", "check", "a:b:c"); + addApplicationPrivilege("app-1", "check", "c:a:b:c"); assertTrue(checkPrivilege("app-1", "check").getApplicationPrivileges() .get("app-1").stream().findFirst().orElseThrow().getPrivileges().get("check")); @@ -264,7 +264,7 @@ public void testRolesCacheIsClearedWhenPrivilegesIsChanged() { if (randomBoolean()) { deleteApplicationPrivilege("app-1", "read"); } else { - addApplicationPrivilege("app-3", "read", "t:u:v"); + addApplicationPrivilege("app-3", "read", "r:q:r:s"); } // Since role cache is cleared, the cluster health action is no longer authorized expectThrows(ElasticsearchSecurityException.class, From a43b3ef2a3ccb784e7bdbfbf1a95788b61607d31 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Sat, 30 May 2020 00:56:35 +1000 Subject: [PATCH 39/45] Fix docs --- .../client/documentation/SecurityDocumentationIT.java | 2 +- .../high-level/security/clear-privileges-cache.asciidoc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java index 65936f64607fa..5c1f454413c7c 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java @@ -1019,7 +1019,7 @@ public void testClearPrivilegesCache() throws Exception { assertThat(response.getNodes(), not(empty())); //tag::clear-privileges-cache-response - List nodes = response.getNodes(); // <2> + List nodes = response.getNodes(); // <1> //end::clear-privileges-cache-response } diff --git a/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc b/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc index ba9df960726e2..2376c6a5bd88e 100644 --- a/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc +++ b/docs/java-rest/high-level/security/clear-privileges-cache.asciidoc @@ -17,6 +17,7 @@ A +{request}+ supports defining the name of applications that the cache should b -------------------------------------------------- include-tagged::{doc-tests-file}[{api}-request] -------------------------------------------------- +<1> the name of the application(s) for which the cache should be cleared include::../execution.asciidoc[] @@ -29,5 +30,4 @@ The returned +{response}+ allows to retrieve information about where the cache w -------------------------------------------------- include-tagged::{doc-tests-file}[{api}-response] -------------------------------------------------- -<1> the name of the application(s) for which the cache should be cleared -<2> the list of nodes that the cache was cleared on +<1> the list of nodes that the cache was cleared on From ac6c39f8beb3f496832074b355df868358c29b6e Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 2 Jun 2020 23:56:02 +1000 Subject: [PATCH 40/45] Address feedback --- .../authz/store/NativePrivilegeStore.java | 156 +++++++++--------- .../store/NativePrivilegeStoreTests.java | 15 ++ 2 files changed, 93 insertions(+), 78 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 96c8f8a17b2e1..c1a6e954e52b8 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -203,27 +203,61 @@ private void innerGetPrivileges(Collection applications, ActionListener< } } - public void onSecurityIndexStateChange(SecurityIndexManager.State previousState, SecurityIndexManager.State currentState) { - if (isMoveFromRedToNonRed(previousState, currentState) || isIndexDeleted(previousState, currentState) - || previousState.isIndexUpToDate != currentState.isIndexUpToDate) { - invalidateAll(); + private QueryBuilder getApplicationNameQuery(Collection applications) { + if (applications.contains("*")) { + return QueryBuilders.existsQuery(APPLICATION.getPreferredName()); + } + final List rawNames = new ArrayList<>(applications.size()); + final List wildcardNames = new ArrayList<>(applications.size()); + for (String name : applications) { + if (name.endsWith("*")) { + wildcardNames.add(name); + } else { + rawNames.add(name); + } } - } - public void invalidate(Collection updatedApplicationNames) { - logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); - numInvalidation.incrementAndGet(); - final Set uniqueNames = Set.copyOf(updatedApplicationNames); - // Always completely invalidate application names cache due to wildcard - applicationNamesCache.invalidateAll(); - uniqueNames.forEach(descriptorsCache::invalidate); + assert rawNames.isEmpty() == false || wildcardNames.isEmpty() == false; + + TermsQueryBuilder termsQuery = rawNames.isEmpty() ? null : QueryBuilders.termsQuery(APPLICATION.getPreferredName(), rawNames); + if (wildcardNames.isEmpty()) { + return termsQuery; + } + final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery(); + if (termsQuery != null) { + boolQuery.should(termsQuery); + } + for (String wildcard : wildcardNames) { + final String prefix = wildcard.substring(0, wildcard.length() - 1); + boolQuery.should(QueryBuilders.prefixQuery(APPLICATION.getPreferredName(), prefix)); + } + boolQuery.minimumShouldMatch(1); + return boolQuery; } - public void invalidateAll() { - logger.debug("Invalidating all application privileges caches"); - numInvalidation.incrementAndGet(); - applicationNamesCache.invalidateAll(); - descriptorsCache.invalidateAll(); + private ApplicationPrivilegeDescriptor buildPrivilege(String docId, BytesReference source) { + logger.trace("Building privilege from [{}] [{}]", docId, source == null ? "<>" : source.utf8ToString()); + if (source == null) { + return null; + } + final Tuple name = nameFromDocId(docId); + try { + // EMPTY is safe here because we never use namedObject + + try (StreamInput input = source.streamInput(); + XContentParser parser = XContentType.JSON.xContent().createParser(NamedXContentRegistry.EMPTY, + LoggingDeprecationHandler.INSTANCE, input)) { + final ApplicationPrivilegeDescriptor privilege = ApplicationPrivilegeDescriptor.parse(parser, null, null, true); + assert privilege.getApplication().equals(name.v1()) + : "Incorrect application name for privilege. Expected [" + name.v1() + "] but was " + privilege.getApplication(); + assert privilege.getName().equals(name.v2()) + : "Incorrect name for application privilege. Expected [" + name.v2() + "] but was " + privilege.getName(); + return privilege; + } + } catch (IOException | XContentParseException e) { + logger.error(new ParameterizedMessage("cannot parse application privilege [{}]", name), e); + return null; + } } /** @@ -263,42 +297,6 @@ private Collection filterDescriptorsForPrivilege return descriptors.stream().filter(d -> privilegeNames.contains(d.getName())).collect(Collectors.toUnmodifiableSet()); } - private QueryBuilder getApplicationNameQuery(Collection applications) { - if (applications.contains("*")) { - return QueryBuilders.existsQuery(APPLICATION.getPreferredName()); - } - final List rawNames = new ArrayList<>(applications.size()); - final List wildcardNames = new ArrayList<>(applications.size()); - for (String name : applications) { - if (name.endsWith("*")) { - wildcardNames.add(name); - } else { - rawNames.add(name); - } - } - - assert rawNames.isEmpty() == false || wildcardNames.isEmpty() == false; - - TermsQueryBuilder termsQuery = rawNames.isEmpty() ? null : QueryBuilders.termsQuery(APPLICATION.getPreferredName(), rawNames); - if (wildcardNames.isEmpty()) { - return termsQuery; - } - final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery(); - if (termsQuery != null) { - boolQuery.should(termsQuery); - } - for (String wildcard : wildcardNames) { - final String prefix = wildcard.substring(0, wildcard.length() - 1); - boolQuery.should(QueryBuilders.prefixQuery(APPLICATION.getPreferredName(), prefix)); - } - boolQuery.minimumShouldMatch(1); - return boolQuery; - } - - private static boolean isEmpty(Collection collection) { - return collection == null || collection.isEmpty(); - } - public void putPrivileges(Collection privileges, WriteRequest.RefreshPolicy refreshPolicy, ActionListener>> listener) { securityIndexManager.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { @@ -384,31 +382,6 @@ public void onFailure(Exception e) { }); } - private ApplicationPrivilegeDescriptor buildPrivilege(String docId, BytesReference source) { - logger.trace("Building privilege from [{}] [{}]", docId, source == null ? "<>" : source.utf8ToString()); - if (source == null) { - return null; - } - final Tuple name = nameFromDocId(docId); - try { - // EMPTY is safe here because we never use namedObject - - try (StreamInput input = source.streamInput(); - XContentParser parser = XContentType.JSON.xContent().createParser(NamedXContentRegistry.EMPTY, - LoggingDeprecationHandler.INSTANCE, input)) { - final ApplicationPrivilegeDescriptor privilege = ApplicationPrivilegeDescriptor.parse(parser, null, null, true); - assert privilege.getApplication().equals(name.v1()) - : "Incorrect application name for privilege. Expected [" + name.v1() + "] but was " + privilege.getApplication(); - assert privilege.getName().equals(name.v2()) - : "Incorrect name for application privilege. Expected [" + name.v2() + "] but was " + privilege.getName(); - return privilege; - } - } catch (IOException | XContentParseException e) { - logger.error(new ParameterizedMessage("cannot parse application privilege [{}]", name), e); - return null; - } - } - private static Tuple nameFromDocId(String docId) { final String name = docId.substring(DOC_TYPE_VALUE.length() + 1); assert name != null && name.length() > 0 : "Invalid name '" + name + "'"; @@ -421,6 +394,33 @@ private static String toDocId(String application, String name) { return DOC_TYPE_VALUE + "_" + application + ":" + name; } + public void onSecurityIndexStateChange(SecurityIndexManager.State previousState, SecurityIndexManager.State currentState) { + if (isMoveFromRedToNonRed(previousState, currentState) || isIndexDeleted(previousState, currentState) + || previousState.isIndexUpToDate != currentState.isIndexUpToDate) { + invalidateAll(); + } + } + + public void invalidate(Collection updatedApplicationNames) { + logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); + numInvalidation.incrementAndGet(); + final Set uniqueNames = Set.copyOf(updatedApplicationNames); + // Always completely invalidate application names cache due to wildcard + applicationNamesCache.invalidateAll(); + uniqueNames.forEach(descriptorsCache::invalidate); + } + + public void invalidateAll() { + logger.debug("Invalidating all application privileges caches"); + numInvalidation.incrementAndGet(); + applicationNamesCache.invalidateAll(); + descriptorsCache.invalidateAll(); + } + + private static boolean isEmpty(Collection collection) { + return collection == null || collection.isEmpty(); + } + // Package private for tests Cache, Set> getApplicationNamesCache() { return applicationNamesCache; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index 276abd39896b7..a0c8fa3f0397e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -51,7 +51,9 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; @@ -142,6 +144,19 @@ public void testGetSinglePrivilegeByName() throws Exception { assertResult(sourcePrivileges, future); } + public void testGetMissingPrivilege() throws InterruptedException, ExecutionException, TimeoutException { + final PlainActionFuture> future = new PlainActionFuture<>(); + store.getPrivileges(List.of("myapp"), List.of("admin"), future); + final SearchHit[] hits = new SearchHit[0]; + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null)); + + final Collection applicationPrivilegeDescriptors = future.get(1, TimeUnit.SECONDS); + assertThat(applicationPrivilegeDescriptors.size(), equalTo(0)); + } + public void testGetPrivilegesByApplicationName() throws Exception { final List sourcePrivileges = Arrays.asList( new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()), From 21c89fba7a493461dca8a8551fb7dc3d54e96a67 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 2 Jun 2020 23:58:25 +1000 Subject: [PATCH 41/45] Remove todo as it is pointless --- .../xpack/security/authz/store/NativePrivilegeStore.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index c1a6e954e52b8..57d9ec4461c2c 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -149,7 +149,6 @@ public void getPrivileges(Collection applications, Collection na final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() .collect(Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); // Avoid caching potential stale results. - // TODO: It is still possible that cache gets invalidated immediately after the if check if (invalidationCounter == numInvalidation.get()) { final Set fetchedApplicationNames = Collections.unmodifiableSet(mapOfFetchedDescriptors.keySet()); // Do not cache the names if expansion has no effect From 1499a26c051cae85fc78aa80c33c6bc5fa7abfdd Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 3 Jun 2020 23:10:21 +1000 Subject: [PATCH 42/45] Address feedback for readwritelock and cache invalidation --- .../authz/store/NativePrivilegeStore.java | 53 ++++++++++++------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 57d9ec4461c2c..6e714c973986f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.CollectionUtils; +import org.elasticsearch.common.util.concurrent.ReleasableLock; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -56,6 +57,8 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; import java.util.stream.Collector; import java.util.stream.Collectors; @@ -98,6 +101,10 @@ public class NativePrivilegeStore { private final Cache> descriptorsCache; private final Cache, Set> applicationNamesCache; private final AtomicLong numInvalidation = new AtomicLong(); + private final ReadWriteLock invalidationLock = new ReentrantReadWriteLock(); + private final ReleasableLock invalidationReadLock = new ReleasableLock(invalidationLock.readLock()); + private final ReleasableLock invalidationWriteLock = new ReleasableLock(invalidationLock.writeLock()); + public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManager securityIndexManager) { this.settings = settings; @@ -148,17 +155,21 @@ public void getPrivileges(Collection applications, Collection na innerGetPrivileges(applicationNamesCacheKey, ActionListener.wrap(fetchedDescriptors -> { final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() .collect(Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); - // Avoid caching potential stale results. - if (invalidationCounter == numInvalidation.get()) { - final Set fetchedApplicationNames = Collections.unmodifiableSet(mapOfFetchedDescriptors.keySet()); - // Do not cache the names if expansion has no effect - if (fetchedApplicationNames.equals(applicationNamesCacheKey) == false) { - logger.debug("Caching application names query: {} = {}", applicationNamesCacheKey, fetchedApplicationNames); - applicationNamesCache.put(applicationNamesCacheKey, fetchedApplicationNames); - } - for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { - logger.debug("Caching descriptors for application: {}", entry.getKey()); - descriptorsCache.put(entry.getKey(), entry.getValue()); + // Use RWLock and atomic counter to: + // 1. Avoid caching potential stale results as much as possible + // 2. If stale results are cached, ensure they will be invalidated as soon as possible + try (ReleasableLock ignored = invalidationReadLock.acquire()) { + if (invalidationCounter == numInvalidation.get()) { + final Set fetchedApplicationNames = Collections.unmodifiableSet(mapOfFetchedDescriptors.keySet()); + // Do not cache the names if expansion has no effect + if (fetchedApplicationNames.equals(applicationNamesCacheKey) == false) { + logger.debug("Caching application names query: {} = {}", applicationNamesCacheKey, fetchedApplicationNames); + applicationNamesCache.put(applicationNamesCacheKey, fetchedApplicationNames); + } + for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { + logger.debug("Caching descriptors for application: {}", entry.getKey()); + descriptorsCache.put(entry.getKey(), entry.getValue()); + } } } listener.onResponse(filterDescriptorsForPrivilegeNames(fetchedDescriptors, names)); @@ -402,18 +413,22 @@ public void onSecurityIndexStateChange(SecurityIndexManager.State previousState, public void invalidate(Collection updatedApplicationNames) { logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); - numInvalidation.incrementAndGet(); - final Set uniqueNames = Set.copyOf(updatedApplicationNames); - // Always completely invalidate application names cache due to wildcard - applicationNamesCache.invalidateAll(); - uniqueNames.forEach(descriptorsCache::invalidate); + try (ReleasableLock ignored = invalidationWriteLock.acquire()) { + numInvalidation.incrementAndGet(); + final Set uniqueNames = Set.copyOf(updatedApplicationNames); + // Always completely invalidate application names cache due to wildcard + applicationNamesCache.invalidateAll(); + uniqueNames.forEach(descriptorsCache::invalidate); + } } public void invalidateAll() { logger.debug("Invalidating all application privileges caches"); - numInvalidation.incrementAndGet(); - applicationNamesCache.invalidateAll(); - descriptorsCache.invalidateAll(); + try (ReleasableLock ignored = invalidationWriteLock.acquire()) { + numInvalidation.incrementAndGet(); + applicationNamesCache.invalidateAll(); + descriptorsCache.invalidateAll(); + } } private static boolean isEmpty(Collection collection) { From 83a0e196d5e3b39a70a30c9c4f3df800f77ee19c Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 4 Jun 2020 18:47:19 +1000 Subject: [PATCH 43/45] add tests for caching behaviour --- .../authz/store/NativePrivilegeStore.java | 26 ++++--- .../store/NativePrivilegeStoreTests.java | 75 ++++++++++++++++++- 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 6e714c973986f..d0a332434b61d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -160,16 +160,7 @@ public void getPrivileges(Collection applications, Collection na // 2. If stale results are cached, ensure they will be invalidated as soon as possible try (ReleasableLock ignored = invalidationReadLock.acquire()) { if (invalidationCounter == numInvalidation.get()) { - final Set fetchedApplicationNames = Collections.unmodifiableSet(mapOfFetchedDescriptors.keySet()); - // Do not cache the names if expansion has no effect - if (fetchedApplicationNames.equals(applicationNamesCacheKey) == false) { - logger.debug("Caching application names query: {} = {}", applicationNamesCacheKey, fetchedApplicationNames); - applicationNamesCache.put(applicationNamesCacheKey, fetchedApplicationNames); - } - for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { - logger.debug("Caching descriptors for application: {}", entry.getKey()); - descriptorsCache.put(entry.getKey(), entry.getValue()); - } + cacheFetchedDescriptors(applicationNamesCacheKey, mapOfFetchedDescriptors); } } listener.onResponse(filterDescriptorsForPrivilegeNames(fetchedDescriptors, names)); @@ -307,6 +298,21 @@ private Collection filterDescriptorsForPrivilege return descriptors.stream().filter(d -> privilegeNames.contains(d.getName())).collect(Collectors.toUnmodifiableSet()); } + // protected for tests + protected void cacheFetchedDescriptors(Set applicationNamesCacheKey, + Map> mapOfFetchedDescriptors) { + final Set fetchedApplicationNames = Collections.unmodifiableSet(mapOfFetchedDescriptors.keySet()); + // Do not cache the names if expansion has no effect + if (fetchedApplicationNames.equals(applicationNamesCacheKey) == false) { + logger.debug("Caching application names query: {} = {}", applicationNamesCacheKey, fetchedApplicationNames); + applicationNamesCache.put(applicationNamesCacheKey, fetchedApplicationNames); + } + for (Map.Entry> entry : mapOfFetchedDescriptors.entrySet()) { + logger.debug("Caching descriptors for application: {}", entry.getKey()); + descriptorsCache.put(entry.getKey(), entry.getValue()); + } + } + public void putPrivileges(Collection privileges, WriteRequest.RefreshPolicy refreshPolicy, ActionListener>> listener) { securityIndexManager.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index a0c8fa3f0397e..1e90621089ebe 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -51,6 +51,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -83,6 +84,7 @@ public class NativePrivilegeStoreTests extends ESTestCase { private List requests; private AtomicReference listener; private Client client; + private SecurityIndexManager securityIndex; @Before public void setup() { @@ -96,7 +98,7 @@ void doExecute(ActionType action, Request request, ActionListener sourcePrivileges = singletonList( + new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()) + ); + + final PlainActionFuture> future = new PlainActionFuture<>(); + store.getPrivileges(null, null, future); + + // Before the results can be cached, invalidate the cache to simulate stale search results + store.invalidateAll(); + final SearchHit[] hits = buildHits(sourcePrivileges); + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null)); + + // Nothing should be cached since the results are stale + assertEquals(0, store.getApplicationNamesCache().count()); + assertEquals(0, store.getDescriptorsCache().count()); + } + + public void testWhenStaleResultsAreCachedTheyWillBeCleared() throws InterruptedException { + final List sourcePrivileges = singletonList( + new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()) + ); + + final CountDownLatch getPrivilegeCountDown = new CountDownLatch(1); + final CountDownLatch invalidationCountDown = new CountDownLatch(1); + // Use subclass so we can put the caching process on hold, which allows time to fire the cache invalidation call + // When the process reaches the overridden method, it already acquires the read lock. + // Hence the cache invalidation will be block at acquiring the write lock. + // This simulates the scenario when stale results are cached just before the invalidation call arrives. + // In this case, we guarantee the cache will be invalidate and the stale results won't stay for long. + final NativePrivilegeStore store1 = new NativePrivilegeStore(Settings.EMPTY, client, securityIndex) { + @Override + protected void cacheFetchedDescriptors( + Set applicationNamesCacheKey, Map> mapOfFetchedDescriptors) { + getPrivilegeCountDown.countDown(); + try { + // wait till the invalidation call is at the door step + invalidationCountDown.await(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + super.cacheFetchedDescriptors(applicationNamesCacheKey, mapOfFetchedDescriptors); + // Assert that cache is successful + assertEquals(1, getApplicationNamesCache().count()); + assertEquals(1, getDescriptorsCache().count()); + } + }; + final PlainActionFuture> future = new PlainActionFuture<>(); + store1.getPrivileges(null, null, future); + final SearchHit[] hits = buildHits(sourcePrivileges); + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null)); + + // Make sure the caching is about to happen + getPrivilegeCountDown.await(5, TimeUnit.SECONDS); + // Fire the invalidation call in another thread + new Thread(() -> { + // Let the caching proceed + invalidationCountDown.countDown(); + store.invalidateAll(); + }).start(); + // The cache should be cleared + assertEquals(0, store.getApplicationNamesCache().count()); + assertEquals(0, store.getDescriptorsCache().count()); + } + public void testPutPrivileges() throws Exception { final List putPrivileges = Arrays.asList( new ApplicationPrivilegeDescriptor("app1", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()), From 9364e36f80aa3d4f8c2a14267a4abddf032d3dba Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 15 Jun 2020 11:56:41 +1000 Subject: [PATCH 44/45] Add TTL as a safety net. Also consolidate cache size to a single setting --- .../xpack/security/Security.java | 2 + .../authz/store/NativePrivilegeStore.java | 55 ++++++++++++------- .../store/NativePrivilegeStoreCacheTests.java | 10 ++++ .../store/NativePrivilegeStoreTests.java | 26 +++++++++ 4 files changed, 73 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 9a04325b2dce7..920e2363c2d85 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -646,6 +646,8 @@ public static List> getSettings(List securityExten settingsList.add(ApiKeyService.CACHE_HASH_ALGO_SETTING); settingsList.add(ApiKeyService.CACHE_MAX_KEYS_SETTING); settingsList.add(ApiKeyService.CACHE_TTL_SETTING); + settingsList.add(NativePrivilegeStore.CACHE_MAX_APPLICATIONS_SETTING); + settingsList.add(NativePrivilegeStore.CACHE_TTL_SETTING); // hide settings settingsList.add(Setting.listSetting(SecurityField.setting("hide_settings"), Collections.emptyList(), Function.identity(), diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index d0a332434b61d..f782d22f7cc19 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -26,6 +26,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.util.concurrent.ReleasableLock; import org.elasticsearch.common.util.concurrent.ThreadContext; @@ -79,13 +80,13 @@ */ public class NativePrivilegeStore { - private static final Setting DESCRIPTOR_CACHE_SIZE_SETTING = + + public static final Setting CACHE_MAX_APPLICATIONS_SETTING = Setting.intSetting("xpack.security.authz.store.privileges.cache.max_size", 10_000, Setting.Property.NodeScope); - private static final Setting APPLICATION_NAME_CACHE_SIZE_SETTING = - Setting.intSetting("xpack.security.authz.store.privileges.application_name.cache.max_size", - 10_000, Setting.Property.NodeScope); + public static final Setting CACHE_TTL_SETTING = Setting.timeSetting("xpack.security.authz.store.privileges.cache.ttl", + TimeValue.timeValueHours(24L), Setting.Property.NodeScope); private static final Collector, ?, Map>> TUPLES_TO_MAP = Collectors.toMap( Tuple::v1, @@ -110,21 +111,24 @@ public NativePrivilegeStore(Settings settings, Client client, SecurityIndexManag this.settings = settings; this.client = client; this.securityIndexManager = securityIndexManager; - CacheBuilder> builder = CacheBuilder.builder(); - final int cacheSize = DESCRIPTOR_CACHE_SIZE_SETTING.get(settings); - if (cacheSize >= 0) { - builder.setMaximumWeight(cacheSize); - builder.weigher((k, v) -> v.size()); - } - descriptorsCache = builder.build(); - - CacheBuilder, Set> applicationNamesCacheBuilder = CacheBuilder.builder(); - final int nameCacheSize = APPLICATION_NAME_CACHE_SIZE_SETTING.get(settings); - if (nameCacheSize >= 0) { - applicationNamesCacheBuilder.setMaximumWeight(nameCacheSize); - applicationNamesCacheBuilder.weigher((k, v) -> k.size() + v.size()); + final TimeValue ttl = CACHE_TTL_SETTING.get(settings); + if (ttl.getNanos() > 0) { + final int cacheSize = CACHE_MAX_APPLICATIONS_SETTING.get(settings); + descriptorsCache = CacheBuilder.>builder() + .setMaximumWeight(cacheSize) + .weigher((k, v) -> v.size()) + .setExpireAfterWrite(ttl).build(); + applicationNamesCache = CacheBuilder., Set>builder() + .setMaximumWeight(cacheSize) + .weigher((k, v) -> k.size() + v.size()) + .setExpireAfterWrite(ttl).build(); + } else { + descriptorsCache = null; + applicationNamesCache = null; } - applicationNamesCache = applicationNamesCacheBuilder.build(); + assert (descriptorsCache == null && applicationNamesCache == null) + || (descriptorsCache != null && applicationNamesCache != null) + : "descriptor and application names cache must be enabled or disabled together"; } public void getPrivileges(Collection applications, Collection names, @@ -136,7 +140,7 @@ public void getPrivileges(Collection applications, Collection na // Always fetch for the concrete application names even when the passed-in application names has no wildcard. // This serves as a negative lookup, i.e. when a passed-in non-wildcard application does not exist. - Set concreteApplicationNames = applicationNamesCache.get(applicationNamesCacheKey); + Set concreteApplicationNames = applicationNamesCache == null ? null : applicationNamesCache.get(applicationNamesCacheKey); if (concreteApplicationNames != null && concreteApplicationNames.size() == 0) { logger.debug("returning empty application privileges for [{}] as application names result in empty list", @@ -269,7 +273,9 @@ private ApplicationPrivilegeDescriptor buildPrivilege(String docId, BytesReferen * name, this means any wildcard will result in null. */ private Set cachedDescriptorsForApplicationNames(Set applicationNames) { - + if (descriptorsCache == null) { + return null; + } final Set cachedDescriptors = new HashSet<>(); for (String applicationName: applicationNames) { if (applicationName.endsWith("*")) { @@ -301,6 +307,9 @@ private Collection filterDescriptorsForPrivilege // protected for tests protected void cacheFetchedDescriptors(Set applicationNamesCacheKey, Map> mapOfFetchedDescriptors) { + if (descriptorsCache == null) { + return; + } final Set fetchedApplicationNames = Collections.unmodifiableSet(mapOfFetchedDescriptors.keySet()); // Do not cache the names if expansion has no effect if (fetchedApplicationNames.equals(applicationNamesCacheKey) == false) { @@ -418,6 +427,9 @@ public void onSecurityIndexStateChange(SecurityIndexManager.State previousState, } public void invalidate(Collection updatedApplicationNames) { + if (descriptorsCache == null) { + return; + } logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); try (ReleasableLock ignored = invalidationWriteLock.acquire()) { numInvalidation.incrementAndGet(); @@ -429,6 +441,9 @@ public void invalidate(Collection updatedApplicationNames) { } public void invalidateAll() { + if (descriptorsCache == null) { + return; + } logger.debug("Invalidating all application privileges caches"); try (ReleasableLock ignored = invalidationWriteLock.acquire()) { numInvalidation.incrementAndGet(); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java index fe71467f8ef65..84808b04473cd 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreCacheTests.java @@ -15,6 +15,7 @@ import org.elasticsearch.action.delete.DeleteResponse; import org.elasticsearch.client.Client; import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.SecuritySingleNodeTestCase; import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction; @@ -88,6 +89,15 @@ protected String configUsersRoles() { + TEST_ROLE + ":" + APP_USER_NAME + "\n"; } + @Override + protected Settings nodeSettings() { + Settings.Builder builder = Settings.builder().put(super.nodeSettings()); + // Ensure the new settings can be configured + builder.put("xpack.security.authz.store.privileges.cache.max_size", 5000); + builder.put("xpack.security.authz.store.privileges.cache.ttl", "12h"); + return builder.build(); + } + @Before public void configureApplicationPrivileges() { final List applicationPrivilegeDescriptors = Arrays.asList( diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index 1e90621089ebe..ab02a55e6df7c 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -579,6 +579,32 @@ public void testCacheClearOnIndexHealthChange() { assertEquals(++count, store.getNumInvalidation().get()); } + public void testCacheWillBeDisabledWhenTtlIsZero() { + final Settings settings = Settings.builder().put("xpack.security.authz.store.privileges.cache.ttl", 0).build(); + final NativePrivilegeStore store1 = new NativePrivilegeStore(settings, client, securityIndex); + assertNull(store1.getApplicationNamesCache()); + assertNull(store1.getDescriptorsCache()); + } + public void testGetPrivilegesWorkWithoutCache() throws Exception { + final Settings settings = Settings.builder().put("xpack.security.authz.store.privileges.cache.ttl", 0).build(); + final NativePrivilegeStore store1 = new NativePrivilegeStore(settings, client, securityIndex); + final List sourcePrivileges = Arrays.asList( + new ApplicationPrivilegeDescriptor("myapp", "admin", newHashSet("action:admin/*", "action:login", "data:read/*"), emptyMap()) + ); + final PlainActionFuture> future = new PlainActionFuture<>(); + store1.getPrivileges(singletonList("myapp"), null, future); + final SearchHit[] hits = buildHits(sourcePrivileges); + listener.get().onResponse(new SearchResponse(new SearchResponseSections( + new SearchHits(hits, new TotalHits(hits.length, TotalHits.Relation.EQUAL_TO), 0f), + null, null, false, false, null, 1), + "_scrollId1", 1, 1, 0, 1, null, null)); + + assertResult(sourcePrivileges, future); + // They are no-op but should "work" (pass-through) + store1.invalidate(singleton("myapp")); + store1.invalidateAll(); + } + private SecurityIndexManager.State dummyState( String concreteSecurityIndexName, boolean isIndexUpToDate, ClusterHealthStatus healthStatus) { return new SecurityIndexManager.State( From ae3c7842c5fe525c7bce92fe38dcfccec4e16c7c Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 29 Jun 2020 12:35:50 +1000 Subject: [PATCH 45/45] Address feedback about lock duration --- .../authz/store/NativePrivilegeStore.java | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index f782d22f7cc19..e46c5d3b5657d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -159,12 +159,14 @@ public void getPrivileges(Collection applications, Collection na innerGetPrivileges(applicationNamesCacheKey, ActionListener.wrap(fetchedDescriptors -> { final Map> mapOfFetchedDescriptors = fetchedDescriptors.stream() .collect(Collectors.groupingBy(ApplicationPrivilegeDescriptor::getApplication, Collectors.toUnmodifiableSet())); - // Use RWLock and atomic counter to: - // 1. Avoid caching potential stale results as much as possible - // 2. If stale results are cached, ensure they will be invalidated as soon as possible - try (ReleasableLock ignored = invalidationReadLock.acquire()) { - if (invalidationCounter == numInvalidation.get()) { - cacheFetchedDescriptors(applicationNamesCacheKey, mapOfFetchedDescriptors); + if (descriptorsCache != null) { + // Use RWLock and atomic counter to: + // 1. Avoid caching potential stale results as much as possible + // 2. If stale results are cached, ensure they will be invalidated as soon as possible + try (ReleasableLock ignored = invalidationReadLock.acquire()) { + if (invalidationCounter == numInvalidation.get()) { + cacheFetchedDescriptors(applicationNamesCacheKey, mapOfFetchedDescriptors); + } } } listener.onResponse(filterDescriptorsForPrivilegeNames(fetchedDescriptors, names)); @@ -307,9 +309,6 @@ private Collection filterDescriptorsForPrivilege // protected for tests protected void cacheFetchedDescriptors(Set applicationNamesCacheKey, Map> mapOfFetchedDescriptors) { - if (descriptorsCache == null) { - return; - } final Set fetchedApplicationNames = Collections.unmodifiableSet(mapOfFetchedDescriptors.keySet()); // Do not cache the names if expansion has no effect if (fetchedApplicationNames.equals(applicationNamesCacheKey) == false) { @@ -430,26 +429,31 @@ public void invalidate(Collection updatedApplicationNames) { if (descriptorsCache == null) { return; } - logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); + // Increment the invalidation count to avoid caching stale results + // Release the lock as soon as we increment the counter so the actual invalidation + // does not have to block other threads. In theory, there could be very rare edge + // cases that we would invalidate more than necessary. But we consider less locking + // duration is overall better. try (ReleasableLock ignored = invalidationWriteLock.acquire()) { numInvalidation.incrementAndGet(); - final Set uniqueNames = Set.copyOf(updatedApplicationNames); - // Always completely invalidate application names cache due to wildcard - applicationNamesCache.invalidateAll(); - uniqueNames.forEach(descriptorsCache::invalidate); } + logger.debug("Invalidating application privileges caches for: {}", updatedApplicationNames); + final Set uniqueNames = Set.copyOf(updatedApplicationNames); + // Always completely invalidate application names cache due to wildcard + applicationNamesCache.invalidateAll(); + uniqueNames.forEach(descriptorsCache::invalidate); } public void invalidateAll() { if (descriptorsCache == null) { return; } - logger.debug("Invalidating all application privileges caches"); try (ReleasableLock ignored = invalidationWriteLock.acquire()) { numInvalidation.incrementAndGet(); - applicationNamesCache.invalidateAll(); - descriptorsCache.invalidateAll(); } + logger.debug("Invalidating all application privileges caches"); + applicationNamesCache.invalidateAll(); + descriptorsCache.invalidateAll(); } private static boolean isEmpty(Collection collection) {