Fix roles and roles mapping verification for roles mapping and intern…

…al users
opensearch-project · Aug 31, 2023 · fb956de · fb956de
1 parent 0338cdd
commit fb956de
Show file tree

Hide file tree

Showing 7 changed files with 136 additions and 39 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,7 @@
 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;
+import static org.opensearch.security.dlic.rest.api.Responses.*;
 
 public interface EndpointValidator {
 
@@ -132,18 +128,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