Refactor IdentityAwarePlugin interface to be assigned a client for executing actions #16976
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| import org.apache.shiro.SecurityUtils; | ||
| import org.apache.shiro.mgt.SecurityManager; | ||
| import org.opensearch.client.Client; | ||
| import org.opensearch.client.FilterClient; | ||
| import org.opensearch.client.node.NodeClient; | ||
| import org.opensearch.cluster.metadata.IndexNameExpressionResolver; | ||
| import org.opensearch.cluster.service.ClusterService; | ||
|
|
@@ -23,8 +24,8 @@ | |
| import org.opensearch.core.xcontent.NamedXContentRegistry; | ||
| import org.opensearch.env.Environment; | ||
| import org.opensearch.env.NodeEnvironment; | ||
| import org.opensearch.identity.Subject; | ||
| import org.opensearch.identity.noop.RunAsSystemClient; | ||
| import org.opensearch.identity.tokens.AuthToken; | ||
| import org.opensearch.identity.tokens.TokenManager; | ||
| import org.opensearch.plugins.ActionPlugin; | ||
|
|
@@ -54,6 +55,7 @@ | |
| private final ShiroTokenManager authTokenHandler; | ||
|
|
||
| private ThreadPool threadPool; | ||
| private Client client; | ||
|
|
||
| /** | ||
| * Create a new instance of the Shiro Identity Plugin | ||
|
|
@@ -83,6 +85,7 @@ | |
| Supplier<RepositoriesService> repositoriesServiceSupplier | ||
| ) { | ||
| this.threadPool = threadPool; | ||
| this.client = client; | ||
|
Check warning on line 88 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java
|
||
| return Collections.emptyList(); | ||
| } | ||
|
|
||
|
|
@@ -138,7 +141,7 @@ | |
| } | ||
| } | ||
|
|
||
| public FilterClient getRunAsClient(Plugin plugin) { | ||
cwperks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return new RunAsSystemClient(client); | ||
|
Check warning on line 145 in plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java
|
||
|
There was a problem hiding this comment. With the new multitenancy feature, we're using this wrapped execution with a different client interface. So the thread context stashing wraps other code elsewhere (with a client object included) using both clients ( See https://github.com/opensearch-project/ml-commons/blob/3bbc70077bc76f790077cf46666569017a7032ed/plugin/src/main/java/org/opensearch/ml/action/connector/GetConnectorTransportAction.java#L83-L96 for an example of delegating to a utility class method See https://github.com/opensearch-project/ml-commons/blob/3bbc70077bc76f790077cf46666569017a7032ed/plugin/src/main/java/org/opensearch/ml/action/connector/DeleteConnectorTransportAction.java#L121-L162 for a wrapped call using a different client interface |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* | ||
| * 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. | ||
| */ | ||
|
|
||
| package org.opensearch.identity.noop; | ||
|
|
||
| import org.opensearch.action.ActionRequest; | ||
| import org.opensearch.action.ActionType; | ||
| import org.opensearch.client.Client; | ||
| import org.opensearch.client.FilterClient; | ||
| import org.opensearch.common.annotation.InternalApi; | ||
| import org.opensearch.common.util.concurrent.ThreadContext; | ||
| import org.opensearch.core.action.ActionListener; | ||
| import org.opensearch.core.action.ActionResponse; | ||
|
|
||
| /** | ||
| * Implementation of client that will run transport actions in a stashed context | ||
| * <p> | ||
| * This class and related classes in this package will not return nulls or fail permissions checks | ||
| * | ||
| * This class is used by the NoopIdentityPlugin to initialize IdentityAwarePlugins | ||
| * | ||
| * @opensearch.internal | ||
| */ | ||
| @InternalApi | ||
| public class RunAsSystemClient extends FilterClient { | ||
|
There was a problem hiding this comment. This isn't an instance of a new client, its a wrapper around that local node client initialized in Node.java that overrides the In particular, this is the default implementation that stashes the context prior to executing an action and restores it prior to delegating back to the original actionListener's onResponse or onFailure. There was a problem hiding this comment. In the multitenant client mentioned earlier, the pattern is:
So for this to work we would need to change the default client implementation to use this See https://github.com/opensearch-project/opensearch-remote-metadata-sdk/blob/main/core/src/main/java/org/opensearch/remote/metadata/client/impl/LocalClusterIndicesClient.java for existing implementation |
||
| public RunAsSystemClient(Client delegate) { | ||
| super(delegate); | ||
| } | ||
|
|
||
| @Override | ||
| protected <Request extends ActionRequest, Response extends ActionResponse> void doExecute( | ||
| ActionType<Response> action, | ||
| Request request, | ||
| ActionListener<Response> actionListener | ||
| ) { | ||
| ThreadContext threadContext = threadPool().getThreadContext(); | ||
|
|
||
| try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { | ||
|
|
||
| ActionListener<Response> wrappedListener = ActionListener.wrap(r -> { | ||
| ctx.restore(); | ||
|
There was a problem hiding this comment. This is the main reason for introducing this PR, to ensure that the original context is restored when an action is completed. When the Security Plugin provides its implementation of a RunAsClient, it would inject a user corresponding to the plugin before There was a problem hiding this comment. I created a PR on my own fork of the security plugin to demonstrate how the changes would be integrated into a sample plugin: cwperks/security#40 There was a problem hiding this comment. rather than |
||
| actionListener.onResponse(r); | ||
| }, e -> { | ||
| ctx.restore(); | ||
| actionListener.onFailure(e); | ||
| }); | ||
|
|
||
| super.doExecute(action, request, wrappedListener); | ||
| } | ||
| } | ||
| } | ||
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.
When I see
noopI think it does nothing but this seems to be doing "something". Is this the right package for it?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.
This is the rationale, and sorry if there's not enough background context on the PR description.
There are 2 different scenarios this PR needs to account for:
When security is not installed, there is no IdentityPlugin and what's provided to the IdentityAwarePlugins is this
RunAsSystemClient. This client does the current system index access pattern seen across the plugins, it stashes the thread context before executing a transport action. This client will then restore back the original context before delegating back to the original actionListener.When security is installed, what would be provided is not this class, but another client defined by the security plugin. Its not introduced yet, but the code may look similar to this:
This client stashes the threadcontext, but then it injects an identity corresponding to the respective plugin that this client was assigned to. Security will use this identity to run authz checks which it does not currently do today. Currently, plugins can perform any action and are allowed to do so. The intent of this client is to allow system index access (to their own system indices) and prohibit other actions unless the cluster admin explicitly allows a plugin to perform an action outside the authenticated user context.
The one in this PR is in a package called
noopbecause there's a notion of a NoopIdentityPlugin, but I agree that the naming is confusing.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.
Plugins already get a node client through
createComponents. This particular client (the client provided inIdentityAwarePlugin.assignRunAsClient) is intended to perform operations outside the authenticated user context (in the context of the plugin if you will).I want to work towards cluster administrators knowing explicitly what actions a plugin will perform outside the authenticated user context and have the cluster administrator sign-off at installation time. Similar to JSM.
For instance, one use-case the security plugin will need facilitated is the ability to write to the audit log index if a cluster is using an opensearch index for the audit log. The security plugin needs a guarantee that writes to this index will succeed regardless of the callers permissions and it stashes the ThreadContext to do this operation today.