From 89853df0447c35e3cc5620f6edd82de1f05c809e Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Fri, 8 Dec 2023 10:52:29 +0000 Subject: [PATCH] Restrict the index authorization checks in PIT search Search requests executed against a PIT do not have to set the target indices explicitly. Instead the list of targeted index is extracted from the PIT id directly. When executed against the search rest layer, the indices of the PIT are copied in the search request in order to fullfill the authorization check when security is enabled. However, this crucial copy operation is omitted when the search request is internally executed from a node within the cluster. Consequently, the authorization check ends up evaluating the request against all indices accessible to the user. To address this performance issue, the proposed change ensures this copy occurs explicitly within the security layer. This modification restricts the authorization check to indices that are part of the PIT, mitigating the performance bug. It is essential to note that even in the presence of this bug, the request does not route to all indices; only those present in the PIT are queried. Furthermore, this issue is not a security vulnerability, as the authorization check continues to limit the list of accessible indices to those permitted for the user. Closes #99684 --- .../action/search/SearchRequest.java | 17 +++++- .../rest/action/search/RestSearchAction.java | 32 ---------- .../action/search/SearchRequestTests.java | 37 ++++++++++++ .../execution/search/PITAwareQueryClient.java | 1 + .../security/authz/AuthorizationService.java | 27 ++------- .../authz/IndicesAndAliasesResolver.java | 20 +++++++ .../authz/AuthorizationServiceTests.java | 60 ++++++++----------- .../xpack/sql/execution/search/Querier.java | 1 + .../transforms/ClientTransformIndexer.java | 25 ++++---- 9 files changed, 120 insertions(+), 100 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java b/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java index 7ac8c4d5299d4..b1438030e3eed 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java @@ -407,6 +407,21 @@ public ActionRequestValidationException validate() { if (scroll) { validationException = addValidationError("using [point in time] is not allowed in a scroll context", validationException); } + if (indices().length > 0) { + validationException = addValidationError( + "[indices] cannot be used with point in time. Do not specify any index with point in time.", + validationException + ); + } + if (indicesOptions().equals(DEFAULT_INDICES_OPTIONS) == false) { + validationException = addValidationError("[indicesOptions] cannot be used with point in time", validationException); + } + if (routing() != null) { + validationException = addValidationError("[routing] cannot be used with point in time", validationException); + } + if (preference() != null) { + validationException = addValidationError("[preference] cannot be used with point in time", validationException); + } } else if (source != null && source.sorts() != null) { for (SortBuilder sortBuilder : source.sorts()) { if (sortBuilder instanceof FieldSortBuilder @@ -815,7 +830,7 @@ public SearchRequest rewrite(QueryRewriteContext ctx) throws IOException { // add a sort tiebreaker for PIT search requests if not explicitly set Object[] searchAfter = source.searchAfter(); if (source.pointInTimeBuilder() != null && source.sorts() != null && source.sorts().isEmpty() == false - // skip the tiebreaker if it is not provided in the search after values + // skip the tiebreaker if it is not provided in the search after values && (searchAfter == null || searchAfter.length == source.sorts().size() + 1)) { SortBuilder lastSort = source.sorts().get(source.sorts().size() - 1); if (lastSort instanceof FieldSortBuilder == false diff --git a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java index d8749f5e0fdb6..711aec182525e 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java @@ -11,7 +11,6 @@ import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.Version; import org.elasticsearch.action.ActionRequestValidationException; -import org.elasticsearch.action.search.SearchContextId; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.TransportSearchAction; import org.elasticsearch.action.support.IndicesOptions; @@ -50,7 +49,6 @@ import java.util.function.IntConsumer; import static org.elasticsearch.action.ValidateActions.addValidationError; -import static org.elasticsearch.action.search.SearchRequest.DEFAULT_INDICES_OPTIONS; import static org.elasticsearch.core.TimeValue.parseTimeValue; import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; @@ -376,41 +374,11 @@ static SuggestBuilder parseSuggestUrlParameters(RestRequest request) { static void preparePointInTime(SearchRequest request, RestRequest restRequest) { assert request.pointInTimeBuilder() != null; ActionRequestValidationException validationException = null; - if (request.indices().length > 0) { - validationException = addValidationError( - "[indices] cannot be used with point in time. Do not specify any index with point in time.", - validationException - ); - } - if (request.indicesOptions().equals(DEFAULT_INDICES_OPTIONS) == false) { - validationException = addValidationError("[indicesOptions] cannot be used with point in time", validationException); - } - if (request.routing() != null) { - validationException = addValidationError("[routing] cannot be used with point in time", validationException); - } - if (request.preference() != null) { - validationException = addValidationError("[preference] cannot be used with point in time", validationException); - } if (restRequest.paramAsBoolean("ccs_minimize_roundtrips", false)) { validationException = addValidationError("[ccs_minimize_roundtrips] cannot be used with point in time", validationException); request.setCcsMinimizeRoundtrips(false); } ExceptionsHelper.reThrowIfNotNull(validationException); - - final IndicesOptions indicesOptions = request.indicesOptions(); - final IndicesOptions stricterIndicesOptions = IndicesOptions.fromOptions( - indicesOptions.ignoreUnavailable(), - indicesOptions.allowNoIndices(), - false, - false, - false, - true, - true, - indicesOptions.ignoreThrottled() - ); - request.indicesOptions(stricterIndicesOptions); - var indices = SearchContextId.decodeIndices(request.pointInTimeBuilder().getEncodedId()); - request.indices(indices); } /** diff --git a/server/src/test/java/org/elasticsearch/action/search/SearchRequestTests.java b/server/src/test/java/org/elasticsearch/action/search/SearchRequestTests.java index 0c8496081ff19..56c0feb9b77d3 100644 --- a/server/src/test/java/org/elasticsearch/action/search/SearchRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/SearchRequestTests.java @@ -457,6 +457,43 @@ public void testValidate() throws IOException { assertEquals(1, validationErrors.validationErrors().size()); assertEquals("[rank] requires [explain] is [false]", validationErrors.validationErrors().get(0)); } + { + SearchRequest searchRequest = new SearchRequest("test").source( + new SearchSourceBuilder() + .pointInTimeBuilder(new PointInTimeBuilder("")) + ); + ActionRequestValidationException validationErrors = searchRequest.validate(); + assertNotNull(validationErrors); + assertEquals(1, validationErrors.validationErrors().size()); + assertEquals("[indices] cannot be used with point in time. Do not specify any index with point in time.", validationErrors.validationErrors().get(0)); + } + { + SearchRequest searchRequest = new SearchRequest().indicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN_CLOSED) + .source(new SearchSourceBuilder().pointInTimeBuilder(new PointInTimeBuilder("")) + ); + ActionRequestValidationException validationErrors = searchRequest.validate(); + assertNotNull(validationErrors); + assertEquals(1, validationErrors.validationErrors().size()); + assertEquals("[indicesOptions] cannot be used with point in time", validationErrors.validationErrors().get(0)); + } + { + SearchRequest searchRequest = new SearchRequest().routing("route1") + .source(new SearchSourceBuilder().pointInTimeBuilder(new PointInTimeBuilder("")) + ); + ActionRequestValidationException validationErrors = searchRequest.validate(); + assertNotNull(validationErrors); + assertEquals(1, validationErrors.validationErrors().size()); + assertEquals("[routing] cannot be used with point in time", validationErrors.validationErrors().get(0)); + } + { + SearchRequest searchRequest = new SearchRequest().preference("pref1") + .source(new SearchSourceBuilder().pointInTimeBuilder(new PointInTimeBuilder("")) + ); + ActionRequestValidationException validationErrors = searchRequest.validate(); + assertNotNull(validationErrors); + assertEquals(1, validationErrors.validationErrors().size()); + assertEquals("[preference] cannot be used with point in time", validationErrors.validationErrors().get(0)); + } } public void testCopyConstructor() throws IOException { diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/PITAwareQueryClient.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/PITAwareQueryClient.java index 707964a93ab9e..a2627c4671060 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/PITAwareQueryClient.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/PITAwareQueryClient.java @@ -98,6 +98,7 @@ private void searchWithPIT(MultiSearchRequest search, ActionListener 0 && Arrays.equals(searchRequest.indices(), indices) == false) { - IllegalArgumentException cause = new IllegalArgumentException( - "indices defined in the [pit] don't match the request's indices" - ); - auditTrail.accessDenied(requestId, authentication, action, request, authzInfo); - listener.onFailure(actionDenied(authentication, authzInfo, action, request, cause)); - return; - } - searchRequest.indices(indices); - } - logger.info("request resolved=" + request.getClass().getSimpleName()); final Metadata metadata = clusterService.state().metadata(); final AsyncSupplier resolvedIndicesAsyncSupplier = new CachingAsyncSupplier<>(resolvedIndicesListener -> { + if (request instanceof SearchRequest searchRequest && searchRequest.pointInTimeBuilder() != null) { + var resolvedIndices = indicesAndAliasesResolver.resolvePITIndices(searchRequest); + resolvedIndicesListener.onResponse(resolvedIndices); + return; + } final ResolvedIndices resolvedIndices = IndicesAndAliasesResolver.tryResolveWithoutWildcards(action, request); if (resolvedIndices != null) { resolvedIndicesListener.onResponse(resolvedIndices); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java index 16258e71e85b8..a4163b6f10fc0 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java @@ -11,6 +11,8 @@ import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest; import org.elasticsearch.action.admin.indices.alias.get.GetAliasesRequest; import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest; +import org.elasticsearch.action.search.SearchContextId; +import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.cluster.metadata.AliasMetadata; import org.elasticsearch.cluster.metadata.IndexAbstraction; @@ -176,6 +178,24 @@ static ResolvedIndices resolveIndicesAndAliasesWithoutWildcards(String action, I return new ResolvedIndices(localIndices, List.of()); } + /** + * Returns the resolved indices from the {@link SearchContextId} within the provided {@link SearchRequest}. + */ + ResolvedIndices resolvePITIndices(SearchRequest request) { + assert request.pointInTimeBuilder() != null; + var indices = SearchContextId.decodeIndices(request.pointInTimeBuilder().getEncodedId()); + final ResolvedIndices split; + if (request.allowsRemoteIndices()) { + split = remoteClusterResolver.splitLocalAndRemoteIndexNames(indices); + } else { + split = new ResolvedIndices(Arrays.asList(indices), Collections.emptyList()); + } + if (split.isEmpty()) { + return new ResolvedIndices(List.of(NO_INDEX_PLACEHOLDER), Collections.emptyList()); + } + return split; + } + private static void throwOnUnexpectedWildcards(String action, String[] indices) { final List wildcards = Stream.of(indices).filter(Regex::isSimpleMatchPattern).toList(); assert wildcards.isEmpty() == false : "we already know that there's at least one wildcard in the indices"; 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 1911a3f25a880..df03f2069d230 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 @@ -1239,10 +1239,10 @@ public void testSearchAgainstIndex() { public void testSearchPITAgainstIndex() { RoleDescriptor role = new RoleDescriptor( - "search_index", - null, - new IndicesPrivileges[] { IndicesPrivileges.builder().indices("index-*").privileges("read").build() }, - null + "search_index", + null, + new IndicesPrivileges[]{IndicesPrivileges.builder().indices("index-*").privileges("read").build()}, + null ); roleMap.put(role.getName(), role); final Authentication authentication = createAuthentication(new User("test search user", role.getName())); @@ -1255,17 +1255,17 @@ public void testSearchPITAgainstIndex() { PointInTimeBuilder pit = new PointInTimeBuilder(createEncodedPIT(indexMetadata.getIndex())); SearchRequest searchRequest = new SearchRequest().source(new SearchSourceBuilder().pointInTimeBuilder(pit)) - .allowPartialSearchResults(false); + .allowPartialSearchResults(false); final ShardSearchRequest shardRequest = new ShardSearchRequest( - new OriginalIndices(new String[] { indexName }, searchRequest.indicesOptions()), - searchRequest, - new ShardId(indexMetadata.getIndex(), 0), - 0, - 1, - AliasFilter.EMPTY, - 1.0f, - System.currentTimeMillis(), - null + new OriginalIndices(new String[]{indexName}, searchRequest.indicesOptions()), + searchRequest, + new ShardId(indexMetadata.getIndex(), 0), + 0, + 1, + AliasFilter.EMPTY, + 1.0f, + System.currentTimeMillis(), + null ); this.setFakeOriginatingAction = false; authorize(authentication, TransportSearchAction.TYPE.name(), searchRequest, true, () -> { @@ -1285,32 +1285,22 @@ public void testSearchPITAgainstIndex() { assertThat(securityContext.getParentAuthorization(), nullValue()); }); }); - assertArrayEquals(new String[] { indexName }, searchRequest.indices()); + assertThat(searchRequest.indices().length, equalTo(0)); verify(auditTrail).accessGranted( - eq(requestId), - eq(authentication), - eq(TransportSearchAction.TYPE.name()), - eq(searchRequest), - authzInfoRoles(new String[] { role.getName() }) + eq(requestId), + eq(authentication), + eq(TransportSearchAction.TYPE.name()), + eq(searchRequest), + authzInfoRoles(new String[]{role.getName()}) ); verify(auditTrail).accessGranted( - eq(requestId), - eq(authentication), - eq(SearchTransportService.QUERY_ACTION_NAME), - eq(shardRequest), - authzInfoRoles(new String[] { role.getName() }) + eq(requestId), + eq(authentication), + eq(SearchTransportService.QUERY_ACTION_NAME), + eq(shardRequest), + authzInfoRoles(new String[]{role.getName()}) ); verifyNoMoreInteractions(auditTrail); - - // verify invalid search request that tries to override the pit indices - SearchRequest invalidSearchRequest = new SearchRequest("another_index").source(new SearchSourceBuilder().pointInTimeBuilder(pit)) - .allowPartialSearchResults(false); - Exception exc = expectThrows( - Exception.class, - () -> authorize(authentication, TransportSearchAction.TYPE.name(), invalidSearchRequest) - ); - assertThat(exc.getCause(), instanceOf(IllegalArgumentException.class)); - assertThat(exc.getCause().getMessage(), containsString("indices defined in the [pit] don't match the request's indices")); } public void testScrollRelatedRequestsAllowed() { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java index a0da67f3006a3..256c8eb3b883b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java @@ -161,6 +161,7 @@ private void searchWithPointInTime(SearchRequest search, ActionListener { String pitId = openPointInTimeResponse.getPointInTimeId(); + search.indicesOptions(SearchRequest.DEFAULT_INDICES_OPTIONS); search.indices(Strings.EMPTY_ARRAY); search.source().pointInTimeBuilder(new PointInTimeBuilder(pitId)); ActionListener closePitOnErrorListener = wrap(searchResponse -> { diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/ClientTransformIndexer.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/ClientTransformIndexer.java index 8bb7b813687a5..0a57d8e1214ad 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/ClientTransformIndexer.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/ClientTransformIndexer.java @@ -519,19 +519,24 @@ private void injectPointInTimeIfNeeded( void doSearch(Tuple namedSearchRequest, ActionListener listener) { String name = namedSearchRequest.v1(); - SearchRequest searchRequest = namedSearchRequest.v2(); + SearchRequest originalRequest = namedSearchRequest.v2(); // We want to treat a request to search 0 indices as a request to do nothing, not a request to search all indices - if (searchRequest.indices().length == 0) { - logger.debug("[{}] Search request [{}] optimized to noop; searchRequest [{}]", getJobId(), name, searchRequest); + if (originalRequest.indices().length == 0) { + logger.debug("[{}] Search request [{}] optimized to noop; searchRequest [{}]", getJobId(), name, originalRequest); listener.onResponse(null); return; } - logger.trace("searchRequest: [{}]", searchRequest); + logger.info("searchRequest: [{}]", originalRequest); - PointInTimeBuilder pit = searchRequest.pointInTimeBuilder(); + final SearchRequest searchRequest; + PointInTimeBuilder pit = originalRequest.pointInTimeBuilder(); if (pit != null) { // remove the indices from the request, they will be derived from the provided pit - searchRequest = new SearchRequest(searchRequest).indices(new String[0]); + searchRequest = new SearchRequest(originalRequest) + .indices(new String[0]) + .indicesOptions(SearchRequest.DEFAULT_INDICES_OPTIONS); + } else { + searchRequest = originalRequest; } ClientHelper.executeWithHeadersAsync( @@ -559,13 +564,13 @@ void doSearch(Tuple namedSearchRequest, ActionListener namedSearchRequest, ActionListener