Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rest admin permissions #2411

Merged
merged 2 commits into from
Feb 9, 2023
Merged

Rest admin permissions #2411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a772a9
Initial support of permissions for REST admin user
willyborankin Feb 6, 2023
b336e27
Tests for support of permissions for REST admin user
willyborankin Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion config/roles.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,20 @@ 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: 17 additions & 12 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,6 @@
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 @@ -466,18 +464,25 @@ 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));
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());
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());
}
}

Expand Down
22 changes: 16 additions & 6 deletions src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java
View file
Open in desktop
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,12 +88,13 @@ 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 @@ -195,7 +196,12 @@ protected void handlePut(final RestChannel channel, final RestRequest request, f
}

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

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

Expand All @@ -216,6 +222,12 @@ 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 @@ -448,7 +460,6 @@ 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 @@ -558,8 +569,7 @@ public String getName() {
protected abstract Endpoint getEndpoint();

protected boolean isSuperAdmin() {
User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation for this function should not change. Super admin is reserved in the security plugin to indicate that a user is using client cert authentication and the principal of the cert matches a configured admin_dn in opensearch.yml.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it isn't change. I just move this check here: https://github.com/willyborankin/security/blob/5110fb1fb7a673177641a6803cbc16ea56b24ec4/src/main/java/org/opensearch/security/dlic/rest/api/RestApiAdminPrivilegesEvaluator.java#L98. I think it is better to keep this code in one place instead of:

if (adminDNs.isAdmin(user.getName()) && ....)

otherwise tests wont pass for existing behave

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwperks Regarding admin and current roles. True I missed it!. I think split up roles is a good idea otherwise the end user can easily shoot their foot and cluster could be compromised. An alternative solution would be just forbid to create any role, roles mappings and action group for such permissions and config them ones before cluster up and running but I'm not sure that this is a good idea. Just to be on the same page you idea is to introduce:

all_cluster_and_indices:
  reserved: true
  hidden: false
  static: true
  description: "Allow full access to all indices and all cluster APIs"
  cluster_permissions:
    - "cluster:*"
    - "indices:*"

and assign admin by default to it right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the default configuration gives admin the permission to perform:

nodesdn
allowlist
actiongroup
user / internalusers
roles
rolesmapping
tenants

but not

reloadcerts

Only the superadmin (connected via client cert) can perform reloadcerts now.

After this change I would still expect the admin to perform the same actions without additional config.

Thank you for addressing all of the PR comments right away, this change looks good to me. Thank you for the contribution!

return adminDNs.isAdmin(user);
return restApiAdminPrivilegesEvaluator.isCurrentUserRestApiAdminFor(getEndpoint());
}

/**
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,13 @@ 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: 25 additions & 2 deletions src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,35 @@ 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);
existingActionGroupsConfig.putCObject(name, DefaultObjectMapper.readTree(content, existingActionGroupsConfig.getImplementingClass()));
final Object actionGroup = DefaultObjectMapper.readTree(content, existingActionGroupsConfig.getImplementingClass());
existingActionGroupsConfig.putCObject(name, actionGroup);
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);
}
}
7 changes: 7 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/AllowlistApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ 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: 7 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,13 @@ 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: 8 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/AuthTokenProcessorAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
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 @@ -53,6 +54,13 @@ 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: 2 additions & 1 deletion src/main/java/org/opensearch/security/dlic/rest/api/Endpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,6 @@ public enum Endpoint {
VALIDATE,
WHITELIST,
ALLOWLIST,
NODESDN;
NODESDN,
SSL;
}
8 changes: 8 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/FlushCacheApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
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 @@ -58,6 +59,13 @@ 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: 7 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ 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: 7 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@ 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: 7 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/NodesDnApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ 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: 13 additions & 8 deletions src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java
View file
Open in desktop
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,7 +188,6 @@ 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 @@ -206,7 +205,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 @@ -222,7 +221,13 @@ private void handleBulkPatch(RestChannel channel, RestRequest request, Client cl

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