From 81974316a779dbb23cccc4ca383396b1bc37c7f5 Mon Sep 17 00:00:00 2001 From: Derek Ho Date: Tue, 29 Aug 2023 14:26:51 -0400 Subject: [PATCH] [Backport 2.x] Command `cat/indices` will filter results per the do not fail on forbidden setting (#3258) ### Description Backport 4c095d2 of #3236 ### Check List - [ ] New functionality includes testing - [ ] New functionality has been documented - [X] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Derek Ho --- .../privileges/PrivilegesEvaluator.java | 19 ++-- .../opensearch/security/IntegrationTests.java | 7 ++ .../PrivilegesEvaluatorUnitTest.java | 95 +++++++++++++++++++ 3 files changed, 115 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 374a3cdf36..c423e9276d 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -35,8 +35,8 @@ import java.util.Map; import java.util.Set; import java.util.StringJoiner; -import java.util.regex.Pattern; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -101,12 +101,19 @@ 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)))" + 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); protected final Logger log = LogManager.getLogger(this.getClass()); @@ -474,7 +481,7 @@ public PrivilegesEvaluatorResponse evaluate( } } - if (dnfofEnabled && DNFOF_PATTERNS.matcher(action0).matches()) { + if (dnfofEnabled && DNFOF_MATCHER.test(action0)) { if (requestedResolved.getAllIndices().isEmpty()) { presponse.missingPrivileges.clear(); diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java index 522c9c439e..0319d7d911 100644 --- a/src/test/java/org/opensearch/security/IntegrationTests.java +++ b/src/test/java/org/opensearch/security/IntegrationTests.java @@ -768,6 +768,13 @@ public void testDnfof() throws Exception { HttpStatus.SC_OK, rh.executeGetRequest("_all/_mapping/field/*", encodeBasicHeader("nagilum", "nagilum")).getStatusCode() ); + + Assert.assertEquals( + HttpStatus.SC_OK, + (resc = rh.executeGetRequest("_cat/indices", encodeBasicHeader("user_a", "user_a"))).getStatusCode() + ); + Assert.assertTrue(resc.getBody(), resc.getBody().contains("indexa")); + Assert.assertFalse(resc.getBody(), resc.getBody().contains("indexb")); } @Test diff --git a/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java b/src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java index e7412f43b4..811c817b65 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_MATCHER; 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_MATCHER.test(permission), equalTo(false)); + } + } + + @Test + public void testDnfofPermissions_positive() { + for (final String permission : allowedDnfof) { + assertThat(DNFOF_MATCHER.test(permission), equalTo(true)); + } + } }