Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix all SearchResponse leaks in x-pack security #103239

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Fix all SearchResponse leaks in x-pack security #103239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions .../internalClusterTest/java/org/elasticsearch/integration/DateMathExpressionIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import org.elasticsearch.action.admin.indices.create.CreateIndexResponse;
import org.elasticsearch.action.get.GetResponse;
import org.elasticsearch.action.get.MultiGetResponse;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.action.support.master.AcknowledgedResponse;
import org.elasticsearch.action.update.UpdateResponse;
import org.elasticsearch.client.internal.Client;
Expand All @@ -25,6 +24,7 @@

import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.NONE;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse;
import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -77,8 +77,7 @@ public void testDateMathExpressionsCanBeAuthorized() throws Exception {
if (refeshOnOperation == false) {
client.admin().indices().prepareRefresh(expression).get();
}
SearchResponse searchResponse = client.prepareSearch(expression).setQuery(QueryBuilders.matchAllQuery()).get();
assertThat(searchResponse.getHits().getTotalHits().value, is(1L));
assertHitCount(client.prepareSearch(expression).setQuery(QueryBuilders.matchAllQuery()), 1);

assertResponse(
client.prepareMultiSearch().add(client.prepareSearch(expression).setQuery(QueryBuilders.matchAllQuery()).request()),
Expand Down
113 changes: 41 additions & 72 deletions ...y/src/internalClusterTest/java/org/elasticsearch/integration/DlsFlsRequestCacheTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import org.elasticsearch.action.admin.indices.alias.Alias;
import org.elasticsearch.action.admin.indices.forcemerge.ForceMergeResponse;
import org.elasticsearch.action.admin.indices.refresh.RefreshResponse;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.action.search.SearchRequestBuilder;
import org.elasticsearch.client.internal.Client;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.settings.SecureString;
Expand Down Expand Up @@ -191,31 +191,31 @@ public void testRequestCacheForDLS() {
final Client limitedClient = limitedClient();

// Search first with power client, it should see all docs
assertSearchResponse(powerClient.prepareSearch(DLS_INDEX).setRequestCache(true).get(), Set.of("101", "102"));
assertSearchResponse(powerClient.prepareSearch(DLS_INDEX).setRequestCache(true), Set.of("101", "102"));
assertCacheState(DLS_INDEX, 0, 1);

// Search with the limited client and it should see only one doc (i.e. it won't use cache entry for power client)
assertSearchResponse(limitedClient.prepareSearch(DLS_INDEX).setRequestCache(true).get(), Set.of("101"));
assertSearchResponse(limitedClient.prepareSearch(DLS_INDEX).setRequestCache(true), Set.of("101"));
assertCacheState(DLS_INDEX, 0, 2);

// Execute the above search again and it should use the cache entry for limited client
assertSearchResponse(limitedClient.prepareSearch(DLS_INDEX).setRequestCache(true).get(), Set.of("101"));
assertSearchResponse(limitedClient.prepareSearch(DLS_INDEX).setRequestCache(true), Set.of("101"));
assertCacheState(DLS_INDEX, 1, 2);

// Execute the search with power client again and it should still see all docs
assertSearchResponse(powerClient.prepareSearch(DLS_INDEX).setRequestCache(true).get(), Set.of("101", "102"));
assertSearchResponse(powerClient.prepareSearch(DLS_INDEX).setRequestCache(true), Set.of("101", "102"));
assertCacheState(DLS_INDEX, 2, 2);

// The limited client has a different DLS query for dls-alias compared to the underlying dls-index
assertSearchResponse(limitedClient.prepareSearch(DLS_ALIAS).setRequestCache(true).get(), Set.of("102"));
assertSearchResponse(limitedClient.prepareSearch(DLS_ALIAS).setRequestCache(true), Set.of("102"));
assertCacheState(DLS_INDEX, 2, 3);
assertSearchResponse(limitedClient.prepareSearch(DLS_ALIAS).setRequestCache(true).get(), Set.of("102"));
assertSearchResponse(limitedClient.prepareSearch(DLS_ALIAS).setRequestCache(true), Set.of("102"));
assertCacheState(DLS_INDEX, 3, 3);

// Search with limited client for dls-alias and dls-index returns all docs. The cache entry is however different
// from the power client, i.e. still no sharing even if the end results are the same. This is because the
// search with limited client still have DLS queries attached to it.
assertSearchResponse(limitedClient.prepareSearch(DLS_ALIAS, DLS_INDEX).setRequestCache(true).get(), Set.of("101", "102"));
assertSearchResponse(limitedClient.prepareSearch(DLS_ALIAS, DLS_INDEX).setRequestCache(true), Set.of("101", "102"));
assertCacheState(DLS_INDEX, 3, 4);
}

Expand All @@ -224,37 +224,29 @@ public void testRequestCacheForFLS() {
final Client limitedClient = limitedClient();

// Search first with power client, it should see all fields
assertSearchResponse(
powerClient.prepareSearch(FLS_INDEX).setRequestCache(true).get(),
Set.of("201", "202"),
Set.of("public", "private")
);
assertSearchResponse(powerClient.prepareSearch(FLS_INDEX).setRequestCache(true), Set.of("201", "202"), Set.of("public", "private"));
assertCacheState(FLS_INDEX, 0, 1);

// Search with limited client and it should see only public field
assertSearchResponse(limitedClient.prepareSearch(FLS_INDEX).setRequestCache(true).get(), Set.of("201", "202"), Set.of("public"));
assertSearchResponse(limitedClient.prepareSearch(FLS_INDEX).setRequestCache(true), Set.of("201", "202"), Set.of("public"));
assertCacheState(FLS_INDEX, 0, 2);

// Search with limited client again and it should use the cache
assertSearchResponse(limitedClient.prepareSearch(FLS_INDEX).setRequestCache(true).get(), Set.of("201", "202"), Set.of("public"));
assertSearchResponse(limitedClient.prepareSearch(FLS_INDEX).setRequestCache(true), Set.of("201", "202"), Set.of("public"));
assertCacheState(FLS_INDEX, 1, 2);

// Search again with power client, it should use its own cache entry
assertSearchResponse(
powerClient.prepareSearch(FLS_INDEX).setRequestCache(true).get(),
Set.of("201", "202"),
Set.of("public", "private")
);
assertSearchResponse(powerClient.prepareSearch(FLS_INDEX).setRequestCache(true), Set.of("201", "202"), Set.of("public", "private"));
assertCacheState(FLS_INDEX, 2, 2);

// The fls-alias has a different FLS definition compared to its underlying fls-index.
assertSearchResponse(limitedClient.prepareSearch(FLS_ALIAS).setRequestCache(true).get(), Set.of("201", "202"), Set.of("private"));
assertSearchResponse(limitedClient.prepareSearch(FLS_ALIAS).setRequestCache(true), Set.of("201", "202"), Set.of("private"));
assertCacheState(FLS_INDEX, 2, 3);

// Search with the limited client for both fls-alias and fls-index and all docs and fields are also returned.
// But request cache is not shared with the power client because it still has a different indexAccessControl
assertSearchResponse(
limitedClient.prepareSearch(FLS_ALIAS, FLS_INDEX).setRequestCache(true).get(),
limitedClient.prepareSearch(FLS_ALIAS, FLS_INDEX).setRequestCache(true),
Set.of("201", "202"),
Set.of("public", "private")
);
Expand All @@ -267,7 +259,7 @@ public void testRequestCacheForBothDLSandFLS() throws ExecutionException, Interr

// Search first with power client, it should see all fields
assertSearchResponse(
powerClient.prepareSearch(INDEX).setRequestCache(true).get(),
powerClient.prepareSearch(INDEX).setRequestCache(true),
Set.of("1", "2"),
Set.of("number", "letter", "public", "private")
);
Expand All @@ -278,25 +270,17 @@ public void testRequestCacheForBothDLSandFLS() throws ExecutionException, Interr
expectThrows(ElasticsearchSecurityException.class, () -> limitedClient.prepareSearch(INDEX).setRequestCache(true).get());

// Search for alias1 that points to index and has DLS/FLS
assertSearchResponse(
limitedClient.prepareSearch(ALIAS1).setRequestCache(true).get(),
Set.of("1"),
Set.of("number", "letter", "public")
);
assertSearchResponse(limitedClient.prepareSearch(ALIAS1).setRequestCache(true), Set.of("1"), Set.of("number", "letter", "public"));
assertCacheState(INDEX, 0, 2);

// Search for alias2 that also points to index but has a different set of DLS/FLS
assertSearchResponse(
limitedClient.prepareSearch(ALIAS2).setRequestCache(true).get(),
Set.of("2"),
Set.of("number", "letter", "private")
);
assertSearchResponse(limitedClient.prepareSearch(ALIAS2).setRequestCache(true), Set.of("2"), Set.of("number", "letter", "private"));
assertCacheState(INDEX, 0, 3);

// Search for all-alias that has full read access to the underlying index
// This makes it share the cache entry of the power client
assertSearchResponse(
limitedClient.prepareSearch(ALL_ALIAS).setRequestCache(true).get(),
limitedClient.prepareSearch(ALL_ALIAS).setRequestCache(true),
Set.of("1", "2"),
Set.of("number", "letter", "public", "private")
);
Expand All @@ -305,7 +289,7 @@ public void testRequestCacheForBothDLSandFLS() throws ExecutionException, Interr
// Similarly, search for alias1 and all-alias results in full read access to the index
// and again reuse the cache entry of the power client
assertSearchResponse(
limitedClient.prepareSearch(ALIAS1, ALL_ALIAS).setRequestCache(true).get(),
limitedClient.prepareSearch(ALIAS1, ALL_ALIAS).setRequestCache(true),
Set.of("1", "2"),
Set.of("number", "letter", "public", "private")
);
Expand All @@ -314,7 +298,7 @@ public void testRequestCacheForBothDLSandFLS() throws ExecutionException, Interr
// Though search for both alias1 and alias2 is effectively full read access to index,
// it does not share the cache entry of the power client because role queries still exist.
assertSearchResponse(
limitedClient.prepareSearch(ALIAS1, ALIAS2).setRequestCache(true).get(),
limitedClient.prepareSearch(ALIAS1, ALIAS2).setRequestCache(true),
Set.of("1", "2"),
Set.of("number", "letter", "public", "private")
);
Expand All @@ -325,7 +309,7 @@ public void testRequestCacheForBothDLSandFLS() throws ExecutionException, Interr

// It should not reuse any entries from the cache
assertSearchResponse(
limitedClientApiKey.prepareSearch(ALL_ALIAS).setRequestCache(true).get(),
limitedClientApiKey.prepareSearch(ALL_ALIAS).setRequestCache(true),
Set.of("1"),
Set.of("letter", "public", "private")
);
Expand All @@ -341,43 +325,23 @@ public void testRequestCacheWithTemplateRoleQuery() {
);

// Search first with user1 and only one document will be return with the corresponding username
assertSearchResponse(
client1.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_INDEX).setRequestCache(true).get(),
Set.of("1"),
Set.of("username")
);
assertSearchResponse(client1.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_INDEX).setRequestCache(true), Set.of("1"), Set.of("username"));
assertCacheState(DLS_TEMPLATE_ROLE_QUERY_INDEX, 0, 1);

// Search with user2 will not use user1's cache because template query is resolved differently for them
assertSearchResponse(
client2.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_INDEX).setRequestCache(true).get(),
Set.of("2"),
Set.of("username")
);
assertSearchResponse(client2.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_INDEX).setRequestCache(true), Set.of("2"), Set.of("username"));
assertCacheState(DLS_TEMPLATE_ROLE_QUERY_INDEX, 0, 2);

// Search with user1 again will use user1's cache
assertSearchResponse(
client1.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_INDEX).setRequestCache(true).get(),
Set.of("1"),
Set.of("username")
);
assertSearchResponse(client1.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_INDEX).setRequestCache(true), Set.of("1"), Set.of("username"));
assertCacheState(DLS_TEMPLATE_ROLE_QUERY_INDEX, 1, 2);

// Search with user2 again will use user2's cache
assertSearchResponse(
client2.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_INDEX).setRequestCache(true).get(),
Set.of("2"),
Set.of("username")
);
assertSearchResponse(client2.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_INDEX).setRequestCache(true), Set.of("2"), Set.of("username"));
assertCacheState(DLS_TEMPLATE_ROLE_QUERY_INDEX, 2, 2);

// Since the DLS for the alias uses a stored script, this should cause the request cached to be disabled
assertSearchResponse(
client1.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_ALIAS).setRequestCache(true).get(),
Set.of("1"),
Set.of("username")
);
assertSearchResponse(client1.prepareSearch(DLS_TEMPLATE_ROLE_QUERY_ALIAS).setRequestCache(true), Set.of("1"), Set.of("username"));
// No cache should be used
assertCacheState(DLS_TEMPLATE_ROLE_QUERY_INDEX, 2, 2);
}
Expand Down Expand Up @@ -455,19 +419,24 @@ private Client limitedClientApiKey() throws ExecutionException, InterruptedExcep
return client().filterWithHeader(Map.of("Authorization", "ApiKey " + base64ApiKey));
}

private void assertSearchResponse(SearchResponse searchResponse, Set<String> docIds) {
assertSearchResponse(searchResponse, docIds, null);
private void assertSearchResponse(SearchRequestBuilder requestBuilder, Set<String> docIds) {
assertSearchResponse(requestBuilder, docIds, null);
}

private void assertSearchResponse(SearchResponse searchResponse, Set<String> docIds, Set<String> fieldNames) {
assertThat(searchResponse.getFailedShards(), equalTo(0));
assertThat(searchResponse.getHits().getTotalHits().value, equalTo((long) docIds.size()));
final SearchHit[] hits = searchResponse.getHits().getHits();
assertThat(Arrays.stream(hits).map(SearchHit::getId).collect(Collectors.toUnmodifiableSet()), equalTo(docIds));
if (fieldNames != null) {
for (SearchHit hit : hits) {
assertThat(hit.getSourceAsMap().keySet(), equalTo(fieldNames));
private void assertSearchResponse(SearchRequestBuilder requestBuilder, Set<String> docIds, Set<String> fieldNames) {
var searchResponse = requestBuilder.get();
try {
assertThat(searchResponse.getFailedShards(), equalTo(0));
assertThat(searchResponse.getHits().getTotalHits().value, equalTo((long) docIds.size()));
final SearchHit[] hits = searchResponse.getHits().getHits();
assertThat(Arrays.stream(hits).map(SearchHit::getId).collect(Collectors.toUnmodifiableSet()), equalTo(docIds));
if (fieldNames != null) {
for (SearchHit hit : hits) {
assertThat(hit.getSourceAsMap().keySet(), equalTo(fieldNames));
}
}
} finally {
searchResponse.decRef();
}
}

Expand Down
Loading