From b34410df3fe341d5d231276c8db2aae01334821e Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 24 Feb 2021 21:49:39 +1100 Subject: [PATCH] Improve shard level request cache efficiency (#69505) (#69507) (#69514) Shard level request cache is improved to work correctly at all time. Also ensure profiling and suggester are properly disabled when not supported. --- .../search/internal/ShardSearchRequest.java | 6 +- .../DocumentLevelSecurityTests.java | 58 +++++++++++--- ...cumentLevelSecurityRequestInterceptor.java | 33 +++++--- .../interceptor/SearchRequestInterceptor.java | 25 ++++-- .../build.gradle | 23 +++++- .../test/multi_cluster/100_resolve_index.yml | 11 +-- .../test/multi_cluster/110_dls_fls.yml | 76 +++++++++++++++++++ .../test/remote_cluster/10_basic.yml | 34 +++++++++ 8 files changed, 230 insertions(+), 36 deletions(-) create mode 100644 x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/110_dls_fls.yml diff --git a/server/src/main/java/org/elasticsearch/search/internal/ShardSearchRequest.java b/server/src/main/java/org/elasticsearch/search/internal/ShardSearchRequest.java index ba9cee1639b5d..3dda1b9867092 100644 --- a/server/src/main/java/org/elasticsearch/search/internal/ShardSearchRequest.java +++ b/server/src/main/java/org/elasticsearch/search/internal/ShardSearchRequest.java @@ -66,7 +66,7 @@ public class ShardSearchRequest extends TransportRequest implements IndicesReque private final Scroll scroll; private final String[] types; private final float indexBoost; - private final Boolean requestCache; + private Boolean requestCache; private final long nowInMillis; private final boolean allowPartialSearchResults; private final OriginalIndices originalIndices; @@ -345,6 +345,10 @@ public Boolean requestCache() { return requestCache; } + public void requestCache(Boolean requestCache) { + this.requestCache = requestCache; + } + public boolean allowPartialSearchResults() { return allowPartialSearchResults; } diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DocumentLevelSecurityTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DocumentLevelSecurityTests.java index 2c8761b8d0229..a23cf7903f9b3 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DocumentLevelSecurityTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DocumentLevelSecurityTests.java @@ -129,7 +129,8 @@ protected String configUsers() { "user1:" + usersPasswdHashed + "\n" + "user2:" + usersPasswdHashed + "\n" + "user3:" + usersPasswdHashed + "\n" + - "user4:" + usersPasswdHashed + "\n"; + "user4:" + usersPasswdHashed + "\n" + + "user5:" + usersPasswdHashed + "\n"; } @Override @@ -138,7 +139,8 @@ protected String configUsersRoles() { "role1:user1,user2,user3\n" + "role2:user1,user3\n" + "role3:user2,user3\n" + - "role4:user4\n"; + "role4:user4\n" + + "role5:user5\n"; } @Override @@ -171,7 +173,17 @@ protected String configRoles() { " - names: '*'\n" + " privileges: [ ALL ]\n" + // query that can match nested documents - " query: '{\"bool\": { \"must_not\": { \"term\" : {\"field1\" : \"value2\"}}}}'"; + " query: '{\"bool\": { \"must_not\": { \"term\" : {\"field1\" : \"value2\"}}}}'\n" + + "role5:\n" + + " cluster: [ all ]\n" + + " indices:\n" + + " - names: [ 'test' ]\n" + + " privileges: [ read ]\n" + + " query: '{\"term\" : {\"field2\" : \"value2\"}}'\n" + + " - names: [ 'fls-index' ]\n" + + " privileges: [ read ]\n" + + " field_security:\n" + + " grant: [ 'field1', 'other_field', 'suggest_field2' ]\n"; } @Override @@ -1278,6 +1290,15 @@ public void testSuggesters() throws Exception { .endObject()).get(); refresh("test"); + assertAcked(client().admin().indices().prepareCreate("fls-index") + .setSettings(Settings.builder() + .put("index.number_of_shards", 1) + .put("index.number_of_replicas", 0) + ) + .addMapping("type1", "field1", "type=text", "suggest_field1", "type=text", "suggest_field2", "type=completion", + "yet_another", "type=text") + ); + // Term suggester: SearchResponse response = client() .prepareSearch("test") @@ -1293,9 +1314,13 @@ public void testSuggesters() throws Exception { assertThat(termSuggestion.getEntries().get(0).getOptions().size(), equalTo(1)); assertThat(termSuggestion.getEntries().get(0).getOptions().get(0).getText().string(), equalTo("value")); + final String[] indices = + randomFrom(org.elasticsearch.common.collect.List.of( + new String[] { "test" }, new String[] { "fls-index", "test" }, new String[] { "test", "fls-index" })); + Exception e = expectThrows(ElasticsearchSecurityException.class, () -> client() - .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user2", USERS_PASSWD))) - .prepareSearch("test") + .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD))) + .prepareSearch(indices) .suggest(new SuggestBuilder() .setGlobalText("valeu") .addSuggestion("_name1", new TermSuggestionBuilder("suggest_field1")) @@ -1318,8 +1343,8 @@ public void testSuggesters() throws Exception { assertThat(phraseSuggestion.getEntries().get(0).getOptions().get(0).getText().string(), equalTo("value")); e = expectThrows(ElasticsearchSecurityException.class, () -> client() - .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user2", USERS_PASSWD))) - .prepareSearch("test") + .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD))) + .prepareSearch(indices) .suggest(new SuggestBuilder() .setGlobalText("valeu") .addSuggestion("_name1", new PhraseSuggestionBuilder("suggest_field1")) @@ -1342,8 +1367,8 @@ public void testSuggesters() throws Exception { assertThat(completionSuggestion.getEntries().get(0).getOptions().get(0).getText().string(), equalTo("value")); e = expectThrows(ElasticsearchSecurityException.class, () -> client() - .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user2", USERS_PASSWD))) - .prepareSearch("test") + .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD))) + .prepareSearch(indices) .suggest(new SuggestBuilder() .setGlobalText("valeu") .addSuggestion("_name1", new CompletionSuggestionBuilder("suggest_field2")) @@ -1373,6 +1398,14 @@ public void testProfile() throws Exception { .endObject()).get(); refresh("test"); + assertAcked(client().admin().indices().prepareCreate("fls-index") + .setSettings(Settings.builder() + .put("index.number_of_shards", 1) + .put("index.number_of_replicas", 0) + ) + .addMapping("type1", "field1", "type=text", "other_field", "type=text", "yet_another", "type=text") + ); + SearchResponse response = client() .prepareSearch("test") .setProfile(true) @@ -1389,9 +1422,12 @@ public void testProfile() throws Exception { // ProfileResult profileResult = queryProfileShardResult.getQueryResults().get(0); // assertThat(profileResult.getLuceneDescription(), equalTo("(other_field:value)^0.8")); + final String[] indices = + randomFrom(org.elasticsearch.common.collect.List.of( + new String[] { "test" }, new String[] { "fls-index", "test" }, new String[] { "test", "fls-index" })); Exception e = expectThrows(ElasticsearchSecurityException.class, () -> client() - .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user2", USERS_PASSWD))) - .prepareSearch("test") + .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD))) + .prepareSearch(indices) .setProfile(true) .setQuery(new FuzzyQueryBuilder("other_field", "valeu")) .get()); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/FieldAndDocumentLevelSecurityRequestInterceptor.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/FieldAndDocumentLevelSecurityRequestInterceptor.java index 1810f3345c9f5..c1bc86cc4e761 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/FieldAndDocumentLevelSecurityRequestInterceptor.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/FieldAndDocumentLevelSecurityRequestInterceptor.java @@ -11,9 +11,11 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.IndicesRequest; import org.elasticsearch.common.MemoizedSupplier; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.license.XPackLicenseState.Feature; +import org.elasticsearch.transport.TransportActionProxy; import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine; import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo; import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.RequestInfo; @@ -39,28 +41,37 @@ abstract class FieldAndDocumentLevelSecurityRequestInterceptor implements Reques @Override public void intercept(RequestInfo requestInfo, AuthorizationEngine authorizationEngine, AuthorizationInfo authorizationInfo, ActionListener listener) { - if (requestInfo.getRequest() instanceof IndicesRequest) { + if (requestInfo.getRequest() instanceof IndicesRequest && false == TransportActionProxy.isProxyAction(requestInfo.getAction())) { IndicesRequest indicesRequest = (IndicesRequest) requestInfo.getRequest(); boolean shouldIntercept = licenseState.isSecurityEnabled(); MemoizedSupplier licenseChecker = new MemoizedSupplier<>(() -> licenseState.checkFeature(Feature.SECURITY_DLS_FLS)); if (supports(indicesRequest) && shouldIntercept) { final IndicesAccessControl indicesAccessControl = threadContext.getTransient(AuthorizationServiceField.INDICES_PERMISSIONS_KEY); - for (String index : requestIndices(indicesRequest)) { + boolean fieldLevelSecurityEnabled = false; + boolean documentLevelSecurityEnabled = false; + final String[] requestIndices = requestIndices(indicesRequest); + for (String index : requestIndices) { IndicesAccessControl.IndexAccessControl indexAccessControl = indicesAccessControl.getIndexPermissions(index); if (indexAccessControl != null) { - boolean fieldLevelSecurityEnabled = indexAccessControl.getFieldPermissions().hasFieldLevelSecurity(); - boolean documentLevelSecurityEnabled = indexAccessControl.getDocumentPermissions().hasDocumentLevelPermissions(); - if ((fieldLevelSecurityEnabled || documentLevelSecurityEnabled) && licenseChecker.get()) { - logger.trace("intercepted request for index [{}] with field level access controls [{}] " + - "document level access controls [{}]. disabling conflicting features", - index, fieldLevelSecurityEnabled, documentLevelSecurityEnabled); - disableFeatures(indicesRequest, fieldLevelSecurityEnabled, documentLevelSecurityEnabled, listener); - return; + fieldLevelSecurityEnabled = + fieldLevelSecurityEnabled || indexAccessControl.getFieldPermissions().hasFieldLevelSecurity(); + documentLevelSecurityEnabled = + documentLevelSecurityEnabled || indexAccessControl.getDocumentPermissions().hasDocumentLevelPermissions(); + if (fieldLevelSecurityEnabled && documentLevelSecurityEnabled) { + break; } } - logger.trace("intercepted request for index [{}] without field or document level access controls", index); } + if ((fieldLevelSecurityEnabled || documentLevelSecurityEnabled) && licenseChecker.get()) { + logger.trace("intercepted request for indices [{}] with field level access controls [{}] " + + "document level access controls [{}]. disabling conflicting features", + Strings.arrayToDelimitedString(requestIndices, ","), fieldLevelSecurityEnabled, documentLevelSecurityEnabled); + disableFeatures(indicesRequest, fieldLevelSecurityEnabled, documentLevelSecurityEnabled, listener); + return; + } + logger.trace("intercepted request for indices [{}] without field or document level access controls", + Strings.arrayToDelimitedString(requestIndices, ",")); } } listener.onResponse(null); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/SearchRequestInterceptor.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/SearchRequestInterceptor.java index 89712177d5dc5..8495024f5149d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/SearchRequestInterceptor.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/SearchRequestInterceptor.java @@ -12,10 +12,12 @@ import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.rest.RestStatus; +import org.elasticsearch.search.builder.SearchSourceBuilder; +import org.elasticsearch.search.internal.ShardSearchRequest; import org.elasticsearch.threadpool.ThreadPool; /** - * If field level security is enabled this interceptor disables the request cache for search requests. + * If field level security is enabled this interceptor disables the request cache for search and shardSearch requests. */ public class SearchRequestInterceptor extends FieldAndDocumentLevelSecurityRequestInterceptor { @@ -26,14 +28,25 @@ public SearchRequestInterceptor(ThreadPool threadPool, XPackLicenseState license @Override public void disableFeatures(IndicesRequest indicesRequest, boolean fieldLevelSecurityEnabled, boolean documentLevelSecurityEnabled, ActionListener listener) { - final SearchRequest request = (SearchRequest) indicesRequest; - request.requestCache(false); + assert indicesRequest instanceof SearchRequest || indicesRequest instanceof ShardSearchRequest + : "request must be either SearchRequest or ShardSearchRequest"; + + final SearchSourceBuilder source; + if (indicesRequest instanceof SearchRequest) { + final SearchRequest request = (SearchRequest) indicesRequest; + request.requestCache(false); + source = request.source(); + } else { + final ShardSearchRequest request = (ShardSearchRequest) indicesRequest; + request.requestCache(false); + source = request.source(); + } if (documentLevelSecurityEnabled) { - if (request.source() != null && request.source().suggest() != null) { + if (source != null && source.suggest() != null) { listener.onFailure(new ElasticsearchSecurityException("Suggest isn't supported if document level security is enabled", RestStatus.BAD_REQUEST)); - } else if (request.source() != null && request.source().profile()) { + } else if (source != null && source.profile()) { listener.onFailure(new ElasticsearchSecurityException("A search request cannot be profiled if document level security " + "is enabled", RestStatus.BAD_REQUEST)); } else { @@ -46,6 +59,6 @@ public void disableFeatures(IndicesRequest indicesRequest, boolean fieldLevelSec @Override public boolean supports(IndicesRequest request) { - return request instanceof SearchRequest; + return request instanceof SearchRequest || request instanceof ShardSearchRequest; } } diff --git a/x-pack/qa/multi-cluster-search-security/build.gradle b/x-pack/qa/multi-cluster-search-security/build.gradle index 10ebb03edc89c..f9c9e55a1b0a8 100644 --- a/x-pack/qa/multi-cluster-search-security/build.gradle +++ b/x-pack/qa/multi-cluster-search-security/build.gradle @@ -1,3 +1,4 @@ +import org.elasticsearch.gradle.info.BuildParams import org.elasticsearch.gradle.test.RestIntegTestTask apply plugin: 'elasticsearch.testclusters' @@ -19,6 +20,9 @@ tasks.register('remote-cluster', RestIntegTestTask) { systemProperty 'tests.rest.suite', 'remote_cluster' } +// randomise between sniff and proxy modes +boolean proxyMode = (new Random(Long.parseUnsignedLong(BuildParams.testSeed.tokenize(':').get(0), 16))).nextBoolean() + testClusters { 'remote-cluster' { testDistribution = 'DEFAULT' @@ -38,8 +42,15 @@ testClusters { setting 'xpack.watcher.enabled', 'false' setting 'xpack.ml.enabled', 'false' setting 'xpack.license.self_generated.type', 'trial' - setting 'cluster.remote.my_remote_cluster.seeds', { - testClusters.'remote-cluster'.getAllTransportPortURI().collect { "\"$it\"" }.toString() + if (proxyMode) { + setting 'cluster.remote.my_remote_cluster.mode', 'proxy' + setting 'cluster.remote.my_remote_cluster.proxy_address', { + "\"${testClusters.'remote-cluster'.getAllTransportPortURI().get(0)}\"" + } + } else { + setting 'cluster.remote.my_remote_cluster.seeds', { + testClusters.'remote-cluster'.getAllTransportPortURI().collect { "\"$it\"" }.toString() + } } setting 'cluster.remote.connections_per_cluster', "1" @@ -51,6 +62,14 @@ tasks.register('mixed-cluster', RestIntegTestTask) { dependsOn 'remote-cluster' useCluster testClusters.'remote-cluster' systemProperty 'tests.rest.suite', 'multi_cluster' + if (proxyMode) { + systemProperty 'tests.rest.blacklist', [ + 'multi_cluster/10_basic/Add transient remote cluster based on the preset cluster', + 'multi_cluster/20_info/Add transient remote cluster based on the preset cluster and check remote info', + 'multi_cluster/20_info/Fetch remote cluster info for existing cluster', + 'multi_cluster/70_connection_mode_configuration/*', + ].join(",") + } } tasks.register("integTest") { diff --git a/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/100_resolve_index.yml b/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/100_resolve_index.yml index 70bb0bbd87984..0e7d124ff4d72 100644 --- a/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/100_resolve_index.yml +++ b/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/100_resolve_index.yml @@ -34,11 +34,12 @@ - match: {indices.7.attributes.0: open } - match: {indices.8.name: my_remote_cluster:secured_via_alias} - match: {indices.8.attributes.0: open} - - match: {indices.9.name: my_remote_cluster:single_doc_index} - - match: {indices.10.attributes.0: open} - - match: {indices.10.name: my_remote_cluster:test_index} - - match: {indices.10.aliases.0: aliased_test_index} - - match: {indices.10.attributes.0: open} + - match: {indices.9.name: my_remote_cluster:shared_index} + - match: {indices.10.name: my_remote_cluster:single_doc_index} + - match: {indices.11.attributes.0: open} + - match: {indices.11.name: my_remote_cluster:test_index} + - match: {indices.11.aliases.0: aliased_test_index} + - match: {indices.11.attributes.0: open} - match: {aliases.0.name: my_remote_cluster:.security} - match: {aliases.0.indices.0: .security-7} - match: {aliases.1.name: my_remote_cluster:aliased_closed_index} diff --git a/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/110_dls_fls.yml b/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/110_dls_fls.yml new file mode 100644 index 0000000000000..ffdce6e336464 --- /dev/null +++ b/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/110_dls_fls.yml @@ -0,0 +1,76 @@ +--- +setup: + - skip: + features: headers + + - do: + cluster.health: + wait_for_status: yellow + + - do: + security.put_user: + username: "dls_fls_user" + body: > + { + "password": "s3krit-password", + "roles" : [ "dls_fls_role" ] + } + +--- +teardown: + - do: + security.delete_user: + username: "dls_fls_user" + ignore: 404 + +--- +"Search with document and field level security": + - do: + search: + rest_total_hits_as_int: true + request_cache: true + index: my_remote_cluster:shared_index + + - match: { hits.total: 2} + - length: { hits.hits.0._source: 3 } + - match: { hits.hits.0._source.secret: "sesame" } + + - do: + headers: { Authorization: "Basic ZGxzX2Zsc191c2VyOnMza3JpdC1wYXNzd29yZA==" } + search: + rest_total_hits_as_int: true + ccs_minimize_roundtrips: false + request_cache: true + index: my_remote_cluster:shared_index + + - match: { hits.total: 1} + - length: { hits.hits.0._source: 2 } + - is_true: hits.hits.0._source.public + - match: { hits.hits.0._source.name: "doc 1" } + +--- +"Async search with document and field level security": + - skip: + version: " - 7.6.99" + reason: "async search available since 7.7" + + - do: + async_search.submit: + index: my_remote_cluster:shared_index + wait_for_completion_timeout: 10s + + - match: { response.hits.total.value: 2} + - length: { response.hits.hits.0._source: 3 } + - match: { response.hits.hits.0._source.secret: "sesame" } + + - do: + headers: { Authorization: "Basic ZGxzX2Zsc191c2VyOnMza3JpdC1wYXNzd29yZA==" } + async_search.submit: + index: my_remote_cluster:shared_index + wait_for_completion_timeout: 10s + + - match: { response.hits.total.value: 1} + - length: { response.hits.hits.0._source: 2 } + - is_true: response.hits.hits.0._source.public + - match: { response.hits.hits.0._source.name: "doc 1" } + diff --git a/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/remote_cluster/10_basic.yml b/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/remote_cluster/10_basic.yml index b30d56c6b402a..f52b2f069e8c0 100644 --- a/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/remote_cluster/10_basic.yml +++ b/x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/remote_cluster/10_basic.yml @@ -51,6 +51,23 @@ setup: } ] } + + - do: + security.put_role: + name: "dls_fls_role" + body: > + { + "cluster": ["monitor"], + "indices": [ + { + "names": ["shared_index"], + "privileges": ["read", "read_cross_cluster"], + "query": "{ \"term\": { \"public\" : true } }", + "field_security": { "grant": ["*"], "except": ["secret"] } + } + ] + } + --- "Index data and search on the remote cluster": - skip: @@ -301,3 +318,20 @@ setup: - '{"index": {"_index": "point_in_time_index"}}' - '{"f": "r4", "created_at" : "2020-01-04"}' + - do: + indices.create: + index: shared_index + body: + settings: + index: + number_of_shards: 1 + number_of_replicas: 0 + + - do: + bulk: + refresh: true + body: + - '{"index": {"_index": "shared_index", "_id": 1}}' + - '{"public": true, "name": "doc 1", "secret": "sesame"}' + - '{"index": {"_index": "shared_index", "_id": 2}}' + - '{"public": false, "name": "doc 2", "secret": "sesame"}'