From d03576666cd599139e739c8aaad795e7620b33df Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Thu, 3 Oct 2024 16:55:24 +0200 Subject: [PATCH] Tests Signed-off-by: Nils Bandener --- .../privileges/ActionPrivilegesTest.java | 121 +++++++++++++++++- .../security/privileges/IndexPatternTest.java | 67 +++++++++- .../util/MockIndexMetadataBuilder.java | 17 ++- .../security/privileges/ActionPrivileges.java | 22 +++- .../security/privileges/IndexPattern.java | 32 +++-- 5 files changed, 230 insertions(+), 29 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java b/src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java index 5619dc4f41..1a9bf3ab46 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java @@ -31,6 +31,7 @@ import org.opensearch.action.support.IndicesOptions; import org.opensearch.cluster.metadata.IndexAbstraction; +import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; @@ -39,6 +40,7 @@ import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; import org.opensearch.security.securityconf.impl.v7.RoleV7; +import org.opensearch.security.support.WildcardMatcher; import org.opensearch.security.user.User; import org.opensearch.security.util.MockIndexMetadataBuilder; @@ -61,6 +63,7 @@ ActionPrivilegesTest.ClusterPrivileges.class, ActionPrivilegesTest.IndexPrivileges.IndicesAndAliases.class, ActionPrivilegesTest.IndexPrivileges.DataStreams.class, + ActionPrivilegesTest.Misc.class, ActionPrivilegesTest.StatefulIndexPrivilegesHeapSize.class }) public class ActionPrivilegesTest { public static class ClusterPrivileges { @@ -102,6 +105,21 @@ public void notWellKnown() throws Exception { ); } + @Test + public void wildcard() throws Exception { + SecurityDynamicConfiguration roles = SecurityDynamicConfiguration.fromYaml("test_role:\n" + // + " cluster_permissions:\n" + // + " - '*'", CType.ROLES); + + ActionPrivileges subject = new ActionPrivileges(roles, FlattenedActionGroups.EMPTY, null, Settings.EMPTY); + + assertThat(subject.hasClusterPrivilege(ctx("test_role"), "cluster:whatever"), isAllowed()); + assertThat( + subject.hasClusterPrivilege(ctx("other_role"), "cluster:whatever"), + isForbidden(missingPrivileges("cluster:whatever")) + ); + } + @Test public void explicit_wellKnown() throws Exception { SecurityDynamicConfiguration roles = SecurityDynamicConfiguration.fromYaml("non_explicit_role:\n" + // @@ -213,6 +231,26 @@ public void hasAny_notWellKnown() throws Exception { subject.hasAnyClusterPrivilege(ctx("test_role"), ImmutableSet.of("cluster:monitor/other")), isForbidden(missingPrivileges("cluster:monitor/other")) ); + assertThat( + subject.hasAnyClusterPrivilege(ctx("test_role"), ImmutableSet.of("cluster:monitor/other", "cluster:monitor/yetanother")), + isForbidden() + ); + } + + @Test + public void hasAny_wildcard() throws Exception { + SecurityDynamicConfiguration roles = SecurityDynamicConfiguration.fromYaml("test_role:\n" + // + " cluster_permissions:\n" + // + " - '*'", CType.ROLES); + + ActionPrivileges subject = new ActionPrivileges(roles, FlattenedActionGroups.EMPTY, null, Settings.EMPTY); + + assertThat(subject.hasAnyClusterPrivilege(ctx("test_role"), ImmutableSet.of("cluster:whatever")), isAllowed()); + + assertThat( + subject.hasAnyClusterPrivilege(ctx("other_role"), ImmutableSet.of("cluster:whatever")), + isForbidden(missingPrivileges("cluster:whatever")) + ); } } @@ -282,6 +320,19 @@ public void positive_partial2() throws Exception { } } + @Test + public void positive_noLocal() throws Exception { + IndexResolverReplacer.Resolved resolved = new IndexResolverReplacer.Resolved( + ImmutableSet.of(), + ImmutableSet.of(), + ImmutableSet.of("remote:a"), + ImmutableSet.of("remote:a"), + IndicesOptions.LENIENT_EXPAND_OPEN + ); + PrivilegesEvaluatorResponse result = subject.hasIndexPrivilege(ctx("test_role"), requiredActions, resolved); + assertThat(result, isAllowed()); + } + @Test public void negative_wrongRole() throws Exception { PrivilegesEvaluationContext ctx = ctx("other_role"); @@ -592,7 +643,6 @@ static IndexResolverReplacer.Resolved resolved(String... indices) { IndicesOptions.LENIENT_EXPAND_OPEN ); } - } static class IndexSpec { @@ -717,6 +767,75 @@ enum Statefulness { } } + public static class Misc { + @Test + public void relevantOnly_identity() throws Exception { + Map metadata = // + indices("index_a11", "index_a12", "index_b")// + .alias("alias_a") + .of("index_a11", "index_a12")// + .build() + .getIndicesLookup(); + + Assert.assertTrue( + "relevantOnly() returned identical object", + ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata, null) == metadata + ); + } + + @Test + public void relevantOnly_closed() throws Exception { + Map metadata = indices("index_open_1", "index_open_2")// + .index("index_closed", IndexMetadata.State.CLOSE) + .build() + .getIndicesLookup(); + + Assert.assertNotNull("Original metadata contains index_open_1", metadata.get("index_open_1")); + Assert.assertNotNull("Original metadata contains index_closed", metadata.get("index_closed")); + + Map filteredMetadata = ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata, null); + + Assert.assertNotNull("Filtered metadata contains index_open_1", filteredMetadata.get("index_open_1")); + Assert.assertNull("Filtered metadata does not contain index_closed", filteredMetadata.get("index_closed")); + } + + @Test + public void relevantOnly_dataStreamBackingIndices() throws Exception { + Map metadata = dataStreams("data_stream_1").build().getIndicesLookup(); + + Assert.assertNotNull("Original metadata contains backing index", metadata.get(".ds-data_stream_1-000001")); + Assert.assertNotNull("Original metadata contains data stream", metadata.get("data_stream_1")); + + Map filteredMetadata = ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata, null); + + Assert.assertNull("Filtered metadata does not contain backing index", filteredMetadata.get(".ds-data_stream_1-000001")); + Assert.assertNotNull("Filtered metadata contains data stream", filteredMetadata.get("data_stream_1")); + } + + @Test + public void relevantOnly_includePattern() throws Exception { + Map metadata = // + indices("index_a11", "index_a12", "index_b1")// + .alias("alias_a") + .of("index_a11")// + .build() + .getIndicesLookup(); + + Assert.assertNotNull("Original metadata contains index_a11", metadata.get("index_a11")); + Assert.assertNotNull("Original metadata contains index_b1", metadata.get("index_b1")); + Assert.assertNotNull("Original metadata contains alias_a", metadata.get("alias_a")); + + Map filteredMetadata = ActionPrivileges.StatefulIndexPrivileges.relevantOnly( + metadata, + WildcardMatcher.from("index_a*", "alias_a*") + ); + + Assert.assertNotNull("Filtered metadata contains index_a11", filteredMetadata.get("index_a11")); + Assert.assertNull("Filtered metadata does not contain index_b1", filteredMetadata.get("index_b1")); + Assert.assertNotNull("Filtered metadata contains alias_a", filteredMetadata.get("alias_a")); + } + } + /** * Verifies that the heap size used by StatefulIndexPrivileges stays within expected bounds. */ diff --git a/src/integrationTest/java/org/opensearch/security/privileges/IndexPatternTest.java b/src/integrationTest/java/org/opensearch/security/privileges/IndexPatternTest.java index 47db3fc9f7..4b883183dc 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/IndexPatternTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/IndexPatternTest.java @@ -13,6 +13,7 @@ import java.time.ZonedDateTime; import java.time.temporal.ChronoField; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import org.junit.Test; @@ -22,10 +23,13 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.security.resolver.IndexResolverReplacer; +import org.opensearch.security.support.WildcardMatcher; import org.opensearch.security.user.User; import static org.opensearch.security.util.MockIndexMetadataBuilder.indices; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; public class IndexPatternTest { final static int CURRENT_YEAR = ZonedDateTime.now().get(ChronoField.YEAR); @@ -45,13 +49,17 @@ public class IndexPatternTest { .of("index_current_year")// .alias("alias_year_" + NEXT_YEAR) .of("index_next_year")// - .build(); final static ClusterState CLUSTER_STATE = ClusterState.builder(ClusterState.EMPTY_STATE).metadata(INDEX_METADATA).build(); @Test public void constantIndex() throws Exception { IndexPattern indexPattern = IndexPattern.from("index_a11"); + assertTrue(indexPattern.hasStaticPattern()); + assertFalse(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.dynamicOnly().isEmpty()); + assertEquals("index_a11", indexPattern.toString()); + assertTrue(indexPattern.matches("index_a11", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches("index_a12", ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -59,6 +67,9 @@ public void constantIndex() throws Exception { @Test public void constantAlias() throws Exception { IndexPattern indexPattern = IndexPattern.from("alias_a"); + assertTrue(indexPattern.hasStaticPattern()); + assertFalse(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.matches("alias_a", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches("alias_a1", ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -66,6 +77,9 @@ public void constantAlias() throws Exception { @Test public void constantAlias_onIndex() throws Exception { IndexPattern indexPattern = IndexPattern.from("alias_a"); + assertTrue(indexPattern.hasStaticPattern()); + assertFalse(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.matches("index_a11", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches("index_b1", ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -73,6 +87,9 @@ public void constantAlias_onIndex() throws Exception { @Test public void constantDataStream_onIndex() throws Exception { IndexPattern indexPattern = IndexPattern.from("data_stream_a1"); + assertTrue(indexPattern.hasStaticPattern()); + assertFalse(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.matches(".ds-data_stream_a1-000001", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches(".ds-data_stream_a2-000001", ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -80,6 +97,9 @@ public void constantDataStream_onIndex() throws Exception { @Test public void patternIndex() throws Exception { IndexPattern indexPattern = IndexPattern.from("index_a1*"); + assertTrue(indexPattern.hasStaticPattern()); + assertFalse(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.matches("index_a11", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches("index_a21", ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -87,6 +107,9 @@ public void patternIndex() throws Exception { @Test public void patternAlias() throws Exception { IndexPattern indexPattern = IndexPattern.from("alias_a*"); + assertTrue(indexPattern.hasStaticPattern()); + assertFalse(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.matches("alias_a", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches("alias_b", ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -94,6 +117,9 @@ public void patternAlias() throws Exception { @Test public void patternAlias_onIndex() throws Exception { IndexPattern indexPattern = IndexPattern.from("alias_a*"); + assertTrue(indexPattern.hasStaticPattern()); + assertFalse(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.matches("index_a11", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches("index_b1", ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -101,6 +127,9 @@ public void patternAlias_onIndex() throws Exception { @Test public void patternDataStream_onIndex() throws Exception { IndexPattern indexPattern = IndexPattern.from("data_stream_a*"); + assertTrue(indexPattern.hasStaticPattern()); + assertFalse(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.matches(".ds-data_stream_a1-000001", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches(".ds-data_stream_b1-000001", ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -108,6 +137,10 @@ public void patternDataStream_onIndex() throws Exception { @Test public void dateMathIndex() throws Exception { IndexPattern indexPattern = IndexPattern.from(""); + assertFalse(indexPattern.hasStaticPattern()); + assertTrue(indexPattern.hasDynamicPattern()); + assertEquals("index_a11", indexPattern.toString()); + assertTrue(indexPattern.matches("index_year_" + CURRENT_YEAR, ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches("index_year_" + NEXT_YEAR, ctx(), INDEX_METADATA.getIndicesLookup())); } @@ -115,15 +148,43 @@ public void dateMathIndex() throws Exception { @Test public void dateMathAlias_onIndex() throws Exception { IndexPattern indexPattern = IndexPattern.from(""); + assertFalse(indexPattern.hasStaticPattern()); + assertTrue(indexPattern.hasDynamicPattern()); + assertTrue(indexPattern.matches("index_current_year", ctx(), INDEX_METADATA.getIndicesLookup())); assertFalse(indexPattern.matches("index_next_year", ctx(), INDEX_METADATA.getIndicesLookup())); } + @Test + public void templatedIndex() throws Exception { + IndexPattern indexPattern = IndexPattern.from("index_${attrs.a11}"); + assertFalse(indexPattern.hasStaticPattern()); + assertTrue(indexPattern.hasDynamicPattern()); + assertEquals(indexPattern, indexPattern.dynamicOnly()); + + assertTrue(indexPattern.matches("index_a11", ctx(), INDEX_METADATA.getIndicesLookup())); + assertFalse(indexPattern.matches("index_a12", ctx(), INDEX_METADATA.getIndicesLookup())); + } + + @Test + public void mixed() throws Exception { + IndexPattern indexPattern = IndexPattern.from("index_${attrs.a11}", "index_a12"); + assertTrue(indexPattern.hasStaticPattern()); + assertTrue(indexPattern.hasDynamicPattern()); + + assertEquals(WildcardMatcher.from("index_a12"), indexPattern.getStaticPattern()); + assertEquals(IndexPattern.from("index_${attrs.a11}"), indexPattern.dynamicOnly()); + } + private static PrivilegesEvaluationContext ctx() { IndexNameExpressionResolver indexNameExpressionResolver = new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)); IndexResolverReplacer indexResolverReplacer = new IndexResolverReplacer(indexNameExpressionResolver, () -> CLUSTER_STATE, null); + User user = new User("test_user"); + user.addAttributes(ImmutableMap.of("attrs.a11", "a11")); + user.addAttributes(ImmutableMap.of("attrs.year", "year")); + return new PrivilegesEvaluationContext( - new User("test_user"), + user, ImmutableSet.of(), "indices:action/test", null, diff --git a/src/integrationTest/java/org/opensearch/security/util/MockIndexMetadataBuilder.java b/src/integrationTest/java/org/opensearch/security/util/MockIndexMetadataBuilder.java index aa83367803..cb0e4f32c4 100644 --- a/src/integrationTest/java/org/opensearch/security/util/MockIndexMetadataBuilder.java +++ b/src/integrationTest/java/org/opensearch/security/util/MockIndexMetadataBuilder.java @@ -67,7 +67,11 @@ public Metadata build() { } public MockIndexMetadataBuilder index(String indexName) { - getIndexMetadataBuilder(indexName); + return index(indexName, IndexMetadata.State.OPEN); + } + + public MockIndexMetadataBuilder index(String indexName, IndexMetadata.State state) { + getIndexMetadataBuilder(indexName, state); return this; } @@ -85,7 +89,7 @@ public MockIndexMetadataBuilder dataStream(String dataStream, int generations) { for (int i = 1; i <= generations; i++) { String backingIndexName = DataStream.getDefaultBackingIndexName(dataStream, i); backingIndices.add(new Index(backingIndexName, backingIndexName)); - getIndexMetadataBuilder(backingIndexName); + getIndexMetadataBuilder(backingIndexName, IndexMetadata.State.OPEN); } DataStream dataStreamMetadata = new DataStream(dataStream, new DataStream.TimestampField("@timestamp"), backingIndices); @@ -94,16 +98,15 @@ public MockIndexMetadataBuilder dataStream(String dataStream, int generations) { return this; } - private IndexMetadata.Builder getIndexMetadataBuilder(String indexName) { + private IndexMetadata.Builder getIndexMetadataBuilder(String indexName, IndexMetadata.State state) { IndexMetadata.Builder result = this.nameToIndexMetadataBuilderMap.get(indexName); if (result != null) { return result; } - result = new IndexMetadata.Builder(indexName).settings( - Settings.builder().put(INDEX_SETTINGS).put(IndexMetadata.SETTING_INDEX_UUID, indexName).build() - ); + result = new IndexMetadata.Builder(indexName).state(state) + .settings(Settings.builder().put(INDEX_SETTINGS).put(IndexMetadata.SETTING_INDEX_UUID, indexName).build()); this.nameToIndexMetadataBuilderMap.put(indexName, result); @@ -121,7 +124,7 @@ public MockIndexMetadataBuilder of(String... indices) { AliasMetadata aliasMetadata = new AliasMetadata.Builder(aliasName).build(); for (String index : indices) { - IndexMetadata.Builder indexMetadataBuilder = getIndexMetadataBuilder(index); + IndexMetadata.Builder indexMetadataBuilder = getIndexMetadataBuilder(index, IndexMetadata.State.OPEN); indexMetadataBuilder.putAlias(aliasMetadata); } diff --git a/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java b/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java index 51c382f3a5..8b4082848d 100644 --- a/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java +++ b/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java @@ -192,7 +192,9 @@ public PrivilegesEvaluatorResponse hasIndexPrivilege( return response; } - if (resolvedIndices.getAllIndices().isEmpty()) { + if (!resolvedIndices.isLocalAll() && resolvedIndices.getAllIndices().isEmpty()) { + // This is necessary for requests which operate on remote indices. + // Access control for the remote indices will be performed on the remote cluster. log.debug("No local indices; grant the request"); return PrivilegesEvaluatorResponse.ok(); } @@ -472,7 +474,7 @@ PrivilegesEvaluatorResponse providesPrivilege(PrivilegesEvaluationContext contex // 2: Check well-known actions - this should cover most cases ImmutableCompactSubSet rolesWithPrivileges = this.actionToRoles.get(action); - if (rolesWithPrivileges != null && CollectionUtils.containsAny(roles, rolesWithPrivileges)) { + if (rolesWithPrivileges != null && rolesWithPrivileges.containsAny(roles)) { return PrivilegesEvaluatorResponse.ok(); } @@ -505,7 +507,7 @@ PrivilegesEvaluatorResponse providesExplicitPrivilege(PrivilegesEvaluationContex // 1: Check well-known actions - this should cover most cases ImmutableCompactSubSet rolesWithPrivileges = this.actionToRoles.get(action); - if (rolesWithPrivileges != null && CollectionUtils.containsAny(roles, rolesWithPrivileges)) { + if (rolesWithPrivileges != null && rolesWithPrivileges.containsAny(roles)) { return PrivilegesEvaluatorResponse.ok(); } @@ -1139,9 +1141,20 @@ private String backingIndexToDataStream(String index, Map + * This removes the following entries: + *
    + *
  • closed indices - closed indices do not need any fast privilege evaluation + *
  • backing indices of data streams - privileges should be only assigned directly to the data streams. + * the privilege evaluation code is able to recognize that an index is member of a data stream and test + * its privilege via that data stream. If a privilege is directly assigned to a backing index, we use + * the "slowish" code paths. + *
  • Indices which are not matched by includeIndices + *
*/ static Map relevantOnly(Map indices, WildcardMatcher includeIndices) { + // First pass: Check if we need to filter at all boolean doFilter = false; for (IndexAbstraction indexAbstraction : indices.values()) { @@ -1163,6 +1176,7 @@ static Map relevantOnly(Map return indices; } + // Second pass: Only if we actually need filtering, we will do it ImmutableMap.Builder builder = ImmutableMap.builder(); for (IndexAbstraction indexAbstraction : indices.values()) { diff --git a/src/main/java/org/opensearch/security/privileges/IndexPattern.java b/src/main/java/org/opensearch/security/privileges/IndexPattern.java index b1556dba65..2def420cf0 100644 --- a/src/main/java/org/opensearch/security/privileges/IndexPattern.java +++ b/src/main/java/org/opensearch/security/privileges/IndexPattern.java @@ -80,18 +80,10 @@ public boolean matches(String index, PrivilegesEvaluationContext context, Map constantPatterns = new ArrayList<>(); private List patternTemplates = new ArrayList<>(); private List dateMathExpressions = new ArrayList<>(); - private int initializationErrors = 0; void add(List source) { for (int i = 0; i < source.size(); i++) { @@ -196,14 +201,13 @@ void add(List source) { } catch (Exception e) { // This usually happens when the index pattern defines an unparseable regular expression log.error("Error while creating index pattern for {}", source, e); - this.initializationErrors++; } } } IndexPattern build() { return new IndexPattern( - WildcardMatcher.from(constantPatterns), + constantPatterns.size() != 0 ? WildcardMatcher.from(constantPatterns) : null, ImmutableList.copyOf(patternTemplates), ImmutableList.copyOf(dateMathExpressions) );