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 rest requests #2753

Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
a8a1660
WIP on rest layer authz
cwperks Mar 27, 2023
0fedc03
WIP on rest-layer authz
cwperks Mar 28, 2023
1793a5b
Extension handshake
cwperks Mar 28, 2023
32d8763
Extension TLS
cwperks Mar 28, 2023
a5a44b8
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
99bfd94
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
f62f590
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
7596b7e
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
1fb5316
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
25fda77
WIP for HelloWorld sample extension role
cwperks Mar 29, 2023
aea8ede
Initial implementation of authz check in REST layer
cwperks Mar 29, 2023
15eb6b9
Remove header
cwperks Mar 29, 2023
f2cba87
Create authorizeRequest method
cwperks Apr 2, 2023
77700e5
small fix
cwperks Apr 22, 2023
d1d9ed1
Change to ProtectedRoute
cwperks Apr 28, 2023
ccab6ce
Remove extension permissions
cwperks May 8, 2023
174a142
Initial implementation of authz check in REST layer
cwperks Mar 29, 2023
2042a60
Extension TLS
cwperks Mar 28, 2023
0c70700
Adds dummy roles for testing rest authorization against legacy permis…
DarshitChanpura May 9, 2023
9837378
Adds support for legacy permissions to perform rest authorization
DarshitChanpura May 9, 2023
a34c3a4
Fixes white-space changes
DarshitChanpura May 9, 2023
f5afd62
Rebases ConfigConstants with main
DarshitChanpura May 24, 2023
918ab51
Implements a new logic for rest permissions check to be more flexible
DarshitChanpura May 24, 2023
98791a2
Fixes spotless errors
DarshitChanpura May 24, 2023
b639544
Adds regex to match against current role permissions when comparing n…
DarshitChanpura May 25, 2023
4288235
Moves legacy permission check logic to ConfigModelV7
DarshitChanpura May 26, 2023
93a2c66
Fixes extra new-lines
DarshitChanpura May 26, 2023
6e9e83d
Fixes unused imports
DarshitChanpura May 26, 2023
5ee7b12
Fixes out-of-scope white space changes
DarshitChanpura May 26, 2023
34092f9
Fixes code-ql errors
DarshitChanpura May 30, 2023
c41023b
Fixes spotless and code-ql errors
DarshitChanpura May 30, 2023
d746435
Fixes variable name and remove references to whitelist in javadoc
DarshitChanpura May 31, 2023
d68c76e
Adds tests for rest layer privilege evaluator
DarshitChanpura May 31, 2023
2814283
Adds license header to the test file
DarshitChanpura May 31, 2023
3b41475
Merge branch 'main' into authorize-rest-requests
DarshitChanpura Jun 1, 2023
d5392e4
Updates zstd dependency to fetch from core version.properties
DarshitChanpura Jun 1, 2023
425c22b
Updates action name in the regex to be dynamic
DarshitChanpura Jun 2, 2023
b1ed481
Adds support for allowing evaluation against multiple actions names f…
DarshitChanpura Jun 13, 2023
1e3efe2
Updates tests
DarshitChanpura Jun 13, 2023
590f55a
Adds null check
DarshitChanpura Jun 15, 2023
6918a80
Merge branch 'main' into authorize-rest-requests
DarshitChanpura Jun 16, 2023
427460c
Makes authorize logic clearer to follow
DarshitChanpura Jun 20, 2023
f3c4a77
Adds extra check to ensure new actions are also evaluated against tra…
DarshitChanpura Jun 20, 2023
8557fce
Fixes spotless errors
DarshitChanpura Jun 20, 2023
d66294d
Fixes security rest filter setup
DarshitChanpura Jun 20, 2023
4a33b33
Removes extension reference
DarshitChanpura Jun 20, 2023
6a1c25a
Merge branch 'main' into authorize-rest-requests
DarshitChanpura Jun 21, 2023
8503358
turn on audit logging
MaciejMierzwa Jun 22, 2023
afc8c81
Adds unit tests for restPathMatches method
DarshitChanpura Jun 22, 2023
14fe152
Cleans up TODOs
DarshitChanpura Jun 22, 2023
5bae5e8
Organizes demo users and roles for extension
DarshitChanpura Jun 22, 2023
24be564
Address PR feedback
DarshitChanpura Jun 22, 2023
2edd319
Adds more comments
DarshitChanpura Jun 22, 2023
3c397e2
add privileges info
MaciejMierzwa Jun 23, 2023
e7d10c7
Audit Logging for NamedRoutes
DarshitChanpura Jun 27, 2023
54cfcd9
Merge remote-tracking branch 'upstream/main' into authorize-rest-requ…
DarshitChanpura Jun 29, 2023
8bfeb25
Makes whoami action a named route and fixes license header check
DarshitChanpura Jun 29, 2023
ccd3d2d
Adds integ tests for whoami route
DarshitChanpura Jul 5, 2023
c68083c
Merge branch 'main' into authorize-rest-requests
DarshitChanpura Jul 6, 2023
dc0c1e2
Change permissions order in roles.yml
DarshitChanpura Jul 6, 2023
39314d7
Adds developer documentation for authorization in REST layer
DarshitChanpura Jul 6, 2023
ebc5486
Fixes broken tests
DarshitChanpura Jul 6, 2023
a78582f
Fixes checkstyle errors
DarshitChanpura Jul 7, 2023
a325d8e
Addresses feedback and cleans up logic for super admin check
DarshitChanpura Jul 7, 2023
0e905a0
Addresses Plugin Install CI failure
DarshitChanpura Jul 7, 2023
25a41b6
Fixes failing citest task
DarshitChanpura Jul 7, 2023
1b1f519
Modifies WhoAmI integ tests
DarshitChanpura Jul 7, 2023
ffdd927
Adds a new endpoint called whoamiprotected and removes changes made t…
DarshitChanpura Jul 7, 2023
9b05d44
Updates documentation to reflect the new API
DarshitChanpura Jul 7, 2023
a21c4c0
Addresses PR feedback
DarshitChanpura Jul 7, 2023
6c5ee8e
Renames action0 to actions
DarshitChanpura Jul 7, 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
4 changes: 2 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ configurations {
force "io.netty:netty-transport:${versions.netty}"
force "io.netty:netty-transport-native-unix-common:${versions.netty}"
force "org.apache.bcel:bcel:6.6.0" // This line should be removed once Spotbugs is upgraded to 4.7.4
force "com.github.luben:zstd-jni:${versions.zstd}"
force "com.github.luben:zstd-jni:1.5.5-3"
stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -412,7 +412,7 @@ dependencies {
runtimeOnly 'com.fasterxml.woodstox:woodstox-core:6.4.0'
runtimeOnly 'org.apache.ws.xmlschema:xmlschema-core:2.2.5'
runtimeOnly 'org.apache.santuario:xmlsec:2.2.3'
runtimeOnly "com.github.luben:zstd-jni:${versions.zstd}"
runtimeOnly "com.github.luben:zstd-jni:1.5.5-3"
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
runtimeOnly 'org.checkerframework:checker-qual:3.5.0'
runtimeOnly "org.bouncycastle:bcpkix-jdk15on:${versions.bouncycastle}"

Expand Down
5 changes: 5 additions & 0 deletions config/internal_users.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,8 @@ snapshotrestore:
backend_roles:
- "snapshotrestore"
description: "Demo snapshotrestore user, using external role mapping"

new-user:
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
hash: "$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG"
reserved: true
description: "Demo user for ext-test"
19 changes: 19 additions & 0 deletions config/roles.yml
Original file line number Diff line number Diff line change
Expand Up @@ -355,3 +355,22 @@ 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
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
extension_hw_greet:
reserved: true
cluster_permissions:
- 'hw:greet'

legacy_hw_greet_with_name:
reserved: true
cluster_permissions:
- 'cluster:admin/opensearch/hw/greet_with_name'

stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved
extension_hw_full:
reserved: true
cluster_permissions:
- 'hw:greet'
- 'hw:greet_with_adjective'
- 'hw:great_with_name'
- 'hw:goodbye'
11 changes: 11 additions & 0 deletions config/roles_mapping.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,14 @@ kibana_server:
reserved: true
users:
- "kibanaserver"

## Demo test roles
legacy_hw_greet_with_name:
reserved: true
users:
- "new-user"

extension_hw_greet:
reserved: true
users:
- "new-user"
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,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 @@ -205,6 +206,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
private volatile SecurityInterceptor si;
private volatile PrivilegesEvaluator evaluator;
private volatile UserService userService;
private volatile RestLayerPrivilegesEvaluator restLayerEvaluator;
private volatile ThreadPool threadPool;
private volatile ConfigurationRepository cr;
private volatile AdminDNs adminDns;
Expand Down Expand Up @@ -1019,8 +1021,11 @@ public Collection<Object> createComponents(
principalExtractor = ReflectionHelper.instantiatePrincipalExtractor(principalExtractorClass);
}

restLayerEvaluator = new RestLayerPrivilegesEvaluator(clusterService, threadPool, auditLog, cih, namedXContentRegistry);

securityRestHandler = new SecurityRestFilter(
backendRegistry,
restLayerEvaluator,
auditLog,
threadPool,
principalExtractor,
Expand All @@ -1035,6 +1040,7 @@ public Collection<Object> createComponents(
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 Down Expand Up @@ -1072,6 +1078,7 @@ public Collection<Object> createComponents(
components.add(xffResolver);
components.add(backendRegistry);
components.add(evaluator);
components.add(restLayerEvaluator);
components.add(si);
components.add(dcf);
components.add(userService);
Expand Down
119 changes: 100 additions & 19 deletions src/main/java/org/opensearch/security/filter/SecurityRestFilter.java
peternied marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
package org.opensearch.security.filter;

import java.nio.file.Path;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand All @@ -37,10 +40,10 @@
import org.greenrobot.eventbus.Subscribe;

import org.opensearch.OpenSearchException;
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.NamedRoute;
import org.opensearch.rest.RestChannel;
import org.opensearch.rest.RestHandler;
import org.opensearch.rest.RestRequest;
Expand All @@ -52,6 +55,8 @@
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 @@ -70,6 +75,7 @@ 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 @@ -88,6 +94,7 @@ public class SecurityRestFilter {

public SecurityRestFilter(
final BackendRegistry registry,
final RestLayerPrivilegesEvaluator evaluator,
final AuditLog auditLog,
final ThreadPool threadPool,
final PrincipalExtractor principalExtractor,
Expand All @@ -97,6 +104,7 @@ public SecurityRestFilter(
) {
super();
this.registry = registry;
this.evaluator = evaluator;
this.auditLog = auditLog;
this.threadContext = threadPool.getThreadContext();
this.principalExtractor = principalExtractor;
Expand All @@ -109,28 +117,26 @@ public SecurityRestFilter(

/**
* This function wraps around all rest requests
* If the request is authenticated, then it goes through a whitelisting check.
* The whitelisting check works as follows:
* If whitelisting is not enabled, then requests are handled normally.
* If whitelisting is enabled, then SuperAdmin is allowed access to all APIs, regardless of what is currently whitelisted.
* If whitelisting is enabled, then Non-SuperAdmin is allowed to access only those APIs that are whitelisted in {@link #requests}
* For example: if whitelisting is enabled and requests = ["/_cat/nodes"], then SuperAdmin can access all APIs, but non SuperAdmin
* If the request is authenticated, then it goes through a allowlisting check.
* The allowlisting check works as follows:
* If allowlisting is not enabled, then requests are handled normally.
* If allowlisting is enabled, then SuperAdmin is allowed access to all APIs, regardless of what is currently allowlisted.
* If allowlisting is enabled, then Non-SuperAdmin is allowed to access only those APIs that are allowlisted in {@link #requests}
* For example: if allowlisting is enabled and requests = ["/_cat/nodes"], then SuperAdmin can access all APIs, but non SuperAdmin
* can only access "/_cat/nodes"
* Further note: Some APIs are only accessible by SuperAdmin, regardless of whitelisting. For example: /_opendistro/_security/api/whitelist is only accessible by SuperAdmin.
* Further note: Some APIs are only accessible by SuperAdmin, regardless of allowlisting. For example: /_opendistro/_security/api/whitelist is only accessible by SuperAdmin.
* See {@link AllowlistApiAction} for the implementation of this API.
* SuperAdmin is identified by credentials, which can be passed in the curl request.
*/
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))) {
return (request, channel, client) -> {
org.apache.logging.log4j.ThreadContext.clearAll();
if (!checkAndAuthenticateRequest(request, channel)) {
User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
if (userIsSuperAdmin(user, adminDNs)
|| (whitelistingSettings.checkRequestIsAllowed(request, channel, client)
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
&& allowlistingSettings.checkRequestIsAllowed(request, channel, client))) {
if (authorizeRequest(original, request, channel, user)) {
original.handleRequest(request, channel, client);
}
}
Expand All @@ -145,7 +151,56 @@ private boolean userIsSuperAdmin(User user, AdminDNs adminDNs) {
return user != null && adminDNs.isAdmin(user);
}

private boolean checkAndAuthenticateRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
private boolean authorizeRequest(RestHandler original, RestRequest request, RestChannel channel, User user) {
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved

List<RestHandler.Route> restRoutes = original.routes();
Optional<RestHandler.Route> handler = restRoutes.stream()
.filter(rh -> rh.getMethod().equals(request.method()))
.filter(rh -> restPathMatches(request.path(), rh.getPath()))
.findFirst();
if (handler.isPresent() && handler.get() instanceof NamedRoute) {
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
PrivilegesEvaluatorResponse pres = new PrivilegesEvaluatorResponse();
NamedRoute route = ((NamedRoute) handler.get());
// if actionNames are present evaluate those first
Set<String> actionNames = route.actionNames();
if (actionNames != null && !actionNames.isEmpty()) {
pres = evaluator.evaluate(user, actionNames);
}

// now if pres.allowed is still false check for the NamedRoute name as a permission
if (!pres.isAllowed()) {
String action = route.name();
pres = evaluator.evaluate(user, Set.of(action));
}

if (log.isDebugEnabled()) {
log.debug(pres.toString());
}
if (pres.isAllowed()) {
// TODO make sure this is audit logged
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
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 ext hangs intermittently after single unauthorized request
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
channel.sendResponse(new BytesRestResponse(RestStatus.UNAUTHORIZED, err));
return false;
}
}

// if handler is not an instance of NamedRoute then we pass through to eval at Transport Layer.
// TODO Change this to false once all plugins have migrated to use NamedRoutes
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
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.

If I am reading this correctly, we are always going to be evaluating both on the REST and trasnport layer correct? The only case where things would happen only in the REST layer would be if the request were unauthorized?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I am reading this correctly, we are always going to be evaluating both on the REST and trasnport layer correct?

Yes.

The only case where things would happen only in the REST layer would be if the request were unauthorized?

I'm not quite sure I understand this. Can you elaborate a little?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to clarify that the only time a request would end in the REST layer would be if it were unauthorized by the REST layer auth checker. Otherwise, it will always hit the transport layer checks.

}

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

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

Expand Down Expand Up @@ -217,4 +272,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 NamedRoute}
* @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) {
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Units like this should have tests with them. I've seen code similar to this in core, can this be extracted to a library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Test file already exists. Check it out https://github.com/opensearch-project/security/blob/9b05d44a6ba15bca521e1ccbd6e33124facddcc0/src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java

can this be extracted to a library?

It's only used at 2 places and I don't think it will be used at other places in future. I think keeping it here is fine.

// 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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,10 @@ public PrivilegesEvaluatorResponse evaluate(
);

if (isClusterPerm(action0)) {
if (!securityRoles.impliesClusterPermissionPermission(action0)) {
// We add a check here for `impliesLegacyPermission` to ensure that no new action names miss evaluation here
// e.g. ad:get/detector/info will be evaluated as `cluster:admin/opensearch/ad/get/detector/info` against user's granted
// permissions
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
if (!securityRoles.impliesClusterPermissionPermission(action0) && !securityRoles.impliesLegacyPermission(action0)) {
presponse.missingPrivileges.add(action0);
presponse.allowed = false;
log.info(
Expand Down
Loading