From 897d2e8a023fb5a5e8190c5c46d82b48bc4a9263 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 22 Sep 2020 11:49:40 +1000 Subject: [PATCH] Fix ccs permission for search with a scroll id (#62053) (#62695) CCS with remote indices only does not require any privileges on the local cluster. This PR ensures that search with scroll follow the permission model. --- .../action/search/ParsedScrollId.java | 8 +++- .../action/search/SearchScrollRequest.java | 4 ++ .../action/search/ParsedScrollIdTests.java | 41 +++++++++++++++++++ .../xpack/security/authz/RBACEngine.java | 13 +++++- .../authz/AuthorizationServiceTests.java | 32 ++++++++++++++- .../test/multi_cluster/40_scroll.yml | 11 ++--- 6 files changed, 98 insertions(+), 11 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/action/search/ParsedScrollIdTests.java diff --git a/server/src/main/java/org/elasticsearch/action/search/ParsedScrollId.java b/server/src/main/java/org/elasticsearch/action/search/ParsedScrollId.java index 43ae3966960..fbde7fffca8 100644 --- a/server/src/main/java/org/elasticsearch/action/search/ParsedScrollId.java +++ b/server/src/main/java/org/elasticsearch/action/search/ParsedScrollId.java @@ -19,7 +19,9 @@ package org.elasticsearch.action.search; -class ParsedScrollId { +import java.util.Arrays; + +public class ParsedScrollId { public static final String QUERY_THEN_FETCH_TYPE = "queryThenFetch"; @@ -48,4 +50,8 @@ class ParsedScrollId { public SearchContextIdForNode[] getContext() { return context; } + + public boolean hasLocalIndices() { + return Arrays.stream(context).anyMatch(c -> c.getClusterAlias() == null); + } } diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchScrollRequest.java b/server/src/main/java/org/elasticsearch/action/search/SearchScrollRequest.java index 7a320d81f2d..e49485a167f 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchScrollRequest.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchScrollRequest.java @@ -83,6 +83,10 @@ public class SearchScrollRequest extends ActionRequest implements ToXContentObje return this; } + public ParsedScrollId parseScrollId() { + return TransportSearchHelper.parseScrollId(scrollId); + } + /** * If set, will enable scrolling of the search request. */ diff --git a/server/src/test/java/org/elasticsearch/action/search/ParsedScrollIdTests.java b/server/src/test/java/org/elasticsearch/action/search/ParsedScrollIdTests.java new file mode 100644 index 00000000000..789638acde9 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/action/search/ParsedScrollIdTests.java @@ -0,0 +1,41 @@ +/* + * 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.action.search; + +import org.elasticsearch.search.internal.ShardSearchContextId; +import org.elasticsearch.test.ESTestCase; + +public class ParsedScrollIdTests extends ESTestCase { + public void testHasLocalIndices() { + final int nResults = randomIntBetween(1, 3); + final SearchContextIdForNode[] searchContextIdForNodes = new SearchContextIdForNode[nResults]; + + boolean hasLocal = false; + for (int i = 0; i < nResults; i++) { + String clusterAlias = randomBoolean() ? randomAlphaOfLength(8) : null; + hasLocal = hasLocal || (clusterAlias == null); + searchContextIdForNodes[i] = + new SearchContextIdForNode(clusterAlias, "node_" + i, new ShardSearchContextId(randomAlphaOfLength(8), randomLong())); + } + final ParsedScrollId parsedScrollId = new ParsedScrollId(randomAlphaOfLength(8), randomAlphaOfLength(8), searchContextIdForNodes); + + assertEquals(hasLocal, parsedScrollId.hasLocalIndices()); + } +} 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 050fce8b931..aeba5de9688 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 @@ -12,6 +12,7 @@ import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.lucene.util.automaton.Automaton; import org.apache.lucene.util.automaton.Operations; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionRunnable; import org.elasticsearch.action.CompositeIndicesRequest; import org.elasticsearch.action.IndicesRequest; import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest; @@ -20,6 +21,7 @@ import org.elasticsearch.action.bulk.BulkShardRequest; import org.elasticsearch.action.delete.DeleteAction; import org.elasticsearch.action.get.MultiGetAction; import org.elasticsearch.action.index.IndexAction; +import org.elasticsearch.action.search.SearchScrollRequest; import org.elasticsearch.xpack.core.search.action.ClosePointInTimeAction; import org.elasticsearch.action.search.ClearScrollAction; import org.elasticsearch.action.search.MultiSearchAction; @@ -262,7 +264,16 @@ public class RBACEngine implements AuthorizationEngine { // index and if they cannot, we can fail the request early before we allow the execution of the action and in // turn the shard actions if (SearchScrollAction.NAME.equals(action)) { - authorizeIndexActionName(action, authorizationInfo, null, listener); + ActionRunnable.supply( + ActionListener.wrap(parsedScrollId -> { + if (parsedScrollId.hasLocalIndices()) { + authorizeIndexActionName(action, authorizationInfo, null, listener); + } else { + listener.onResponse(new IndexAuthorizationResult(true, null)); + } + }, listener::onFailure), + ((SearchScrollRequest) request)::parseScrollId + ).run(); } else { // RBACEngine simply authorizes scroll related actions without filling in any DLS/FLS permissions. // Scroll related actions have special security logic, where the security context of the initial search diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java index 28a2ae3d6b8..9a81987f51e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java @@ -56,6 +56,7 @@ import org.elasticsearch.action.search.ClearScrollAction; import org.elasticsearch.action.search.ClearScrollRequest; import org.elasticsearch.action.search.MultiSearchAction; import org.elasticsearch.action.search.MultiSearchRequest; +import org.elasticsearch.action.search.ParsedScrollId; import org.elasticsearch.action.search.SearchAction; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.SearchScrollAction; @@ -469,6 +470,32 @@ public class AuthorizationServiceTests extends ESTestCase { verifyNoMoreInteractions(auditTrail); } + public void testUserWithNoRolesPerformsRemoteSearchWithScroll() { + final ParsedScrollId parsedScrollId = mock(ParsedScrollId.class); + final SearchScrollRequest searchScrollRequest = mock(SearchScrollRequest.class); + when(searchScrollRequest.parseScrollId()).thenReturn(parsedScrollId); + final Authentication authentication = createAuthentication(new User("test user")); + mockEmptyMetadata(); + final String requestId = AuditUtil.getOrGenerateRequestId(threadContext); + for (final boolean hasLocalIndices: org.elasticsearch.common.collect.List.of(true, false)) { + when(parsedScrollId.hasLocalIndices()).thenReturn(hasLocalIndices); + if (hasLocalIndices) { + assertThrowsAuthorizationException( + () -> authorize(authentication, SearchScrollAction.NAME, searchScrollRequest), + "indices:data/read/scroll", "test user" + ); + verify(auditTrail).accessDenied(eq(requestId), eq(authentication), + eq("indices:data/read/scroll"), eq(searchScrollRequest), + authzInfoRoles(Role.EMPTY.names())); + } else { + authorize(authentication, SearchScrollAction.NAME, searchScrollRequest); + verify(auditTrail).accessGranted(eq(requestId), eq(authentication), eq(SearchScrollAction.NAME), eq(searchScrollRequest), + authzInfoRoles(Role.EMPTY.names())); + } + verifyNoMoreInteractions(auditTrail); + } + } + /** * This test mimics {@link #testUserWithNoRolesCanPerformRemoteSearch()} except that * while the referenced index _looks_ like a remote index, the remote cluster name has not @@ -659,7 +686,10 @@ public class AuthorizationServiceTests extends ESTestCase { verify(auditTrail).accessGranted(eq(requestId), eq(authentication), eq(ClearScrollAction.NAME), eq(clearScrollRequest), authzInfoRoles(new String[]{role.getName()})); - final SearchScrollRequest searchScrollRequest = new SearchScrollRequest(); + final ParsedScrollId parsedScrollId = mock(ParsedScrollId.class); + when(parsedScrollId.hasLocalIndices()).thenReturn(true); + final SearchScrollRequest searchScrollRequest = mock(SearchScrollRequest.class); + when(searchScrollRequest.parseScrollId()).thenReturn(parsedScrollId); authorize(authentication, SearchScrollAction.NAME, searchScrollRequest); verify(auditTrail).accessGranted(eq(requestId), eq(authentication), eq(SearchScrollAction.NAME), eq(searchScrollRequest), authzInfoRoles(new String[]{role.getName()})); diff --git a/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/40_scroll.yml b/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/40_scroll.yml index 0026df49780..1d67742a5d2 100644 --- a/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/40_scroll.yml +++ b/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/40_scroll.yml @@ -14,18 +14,13 @@ setup: "password": "s3krit", "roles" : [ "x_cluster_role" ] } - - do: + - do: # Remote indices only CCS does not require any privileges on the local cluster security.put_role: name: "x_cluster_role" body: > { - "cluster": ["all"], - "indices": [ - { - "names": ["local_index", "my_remote_cluster:test_i*", "my_remote_cluster:aliased_test_index", "test_remote_cluster:test_i*", "my_remote_cluster:secure_alias"], - "privileges": ["read"] - } - ] + "cluster": [], + "indices": [] } --- teardown: