diff --git a/docs/changelog/81237.yaml b/docs/changelog/81237.yaml new file mode 100644 index 0000000000000..14414f291481d --- /dev/null +++ b/docs/changelog/81237.yaml @@ -0,0 +1,5 @@ +pr: 81237 +summary: Avoid loading authorized indices when requested indices are all concrete names +area: Authorization +type: enhancement +issues: [] diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java index aeb2349160bc7..a793ad6630d4c 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java @@ -129,7 +129,7 @@ public class AuthorizationService { private final Set requestInterceptors; private final XPackLicenseState licenseState; private final OperatorPrivilegesService operatorPrivilegesService; - private final LoadAuthorizedIndicesTimeChecker.Factory authzIndicesTimerFactory; + private final boolean isAnonymousEnabled; private final boolean anonymousAuthzExceptionEnabled; @@ -155,13 +155,16 @@ public AuthorizationService( this.anonymousUser = anonymousUser; this.isAnonymousEnabled = AnonymousUser.isAnonymousEnabled(settings); this.anonymousAuthzExceptionEnabled = ANONYMOUS_AUTHORIZATION_EXCEPTION_SETTING.get(settings); - this.rbacEngine = new RBACEngine(settings, rolesStore); + this.rbacEngine = new RBACEngine( + settings, + rolesStore, + new LoadAuthorizedIndicesTimeChecker.Factory(logger, settings, clusterService.getClusterSettings()) + ); this.authorizationEngine = authorizationEngine == null ? this.rbacEngine : authorizationEngine; this.requestInterceptors = requestInterceptors; this.settings = settings; this.licenseState = licenseState; this.operatorPrivilegesService = operatorPrivilegesService; - this.authzIndicesTimerFactory = new LoadAuthorizedIndicesTimeChecker.Factory(logger, settings, clusterService.getClusterSettings()); } public void checkPrivileges( @@ -404,24 +407,15 @@ private void authorizeAction( }, clusterAuthzListener::onFailure)); } else if (isIndexAction(action)) { final Metadata metadata = clusterService.state().metadata(); - final AsyncSupplier> authorizedIndicesSupplier = new CachingAsyncSupplier<>(authzIndicesListener -> { - Consumer> timeChecker = authzIndicesTimerFactory.newTimer(requestInfo); - authzEngine.loadAuthorizedIndices( - requestInfo, - authzInfo, - metadata.getIndicesLookup(), - authzIndicesListener.map(authzIndices -> { - timeChecker.accept(authzIndices); - return authzIndices; - }) - ); - }); final AsyncSupplier resolvedIndicesAsyncSupplier = new CachingAsyncSupplier<>(resolvedIndicesListener -> { final ResolvedIndices resolvedIndices = indicesAndAliasesResolver.tryResolveWithoutWildcards(action, request); if (resolvedIndices != null) { resolvedIndicesListener.onResponse(resolvedIndices); } else { - authorizedIndicesSupplier.getAsync( + authzEngine.loadAuthorizedIndices( + requestInfo, + authzInfo, + metadata.getIndicesLookup(), ActionListener.wrap( authorizedIndices -> resolvedIndicesListener.onResponse( indicesAndAliasesResolver.resolve(action, request, metadata, authorizedIndices) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java index fc44d8efed683..df4e40524abb9 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java @@ -83,6 +83,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -90,7 +91,9 @@ import java.util.Objects; import java.util.Set; import java.util.TreeSet; +import java.util.function.Consumer; import java.util.function.Predicate; +import java.util.function.Supplier; import static org.elasticsearch.common.Strings.arrayToCommaDelimitedString; import static org.elasticsearch.xpack.security.action.user.TransportHasPrivilegesAction.getApplicationNames; @@ -114,10 +117,16 @@ public class RBACEngine implements AuthorizationEngine { private final CompositeRolesStore rolesStore; private final FieldPermissionsCache fieldPermissionsCache; + private final LoadAuthorizedIndicesTimeChecker.Factory authzIndicesTimerFactory; - public RBACEngine(Settings settings, CompositeRolesStore rolesStore) { + public RBACEngine( + Settings settings, + CompositeRolesStore rolesStore, + LoadAuthorizedIndicesTimeChecker.Factory authzIndicesTimerFactory + ) { this.rolesStore = rolesStore; this.fieldPermissionsCache = new FieldPermissionsCache(settings); + this.authzIndicesTimerFactory = authzIndicesTimerFactory; } @Override @@ -458,7 +467,9 @@ public void loadAuthorizedIndices( ) { if (authorizationInfo instanceof RBACAuthorizationInfo) { final Role role = ((RBACAuthorizationInfo) authorizationInfo).getRole(); - listener.onResponse(resolveAuthorizedIndicesFromRole(role, requestInfo, indicesLookup)); + listener.onResponse( + resolveAuthorizedIndicesFromRole(role, requestInfo, indicesLookup, () -> authzIndicesTimerFactory.newTimer(requestInfo)) + ); } else { listener.onFailure( new IllegalArgumentException("unsupported authorization info:" + authorizationInfo.getClass().getSimpleName()) @@ -659,34 +670,64 @@ GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole) { } static Set resolveAuthorizedIndicesFromRole(Role role, RequestInfo requestInfo, Map lookup) { + return resolveAuthorizedIndicesFromRole(role, requestInfo, lookup, () -> LoadAuthorizedIndicesTimeChecker.NO_OP_CONSUMER); + } + + static Set resolveAuthorizedIndicesFromRole( + Role role, + RequestInfo requestInfo, + Map lookup, + Supplier>> timerSupplier + ) { Predicate predicate = role.allowedIndicesMatcher(requestInfo.getAction()); // do not include data streams for actions that do not operate on data streams TransportRequest request = requestInfo.getRequest(); final boolean includeDataStreams = (request instanceof IndicesRequest) && ((IndicesRequest) request).includeDataStreams(); - Set indicesAndAliases = new HashSet<>(); - // TODO: can this be done smarter? I think there are usually more indices/aliases in the cluster then indices defined a roles? - if (includeDataStreams) { - for (IndexAbstraction indexAbstraction : lookup.values()) { - if (predicate.test(indexAbstraction)) { - indicesAndAliases.add(indexAbstraction.getName()); - if (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM) { - // add data stream and its backing indices for any authorized data streams - for (Index index : indexAbstraction.getIndices()) { - indicesAndAliases.add(index.getName()); + return new AuthorizedIndicesSet(() -> { + Consumer> timeChecker = timerSupplier.get(); + Set indicesAndAliases = new HashSet<>(); + // TODO: can this be done smarter? I think there are usually more indices/aliases in the cluster then indices defined a roles? + if (includeDataStreams) { + for (IndexAbstraction indexAbstraction : lookup.values()) { + if (predicate.test(indexAbstraction)) { + indicesAndAliases.add(indexAbstraction.getName()); + if (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM) { + // add data stream and its backing indices for any authorized data streams + for (Index index : indexAbstraction.getIndices()) { + indicesAndAliases.add(index.getName()); + } } } } + } else { + for (IndexAbstraction indexAbstraction : lookup.values()) { + if (indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM && predicate.test(indexAbstraction)) { + indicesAndAliases.add(indexAbstraction.getName()); + } + } } - } else { - for (IndexAbstraction indexAbstraction : lookup.values()) { - if (indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM && predicate.test(indexAbstraction)) { - indicesAndAliases.add(indexAbstraction.getName()); + timeChecker.accept(indicesAndAliases); + return indicesAndAliases; + }, name -> { + final IndexAbstraction indexAbstraction = lookup.get(name); + if (indexAbstraction == null) { + return false; + } + if (includeDataStreams) { + // We check the parent data stream first if there is one. For testing requested indices, this is most likely + // more efficient than checking the index name first because we recommend grant privileges over data stream + // instead of backing indices. + if (indexAbstraction.getParentDataStream() != null && predicate.test(indexAbstraction.getParentDataStream())) { + return true; + } else { + return predicate.test(indexAbstraction); } + } else { + return indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM && predicate.test(indexAbstraction); } - } - return Collections.unmodifiableSet(indicesAndAliases); + }); } private IndexAuthorizationResult buildIndicesAccessControl( @@ -813,4 +854,104 @@ private static boolean isAsyncRelatedAction(String action) { || action.equals(EqlAsyncActionNames.EQL_ASYNC_GET_RESULT_ACTION_NAME) || action.equals(SqlAsyncActionNames.SQL_ASYNC_GET_RESULT_ACTION_NAME); } + + /** + * A lazily loaded Set for authorized indices. It avoids loading the set if only contains check is required. + * It only loads the set if iterating through it is necessary, i.e. when expanding wildcards. + * + * NOTE that the lazy loading is NOT thread-safe and must NOT be used by multi-threads. + * The current usage has it wrapped inside a CachingAsyncSupplier which guarantees it to be accessed + * from a single thread. Extra caution is needed if moving or using this class in other places. + */ + static class AuthorizedIndicesSet implements Set { + + private final Supplier> supplier; + private final Predicate predicate; + private Set authorizedIndices = null; + + AuthorizedIndicesSet(Supplier> supplier, Predicate predicate) { + this.supplier = Objects.requireNonNull(supplier); + this.predicate = Objects.requireNonNull(predicate); + } + + private Set getAuthorizedIndices() { + if (authorizedIndices == null) { + authorizedIndices = supplier.get(); + } + return authorizedIndices; + } + + @Override + public int size() { + return getAuthorizedIndices().size(); + } + + @Override + public boolean isEmpty() { + return getAuthorizedIndices().isEmpty(); + } + + @Override + public boolean contains(Object o) { + if (authorizedIndices == null) { + return predicate.test((String) o); + } else { + return authorizedIndices.contains(o); + } + } + + @Override + public Iterator iterator() { + return getAuthorizedIndices().iterator(); + } + + @Override + public Object[] toArray() { + return getAuthorizedIndices().toArray(); + } + + @Override + public T[] toArray(T[] a) { + return getAuthorizedIndices().toArray(a); + } + + @Override + public boolean add(String s) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean remove(Object o) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean containsAll(Collection c) { + if (authorizedIndices == null) { + return c.stream().allMatch(this::contains); + } else { + return authorizedIndices.containsAll(c); + } + } + + @Override + public boolean addAll(Collection c) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean retainAll(Collection c) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean removeAll(Collection c) { + throw new UnsupportedOperationException(); + } + + @Override + public void clear() { + throw new UnsupportedOperationException(); + } + } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java index c16dba79c0d23..0aa5aeabb8d36 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java @@ -71,9 +71,11 @@ import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; import org.hamcrest.Matchers; import org.junit.Before; +import org.mockito.Mockito; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; @@ -81,6 +83,8 @@ import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.function.Predicate; +import java.util.function.Supplier; import java.util.stream.Collectors; import static java.util.Collections.emptyMap; @@ -97,7 +101,12 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.iterableWithSize; import static org.hamcrest.Matchers.notNullValue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -109,7 +118,9 @@ public class RBACEngineTests extends ESTestCase { @Before public void createEngine() { - engine = new RBACEngine(Settings.EMPTY, mock(CompositeRolesStore.class)); + final LoadAuthorizedIndicesTimeChecker.Factory timerFactory = mock(LoadAuthorizedIndicesTimeChecker.Factory.class); + when(timerFactory.newTimer(any())).thenReturn(LoadAuthorizedIndicesTimeChecker.NO_OP_CONSUMER); + engine = new RBACEngine(Settings.EMPTY, mock(CompositeRolesStore.class), timerFactory); } public void testSameUserPermission() { @@ -1407,6 +1418,8 @@ public void testBackingIndicesAreIncludedForAuthorizedDataStreams() { getRequestInfo(request, SearchAction.NAME), lookup ); + // The authorized indices is the lazily loading set implementation + assertThat(authorizedIndices, instanceOf(RBACEngine.AuthorizedIndicesSet.class)); assertThat(authorizedIndices, hasItem(dataStreamName)); assertThat( authorizedIndices, @@ -1449,6 +1462,8 @@ public void testExplicitMappingUpdatesAreNotGrantedWithIngestPrivileges() { getRequestInfo(request, PutMappingAction.NAME), lookup ); + // The authorized indices is the lazily loading set implementation + assertThat(authorizedIndices, instanceOf(RBACEngine.AuthorizedIndicesSet.class)); assertThat(authorizedIndices.isEmpty(), is(true)); } @@ -1459,6 +1474,54 @@ public void testNoInfiniteRecursionForRBACAuthorizationInfoHashCode() { new RBACAuthorizationInfo(role, null).hashCode(); } + @SuppressWarnings("unchecked") + public void testLazinessForAuthorizedIndicesSet() { + final Set authorizedNames = Set.of("foo", "bar", "baz"); + final HashSet allNames = new HashSet<>(authorizedNames); + allNames.addAll(Set.of("buzz", "fiz")); + + final Supplier> supplier = mock(Supplier.class); + when(supplier.get()).thenReturn(authorizedNames); + final Predicate predicate = mock(Predicate.class); + doAnswer(invocation -> { + final String name = (String) invocation.getArguments()[0]; + return authorizedNames.contains(name); + }).when(predicate).test(anyString()); + final RBACEngine.AuthorizedIndicesSet authorizedIndicesSet = new RBACEngine.AuthorizedIndicesSet(supplier, predicate); + + // Check with contains or containsAll do not trigger loading + final String name1 = randomFrom(allNames); + final String name2 = randomValueOtherThan(name1, () -> randomFrom(allNames)); + final boolean containsAll = randomBoolean(); + if (containsAll) { + assertThat(authorizedIndicesSet.containsAll(Set.of(name1, name2)), equalTo(authorizedNames.containsAll(Set.of(name1, name2)))); + } else { + assertThat(authorizedIndicesSet.contains(name1), equalTo(authorizedNames.contains(name1))); + } + verify(supplier, never()).get(); + verify(predicate, atLeastOnce()).test(anyString()); + + // Iterating through the set triggers loading + Mockito.clearInvocations(predicate); + final Set collectedNames = new HashSet<>(); + for (String name : authorizedIndicesSet) { + collectedNames.add(name); + } + verify(supplier).get(); + assertThat(collectedNames, equalTo(authorizedNames)); + + // Check with contains and containsAll again now uses the loaded set not the predicate anymore + Mockito.clearInvocations(supplier); + if (containsAll) { + assertThat(authorizedIndicesSet.containsAll(Set.of(name1, name2)), equalTo(authorizedNames.containsAll(Set.of(name1, name2)))); + } else { + assertThat(authorizedIndicesSet.contains(name1), equalTo(authorizedNames.contains(name1))); + } + verify(predicate, never()).test(anyString()); + // It also does not load twice + verify(supplier, never()).get(); + } + private GetUserPrivilegesResponse.Indices findIndexPrivilege(Set indices, String name) { return indices.stream().filter(i -> i.getIndices().contains(name)).findFirst().get(); }