-
Notifications
You must be signed in to change notification settings - Fork 282
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
Changes from all commits
e2b750d
478523a
3444295
eb03f38
85dba00
3419692
9ef4e0f
b7686dd
298aa27
5b6ca7b
e3c1b47
f3dadf8
a1826d5
49fff14
5a64ef7
62a076c
66f4d5c
87c421b
e83aace
40b9c98
b1f7369
e30e8be
1ef9083
a740ea4
6cdda66
f2430ff
364793c
0dae911
0e9dc24
30d7899
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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); | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
|
@@ -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()); | ||
} | ||
|
@@ -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; | ||
} | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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; | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.impliesClusterPermissionPermission(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); | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.