diff --git a/CHANGELOG.md b/CHANGELOG.md index d00370b5d4..96ad79717f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Make security plugin aware of FIPS build param (-Pcrypto.standard=FIPS-140-3) ([#5952](https://github.com/opensearch-project/security/pull/5952)) - Hardens input validation for resource sharing APIs ([#5831](https://github.com/opensearch-project/security/pull/5831) - Optimize getFieldFilter to only return a predicate when index has FLS restrictions for user ([#5777](https://github.com/opensearch-project/security/pull/5777)) +- Make `plugins.security.dfm_empty_overrides_all` dynamically toggleable ([#6016](https://github.com/opensearch-project/security/pull/6016) - Performance optimizations for building internal authorization data structures upon config updates ([#5988](https://github.com/opensearch-project/security/pull/5988)) - Make encryption_key optional for obo token authenticator ([#6017](https://github.com/opensearch-project/security/pull/6017) - [Resource Sharing] Using custom action prefixes for sample resource plugin ([#6020](https://github.com/opensearch-project/security/pull/6020) diff --git a/src/integrationTest/java/org/opensearch/security/dlsfls/DfmEmptyOverridesAllDynamicSettingTest.java b/src/integrationTest/java/org/opensearch/security/dlsfls/DfmEmptyOverridesAllDynamicSettingTest.java new file mode 100644 index 0000000000..3466675332 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/dlsfls/DfmEmptyOverridesAllDynamicSettingTest.java @@ -0,0 +1,151 @@ +/* + * 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 java.util.List; +import java.util.Map; + +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 org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +/** + * Integration tests verifying that plugins.security.dfm_empty_overrides_all can be toggled + * dynamically via the cluster settings API without requiring a node restart. + * + *

The setting controls whether a role without a DLS/FLS rule overrides roles that do have + * restrictions. When true, a role with no restriction on an index grants full access even if + * another mapped role restricts it. + */ +public class DfmEmptyOverridesAllDynamicSettingTest { + + static final String INDEX = "dfm_test_index"; + + static final User ADMIN_USER = new User("admin").roles(ALL_ACCESS); + + /** + * User with two roles: + * - one role that applies a DLS filter (only docs where dept=sales) + * - one role with a wildcard index pattern and NO DLS rule (unrestricted) + * + * When dfm_empty_overrides_all=true the unrestricted role wins and the user sees all docs. + * When dfm_empty_overrides_all=false the restricted role wins and the user sees only sales docs. + */ + static final User MIXED_ROLE_USER = new User("mixed_role_user").roles( + new Role("dls_restricted_role").clusterPermissions("cluster_composite_ops_ro") + .indexPermissions("read") + .dls("{\"term\":{\"dept\":\"sales\"}}") + .on(INDEX), + new Role("unrestricted_wildcard_role").clusterPermissions("cluster_composite_ops_ro").indexPermissions("read").on("*") + ); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(ADMIN_USER, MIXED_ROLE_USER) + .nodeSettings(Map.of(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()))) + .build(); + + @BeforeClass + public static void createTestData() throws IOException { + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + client.putJson(INDEX + "/_doc/1?refresh=true", "{\"dept\":\"sales\",\"value\":1}"); + client.putJson(INDEX + "/_doc/2?refresh=true", "{\"dept\":\"engineering\",\"value\":2}"); + client.putJson(INDEX + "/_doc/3?refresh=true", "{\"dept\":\"marketing\",\"value\":3}"); + } + } + + private void setDfmEmptyOverridesAll(boolean enabled) throws IOException { + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + HttpResponse response = client.putJson( + "_cluster/settings", + String.format("{\"persistent\":{\"plugins.security.dfm_empty_overrides_all\":%b}}", enabled) + ); + assertThat("Failed to update cluster setting", response.getStatusCode(), is(200)); + } + } + + private int countHits(TestRestClient client) throws IOException { + HttpResponse response = client.get(INDEX + "/_search?size=10"); + assertThat("Search failed", response.getStatusCode(), is(200)); + return response.getIntFromJsonBody("/hits/total/value"); + } + + @Test + public void testSettingFalse_restrictedRoleWins_userSeesOnlySalesDocs() throws IOException { + setDfmEmptyOverridesAll(false); + try (TestRestClient client = cluster.getRestClient(MIXED_ROLE_USER)) { + int hits = countHits(client); + assertThat("With dfm_empty_overrides_all=false, DLS filter should apply and only sales doc visible", hits, is(1)); + } + } + + @Test + public void testSettingTrue_unrestrictedRoleWins_userSeesAllDocs() throws IOException { + setDfmEmptyOverridesAll(true); + try (TestRestClient client = cluster.getRestClient(MIXED_ROLE_USER)) { + int hits = countHits(client); + assertThat("With dfm_empty_overrides_all=true, unrestricted role should override DLS and all docs visible", hits, is(3)); + } + } + + @Test + public void testDynamicToggle_fromTrueToFalse_restrictionApplied() throws IOException { + setDfmEmptyOverridesAll(true); + try (TestRestClient client = cluster.getRestClient(MIXED_ROLE_USER)) { + assertThat("Expected all docs visible when setting is true", countHits(client), is(3)); + } + + setDfmEmptyOverridesAll(false); + try (TestRestClient client = cluster.getRestClient(MIXED_ROLE_USER)) { + assertThat("Expected only sales doc visible after toggling setting to false", countHits(client), is(1)); + } + } + + @Test + public void testDynamicToggle_fromFalseToTrue_restrictionLifted() throws IOException { + setDfmEmptyOverridesAll(false); + try (TestRestClient client = cluster.getRestClient(MIXED_ROLE_USER)) { + assertThat("Expected only sales doc visible when setting is false", countHits(client), is(1)); + } + + setDfmEmptyOverridesAll(true); + try (TestRestClient client = cluster.getRestClient(MIXED_ROLE_USER)) { + assertThat("Expected all docs visible after toggling setting to true", countHits(client), is(3)); + } + } + + @Test + public void testAdminUser_alwaysSeesAllDocs_regardlessOfSetting() throws IOException { + setDfmEmptyOverridesAll(false); + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + assertThat("Admin should always see all docs", countHits(client), is(3)); + } + + setDfmEmptyOverridesAll(true); + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + assertThat("Admin should always see all docs", countHits(client), is(3)); + } + } +} diff --git a/src/integrationTest/java/org/opensearch/security/dlsfls/SensitiveClusterSettingsAccessTest.java b/src/integrationTest/java/org/opensearch/security/dlsfls/SensitiveClusterSettingsAccessTest.java new file mode 100644 index 0000000000..e29b86a4da --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/dlsfls/SensitiveClusterSettingsAccessTest.java @@ -0,0 +1,101 @@ +/* + * 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.util.List; +import java.util.Map; + +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 org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +/** + * Verifies that plugins.security.dfm_empty_overrides_all (a Sensitive setting) can only be + * updated by users whose role is listed in plugins.security.restapi.roles_enabled, and that + * a user with only cluster:admin/settings/put is denied with 403. + */ +public class SensitiveClusterSettingsAccessTest { + + static final String DFM_SETTING_BODY = "{\"persistent\":{\"plugins.security.dfm_empty_overrides_all\":true}}"; + + static final User SECURITY_ADMIN = new User("security_admin").roles(ALL_ACCESS); + + static final Role SETTINGS_ONLY_ROLE = new Role("settings_only_role").clusterPermissions( + "cluster:admin/settings/put", + "cluster:admin/settings/update" + ); + static final User SETTINGS_USER = new User("settings_user").roles(SETTINGS_ONLY_ROLE); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(SECURITY_ADMIN, SETTINGS_USER) + .nodeSettings(Map.of(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + SECURITY_ADMIN.getName() + "__" + ALL_ACCESS.getName()))) + .build(); + + @Test + public void adminCertUser_canUpdateSensitiveSetting() { + try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { + HttpResponse response = client.putJson("_cluster/settings", DFM_SETTING_BODY); + assertThat(response.getStatusCode(), is(200)); + } + } + + @Test + public void securityAdmin_canUpdateSensitiveSetting() { + try (TestRestClient client = cluster.getRestClient(SECURITY_ADMIN)) { + HttpResponse response = client.putJson("_cluster/settings", DFM_SETTING_BODY); + assertThat(response.getStatusCode(), is(200)); + } + } + + @Test + public void userWithOnlyClusterSettingsPerm_cannotUpdateSensitiveSetting() { + try (TestRestClient client = cluster.getRestClient(SETTINGS_USER)) { + HttpResponse response = client.putJson("_cluster/settings", DFM_SETTING_BODY); + assertThat(response.getStatusCode(), is(403)); + } + } + + @Test + public void userWithOnlyClusterSettingsPerm_cannotUpdateMixedPayloadContainingSensitiveSetting() { + try (TestRestClient client = cluster.getRestClient(SETTINGS_USER)) { + HttpResponse response = client.putJson( + "_cluster/settings", + "{\"persistent\":{\"indices.recovery.max_bytes_per_sec\":\"50mb\",\"plugins.security.dfm_empty_overrides_all\":true}}" + ); + assertThat(response.getStatusCode(), is(403)); + } + } + + @Test + public void userWithOnlyClusterSettingsPerm_canStillUpdateNonSensitiveSetting() { + try (TestRestClient client = cluster.getRestClient(SETTINGS_USER)) { + // indices.recovery.max_bytes_per_sec is a core Dynamic setting with no Sensitive property + HttpResponse response = client.putJson( + "_cluster/settings", + "{\"transient\":{\"indices.recovery.max_bytes_per_sec\":\"50mb\"}}" + ); + assertThat(response.getStatusCode(), is(200)); + } + } +} diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 9e342bcd8f..6cd6175a71 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1646,9 +1646,7 @@ public List> getSettings() { Property.Filtered ) ); - settings.add( - Setting.boolSetting(ConfigConstants.SECURITY_DFM_EMPTY_OVERRIDES_ALL, false, Property.NodeScope, Property.Filtered) - ); + settings.add(SecuritySettings.DFM_EMPTY_OVERRIDES_ALL_SETTING); settings.add(Setting.groupSetting(ConfigConstants.SECURITY_AUTHCZ_REST_IMPERSONATION_USERS + ".", Property.NodeScope)); // not // filtered // here diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index 34d77a35d5..2865b6b64b 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -146,6 +146,15 @@ public DlsFlsValveImpl( clusterService.getClusterSettings().addSettingsUpdateConsumer(SecuritySettings.DLS_WRITE_BLOCKED, newDlsWriteBlockedEnabled -> { dlsWriteBlockedEnabled = newDlsWriteBlockedEnabled; }); + clusterService.getClusterSettings() + .addSettingsUpdateConsumer(SecuritySettings.DFM_EMPTY_OVERRIDES_ALL_SETTING, newDfmEmptyOverridesAll -> { + DlsFlsProcessedConfig config = dlsFlsBaseContext.config(); + if (config != null) { + config.getDocumentPrivileges().setDfmEmptyOverridesAll(newDfmEmptyOverridesAll); + config.getFieldPrivileges().setDfmEmptyOverridesAll(newDfmEmptyOverridesAll); + config.getFieldMasking().setDfmEmptyOverridesAll(newDfmEmptyOverridesAll); + } + }); } this.resourceSharingEnabledSetting = resourceSharingEnabledSetting; } diff --git a/src/main/java/org/opensearch/security/filter/SecurityFilter.java b/src/main/java/org/opensearch/security/filter/SecurityFilter.java index f4e005d6a2..ef1196fafa 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityFilter.java @@ -31,6 +31,7 @@ import java.util.UUID; import java.util.function.Consumer; import java.util.stream.Collectors; +import java.util.stream.Stream; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; @@ -43,6 +44,8 @@ import org.opensearch.ResourceAlreadyExistsException; import org.opensearch.action.ActionRequest; import org.opensearch.action.DocWriteRequest.OpType; +import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsAction; +import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest; import org.opensearch.action.admin.indices.alias.Alias; import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest; @@ -123,6 +126,7 @@ public class SecurityFilter implements ActionFilter { private final UserInjector userInjector; private final ResourceAccessEvaluator resourceAccessEvaluator; private final ThreadContextUserInfo threadContextUserInfo; + private final Set restApiAllowedRoles; public SecurityFilter( final Settings settings, @@ -153,6 +157,7 @@ public SecurityFilter( this.rolesInjector = new RolesInjector(auditLog); this.userInjector = new UserInjector(settings, threadPool, auditLog, xffResolver); this.resourceAccessEvaluator = resourceAccessEvaluator; + this.restApiAllowedRoles = Set.copyOf(settings.getAsList(ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED)); this.threadContextUserInfo = new ThreadContextUserInfo( threadPool.getThreadContext(), privilegesConfiguration, @@ -435,6 +440,11 @@ private void ap return; } + // Block cluster-settings updates that touch Sensitive settings unless the user holds a restapi-allowed role + if (handleBlockedSensitiveSettingsUpdate(action, request, context, listener)) { + return; + } + // not a resource‐sharing request → fall back into the normal PrivilegesEvaluator PrivilegesEvaluatorResponse pres = eval.evaluate(context); @@ -520,6 +530,37 @@ private void ap } } + private boolean handleBlockedSensitiveSettingsUpdate( + String action, + Request request, + PrivilegesEvaluationContext context, + ActionListener listener + ) { + if (!ClusterUpdateSettingsAction.NAME.equals(action)) { + return false; + } + ClusterUpdateSettingsRequest settingsRequest = (ClusterUpdateSettingsRequest) request; + boolean touchesSensitiveSetting = Stream.concat( + settingsRequest.transientSettings().keySet().stream(), + settingsRequest.persistentSettings().keySet().stream() + ).anyMatch(key -> cs.getClusterSettings().isSensitiveSetting(key)); + + if (!touchesSensitiveSetting) { + return false; + } + if (Collections.disjoint(restApiAllowedRoles, context.getMappedRoles())) { + log.debug("User {} does not have a role permitted to update sensitive cluster settings", context.getUser().getName()); + listener.onFailure( + new OpenSearchSecurityException( + "User " + context.getUser().getName() + " does not have permission to update sensitive cluster settings", + RestStatus.FORBIDDEN + ) + ); + return true; + } + return false; + } + private static boolean isUserAdmin(User user, final AdminDNs adminDns) { return user != null && adminDns.isAdmin(user); } diff --git a/src/main/java/org/opensearch/security/privileges/dlsfls/AbstractRuleBasedPrivileges.java b/src/main/java/org/opensearch/security/privileges/dlsfls/AbstractRuleBasedPrivileges.java index 45a3180fb2..f69fda20fe 100644 --- a/src/main/java/org/opensearch/security/privileges/dlsfls/AbstractRuleBasedPrivileges.java +++ b/src/main/java/org/opensearch/security/privileges/dlsfls/AbstractRuleBasedPrivileges.java @@ -90,7 +90,7 @@ abstract class AbstractRuleBasedPrivileges(compiledRoles, indexMetadata, roleToRuleFunction) : null; } + public void setDfmEmptyOverridesAll(boolean dfmEmptyOverridesAll) { + this.dfmEmptyOverridesAll = dfmEmptyOverridesAll; + } + /** * Returns true if the user identified in the PrivilegesEvaluationContext does not have any restrictions in any case, * independently of the indices they are requesting. diff --git a/src/main/java/org/opensearch/security/support/SecuritySettings.java b/src/main/java/org/opensearch/security/support/SecuritySettings.java index cb5e6c1cd1..61a17a953e 100644 --- a/src/main/java/org/opensearch/security/support/SecuritySettings.java +++ b/src/main/java/org/opensearch/security/support/SecuritySettings.java @@ -49,4 +49,12 @@ public class SecuritySettings { Setting.Property.NodeScope, Setting.Property.Dynamic ); + + public static final Setting DFM_EMPTY_OVERRIDES_ALL_SETTING = Setting.boolSetting( + ConfigConstants.SECURITY_DFM_EMPTY_OVERRIDES_ALL, + false, + Setting.Property.NodeScope, + Setting.Property.Dynamic, + Setting.Property.Sensitive + ); }