Add runAs(Subject subject) to Client interface

[cwperks]

Description

Opening up this PR for discussion about a change in the interface that was introduced to formalize how plugins should interact with their own System Indices.

In the previous PR, there was a concept of a PluginSubject that was introduced that was assigned to IdentityAwarePlugins that could be used as a drop in replacement for try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { ... } which is the pattern prevalently used for programmatic system index access.

There was discussion on that PR against introducing a separate client to make calls that execute actions in the context of the plugin's identity vs the authenticated user context. i.e. stashContext is a method to effectively switch contexts where plugins behave as the system and run without authz checks which allows access to a system index. There is an effort to put stronger mechanisms in place to perform authz checks when plugins switch context to better sandbox plugins and empower system administrators with information at installation-time with access that a plugin needs to operate normally.

Opening up this PR in response to a review comment that brings up reasons to pursue a solution with a separate client. This PR creates a subclass of FilterClient (called RunAsClient) that stashes the context prior to execution and action and restoring the original context before delegating back to the corresponding ActionListener's onResponse or onFailure method.

Related Issues

Related to opensearch-project/security#4439

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[cwperks]

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 doExecute and restore the original context (including authenticated user info) before calling the original actionListener's onResponse or onFailure.

[cwperks]

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

[dbwiddis]

rather than ActionListener.wrap( ... restore; onResponse ... restore; onFailure ) why not just use ActionListener.runBefore(listener, () -> context.restore()) (or better yet ActionListener.runBefore(listener, context::restore))?

[github-actions]

❌ Gradle check result for 2765e88: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❕ Gradle check result for 5415ab3: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[cwperks]

@nibix @dbwiddis @reta @derek-ho @DarshitChanpura Thank you all for engaging on this PR. I greatly simplified this now to keep the existing IdentityPlugin and IdentityAwarePlugin interfaces in tact.

This now introduces a new method on Client called runAs that accepts a Subject and returns back a client with the doExecute method overridden.

I think this both addresses @nibix' concerns about restoring the context after the action is completed and @reta's concerns about proliferation of clients.

The caller of this method can pass the subject that is assigned to IdentityAwarePlugins from the IdentityAwarePlugin.assignPluginSubject call.

[reta]

@reta I'm busy writing tests, but what do you think of this PR in its current form where it adds a new method to the Client interface?

Client runAs(Subject subject);

@cwperks I don't understand why we need that:

• we designed Subject::runAs to solve exactly such kind of problems
• Subject::runAs also goes way beyond the just Client but has much larger scope
• combined with the possibility to inject a wrapper (and creation of FilterClient), it should give the security plugin all the levers to stash / restore context / reject the calls without subject

[cwperks]

@reta I'm busy writing tests, but what do you think of this PR in its current form where it adds a new method to the Client interface?
Client runAs(Subject subject);

@cwperks I don't understand why we need that:

• we designed Subject::runAs to solve exactly such kind of problems
• Subject::runAs also goes way beyond the just Client but has much larger scope
• combined with the possibility to inject a wrapper (and creation of FilterClient), it should give the security plugin all the levers to stash / restore context / reject the calls without subject

Can you elaborate on injecting a wrapper?

I opened this PR because of the concerns raised here about not restoring the original context on completion of a transport action. If a plugin needs to restore the context, they need to first call StoredContext ctx = threadContext.newStoredContext(false) and then restore later with ctx.restore(). By calling the method introduced in this PR, all thread context operations would be abstracted away from plugin developers.

[reta]

Can you elaborate on injecting a wrapper?

Sure, mentioned it here: #16976 (comment). The idea is to have the ClientWrapper, similarly to getRestHandlerWrapper, roughly:

    default UnaryOperator<Client> getClientWrapper(ThreadContext threadContext) {
        return null;
    }

The core, if getClientWrapper is provided by only one plugin, will create FilterClient with the wrapper in the mix.

[cwperks]

Can you elaborate on injecting a wrapper?

Sure, mentioned it here: #16976 (comment). The idea is to have the ClientWrapper, similarly to getRestHandlerWrapper, roughly:

    default UnaryOperator<Client> getClientWrapper(ThreadContext threadContext) {
        return null;
    }

The core, if getClientWrapper is provided by only one plugin, will create FilterClient with the wrapper in the mix.

But how does a plugin dev select which mode to use?

i.e. run this TransportAction in the context of the authenticated user (w/o stashing the context) vs. running the transport action in the system (plugin-level) context that permits the plugin system index acces?

[cwperks]

One of the goals is to make this specific to each IdentityAwarePlugin installed in the cluster as well, there would need to be a FilterClient for each IdentityAwarePlugin.

One of the goals of this change is better plugin isolation.

PluginA should be able to access its own system indices, but not system indices of other plugins.

For instance, other plugins of the cluster should not be able to delete the security index or write directly to it.

[reta]

But how does a plugin dev select which mode to use?

It does not need to - it will have to: the call to access plugin system index should fail with an error that the call has to be executed with PluginSubject::runAs context.

[cwperks]

ok, I will close this PR then. I raised this PR because @nibix had some concerns about that model: opensearch-project/security#4896 (comment) and how providing a more convenient mechanism that restores the original context would be ideal.

I think the changes in this PR would provide a nice plugin developer experience (while also supporting pluginSubject.runAs(() -> { ... })) as well if plugin devs would prefer to wrap a lot of action calls at once.

With the changes in this PR, a plugin dev would just add on .runAs(pluginSubject) when accessing a system index.

i.e.

client().runAs(pluginSubject).index(new IndexRequest(".system-index"), new ActionListener<>() { ... }

[github-actions]

✅ Gradle check result for 0728709: SUCCESS

[nibix]

@reta

• we designed Subject::runAs to solve exactly such kind of problems

If you refer to the sync runAs() method: <T> T runAs(Callable<T> callable). This will leak the plugin user context to the action listener provided for the respective action the runAs() call is executed in. Thus, any code which calls an action which uses this method and calls the action listener inside will suddenly execute any further actions as the plugin user.

I am quite sure that a solution to the problem needs to be aware of action listeners.

[reta]

Thus, any code which calls an action which uses this method and calls the action listener inside will suddenly execute any further actions as the plugin user.

@nibix I would argue this is not a leak, this is what "run as" means: execute any further actions as the Subject (plugin or system or user).

[nibix]

Any further actions called by the caller of the action that executes the run as. This can cross plugin boundaries.

[nibix]

For example:

• Plugin B defines an action PluginBAction and uses runAs() to run another action as its plugin user

 protected void doExecute(Task task, PluginBRequest request, ActionListener<PluginBResponse> actionListener) { 
                 this.pluginUser.runAs(() -> { 
                        nodeClient.search(new SearchRequest(), new ActionListener() {
                            public void onSuccess(r) {
                                 // Here, the plugin user thread context is still active. There is no way to reset it to the original thread context. Thus, the action listener (possibly from another plugin) will also execute with the plugin user thread context.
                                 actionListener.onSuccess(r);
                            }
                         });
                     }
                     ); 

}

• Plugin A calls an action from plugin B

        client.execute(PluginBAction.INSTANCE; new PluginBRequest(), new ActionListener() {
            public void onSuccess(r) {
                // Now plugin A runs in the context of plugin B. The following search request will be executed as plugin B user
               client.search(...)
            }
       );

[cwperks]

For example:

• Plugin B defines an action PluginBAction and uses runAs() to run another action as its plugin user

 protected void doExecute(Task task, PluginBRequest request, ActionListener<PluginBResponse> actionListener) { 
                 this.pluginUser.runAs(() -> { 
                        nodeClient.search(new SearchRequest(), new ActionListener() {
                            public void onSuccess(r) {
                                 // Here, the plugin user thread context is still active. There is no way to reset it to the original thread context. Thus, the action listener (possibly from another plugin) will also execute with the plugin user thread context.
                                 actionListener.onSuccess(r);
                            }
                         });
                     }
                     ); 

}

• Plugin A calls an action from plugin B

        client.execute(PluginBAction.INSTANCE; new PluginBRequest(), new ActionListener() {
            public void onSuccess(r) {
                // Now plugin A runs in the context of plugin B. The following search request will be executed as plugin B user
               client.search(...)
            }
       );

This is the existing case with ThreadContext.stashContext too correct? If the context is not explicitly restored, then any subsequent action is run in the fresh context (lacking headers needed to perform authz checks)

Edit: Are there instances of plugins calling transport actions that are defined in another plugin?

[reta]

Got it, thanks @nibix , that is again raises the question - sometimes this is desired behaviour, sometimes it is not, the caller could make this choice, for example, we could have SubjectPreservingActionListener for such cases and never actually propagate the subject by default to any other thread (we have a number of specialized ActionListener implementations):

 protected void doExecute(Task task, PluginBRequest request, ActionListener<PluginBResponse> actionListener) { 
                 this.pluginUser.runAs(() -> { 
                        nodeClient.search(new SearchRequest(), new SubjectPreservingActionListener() {
                            public void onSuccess(r) {
                                 // Here, the plugin user thread context is still active. There is no way to reset it to the original thread context. Thus, the action listener (possibly from another plugin) will also execute with the plugin user thread context.
                                 actionListener.onSuccess(r);
                            }
                         });
                     }
                     ); 

}

[nibix]

@cwperks

This is the existing case with ThreadContext.stashContext too correct? If the context is not explicitly restored, then any subsequent action is run in the fresh context (lacking headers needed to perform authz checks)

Sorry, can you rephrase the question?

Edit: Are there instances of plugins calling transport actions that are defined in another plugin?

Well, there is nothing that prevents this scenario. There's quite a lot of reusable functionality also organized as separate plugins (modules), for example the geospatial features.

@reta

• sometimes this is desired behaviour, sometimes it is not

For the call to the onSuccess/onFailure methods of the calling action listener, I would argue that it will be never desired behavior to keep the plugin thread context.

The only cases where this would be the case when there shall be further transport actions to be executed by the plugin itself. However, this could be easily solved with the client solution. This would make it also very clear whether a transport action is executed in a "normal" thread context or in the plugin user thread context.

[reta]

For the call to the onSuccess/onFailure methods of the calling action listener, I would argue that it will be never desired behavior to keep the plugin thread context.

So this is what should happen - the subject should never cross the thread boundaries by default and the core (with security plugin help) is in full control of it.

[cwperks]

Sorry, can you rephrase the question?

From the example you posted, the same problem is the case if you replace pluginSubject.runAs(() -> { ... }) w/ Thread context stashing.

i.e.

 protected void doExecute(Task task, PluginBRequest request, ActionListener<PluginBResponse> actionListener) { 
                 try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { 
                        nodeClient.search(new SearchRequest(), new ActionListener() {
                            public void onSuccess(r) {
                                 // Here, the thread context is still stashed. If a plugin dev doesn't explicitly call ctx.restore() here then the original context would not be propagated to the actionListener
                                 actionListener.onSuccess(r);
                            }
                         });
                     }
                     ); 

}

[nibix]

@cwperks

Yes, that's why it is essential to call ctx.restore() in the action listener. The API docs contain a warning about that

OpenSearch/server/src/main/java/org/opensearch/common/util/concurrent/ThreadContext.java

Lines 198 to 205 in f9c239d

	/**
	* Removes the current context and resets a default context marked with as
	* originating from the supplied string. The removed context can be
	* restored by closing the returned {@link StoredContext}. Callers should
	* be careful to save the current context before calling this method and
	* restore it any listeners, likely with
	* {@link ContextPreservingActionListener}. Use {@link OriginSettingClient}
	* which can be used to do this automatically.

But it is clear that is issue is non-obvious and often overlooked, despite the warning. As we are talking about security critical stuff, I would recommend to seek for solutions that are less easy to get wrong.

[nibix]

@reta

So this is what should happen - the subject should never cross the thread boundaries by default and the core (with security plugin help) is in full control of it.

Due to the async architecture, it must be able to cross thread boundaries. But it must not propagate back to a action listener onResult/onFailure call.

sequenceDiagram
Plugin A->>Plugin B: "client.execute(PluginBAction.INSTANCE)"
Plugin B->>Core: "stash thread context, set as plugin subject<br>client.execute(SearchAction.INSTANCE)"
Core->>Plugin B: "actionListener.onSuccess()<br>// thread context is still set to subject of plugin B"
Plugin B->>Plugin A: "actionListener.onSuccess()<br>// Before calling onSuccess(), Plugin B MUST make sure that the original thread context is restored"

Loading

[reta]

But it must not propagate back to a action listener onResult/onFailure call.

Sure, this is why we have a hierarchy of ActionListener, having opt-out from Subject propagation by default would make sense, ability to opt-in with SubjectPreservingActionListener should work (similarly to other variations of ActionListener)