diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b559e38a5..5157f6376a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Features ### Enhancements +- Introduce new dynamic setting (plugins.security.dls.write_blocked) to block all writes when restrictions apply ([#5828](https://github.com/opensearch-project/security/pull/5828)) + - Support nested JWT claims in role DLS queries ([#5687](https://github.com/opensearch-project/security/issues/5687)) ### Bug Fixes - Fix IllegalArgumentException when resolved indices are empty in PrivilegesEvaluator ([#5770](https://github.com/opensearch-project/security/pull/5797)) diff --git a/bwc-test/src/test/java/org/opensearch/security/bwc/SecurityBackwardsCompatibilityIT.java b/bwc-test/src/test/java/org/opensearch/security/bwc/SecurityBackwardsCompatibilityIT.java index f32051967c..ab4abb6f55 100644 --- a/bwc-test/src/test/java/org/opensearch/security/bwc/SecurityBackwardsCompatibilityIT.java +++ b/bwc-test/src/test/java/org/opensearch/security/bwc/SecurityBackwardsCompatibilityIT.java @@ -337,7 +337,7 @@ private void ingestData(String index) throws IOException { bulkRequestBody.append(Song.randomSong().asJson() + "\n"); } List responses = RestHelper.requestAgainstAllNodes( - testUserRestClient, + adminClient(), "POST", "_bulk?refresh=wait_for", new StringEntity(bulkRequestBody.toString(), APPLICATION_NDJSON) @@ -413,30 +413,31 @@ private boolean resourceExists(String url) throws IOException { */ private void createTestRoleIfNotExists(String role) throws IOException { String url = "_plugins/_security/api/roles/" + role; - String roleSettings = "{\n" - + " \"cluster_permissions\": [\n" - + " \"unlimited\"\n" - + " ],\n" - + " \"index_permissions\": [\n" - + " {\n" - + " \"index_patterns\": [\n" - + " \"test_index*\"\n" - + " ],\n" - + " \"dls\": \"{ \\\"bool\\\": { \\\"must\\\": { \\\"match\\\": { \\\"genre\\\": \\\"rock\\\" } } } }\",\n" - + " \"fls\": [\n" - + " \"~lyrics\"\n" - + " ],\n" - + " \"masked_fields\": [\n" - + " \"artist\"\n" - + " ],\n" - + " \"allowed_actions\": [\n" - + " \"read\",\n" - + " \"write\"\n" - + " ]\n" - + " }\n" - + " ],\n" - + " \"tenant_permissions\": []\n" - + "}\n"; + String roleSettings = """ + { + "cluster_permissions": [ + "unlimited" + ], + "index_permissions": [ + { + "index_patterns": [ + "test_index*" + ], + "dls": "{ \\\"bool\\\": { \\\"must\\\": { \\\"match\\\": { \\\"genre\\\": \\\"rock\\\" } } } }", + "fls": [ + "~lyrics" + ], + "masked_fields": [ + "artist" + ], + "allowed_actions": [ + "read" + ] + } + ], + "tenant_permissions": [] + } + """; Response response = RestHelper.makeRequest(adminClient(), "PUT", url, RestHelper.toHttpEntity(roleSettings)); assertThat(response.getStatusLine().getStatusCode(), anyOf(equalTo(200), equalTo(201))); diff --git a/src/integrationTest/java/org/opensearch/security/TlsHostnameVerificationTests.java b/src/integrationTest/java/org/opensearch/security/TlsHostnameVerificationTests.java index 3316888029..6b3768834f 100644 --- a/src/integrationTest/java/org/opensearch/security/TlsHostnameVerificationTests.java +++ b/src/integrationTest/java/org/opensearch/security/TlsHostnameVerificationTests.java @@ -10,48 +10,66 @@ package org.opensearch.security; import java.util.Map; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; -import org.junit.Assert; import org.junit.Rule; import org.junit.Test; -import org.opensearch.security.support.ConfigConstants; import org.opensearch.test.framework.certificate.TestCertificates; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.log.LogCapturingAppender; import org.opensearch.test.framework.log.LogsRule; -import static org.opensearch.common.network.NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY; +import static java.util.concurrent.Executors.newSingleThreadExecutor; +import static org.awaitility.Awaitility.await; public class TlsHostnameVerificationTests { @Rule public LogsRule logsRule = new LogsRule("org.opensearch.transport.netty4.ssl.SecureNetty4Transport"); - public LocalCluster.Builder clusterBuilder = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) + public LocalCluster.Builder clusterBuilder = new LocalCluster.Builder().clusterManager(ClusterManager.DEFAULT) .anonymousAuth(false) .loadConfigurationIntoIndex(false) - .nodeSettings(Map.of(ConfigConstants.SECURITY_SSL_ONLY, true, TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY, true)) + .nodeSettings( + Map.of("plugins.security.ssl_only", true, "transport.ssl.enforce_hostname_verification", true, "cluster.join.timeout", "10s") + ) .sslOnly(true); @Test public void clusterShouldStart_nodesSanIpsAreValid() { // Note: We cannot use hostnames in this environment. However, IP addresses also work as valid SANs which are also // subject to hostname verification. Thus, we use here certificates with IP SANs - TestCertificates testCertificates = new TestCertificates(ClusterManager.THREE_CLUSTER_MANAGERS.getNodes(), "127.0.0.1"); + TestCertificates testCertificates = new TestCertificates(ClusterManager.DEFAULT.getNodes(), "127.0.0.1"); try (LocalCluster cluster = clusterBuilder.testCertificates(testCertificates).build()) { cluster.before(); - } catch (Exception e) { - Assert.fail("Cluster should start, no exception expected but got: " + e.getMessage()); } } @Test public void clusterShouldNotStart_nodesSanIpsAreInvalid() { - TestCertificates testCertificates = new TestCertificates(ClusterManager.THREE_CLUSTER_MANAGERS.getNodes(), "127.0.0.2"); - try (LocalCluster cluster = clusterBuilder.testCertificates(testCertificates).build()) { - cluster.before(); - Assert.fail("Cluster should not start, an exception expected"); + TestCertificates testCertificates = new TestCertificates(ClusterManager.DEFAULT.getNodes(), "127.0.0.2"); + try ( + LocalCluster cluster = clusterBuilder.testCertificates(testCertificates).build(); + ExecutorService executorService = newSingleThreadExecutor() + ) { + Future clusterFuture = executorService.submit(() -> { + cluster.before(); + return null; + }); + await().alias("expect error message about hostname verification") + .pollDelay(10, TimeUnit.MILLISECONDS) + .until( + () -> LogCapturingAppender.getLogMessagesAsString() + .stream() + .anyMatch( + message -> message.contains("(certificate_unknown) No subject alternative names matching IP address 127.0.0.1") + ) + ); + clusterFuture.cancel(true); } catch (Exception e) { logsRule.assertThatContain("No subject alternative names matching IP address 127.0.0.1 found"); } diff --git a/src/integrationTest/java/org/opensearch/security/dlsfls/DlsWriteBlockedIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/dlsfls/DlsWriteBlockedIntegrationTest.java new file mode 100644 index 0000000000..8cfb95de87 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/dlsfls/DlsWriteBlockedIntegrationTest.java @@ -0,0 +1,133 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.security.dlsfls; + +import java.io.IOException; + +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; + +import org.opensearch.test.framework.TestSecurityConfig.Role; +import org.opensearch.test.framework.TestSecurityConfig.User; +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.is; +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.matcher.RestMatchers.isCreated; + +/** + * Integration tests for DLS_WRITE_BLOCKED setting which blocks write operations + * when users have DLS, FLS, or Field Masking restrictions. + */ +public class DlsWriteBlockedIntegrationTest { + + private static final String DLS_INDEX = "dls_index"; + private static final String FLS_INDEX = "fls_index"; + private static final String NO_RESTRICTION_INDEX = "no_restriction_index"; + + static final User ADMIN_USER = new User("admin").roles(ALL_ACCESS); + + static final User DLS_USER = new User("dls_user").roles( + new Role("dls_role").clusterPermissions("*").indexPermissions("*").dls("{\"term\": {\"dept\": \"sales\"}}").on(DLS_INDEX) + ); + + static final User FLS_USER = new User("fls_user").roles( + new Role("fls_role").clusterPermissions("*").indexPermissions("*").fls("public").on(FLS_INDEX) + ); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(ADMIN_USER, DLS_USER, FLS_USER) + .build(); + + @BeforeClass + public static void createTestData() throws IOException { + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + client.putJson(DLS_INDEX + "/_doc/1?refresh=true", "{\"dept\":\"sales\",\"amount\":100}"); + client.putJson(FLS_INDEX + "/_doc/1?refresh=true", "{\"public\":\"data\",\"secret\":\"hidden\"}"); + client.putJson(NO_RESTRICTION_INDEX + "/_doc/1?refresh=true", "{\"data\":\"value1\"}"); + } + } + + private void setDlsWriteBlocked(boolean enabled) throws IOException { + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + client.putJson("_cluster/settings", String.format("{\"transient\":{\"plugins.security.dls.write_blocked\":%b}}", enabled)); + } + } + + @Test + public void testDlsUser_CanWrite_WhenSettingDisabled() throws IOException { + setDlsWriteBlocked(false); + try (TestRestClient client = cluster.getRestClient(DLS_USER)) { + var response = client.putJson(DLS_INDEX + "/_doc/test1?refresh=true", "{\"dept\":\"sales\",\"amount\":400}"); + + assertThat(response, isCreated()); + } + } + + @Test + public void testDlsUser_CannotWrite_WhenSettingEnabled() throws IOException { + setDlsWriteBlocked(true); + try (TestRestClient client = cluster.getRestClient(DLS_USER)) { + var response = client.putJson(DLS_INDEX + "/_doc/test2?refresh=true", "{\"dept\":\"sales\",\"amount\":400}"); + + assertThat(response.getStatusCode(), is(500)); + assertThat(response.getBody(), containsString("is not supported when FLS or DLS or Fieldmasking is activated")); + } + } + + @Test + public void testFlsUser_CanWrite_WhenSettingDisabled() throws IOException { + setDlsWriteBlocked(false); + try (TestRestClient client = cluster.getRestClient(FLS_USER)) { + var response = client.putJson(FLS_INDEX + "/_doc/test3?refresh=true", "{\"public\":\"new_data\",\"secret\":\"new_secret\"}"); + + assertThat(response.getStatusCode(), is(201)); + } + } + + @Test + public void testFlsUser_CannotWrite_WhenSettingEnabled() throws IOException { + setDlsWriteBlocked(true); + try (TestRestClient client = cluster.getRestClient(FLS_USER)) { + var response = client.putJson(FLS_INDEX + "/_doc/test4?refresh=true", "{\"public\":\"new_data\",\"secret\":\"new_secret\"}"); + + assertThat(response.getStatusCode(), is(500)); + assertThat(response.getBody(), containsString("is not supported when FLS or DLS or Fieldmasking is activated")); + } + } + + @Test + public void testAdminUser_CanWrite_WhenSettingEnabled() throws IOException { + setDlsWriteBlocked(true); + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + var response = client.putJson(DLS_INDEX + "/_doc/test6?refresh=true", "{\"dept\":\"admin\",\"amount\":999}"); + + assertThat(response.getStatusCode(), is(201)); + } + } + + @Test + public void testDlsUser_CanRead_WhenSettingEnabled() throws IOException { + setDlsWriteBlocked(true); + try (TestRestClient client = cluster.getRestClient(DLS_USER)) { + var response = client.get(DLS_INDEX + "/_search"); + + assertThat(response.getStatusCode(), is(200)); + } + } +} diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/ClusterConfig.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/ClusterConfig.java index 2d3a1627e0..cfed0b4262 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/ClusterConfig.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/ClusterConfig.java @@ -12,9 +12,12 @@ package org.opensearch.security.privileges.int_tests; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; import java.util.function.Supplier; +import org.junit.rules.ExternalResource; + import org.opensearch.test.framework.cluster.LocalCluster; /** @@ -46,8 +49,6 @@ public enum ClusterConfig { final boolean systemIndexPrivilegeEnabled; final boolean allowsEmptyResultSets; - private LocalCluster cluster; - ClusterConfig( String name, Function clusterConfiguration, @@ -62,25 +63,38 @@ public enum ClusterConfig { this.allowsEmptyResultSets = allowsEmptyResultSets; } - LocalCluster cluster(Supplier clusterBuilder) { - if (cluster == null) { - cluster = this.clusterConfiguration.apply(clusterBuilder.get()).build(); - cluster.before(); - } - return cluster; + @Override + public String toString() { + return name; } - void shutdown() { - if (cluster != null) { - try { - cluster.close(); - } catch (Exception e) {} - cluster = null; + public static class ClusterInstances extends ExternalResource { + private final Supplier clusterBuilder; + + public ClusterInstances(Supplier clusterBuilder) { + this.clusterBuilder = clusterBuilder; } - } - @Override - public String toString() { - return name; + private Map configToInstanceMap = new ConcurrentHashMap<>(); + + public LocalCluster get(ClusterConfig config) { + LocalCluster cluster = configToInstanceMap.get(config); + if (cluster == null) { + cluster = config.clusterConfiguration.apply(clusterBuilder.get()).build(); + cluster.before(); + configToInstanceMap.put(config, cluster); + } + + return cluster; + } + + @Override + protected void after() { + for (Map.Entry entry : configToInstanceMap.entrySet()) { + entry.getValue().stopSafe(); + } + configToInstanceMap.clear(); + }; + } } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/CrossClusterAuthorizationIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/CrossClusterAuthorizationIntTests.java index be24358308..76f28dd96a 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/CrossClusterAuthorizationIntTests.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/CrossClusterAuthorizationIntTests.java @@ -16,7 +16,6 @@ import java.util.List; import com.google.common.collect.ImmutableList; -import org.junit.AfterClass; import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; @@ -189,12 +188,10 @@ static LocalCluster.Builder clusterBuilder() { .indices(LocalIndices.index_a1, LocalIndices.index_a2); } - @AfterClass - public static void stopClusters() { - for (ClusterConfig clusterConfig : ClusterConfig.values()) { - clusterConfig.shutdown(); - } - } + @ClassRule + public static final ClusterConfig.ClusterInstances clusters = new ClusterConfig.ClusterInstances( + CrossClusterAuthorizationIntTests::clusterBuilder + ); final TestSecurityConfig.User user; final LocalCluster cluster; @@ -479,7 +476,7 @@ public static Collection params() { public CrossClusterAuthorizationIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description) throws Exception { this.user = user; - this.cluster = clusterConfig.cluster(CrossClusterAuthorizationIntTests::clusterBuilder); + this.cluster = clusters.get(clusterConfig); this.clusterConfig = clusterConfig; } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DashboardMultiTenancyIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DashboardMultiTenancyIntTests.java index cc7ab237c1..5b67d241e2 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DashboardMultiTenancyIntTests.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DashboardMultiTenancyIntTests.java @@ -17,7 +17,7 @@ import java.util.UUID; import org.apache.hc.core5.http.message.BasicHeader; -import org.junit.AfterClass; +import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -361,12 +361,10 @@ static LocalCluster.Builder clusterBuilder() { ); } - @AfterClass - public static void stopClusters() { - for (ClusterConfig clusterConfig : ClusterConfig.values()) { - clusterConfig.shutdown(); - } - } + @ClassRule + public static final ClusterConfig.ClusterInstances clusterInstances = new ClusterConfig.ClusterInstances( + DashboardMultiTenancyIntTests::clusterBuilder + ); final TestSecurityConfig.User user; final LocalCluster cluster; @@ -763,7 +761,7 @@ public DashboardMultiTenancyIntTests( @SuppressWarnings("unused") String description ) { this.user = user; - this.cluster = clusterConfig.cluster(DashboardMultiTenancyIntTests::clusterBuilder); + this.cluster = clusterInstances.get(clusterConfig); this.clusterConfig = clusterConfig; } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DataStreamAuthorizationReadOnlyIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DataStreamAuthorizationReadOnlyIntTests.java index 2eaf0b5012..a43be48ca6 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DataStreamAuthorizationReadOnlyIntTests.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DataStreamAuthorizationReadOnlyIntTests.java @@ -15,7 +15,7 @@ import java.util.Collection; import java.util.List; -import org.junit.AfterClass; +import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -228,12 +228,10 @@ static LocalCluster.Builder clusterBuilder() { .indices(index_c1); } - @AfterClass - public static void stopClusters() { - for (ClusterConfig clusterConfig : ClusterConfig.values()) { - clusterConfig.shutdown(); - } - } + @ClassRule + public static final ClusterConfig.ClusterInstances clusterInstances = new ClusterConfig.ClusterInstances( + DataStreamAuthorizationReadOnlyIntTests::clusterBuilder + ); final TestSecurityConfig.User user; final LocalCluster cluster; @@ -875,7 +873,7 @@ public static Collection params() { public DataStreamAuthorizationReadOnlyIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description) throws Exception { this.user = user; - this.cluster = clusterConfig.cluster(DataStreamAuthorizationReadOnlyIntTests::clusterBuilder); + this.cluster = clusterInstances.get(clusterConfig); this.clusterConfig = clusterConfig; } } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DataStreamAuthorizationReadWriteIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DataStreamAuthorizationReadWriteIntTests.java index a378609ff4..b43111158e 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DataStreamAuthorizationReadWriteIntTests.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/DataStreamAuthorizationReadWriteIntTests.java @@ -19,7 +19,7 @@ import com.google.common.collect.ImmutableList; import org.junit.After; -import org.junit.AfterClass; +import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -420,12 +420,10 @@ static LocalCluster.Builder clusterBuilder() { .plugin(IndexAuthorizationReadOnlyIntTests.SystemIndexTestPlugin.class); } - @AfterClass - public static void stopClusters() { - for (ClusterConfig clusterConfig : ClusterConfig.values()) { - clusterConfig.shutdown(); - } - } + @ClassRule + public static final ClusterConfig.ClusterInstances clusterInstances = new ClusterConfig.ClusterInstances( + DataStreamAuthorizationReadWriteIntTests::clusterBuilder + ); final TestSecurityConfig.User user; final LocalCluster cluster; @@ -601,7 +599,7 @@ public static Collection params() { public DataStreamAuthorizationReadWriteIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description) throws Exception { this.user = user; - this.cluster = clusterConfig.cluster(DataStreamAuthorizationReadWriteIntTests::clusterBuilder); + this.cluster = clusterInstances.get(clusterConfig); this.clusterConfig = clusterConfig; } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadOnlyIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadOnlyIntTests.java index 75c35ebbf5..1642398de3 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadOnlyIntTests.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadOnlyIntTests.java @@ -19,7 +19,7 @@ import java.util.stream.Stream; import com.google.common.collect.ImmutableList; -import org.junit.AfterClass; +import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -395,12 +395,10 @@ static LocalCluster.Builder clusterBuilder() { .plugin(SystemIndexTestPlugin.class, MustacheModulePlugin.class); } - @AfterClass - public static void stopClusters() { - for (ClusterConfig clusterConfig : ClusterConfig.values()) { - clusterConfig.shutdown(); - } - } + @ClassRule + public static final ClusterConfig.ClusterInstances clusterInstances = new ClusterConfig.ClusterInstances( + IndexAuthorizationReadOnlyIntTests::clusterBuilder + ); final TestSecurityConfig.User user; final LocalCluster cluster; @@ -1878,7 +1876,7 @@ public static Collection params() { public IndexAuthorizationReadOnlyIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description) throws Exception { this.user = user; - this.cluster = clusterConfig.cluster(IndexAuthorizationReadOnlyIntTests::clusterBuilder); + this.cluster = clusterInstances.get(clusterConfig); this.clusterConfig = clusterConfig; } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadWriteIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadWriteIntTests.java index 1dbbee4a78..db46b42820 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadWriteIntTests.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadWriteIntTests.java @@ -18,7 +18,7 @@ import com.google.common.collect.ImmutableList; import org.junit.After; -import org.junit.AfterClass; +import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -570,12 +570,10 @@ static LocalCluster.Builder clusterBuilder() { .plugin(IndexAuthorizationReadOnlyIntTests.SystemIndexTestPlugin.class); } - @AfterClass - public static void stopClusters() { - for (ClusterConfig clusterConfig : ClusterConfig.values()) { - clusterConfig.shutdown(); - } - } + @ClassRule + public static final ClusterConfig.ClusterInstances clusterInstances = new ClusterConfig.ClusterInstances( + IndexAuthorizationReadWriteIntTests::clusterBuilder + ); final TestSecurityConfig.User user; final LocalCluster cluster; @@ -1157,7 +1155,7 @@ public static Collection params() { public IndexAuthorizationReadWriteIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description) throws Exception { this.user = user; - this.cluster = clusterConfig.cluster(IndexAuthorizationReadWriteIntTests::clusterBuilder); + this.cluster = clusterInstances.get(clusterConfig); this.clusterConfig = clusterConfig; } diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationWithClosedIndicesIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationWithClosedIndicesIntTests.java index e94d377704..eddf0cb69e 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationWithClosedIndicesIntTests.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationWithClosedIndicesIntTests.java @@ -20,6 +20,7 @@ import org.apache.logging.log4j.Logger; import org.junit.After; import org.junit.Before; +import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -140,9 +141,13 @@ static LocalCluster.Builder clusterBuilder() { .plugin(MustacheModulePlugin.class); } + @ClassRule + public static final ClusterConfig.ClusterInstances clusterInstances = new ClusterConfig.ClusterInstances( + IndexAuthorizationWithClosedIndicesIntTests::clusterBuilder + ); + private final TestSecurityConfig.User user; private final LocalCluster cluster; - private final ClusterConfig clusterConfig; @Parameters(name = "{0}, {2}") public static Collection params() { @@ -159,8 +164,7 @@ public static Collection params() { public IndexAuthorizationWithClosedIndicesIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description) throws Exception { this.user = user; - this.cluster = clusterConfig.cluster(IndexAuthorizationWithClosedIndicesIntTests::clusterBuilder); - this.clusterConfig = clusterConfig; + this.cluster = clusterInstances.get(clusterConfig); } @Before diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/ProtectedIndexAuthorizationIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/ProtectedIndexAuthorizationIntTests.java new file mode 100644 index 0000000000..2a6e887069 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/ProtectedIndexAuthorizationIntTests.java @@ -0,0 +1,338 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.privileges.int_tests; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Map; + +import com.google.common.collect.ImmutableList; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; +import org.opensearch.test.framework.data.TestAlias; +import org.opensearch.test.framework.data.TestIndex; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.cluster.TestRestClient.json; +import static org.opensearch.test.framework.matcher.RestIndexMatchers.IndexMatcher; +import static org.opensearch.test.framework.matcher.RestIndexMatchers.OnResponseIndexMatcher.containsExactly; +import static org.opensearch.test.framework.matcher.RestIndexMatchers.OnUserIndexMatcher.limitedTo; +import static org.opensearch.test.framework.matcher.RestMatchers.isCreated; +import static org.opensearch.test.framework.matcher.RestMatchers.isForbidden; +import static org.opensearch.test.framework.matcher.RestMatchers.isOk; + +/** + * This class tests protected indices functionality with different cluster configurations. + * It uses the following dimensions: + *
    + *
  • ClusterConfig: Tests with protected indices enabled and disabled
    • + *
  • TestSecurityConfig.User: Different users with different protected index role assignments
    • + *
  • Test methods: Different operations (search, create, delete, update, etc.)
    • + *
+ */ +@RunWith(Parameterized.class) +public class ProtectedIndexAuthorizationIntTests { + + // ------------------------------------------------------------------------------------------------------- + // Test indices used by this test suite + // ------------------------------------------------------------------------------------------------------- + + static final TestIndex protected_index1 = TestIndex.name("protected_index1").documentCount(10).seed(1).build(); + static final TestIndex protected_index2 = TestIndex.name("protected_index2").documentCount(10).seed(2).build(); + static final TestIndex unprotected_index = TestIndex.name("unprotected_index").documentCount(10).seed(4).build(); + + static final TestAlias alias_protected = new TestAlias("alias_protected").on(protected_index1); + + static final TestSecurityConfig.User.MetadataKey ALLOWED = new TestSecurityConfig.User.MetadataKey<>( + "allowed", + IndexMatcher.class + ); + + // ------------------------------------------------------------------------------------------------------- + // Test users with different privilege configurations + // ------------------------------------------------------------------------------------------------------- + + static final TestSecurityConfig.Role PROTECTED_INDEX_ROLE = new TestSecurityConfig.Role("protected_index_role"); + + /** + * User with all index permissions but NOT member of protected index roles. + * When protected indices are enabled, they should NOT have access to protected indices. + */ + static final TestSecurityConfig.User NORMAL_USER = new TestSecurityConfig.User("normal_user")// + .description("all_access but no protected role")// + .roles( + new TestSecurityConfig.Role("all_access_role")// + .clusterPermissions("*") + .indexPermissions("*") + .on("*") + )// + .reference(ALLOWED, limitedTo(unprotected_index)); + + /** + * User with all index permissions AND member of protected_index_role. + * When protected indices are enabled, they SHOULD have full access to protected indices. + */ + static final TestSecurityConfig.User PROTECTED_INDEX_USER = new TestSecurityConfig.User("protected_index_user")// + .description("all_access with protected role")// + .roles( + new TestSecurityConfig.Role("all_access_role")// + .clusterPermissions("*") + .indexPermissions("*") + .on("*") + ) + .referencedRoles(PROTECTED_INDEX_ROLE)// + .reference(ALLOWED, limitedTo(protected_index1, protected_index2, unprotected_index)); + + static final List USERS = ImmutableList.of(NORMAL_USER, PROTECTED_INDEX_USER); + + static LocalCluster.Builder clusterBuilder() { + return new LocalCluster.Builder().singleNode() + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(USERS) + .indices(protected_index1, protected_index2, unprotected_index) + .aliases(alias_protected) + .roles(PROTECTED_INDEX_ROLE) + .nodeSettings( + Map.of( + "plugins.security.protected_indices.enabled", + true, + "plugins.security.protected_indices.indices", + "protected_index*", + "plugins.security.protected_indices.roles", + "protected_index_role" + ) + ); + } + + @ClassRule + public static final ClusterConfig.ClusterInstances clusterInstances = new ClusterConfig.ClusterInstances( + ProtectedIndexAuthorizationIntTests::clusterBuilder + ); + + final TestSecurityConfig.User user; + final LocalCluster cluster; + final ClusterConfig clusterConfig; + + @Test + public void search_protectedIndex() { + try (TestRestClient restClient = cluster.getRestClient(user)) { + String matchAllQuery = "{\"query\": {\"match_all\": {}}}"; + + TestRestClient.HttpResponse response = restClient.postJson("protected_index1/_search", matchAllQuery); + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isOk()); + assertThat(response, containsExactly(protected_index1).at("hits.hits[*]._index")); + } else if (clusterConfig.legacyPrivilegeEvaluation) { + assertThat(response, isOk()); + assertThat(response, containsExactly().at("hits.hits[*]._index")); + } else { + // Thew new privilege evaluation just forbids this request; this follows the normal index reduction semantics + assertThat(response, isForbidden()); + } + } + } + + @Test + public void search_unprotectedIndex() { + try (TestRestClient restClient = cluster.getRestClient(user)) { + String matchAllQuery = "{\"query\": {\"match_all\": {}}}"; + + TestRestClient.HttpResponse response = restClient.postJson("unprotected_index/_search", matchAllQuery); + assertThat(response, isOk()); + assertThat(response, containsExactly(unprotected_index).at("hits.hits[*]._index")); + } + } + + @Test + public void search_protectedIndexPattern() { + try (TestRestClient restClient = cluster.getRestClient(user)) { + String matchAllQuery = "{\"query\": {\"match_all\": {}}}"; + + TestRestClient.HttpResponse response = restClient.postJson("protected_index*/_search?size=100", matchAllQuery); + + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isOk()); + assertThat(response, containsExactly(protected_index1, protected_index2).at("hits.hits[*]._index")); + } else { + assertThat(response, isOk()); + assertThat(response, containsExactly().at("hits.hits[*]._index")); + } + } + } + + @Test + public void search_aliasContainingProtectedIndices() { + try (TestRestClient restClient = cluster.getRestClient(user)) { + String matchAllQuery = "{\"query\": {\"match_all\": {}}}"; + + TestRestClient.HttpResponse response = restClient.postJson("alias_protected/_search?size=100", matchAllQuery); + + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isOk()); + assertThat(response, containsExactly(protected_index1).at("hits.hits[*]._index")); + } else { + assertThat(response, isOk()); + assertThat(response, containsExactly().at("hits.hits[*]._index")); + } + } + } + + @Test + public void createDocument_protectedIndex() { + String docId = "protected_index1/_doc/create_test_doc"; + try (TestRestClient restClient = cluster.getRestClient(user)) { + TestRestClient.HttpResponse response = restClient.put(docId, json("foo", "bar")); + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isCreated()); + } else { + assertThat(response, isForbidden()); + } + } finally { + delete(docId); + } + } + + @Test + public void deleteDocument_protectedIndex() { + String docId = "protected_index1/_doc/create_test_doc"; + try (TestRestClient restClient = cluster.getRestClient(user); TestRestClient adminRestClient = cluster.getAdminCertRestClient()) { + + // Initialization: Create document as admin + { + TestRestClient.HttpResponse httpResponse = adminRestClient.put(docId + "?refresh=true", json("foo", "bar")); + assertThat(httpResponse, isCreated()); + } + + TestRestClient.HttpResponse response = restClient.delete(docId); + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isOk()); + } else { + assertThat(response, isForbidden()); + } + } finally { + delete(docId); + } + } + + @Test + public void updateMappings_protectedIndex() { + try (TestRestClient restClient = cluster.getRestClient(user)) { + String newMappings = "{\"properties\": {\"user_name\": {\"type\": \"text\"}}}"; + TestRestClient.HttpResponse response = restClient.putJson("protected_index1/_mapping", newMappings); + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isOk()); + } else { + assertThat(response, isForbidden()); + } + } + } + + @Test + public void closeIndex_protectedIndex() { + try (TestRestClient restClient = cluster.getRestClient(user)) { + TestRestClient.HttpResponse response = restClient.post("protected_index2/_close"); + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isOk()); + } else { + assertThat(response, isForbidden()); + } + } finally { + try (TestRestClient adminRestClient = cluster.getAdminCertRestClient()) { + adminRestClient.post("protected_index2/_open"); + } + } + } + + @Test + public void updateSettings_protectedIndex() { + try (TestRestClient restClient = cluster.getRestClient(user)) { + String indexSettings = "{\"index\": {\"refresh_interval\": \"5s\"}}"; + TestRestClient.HttpResponse response = restClient.putJson("protected_index1/_settings", indexSettings); + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isOk()); + } else { + assertThat(response, isForbidden()); + } + } finally { + try (TestRestClient adminRestClient = cluster.getAdminCertRestClient()) { + adminRestClient.putJson("protected_index1/_settings", "{\"index\": {\"refresh_interval\": null}}"); + } + } + } + + @Test + public void aliasOperations_protectedIndex() { + String aliasName = "test_alias_protected"; + try (TestRestClient restClient = cluster.getRestClient(user)) { + String addAliasBody = """ + {"actions": [{"add": {"index": "protected_index1", "alias": "%s"}}]} + """.formatted(aliasName); + + TestRestClient.HttpResponse response = restClient.postJson("_aliases", addAliasBody); + + if (user == PROTECTED_INDEX_USER) { + assertThat(response, isOk()); + } else { + assertThat(response, isForbidden()); + } + } finally { + // Cleanup - remove alias + try (TestRestClient adminRestClient = cluster.getAdminCertRestClient()) { + String removeAliasBody = """ + {"actions": [{"remove": {"index": "protected_index1", "alias": "%s"}}]} + """.formatted(aliasName); + adminRestClient.postJson("_aliases", removeAliasBody); + } + } + } + + @Parameterized.Parameters(name = "{0}, {2}") + public static Collection params() { + List result = new ArrayList<>(); + + for (ClusterConfig clusterConfig : ClusterConfig.values()) { + for (TestSecurityConfig.User user : USERS) { + result.add(new Object[] { clusterConfig, user, user.getDescription() }); + } + } + return result; + } + + public ProtectedIndexAuthorizationIntTests( + ClusterConfig clusterConfig, + TestSecurityConfig.User user, + @SuppressWarnings("unused") String description + ) { + this.user = user; + this.cluster = clusterInstances.get(clusterConfig); + this.clusterConfig = clusterConfig; + } + + private void delete(String... paths) { + try (TestRestClient adminRestClient = cluster.getAdminCertRestClient()) { + for (String path : paths) { + TestRestClient.HttpResponse response = adminRestClient.delete(path); + if (response.getStatusCode() != 200 && response.getStatusCode() != 404) { + throw new RuntimeException("Error while deleting " + path + "\n" + response.getBody()); + } + } + } + } +} diff --git a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/SnapshotAuthorizationIntTests.java b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/SnapshotAuthorizationIntTests.java index 3f999cbc69..bf0b4b9e4f 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/int_tests/SnapshotAuthorizationIntTests.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/int_tests/SnapshotAuthorizationIntTests.java @@ -18,7 +18,7 @@ import com.google.common.collect.ImmutableList; import org.apache.hc.core5.http.HttpEntity; import org.junit.After; -import org.junit.AfterClass; +import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -195,12 +195,10 @@ static LocalCluster.Builder clusterBuilder() { .plugin(IndexAuthorizationReadOnlyIntTests.SystemIndexTestPlugin.class); } - @AfterClass - public static void stopClusters() { - for (ClusterConfig clusterConfig : ClusterConfig.values()) { - clusterConfig.shutdown(); - } - } + @ClassRule + public static final ClusterConfig.ClusterInstances clusterInstances = new ClusterConfig.ClusterInstances( + SnapshotAuthorizationIntTests::clusterBuilder + ); final TestSecurityConfig.User user; final LocalCluster cluster; @@ -354,7 +352,7 @@ public static Collection params() { public SnapshotAuthorizationIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description) throws Exception { this.user = user; - this.cluster = clusterConfig.cluster(SnapshotAuthorizationIntTests::clusterBuilder); + this.cluster = clusterInstances.get(clusterConfig); this.clusterConfig = clusterConfig; } diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java index c84601dcc5..1be176ec8c 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java @@ -201,6 +201,17 @@ public void close() { } } + /** + * Stops the cluster without throwing an exception in case of an error. + */ + public void stopSafe() { + try { + close(); + } catch (Exception e) { + log.error("Error while stopping LocalCluster", e); + } + }; + @Override public String getClusterName() { return clusterName; diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 57af519311..8ed0d6de9b 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -2269,6 +2269,7 @@ public List> getSettings() { ); settings.add(SecuritySettings.USER_ATTRIBUTE_SERIALIZATION_ENABLED_SETTING); + settings.add(SecuritySettings.DLS_WRITE_BLOCKED); } return settings; diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index 8f9b1cc6c6..122ef92c86 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -85,11 +85,14 @@ import org.opensearch.security.setting.OpensearchDynamicSetting; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.HeaderHelper; +import org.opensearch.security.support.SecuritySettings; import org.opensearch.security.support.WildcardMatcher; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.client.Client; import static org.opensearch.security.privileges.PrivilegesEvaluatorImpl.isClusterPerm; +import static org.opensearch.security.support.ConfigConstants.SECURITY_DLS_WRITE_BLOCKED; +import static org.opensearch.security.support.ConfigConstants.SECURITY_DLS_WRITE_BLOCKED_ENABLED_DEFAULT; public class DlsFlsValveImpl implements DlsFlsRequestValve { @@ -109,6 +112,7 @@ public class DlsFlsValveImpl implements DlsFlsRequestValve { private final AdminDNs adminDNs; private final OpensearchDynamicSetting resourceSharingEnabledSetting; private final ResourcePluginInfo resourcePluginInfo; + private volatile boolean dlsWriteBlockedEnabled; public DlsFlsValveImpl( Settings settings, @@ -142,6 +146,12 @@ public DlsFlsValveImpl( config.updateClusterStateMetadataAsync(clusterService, threadPool); } }); + this.dlsWriteBlockedEnabled = settings.getAsBoolean(SECURITY_DLS_WRITE_BLOCKED, SECURITY_DLS_WRITE_BLOCKED_ENABLED_DEFAULT); + if (clusterService.getClusterSettings() != null) { + clusterService.getClusterSettings().addSettingsUpdateConsumer(SecuritySettings.DLS_WRITE_BLOCKED, newDlsWriteBlockedEnabled -> { + dlsWriteBlockedEnabled = newDlsWriteBlockedEnabled; + }); + } this.resourceSharingEnabledSetting = resourceSharingEnabledSetting; } @@ -333,11 +343,21 @@ public boolean invoke(PrivilegesEvaluationContext context, final ActionListener< if (request instanceof BulkShardRequest) { for (BulkItemRequest inner : ((BulkShardRequest) request).items()) { - if (inner.request() instanceof UpdateRequest) { + if (dlsWriteBlockedEnabled) { listener.onFailure( - new OpenSearchSecurityException("Update is not supported when FLS or DLS or Fieldmasking is activated") + new OpenSearchSecurityException( + inner.request().getClass().getSimpleName() + + " is not supported when FLS or DLS or Fieldmasking is activated" + ) ); return false; + } else { + if (inner.request() instanceof UpdateRequest) { + listener.onFailure( + new OpenSearchSecurityException("Update is not supported when FLS or DLS or Fieldmasking is activated") + ); + return false; + } } } } diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index a7a271c758..37eb41a771 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -340,6 +340,8 @@ public enum RolesMappingResolution { public static final String SECURITY_FILTER_SECURITYINDEX_FROM_ALL_REQUESTS = SECURITY_SETTINGS_PREFIX + "filter_securityindex_from_all_requests"; public static final String SECURITY_DLS_MODE = SECURITY_SETTINGS_PREFIX + "dls.mode"; + public static final String SECURITY_DLS_WRITE_BLOCKED = SECURITY_SETTINGS_PREFIX + "dls.write_blocked"; + public static final boolean SECURITY_DLS_WRITE_BLOCKED_ENABLED_DEFAULT = false; // REST API public static final String SECURITY_RESTAPI_ROLES_ENABLED = SECURITY_SETTINGS_PREFIX + "restapi.roles_enabled"; public static final String SECURITY_RESTAPI_ADMIN_ENABLED = SECURITY_SETTINGS_PREFIX + "restapi.admin.enabled"; diff --git a/src/main/java/org/opensearch/security/support/SecuritySettings.java b/src/main/java/org/opensearch/security/support/SecuritySettings.java index 4a442c9316..cb5e6c1cd1 100644 --- a/src/main/java/org/opensearch/security/support/SecuritySettings.java +++ b/src/main/java/org/opensearch/security/support/SecuritySettings.java @@ -42,4 +42,11 @@ public class SecuritySettings { Setting.Property.NodeScope, Setting.Property.Dynamic ); // Not filtered + + public static final Setting DLS_WRITE_BLOCKED = Setting.boolSetting( + ConfigConstants.SECURITY_DLS_WRITE_BLOCKED, + false, + Setting.Property.NodeScope, + Setting.Property.Dynamic + ); }