From 7f6944c0de7f8a72bb72f2248153432614aaba57 Mon Sep 17 00:00:00 2001 From: Sam <128482925+samuelcostae@users.noreply.github.com> Date: Fri, 22 Sep 2023 22:35:04 +0100 Subject: [PATCH] Add backend filtering for legacy internal user and service accounts (#2786) ### Description Update UserAPIAction to Filter internal accounts and Service accounts. Dashboards Plugin changes via PR https://github.com/opensearch-project/security-dashboards-plugin/pull/1502 ### Issues Resolved - Resolves #2704 ### Testing Unit Tests created ### Check List - [x] New functionality includes testing - [ ] New functionality has been documented - [x] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: scosta Signed-off-by: Ryan Liang Signed-off-by: Sam <128482925+samuelcostae@users.noreply.github.com> Signed-off-by: Sam Co-authored-by: Ryan Liang Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> --- .../dlic/rest/api/InternalUsersApiAction.java | 35 +++++- .../impl/SecurityDynamicConfiguration.java | 5 + .../security/user/UserFilterType.java | 41 +++++++ .../opensearch/security/user/UserService.java | 44 ++++++++ .../security/UserServiceUnitTests.java | 100 ++++++++++++++++++ .../security/dlic/rest/api/UserApiTest.java | 54 +++++++++- src/test/resources/internal_users.yml | 2 +- 7 files changed, 276 insertions(+), 5 deletions(-) create mode 100644 src/main/java/org/opensearch/security/user/UserFilterType.java create mode 100644 src/test/java/org/opensearch/security/UserServiceUnitTests.java 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 7b9a832a21..253471e6c5 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 @@ -28,7 +28,9 @@ import org.opensearch.security.dlic.rest.validation.ValidationResult; import org.opensearch.security.securityconf.Hashed; import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; import org.opensearch.security.support.SecurityJsonNode; +import org.opensearch.security.user.UserFilterType; import org.opensearch.security.user.UserService; import org.opensearch.security.user.UserServiceException; import org.opensearch.threadpool.ThreadPool; @@ -48,6 +50,12 @@ public class InternalUsersApiAction extends AbstractApiAction { + @Override + protected void consumeParameters(final RestRequest request) { + request.param("name"); + request.param("filterBy"); + } + static final List RESTRICTED_FROM_USERNAME = ImmutableList.of( ":" // Not allowed in basic auth, see https://stackoverflow.com/a/33391003/533057 ); @@ -96,7 +104,13 @@ protected CType getConfigType() { } private void internalUsersApiRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) { - requestHandlersBuilder + requestHandlersBuilder.onGetRequest( + request -> ValidationResult.success(request).map(this::processGetRequest).map(securityConfiguration -> { + final var configuration = securityConfiguration.configuration(); + filterUsers(configuration, filterParam(request)); + return ValidationResult.success(securityConfiguration); + }) + ) // Overrides the GET request functionality to allow for the special case of requesting an auth token. .override( Method.POST, @@ -121,6 +135,21 @@ private void internalUsersApiRequestHandlers(RequestHandler.RequestHandlersBuild .map(this::validateAndUpdatePassword) .map(this::addEntityToConfig) ); + + } + + protected final ValidationResult filterUsers(SecurityDynamicConfiguration users, UserFilterType userType) { + userService.includeAccountsIfType(users, userType); + return ValidationResult.success(SecurityConfiguration.of(users.getCType().toString(), users)); + + } + + protected final UserFilterType filterParam(final RestRequest request) { + final String filter = request.param("filterBy"); + if (Strings.isNullOrEmpty(filter)) { + return UserFilterType.ANY; + } + return UserFilterType.fromString(filter); } ValidationResult withAuthTokenPath(final RestRequest request) throws IOException { @@ -179,8 +208,8 @@ ValidationResult createOrUpdateAccount( try { final var username = securityConfiguration.entityName(); final var content = (ObjectNode) securityConfiguration.requestContent(); - if (request.hasParam("service")) { - content.put("service", request.param("service")); + if (request.hasParam("attributes")) { + content.put("attributes", request.param("attributes")); } if (request.hasParam("enabled")) { content.put("enabled", request.param("enabled")); diff --git a/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java b/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java index 4d10cb8ad6..4623a2317b 100644 --- a/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java +++ b/src/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java @@ -30,6 +30,7 @@ import java.io.IOException; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -285,7 +286,11 @@ public SecurityDynamicConfiguration deepClone() { @JsonIgnore public void remove(String key) { centries.remove(key); + } + @JsonIgnore + public void remove(List keySet) { + keySet.stream().forEach(this::remove); } @SuppressWarnings({ "rawtypes", "unchecked" }) diff --git a/src/main/java/org/opensearch/security/user/UserFilterType.java b/src/main/java/org/opensearch/security/user/UserFilterType.java new file mode 100644 index 0000000000..e13622e246 --- /dev/null +++ b/src/main/java/org/opensearch/security/user/UserFilterType.java @@ -0,0 +1,41 @@ +/* + * 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.user; + +/** + * Filter types to be used when requesting the list of users. + * 'Service' refers to accounts used by other services like Dashboards + * 'Internal' refers the standard user accounts + * 'Any' refers to both types of accounts + */ +public enum UserFilterType { + + ANY("any"), + INTERNAL("internal"), + SERVICE("service"); + + private String name; + + UserFilterType(String name) { + this.name = name; + } + + public static UserFilterType fromString(String name) { + for (UserFilterType b : UserFilterType.values()) { + if (b.name.equalsIgnoreCase(name)) { + return b; + } + } + return UserFilterType.ANY; + } + +} diff --git a/src/main/java/org/opensearch/security/user/UserService.java b/src/main/java/org/opensearch/security/user/UserService.java index bf2e3e0273..1435d6e531 100644 --- a/src/main/java/org/opensearch/security/user/UserService.java +++ b/src/main/java/org/opensearch/security/user/UserService.java @@ -13,10 +13,12 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Random; import java.util.stream.Collectors; @@ -44,6 +46,7 @@ import org.opensearch.security.securityconf.Hashed; 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.support.ConfigConstants; import org.opensearch.security.support.SecurityJsonNode; import org.passay.CharacterRule; @@ -314,4 +317,45 @@ public static void saveAndUpdateConfigs( throw ExceptionsHelper.convertToOpenSearchException(e); } } + + /** + * Removes accounts that are not of the requested type from the SecurityDynamicConfiguration object passed. + * + * Accounts with the 'service' attribute set to true, are considered of type 'service'. + * Accounts with the 'service' attribute set to false or without the 'service' attribute, are considered of type 'internal'. + * + * @param configuration SecurityDynamicConfiguration object containing all accounts + * @param requestedAccountType The type of account to be kept. Should be "service" or "internal" + * + */ + public void includeAccountsIfType(SecurityDynamicConfiguration configuration, UserFilterType requestedAccountType) { + if (requestedAccountType != UserFilterType.INTERNAL && requestedAccountType != UserFilterType.SERVICE) { + return; + } + List toBeRemoved = new ArrayList<>(); + + if (requestedAccountType == UserFilterType.SERVICE) { + accountsToRemoveFromConfiguration(configuration, toBeRemoved, false); + } else if (requestedAccountType == UserFilterType.INTERNAL) { + accountsToRemoveFromConfiguration(configuration, toBeRemoved, true); + } + configuration.remove(toBeRemoved); + } + + private void accountsToRemoveFromConfiguration( + SecurityDynamicConfiguration configuration, + List toBeRemoved, + boolean isServiceAccountRequested + ) { + for (Map.Entry entry : configuration.getCEntries().entrySet()) { + final InternalUserV7 internalUserEntry = (InternalUserV7) entry.getValue(); + final Map accountAttributes = internalUserEntry.getAttributes(); + final String accountName = entry.getKey(); + final boolean isServiceAccount = Boolean.parseBoolean(accountAttributes.getOrDefault("service", "false").toString()); + + if (isServiceAccount == isServiceAccountRequested) { + toBeRemoved.add(accountName); + } + } + } } diff --git a/src/test/java/org/opensearch/security/UserServiceUnitTests.java b/src/test/java/org/opensearch/security/UserServiceUnitTests.java new file mode 100644 index 0000000000..533a4c7366 --- /dev/null +++ b/src/test/java/org/opensearch/security/UserServiceUnitTests.java @@ -0,0 +1,100 @@ +/* + * 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; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; +import org.opensearch.client.Client; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Settings; +import org.opensearch.security.configuration.ConfigurationRepository; +import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; +import org.opensearch.security.user.UserFilterType; +import org.opensearch.security.user.UserService; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; + +import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; + +public class UserServiceUnitTests { + SecurityDynamicConfiguration config; + @Mock + ClusterService clusterService; + @Mock + ConfigurationRepository configurationRepository; + @Mock + Client client; + UserService userService; + + final int SERVICE_ACCOUNTS_IN_SETTINGS = 1; + final int INTERNAL_ACCOUNTS_IN_SETTINGS = 66; + String serviceAccountUsername = "bug.99"; + String internalAccountUsername = "sarek"; + + @Before + public void setup() throws Exception { + String usersYmlFile = "./internal_users.yml"; + Settings.Builder builder = Settings.builder(); + userService = new UserService(clusterService, configurationRepository, builder.build(), client); + config = readConfigFromYml(usersYmlFile, CType.INTERNALUSERS); + } + + @Test + public void testServiceUserTypeFilter() { + + userService.includeAccountsIfType(config, UserFilterType.SERVICE); + Assert.assertEquals(SERVICE_ACCOUNTS_IN_SETTINGS, config.getCEntries().size()); + Assert.assertEquals(config.getCEntries().containsKey(serviceAccountUsername), true); + Assert.assertEquals(config.getCEntries().containsKey(internalAccountUsername), false); + + } + + @Test + public void testInternalUserTypeFilter() { + userService.includeAccountsIfType(config, UserFilterType.INTERNAL); + Assert.assertEquals(INTERNAL_ACCOUNTS_IN_SETTINGS, config.getCEntries().size()); + Assert.assertEquals(config.getCEntries().containsKey(serviceAccountUsername), false); + Assert.assertEquals(config.getCEntries().containsKey(internalAccountUsername), true); + + } + + @Test + public void testAnyUserTypeFilter() { + userService.includeAccountsIfType(config, UserFilterType.ANY); + Assert.assertEquals(INTERNAL_ACCOUNTS_IN_SETTINGS + SERVICE_ACCOUNTS_IN_SETTINGS, config.getCEntries().size()); + Assert.assertEquals(config.getCEntries().containsKey(serviceAccountUsername), true); + Assert.assertEquals(config.getCEntries().containsKey(internalAccountUsername), true); + } + + private SecurityDynamicConfiguration readConfigFromYml(String file, CType cType) throws Exception { + final ObjectMapper YAML = new ObjectMapper(new YAMLFactory()); + final String TEST_RESOURCE_RELATIVE_PATH = "../../resources/test/"; + + final String adjustedFilePath = TEST_RESOURCE_RELATIVE_PATH + file; + JsonNode jsonNode = YAML.readTree(Files.readString(new File(adjustedFilePath).toPath(), StandardCharsets.UTF_8)); + int configVersion = 1; + + if (jsonNode.get("_meta") != null) { + Assert.assertEquals(jsonNode.get("_meta").get("type").asText(), cType.toLCString()); + configVersion = jsonNode.get("_meta").get("config_version").asInt(); + } + return SecurityDynamicConfiguration.fromNode(jsonNode, cType, configVersion, 0, 0); + } + +} diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java index 754e555fdf..3b959b4d26 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java @@ -15,14 +15,17 @@ import java.util.Base64; import java.util.List; +import com.fasterxml.jackson.databind.JsonNode; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.message.BasicHeader; +import org.hamcrest.Matchers; import org.junit.Assert; import org.junit.Test; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.dlic.rest.validation.PasswordValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; import org.opensearch.security.securityconf.impl.CType; @@ -55,7 +58,7 @@ protected String getEndpointPrefix() { private static final String ENABLED_SERVICE_ACCOUNT_BODY = "{" + " \"attributes\": { \"service\": \"true\", " - + "\"enabled\": \"true\"}" + + " \"enabled \": \"true\"}" + " }\n"; private static final String DISABLED_SERVICE_ACCOUNT_BODY = "{" @@ -144,6 +147,55 @@ public void testParallelPutRequests() throws Exception { deleteUser("test1"); } + @Test + public void testUserFilters() throws Exception { + setup(); + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = true; + final int SERVICE_ACCOUNTS_IN_SETTINGS = 1; + final int INTERNAL_ACCOUNTS_IN_SETTINGS = 19; + final String serviceAccountName = "JohnDoeService"; + HttpResponse response; + + response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=internal"); + + Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); + JsonNode list = DefaultObjectMapper.readTree(response.getBody()); + Assert.assertEquals(INTERNAL_ACCOUNTS_IN_SETTINGS, list.size()); + + response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=service"); + Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); + list = DefaultObjectMapper.readTree(response.getBody()); + assertThat(list, Matchers.emptyIterable()); + + response = rh.executePutRequest(ENDPOINT + "/internalusers/" + serviceAccountName, ENABLED_SERVICE_ACCOUNT_BODY); + + // repeat assertions after adding the service account + + response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=internal"); + + Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); + list = DefaultObjectMapper.readTree(response.getBody()); + Assert.assertEquals(INTERNAL_ACCOUNTS_IN_SETTINGS, list.size()); + + response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=service"); + Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); + list = DefaultObjectMapper.readTree(response.getBody()); + Assert.assertEquals(SERVICE_ACCOUNTS_IN_SETTINGS, list.size()); + assertThat(response.findValueInJson(serviceAccountName + ".attributes.service"), containsString("true")); + + response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=ssas"); + Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); + list = DefaultObjectMapper.readTree(response.getBody()); + Assert.assertEquals(SERVICE_ACCOUNTS_IN_SETTINGS + INTERNAL_ACCOUNTS_IN_SETTINGS, list.size()); + + response = rh.executeGetRequest(ENDPOINT + "/internalusers?wrongparameter=jhondoe"); + Assert.assertEquals(response.getBody(), HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + response = rh.executePutRequest(ENDPOINT + "/internalusers", "{sample:value"); + Assert.assertEquals(response.getBody(), HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); + } + @Test public void testUserApi() throws Exception { diff --git a/src/test/resources/internal_users.yml b/src/test/resources/internal_users.yml index 62bf83ae58..b078d6920f 100644 --- a/src/test/resources/internal_users.yml +++ b/src/test/resources/internal_users.yml @@ -7,7 +7,7 @@ bug.99: reserved: false hidden: false backend_roles: [] - attributes: {} + attributes: {service : "true"} description: "Migrated from v6" user_c: hash: "$2a$04$jQcEXpODnTFoGDuA7DPdSevA84CuH/7MOYkb80M3XZIrH76YMWS9G"