From 99457b11bdca9752f3003b29209588db91bca248 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 4 Oct 2024 10:56:00 -0400 Subject: [PATCH] Fix issue with Get mappings on a Closed index (#4685) Signed-off-by: Craig Perkins --- .../security/FlsAndFieldMaskingTests.java | 36 +++++++++++++++++++ .../security/securityconf/ConfigModelV7.java | 24 +++++++++---- .../SystemIndexAccessEvaluatorTest.java | 2 +- .../impl/v7/IndexPatternTests.java | 26 +++++++++++--- 4 files changed, 77 insertions(+), 11 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java b/src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java index 71a1e21444..7ba435b1c3 100644 --- a/src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java +++ b/src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java @@ -62,6 +62,7 @@ import org.opensearch.test.framework.cluster.TestRestClient; import org.opensearch.test.framework.log.LogsRule; +import static org.apache.http.HttpStatus.SC_OK; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; @@ -230,6 +231,12 @@ public class FlsAndFieldMaskingTests { "cluster_composite_ops_ro" ).indexPermissions("read").fls(String.format("~%s", FIELD_TITLE)).on(FIRST_INDEX_NAME); + static final TestSecurityConfig.Role ROLE_NO_FIELD_TITLE_WILDCARD_INDEX_FLS = new TestSecurityConfig.Role("example_exclusive_fls") + .clusterPermissions("cluster_composite_ops_ro") + .indexPermissions("read", "indices:admin/mappings/get") + .fls(String.format("~%s", FIELD_TITLE)) + .on("*"); + static final TestSecurityConfig.Role ROLE_ONLY_FIELD_TITLE_MASKED = new TestSecurityConfig.Role("example_mask").clusterPermissions( "cluster_composite_ops_ro" ).indexPermissions("read").maskedFields(FIELD_TITLE.concat("::/(?<=.{1})./::").concat(MASK_VALUE)).on(FIRST_INDEX_NAME); @@ -248,6 +255,12 @@ public class FlsAndFieldMaskingTests { ROLE_NO_FIELD_TITLE_FLS ); + /** + * Example user with fls filter in which the user can see every field but the {@link Song#FIELD_TITLE} field. + */ + static final TestSecurityConfig.User USER_NO_FIELD_TITLE_WILDCARD_INDEX_FLS = new TestSecurityConfig.User("exclusive_wildcard_fls_user") + .roles(ROLE_NO_FIELD_TITLE_WILDCARD_INDEX_FLS); + /** * Example user in which {@link Song#FIELD_TITLE} field is masked. */ @@ -305,6 +318,7 @@ public class FlsAndFieldMaskingTests { ALL_INDICES_STARS_LESS_THAN_ZERO_READER, TWINS_FIRST_ARTIST_READER, USER_ONLY_FIELD_TITLE_FLS, + USER_NO_FIELD_TITLE_WILDCARD_INDEX_FLS, USER_NO_FIELD_TITLE_FLS, USER_ONLY_FIELD_TITLE_MASKED, USER_BOTH_ONLY_AND_NO_FIELD_TITLE_FLS, @@ -1778,4 +1792,26 @@ public void testFlsOnAClosedAndReopenedIndex() throws IOException { } } + @SuppressWarnings("unchecked") + @Test + public void testGetMappingsOnAClosedIndexWithFlsRestrictions() throws IOException { + String indexName = "fls_excludes_mappings"; + List docIds = createIndexWithDocs(indexName, SONGS[0], SONGS[1]); + + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + TestRestClient.HttpResponse mappingsResponse = client.get(indexName + "/_mapping"); + mappingsResponse.assertStatusCode(SC_OK); + assertThat(mappingsResponse.getBody(), containsString("title")); + + TestRestClient.HttpResponse closeResponse = client.post(indexName + "/_close"); + closeResponse.assertStatusCode(SC_OK); + } + + try (TestRestClient client = cluster.getRestClient(USER_NO_FIELD_TITLE_WILDCARD_INDEX_FLS)) { + TestRestClient.HttpResponse mappingsResponse = client.get(indexName + "/_mapping"); + mappingsResponse.assertStatusCode(SC_OK); + assertThat(mappingsResponse.getBody(), not(containsString("title"))); + } + } + } diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java index f78c173202..29bc0c4426 100644 --- a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java +++ b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java @@ -304,7 +304,7 @@ public EvaluatedDlsFlsConfig getDlsFls( for (SecurityRole role : roles) { for (IndexPattern ip : role.getIpatterns()) { - final Set concreteIndices = ip.concreteIndexNames(user, resolver, cs); + final Set concreteIndices = ip.concreteIndexNames(user, resolver, cs, true); String dls = ip.getDlsQuery(user); if (dls != null && dls.length() > 0) { @@ -758,24 +758,36 @@ public String getUnresolvedIndexPattern(User user) { return UserAttributes.replaceProperties(indexPattern, user); } + /** Finds the indices accessible to the user and resolves them to concrete names */ + public Set concreteIndexNames( + final User user, + final IndexNameExpressionResolver resolver, + final ClusterService cs, + final boolean includeClosed + ) { + return getResolvedIndexPattern(user, resolver, cs, false, includeClosed); + } + /** Finds the indices accessible to the user and resolves them to concrete names */ public Set concreteIndexNames(final User user, final IndexNameExpressionResolver resolver, final ClusterService cs) { - return getResolvedIndexPattern(user, resolver, cs, false); + return getResolvedIndexPattern(user, resolver, cs, false, false); } /** Finds the indices accessible to the user and attempts to resolve them to names, also includes any unresolved names */ public Set attemptResolveIndexNames(final User user, final IndexNameExpressionResolver resolver, final ClusterService cs) { - return getResolvedIndexPattern(user, resolver, cs, true); + return getResolvedIndexPattern(user, resolver, cs, true, false); } public Set getResolvedIndexPattern( final User user, final IndexNameExpressionResolver resolver, final ClusterService cs, - final boolean appendUnresolved + final boolean appendUnresolved, + final boolean includeClosed ) { final String unresolved = getUnresolvedIndexPattern(user); final ImmutableSet.Builder resolvedIndices = new ImmutableSet.Builder<>(); + final IndicesOptions expansionMode = includeClosed ? IndicesOptions.lenientExpand() : IndicesOptions.lenientExpandOpen(); final WildcardMatcher matcher = WildcardMatcher.from(unresolved); boolean includeDataStreams = true; @@ -792,7 +804,7 @@ public Set getResolvedIndexPattern( if (aliasesAndDataStreamsForPermittedPattern.length > 0) { final String[] resolvedAliasesAndDataStreamIndices = resolver.concreteIndexNames( cs.state(), - IndicesOptions.lenientExpandOpen(), + expansionMode, includeDataStreams, aliasesAndDataStreamsForPermittedPattern ); @@ -803,7 +815,7 @@ public Set getResolvedIndexPattern( if (!(unresolved == null || unresolved.isBlank())) { final String[] resolvedIndicesFromPattern = resolver.concreteIndexNames( cs.state(), - IndicesOptions.lenientExpandOpen(), + expansionMode, includeDataStreams, unresolved ); diff --git a/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java index fa8a991db9..58e811fa24 100644 --- a/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java @@ -151,7 +151,7 @@ public void setup( when(log.isDebugEnabled()).thenReturn(true); when(log.isInfoEnabled()).thenReturn(true); - doReturn(ImmutableSet.of(index)).when(ip).getResolvedIndexPattern(user, indexNameExpressionResolver, cs, true); + doReturn(ImmutableSet.of(index)).when(ip).getResolvedIndexPattern(user, indexNameExpressionResolver, cs, true, false); } @After diff --git a/src/test/java/org/opensearch/security/securityconf/impl/v7/IndexPatternTests.java b/src/test/java/org/opensearch/security/securityconf/impl/v7/IndexPatternTests.java index 513e5bb2cc..4ea364f663 100644 --- a/src/test/java/org/opensearch/security/securityconf/impl/v7/IndexPatternTests.java +++ b/src/test/java/org/opensearch/security/securityconf/impl/v7/IndexPatternTests.java @@ -79,13 +79,13 @@ public void testCtor() { /** Ensure that concreteIndexNames sends correct parameters are sent to getResolvedIndexPattern */ @Test public void testConcreteIndexNamesOverload() { - doReturn(ImmutableSet.of("darn")).when(ip).getResolvedIndexPattern(user, resolver, clusterService, false); + doReturn(ImmutableSet.of("darn")).when(ip).getResolvedIndexPattern(user, resolver, clusterService, false, false); final Set results = ip.concreteIndexNames(user, resolver, clusterService); assertThat(results, contains("darn")); - verify(ip).getResolvedIndexPattern(user, resolver, clusterService, false); + verify(ip).getResolvedIndexPattern(user, resolver, clusterService, false, false); verify(ip).concreteIndexNames(user, resolver, clusterService); verifyNoMoreInteractions(ip); } @@ -93,13 +93,13 @@ public void testConcreteIndexNamesOverload() { /** Ensure that attemptResolveIndexNames sends correct parameters are sent to getResolvedIndexPattern */ @Test public void testAttemptResolveIndexNamesOverload() { - doReturn(ImmutableSet.of("yarn")).when(ip).getResolvedIndexPattern(user, resolver, clusterService, true); + doReturn(ImmutableSet.of("yarn")).when(ip).getResolvedIndexPattern(user, resolver, clusterService, true, false); final Set results = ip.attemptResolveIndexNames(user, resolver, clusterService); assertThat(results, contains("yarn")); - verify(ip).getResolvedIndexPattern(user, resolver, clusterService, true); + verify(ip).getResolvedIndexPattern(user, resolver, clusterService, true, false); verify(ip).attemptResolveIndexNames(user, resolver, clusterService); verifyNoMoreInteractions(ip); } @@ -140,6 +140,24 @@ public void testExactName() { verify(resolver).concreteIndexNames(any(), eq(IndicesOptions.lenientExpandOpen()), eq(true), eq("index-17")); } + /** Verify concreteIndexNames on exact name matches */ + @Test + public void testExactNameOnClosedIndex() { + doReturn("index-17").when(ip).getUnresolvedIndexPattern(user); + when(clusterService.state()).thenReturn(mock(ClusterState.class)); + when(resolver.concreteIndexNames(any(), eq(IndicesOptions.lenientExpand()), eq(true), eq("index-17"))).thenReturn( + new String[] { "resolved-index-17" } + ); + + final Set results = ip.concreteIndexNames(user, resolver, clusterService, true); + + assertThat(results, contains("resolved-index-17")); + + verify(clusterService).state(); + verify(ip).getUnresolvedIndexPattern(user); + verify(resolver).concreteIndexNames(any(), eq(IndicesOptions.lenientExpand()), eq(true), eq("index-17")); + } + /** Verify concreteIndexNames on multiple matches */ @Test public void testMultipleConcreteIndices() {