From d31808de80f368263a84617b99fd7f404058d2f0 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Mon, 20 Jul 2020 08:44:54 +0300 Subject: [PATCH] Fix DLS/FLS permission for the submit async search action (#59693) The submit async search action should not populate the thread context DLS/FLS permission set, because it is not currently authorised as an "indices request" and hence the permission set that it builds is incomplete and it overrides the DLS/FLS permission set of the actual spawned search request (which is built correctly). --- .../async-search/qa/security/build.gradle | 1 + .../plugin/async-search/qa/security/roles.yml | 24 ++++++++ .../xpack/search/AsyncSearchSecurityIT.java | 61 ++++++++++++++++++- .../xpack/security/authz/RBACEngine.java | 12 +++- 4 files changed, 95 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/async-search/qa/security/build.gradle b/x-pack/plugin/async-search/qa/security/build.gradle index 8f2d0fb72859b..b46cd20397ac9 100644 --- a/x-pack/plugin/async-search/qa/security/build.gradle +++ b/x-pack/plugin/async-search/qa/security/build.gradle @@ -17,4 +17,5 @@ testClusters.integTest { user username: "test-admin", password: 'x-pack-test-password', role: "test-admin" user username: "user1", password: 'x-pack-test-password', role: "user1" user username: "user2", password: 'x-pack-test-password', role: "user2" + user username: "user_dls", password: 'x-pack-test-password', role: "user_dls" } diff --git a/x-pack/plugin/async-search/qa/security/roles.yml b/x-pack/plugin/async-search/qa/security/roles.yml index 4ab3be5ff0571..3dd737d30cadb 100644 --- a/x-pack/plugin/async-search/qa/security/roles.yml +++ b/x-pack/plugin/async-search/qa/security/roles.yml @@ -31,3 +31,27 @@ user2: - write - create_index - indices:admin/refresh + +user_dls: + cluster: + - cluster:monitor/main + indices: + - names: + - 'index*' + privileges: + - read + field_security: + grant: + - baz + query: | + { + "bool": { + "must_not": [ + { + "match": { + "foo": "bar" + } + } + ] + } + } diff --git a/x-pack/plugin/async-search/qa/security/src/test/java/org/elasticsearch/xpack/search/AsyncSearchSecurityIT.java b/x-pack/plugin/async-search/qa/security/src/test/java/org/elasticsearch/xpack/search/AsyncSearchSecurityIT.java index 9db013e7efda1..d4f75b1fd39b1 100644 --- a/x-pack/plugin/async-search/qa/security/src/test/java/org/elasticsearch/xpack/search/AsyncSearchSecurityIT.java +++ b/x-pack/plugin/async-search/qa/security/src/test/java/org/elasticsearch/xpack/search/AsyncSearchSecurityIT.java @@ -11,25 +11,34 @@ import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; import org.elasticsearch.client.ResponseException; +import org.elasticsearch.client.asyncsearch.AsyncSearchResponse; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.search.SearchHit; import org.elasticsearch.test.rest.ESRestTestCase; import org.elasticsearch.xpack.core.async.AsyncExecutionId; +import org.hamcrest.CustomMatcher; import org.junit.Before; import java.io.IOException; +import java.util.List; import java.util.Map; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.elasticsearch.xpack.core.XPackPlugin.ASYNC_RESULTS_INDEX; import static org.elasticsearch.xpack.core.security.authc.AuthenticationServiceField.RUN_AS_USER_HEADER; import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; -import static org.elasticsearch.xpack.core.XPackPlugin.ASYNC_RESULTS_INDEX; +import static org.hamcrest.Matchers.arrayContainingInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -49,6 +58,8 @@ protected Settings restClientSettings() { public void indexDocuments() throws IOException { createIndex("index", Settings.EMPTY); index("index", "0", "foo", "bar"); + index("index", "1", "bar", "baz"); + index("index", "2", "baz", "boo"); refresh("index"); createIndex("index-user1", Settings.EMPTY); @@ -57,9 +68,51 @@ public void indexDocuments() throws IOException { createIndex("index-user2", Settings.EMPTY); index("index-user2", "0", "foo", "bar"); + index("index-user2", "1", "bar", "baz"); refresh("index-user2"); } + public void testWithDlsAndFls() throws Exception { + Response submitResp = submitAsyncSearch("*", "*", TimeValue.timeValueSeconds(10), "user_dls"); + assertOK(submitResp); + String id = extractResponseId(submitResp); + Response getResp = getAsyncSearch(id, "user_dls"); + AsyncSearchResponse searchResponse = AsyncSearchResponse.fromXContent(XContentHelper.createParser(NamedXContentRegistry.EMPTY, + LoggingDeprecationHandler.INSTANCE, + new BytesArray(EntityUtils.toByteArray(getResp.getEntity())), + XContentType.JSON)); + SearchHit[] hits = searchResponse.getSearchResponse().getHits().getHits(); + + assertThat(hits, arrayContainingInAnyOrder( + new CustomMatcher("\"index\" doc 1 matcher") { + @Override + public boolean matches(Object actual) { + SearchHit hit = (SearchHit) actual; + return "index".equals(hit.getIndex()) && + "1".equals(hit.getId()) && + hit.getSourceAsMap().isEmpty(); + } + }, + new CustomMatcher("\"index\" doc 2 matcher") { + @Override + public boolean matches(Object actual) { + SearchHit hit = (SearchHit) actual; + return "index".equals(hit.getIndex()) && + "2".equals(hit.getId()) && + "boo".equals(hit.getSourceAsMap().get("baz")); + } + }, + new CustomMatcher("\"index-user2\" doc 1 matcher") { + @Override + public boolean matches(Object actual) { + SearchHit hit = (SearchHit) actual; + return "index-user2".equals(hit.getIndex()) && + "1".equals(hit.getId()) && + hit.getSourceAsMap().isEmpty(); + } + })); + } + public void testWithUsers() throws Exception { testCase("user1", "user2"); testCase("user2", "user1"); @@ -103,6 +156,12 @@ static String extractResponseId(Response response) throws IOException { return (String) map.get("id"); } + @SuppressWarnings("unchecked") + static List>> extractHits(Map respMap) { + Map response = ((Map) respMap.get("response")); + return ((List>>)((Map) response.get("hits")).get("hits")); + } + static void index(String index, String id, Object... fields) throws IOException { XContentBuilder document = jsonBuilder().startObject(); for (int i = 0; i < fields.length; i += 2) { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java index 12f7884b7debb..9d9ff5671d592 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java @@ -269,8 +269,16 @@ public void authorizeIndexAction(RequestInfo requestInfo, AuthorizationInfo auth listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.ALLOW_NO_INDICES)); } } else if (isAsyncRelatedAction(action)) { - //index-level permissions are handled by the search action that is triggered internally by the submit API. - listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.ALLOW_NO_INDICES)); + if (SubmitAsyncSearchAction.NAME.equals(action)) { + // authorize submit async search but don't fill in the DLS/FLS permissions + // the `null` IndicesAccessControl parameter indicates that this action has *not* determined + // which DLS/FLS controls should be applied to this action + listener.onResponse(new IndexAuthorizationResult(true, null)); + } else { + // async-search actions other than submit have a custom security layer that checks if the current user is + // the same as the user that submitted the original request so no additional checks are needed here. + listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.ALLOW_NO_INDICES)); + } } else { assert false : "only scroll and async-search related requests are known indices api that don't " + "support retrieving the indices they relate to";