Skip to content

Commit

Permalink
Fix all SearchResponse leaks in x-pack security (#103239)
Browse files Browse the repository at this point in the history 
Just like everywhere else, only fixing the ref-counting here, no other changes to the tests.
  • Loading branch information
@original-brownbear
original-brownbear authored Dec 11, 2023
1 parent 543919b commit 3c8b979
Show file tree
Hide file tree
Showing 22 changed files with 1,376 additions and 1,164 deletions.
5 changes: 2 additions & 3 deletions .../internalClusterTest/java/org/elasticsearch/integration/DateMathExpressionIntegTests.java
View file
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
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

0 comments on commit 3c8b979

Please sign in to comment.