Skip to content

Commit

Permalink
Security User Refactor (opensearch-project#2594)
Browse files Browse the repository at this point in the history 
---------

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com>
  • Loading branch information
@stephen-crawford
stephen-crawford committed May 3, 2023
1 parent 5cd57ae commit 47397eb
Show file tree
Hide file tree
Showing 8 changed files with 79 additions and 73 deletions.
6 changes: 0 additions & 6 deletions build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down
28 changes: 19 additions & 9 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -479,14 +481,20 @@ public List<RestHandler> 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<RestHandler> 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());
}
}

Expand Down Expand Up @@ -811,6 +819,8 @@ public Collection<Object> 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);

Expand Down Expand Up @@ -867,6 +877,7 @@ public Collection<Object> createComponents(Client localClient, ClusterService cl
components.add(evaluator);
components.add(si);
components.add(dcf);
components.add(userService);


return components;
Expand Down Expand Up @@ -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
Expand Down
54 changes: 12 additions & 42 deletions src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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<String> 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;
Expand All @@ -123,22 +112,12 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
final List<String> 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
Expand All @@ -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
Expand Down
20 changes: 15 additions & 5 deletions src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<RestHandler> 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<RestHandler> handlers = new ArrayList<RestHandler>(15);
handlers.add(new InternalUsersApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
public static Collection<RestHandler> 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<RestHandler> handlers = new ArrayList<RestHandler>(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));
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/opensearch/security/user/UserService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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: ";

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand Down
20 changes: 20 additions & 0 deletions src/main/java/org/opensearch/security/user/UserServiceException.java
View file
Original file line number Diff line number Diff line change
@@ -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);
}

}
6 changes: 3 additions & 3 deletions src/test/java/org/opensearch/security/TransportUserInjectorIntegTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -157,6 +158,5 @@ public void testSecurityUserInjectionWithConfigDisabled() throws Exception {
// Should pass as the user injection is disabled
Assert.assertTrue(cir.isAcknowledged());
}

}
}
14 changes: 8 additions & 6 deletions src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "{"
Expand Down Expand Up @@ -71,7 +70,6 @@ protected String getEndpointPrefix() {
+ "\"enabled\": \"true\"}"
+ " }\n";


public UserApiTest(){
ENDPOINT = getEndpointPrefix() + "/api";
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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",
Expand All @@ -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");

Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());

Expand Down

0 comments on commit 47397eb

Please sign in to comment.