diff --git a/build.gradle b/build.gradle index 9d8d61e685..5a97f38b67 100644 --- a/build.gradle +++ b/build.gradle @@ -75,12 +75,6 @@ spotless { java { // note: you can use an empty string for all the imports you didn't specify explicitly, and '\\#` prefix for static imports importOrder('java', 'javax', '', 'com.amazon', 'org.opensearch', '\\#') - targetExclude('src/integrationTest/**') - } - format("integrationTest", JavaExtension) { - target('src/integrationTest/java/**/*.java') - importOrder('java', 'javax', '', 'com.amazon', 'org.opensearch', '\\#') - indentWithTabs(4) } } diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index b972a90000..d01fd3bec5 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -178,6 +178,7 @@ import org.opensearch.security.transport.InterClusterRequestEvaluator; import org.opensearch.security.transport.SecurityInterceptor; import org.opensearch.security.user.User; +import org.opensearch.security.user.UserService; import org.opensearch.tasks.Task; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.RemoteClusterService; @@ -206,6 +207,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin private volatile SecurityRestFilter securityRestHandler; private volatile SecurityInterceptor si; private volatile PrivilegesEvaluator evaluator; + private volatile UserService userService; private volatile ThreadPool threadPool; private volatile ConfigurationRepository cr; private volatile AdminDNs adminDns; @@ -479,14 +481,20 @@ public List getRestHandlers(Settings settings, RestController restC handlers.add(new TenantInfoAction(settings, restController, Objects.requireNonNull(evaluator), Objects.requireNonNull(threadPool), Objects.requireNonNull(cs), Objects.requireNonNull(adminDns), Objects.requireNonNull(cr))); handlers.add(new TenancyConfigRestHandler()); - handlers.add(new SecurityConfigUpdateAction(settings, restController,Objects.requireNonNull(threadPool), adminDns, configPath, principalExtractor)); - handlers.add(new SecurityWhoAmIAction(settings ,restController,Objects.requireNonNull(threadPool), adminDns, configPath, principalExtractor)); - if (sslCertReloadEnabled) { - handlers.add(new SecuritySSLReloadCertsAction(settings, restController, sks, Objects.requireNonNull(threadPool), Objects.requireNonNull(adminDns))); - } - final Collection apiHandlers = SecurityRestApiActions.getHandler(settings, configPath, restController, localClient, adminDns, cr, cs, principalExtractor, evaluator, threadPool, Objects.requireNonNull(auditLog)); - handlers.addAll(apiHandlers); - log.debug("Added {} management rest handler(s)", apiHandlers.size()); + handlers.addAll( + SecurityRestApiActions.getHandler( + settings, + configPath, + restController, + localClient, + adminDns, + cr, cs, principalExtractor, + evaluator, + threadPool, + Objects.requireNonNull(auditLog), + Objects.requireNonNull(userService)) + ); + log.debug("Added {} rest handler(s)", handlers.size()); } } @@ -811,6 +819,8 @@ public Collection createComponents(Client localClient, ClusterService cl cr = ConfigurationRepository.create(settings, this.configPath, threadPool, localClient, clusterService, auditLog); + userService = new UserService(cs, cr, settings, localClient); + final XFFResolver xffResolver = new XFFResolver(threadPool); backendRegistry = new BackendRegistry(settings, adminDns, xffResolver, auditLog, threadPool); @@ -867,6 +877,7 @@ public Collection createComponents(Client localClient, ClusterService cl components.add(evaluator); components.add(si); components.add(dcf); + components.add(userService); return components; @@ -1173,7 +1184,6 @@ public static class GuiceHolder implements LifecycleComponent { private static RepositoriesService repositoriesService; private static RemoteClusterService remoteClusterService; private static IndicesService indicesService; - private static PitService pitService; @Inject 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 43b36b2de5..e0cc5d2301 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 @@ -14,7 +14,6 @@ import java.io.IOException; import java.nio.file.Path; import java.util.List; -import java.util.stream.Collectors; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ObjectNode; @@ -44,6 +43,8 @@ import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; import org.opensearch.security.ssl.transport.PrincipalExtractor; import org.opensearch.security.support.SecurityJsonNode; +import org.opensearch.security.user.UserService; +import org.opensearch.security.user.UserServiceException; import org.opensearch.threadpool.ThreadPool; import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; @@ -71,13 +72,16 @@ public class InternalUsersApiAction extends PatchableResourceApiAction { new Route(Method.PATCH, "/internalusers/{name}") )); + UserService userService; + @Inject public InternalUsersApiAction(final Settings settings, final Path configPath, final RestController controller, final Client client, final AdminDNs adminDNs, final ConfigurationRepository cl, final ClusterService cs, final PrincipalExtractor principalExtractor, final PrivilegesEvaluator evaluator, - ThreadPool threadPool, AuditLog auditLog) { + ThreadPool threadPool, UserService userService, AuditLog auditLog) { super(settings, configPath, controller, client, adminDNs, cl, cs, principalExtractor, evaluator, threadPool, auditLog); + this.userService = userService; } @Override @@ -95,22 +99,7 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C final String username = request.param("name"); - if (username == null || username.length() == 0) { - badRequestResponse(channel, "No " + getResourceName() + " specified."); - return; - } - - final List foundRestrictedContents = RESTRICTED_FROM_USERNAME.stream().filter(username::contains).collect(Collectors.toList()); - if (!foundRestrictedContents.isEmpty()) { - final String restrictedContents = foundRestrictedContents.stream().map(s -> "'" + s + "'").collect(Collectors.joining(",")); - badRequestResponse(channel, "Username has restricted characters " + restrictedContents + " that are not permitted."); - return; - } - - // TODO it might be sensible to consolidate this with the overridden method in - // order to minimize duplicated logic - - final SecurityDynamicConfiguration internalUsersConfiguration = load(getConfigName(), false); + SecurityDynamicConfiguration internalUsersConfiguration = load(getConfigName(), false); if (!isWriteable(channel, internalUsersConfiguration, username)) { return; @@ -123,22 +112,12 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C final List securityRoles = securityJsonNode.get("opendistro_security_roles").asList(); if (securityRoles != null) { for (final String role: securityRoles) { - if (!isValidRolesMapping(channel, role)) return; + if (!isValidRolesMapping(channel, role)) { + return; + } } } - // if password is set, it takes precedence over hash - final String plainTextPassword = securityJsonNode.get("password").asString(); - final String origHash = securityJsonNode.get("hash").asString(); - if (plainTextPassword != null && plainTextPassword.length() > 0) { - contentAsNode.remove("password"); - contentAsNode.put("hash", hash(plainTextPassword.toCharArray())); - } else if (origHash != null && origHash.length() > 0) { - contentAsNode.remove("password"); - } else if (plainTextPassword != null && plainTextPassword.isEmpty() && origHash == null) { - contentAsNode.remove("password"); - } - final boolean userExisted = internalUsersConfiguration.exists(username); // when updating an existing user password hash can be blank, which means no @@ -159,17 +138,8 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C return; } - - // 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); + catch (IOException ex) { + throw new IOException(ex); } // for existing users, hash is optional diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java index 54ea46efd1..af8c84f988 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java @@ -27,15 +27,25 @@ import org.opensearch.security.configuration.ConfigurationRepository; import org.opensearch.security.privileges.PrivilegesEvaluator; import org.opensearch.security.ssl.transport.PrincipalExtractor; +import org.opensearch.security.user.UserService; import org.opensearch.threadpool.ThreadPool; public class SecurityRestApiActions { - public static Collection getHandler(Settings settings, Path configPath, RestController controller, Client client, - AdminDNs adminDns, ConfigurationRepository cr, ClusterService cs, PrincipalExtractor principalExtractor, - final PrivilegesEvaluator evaluator, ThreadPool threadPool, AuditLog auditLog) { - final List handlers = new ArrayList(15); - handlers.add(new InternalUsersApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog)); + public static Collection getHandler(final Settings settings, + final Path configPath, + final RestController controller, + final Client client, + final AdminDNs adminDns, + final ConfigurationRepository cr, + final ClusterService cs, + final PrincipalExtractor principalExtractor, + final PrivilegesEvaluator evaluator, + final ThreadPool threadPool, + final AuditLog auditLog, + final UserService userService) { + final List handlers = new ArrayList(16); + handlers.add(new InternalUsersApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, userService, auditLog)); handlers.add(new RolesMappingApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog)); handlers.add(new RolesApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog)); handlers.add(new ActionGroupsApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog)); diff --git a/src/main/java/org/opensearch/security/user/UserService.java b/src/main/java/org/opensearch/security/user/UserService.java index 4a4193590e..8f7c5403c7 100644 --- a/src/main/java/org/opensearch/security/user/UserService.java +++ b/src/main/java/org/opensearch/security/user/UserService.java @@ -26,6 +26,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; + import org.opensearch.ExceptionsHelper; import org.opensearch.action.index.IndexRequest; import org.opensearch.action.support.WriteRequest; @@ -57,7 +58,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: "; @@ -114,6 +114,7 @@ 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 @@ -181,7 +182,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/main/java/org/opensearch/security/user/UserServiceException.java b/src/main/java/org/opensearch/security/user/UserServiceException.java new file mode 100644 index 0000000000..b8e6843751 --- /dev/null +++ b/src/main/java/org/opensearch/security/user/UserServiceException.java @@ -0,0 +1,20 @@ +/* + * 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; + +public class UserServiceException extends RuntimeException { + + public UserServiceException(String message) { + super(message); + } + +} diff --git a/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java b/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java index 6ede1ab81f..092c6968d9 100644 --- a/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java +++ b/src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java @@ -106,11 +106,12 @@ public void testSecurityUserInjection() throws Exception { Assert.fail("Expecting exception"); } catch (OpenSearchSecurityException ex) { exception = ex; - log.warn(ex.toString()); + log.debug(ex.toString()); Assert.assertNotNull(exception); - Assert.assertTrue(exception.getMessage().contains("indices:admin/create")); + Assert.assertTrue(exception.getMessage().toString().contains("no permissions for [indices:admin/create]")); } + // 3. with valid backend roles for injected user UserInjectorPlugin.injectedUser = "injectedadmin|injecttest"; try (Node node = new PluginAwareNode(false, tcSettings, Netty4Plugin.class, @@ -157,6 +158,5 @@ public void testSecurityUserInjectionWithConfigDisabled() throws Exception { // Should pass as the user injection is disabled Assert.assertTrue(cir.isAcknowledged()); } - } } 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 a437531e3e..d8e1d49294 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 @@ -42,7 +42,6 @@ protected String getEndpointPrefix() { return PLUGINS_PREFIX; } - final int USER_SETTING_SIZE = 56; // Lines per account entry * number of accounts private static final String ENABLED_SERVICE_ACCOUNT_BODY = "{" @@ -71,7 +70,6 @@ protected String getEndpointPrefix() { + "\"enabled\": \"true\"}" + " }\n"; - public UserApiTest(){ ENDPOINT = getEndpointPrefix() + "/api"; } @@ -174,6 +172,8 @@ private void verifyGet(final Header... header) throws Exception { response = rh.executeGetRequest(ENDPOINT + "/user", new Header[0]); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + } + // -- PUT // no username given @@ -229,6 +229,8 @@ private void verifyGet(final Header... header) throws Exception { Assert.assertTrue(settings.get(AbstractConfigurationValidator.INVALID_KEYS_KEY + ".keys").contains("some")); Assert.assertTrue(settings.get(AbstractConfigurationValidator.INVALID_KEYS_KEY + ".keys").contains("other")); + } + // -- PATCH // PATCH on non-existing resource rh.sendAdminCertificate = true; @@ -323,7 +325,6 @@ private void verifyGet(final Header... header) throws Exception { addUserWithHash("nagilum", "$2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m", HttpStatus.SC_CREATED); - // Add enabled service account then get it response = rh.executePutRequest(ENDPOINT + "/internalusers/happyServiceLive", ENABLED_SERVICE_ACCOUNT_BODY, restAdminHeader); @@ -337,6 +338,10 @@ private void verifyGet(final Header... header) throws Exception { DISABLED_SERVICE_ACCOUNT_BODY, 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", @@ -353,7 +358,6 @@ private void verifyGet(final Header... header) throws Exception { PASSWORD_HASH_SERVICE, restAdminHeader); Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); - // access must be allowed now checkGeneralAccess(HttpStatus.SC_OK, "nagilum", "nagilum"); @@ -527,7 +531,6 @@ private void verifyRoles(final boolean sendAdminCert, Header... header) throws E addUserWithPassword("$1aAAAAAAAAC", "$1aAAAAAAAAC", HttpStatus.SC_CREATED); addUserWithPassword("abc", "abc", HttpStatus.SC_CREATED); - // check tabs in json response = rh.executePutRequest(ENDPOINT + "/internalusers/userwithtabs", "\t{\"hash\": \t \"123\"\t} ", new Header[0]); Assert.assertEquals(response.getBody(), HttpStatus.SC_CREATED, response.getStatusCode()); @@ -588,7 +591,6 @@ public void testPasswordRules() throws Exception { HttpResponse response = rh .executeGetRequest("_plugins/_security/api/" + CType.INTERNALUSERS.toLCString()); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - System.out.println(response.getBody()); Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); Assert.assertEquals(USER_SETTING_SIZE, settings.size());