Skip to content

Commit 24e08bd

Browse files
stephen-crawfordstephen-crawford
authored
Security User Refactor (#2594)
 --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com>
1 parent d168c1a commit 24e08bd

File tree

8 files changed

+306
-64
lines changed

8 files changed

+306
-64
lines changed

‎src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@
176176
import org.opensearch.security.transport.InterClusterRequestEvaluator;
177177
import org.opensearch.security.transport.SecurityInterceptor;
178178
import org.opensearch.security.user.User;
179+
import org.opensearch.security.user.UserService;
179180
import org.opensearch.tasks.Task;
180181
import org.opensearch.threadpool.ThreadPool;
181182
import org.opensearch.transport.RemoteClusterService;
@@ -204,6 +205,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
204205
private volatile SecurityRestFilter securityRestHandler;
205206
private volatile SecurityInterceptor si;
206207
private volatile PrivilegesEvaluator evaluator;
208+
private volatile UserService userService;
207209
private volatile ThreadPool threadPool;
208210
private volatile ConfigurationRepository cr;
209211
private volatile AdminDNs adminDns;
@@ -487,6 +489,7 @@ public List<RestHandler> getRestHandlers(Settings settings, RestController restC
487489
evaluator,
488490
threadPool,
489491
Objects.requireNonNull(auditLog), sks,
492+
Objects.requireNonNull(userService),
490493
sslCertReloadEnabled)
491494
);
492495
log.debug("Added {} rest handler(s)", handlers.size());
@@ -813,9 +816,11 @@ public Collection<Object> createComponents(Client localClient, ClusterService cl
813816
sslExceptionHandler = new AuditLogSslExceptionHandler(auditLog);
814817

815818
adminDns = new AdminDNs(settings);
816-
819+
817820
cr = ConfigurationRepository.create(settings, this.configPath, threadPool, localClient, clusterService, auditLog);
818821

822+
userService = new UserService(cs, cr, settings, localClient);
823+
819824
final XFFResolver xffResolver = new XFFResolver(threadPool);
820825
backendRegistry = new BackendRegistry(settings, adminDns, xffResolver, auditLog, threadPool);
821826

@@ -872,6 +877,7 @@ public Collection<Object> createComponents(Client localClient, ClusterService cl
872877
components.add(evaluator);
873878
components.add(si);
874879
components.add(dcf);
880+
components.add(userService);
875881

876882

877883
return components;
@@ -1179,7 +1185,6 @@ public static class GuiceHolder implements LifecycleComponent {
11791185
private static RepositoriesService repositoriesService;
11801186
private static RemoteClusterService remoteClusterService;
11811187
private static IndicesService indicesService;
1182-
11831188
private static PitService pitService;
11841189

11851190
@Inject
@@ -1204,7 +1209,8 @@ public static IndicesService getIndicesService() {
12041209
}
12051210

12061211
public static PitService getPitService() { return pitService; }
1207-
1212+
1213+
12081214
@Override
12091215
public void close() {
12101216
}

‎src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java‎

Lines changed: 24 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
import java.io.IOException;
1515
import java.nio.file.Path;
1616
import java.util.List;
17-
import java.util.stream.Collectors;
1817

1918
import com.fasterxml.jackson.databind.JsonNode;
2019
import com.fasterxml.jackson.databind.node.ObjectNode;
@@ -32,18 +31,18 @@
3231
import org.opensearch.rest.RestController;
3332
import org.opensearch.rest.RestRequest;
3433
import org.opensearch.rest.RestRequest.Method;
35-
import org.opensearch.security.DefaultObjectMapper;
3634
import org.opensearch.security.auditlog.AuditLog;
3735
import org.opensearch.security.configuration.AdminDNs;
3836
import org.opensearch.security.configuration.ConfigurationRepository;
3937
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator;
4038
import org.opensearch.security.dlic.rest.validation.InternalUsersValidator;
4139
import org.opensearch.security.privileges.PrivilegesEvaluator;
42-
import org.opensearch.security.securityconf.Hashed;
4340
import org.opensearch.security.securityconf.impl.CType;
4441
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
4542
import org.opensearch.security.ssl.transport.PrincipalExtractor;
4643
import org.opensearch.security.support.SecurityJsonNode;
44+
import org.opensearch.security.user.UserService;
45+
import org.opensearch.security.user.UserServiceException;
4746
import org.opensearch.threadpool.ThreadPool;
4847

4948
import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
@@ -69,13 +68,16 @@ public class InternalUsersApiAction extends PatchableResourceApiAction {
6968
new Route(Method.PATCH, "/internalusers/{name}")
7069
));
7170

71+
UserService userService;
72+
7273
@Inject
7374
public InternalUsersApiAction(final Settings settings, final Path configPath, final RestController controller,
7475
final Client client, final AdminDNs adminDNs, final ConfigurationRepository cl,
7576
final ClusterService cs, final PrincipalExtractor principalExtractor, final PrivilegesEvaluator evaluator,
76-
ThreadPool threadPool, AuditLog auditLog) {
77+
ThreadPool threadPool, UserService userService, AuditLog auditLog) {
7778
super(settings, configPath, controller, client, adminDNs, cl, cs, principalExtractor, evaluator, threadPool,
7879
auditLog);
80+
this.userService = userService;
7981
}
8082

8183
@Override
@@ -100,22 +102,7 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
100102

101103
final String username = request.param("name");
102104

103-
if (username == null || username.length() == 0) {
104-
badRequestResponse(channel, "No " + getResourceName() + " specified.");
105-
return;
106-
}
107-
108-
final List<String> foundRestrictedContents = RESTRICTED_FROM_USERNAME.stream().filter(username::contains).collect(Collectors.toList());
109-
if (!foundRestrictedContents.isEmpty()) {
110-
final String restrictedContents = foundRestrictedContents.stream().map(s -> "'" + s + "'").collect(Collectors.joining(","));
111-
badRequestResponse(channel, "Username has restricted characters " + restrictedContents + " that are not permitted.");
112-
return;
113-
}
114-
115-
// TODO it might be sensible to consolidate this with the overridden method in
116-
// order to minimize duplicated logic
117-
118-
final SecurityDynamicConfiguration<?> internalUsersConfiguration = load(getConfigName(), false);
105+
SecurityDynamicConfiguration<?> internalUsersConfiguration = load(getConfigName(), false);
119106

120107
if (!isWriteable(channel, internalUsersConfiguration, username)) {
121108
return;
@@ -128,50 +115,35 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
128115
final List<String> securityRoles = securityJsonNode.get("opendistro_security_roles").asList();
129116
if (securityRoles != null) {
130117
for (final String role: securityRoles) {
131-
if (!isValidRolesMapping(channel, role)) return;
118+
if (!isValidRolesMapping(channel, role)) {
119+
return;
120+
}
132121
}
133122
}
134123

135-
// if password is set, it takes precedence over hash
136-
final String plainTextPassword = securityJsonNode.get("password").asString();
137-
final String origHash = securityJsonNode.get("hash").asString();
138-
if (plainTextPassword != null && plainTextPassword.length() > 0) {
139-
contentAsNode.remove("password");
140-
contentAsNode.put("hash", hash(plainTextPassword.toCharArray()));
141-
} else if (origHash != null && origHash.length() > 0) {
142-
contentAsNode.remove("password");
143-
} else if (plainTextPassword != null && plainTextPassword.isEmpty() && origHash == null) {
144-
contentAsNode.remove("password");
145-
}
146-
147124
final boolean userExisted = internalUsersConfiguration.exists(username);
148125

149126
// when updating an existing user password hash can be blank, which means no
150127
// changes
151128

152-
// sanity checks, hash is mandatory for newly created users
153-
if (!userExisted && securityJsonNode.get("hash").asString() == null) {
154-
badRequestResponse(channel, "Please specify either 'hash' or 'password' when creating a new internal user.");
129+
try {
130+
if (request.hasParam("owner")) {
131+
((ObjectNode) content).put("owner", request.param("owner"));
132+
}
133+
if (request.hasParam("isEnabled")) {
134+
((ObjectNode) content).put("isEnabled", request.param("isEnabled"));
135+
}
136+
((ObjectNode) content).put("name", username);
137+
internalUsersConfiguration = userService.createOrUpdateAccount((ObjectNode) content);
138+
}
139+
catch (UserServiceException ex) {
140+
badRequestResponse(channel, ex.getMessage());
155141
return;
156142
}
157-
158-
// for existing users, hash is optional
159-
if (userExisted && securityJsonNode.get("hash").asString() == null) {
160-
// sanity check, this should usually not happen
161-
final String hash = ((Hashed) internalUsersConfiguration.getCEntry(username)).getHash();
162-
if (hash == null || hash.length() == 0) {
163-
internalErrorResponse(channel,
164-
"Existing user " + username + " has no password, and no new password or hash was specified.");
165-
return;
166-
}
167-
contentAsNode.put("hash", hash);
143+
catch (IOException ex) {
144+
throw new IOException(ex);
168145
}
169146

170-
internalUsersConfiguration.remove(username);
171-
172-
// checks complete, create or update the user
173-
internalUsersConfiguration.putCObject(username, DefaultObjectMapper.readTree(contentAsNode, internalUsersConfiguration.getImplementingClass()));
174-
175147
saveAndUpdateConfigs(this.securityIndexName,client, CType.INTERNALUSERS, internalUsersConfiguration, new OnSucessActionListener<IndexResponse>(channel) {
176148

177149
@Override

‎src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.opensearch.security.privileges.PrivilegesEvaluator;
2929
import org.opensearch.security.ssl.SecurityKeyStore;
3030
import org.opensearch.security.ssl.transport.PrincipalExtractor;
31+
import org.opensearch.security.user.UserService;
3132
import org.opensearch.threadpool.ThreadPool;
3233

3334
public class SecurityRestApiActions {
@@ -44,9 +45,10 @@ public static Collection<RestHandler> getHandler(final Settings settings,
4445
final ThreadPool threadPool,
4546
final AuditLog auditLog,
4647
final SecurityKeyStore securityKeyStore,
48+
final UserService userService,
4749
final boolean certificatesReloadEnabled) {
4850
final List<RestHandler> handlers = new ArrayList<RestHandler>(16);
49-
handlers.add(new InternalUsersApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
51+
handlers.add(new InternalUsersApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, userService, auditLog));
5052
handlers.add(new RolesMappingApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
5153
handlers.add(new RolesApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));
5254
handlers.add(new ActionGroupsApiAction(settings, configPath, controller, client, adminDns, cr, cs, principalExtractor, evaluator, threadPool, auditLog));

0 commit comments

Comments
 (0)