From cca9ecaae5d452d6496db58e309b5e27425952a9 Mon Sep 17 00:00:00 2001 From: Andrey Pleskach Date: Thu, 31 Aug 2023 21:35:28 +0200 Subject: [PATCH] Fix roles verification for roles mapping and internal users --- .../dlic/rest/api/InternalUsersApiAction.java | 29 +++++-- .../dlic/rest/api/RolesMappingApiAction.java | 5 +- .../rest/validation/EndpointValidator.java | 28 +++---- .../api/AbstractApiActionValidationTest.java | 3 +- .../InternalUsersApiActionValidationTest.java | 75 +++++++++++++++++++ .../RolesMappingApiActionValidationTest.java | 12 ++- .../validation/EndpointValidatorTest.java | 14 ++-- 7 files changed, 127 insertions(+), 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 index 14036918b3..ef5a3d49c8 100644 --- 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 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 one of roles is not hidden and all mappings for the role is not immutable + 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 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 index 0c71a4731f..15fef92a5f 100644 --- 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 onConfigChange(SecurityConfiguration securityConfiguration) throws IOException { - return EndpointValidator.super.onConfigChange(securityConfiguration).map(this::validateRoleForMapping); + return EndpointValidator.super.onConfigChange(securityConfiguration).map(this::validateRole); } - private ValidationResult validateRoleForMapping(final SecurityConfiguration securityConfiguration) + private ValidationResult 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 index 044ccd996a..b618423058 100644 --- 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 entityHidden(final SecurityConfi default ValidationResult> validateRoles( final List roles, final SecurityDynamicConfiguration rolesConfiguration - ) { - final var rolesToCheck = roles == null ? List.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() - .>>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 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 index 5336b5e8de..b3d3dc0571 100644 --- 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/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java index 137b4b5a9e..a87d6248c5 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/InternalUsersApiActionValidationTest.java +++ b/src/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,36 @@ 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) 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 +130,45 @@ 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 index bbcc5021ff..8c1b6b9285 100644 --- 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 index 20cfe33126..8e3d6f5e70 100644 --- 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