Skip to content

Commit

Permalink
Restrict the index authorization checks in PIT search
Browse files Browse the repository at this point in the history
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 elastic#99684
  • Loading branch information
jimczi committed Dec 8, 2023
1 parent c2388ec commit 89853df
Show file tree
Hide file tree
Showing 9 changed files with 120 additions and 100 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ private void searchWithPIT(MultiSearchRequest search, ActionListener<MultiSearch
}

private void makeRequestPITCompatible(SearchRequest request) {
request.indicesOptions(SearchRequest.DEFAULT_INDICES_OPTIONS);
SearchSourceBuilder source = request.source();
// don't increase the keep alive
source.pointInTimeBuilder(new PointInTimeBuilder(pitId));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.elasticsearch.action.datastreams.MigrateToDataStreamAction;
import org.elasticsearch.action.delete.DeleteAction;
import org.elasticsearch.action.index.IndexAction;
import org.elasticsearch.action.search.SearchContextId;
import org.elasticsearch.action.search.SearchRequest;
import org.elasticsearch.action.support.GroupedActionListener;
import org.elasticsearch.action.support.replication.TransportReplicationAction.ConcreteShardRequest;
Expand Down Expand Up @@ -86,7 +85,6 @@
import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore;
import org.elasticsearch.xpack.security.operator.OperatorPrivileges.OperatorPrivilegesService;

import java.util.Arrays;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -472,28 +470,13 @@ private void authorizeAction(
l.onResponse(result);
}));
} else if (isIndexAction(action)) {
if (request instanceof SearchRequest searchRequest && searchRequest.pointInTimeBuilder() != null) {
/**
* We override the {@link SearchRequest#indices} with the PIT's indices for the authorization check.
* Although this copy also occurs in the rest layer, we cannot rely on it, as the action might be executed
* directly in the transport client.
* For backward compatibility reasons, we refrain from failing the request if {@link SearchRequest#indices}
* is non-empty. Instead, we verify that the indices match the PIT definition and deny access if they don't.
*/
var indices = SearchContextId.decodeIndices(searchRequest.pointInTimeBuilder().getEncodedId());
if (searchRequest.indices().length > 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<ResolvedIndices> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String> 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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand All @@ -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, () -> {
Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ private void searchWithPointInTime(SearchRequest search, ActionListener<SearchRe
openPitRequest,
listener.delegateFailureAndWrap((delegate, openPointInTimeResponse) -> {
String pitId = openPointInTimeResponse.getPointInTimeId();
search.indicesOptions(SearchRequest.DEFAULT_INDICES_OPTIONS);
search.indices(Strings.EMPTY_ARRAY);
search.source().pointInTimeBuilder(new PointInTimeBuilder(pitId));
ActionListener<SearchResponse> closePitOnErrorListener = wrap(searchResponse -> {
Expand Down
Loading

0 comments on commit 89853df

Please sign in to comment.