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.

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

Authorize Requests bound for extensions in the REST-Layer #2601

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
e2b750d
WIP on rest layer authz
cwperks Mar 27, 2023
478523a
WIP on rest-layer authz
cwperks Mar 28, 2023
3444295
Extension handshake
cwperks Mar 28, 2023
eb03f38
Extension TLS
cwperks Mar 28, 2023
85dba00
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
3419692
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
9ef4e0f
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
b7686dd
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
298aa27
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
5b6ca7b
Merge branch 'extension-tls' into extension-role
cwperks Mar 29, 2023
e3c1b47
WIP for HelloWorld sample extension role
cwperks Mar 29, 2023
f3dadf8
Initial implementation of authz check in REST layer
cwperks Mar 29, 2023
a1826d5
Remove header
cwperks Mar 29, 2023
49fff14
Create authorizeRequest method
cwperks Apr 2, 2023
5a64ef7
Remove local check
cwperks Apr 2, 2023
62a076c
Use extensions feature flag
cwperks Apr 3, 2023
66f4d5c
Merge branch 'extension-tls' into extension-role
cwperks Apr 3, 2023
87c421b
Merge branch 'main' into extension-tls
cwperks Apr 10, 2023
e83aace
Merge branch 'extension-tls' into extension-role
cwperks Apr 10, 2023
40b9c98
Merge branch 'main' into extension-tls
cwperks Apr 22, 2023
b1f7369
Switch to values.iterator
cwperks Apr 22, 2023
e30e8be
Merge branch 'extension-tls' into extension-role
cwperks Apr 22, 2023
1ef9083
small fix
cwperks Apr 22, 2023
a740ea4
Merge branch 'main' into extension-tls
cwperks Apr 27, 2023
6cdda66
Remove ImmutableOpenMap
cwperks Apr 27, 2023
f2430ff
Merge branch 'extension-tls' into extension-role
cwperks Apr 27, 2023
364793c
Change to ProtectedRoute
cwperks Apr 28, 2023
0dae911
Merge branch 'main' into extension-role
cwperks May 5, 2023
0e9dc24
Remove extension permissions
cwperks May 8, 2023
30d7899
Use lookupExtensionSettingsById
cwperks May 9, 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
14 changes: 14 additions & 0 deletions config/roles.yml
Original file line number Diff line number Diff line change
Expand Up @@ -329,3 +329,17 @@ security_analytics_ack_alerts:
reserved: true
cluster_permissions:
- 'cluster:admin/opensearch/securityanalytics/alerts/*'

# Roles for Hello World extension, roles that extensions would like to add should be ultimately be defined elsewhere
extension_hw_greet:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to parse this from the Hello World config? I know you have made changes in the SDK, I am not sure what would be required to read in these roles as part of the registration/installation process.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is ultimately where these should go! I wanted to open up a PR to show REST-layer authz in action with an example role. This issue is about letting an extension pre-define roles and sourcing them into the security configuration.

reserved: true
extension_permissions:
- 'extension:hw/greet'

extension_hw_full:
reserved: true
extension_permissions:
- 'extension:hw/greet'
- 'extension:hw/greet_with_adjective'
- 'extension:hw/great_with_name'
- 'extension:hw/goodbye'
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@
import org.opensearch.security.http.XFFResolver;
import org.opensearch.security.privileges.PrivilegesEvaluator;
import org.opensearch.security.privileges.PrivilegesInterceptor;
import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator;
import org.opensearch.security.resolver.IndexResolverReplacer;
import org.opensearch.security.rest.DashboardsInfoAction;
import org.opensearch.security.rest.SecurityConfigUpdateAction;
Expand Down Expand Up @@ -206,6 +207,8 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
private volatile SecurityRestFilter securityRestHandler;
private volatile SecurityInterceptor si;
private volatile PrivilegesEvaluator evaluator;

private volatile RestLayerPrivilegesEvaluator restLayerEvaluator;
private volatile UserService userService;
private volatile ThreadPool threadPool;
private volatile ConfigurationRepository cr;
Expand Down Expand Up @@ -842,7 +845,10 @@ public Collection<Object> createComponents(Client localClient, ClusterService cl
principalExtractor = ReflectionHelper.instantiatePrincipalExtractor(principalExtractorClass);
}

securityRestHandler = new SecurityRestFilter(backendRegistry, auditLog, threadPool,
restLayerEvaluator = new RestLayerPrivilegesEvaluator(clusterService, threadPool, cr, auditLog,
settings, cih, namedXContentRegistry.get());

securityRestHandler = new SecurityRestFilter(backendRegistry, restLayerEvaluator, auditLog, threadPool,
principalExtractor, settings, configPath, compatConfig);

final DynamicConfigFactory dcf = new DynamicConfigFactory(cr, settings, configPath, localClient, threadPool, cih);
Expand All @@ -851,6 +857,7 @@ public Collection<Object> createComponents(Client localClient, ClusterService cl
dcf.registerDCFListener(irr);
dcf.registerDCFListener(xffResolver);
dcf.registerDCFListener(evaluator);
dcf.registerDCFListener(restLayerEvaluator);
dcf.registerDCFListener(securityRestHandler);
if (!(auditLog instanceof NullAuditLog)) {
// Don't register if advanced modules is disabled in which case auditlog is instance of NullAuditLog
Expand All @@ -876,6 +883,7 @@ public Collection<Object> createComponents(Client localClient, ClusterService cl
components.add(xffResolver);
components.add(backendRegistry);
components.add(evaluator);
components.add(restLayerEvaluator);
components.add(si);
components.add(dcf);
components.add(userService);
Expand Down
105 changes: 89 additions & 16 deletions src/main/java/org/opensearch/security/filter/SecurityRestFilter.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,32 +26,30 @@

package org.opensearch.security.filter;

import java.nio.file.Path;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import javax.net.ssl.SSLPeerUnverifiedException;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.greenrobot.eventbus.Subscribe;

import org.opensearch.OpenSearchException;
import org.opensearch.OpenSearchSecurityException;
import org.opensearch.client.node.NodeClient;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.rest.BytesRestResponse;
import org.opensearch.rest.ProtectedRoute;
import org.opensearch.rest.RestChannel;
import org.opensearch.rest.RestHandler;
import org.opensearch.rest.RestRequest;
import org.opensearch.rest.RestRequest.Method;
import org.opensearch.rest.RestStatus;
import org.opensearch.rest.extensions.RestSendToExtensionAction;
import org.opensearch.security.auditlog.AuditLog;
import org.opensearch.security.auditlog.AuditLog.Origin;
import org.opensearch.security.auth.BackendRegistry;
import org.opensearch.security.configuration.AdminDNs;
import org.opensearch.security.configuration.CompatConfig;
import org.opensearch.security.dlic.rest.api.AllowlistApiAction;
import org.opensearch.security.privileges.PrivilegesEvaluatorResponse;
import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator;
import org.opensearch.security.securityconf.impl.AllowlistingSettings;
import org.opensearch.security.securityconf.impl.WhitelistingSettings;
import org.opensearch.security.ssl.transport.PrincipalExtractor;
Expand All @@ -63,13 +61,22 @@
import org.opensearch.security.user.User;
import org.opensearch.threadpool.ThreadPool;

import javax.net.ssl.SSLPeerUnverifiedException;
import java.nio.file.Path;
import java.util.List;
import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX;
import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX;

public class SecurityRestFilter {

protected final Logger log = LogManager.getLogger(this.getClass());
private final BackendRegistry registry;

private final RestLayerPrivilegesEvaluator evaluator;
private final AuditLog auditLog;
private final ThreadContext threadContext;
private final PrincipalExtractor principalExtractor;
Expand All @@ -87,11 +94,12 @@ public class SecurityRestFilter {
private static final Pattern PATTERN_PATH_PREFIX = Pattern.compile(REGEX_PATH_PREFIX);


public SecurityRestFilter(final BackendRegistry registry, final AuditLog auditLog,
final ThreadPool threadPool, final PrincipalExtractor principalExtractor,
public SecurityRestFilter(final BackendRegistry registry, final RestLayerPrivilegesEvaluator evaluator,
final AuditLog auditLog, final ThreadPool threadPool, final PrincipalExtractor principalExtractor,
final Settings settings, final Path configPath, final CompatConfig compatConfig) {
super();
this.registry = registry;
this.evaluator = evaluator;
this.auditLog = auditLog;
this.threadContext = threadPool.getThreadContext();
this.principalExtractor = principalExtractor;
Expand All @@ -117,14 +125,16 @@ public SecurityRestFilter(final BackendRegistry registry, final AuditLog auditLo
*/
public RestHandler wrap(RestHandler original, AdminDNs adminDNs) {
return new RestHandler() {

@Override
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
org.apache.logging.log4j.ThreadContext.clearAll();
if (!checkAndAuthenticateRequest(request, channel, client)) {
User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
if (userIsSuperAdmin(user, adminDNs) || (whitelistingSettings.checkRequestIsAllowed(request, channel, client) && allowlistingSettings.checkRequestIsAllowed(request, channel, client))) {
original.handleRequest(request, channel, client);
if (!authorizeRequest(original, request, channel, user)) {
original.handleRequest(request, channel, client);
}
}
}
}
Expand All @@ -138,19 +148,56 @@ private boolean userIsSuperAdmin(User user, AdminDNs adminDNs) {
return user != null && adminDNs.isAdmin(user);
}

private boolean authorizeRequest(RestHandler original, RestRequest request, RestChannel channel, User user) throws Exception {
if (original instanceof RestSendToExtensionAction) {
List<RestHandler.Route> extensionRoutes = original.routes();
Optional<RestHandler.Route> handler = extensionRoutes.stream()
.filter(rh -> rh.getMethod().equals(request.method()))
.filter(rh -> restPathMatches(request.path(), rh.getPath()))
.findFirst();
if (handler.isPresent() && handler.get() instanceof ProtectedRoute) {
String action = ((ProtectedRoute)handler.get()).name();
PrivilegesEvaluatorResponse pres = evaluator.evaluate(user, action);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a draft so maybe you have not gotten there yet, but I thought that the evaluator only operates based on role resolution. If this is the case, what is your plan for hosting the REST routes in this mechanism? Are you making each extension add a role with the route as a permission string or are we going to add a new role used to hold these routes or something else?

if (log.isDebugEnabled()) {
log.debug(pres.toString());
}

if (pres.isAllowed()) {
// TODO make sure this is audit logged
log.debug("Request has been granted");
// auditLog.logGrantedPrivileges(action, request, task);
} else {
// auditLog.logMissingPrivileges(action, request, task);
String err;
if(!pres.getMissingSecurityRoles().isEmpty()) {
err = String.format("No mapping for %s on roles %s", user, pres.getMissingSecurityRoles());
} else {
err = String.format("no permissions for %s and %s", pres.getMissingPrivileges(), user);
}
log.debug(err);
// TODO Figure out why extension hangs intermittently after single unauthorized request
channel.sendResponse(new BytesRestResponse(RestStatus.UNAUTHORIZED, err));
return true;
}
}
}

return false;
}

private boolean checkAndAuthenticateRequest(RestRequest request, RestChannel channel,
NodeClient client) throws Exception {

threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN, Origin.REST.toString());

if(HTTPHelper.containsBadHeader(request)) {
final OpenSearchException exception = ExceptionUtils.createBadHeaderException();
log.error(exception.toString());
auditLog.logBadHeaders(request);
channel.sendResponse(new BytesRestResponse(channel, RestStatus.FORBIDDEN, exception));
return true;
}

if(SSLRequestHelper.containsBadHeader(threadContext, ConfigConstants.OPENDISTRO_SECURITY_CONFIG_PREFIX)) {
final OpenSearchException exception = ExceptionUtils.createBadHeaderException();
log.error(exception.toString());
Expand All @@ -165,7 +212,7 @@ private boolean checkAndAuthenticateRequest(RestRequest request, RestChannel cha
if(sslInfo.getPrincipal() != null) {
threadContext.putTransient("_opendistro_security_ssl_principal", sslInfo.getPrincipal());
}

if(sslInfo.getX509Certs() != null) {
threadContext.putTransient("_opendistro_security_ssl_peer_certificates", sslInfo.getX509Certs());
}
Expand All @@ -178,7 +225,7 @@ private boolean checkAndAuthenticateRequest(RestRequest request, RestChannel cha
channel.sendResponse(new BytesRestResponse(channel, RestStatus.FORBIDDEN, e));
return true;
}

if(!compatConfig.restAuthEnabled()) {
return false;
}
Expand All @@ -197,7 +244,7 @@ private boolean checkAndAuthenticateRequest(RestRequest request, RestChannel cha
org.apache.logging.log4j.ThreadContext.put("user", ((User)threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER)).getName());
}
}

return false;
}

Expand All @@ -210,4 +257,30 @@ public void onWhitelistingSettingChanged(WhitelistingSettings whitelistingSettin
public void onAllowlistingSettingChanged(AllowlistingSettings allowlistingSettings) {
this.allowlistingSettings = allowlistingSettings;
}

/**
* Determines if the request's path is a match for the configured handler path.
*
* @param requestPath The path from the {@link ProtectedRoute}
* @param handlerPath The path from the {@link RestHandler.Route}
* @return true if the request path matches the route
*/
private boolean restPathMatches(String requestPath, String handlerPath) {
// Check exact match
if (handlerPath.equals(requestPath)) {
return true;
}
// Split path to evaluate named params
String[] handlerSplit = handlerPath.split("/");
String[] requestSplit = requestPath.split("/");
if (handlerSplit.length != requestSplit.length) {
return false;
}
for (int i = 0; i < handlerSplit.length; i++) {
if (!(handlerSplit[i].equals(requestSplit[i]) || (handlerSplit[i].startsWith("{") && handlerSplit[i].endsWith("}")))) {
return false;
}
}
return true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it makes more sense to have this as a separate file or as part of the existing privileges evaluator logic? It may be possible to add this as a subroutine of the existing code if you were so inclined.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes sense as a separate file since it takes different parameters to instantiate this evaluator. This is a fresh slate to work with and much smaller in size that the transport layer PrivilegesEvaluator.

This PR doesn't cover how index permissions from plugins will be converted to extensions. i.e.

cross_cluster_replication_leader_full_access:
  reserved: true
  index_permissions:
    - index_patterns:
        - '*'
      allowed_actions:
        - "indices:admin/plugins/replication/index/setup/validate"
        - "indices:data/read/plugins/replication/changes"
        - "indices:data/read/plugins/replication/file_chunk"

will not be able to be ported with this PR alone. There will need to be work done to authorize requests that take in resource names like index patterns.

This PR will work for all plugin cluster_permissions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
* 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.privileges;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.greenrobot.eventbus.Subscribe;
import org.opensearch.OpenSearchSecurityException;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.transport.TransportAddress;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.core.xcontent.NamedXContentRegistry;
import org.opensearch.security.auditlog.AuditLog;
import org.opensearch.security.configuration.ClusterInfoHolder;
import org.opensearch.security.configuration.ConfigurationRepository;
import org.opensearch.security.securityconf.ConfigModel;
import org.opensearch.security.securityconf.DynamicConfigModel;
import org.opensearch.security.securityconf.SecurityRoles;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.user.User;
import org.opensearch.threadpool.ThreadPool;

import java.util.Set;

public class RestLayerPrivilegesEvaluator {
protected final Logger log = LogManager.getLogger(this.getClass());
private final ClusterService clusterService;

private final AuditLog auditLog;
private ThreadContext threadContext;

private final ClusterInfoHolder clusterInfoHolder;
private ConfigModel configModel;
private DynamicConfigModel dcm;
private final NamedXContentRegistry namedXContentRegistry;

public RestLayerPrivilegesEvaluator(final ClusterService clusterService, final ThreadPool threadPool,
final ConfigurationRepository configurationRepository,
AuditLog auditLog, final Settings settings, final ClusterInfoHolder clusterInfoHolder,
NamedXContentRegistry namedXContentRegistry) {

super();
this.clusterService = clusterService;
this.auditLog = auditLog;

this.threadContext = threadPool.getThreadContext();

this.clusterInfoHolder = clusterInfoHolder;
this.namedXContentRegistry = namedXContentRegistry;
}

@Subscribe
public void onConfigModelChanged(ConfigModel configModel) {
this.configModel = configModel;
}

@Subscribe
public void onDynamicConfigModelChanged(DynamicConfigModel dcm) {
this.dcm = dcm;
}

private SecurityRoles getSecurityRoles(Set<String> roles) {
return configModel.getSecurityRoles().filter(roles);
}

public boolean isInitialized() {
return configModel !=null && configModel.getSecurityRoles() != null && dcm != null;
}

public PrivilegesEvaluatorResponse evaluate(final User user, String action0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be hit further up in the call chain. For instance the plugin itself probably cannot get the request to this point if security is not initialized.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's exactly how this would work. This evaluator is used in the REST handler wrapper which means this is executed before the original handler is called. If the request is not authorized, the original handler will never be called.


if (!isInitialized()) {
throw new OpenSearchSecurityException("OpenSearch Security is not initialized.");
}

final PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse();

final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS);

Set<String> mappedRoles = mapRoles(user, caller);

presponse.resolvedSecurityRoles.addAll(mappedRoles);
final SecurityRoles securityRoles = getSecurityRoles(mappedRoles);

final boolean isDebugEnabled = log.isDebugEnabled();
if (isDebugEnabled) {
log.debug("Evaluate permissions for {} on {}", user, clusterService.localNode().getName());
log.debug("Action: {}", action0);
log.debug("Mapped roles: {}", mappedRoles.toString());
}
if(!securityRoles.impliesExtensionPermissionPermission(action0)) {
presponse.missingPrivileges.add(action0);
presponse.allowed = false;
log.info("No extension-level perm match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}", user, action0,
securityRoles.getRoleNames(), presponse.missingPrivileges);
return presponse;
} else {
if (isDebugEnabled) {
log.debug("Allowed because we have extension permissions for {}", action0);
}
presponse.allowed = true;
return presponse;
}
}

public Set<String> mapRoles(final User user, final TransportAddress caller) {
return this.configModel.mapSecurityRoles(user, caller);
}
}
Loading