[BUG] Fix roles verification for roles mapping and internal users (#3278

) The resent refactoring of the REST APIs: #3123 introduce a regression in how roles-mapping verification has worked before. The old solution verified only hidden roles both for internal users and roles mapping, while new was too strict and forbid to do it for both. This PR fixes the problem and uses the same logic as it was before. - In case of roles-mapping it verifies only a role associated with it that the role is not hidden. - In case of internal users it verifies that a role is not hidden and roles-mapping associated with the role is mutable So verification was split and added to the corresponding ActionApi class which is more convenient as it was before. Signed-off-by: Andrey Pleskach <[email protected]>
opensearch-project · Sep 1, 2023 · 53f64b9 · 53f64b9
1 parent 14574dd
commit 53f64b9
Show file tree

Hide file tree

Showing 7 changed files with 135 additions and 36 deletions.
diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
@@ -148,13 +148,28 @@ void generateAuthToken(final RestChannel channel, final SecurityConfiguration se
     }
 
     ValidationResult<SecurityConfiguration> validateSecurityRoles(final SecurityConfiguration securityConfiguration) throws IOException {
-        return loadConfiguration(CType.ROLES, false, false).map(rolesConfiguration -> {
-            final var contentAsNode = (ObjectNode) securityConfiguration.requestContent();
-            final var securityJsonNode = new SecurityJsonNode(contentAsNode);
-            final var securityRoles = securityJsonNode.get("opendistro_security_roles").asList();
-            return endpointValidator.validateRoles(securityRoles, rolesConfiguration)
-                .map(ignore -> ValidationResult.success(securityConfiguration));
-        });
+        // check here that all roles are not hidden and all mappings are mutable (not static, reserved or hidden)
+        return loadConfiguration(CType.ROLES, false, false).map(
+            rolesConfiguration -> loadConfiguration(CType.ROLESMAPPING, false, false).map(roleMappingsConfiguration -> {
+                final var contentAsNode = (ObjectNode) securityConfiguration.requestContent();
+                final var securityJsonNode = new SecurityJsonNode(contentAsNode);
+                var securityRoles = securityJsonNode.get("opendistro_security_roles").asList();
+                securityRoles = securityRoles == null ? List.of() : securityRoles;
+                final var rolesValid = endpointValidator.validateRoles(securityRoles, rolesConfiguration);
+                if (!rolesValid.isValid()) {
+                    return ValidationResult.error(rolesValid.status(), rolesValid.errorMessage());
+                }
+                for (final var role : securityRoles) {
+                    final var roleMappingValid = endpointValidator.isAllowedToChangeImmutableEntity(
+                        SecurityConfiguration.of(role, roleMappingsConfiguration)
+                    );
+                    if (!roleMappingValid.isValid()) {
+                        return ValidationResult.error(roleMappingValid.status(), roleMappingValid.errorMessage());
+                    }
+                }
+                return ValidationResult.success(securityConfiguration);
+            })
+        );
     }
 
     ValidationResult<SecurityConfiguration> createOrUpdateAccount(

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/RolesMappingApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/RolesMappingApiAction.java
@@ -84,11 +84,12 @@ public RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator() {
 
             @Override
             public ValidationResult<SecurityConfiguration> onConfigChange(SecurityConfiguration securityConfiguration) throws IOException {
-                return EndpointValidator.super.onConfigChange(securityConfiguration).map(this::validateRoleForMapping);
+                return EndpointValidator.super.onConfigChange(securityConfiguration).map(this::validateRole);
             }
 
-            private ValidationResult<SecurityConfiguration> validateRoleForMapping(final SecurityConfiguration securityConfiguration)
+            private ValidationResult<SecurityConfiguration> validateRole(final SecurityConfiguration securityConfiguration)
                 throws IOException {
+                // check here that role is not hidden for the mapping
                 return loadConfiguration(CType.ROLES, false, false).map(
                     rolesConfiguration -> validateRoles(List.of(securityConfiguration.entityName()), rolesConfiguration)
                 ).map(ignore -> ValidationResult.success(securityConfiguration));

diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java
@@ -11,11 +11,9 @@
 import java.util.List;
 import java.util.Objects;
 
-import static java.util.function.Predicate.not;
 import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage;
 import static org.opensearch.security.dlic.rest.api.Responses.forbiddenMessage;
 import static org.opensearch.security.dlic.rest.api.Responses.notFoundMessage;
-import static org.opensearch.security.dlic.rest.support.Utils.withIOException;
 
 public interface EndpointValidator {
 
@@ -132,18 +130,16 @@ default ValidationResult<SecurityConfiguration> entityHidden(final SecurityConfi
     default ValidationResult<SecurityDynamicConfiguration<?>> validateRoles(
         final List<String> roles,
         final SecurityDynamicConfiguration<?> rolesConfiguration
-    ) {
-        final var rolesToCheck = roles == null ? List.<String>of() : roles;
-        return rolesToCheck.stream().map(role -> withIOException(() -> {
-            final var roleSecConfig = SecurityConfiguration.of(role, rolesConfiguration);
-            return entityExists("role", roleSecConfig).map(this::isAllowedToChangeImmutableEntity);
-        }))
-            .filter(not(ValidationResult::isValid))
-            .findFirst()
-            .<ValidationResult<SecurityDynamicConfiguration<?>>>map(
-                result -> ValidationResult.error(result.status(), result.errorMessage())
-            )
-            .orElseGet(() -> ValidationResult.success(rolesConfiguration));
+    ) throws IOException {
+        for (final var role : roles) {
+            final var validRole = entityExists("role", SecurityConfiguration.of(role, rolesConfiguration)).map(
+                this::isAllowedToLoadOrChangeHiddenEntity
+            );
+            if (!validRole.isValid()) {
+                return ValidationResult.error(validRole.status(), validRole.errorMessage());
+            }
+        }
+        return ValidationResult.success(rolesConfiguration);
     }
 
     default ValidationResult<SecurityConfiguration> isAllowedToChangeEntityWithRestAdminPermissions(

diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/AbstractApiActionValidationTest.java
@@ -78,12 +78,13 @@ void setupRolesConfiguration() throws IOException {
         final var config = objectMapper.createObjectNode();
         config.set("_meta", objectMapper.createObjectNode().put("type", CType.ROLES.toLCString()).put("config_version", 2));
         config.set("kibana_read_only", objectMapper.createObjectNode().put("reserved", true));
+        config.set("some_hidden_role", objectMapper.createObjectNode().put("hidden", true));
+        config.set("all_access", objectMapper.createObjectNode().put("static", true)); // it reserved as well
         config.set("security_rest_api_access", objectMapper.createObjectNode().put("reserved", true));
 
         final var array = objectMapper.createArrayNode();
         restApiAdminPermissions().forEach(array::add);
         config.set("rest_api_admin_role", objectMapper.createObjectNode().set("cluster_permissions", array));
-
         config.set("regular_role", objectMapper.createObjectNode().set("cluster_permissions", objectMapper.createArrayNode().add("*")));
 
         rolesConfiguration = SecurityDynamicConfiguration.fromJson(objectMapper.writeValueAsString(config), CType.ROLES, 2, 1, 1);

diff --git a/...test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java b/...test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java
@@ -12,16 +12,22 @@
 package org.opensearch.security.dlic.rest.api;
 
 import org.bouncycastle.crypto.generators.OpenBSDBCrypt;
+import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.opensearch.core.rest.RestStatus;
 import org.opensearch.rest.RestRequest;
+import org.opensearch.security.DefaultObjectMapper;
+import org.opensearch.security.securityconf.impl.CType;
 import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
 import org.opensearch.security.securityconf.impl.v7.InternalUserV7;
+import org.opensearch.security.securityconf.impl.v7.RoleV7;
 import org.opensearch.security.user.UserService;
 import org.opensearch.security.util.FakeRestRequest;
 
+import java.io.IOException;
+import java.util.List;
 import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
@@ -37,6 +43,42 @@ public class InternalUsersApiActionValidationTest extends AbstractApiActionValid
     @Mock
     SecurityDynamicConfiguration<?> configuration;
 
+    @Before
+    public void setupRolesAndMappings() throws IOException {
+        setupRolesConfiguration();
+
+        final var allClusterPermissions = new RoleV7();
+        allClusterPermissions.setCluster_permissions(List.of("*"));
+        @SuppressWarnings("unchecked")
+        final var c = (SecurityDynamicConfiguration<RoleV7>) rolesConfiguration;
+        c.putCEntry("some_role_with_static_mapping", allClusterPermissions);
+        c.putCEntry("some_role_with_reserved_mapping", allClusterPermissions);
+        c.putCEntry("some_role_with_hidden_mapping", allClusterPermissions);
+
+        final var objectMapper = DefaultObjectMapper.objectMapper;
+        final var config = objectMapper.createObjectNode();
+        config.set("_meta", objectMapper.createObjectNode().put("type", CType.ROLESMAPPING.toLCString()).put("config_version", 2));
+        config.set("kibana_read_only", objectMapper.createObjectNode());
+        config.set("some_hidden_role", objectMapper.createObjectNode());
+        config.set("all_access", objectMapper.createObjectNode());
+        config.set("regular_role", objectMapper.createObjectNode());
+
+        config.set("some_role_with_static_mapping", objectMapper.createObjectNode().put("static", true));
+        config.set("some_role_with_reserved_mapping", objectMapper.createObjectNode().put("reserved", true));
+        config.set("some_role_with_hidden_mapping", objectMapper.createObjectNode().put("hidden", true));
+
+        final var rolesMappingConfiguration = SecurityDynamicConfiguration.fromJson(
+            objectMapper.writeValueAsString(config),
+            CType.ROLES,
+            2,
+            1,
+            1
+        );
+        when(configurationRepository.getConfigurationsFromIndex(List.of(CType.ROLESMAPPING), false)).thenReturn(
+            Map.of(CType.ROLESMAPPING, rolesMappingConfiguration)
+        );
+    }
+
     @Test
     public void replacePasswordWithHash() throws Exception {
         final var internalUsersApiActionEndpointValidator = createInternalUsersApiAction().createEndpointValidator();
@@ -94,6 +136,48 @@ public void validateAndUpdatePassword() throws Exception {
         assertEquals(RestStatus.INTERNAL_SERVER_ERROR, result.status());
     }
 
+    @Test
+    public void validateSecurityRolesWithMutableRolesMappingConfig() throws Exception {
+        final var internalUsersApiAction = createInternalUsersApiAction();
+
+        // should ok to set regular role with mutable role mapping
+        var userJson = objectMapper.createObjectNode().set("opendistro_security_roles", objectMapper.createArrayNode().add("regular_role"));
+        var result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration));
+        assertTrue(result.isValid());
+        // should be ok to set reserved role with mutable role mapping
+        userJson = objectMapper.createObjectNode().set("opendistro_security_roles", objectMapper.createArrayNode().add("kibana_read_only"));
+        result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration));
+        assertTrue(result.isValid());
+        // should be ok to set static role with mutable role mapping
+        userJson = objectMapper.createObjectNode().set("opendistro_security_roles", objectMapper.createArrayNode().add("all_access"));
+        result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration));
+        assertTrue(result.isValid());
+        // should not be ok to set hidden role with mutable role mapping
+        userJson = objectMapper.createObjectNode().set("opendistro_security_roles", objectMapper.createArrayNode().add("some_hidden_role"));
+        result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration));
+        assertFalse(result.isValid());
+    }
+
+    @Test
+    public void validateSecurityRolesWithImmutableRolesMappingConfig() throws Exception {
+        final var internalUsersApiAction = createInternalUsersApiAction();
+        // should not be ok to set role with hidden role mapping
+        var userJson = objectMapper.createObjectNode()
+            .set("opendistro_security_roles", objectMapper.createArrayNode().add("some_role_with_hidden_mapping"));
+        var result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration));
+        assertFalse(result.isValid());
+        // should not be ok to set role with reserved role mapping
+        userJson = objectMapper.createObjectNode()
+            .set("opendistro_security_roles", objectMapper.createArrayNode().add("some_role_with_reserved_mapping"));
+        result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration));
+        assertFalse(result.isValid());
+        // should not be ok to set role with static role mapping
+        userJson = objectMapper.createObjectNode()
+            .set("opendistro_security_roles", objectMapper.createArrayNode().add("some_role_with_static_mapping"));
+        result = internalUsersApiAction.validateSecurityRoles(SecurityConfiguration.of(userJson, "some_user", configuration));
+        assertFalse(result.isValid());
+    }
+
     private InternalUsersApiAction createInternalUsersApiAction() {
         return new InternalUsersApiAction(clusterService, threadPool, userService, securityApiDependencies);
     }

diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/RolesMappingApiActionValidationTest.java
@@ -72,13 +72,19 @@ public void onConfigChangeShouldCheckRoles() throws Exception {
         var result = rolesApiActionEndpointValidator.onConfigChange(SecurityConfiguration.of("aaa", configuration));
         assertFalse(result.isValid());
         assertEquals(RestStatus.NOT_FOUND, result.status());
-        //reserved role is not ok
+        //static role is ok
+        result = rolesApiActionEndpointValidator.onConfigChange(SecurityConfiguration.of("all_access", configuration));
+        assertTrue(result.isValid());
+        //reserved role is ok
         result = rolesApiActionEndpointValidator.onConfigChange(SecurityConfiguration.of("kibana_read_only", configuration));
-        assertFalse(result.isValid());
-        assertEquals(RestStatus.FORBIDDEN, result.status());
+        assertTrue(result.isValid());
         //just regular_role
         result = rolesApiActionEndpointValidator.onConfigChange(SecurityConfiguration.of("regular_role", configuration));
         assertTrue(result.isValid());
+        //hidden role is not ok
+        result = rolesApiActionEndpointValidator.onConfigChange(SecurityConfiguration.of("some_hidden_role", configuration));
+        assertFalse(result.isValid());
+        assertEquals(RestStatus.NOT_FOUND, result.status());
     }
 
 }
diff --git a/src/test/java/org/opensearch/security/dlic/rest/validation/EndpointValidatorTest.java b/src/test/java/org/opensearch/security/dlic/rest/validation/EndpointValidatorTest.java
@@ -28,6 +28,7 @@
 import org.opensearch.security.securityconf.impl.v7.ActionGroupsV7;
 import org.opensearch.security.securityconf.impl.v7.RoleV7;
 
+import java.io.IOException;
 import java.util.List;
 
 import static org.junit.Assert.assertEquals;
@@ -268,7 +269,7 @@ public void entityNotImmutable() throws Exception {
     }
 
     @Test
-    public void validateRolesForAdmin() {
+    public void validateRolesForAdmin() throws IOException {
         configureRoles(true);
         final var expectedResultForRoles = List.of(
             Triple.of("valid_role", true, RestStatus.OK),
@@ -287,12 +288,12 @@ public void validateRolesForAdmin() {
     }
 
     @Test
-    public void validateRolesForRegularUser() {
+    public void validateRolesForRegularUser() throws IOException {
         configureRoles(false);
         final var expectedResultForRoles = List.of(
             Triple.of("valid_role", true, RestStatus.OK),
-            Triple.of("reserved_role", false, RestStatus.FORBIDDEN),
-            Triple.of("static_role", false, RestStatus.FORBIDDEN),
+            Triple.of("reserved_role", true, RestStatus.OK),
+            Triple.of("static_role", true, RestStatus.OK),
             Triple.of("hidden_role", false, RestStatus.NOT_FOUND),
             Triple.of("non_existing_role", false, RestStatus.NOT_FOUND)
         );
@@ -315,17 +316,12 @@ private void configureRoles(final boolean isAdmin) {
 
         when(configuration.exists("static_role")).thenReturn(true);
         when(configuration.isHidden("static_role")).thenReturn(false);
-        when(configuration.isStatic("static_role")).thenReturn(true);
 
         when(configuration.exists("reserved_role")).thenReturn(true);
         when(configuration.isHidden("reserved_role")).thenReturn(false);
-        when(configuration.isStatic("reserved_role")).thenReturn(false);
-        when(configuration.isReserved("reserved_role")).thenReturn(true);
 
         when(configuration.exists("valid_role")).thenReturn(true);
         when(configuration.isHidden("valid_role")).thenReturn(false);
-        when(configuration.isStatic("valid_role")).thenReturn(false);
-        when(configuration.isReserved("valid_role")).thenReturn(false);
     }
 
     @Test