From 259fec8571e290437c96007b93262fbcdf983851 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 15 Oct 2021 18:29:32 +1100 Subject: [PATCH] Improve permission granting for index referred by multiple names (#78902) (#79220) A concrete index can be referred explicitly or implicitly by multiple names including its own name, data stream name, aliases. This PR ensures the permission granting is done correctly in an edge case where the same concrete index is referred by multiple names and the user have different (disagreeing) privileges over these names. --- .../authz/permission/IndicesPermission.java | 13 +++++-- .../MultipleIndicesPermissionsTests.java | 35 ++++++++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java index 06417c7b36d23..ec99c2a3eb587 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java @@ -265,6 +265,10 @@ public Collection resolveConcreteIndices() { return concreteIndices; } } + + public boolean canHaveBackingIndices() { + return indexAbstraction != null && indexAbstraction.getType() != IndexAbstraction.Type.CONCRETE_INDEX; + } } /** @@ -391,8 +395,13 @@ public Map authorize( } grantedBuilder.put(resource.name, granted); - for (String concreteIndex : concreteIndices) { - grantedBuilder.put(concreteIndex, granted); + if (resource.canHaveBackingIndices()) { + for (String concreteIndex : concreteIndices) { + // If the name appear directly as part of the requested indices, it takes precedence over implicit access + if (false == requestedIndicesOrAliases.contains(concreteIndex)) { + grantedBuilder.merge(concreteIndex, granted, Boolean::logicalOr); + } + } } } diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/MultipleIndicesPermissionsTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/MultipleIndicesPermissionsTests.java index e0c760deadd02..881e5a46a1588 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/MultipleIndicesPermissionsTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/MultipleIndicesPermissionsTests.java @@ -8,6 +8,7 @@ import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.DocWriteResponse; +import org.elasticsearch.action.admin.indices.alias.Alias; import org.elasticsearch.action.admin.indices.recovery.RecoveryResponse; import org.elasticsearch.action.admin.indices.segments.IndicesSegmentResponse; import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse; @@ -22,6 +23,7 @@ import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.SecurityIntegTestCase; import org.elasticsearch.test.SecuritySettingsSource; @@ -33,14 +35,18 @@ import java.util.Collections; +import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.BASIC_AUTH_HEADER; import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; -import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; public class MultipleIndicesPermissionsTests extends SecurityIntegTestCase { @@ -81,6 +87,8 @@ protected String configRoles() { " indices:\n" + " - names: 'a'\n" + " privileges: [all]\n" + + " - names: 'alias1'\n" + + " privileges: [read]\n" + "\n" + "role_monitor_all_unrestricted_indices:\n" + " cluster: [monitor]\n" + @@ -304,4 +312,29 @@ public void testMultipleRoles() throws Exception { assertNoFailures(response); assertHitCount(response, 2); } + + public void testMultiNamesWorkCorrectly() { + assertAcked(client().admin().indices().prepareCreate("index1") + .addMapping("_doc","field1", "type=text") + .addAlias(new Alias("alias1").filter(QueryBuilders.termQuery("field1", "public"))) + ); + + client().prepareIndex("index1", "_doc").setId("1").setSource("field1", "private").setRefreshPolicy(IMMEDIATE).get(); + + final Client userAClient = client() + .filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user_a", USERS_PASSWD))); + + final SearchResponse searchResponse = userAClient + .prepareSearch("alias1").setSize(0).get(); + assertThat(searchResponse.getHits().getTotalHits().value, equalTo(0L)); + + final ElasticsearchSecurityException e1 = + expectThrows(ElasticsearchSecurityException.class, () -> userAClient.prepareSearch("index1").get()); + assertThat(e1.getMessage(), containsString("is unauthorized for user [user_a]")); + + // The request should fail because index1 is directly requested and is not authorized for user_a + final ElasticsearchSecurityException e2 = + expectThrows(ElasticsearchSecurityException.class, () -> userAClient.prepareSearch("index1", "alias1").get()); + assertThat(e2.getMessage(), containsString("is unauthorized for user [user_a]")); + } }