From 7c8fea0b2c6cfd3dae101ca44457855a06d641b8 Mon Sep 17 00:00:00 2001 From: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Date: Fri, 21 Apr 2023 13:10:13 -0400 Subject: [PATCH] Security User Refactor (#2594) --------- Signed-off-by: Stephen Crawford Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> --- .../dlic/rest/api/InternalUsersApiAction.java | 19 ------------------- .../opensearch/security/user/UserService.java | 8 +++++--- .../security/dlic/rest/api/UserApiTest.java | 8 +++++--- 3 files changed, 10 insertions(+), 25 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 0a5f25a97a..f218be0468 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 @@ -31,14 +31,12 @@ import org.opensearch.rest.RestController; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; -import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.auditlog.AuditLog; import org.opensearch.security.configuration.AdminDNs; import org.opensearch.security.configuration.ConfigurationRepository; import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator; import org.opensearch.security.dlic.rest.validation.InternalUsersValidator; import org.opensearch.security.privileges.PrivilegesEvaluator; -import org.opensearch.security.securityconf.Hashed; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; import org.opensearch.security.ssl.transport.PrincipalExtractor; @@ -146,26 +144,9 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C } catch (IOException ex) { throw new IOException(ex); - } - // for existing users, hash is optional - if (userExisted && securityJsonNode.get("hash").asString() == null) { - // sanity check, this should usually not happen - final String hash = ((Hashed) internalUsersConfiguration.getCEntry(username)).getHash(); - if (hash == null || hash.length() == 0) { - internalErrorResponse(channel, - "Existing user " + username + " has no password, and no new password or hash was specified."); - return; - } - contentAsNode.put("hash", hash); } - internalUsersConfiguration.remove(username); - - // checks complete, create or update the user - internalUsersConfiguration.putCObject(username, DefaultObjectMapper.readTree(contentAsNode, internalUsersConfiguration.getImplementingClass())); - - saveAndUpdateConfigs(this.securityIndexName,client, CType.INTERNALUSERS, internalUsersConfiguration, new OnSucessActionListener(channel) { diff --git a/src/main/java/org/opensearch/security/user/UserService.java b/src/main/java/org/opensearch/security/user/UserService.java index 738d031460..cf8a210330 100644 --- a/src/main/java/org/opensearch/security/user/UserService.java +++ b/src/main/java/org/opensearch/security/user/UserService.java @@ -57,7 +57,6 @@ public class UserService { String securityIndex; Client client; - User tokenUser; final static String NO_PASSWORD_OR_HASH_MESSAGE = "Please specify either 'hash' or 'password' when creating a new internal user."; final static String RESTRICTED_CHARACTER_USE_MESSAGE = "A restricted character(s) was detected in the account name. Please remove: "; @@ -70,6 +69,7 @@ public class UserService { final static String FAILED_ACCOUNT_RETRIEVAL_MESSAGE = "The account specified could not be accessed at this time."; final static String AUTH_TOKEN_GENERATION_MESSAGE = "An auth token could not be generated for the specified account."; private static CType getUserConfigName() { + return CType.INTERNALUSERS; } @@ -115,13 +115,16 @@ public SecurityDynamicConfiguration createOrUpdateAccount(ObjectNode contentA SecurityJsonNode securityJsonNode = new SecurityJsonNode(contentAsNode); final SecurityDynamicConfiguration internalUsersConfiguration = load(getUserConfigName(), false); + String accountName = securityJsonNode.get("name").asString(); if (accountName == null || accountName.length() == 0) { // Fail if field is present but empty throw new UserServiceException(NO_ACCOUNT_NAME_MESSAGE); } - if (!securityJsonNode.get("attributes").get("owner").isNull() && !securityJsonNode.get("attributes").get("owner").asString().equals(accountName)) { // If this is a service account + + if (!securityJsonNode.get("attributes").get("owner").isNull() && !securityJsonNode.get("attributes").get("owner").equals(accountName)) { // If this is a service account + verifyServiceAccount(securityJsonNode, accountName); String password = generatePassword(); contentAsNode.put("hash", hash(password.toCharArray())); @@ -172,7 +175,6 @@ public SecurityDynamicConfiguration createOrUpdateAccount(ObjectNode contentA private void verifyServiceAccount(SecurityJsonNode securityJsonNode, String accountName) { - final String plainTextPassword = securityJsonNode.get("password").asString(); final String origHash = securityJsonNode.get("hash").asString(); 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 a22e37856e..35d7d2bb74 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 @@ -328,9 +328,6 @@ private void verifyPatch(final boolean sendAdminCert, Header... restAdminHeader) response = rh.executePutRequest(ENDPOINT + "/internalusers/happyServiceLive", ENABLED_SERVICE_ACCOUNT_BODY, restAdminHeader); Assert.assertEquals(response.getBody(), HttpStatus.SC_CREATED, response.getStatusCode()); - rh.sendAdminCertificate = sendAdminCert; - response = rh.executeGetRequest(ENDPOINT + "/internalusers/happyServiceLive", restAdminHeader); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); // Add disabled service account @@ -339,6 +336,11 @@ private void verifyPatch(final boolean sendAdminCert, Header... restAdminHeader) Assert.assertEquals(response.getBody(), HttpStatus.SC_CREATED, response.getStatusCode()); + // Add enabled non-service account + response = rh.executePutRequest(ENDPOINT + "/internalusers/user_is_owner_1", + ENABLED_NOT_SERVICE_ACCOUNT_BODY, restAdminHeader); + Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); + // Add service account with password -- Should Fail response = rh.executePutRequest(ENDPOINT + "/internalusers/passwordService", PASSWORD_SERVICE, restAdminHeader);