Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4896

cwperks · 2024-11-11T22:06:37Z

Description

Re-opening this previously closed PR to rebase on top of Optimized Privilege Evaluation

Companion PR in core: opensearch-project/OpenSearch#14630

This PR by itself does not add additional functionality, it simply implements the new extension points in core and introduces a new class called ContextProvidingPluginSubject which populates a header in the ThreadContext with the canonical class name of the plugin that is executing code using pluginSystemSubject.runAs(() -> { ... }). See ./gradlew integrationTest --tests SystemIndexTests for an example that verifies that a block of code run with pluginSystemSubject.runAs(() -> { ... }) has contextual info in the thread context.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Enhancement

Issues Resolved

Related to #4439

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Craig Perkins <[email protected]>

codecov · 2024-11-12T03:12:09Z

Codecov Report

Attention: Patch coverage is 85.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 71.53%. Comparing base (2fa7eff) to head (6537f14).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ensearch/security/privileges/ActionPrivileges.java	59.09%	5 Missing and 4 partials ⚠️
...ava/org/opensearch/security/auth/SecurityUser.java	90.00%	1 Missing ⚠️
...ecurity/privileges/SystemIndexAccessEvaluator.java	92.85%	0 Missing and 1 partial ⚠️
...c/main/java/org/opensearch/security/user/User.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4896      +/-   ##
==========================================
+ Coverage   71.51%   71.53%   +0.02%     
==========================================
  Files         334      336       +2     
  Lines       22556    22625      +69     
  Branches     3589     3598       +9     
==========================================
+ Hits        16130    16184      +54     
- Misses       4636     4643       +7     
- Partials     1790     1798       +8

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`83.92% <100.00%> (+0.36%)`	⬆️
.../org/opensearch/security/auth/BackendRegistry.java	`78.39% <100.00%> (+0.33%)`	⬆️
...org/opensearch/security/filter/SecurityFilter.java	`66.35% <100.00%> (+0.47%)`	⬆️
...curity/identity/ContextProvidingPluginSubject.java	`100.00% <100.00%> (ø)`
...earch/security/privileges/PrivilegesEvaluator.java	`73.96% <100.00%> (+0.31%)`	⬆️
...g/opensearch/security/support/ConfigConstants.java	`95.23% <ø> (ø)`
...ava/org/opensearch/security/auth/SecurityUser.java	`90.00% <90.00%> (ø)`
...ecurity/privileges/SystemIndexAccessEvaluator.java	`76.92% <92.85%> (+1.72%)`	⬆️
...c/main/java/org/opensearch/security/user/User.java	`69.41% <0.00%> (-0.83%)`	⬇️
...ensearch/security/privileges/ActionPrivileges.java	`95.14% <59.09%> (-2.05%)`	⬇️

... and 3 files with indirect coverage changes

Signed-off-by: Craig Perkins <[email protected]>

nibix · 2024-11-16T05:18:52Z

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

+        Set<String> clusterActions = new HashSet<>();
+        clusterActions.add(BulkAction.NAME);
+        PluginSubject subject = new ContextProvidingPluginSubject(threadPool, settings, plugin);
+        sf.updatePluginToClusterAction(subject.getPrincipal().getName(), clusterActions);


In what life-cycle stage(s) will this method will be called in?

In the current state of the implementation, this method must be called very early, as the constructed ActionPrivileges instance is immutable and will be only updated upon configuration change events. If this method gets called at the wrong time, changes won't be picked up immediately.

Additionaly, an open question is whether one has to think of concurrent calls or not.

This is called during node bootstrap. Its called directly after createComponents

https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/node/Node.java#L1047

https://github.com/opensearch-project/OpenSearch/blob/2ac64a6262973383d71f3e3ee037e4d267c3b592/server/src/main/java/org/opensearch/identity/IdentityService.java#L69

src/main/java/org/opensearch/security/filter/SecurityFilter.java

nibix · 2024-11-16T05:23:33Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

@@ -152,6 +154,7 @@ public class PrivilegesEvaluator {
    private DynamicConfigModel dcm;
    private final NamedXContentRegistry namedXContentRegistry;
    private final Settings settings;
+    private final Map<String, Set<String>> pluginToClusterActions;


I guess, it is only a temporary state that we'll have cluster actions here?

If we get also index privileges and possibly other stuff soon, this certainly would deserve its own class encapsulating and managing the details. Then, one could also avoid this call delegation of updatePluginToClusterActions() through SecurityFilter which feels artificial.

Yes, that's right. I made that chain of calls to updatePluginToClusterActions() out of convenience, but it is less then ideal. I have a PR open in core to declare actions a plugin will perform with its identity and prompt on installation, but it is currently stalled.

I plan to take a little bit of time to see what it would take to move system index protection into the core and introduce a notion of admin certificate to the core as a mechanism for taking invasive action on system indices if required.

The biggest challenge I see is ActionRequest -> concrete index resolution.

Do you mean ActionRequest -> concrete index resolution in the core or in the security plugin?

I really think that each transport request implementation should be required to provide an interface to do such a resolution in its own semantics.

Capturing all of the previous discussions into a comment here. This PR allows a plugin to access its own registered system indices using the pluginSubject that was assigned to the IdentityAwarePlugin.

In some instances, plugins may need to perform actions against regular indices or cluster actions outside of the authenticated user context to guarantee that the action succeeds. The primary example of this is the security plugin needing to be able to write to the auditlog index if an opensearch index is used for the auditlog.

I opened a PR in core that would allow a plugin to declare the actions it will perform on a cluster (outside of system index access) and a cluster admin would be prompted on plugin install that the plugin will perform those actions. Using that PR in core, I would follow-up this PR with another that would allow a plugin to perform index actions using its pluginSubject, but the core PR is in an uncertain state because the main feedback is that system index protection should be a core feature.

I don't disagree with that in principle, but there are complications. For instance, in the security plugin there is a notion of a superuser that is permitted to perform operations on a system index. To authenticate as the superuser, you present the admin certificate and then can run an action against a system index. Assuming a cluster operates normally, this would not be needed, but it is helpful for an operator to perform invasive actions if a cluster gets into a bad state.

In order to use client cert authentication, a cluster needs https. If system index protection is ported to the core, it means that it could run with or without https. In clusters without https, it is not possible to create a secure equivalent of a superuser.

FYI @reta, this is the Draft PR I created in core to demonstrate the challenges of porting system index protection into the core: opensearch-project/OpenSearch#16695

I'd agree to this analysis, @cwperks .

Actually, core already contains ground works for a protection of system indices. It works like this:

In case a REST request is issues against a system index, the following message will be logged: this request accesses system indices: [{}], but in a future major version, direct access to system indices will be prevented by default

Exception: The HTTP header X-opensearch-product-origin is present in the REST request

Other exception: The RestHandler implementation overrides allowSystemIndexAccessByDefault() and returns true.

See this commit for details: opensearch-project/OpenSearch@5c8b066

These concepts still come from the ES times. If I understand it correctly, the goal was to replace the warning message by an error as a breaking change with a major release.

IMHO, this concept makes totally sense for core. As @cwperks has pointed out, core has no safe means of authentication. What core can do is a kind of cooperative check: If a client assures that they need to perform operations on a system index, they are allowed to. If the client signals no concrete intent to operate on system index, they are not allowed to. This avoids unwanted operations in case of bugs on side of the client - be it an overly broad index pattern or a similar case. Of course, it does not provide real security. But that just takes into account the fact that core has no means to perform a secure authentication.

IMHO, a good strategy would be to build on the present ground works and to complete them. That would be also useful from a code hygiene point of view, as otherwise that existing code would be there without real purpose.

Relevant code pieces:

https://github.com/opensearch-project/OpenSearch/blob/57a660558f925da44eca18a7eb21983e61a7124a/server/src/main/java/org/opensearch/cluster/metadata/IndexNameExpressionResolver.java#L359-L378

https://github.com/opensearch-project/OpenSearch/blame/57a660558f925da44eca18a7eb21983e61a7124a/server/src/main/java/org/opensearch/rest/RestController.java#L374-L381

https://github.com/opensearch-project/OpenSearch/blob/57a660558f925da44eca18a7eb21983e61a7124a/server/src/main/java/org/opensearch/cluster/metadata/IndexNameExpressionResolver.java#L767-L774

Signed-off-by: Craig Perkins <[email protected]>

...est/java/org/opensearch/security/plugin/RestBulkIndexDocumentIntoMixOfSystemIndexAction.java

src/main/java/org/opensearch/security/auth/BackendRegistry.java

src/main/java/org/opensearch/security/filter/SecurityFilter.java

nibix · 2024-12-11T14:22:14Z

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

@@ -407,6 +441,14 @@ PrivilegesEvaluatorResponse providesPrivilege(PrivilegesEvaluationContext contex
                }
            }

+            // 4: If plugin is performing the action, check if plugin has permission
+            if (this.usersToActionMatcher.containsKey(context.getUser().getName())) {


If we are assuming that this applies only to plugin users - should we be also somehow verifying this condition, just for hardening?

I didn't explicitly put the check in because this.usersToActionMatcher is only populated on bootstrap and we control this data structure.

While this would be limited to plugins in its current form, it could be extended to the service accounts associated with extensions. I think a check would be redundant, but we could add it in to prevent a map lookup for regular users.

Added the check in

nibix · 2024-12-11T14:29:44Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

@@ -152,6 +154,7 @@ public class PrivilegesEvaluator {
    private DynamicConfigModel dcm;
    private final NamedXContentRegistry namedXContentRegistry;
    private final Settings settings;
+    private final Map<String, Set<String>> pluginToClusterActions;


Do you mean ActionRequest -> concrete index resolution in the core or in the security plugin?

I really think that each transport request implementation should be required to provide an interface to do such a resolution in its own semantics.

Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura

Thank you @cwperks. Left a few comments but generally looks good to me. Will there be a follow-up PR to clean up pluginActions matcher store once the PR in core gets merged?

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java

src/test/java/org/opensearch/security/auth/SecurityUserTests.java

src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java

src/integrationTest/java/org/opensearch/security/SystemIndexTests.java

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2024-12-13T20:58:30Z

Thank you @cwperks. Left a few comments but generally looks good to me. Will there be a follow-up PR to clean up pluginActions matcher store once the PR in core gets merged?

Yes there would be one more PR to implement index actions (for completeness) if opensearch-project/OpenSearch#15778 is merged.

There's been a lot of discussion and I left a comment here to outline some reasons why I don't recommend system index protection to be a core feature. This gist is that the security plugin has a notion of connecting with the admin certificate that can be used as a backdoor for an operator to perform invasive actions on system indices if necessary. The admin certificate requires that a cluster has https enabled and https is provided by the security plugin. There isn't a secure way to create a backdoor for a cluster running without https.

cwperks · 2024-12-23T14:11:09Z

@nibix @derek-ho Can I get another review for this?

src/integrationTest/java/org/opensearch/security/SystemIndexTests.java

derek-ho · 2024-12-23T17:34:27Z

src/main/java/org/opensearch/security/user/User.java

+
+    /**
+     * Check the custom attributes associated with this user
+     *
+     * @return true if it has a plugin account attributes, otherwise false
+     */
+    public boolean isPluginUser() {
+        return name != null && name.startsWith("plugin:");
+    }


QQ: Do we need to add a restriction that usernames cannot begin with this prefix?

I chose this as a convention since : is forbidden from usernames. Users representing plugins will only exist in-memory while executing transport actions with the plugin's subject. If an inter-nodal transport hop occurs then the user is serialized and then deserialized on the receiving node.

src/integrationTest/java/org/opensearch/security/SystemIndexTests.java

src/main/java/org/opensearch/security/auth/BackendRegistry.java

nibix · 2024-12-24T09:02:34Z

src/main/java/org/opensearch/security/auth/BackendRegistry.java

+            final User effectiveUser = impersonatedUser == null ? authenticatedUser : impersonatedUser;
+            threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, effectiveUser);
+            UserSubject subject = new SecurityUser(threadPool, effectiveUser);
+            threadPool.getThreadContext().putPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, subject);


I'm seeing the putPersistent() method for the first time. I have read in the API docs that the difference is that the value cannot be stashed.

This kind of shows me that I do not really understand the sematic differences between UserSubject and User? Why is one persistent, but the other transient?

Ultimately, my goal is that there is a single TC persistent (unstashable) header that represents the authenticated user so that the security plugin can reliably get information about the authenticated user from the TC and does not need to be concerned if the TC has been stashed.

The difference between User and UserSubject is that UserSubject has a runAs method that can be used to switch into the authenticated user context. For instance, its possible to write an API handler that switches to the plugin context and then back to the user context.

The transient user header would be used to hold the value of the "user" to authorize actions with. In most circumstances, this would be the authenticated user, but when a plugin uses their pluginSubject and calls runAs then it will hold the identity associated with the plugin while the OPENDISTRO_SECURITY_AUTHENTICATED_USER header would hold information about the authenticated user.

nibix · 2024-12-24T09:26:41Z

src/main/java/org/opensearch/security/auth/SecurityUser.java

+    }
+
+    @Override
+    public <T> T runAs(Callable<T> callable) throws Exception {


I am wondering if this runAs() API covers all possible scenarios.

It is sufficient for synchronous scenarios, where it allows to run the code subsequent to this runAs() call again in the original context.

However, OpenSearch architecture is fundamentally asynchronos. Often, it happens that "subsequent" logic does actually not follow in the subsequent lines but in the onResult() action listener method. In such a onResult() method, there would be no way to restore the context with this runAs() API.

Using the stashContext() API dirtectly however supports such scenarios, one example of this pattern is here (see the restore call in line 238):

security/src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java

Lines 166 to 257 in 2fa7eff

private boolean handle() {

try (StoredContext ctx = threadContext.newStoredContext(true)) {

threadContext.putHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE, request.toString());

try {

if (!modifyQuery()) {

return true;

}

if (log.isDebugEnabled()) {

log.debug("Created filterLevelQuery for " + request + ":\n" + filterLevelQueryBuilder);

}

} catch (Exception e) {

log.error("Unable to handle filter level DLS", e);

listener.onFailure(new OpenSearchSecurityException("Unable to handle filter level DLS", e));

return false;

}

if (filterLevelQueryBuilder == null) {

return true;

}

if (request instanceof SearchRequest) {

return handle((SearchRequest) request, ctx);

} else if (request instanceof GetRequest) {

return handle((GetRequest) request, ctx);

} else if (request instanceof MultiGetRequest) {

return handle((MultiGetRequest) request, ctx);

} else if (request instanceof ClusterSearchShardsRequest) {

return handle((ClusterSearchShardsRequest) request, ctx);

} else {

log.error("Unsupported request type for filter level DLS: " + request);

listener.onFailure(

new OpenSearchSecurityException(

"Unsupported request type for filter level DLS: " + action + "; " + request.getClass().getName()

)

);

return false;

}

}

}

private boolean handle(SearchRequest searchRequest, StoredContext ctx) {

if (documentAllowlist != null) {

documentAllowlist.applyTo(threadContext);

}

String localClusterAlias = LOCAL_CLUSTER_ALIAS_GETTER.apply(searchRequest);

if (localClusterAlias != null) {

try {

modifyQuery(localClusterAlias);

} catch (Exception e) {

log.error("Unable to handle filter level DLS", e);

listener.onFailure(new OpenSearchSecurityException("Unable to handle filter level DLS", e));

return false;

}

}

if (searchRequest.source().query() != null) {

filterLevelQueryBuilder.must(searchRequest.source().query());

}

searchRequest.source().query(filterLevelQueryBuilder);

nodeClient.search(searchRequest, new ActionListener<SearchResponse>() {

@Override

public void onResponse(SearchResponse response) {

try {

ctx.restore();

@SuppressWarnings("unchecked")

ActionListener<SearchResponse> searchListener = (ActionListener<SearchResponse>) listener;

searchListener.onResponse(response);

} catch (Exception e) {

listener.onFailure(e);

}

}

@Override

public void onFailure(Exception e) {

listener.onFailure(e);

}

});

return false;

}

Note: I understand that this example does not match the use case for runAs() - however I can imagine that also plugins could need such scenarios. Or do I get something wrong?

To illustrate the issue, look at the following code (from the int test):

security/src/integrationTest/java/org/opensearch/security/plugin/TransportRunClusterHealthAction.java

Lines 51 to 84 in 154556c

protected void doExecute(Task task, RunClusterHealthRequest request, ActionListener<RunClusterHealthResponse> actionListener) {

String runAs = request.getRunAs();

if ("user".equalsIgnoreCase(runAs)) {

Subject user = identityService.getCurrentSubject();

try {

user.runAs(() -> {

ActionListener<ClusterHealthResponse> chr = ActionListener.wrap(

r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); },

actionListener::onFailure

);

client.admin().cluster().health(new ClusterHealthRequest(), chr);

return null;

});

} catch (Exception e) {

throw new RuntimeException(e);

}

} else if ("plugin".equalsIgnoreCase(runAs)) {

contextSwitcher.runAs(() -> {

ActionListener<ClusterHealthResponse> chr = ActionListener.wrap(

r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); },

actionListener::onFailure

);

client.admin().cluster().health(new ClusterHealthRequest(), chr);

return null;

});

} else {

ActionListener<ClusterHealthResponse> chr = ActionListener.wrap(

r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); },

actionListener::onFailure

);

client.admin().cluster().health(new ClusterHealthRequest(), chr);

}

}

}

If runAs equals plugin, then the onResponse() method of the provided actionListener would be executed in the plugin user context. Thus, any client which calls this action would suddenly have a plugin user scope.

The ultimate goal is to ensure that authz checks are run with any transport request that is executed. The security plugin needs to have access to an identity associated with the executor of a transport action. Currently with stashContext, the identity is erased from the TC and any plugin can operate w/o authz checks.

What is the purpose of restoring the context in DlsFilterLevelActionHandler? Is restoring the context necessary? wdyt about a higher-level api (similar to runAs) where it can be guaranteed when restoring the context that the OPENDISTRO_SECURITY_USER transient header is populated to ensure that authz checks are performed?

What is the purpose of restoring the context in DlsFilterLevelActionHandler? Is restoring the context necessary?

Yes, because the control is returned then to the caller of the action. The caller might call another action for which DLS/FLS also needs to be enforced. This requires the ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE header to be absent. If it would be still there, no DLS/FLS protections would be applied.

Just, to avoid any confusions: I used this example only to illustrate the pattern - a pattern which is IMHO an essential pattern. However, it is not directly related to runAs().

The actual issue: The runAs() API has IMHO the fundamental issue that it assumes a single threading model. However, single thread scenarios are rather the exception than the norm in OpenSearch. IMHO, there needs to be another way to tell "this block of logic has finished" other than the current thread leaving the runAs() block again.

That's a good point. I just tested out the restore approach you posted above and it works within a runAs block.

i.e.

... } else if ("plugin".equalsIgnoreCase(runAs)) { ThreadContext.StoredContext ctx = threadPool.getThreadContext().newStoredContext(true); ActionListener<ClusterHealthResponse> chr = ActionListener.wrap( r -> { ctx.restore(); actionListener.onResponse(new RunClusterHealthResponse(true)); }, e -> { System.out.println("Before restore"); System.out.println("Current user: " + threadPool.getThreadContext().getTransient(OPENDISTRO_SECURITY_USER)); ctx.restore(); System.out.println("After restore"); System.out.println("Current user: " + threadPool.getThreadContext().getTransient(OPENDISTRO_SECURITY_USER)); actionListener.onFailure(e); } ); contextSwitcher.runAs(() -> { client.admin().cluster().health(new ClusterHealthRequest(), chr); return null; }); } ...

and in the output I see

> Task :integrationTest SystemIndexTests > testPluginShouldNotBeAbleToRunClusterActions STANDARD_OUT Before restore Current user: User [name=plugin:org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1, backend_roles=[], requestedTenant=null] After restore Current user: User [name=admin, backend_roles=[], requestedTenant=null]

Ideally, there is a higher-level API when executing a transport action to ensure the handler for the transport response is run in the original context, but the same workaround from your origin comment works here as well. This PR would incrementally improve the security posture by ensuring that the transport action is run with authz checks and ensuring that a plugin only accesses its own system indices.

nibix · 2024-12-24T11:14:47Z

...ionTest/java/org/opensearch/security/plugin/TransportIndexDocumentIntoSystemIndexAction.java

+ *
+ */
+
+package org.opensearch.security.plugin;


Nit: As the new classes in the org.opensearch.security.plugin package are only used by the SystemIndexTests class, shouldn't these moved into a common package? Otherwise, their purpose might be unclear.

Rearranged this to put the test in a package called systemindex and the sample plugins (and associated classes) in a subpackage named sampleplugin

src/main/java/org/opensearch/security/identity/PluginContextSwitcher.java

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

nibix · 2024-12-24T11:24:39Z

src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java

+                user.getName().replace("plugin:", ""),
+                requestedResolved.getAllIndices()
+            );
+            if (requestedResolved.getAllIndices().equals(matchingSystemIndices)) {


requestedResolved.getAllIndices() must be guarded by a isLocalAll(), otherwise you will get wrong results for _all requests.

See response below. Can the whole top level condition be if (user.isPluginUser() && !requestedResolved.isLocalAll()) { ... }?

nibix · 2024-12-24T11:31:23Z

src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java

+                requestedResolved.getAllIndices()
+            );
+            if (requestedResolved.getAllIndices().equals(matchingSystemIndices)) {
+                // plugin is authorized to perform any actions on its own registered system indices


This code is called for index and cluster actions. Even though getAllIndices() probably returns * for cluster action as a more or less suitable fallback - shouldn't we better check the action type?

Would it be sufficient to change the top level condition to if (user.isPluginUser() && !requestedResolved.isLocalAll()) { ... }?

src/integrationTest/java/org/opensearch/security/plugin/SystemIndexPlugin1.java

Signed-off-by: Craig Perkins <[email protected]>

cwperks added 4 commits November 11, 2024 14:46

Rebasing security subject

779d9d3

Signed-off-by: Craig Perkins <[email protected]>

Complete rebase

de50e87

Signed-off-by: Craig Perkins <[email protected]>

Fix tests

9a997b2

Signed-off-by: Craig Perkins <[email protected]>

Leave action out of error message for now

372c657

Signed-off-by: Craig Perkins <[email protected]>

cwperks added 2 commits November 14, 2024 13:59

Merge branch 'main' into security-subject-rebase

fdf7256

Remove calls to isPluginUser

c25e1ab

Signed-off-by: Craig Perkins <[email protected]>

cwperks marked this pull request as ready for review November 15, 2024 18:34

cwperks requested review from DarshitChanpura, derek-ho, nibix, peternied, RyanL1997, reta and willyborankin as code owners November 15, 2024 18:34

nibix reviewed Nov 16, 2024

View reviewed changes

src/main/java/org/opensearch/security/filter/SecurityFilter.java Show resolved Hide resolved

nibix reviewed Nov 16, 2024

View reviewed changes

cwperks added 3 commits November 19, 2024 13:30

Merge branch 'main' into security-subject-rebase

0adcd76

Pluralize

941c131

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'main' into security-subject-rebase

071cca9

nibix reviewed Dec 11, 2024

View reviewed changes

cwperks added 3 commits December 11, 2024 15:02

Merge branch 'main' into security-subject-rebase

2a0aa3a

Address review feedback

3e353c9

Signed-off-by: Craig Perkins <[email protected]>

Check if pluginUser

ce80b7c

Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura reviewed Dec 13, 2024

View reviewed changes

Remove overloaded method

154556c

Signed-off-by: Craig Perkins <[email protected]>

nibix mentioned this pull request Dec 18, 2024

Allow Plugins to request to perform cluster actions and index actions with their assigned PluginSubject and prompt on install opensearch-project/OpenSearch#15778

Open

3 tasks

cwperks mentioned this pull request Dec 20, 2024

[DRAFT] [Feature] Introduces resource sharing and access-control #4746

Draft

5 tasks

DarshitChanpura previously approved these changes Dec 20, 2024

View reviewed changes

derek-ho reviewed Dec 23, 2024

View reviewed changes

src/integrationTest/java/org/opensearch/security/SystemIndexTests.java Show resolved Hide resolved

derek-ho reviewed Dec 23, 2024

View reviewed changes

nibix reviewed Dec 24, 2024

View reviewed changes

src/main/java/org/opensearch/security/identity/PluginContextSwitcher.java Outdated Show resolved Hide resolved

nibix reviewed Dec 24, 2024

View reviewed changes

src/main/java/org/opensearch/security/privileges/ActionPrivileges.java Show resolved Hide resolved

nibix reviewed Dec 24, 2024

View reviewed changes

src/integrationTest/java/org/opensearch/security/plugin/SystemIndexPlugin1.java Outdated Show resolved Hide resolved

cwperks added 2 commits December 30, 2024 09:34

Merge branch 'main' into security-subject-rebase

fce023a

Respond to PR feedback

1f7417c

Signed-off-by: Craig Perkins <[email protected]>

cwperks dismissed DarshitChanpura’s stale review via 1f7417c December 30, 2024 16:52

cwperks added 4 commits December 30, 2024 15:19

Add unit tests

cb06896

Signed-off-by: Craig Perkins <[email protected]>

Create RestMatchers

40c89ec

Signed-off-by: Craig Perkins <[email protected]>

Add license header

cf1a4d9

Signed-off-by: Craig Perkins <[email protected]>

Check for injected roles

6537f14

Signed-off-by: Craig Perkins <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4896

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4896

cwperks commented Nov 11, 2024

codecov bot commented Nov 12, 2024 •

edited

Loading

nibix Nov 16, 2024 •

edited

Loading

cwperks Nov 19, 2024

nibix Nov 16, 2024

cwperks Nov 20, 2024

cwperks Nov 20, 2024

nibix Dec 11, 2024

cwperks Dec 11, 2024

nibix Dec 18, 2024 •

edited

Loading

nibix Dec 11, 2024

cwperks Dec 11, 2024

cwperks Dec 11, 2024

nibix Dec 11, 2024

DarshitChanpura left a comment

cwperks commented Dec 13, 2024 •

edited

Loading

cwperks commented Dec 23, 2024

derek-ho Dec 23, 2024

cwperks Dec 30, 2024

nibix Dec 24, 2024

cwperks Dec 30, 2024

nibix Dec 24, 2024

nibix Dec 24, 2024

cwperks Dec 30, 2024 •

edited

Loading

nibix Dec 30, 2024 •

edited

Loading

cwperks Dec 30, 2024

nibix Dec 24, 2024

cwperks Dec 30, 2024

nibix Dec 24, 2024

cwperks Dec 30, 2024

nibix Dec 24, 2024

cwperks Dec 30, 2024 •

edited

Loading

	private boolean handle() {

	try (StoredContext ctx = threadContext.newStoredContext(true)) {

	threadContext.putHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE, request.toString());

	try {
	if (!modifyQuery()) {
	return true;
	}

	if (log.isDebugEnabled()) {
	log.debug("Created filterLevelQuery for " + request + ":\n" + filterLevelQueryBuilder);
	}

	} catch (Exception e) {
	log.error("Unable to handle filter level DLS", e);
	listener.onFailure(new OpenSearchSecurityException("Unable to handle filter level DLS", e));
	return false;
	}

	if (filterLevelQueryBuilder == null) {
	return true;
	}

	if (request instanceof SearchRequest) {
	return handle((SearchRequest) request, ctx);
	} else if (request instanceof GetRequest) {
	return handle((GetRequest) request, ctx);
	} else if (request instanceof MultiGetRequest) {
	return handle((MultiGetRequest) request, ctx);
	} else if (request instanceof ClusterSearchShardsRequest) {
	return handle((ClusterSearchShardsRequest) request, ctx);
	} else {
	log.error("Unsupported request type for filter level DLS: " + request);
	listener.onFailure(
	new OpenSearchSecurityException(
	"Unsupported request type for filter level DLS: " + action + "; " + request.getClass().getName()
	)
	);
	return false;
	}
	}
	}

	private boolean handle(SearchRequest searchRequest, StoredContext ctx) {
	if (documentAllowlist != null) {
	documentAllowlist.applyTo(threadContext);
	}

	String localClusterAlias = LOCAL_CLUSTER_ALIAS_GETTER.apply(searchRequest);

	if (localClusterAlias != null) {
	try {
	modifyQuery(localClusterAlias);
	} catch (Exception e) {
	log.error("Unable to handle filter level DLS", e);
	listener.onFailure(new OpenSearchSecurityException("Unable to handle filter level DLS", e));
	return false;
	}
	}

	if (searchRequest.source().query() != null) {
	filterLevelQueryBuilder.must(searchRequest.source().query());
	}

	searchRequest.source().query(filterLevelQueryBuilder);

	nodeClient.search(searchRequest, new ActionListener<SearchResponse>() {
	@Override
	public void onResponse(SearchResponse response) {
	try {
	ctx.restore();

	@SuppressWarnings("unchecked")
	ActionListener<SearchResponse> searchListener = (ActionListener<SearchResponse>) listener;

	searchListener.onResponse(response);
	} catch (Exception e) {
	listener.onFailure(e);
	}
	}

	@Override
	public void onFailure(Exception e) {
	listener.onFailure(e);
	}
	});

	return false;
	}

	protected void doExecute(Task task, RunClusterHealthRequest request, ActionListener<RunClusterHealthResponse> actionListener) {
	String runAs = request.getRunAs();
	if ("user".equalsIgnoreCase(runAs)) {
	Subject user = identityService.getCurrentSubject();
	try {
	user.runAs(() -> {
	ActionListener<ClusterHealthResponse> chr = ActionListener.wrap(
	r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); },
	actionListener::onFailure
	);
	client.admin().cluster().health(new ClusterHealthRequest(), chr);
	return null;
	});
	} catch (Exception e) {
	throw new RuntimeException(e);
	}
	} else if ("plugin".equalsIgnoreCase(runAs)) {
	contextSwitcher.runAs(() -> {
	ActionListener<ClusterHealthResponse> chr = ActionListener.wrap(
	r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); },
	actionListener::onFailure
	);
	client.admin().cluster().health(new ClusterHealthRequest(), chr);
	return null;
	});
	} else {
	ActionListener<ClusterHealthResponse> chr = ActionListener.wrap(
	r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); },
	actionListener::onFailure
	);
	client.admin().cluster().health(new ClusterHealthRequest(), chr);
	}
	}
	}

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4896

Are you sure you want to change the base?

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4896

Conversation

cwperks commented Nov 11, 2024

Description

Issues Resolved

Check List

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

nibix Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibix Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura left a comment

Choose a reason for hiding this comment

cwperks commented Dec 13, 2024 • edited Loading

cwperks commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

nibix Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 12, 2024 •

edited

Loading

nibix Nov 16, 2024 •

edited

Loading

nibix Dec 18, 2024 •

edited

Loading

cwperks commented Dec 13, 2024 •

edited

Loading

cwperks Dec 30, 2024 •

edited

Loading

nibix Dec 30, 2024 •

edited

Loading

cwperks Dec 30, 2024 •

edited

Loading