Skip to content

Commit

Permalink
Revert "Rest admin permissions (#2411) (#2466)" (#2636)
Browse files Browse the repository at this point in the history 
This reverts commit 076715d.

Signed-off-by: Andrey Pleskach <[email protected]>
  • Loading branch information
@willyborankin
willyborankin authored Apr 4, 2023
1 parent 3ad66b3 commit 80961bd
Show file tree
Hide file tree
Showing 46 changed files with 942 additions and 2,522 deletions.
15 changes: 1 addition & 14 deletions config/roles.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,7 @@ kibana_read_only:
# The security REST API access role is used to assign specific users access to change the security settings through the REST API.
security_rest_api_access:
reserved: true

security_rest_api_full_access:
reserved: true
cluster_permissions:
- 'restapi:admin/actiongroups'
- 'restapi:admin/allowlist'
- 'restapi:admin/internalusers'
- 'restapi:admin/nodesdn'
- 'restapi:admin/roles'
- 'restapi:admin/rolesmapping'
- 'restapi:admin/ssl/certs/info'
- 'restapi:admin/ssl/certs/reload'
- 'restapi:admin/tenants'


# Allows users to view monitors, destinations and alerts
alerting_read_access:
reserved: true
Expand Down
29 changes: 12 additions & 17 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@
import org.opensearch.security.ssl.OpenSearchSecuritySSLPlugin;
import org.opensearch.security.ssl.SslExceptionHandler;
import org.opensearch.security.ssl.http.netty.ValidatingDispatcher;
import org.opensearch.security.ssl.rest.SecuritySSLCertsInfoAction;
import org.opensearch.security.ssl.rest.SecuritySSLReloadCertsAction;
import org.opensearch.security.ssl.transport.DefaultPrincipalExtractor;
import org.opensearch.security.ssl.transport.SecuritySSLNettyTransport;
import org.opensearch.security.ssl.util.SSLConfigConstants;
Expand Down Expand Up @@ -464,25 +466,18 @@ public List<RestHandler> getRestHandlers(Settings settings, RestController restC
if(!SSLConfig.isSslOnlyMode()) {
handlers.add(new SecurityInfoAction(settings, restController, Objects.requireNonNull(evaluator), Objects.requireNonNull(threadPool)));
handlers.add(new SecurityHealthAction(settings, restController, Objects.requireNonNull(backendRegistry)));
handlers.add(new SecuritySSLCertsInfoAction(settings, restController, sks, Objects.requireNonNull(threadPool), Objects.requireNonNull(adminDns)));
handlers.add(new DashboardsInfoAction(settings, restController, Objects.requireNonNull(evaluator), Objects.requireNonNull(threadPool)));
handlers.add(new TenantInfoAction(settings, restController, Objects.requireNonNull(evaluator), Objects.requireNonNull(threadPool),
Objects.requireNonNull(cs), Objects.requireNonNull(adminDns), Objects.requireNonNull(cr)));
handlers.add(new SecurityConfigUpdateAction(settings, restController, Objects.requireNonNull(threadPool), adminDns, configPath, principalExtractor));
handlers.add(new SecurityWhoAmIAction(settings, restController, Objects.requireNonNull(threadPool), adminDns, configPath, principalExtractor));
handlers.addAll(
SecurityRestApiActions.getHandler(
settings,
configPath,
restController,
localClient,
adminDns,
cr, cs, principalExtractor,
evaluator,
threadPool,
Objects.requireNonNull(auditLog), sks,
sslCertReloadEnabled)
);
log.debug("Added {} rest handler(s)", handlers.size());
Objects.requireNonNull(cs), Objects.requireNonNull(adminDns), Objects.requireNonNull(cr)));
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());
}
}

Expand Down
22 changes: 6 additions & 16 deletions src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ public abstract class AbstractApiAction extends BaseRestHandler {
final ThreadPool threadPool;
protected String opendistroIndex;
private final RestApiPrivilegesEvaluator restApiPrivilegesEvaluator;
protected final RestApiAdminPrivilegesEvaluator restApiAdminPrivilegesEvaluator;
protected final AuditLog auditLog;
protected final Settings settings;
private AdminDNs adminDNs;

protected AbstractApiAction(final Settings settings, final Path configPath, final RestController controller,
final Client client, final AdminDNs adminDNs, final ConfigurationRepository cl,
Expand All @@ -88,13 +88,12 @@ protected AbstractApiAction(final Settings settings, final Path configPath, fina
this.opendistroIndex = settings.get(ConfigConstants.SECURITY_CONFIG_INDEX_NAME,
ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX);

this.adminDNs = adminDNs;
this.cl = cl;
this.cs = cs;
this.threadPool = threadPool;
this.restApiPrivilegesEvaluator = new RestApiPrivilegesEvaluator(settings, adminDNs, evaluator,
principalExtractor, configPath, threadPool);
this.restApiAdminPrivilegesEvaluator =
new RestApiAdminPrivilegesEvaluator(threadPool.getThreadContext(), evaluator, adminDNs);
this.auditLog = auditLog;
}

Expand Down Expand Up @@ -196,12 +195,7 @@ protected void handlePut(final RestChannel channel, final RestRequest request, f
}

boolean existed = existingConfiguration.exists(name);
final Object newContent = DefaultObjectMapper.readTree(content, existingConfiguration.getImplementingClass());
if (!hasPermissionsToCreate(existingConfiguration, newContent, getResourceName())) {
forbidden(channel, "No permissions");
return;
}
existingConfiguration.putCObject(name, newContent);
existingConfiguration.putCObject(name, DefaultObjectMapper.readTree(content, existingConfiguration.getImplementingClass()));

saveAnUpdateConfigs(client, request, getConfigName(), existingConfiguration, new OnSucessActionListener<IndexResponse>(channel) {

Expand All @@ -222,12 +216,6 @@ protected void handlePost(final RestChannel channel, final RestRequest request,
notImplemented(channel, Method.POST);
}

protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) throws IOException {
return false;
}

protected void handleGet(final RestChannel channel, RestRequest request, Client client, final JsonNode content)
throws IOException{

Expand Down Expand Up @@ -460,6 +448,7 @@ protected static XContentBuilder convertToJson(RestChannel channel, ToXContent t
}

protected void response(RestChannel channel, RestStatus status, String message) {

try {
final XContentBuilder builder = channel.newBuilder();
builder.startObject();
Expand Down Expand Up @@ -569,7 +558,8 @@ public String getName() {
protected abstract Endpoint getEndpoint();

protected boolean isSuperAdmin() {
return restApiAdminPrivilegesEvaluator.isCurrentUserRestApiAdminFor(getEndpoint());
User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
return adminDNs.isAdmin(user);
}

/**
Expand Down
7 changes: 0 additions & 7 deletions src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,6 @@ public AccountApiAction(Settings settings,
this.threadContext = threadPool.getThreadContext();
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) {
return true;
}

@Override
public List<Route> routes() {
return routes;
Expand Down
27 changes: 2 additions & 25 deletions src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,35 +118,12 @@ protected void handlePut(RestChannel channel, RestRequest request, Client client

// Prevent the case where action group references to itself in the allowed_actions.
final SecurityDynamicConfiguration<?> existingActionGroupsConfig = load(getConfigName(), false);
final Object actionGroup = DefaultObjectMapper.readTree(content, existingActionGroupsConfig.getImplementingClass());
existingActionGroupsConfig.putCObject(name, actionGroup);
existingActionGroupsConfig.putCObject(name, DefaultObjectMapper.readTree(content, existingActionGroupsConfig.getImplementingClass()));
if (hasActionGroupSelfReference(existingActionGroupsConfig, name)) {
badRequestResponse(channel, name + " cannot be an allowed_action of itself");
return;
}
// prevent creation of groups for REST admin api
if (restApiAdminPrivilegesEvaluator.containsRestApiAdminPermissions(actionGroup)) {
forbidden(channel, "Not allowed");
return;
}
super.handlePut(channel, request, client, content);
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfiguration,
final Object content,
final String resourceName) throws IOException {
if (restApiAdminPrivilegesEvaluator.containsRestApiAdminPermissions(content)) {
return false;
}
return true;
}

@Override
protected boolean isReadOnly(SecurityDynamicConfiguration<?> existingConfiguration, String name) {
if (restApiAdminPrivilegesEvaluator.containsRestApiAdminPermissions(existingConfiguration.getCEntry(name))) {
return true;
}
return super.isReadOnly(existingConfiguration, name);
super.handlePut(channel, request, client, content);
}
}
7 changes: 0 additions & 7 deletions src/main/java/org/opensearch/security/dlic/rest/api/AllowlistApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,6 @@ public AllowlistApiAction(final Settings settings, final Path configPath, final
super(settings, configPath, controller, client, adminDNs, cl, cs, principalExtractor, evaluator, threadPool, auditLog);
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) {
return true;
}

@Override
protected void handleApiRequest(final RestChannel channel, final RestRequest request, final Client client) throws IOException {
if (!isSuperAdmin()) {
Expand Down
7 changes: 0 additions & 7 deletions src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,6 @@ public AuditApiAction(final Settings settings,
}
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) {
return true;
}

@Override
public List<Route> routes() {
return routes;
Expand Down
8 changes: 0 additions & 8 deletions src/main/java/org/opensearch/security/dlic/rest/api/AuthTokenProcessorAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import org.opensearch.security.dlic.rest.validation.NoOpValidator;
import org.opensearch.security.privileges.PrivilegesEvaluator;
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
import org.opensearch.security.ssl.transport.PrincipalExtractor;
import org.opensearch.threadpool.ThreadPool;

Expand All @@ -54,13 +53,6 @@ public AuthTokenProcessorAction(final Settings settings, final Path configPath,
auditLog);
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) {
return true;
}

@Override
public List<Route> routes() {
return routes;
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/org/opensearch/security/dlic/rest/api/Endpoint.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,5 @@ public enum Endpoint {
VALIDATE,
WHITELIST,
ALLOWLIST,
NODESDN,
SSL;
NODESDN;
}
8 changes: 0 additions & 8 deletions src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.opensearch.security.dlic.rest.validation.NoOpValidator;
import org.opensearch.security.privileges.PrivilegesEvaluator;
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
import org.opensearch.security.ssl.transport.PrincipalExtractor;
import org.opensearch.threadpool.ThreadPool;

Expand All @@ -59,13 +58,6 @@ public FlushCacheApiAction(final Settings settings, final Path configPath, final
super(settings, configPath, controller, client, adminDNs, cl, cs, principalExtractor, evaluator, threadPool, auditLog);
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) {
return true;
}

@Override
public List<Route> routes() {
return routes;
Expand Down
7 changes: 0 additions & 7 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 @@ -78,13 +78,6 @@ public InternalUsersApiAction(final Settings settings, final Path configPath, fi
auditLog);
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) {
return true;
}

@Override
public List<Route> routes() {
return routes;
Expand Down
7 changes: 0 additions & 7 deletions src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,6 @@ protected Endpoint getEndpoint() {
return Endpoint.MIGRATE;
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) {
return true;
}

@SuppressWarnings("unchecked")
@Override
protected void handlePost(RestChannel channel, RestRequest request, Client client, final JsonNode content) throws IOException {
Expand Down
7 changes: 0 additions & 7 deletions src/main/java/org/opensearch/security/dlic/rest/api/NodesDnApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,6 @@ public NodesDnApiAction(final Settings settings, final Path configPath, final Re
this.staticNodesDnFromEsYml = settings.getAsList(ConfigConstants.SECURITY_NODES_DN, Collections.emptyList());
}

@Override
protected boolean hasPermissionsToCreate(final SecurityDynamicConfiguration<?> dynamicConfigFactory,
final Object content,
final String resourceName) {
return true;
}

@Override
public List<Route> routes() {
if (settings.getAsBoolean(ConfigConstants.SECURITY_NODES_DN_DYNAMIC_CONFIG_ENABLED, false)) {
Expand Down
21 changes: 8 additions & 13 deletions src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ public abstract class PatchableResourceApiAction extends AbstractApiAction {
protected final Logger log = LogManager.getLogger(this.getClass());

public PatchableResourceApiAction(Settings settings, Path configPath, RestController controller, Client client,
AdminDNs adminDNs, ConfigurationRepository cl, ClusterService cs,
PrincipalExtractor principalExtractor, PrivilegesEvaluator evaluator, ThreadPool threadPool,
AuditLog auditLog) {
AdminDNs adminDNs, ConfigurationRepository cl, ClusterService cs,
PrincipalExtractor principalExtractor, PrivilegesEvaluator evaluator, ThreadPool threadPool,
AuditLog auditLog) {
super(settings, configPath, controller, client, adminDNs, cl, cs, principalExtractor, evaluator, threadPool,
auditLog);
}
Expand Down Expand Up @@ -146,7 +146,7 @@ private void handleSinglePatch(RestChannel channel, RestRequest request, Client

if (!validator.validate()) {
request.params().clear();
badRequestResponse(channel, validator);
badRequestResponse(channel, validator);
return;
}

Expand All @@ -156,7 +156,7 @@ private void handleSinglePatch(RestChannel channel, RestRequest request, Client
, existingConfiguration.getVersion(), existingConfiguration.getSeqNo(), existingConfiguration.getPrimaryTerm());

if (existingConfiguration.getCType().equals(CType.ACTIONGROUPS)) {
if (hasActionGroupSelfReference(mdc, name)) {
if(hasActionGroupSelfReference(mdc, name)) {
badRequestResponse(channel, name + " cannot be an allowed_action of itself");
return;
}
Expand Down Expand Up @@ -188,6 +188,7 @@ private void handleBulkPatch(RestChannel channel, RestRequest request, Client cl
for (String resourceName : existingConfiguration.getCEntries().keySet()) {
JsonNode oldResource = existingAsObjectNode.get(resourceName);
JsonNode patchedResource = patchedAsJsonNode.get(resourceName);

if (oldResource != null && !oldResource.equals(patchedResource) && !isWriteable(channel, existingConfiguration, resourceName)) {
return;
}
Expand All @@ -205,7 +206,7 @@ private void handleBulkPatch(RestChannel channel, RestRequest request, Client cl
if(originalValidator != null) {
if (!originalValidator.validate()) {
request.params().clear();
badRequestResponse(channel, originalValidator);
badRequestResponse(channel, originalValidator);
return;
}
}
Expand All @@ -221,13 +222,7 @@ private void handleBulkPatch(RestChannel channel, RestRequest request, Client cl

if (!validator.validate()) {
request.params().clear();
badRequestResponse(channel, validator);
return;
}
final Object newContent = DefaultObjectMapper.readTree(patchedResource, existingConfiguration.getImplementingClass());
if (!hasPermissionsToCreate(existingConfiguration, newContent, resourceName)) {
request.params().clear();
forbidden(channel, "No permissions");
badRequestResponse(channel, validator);
return;
}
}
Expand Down
Loading

0 comments on commit 80961bd

Please sign in to comment.