diff --git a/config/internal_users.yml b/config/internal_users.yml index f4d31e52c6..f428f0a24e 100644 --- a/config/internal_users.yml +++ b/config/internal_users.yml @@ -20,7 +20,7 @@ admin: anomalyadmin: hash: "$2y$12$TRwAAJgnNo67w3rVUz4FIeLx9Dy/llB79zf9I15CKJ9vkM4ZzAd3." reserved: false - opendistro_security_roles: + direct_security_roles: - "anomaly_full_access" description: "Demo anomaly admin user, using internal role" diff --git a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/AbstractInternalUsersRestApiIntegrationTest.java similarity index 93% rename from src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java rename to src/integrationTest/java/org/opensearch/security/api/AbstractInternalUsersRestApiIntegrationTest.java index 18769949fe..6e385f4ce5 100644 --- a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/AbstractInternalUsersRestApiIntegrationTest.java @@ -34,7 +34,6 @@ import org.opensearch.security.dlic.rest.api.Endpoint; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.TestRestClient; -import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; @@ -43,9 +42,11 @@ import static org.opensearch.security.api.PatchPayloadHelper.addOp; import static org.opensearch.security.api.PatchPayloadHelper.patch; import static org.opensearch.security.api.PatchPayloadHelper.replaceOp; +import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.DIRECT_SECURITY_ROLES; +import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.OPENDISTRO_SECURITY_ROLES; import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.RESTRICTED_FROM_USERNAME; -public class InternalUsersRestApiIntegrationTest extends AbstractConfigEntityApiIntegrationTest { +public abstract class AbstractInternalUsersRestApiIntegrationTest extends AbstractConfigEntityApiIntegrationTest { private final static String REST_API_ADMIN_INTERNAL_USERS_ONLY = "rest_api_admin_iternal_users_only"; @@ -67,12 +68,12 @@ public class InternalUsersRestApiIntegrationTest extends AbstractConfigEntityApi ); } - public InternalUsersRestApiIntegrationTest() { + public AbstractInternalUsersRestApiIntegrationTest() { super("internalusers", new TestDescriptor() { @Override public ToXContentObject entityPayload(Boolean hidden, Boolean reserved, Boolean _static) { - return internalUser(hidden, reserved, _static, randomAsciiAlphanumOfLength(10), null, null, null); + return internalUserInitWithoutRoles(hidden, reserved, _static, randomAsciiAlphanumOfLength(10)); } @Override @@ -92,11 +93,37 @@ public Optional restAdminLimitedUser() { }); } - static ToXContentObject internalUserWithPassword(final String password) { + static ToXContentObject internalUserInitWithoutRoles( + final Boolean hidden, + final Boolean reserved, + final Boolean _static, + final String password + ) { + return (builder, params) -> { + builder.startObject(); + if (hidden != null) { + builder.field("hidden", hidden); + } + if (reserved != null) { + builder.field("reserved", reserved); + } + if (_static != null) { + builder.field("static", _static); + } + if (password == null) { + builder.field("password").nullValue(); + } else { + builder.field("password", password); + } + return builder.endObject(); + }; + } + + ToXContentObject internalUserWithPassword(final String password) { return internalUser(null, null, null, password, null, null, null); } - static ToXContentObject internalUser( + ToXContentObject internalUser( final Boolean hidden, final Boolean reserved, final String password, @@ -107,7 +134,7 @@ static ToXContentObject internalUser( return internalUser(hidden, reserved, null, password, backendRoles, attributes, securityRoles); } - static ToXContentObject internalUser( + ToXContentObject internalUser( final String password, final ToXContentObject backendRoles, final ToXContentObject attributes, @@ -116,7 +143,7 @@ static ToXContentObject internalUser( return internalUser(null, null, null, password, backendRoles, attributes, securityRoles); } - static ToXContentObject internalUser( + ToXContentObject internalUser( final Boolean hidden, final Boolean reserved, final Boolean _static, @@ -149,13 +176,15 @@ static ToXContentObject internalUser( builder.field("attributes", attributes); } if (securityRoles != null) { - builder.field("opendistro_security_roles"); + builder.field(getRoleField()); securityRoles.toXContent(builder, params); } return builder.endObject(); }; } + protected abstract String getRoleField(); + static ToXContentObject defaultServiceUser() { return serviceUser(null, null, null); // default user is disabled } @@ -413,11 +442,11 @@ void assertInternalUser( assertThat(actualObjectNode.toPrettyString(), not(actualObjectNode.has("hash"))); assertThat(actualObjectNode.toPrettyString(), actualObjectNode.get("backend_roles"), is(expectedObjectNode.get("backend_roles"))); assertThat(actualObjectNode.toPrettyString(), actualObjectNode.get("attributes"), is(expectedObjectNode.get("attributes"))); - assertThat( - actualObjectNode.toPrettyString(), - actualObjectNode.get("opendistro_security_roles"), - is(expectedObjectNode.get("opendistro_security_roles")) - ); + // can be either of OPENSEARCH_SECURITY_ROLES or OPENDISTRO_SECURITY_ROLES + JsonNode expectedRoles = expectedObjectNode.get(DIRECT_SECURITY_ROLES) != null + ? expectedObjectNode.get(DIRECT_SECURITY_ROLES) + : expectedObjectNode.get(OPENDISTRO_SECURITY_ROLES); + assertThat(actualObjectNode.toPrettyString(), actualObjectNode.get("opendistro_security_roles"), is(expectedRoles)); } String filterBy(final String value) { @@ -435,7 +464,7 @@ public void filters() throws Exception { }); } - void assertFilterByUsers(final HttpResponse response, final boolean hasServiceUser, final boolean hasInternalUser) { + void assertFilterByUsers(final TestRestClient.HttpResponse response, final boolean hasServiceUser, final boolean hasInternalUser) { assertThat(response.getBody(), response.bodyAsJsonNode().has(SERVICE_ACCOUNT_USER), is(hasServiceUser)); assertThat(response.getBody(), response.bodyAsJsonNode().has(NEW_USER), is(hasInternalUser)); } @@ -683,10 +712,10 @@ void settingOfUnknownRoleIsNotAllowed(final String predefinedUserName, final Tes public void parallelPutRequests() throws Exception { withUser(ADMIN_USER_NAME, client -> { final var userName = randomAsciiAlphanumOfLength(10); - final var httpResponses = new HttpResponse[10]; + final var httpResponses = new TestRestClient.HttpResponse[10]; try (final var executorService = Executors.newFixedThreadPool(httpResponses.length)) { - final var futures = new ArrayList>(httpResponses.length); + final var futures = new ArrayList>(httpResponses.length); for (int i = 0; i < httpResponses.length; i++) { futures.add( executorService.submit( @@ -702,7 +731,7 @@ public void parallelPutRequests() throws Exception { } } boolean created = false; - for (HttpResponse response : httpResponses) { + for (TestRestClient.HttpResponse response : httpResponses) { int sc = response.getStatusCode(); switch (sc) { case HttpStatus.SC_CREATED: diff --git a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiDirectRolesIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiDirectRolesIntegrationTest.java new file mode 100644 index 0000000000..82506d3994 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiDirectRolesIntegrationTest.java @@ -0,0 +1,21 @@ +/* + * 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.api; + +import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.DIRECT_SECURITY_ROLES; + +public class InternalUsersRestApiDirectRolesIntegrationTest extends AbstractInternalUsersRestApiIntegrationTest { + @Override + protected String getRoleField() { + return DIRECT_SECURITY_ROLES; + } +} diff --git a/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiOpenDistroRolesIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiOpenDistroRolesIntegrationTest.java new file mode 100644 index 0000000000..52b218e1bd --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiOpenDistroRolesIntegrationTest.java @@ -0,0 +1,21 @@ +/* + * 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.api; + +import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.OPENDISTRO_SECURITY_ROLES; + +public class InternalUsersRestApiOpenDistroRolesIntegrationTest extends AbstractInternalUsersRestApiIntegrationTest { + @Override + protected String getRoleField() { + return OPENDISTRO_SECURITY_ROLES; + } +} 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 f0eeb89926..3c325d22b4 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 @@ -12,12 +12,17 @@ package org.opensearch.security.dlic.rest.api; import java.io.IOException; +import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.fasterxml.jackson.databind.node.ObjectNode; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.inject.Inject; @@ -53,8 +58,17 @@ public class InternalUsersApiAction extends AbstractApiAction { + private static final Logger log = LogManager.getLogger(InternalUsersApiAction.class); private final PasswordHasher passwordHasher; + /** + * @deprecated Use direct_security_roles instead. This field will be removed in a future release. + */ + @Deprecated + public static final String OPENDISTRO_SECURITY_ROLES = "opendistro_security_roles"; + + public static final String DIRECT_SECURITY_ROLES = "direct_security_roles"; + @Override protected void consumeParameters(final RestRequest request) { request.param("name"); @@ -209,8 +223,7 @@ ValidationResult validateSecurityRoles(final SecurityConf 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; + var securityRoles = getRoles(securityJsonNode); final var rolesValid = endpointValidator.validateRoles(securityRoles, rolesConfiguration); if (!rolesValid.isValid()) { return ValidationResult.error(rolesValid.status(), rolesValid.errorMessage()); @@ -228,6 +241,37 @@ ValidationResult validateSecurityRoles(final SecurityConf ); } + /** + * This method combines roles from both 'direct_security_roles' and the deprecated 'opendistro_security_roles' fields. + * If the deprecated field is used, a warning message is logged. + * + * @param content The SecurityJsonNode containing the security configuration + * @return A List of merged unique roles. Returns an empty list if no roles are found in either field. + */ + List getRoles(SecurityJsonNode content) { + List openSearchRoles = content.get(DIRECT_SECURITY_ROLES).asList(); + List openDistroRoles = content.get(OPENDISTRO_SECURITY_ROLES).asList(); + + Set mergedRoles = new HashSet<>(); + + // Add OpenSearch roles if present + if (openSearchRoles != null) { + mergedRoles.addAll(openSearchRoles); + } + + // Add OpenDistro roles if present + if (openDistroRoles != null) { + mergedRoles.addAll(openDistroRoles); + log.warn( + "The field '{}' is deprecated and will be removed in a future release. Please use '{}' instead.", + OPENDISTRO_SECURITY_ROLES, + DIRECT_SECURITY_ROLES + ); + } + + return mergedRoles.isEmpty() ? List.of() : new ArrayList<>(mergedRoles); + } + ValidationResult createOrUpdateAccount( final RestRequest request, final SecurityConfiguration securityConfiguration @@ -335,7 +379,8 @@ public Map allowedKeys() { return allowedKeys.put("backend_roles", DataType.ARRAY) .put("attributes", DataType.OBJECT) .put("description", DataType.STRING) - .put("opendistro_security_roles", DataType.ARRAY) + .put(OPENDISTRO_SECURITY_ROLES, DataType.ARRAY) + .put(DIRECT_SECURITY_ROLES, DataType.ARRAY) .put("hash", DataType.STRING) .put("password", DataType.STRING) .build(); diff --git a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java index c427a46685..aceefed34b 100644 --- a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java +++ b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java @@ -308,6 +308,8 @@ public void unregisterDCFListener(Object listener) { private static class InternalUsersModelV7 extends InternalUsersModel { + protected final Logger log = LogManager.getLogger(InternalUsersModelV7.class); + private final SecurityDynamicConfiguration internalUserV7SecurityDynamicConfiguration; private final SecurityDynamicConfiguration rolesV7SecurityDynamicConfiguration; @@ -357,14 +359,35 @@ public String getHash(String user) { public List getSecurityRoles(String user) { InternalUserV7 tmp = internalUserV7SecurityDynamicConfiguration.getCEntry(user); + if (tmp == null) { + return ImmutableList.of(); + } + + // Log opendistro_security_roles regardless of which path we take + if (tmp.getOpendistro_security_roles() != null && !tmp.getOpendistro_security_roles().isEmpty()) { + log.warn( + "Deprecated configuration opendistro_security_roles set for: {} " + + "opendistro_security_roles will not be used in favor of direct_security_roles.", + user + ); + } + // Security roles should only contain roles that exist in the roles dynamic config. // We should filter out any roles that have hidden rolesmapping. - return tmp == null - ? ImmutableList.of() - : tmp.getOpendistro_security_roles() + if (tmp.getDirect_security_roles() != null && !tmp.getDirect_security_roles().isEmpty()) { + return tmp.getDirect_security_roles() .stream() .filter(role -> !isRolesMappingHidden(role) && rolesV7SecurityDynamicConfiguration.exists(role)) .collect(ImmutableList.toImmutableList()); + } + + if (tmp.getOpendistro_security_roles() != null && !tmp.getOpendistro_security_roles().isEmpty()) { + return tmp.getOpendistro_security_roles() + .stream() + .filter(role -> !isRolesMappingHidden(role) && rolesV7SecurityDynamicConfiguration.exists(role)) + .collect(ImmutableList.toImmutableList()); + } + return ImmutableList.of(); } // Remove any hidden rolesmapping from the security roles diff --git a/src/main/java/org/opensearch/security/securityconf/impl/v7/InternalUserV7.java b/src/main/java/org/opensearch/security/securityconf/impl/v7/InternalUserV7.java index cf8212b92f..c11bc3e77d 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/v7/InternalUserV7.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/v7/InternalUserV7.java @@ -33,6 +33,8 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.opensearch.security.securityconf.Hashed; import org.opensearch.security.securityconf.Hideable; @@ -40,6 +42,8 @@ public class InternalUserV7 implements Hideable, Hashed, StaticDefinable { + private final Logger log = LogManager.getLogger(InternalUserV7.class); + private String hash; private boolean reserved; private boolean hidden; @@ -51,6 +55,7 @@ public class InternalUserV7 implements Hideable, Hashed, StaticDefinable { private Map attributes = Collections.emptyMap(); private String description; private List opendistro_security_roles = Collections.emptyList(); + private List direct_security_roles = Collections.emptyList(); private InternalUserV7(String hash, boolean reserved, boolean hidden, List backend_roles, Map attributes) { super(); @@ -116,7 +121,18 @@ public List getOpendistro_security_roles() { } public void setOpendistro_security_roles(List opendistro_security_roles) { + log.warn("Deprecated configuration opendistro_security_roles set. Kindly use direct_security_roles instead."); this.opendistro_security_roles = opendistro_security_roles; + this.direct_security_roles = opendistro_security_roles; + } + + public List getDirect_security_roles() { + return direct_security_roles; + } + + public void setDirect_security_roles(List direct_security_roles) { + this.opendistro_security_roles = direct_security_roles; + this.direct_security_roles = direct_security_roles; } public Map getAttributes() { 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 ffe3461951..8382adbf05 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 @@ -30,6 +30,7 @@ import org.opensearch.security.securityconf.impl.v7.InternalUserV7; import org.opensearch.security.securityconf.impl.v7.RoleV7; import org.opensearch.security.support.ConfigConstants; +import org.opensearch.security.support.SecurityJsonNode; import org.opensearch.security.user.UserService; import org.opensearch.security.util.FakeRestRequest; @@ -39,10 +40,12 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.when; public class InternalUsersApiActionValidationTest extends AbstractApiActionValidationTest { @@ -187,6 +190,62 @@ public void validateSecurityRolesWithImmutableRolesMappingConfig() throws Except assertFalse(result.isValid()); } + @Test + public void testGetRolesOnlyNoRoles() throws IOException { + String jsonString = "{}"; + var content = SecurityJsonNode.fromJson(jsonString); + List result = createInternalUsersApiAction().getRoles(content); + assertEquals(0, result.size()); + } + + @Test + public void testGetRolesOnlyOpenDistroRoles() throws IOException { + String jsonString = """ + { + "opendistro_security_roles": ["opendistro_security_role_1", "opendistro_security_role_2"] + } + """; + var content = SecurityJsonNode.fromJson(jsonString); + List result = createInternalUsersApiAction().getRoles(content); + + assertEquals(2, result.size()); + assertThat(result, hasItem("opendistro_security_role_1")); + assertThat(result, hasItem("opendistro_security_role_2")); + } + + @Test + public void testGetRolesOnlyDirectRoles() throws IOException { + String jsonString = """ + { + "direct_security_roles": ["direct_security_role_1", "direct_security_role_2"] + } + """; + var content = SecurityJsonNode.fromJson(jsonString); + List result = createInternalUsersApiAction().getRoles(content); + + assertEquals(2, result.size()); + assertThat(result, hasItem("direct_security_role_1")); + assertThat(result, hasItem("direct_security_role_2")); + } + + @Test + public void testGetRolesBothSettingsForRoles() throws IOException { + String jsonString = """ + { + "opendistro_security_roles": ["opendistro_security_role_1", "opendistro_security_role_2"], + "direct_security_roles": ["direct_security_role_1", "direct_security_role_2"] + } + """; + List result = createInternalUsersApiAction().getRoles(SecurityJsonNode.fromJson(jsonString)); + + assertEquals(4, result.size()); + assertThat(result, hasItem("direct_security_role_1")); + assertThat(result, hasItem("direct_security_role_2")); + assertThat(result, hasItem("opendistro_security_role_1")); + assertThat(result, hasItem("opendistro_security_role_2")); + + } + private InternalUsersApiAction createInternalUsersApiAction() { return new InternalUsersApiAction( clusterService,