From ce478621fc8386b19cc99d169894af791cd60e59 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Fri, 18 Aug 2023 16:33:48 -0400 Subject: [PATCH 1/8] cat indices fix Signed-off-by: Derek Ho --- .../privileges/PrivilegesEvaluatorTest.java | 16 +++++++++++++++- .../security/resolver/IndexResolverReplacer.java | 13 +++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java index 9f9da4366c..9c50a0d402 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java @@ -49,6 +49,10 @@ public class PrivilegesEvaluatorTest { new Role("search_template_role").indexPermissions("read").on("services").clusterPermissions("cluster_composite_ops") ); + protected final static TestSecurityConfig.User GET_INDICES = new TestSecurityConfig.User("get_indices_user").roles( + new Role("get_indices_role").indexPermissions("read").on("logs-*").clusterPermissions("cluster_composite_ops", "cluster_monitor") + ); + private String TEST_QUERY = "{\"source\":{\"query\":{\"match\":{\"service\":\"{{service_name}}\"}}},\"params\":{\"service_name\":\"Oracle\"}}"; @@ -57,7 +61,7 @@ public class PrivilegesEvaluatorTest { @ClassRule public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) .authc(AUTHC_HTTPBASIC_INTERNAL) - .users(NEGATIVE_LOOKAHEAD, NEGATED_REGEX, SEARCH_TEMPLATE, TestSecurityConfig.User.USER_ADMIN) + .users(NEGATIVE_LOOKAHEAD, NEGATED_REGEX, SEARCH_TEMPLATE, TestSecurityConfig.User.USER_ADMIN, GET_INDICES) .plugin(MustacheModulePlugin.class) .build(); @@ -80,6 +84,16 @@ public void testRegexPattern() throws Exception { } + @Test + public void testGetIndicesSuccess() { + // Insert doc into services index with admin user + try (TestRestClient client = cluster.getRestClient(GET_INDICES)) { + final String searchTemplateOnServicesIndex = "/_cat/indices/logs-*"; + final TestRestClient.HttpResponse searchTemplateOnAuthorizedIndexResponse = client.get(searchTemplateOnServicesIndex); + assertThat(searchTemplateOnAuthorizedIndexResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + } + @Test public void testSearchTemplateRequestSuccess() { // Insert doc into services index with admin user diff --git a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java index ea8985ee69..5a95d0f253 100644 --- a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java +++ b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java @@ -57,6 +57,7 @@ import org.opensearch.action.admin.indices.datastream.CreateDataStreamAction; import org.opensearch.action.admin.indices.mapping.put.PutMappingRequest; import org.opensearch.action.admin.indices.resolve.ResolveIndexAction; +import org.opensearch.action.admin.indices.settings.get.GetSettingsRequest; import org.opensearch.action.admin.indices.template.put.PutComponentTemplateAction; import org.opensearch.action.bulk.BulkRequest; import org.opensearch.action.bulk.BulkShardRequest; @@ -750,11 +751,15 @@ private boolean getOrReplaceAllIndices(final Object request, final IndicesProvid } ((IndexRequest) request).index(newIndices.length != 1 ? null : newIndices[0]); } else if (request instanceof Replaceable) { - String[] newIndices = provider.provide(((Replaceable) request).indices(), request, true); - if (checkIndices(request, newIndices, false, allowEmptyIndices) == false) { - return false; + if (request instanceof GetSettingsRequest) { + provider.provide(((GetSettingsRequest) request).indices(), request, true); + } else { + String[] newIndices = provider.provide(((Replaceable) request).indices(), request, true); + if (checkIndices(request, newIndices, false, allowEmptyIndices) == false) { + return false; + } + ((Replaceable) request).indices(newIndices); } - ((Replaceable) request).indices(newIndices); } else if (request instanceof BulkShardRequest) { provider.provide(((ReplicationRequest) request).indices(), request, false); // replace not supported? From 2bffbdcf5866441e965d70613459452fefefe207 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Mon, 21 Aug 2023 13:29:43 -0400 Subject: [PATCH 2/8] create new dnfof test suite Signed-off-by: Derek Ho --- .../PrivilegesEvaluatorDNFOFTest.java | 42 +++++++++++++++++++ .../privileges/PrivilegesEvaluatorTest.java | 10 ----- .../resolver/IndexResolverReplacer.java | 12 ++---- 3 files changed, 46 insertions(+), 18 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java diff --git a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java new file mode 100644 index 0000000000..57478a8453 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java @@ -0,0 +1,42 @@ +package org.opensearch.security.privileges; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.hc.core5.http.HttpStatus; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.opensearch.script.mustache.MustacheModulePlugin; +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class PrivilegesEvaluatorDNFOFTest { + + protected final static TestSecurityConfig.User GET_INDICES = new TestSecurityConfig.User("get_indices_user").roles( + new TestSecurityConfig.Role("get_indices_role").indexPermissions("read").on("logs-*").clusterPermissions("cluster_composite_ops", "cluster_monitor") + ); + + @ClassRule + public static LocalCluster dnfofCluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(TestSecurityConfig.User.USER_ADMIN, GET_INDICES) + .doNotFailOnForbidden(true) + .build(); + + @Test + public void testGetIndicesSuccess() { + // Insert doc into services index with admin user + try (TestRestClient client = dnfofCluster.getRestClient(GET_INDICES)) { + final String searchTemplateOnServicesIndex = "/_cat/indices"; + final TestRestClient.HttpResponse searchTemplateOnAuthorizedIndexResponse = client.get(searchTemplateOnServicesIndex); + assertThat(searchTemplateOnAuthorizedIndexResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + } +} diff --git a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java index 9c50a0d402..accd241013 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java @@ -84,16 +84,6 @@ public void testRegexPattern() throws Exception { } - @Test - public void testGetIndicesSuccess() { - // Insert doc into services index with admin user - try (TestRestClient client = cluster.getRestClient(GET_INDICES)) { - final String searchTemplateOnServicesIndex = "/_cat/indices/logs-*"; - final TestRestClient.HttpResponse searchTemplateOnAuthorizedIndexResponse = client.get(searchTemplateOnServicesIndex); - assertThat(searchTemplateOnAuthorizedIndexResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); - } - } - @Test public void testSearchTemplateRequestSuccess() { // Insert doc into services index with admin user diff --git a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java index 5a95d0f253..396630dec3 100644 --- a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java +++ b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java @@ -751,15 +751,11 @@ private boolean getOrReplaceAllIndices(final Object request, final IndicesProvid } ((IndexRequest) request).index(newIndices.length != 1 ? null : newIndices[0]); } else if (request instanceof Replaceable) { - if (request instanceof GetSettingsRequest) { - provider.provide(((GetSettingsRequest) request).indices(), request, true); - } else { - String[] newIndices = provider.provide(((Replaceable) request).indices(), request, true); - if (checkIndices(request, newIndices, false, allowEmptyIndices) == false) { - return false; - } - ((Replaceable) request).indices(newIndices); + String[] newIndices = provider.provide(((Replaceable) request).indices(), request, true); + if (checkIndices(request, newIndices, false, allowEmptyIndices) == false) { + return false; } + ((Replaceable) request).indices(newIndices); } else if (request instanceof BulkShardRequest) { provider.provide(((ReplicationRequest) request).indices(), request, false); // replace not supported? From e0c869e64bc077a43a1162524ba84f498a6cf96e Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Thu, 24 Aug 2023 11:33:48 -0400 Subject: [PATCH 3/8] fix teh test Signed-off-by: Derek Ho --- .../privileges/PrivilegesEvaluatorDNFOFTest.java | 11 +++++++++-- .../security/privileges/PrivilegesEvaluator.java | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java index 57478a8453..38e8d90278 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java @@ -5,7 +5,6 @@ import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; -import org.opensearch.script.mustache.MustacheModulePlugin; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; @@ -20,18 +19,26 @@ public class PrivilegesEvaluatorDNFOFTest { protected final static TestSecurityConfig.User GET_INDICES = new TestSecurityConfig.User("get_indices_user").roles( - new TestSecurityConfig.Role("get_indices_role").indexPermissions("read").on("logs-*").clusterPermissions("cluster_composite_ops", "cluster_monitor") + new TestSecurityConfig.Role("get_indices_role").indexPermissions("*").on("logs-*").clusterPermissions("*") ); + private String TEST_DOC = "{\"source\": {\"title\": \"Spirited Away\"}}"; + @ClassRule public static LocalCluster dnfofCluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) .authc(AUTHC_HTTPBASIC_INTERNAL) .users(TestSecurityConfig.User.USER_ADMIN, GET_INDICES) .doNotFailOnForbidden(true) + .anonymousAuth(false) .build(); @Test public void testGetIndicesSuccess() { + try (TestRestClient client = dnfofCluster.getRestClient(TestSecurityConfig.User.USER_ADMIN)) { + TestRestClient.HttpResponse response = client.postJson("logs-123/_doc", TEST_DOC); + assertThat(response.getStatusCode(), equalTo(HttpStatus.SC_CREATED)); + } + // Insert doc into services index with admin user try (TestRestClient client = dnfofCluster.getRestClient(GET_INDICES)) { final String searchTemplateOnServicesIndex = "/_cat/indices"; diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 3ac120b35a..380d8f79d8 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -104,7 +104,7 @@ public class PrivilegesEvaluator { private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*"); private static final Pattern DNFOF_PATTERNS = Pattern.compile( - "indices:(data/read/.*|(admin/(mappings/fields/get.*|shards/search_shards|resolve/index)))" + "indices:(data/read/.*|(admin/(mappings/fields/get.*|shards/search_shards|resolve/index))|(monitor/((settings/get)|(stats))))" ); private static final IndicesOptions ALLOW_EMPTY = IndicesOptions.fromOptions(true, true, false, false); From 89ddca30154acb846a9cd39c88b23caa3fbcdff3 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Thu, 24 Aug 2023 12:01:49 -0400 Subject: [PATCH 4/8] code cleanup and spotlessApply Signed-off-by: Derek Ho --- .../PrivilegesEvaluatorDNFOFTest.java | 22 ++++++++++--------- .../privileges/PrivilegesEvaluatorTest.java | 6 +---- .../resolver/IndexResolverReplacer.java | 1 - 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java index 38e8d90278..45503c0852 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java @@ -11,6 +11,7 @@ import org.opensearch.test.framework.cluster.TestRestClient; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; @@ -19,31 +20,32 @@ public class PrivilegesEvaluatorDNFOFTest { protected final static TestSecurityConfig.User GET_INDICES = new TestSecurityConfig.User("get_indices_user").roles( - new TestSecurityConfig.Role("get_indices_role").indexPermissions("*").on("logs-*").clusterPermissions("*") + new TestSecurityConfig.Role("get_indices_role").indexPermissions("*").on("logs-*").clusterPermissions("*") ); private String TEST_DOC = "{\"source\": {\"title\": \"Spirited Away\"}}"; @ClassRule public static LocalCluster dnfofCluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) - .authc(AUTHC_HTTPBASIC_INTERNAL) - .users(TestSecurityConfig.User.USER_ADMIN, GET_INDICES) - .doNotFailOnForbidden(true) - .anonymousAuth(false) - .build(); + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(TestSecurityConfig.User.USER_ADMIN, GET_INDICES) + .doNotFailOnForbidden(true) + .anonymousAuth(false) + .build(); @Test public void testGetIndicesSuccess() { + // Insert doc into logs-123 index with admin user try (TestRestClient client = dnfofCluster.getRestClient(TestSecurityConfig.User.USER_ADMIN)) { TestRestClient.HttpResponse response = client.postJson("logs-123/_doc", TEST_DOC); assertThat(response.getStatusCode(), equalTo(HttpStatus.SC_CREATED)); } - // Insert doc into services index with admin user try (TestRestClient client = dnfofCluster.getRestClient(GET_INDICES)) { - final String searchTemplateOnServicesIndex = "/_cat/indices"; - final TestRestClient.HttpResponse searchTemplateOnAuthorizedIndexResponse = client.get(searchTemplateOnServicesIndex); - assertThat(searchTemplateOnAuthorizedIndexResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); + final String catIndices = "/_cat/indices"; + final TestRestClient.HttpResponse catIndicesResponse = client.get(catIndices); + assertThat(catIndicesResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); + assertThat(catIndicesResponse.getBody(), containsString("logs-123")); } } } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java index accd241013..9f9da4366c 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java @@ -49,10 +49,6 @@ public class PrivilegesEvaluatorTest { new Role("search_template_role").indexPermissions("read").on("services").clusterPermissions("cluster_composite_ops") ); - protected final static TestSecurityConfig.User GET_INDICES = new TestSecurityConfig.User("get_indices_user").roles( - new Role("get_indices_role").indexPermissions("read").on("logs-*").clusterPermissions("cluster_composite_ops", "cluster_monitor") - ); - private String TEST_QUERY = "{\"source\":{\"query\":{\"match\":{\"service\":\"{{service_name}}\"}}},\"params\":{\"service_name\":\"Oracle\"}}"; @@ -61,7 +57,7 @@ public class PrivilegesEvaluatorTest { @ClassRule public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) .authc(AUTHC_HTTPBASIC_INTERNAL) - .users(NEGATIVE_LOOKAHEAD, NEGATED_REGEX, SEARCH_TEMPLATE, TestSecurityConfig.User.USER_ADMIN, GET_INDICES) + .users(NEGATIVE_LOOKAHEAD, NEGATED_REGEX, SEARCH_TEMPLATE, TestSecurityConfig.User.USER_ADMIN) .plugin(MustacheModulePlugin.class) .build(); diff --git a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java index 396630dec3..ea8985ee69 100644 --- a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java +++ b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java @@ -57,7 +57,6 @@ import org.opensearch.action.admin.indices.datastream.CreateDataStreamAction; import org.opensearch.action.admin.indices.mapping.put.PutMappingRequest; import org.opensearch.action.admin.indices.resolve.ResolveIndexAction; -import org.opensearch.action.admin.indices.settings.get.GetSettingsRequest; import org.opensearch.action.admin.indices.template.put.PutComponentTemplateAction; import org.opensearch.action.bulk.BulkRequest; import org.opensearch.action.bulk.BulkShardRequest; From 92d475bf02c966ae6359eeb222f234cd6766a003 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Thu, 24 Aug 2023 15:47:01 -0400 Subject: [PATCH 5/8] move to existing test file and add unit test to test patterns Signed-off-by: Derek Ho --- .../security/DoNotFailOnForbiddenTests.java | 70 ++++++++++++++++--- .../PrivilegesEvaluatorDNFOFTest.java | 51 -------------- .../privileges/PrivilegesEvaluator.java | 2 +- 3 files changed, 63 insertions(+), 60 deletions(-) delete mode 100644 src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java diff --git a/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java b/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java index fedcf0bbcb..dc71fe21cd 100644 --- a/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java +++ b/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java @@ -9,9 +9,14 @@ */ package org.opensearch.security; +import java.io.BufferedReader; import java.io.IOException; +import java.io.InputStreamReader; +import java.util.List; +import java.util.stream.Collectors; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import com.google.common.collect.ImmutableList; import org.hamcrest.Matchers; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -31,6 +36,8 @@ import org.opensearch.action.search.SearchResponse; import org.opensearch.action.search.SearchScrollRequest; import org.opensearch.client.Client; +import org.opensearch.client.Request; +import org.opensearch.client.Response; import org.opensearch.client.RestHighLevelClient; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.TestSecurityConfig.User; @@ -38,12 +45,7 @@ import org.opensearch.test.framework.cluster.LocalCluster; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.aMapWithSize; -import static org.hamcrest.Matchers.allOf; -import static org.hamcrest.Matchers.arrayContainingInAnyOrder; -import static org.hamcrest.Matchers.arrayWithSize; -import static org.hamcrest.Matchers.hasKey; -import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.*; import static org.opensearch.action.admin.indices.alias.IndicesAliasesRequest.AliasActions.Type.ADD; import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; import static org.opensearch.client.RequestOptions.DEFAULT; @@ -56,6 +58,7 @@ import static org.opensearch.security.Song.SONGS; import static org.opensearch.security.Song.TITLE_MAGNUM_OPUS; import static org.opensearch.security.Song.TITLE_NEXT_SONG; +import static org.opensearch.security.privileges.PrivilegesEvaluator.DNFOF_PATTERNS; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; import static org.opensearch.test.framework.cluster.SearchRequestFactory.averageAggregationRequest; @@ -97,12 +100,33 @@ public class DoNotFailOnForbiddenTests { private static final String ID_3 = "3"; private static final String ID_4 = "4"; + private static final List allowedDnfof = ImmutableList.of( + "indices:monitor/settings/get", + "indices:monitor/stats", + "indices:data/read/search", + "indices:data/read/msearch", + "indices:data/read/scroll", + "indices:data/read/mget", + "indices:admin/mappings/fields/get" + ); + + private static final List disallowedDnfof = ImmutableList.of( + "indices:admin/template/put", + "indices:data/write/index", + "indices:admin/create", + "indices:data/write/bulk", + "indices:admin/aliases", + "indices:data/write/reindex" + ); + private static final User ADMIN_USER = new User("admin").roles(ALL_ACCESS); private static final User LIMITED_USER = new User("limited_user").roles( new TestSecurityConfig.Role("limited-role").clusterPermissions( "indices:data/read/mget", "indices:data/read/msearch", - "indices:data/read/scroll" + "indices:data/read/scroll", + "cluster:monitor/state", + "cluster:monitor/health" ) .indexPermissions( "indices:data/read/search", @@ -110,7 +134,9 @@ public class DoNotFailOnForbiddenTests { "indices:data/read/field_caps", "indices:data/read/field_caps*", "indices:data/read/msearch", - "indices:data/read/scroll" + "indices:data/read/scroll", + "indices:monitor/settings/get", + "indices:monitor/stats" ) .on(MARVELOUS_SONGS) ); @@ -408,4 +434,32 @@ public void shouldPerformStatAggregation_negative() throws IOException { } } + @Test + public void shouldPerformCatIndices_positive() throws IOException { + try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) { + Request getIndicesRequest = new Request("GET", "/_cat/indices"); + // High level client doesn't support _cat/_indices API + Response getIndicesResponse = restHighLevelClient.getLowLevelClient().performRequest(getIndicesRequest); + List indexes = new BufferedReader(new InputStreamReader(getIndicesResponse.getEntity().getContent())).lines() + .collect(Collectors.toList()); + + assertThat(indexes.size(), equalTo(1)); + assertThat(indexes.get(0), containsString("marvelous_songs")); + } + } + + @Test + public void testDnfofPermissions_negative() { + for (final String permission : disallowedDnfof) { + assertThat(DNFOF_PATTERNS.matcher(permission).matches(), equalTo(false)); + } + } + + @Test + public void testDnfofPermissions_positive() { + for (final String permission : allowedDnfof) { + assertThat(DNFOF_PATTERNS.matcher(permission).matches(), equalTo(true)); + } + } + } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java b/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java deleted file mode 100644 index 45503c0852..0000000000 --- a/src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorDNFOFTest.java +++ /dev/null @@ -1,51 +0,0 @@ -package org.opensearch.security.privileges; - -import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; -import org.apache.hc.core5.http.HttpStatus; -import org.junit.ClassRule; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.opensearch.test.framework.TestSecurityConfig; -import org.opensearch.test.framework.cluster.ClusterManager; -import org.opensearch.test.framework.cluster.LocalCluster; -import org.opensearch.test.framework.cluster.TestRestClient; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; -import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; - -@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) -@ThreadLeakScope(ThreadLeakScope.Scope.NONE) -public class PrivilegesEvaluatorDNFOFTest { - - protected final static TestSecurityConfig.User GET_INDICES = new TestSecurityConfig.User("get_indices_user").roles( - new TestSecurityConfig.Role("get_indices_role").indexPermissions("*").on("logs-*").clusterPermissions("*") - ); - - private String TEST_DOC = "{\"source\": {\"title\": \"Spirited Away\"}}"; - - @ClassRule - public static LocalCluster dnfofCluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) - .authc(AUTHC_HTTPBASIC_INTERNAL) - .users(TestSecurityConfig.User.USER_ADMIN, GET_INDICES) - .doNotFailOnForbidden(true) - .anonymousAuth(false) - .build(); - - @Test - public void testGetIndicesSuccess() { - // Insert doc into logs-123 index with admin user - try (TestRestClient client = dnfofCluster.getRestClient(TestSecurityConfig.User.USER_ADMIN)) { - TestRestClient.HttpResponse response = client.postJson("logs-123/_doc", TEST_DOC); - assertThat(response.getStatusCode(), equalTo(HttpStatus.SC_CREATED)); - } - - try (TestRestClient client = dnfofCluster.getRestClient(GET_INDICES)) { - final String catIndices = "/_cat/indices"; - final TestRestClient.HttpResponse catIndicesResponse = client.get(catIndices); - assertThat(catIndicesResponse.getStatusCode(), equalTo(HttpStatus.SC_OK)); - assertThat(catIndicesResponse.getBody(), containsString("logs-123")); - } - } -} diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 380d8f79d8..bf79558158 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -103,7 +103,7 @@ public class PrivilegesEvaluator { private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*"); - private static final Pattern DNFOF_PATTERNS = Pattern.compile( + public static final Pattern DNFOF_PATTERNS = Pattern.compile( "indices:(data/read/.*|(admin/(mappings/fields/get.*|shards/search_shards|resolve/index))|(monitor/((settings/get)|(stats))))" ); From 41858729da8b2af78dd7bde4267613deaf611fd2 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Fri, 25 Aug 2023 15:44:38 -0400 Subject: [PATCH 6/8] move unit test over and create comprehensive list of index permissions Signed-off-by: Derek Ho --- .../security/DoNotFailOnForbiddenTests.java | 35 ------- .../privileges/PrivilegesEvaluator.java | 2 +- .../PrivilegesEvaluatorUnitTest.java | 95 +++++++++++++++++++ 3 files changed, 96 insertions(+), 36 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java b/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java index dc71fe21cd..138a57e60b 100644 --- a/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java +++ b/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java @@ -16,7 +16,6 @@ import java.util.stream.Collectors; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; -import com.google.common.collect.ImmutableList; import org.hamcrest.Matchers; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -58,7 +57,6 @@ import static org.opensearch.security.Song.SONGS; import static org.opensearch.security.Song.TITLE_MAGNUM_OPUS; import static org.opensearch.security.Song.TITLE_NEXT_SONG; -import static org.opensearch.security.privileges.PrivilegesEvaluator.DNFOF_PATTERNS; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; import static org.opensearch.test.framework.cluster.SearchRequestFactory.averageAggregationRequest; @@ -100,25 +98,6 @@ public class DoNotFailOnForbiddenTests { private static final String ID_3 = "3"; private static final String ID_4 = "4"; - private static final List allowedDnfof = ImmutableList.of( - "indices:monitor/settings/get", - "indices:monitor/stats", - "indices:data/read/search", - "indices:data/read/msearch", - "indices:data/read/scroll", - "indices:data/read/mget", - "indices:admin/mappings/fields/get" - ); - - private static final List disallowedDnfof = ImmutableList.of( - "indices:admin/template/put", - "indices:data/write/index", - "indices:admin/create", - "indices:data/write/bulk", - "indices:admin/aliases", - "indices:data/write/reindex" - ); - private static final User ADMIN_USER = new User("admin").roles(ALL_ACCESS); private static final User LIMITED_USER = new User("limited_user").roles( new TestSecurityConfig.Role("limited-role").clusterPermissions( @@ -448,18 +427,4 @@ public void shouldPerformCatIndices_positive() throws IOException { } } - @Test - public void testDnfofPermissions_negative() { - for (final String permission : disallowedDnfof) { - assertThat(DNFOF_PATTERNS.matcher(permission).matches(), equalTo(false)); - } - } - - @Test - public void testDnfofPermissions_positive() { - for (final String permission : allowedDnfof) { - assertThat(DNFOF_PATTERNS.matcher(permission).matches(), equalTo(true)); - } - } - } diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index bf79558158..0690e49787 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -103,7 +103,7 @@ public class PrivilegesEvaluator { private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*"); - public static final Pattern DNFOF_PATTERNS = Pattern.compile( + static final Pattern DNFOF_PATTERNS = Pattern.compile( "indices:(data/read/.*|(admin/(mappings/fields/get.*|shards/search_shards|resolve/index))|(monitor/((settings/get)|(stats))))" ); diff --git a/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java b/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java index e7412f43b4..49d1bbcb78 100644 --- a/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java +++ b/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java @@ -8,14 +8,95 @@ package org.opensearch.security.privileges; +import com.google.common.collect.ImmutableList; import org.junit.Test; +import java.util.List; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.opensearch.security.privileges.PrivilegesEvaluator.DNFOF_PATTERNS; import static org.opensearch.security.privileges.PrivilegesEvaluator.isClusterPerm; public class PrivilegesEvaluatorUnitTest { + private static final List allowedDnfof = ImmutableList.of( + "indices:admin/mappings/fields/get", + "indices:admin/resolve/index", + "indices:admin/shards/search_shards", + "indices:data/read/explain", + "indices:data/read/field_caps", + "indices:data/read/get", + "indices:data/read/mget", + "indices:data/read/msearch", + "indices:data/read/msearch/template", + "indices:data/read/mtv", + "indices:data/read/plugins/replication/file_chunk", + "indices:data/read/plugins/replication/changes", + "indices:data/read/scroll", + "indices:data/read/scroll/clear", + "indices:data/read/search", + "indices:data/read/search/template", + "indices:data/read/tv", + "indices:monitor/settings/get", + "indices:monitor/stats" + ); + + private static final List disallowedDnfof = ImmutableList.of( + "indices:admin/aliases", + "indices:admin/aliases/exists", + "indices:admin/aliases/get", + "indices:admin/analyze", + "indices:admin/cache/clear", + "indices:admin/close", + "indices:admin/create", + "indices:admin/data_stream/create", + "indices:admin/data_stream/delete", + "indices:admin/data_stream/get", + "indices:admin/delete", + "indices:admin/exists", + "indices:admin/flush", + "indices:admin/forcemerge", + "indices:admin/get", + "indices:admin/mapping/put", + "indices:admin/mappings/get", + "indices:admin/open", + "indices:admin/plugins/replication/index/setup/validate", + "indices:admin/plugins/replication/index/start", + "indices:admin/plugins/replication/index/pause", + "indices:admin/plugins/replication/index/resume", + "indices:admin/plugins/replication/index/stop", + "indices:admin/plugins/replication/index/update", + "indices:admin/plugins/replication/index/status_check", + "indices:admin/refresh", + "indices:admin/rollover", + "indices:admin/seq_no/global_checkpoint_sync", + "indices:admin/settings/update", + "indices:admin/shrink", + "indices:admin/synced_flush", + "indices:admin/template/delete", + "indices:admin/template/get", + "indices:admin/template/put", + "indices:admin/types/exists", + "indices:admin/upgrade", + "indices:admin/validate/query", + "indices:data/write/bulk", + "indices:data/write/delete", + "indices:data/write/delete/byquery", + "indices:data/write/plugins/replication/changes", + "indices:data/write/index", + "indices:data/write/reindex", + "indices:data/write/update", + "indices:data/write/update/byquery", + "indices:monitor/data_stream/stats", + "indices:monitor/recovery", + "indices:monitor/segments", + "indices:monitor/shard_stores", + "indices:monitor/upgrade" + ); + @Test public void testClusterPerm() { String multiSearchTemplate = "indices:data/read/msearch/template"; @@ -33,4 +114,18 @@ public void testClusterPerm() { assertFalse(isClusterPerm(adminClose)); assertFalse(isClusterPerm(monitorUpgrade)); } + + @Test + public void testDnfofPermissions_negative() { + for (final String permission : disallowedDnfof) { + assertThat(DNFOF_PATTERNS.matcher(permission).matches(), equalTo(false)); + } + } + + @Test + public void testDnfofPermissions_positive() { + for (final String permission : allowedDnfof) { + assertThat(DNFOF_PATTERNS.matcher(permission).matches(), equalTo(true)); + } + } } From 20493d846812187308e8324077ab489cd7c0da99 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Fri, 25 Aug 2023 17:21:29 -0400 Subject: [PATCH 7/8] turn regex into a function and update unit test to use it Signed-off-by: Derek Ho --- .../security/privileges/PrivilegesEvaluator.java | 16 ++++++++++------ .../privileges/PrivilegesEvaluatorUnitTest.java | 6 +++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 0690e49787..dba4539488 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -35,7 +35,6 @@ import java.util.Map; import java.util.Set; import java.util.StringJoiner; -import java.util.regex.Pattern; import com.google.common.collect.ImmutableSet; import org.apache.logging.log4j.LogManager; @@ -103,10 +102,6 @@ public class PrivilegesEvaluator { private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*"); - static final Pattern DNFOF_PATTERNS = Pattern.compile( - "indices:(data/read/.*|(admin/(mappings/fields/get.*|shards/search_shards|resolve/index))|(monitor/((settings/get)|(stats))))" - ); - private static final IndicesOptions ALLOW_EMPTY = IndicesOptions.fromOptions(true, true, false, false); protected final Logger log = LogManager.getLogger(this.getClass()); @@ -473,7 +468,7 @@ public PrivilegesEvaluatorResponse evaluate( } } - if (dnfofEnabled && DNFOF_PATTERNS.matcher(action0).matches()) { + if (dnfofEnabled && isDnfofAction(action0)) { if (requestedResolved.getAllIndices().isEmpty()) { presponse.missingPrivileges.clear(); @@ -674,6 +669,15 @@ public static boolean isClusterPerm(String action0) { ); } + static boolean isDnfofAction(String action0) { + return (action0.startsWith("indices:data/read/") + || (action0.startsWith("indices:admin/mappings/fields/get") + || action0.equals("indices:admin/shards/search_shards") + || action0.equals("indices:admin/resolve/index")) + || action0.equals("indices:monitor/settings/get") + || action0.equals("indices:monitor/stats")); + } + @SuppressWarnings("unchecked") private boolean checkFilteredAliases(Resolved requestedResolved, String action, boolean isDebugEnabled) { final String faMode = dcm.getFilteredAliasMode();// getConfigSettings().dynamic.filtered_alias_mode; diff --git a/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java b/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java index 49d1bbcb78..79ff6c387e 100644 --- a/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java +++ b/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java @@ -17,8 +17,8 @@ import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.opensearch.security.privileges.PrivilegesEvaluator.DNFOF_PATTERNS; import static org.opensearch.security.privileges.PrivilegesEvaluator.isClusterPerm; +import static org.opensearch.security.privileges.PrivilegesEvaluator.isDnfofAction; public class PrivilegesEvaluatorUnitTest { @@ -118,14 +118,14 @@ public void testClusterPerm() { @Test public void testDnfofPermissions_negative() { for (final String permission : disallowedDnfof) { - assertThat(DNFOF_PATTERNS.matcher(permission).matches(), equalTo(false)); + assertThat(isDnfofAction(permission), equalTo(false)); } } @Test public void testDnfofPermissions_positive() { for (final String permission : allowedDnfof) { - assertThat(DNFOF_PATTERNS.matcher(permission).matches(), equalTo(true)); + assertThat(isDnfofAction(permission), equalTo(true)); } } } From c12333ce5e1442bdd206d23664a5b71e43787203 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Mon, 28 Aug 2023 10:18:01 -0400 Subject: [PATCH 8/8] change to wildcard matcher Signed-off-by: Derek Ho --- .../privileges/PrivilegesEvaluator.java | 23 +++++++++++-------- .../PrivilegesEvaluatorUnitTest.java | 7 +++--- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index dba4539488..83dba986ee 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -36,6 +36,7 @@ import java.util.Set; import java.util.StringJoiner; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -100,6 +101,17 @@ public class PrivilegesEvaluator { + static final WildcardMatcher DNFOF_MATCHER = WildcardMatcher.from( + ImmutableList.of( + "indices:data/read/*", + "indices:admin/mappings/fields/get*", + "indices:admin/shards/search_shards", + "indices:admin/resolve/index", + "indices:monitor/settings/get", + "indices:monitor/stats" + ) + ); + private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*"); private static final IndicesOptions ALLOW_EMPTY = IndicesOptions.fromOptions(true, true, false, false); @@ -468,7 +480,7 @@ public PrivilegesEvaluatorResponse evaluate( } } - if (dnfofEnabled && isDnfofAction(action0)) { + if (dnfofEnabled && DNFOF_MATCHER.test(action0)) { if (requestedResolved.getAllIndices().isEmpty()) { presponse.missingPrivileges.clear(); @@ -669,15 +681,6 @@ public static boolean isClusterPerm(String action0) { ); } - static boolean isDnfofAction(String action0) { - return (action0.startsWith("indices:data/read/") - || (action0.startsWith("indices:admin/mappings/fields/get") - || action0.equals("indices:admin/shards/search_shards") - || action0.equals("indices:admin/resolve/index")) - || action0.equals("indices:monitor/settings/get") - || action0.equals("indices:monitor/stats")); - } - @SuppressWarnings("unchecked") private boolean checkFilteredAliases(Resolved requestedResolved, String action, boolean isDebugEnabled) { final String faMode = dcm.getFilteredAliasMode();// getConfigSettings().dynamic.filtered_alias_mode; diff --git a/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java b/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java index 79ff6c387e..1b1874a106 100644 --- a/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java +++ b/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java @@ -17,8 +17,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.opensearch.security.privileges.PrivilegesEvaluator.isClusterPerm; -import static org.opensearch.security.privileges.PrivilegesEvaluator.isDnfofAction; +import static org.opensearch.security.privileges.PrivilegesEvaluator.*; public class PrivilegesEvaluatorUnitTest { @@ -118,14 +117,14 @@ public void testClusterPerm() { @Test public void testDnfofPermissions_negative() { for (final String permission : disallowedDnfof) { - assertThat(isDnfofAction(permission), equalTo(false)); + assertThat(DNFOF_MATCHER.test(permission), equalTo(false)); } } @Test public void testDnfofPermissions_positive() { for (final String permission : allowedDnfof) { - assertThat(isDnfofAction(permission), equalTo(true)); + assertThat(DNFOF_MATCHER.test(permission), equalTo(true)); } } }