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

[Feature Request] Create a mechanism to track if the current execution is within a block where the ThreadContext has been stashed #14733

Closed
cwperks opened this issue Jul 11, 2024 · 24 comments
Labels
enhancement Enhancement or improvement to existing feature or request Plugins untriaged

Comments

@cwperks
Copy link
Member

cwperks commented Jul 11, 2024

Is your feature request related to a problem? Please describe

Related RFC in the security plugin: opensearch-project/security#4439

I would like to have a mechanism to track which plugin most recently stashed the ThreadContext. Stashing the threadContext is done in the plugin ecosystem in order to allow a plugin to interact with its system indices when the security plugin is installed.

Stashing the threadContext is analogous to running a command as sudo. A plugin that stashes its threadContext is effectively switching contexts, that is instead of running in an authenticated user context its running in an elevated context.

By being able to keep track of where the ThreadContext was most recently stashed, it would be possible to provide richer authorization mechanisms within the block where the threadContext was stashed.

Better authorization includes:

  1. Enforcing plugins only interact with their own system indices
  2. Allowing a cluster administrator to explicitly define what actions a plugin can do in a block where the plugin has switched out of the authenticated user context

Describe the solution you'd like

Have some sort of lookup to query if the current execution is within a block where the threadContext is stashed and identify the plugin that stashed the context.

Related component

Plugins

Describe alternatives you've considered

Keep the existing implementation

Additional context

Related RFC in the security plugin: opensearch-project/security#4439

@reta
Copy link
Collaborator

reta commented Jul 16, 2024

Thanks for the feature @cwperks

I would like to have a mechanism to track which plugin most recently stashed the ThreadContext. Stashing the threadContext is done in the plugin ecosystem in order to allow a plugin to interact with its system indices when the security plugin is installed.

I believe thread context stashing is used in many, many different places and is not really related to plugin, but probably more generic to "caller", it is used heavily by SecurityManager APIs and relies on jdk.internal.reflect.Reflection:

Class <?> caller = Reflection.getCallerClass();

But to the point, I believe the thread context is not the place where any authorization decisions could be made, but the SecurityManager is (sadly, we have to find the replacement for it).

-. enforcing plugins only interact with their own system indices

It could be done using the AccessController + Permission model (we know system indices <-> plugin mapping)

  1. allowing a cluster administrator to explicitly define what actions a plugin can do in a block where the plugin has switched out of the authenticated user context

This statement is unclear to me, the "authenticated user" concept is (at least for now) strictly specific to security plugin and should not leak into core anyhow. Could you please elaborate what exactly you mean by "define what actions a plugin can do in a block where the plugin has switched out of the authenticated"? Where this block come from? What is cluster administrator? How it is supposed to explicitly define actions ?

@cwperks
Copy link
Member Author

cwperks commented Jul 16, 2024

This statement is unclear to me, the "authenticated user" concept is (at least for now) strictly specific to security plugin and should not leak into core anyhow. Could you please elaborate what exactly you mean by "define what actions a plugin can do in a block where the plugin has switched out of the authenticated"? Where this block come from? What is cluster administrator? How it is supposed to explicitly define actions ?

The best analogy is that stashing the threadContext is equivalent to running a command as sudo from the existing security plugin perspective.

Effectively, a plugin has switched contexts and is allowed to perform any action on the cluster as sudo.

define what actions a plugin can do in a block where the plugin has switched out of the authenticated user context

This is forward looking outside of the stronger system index authz that I have been working on. Essentially, instead of running as sudo within the stashed context block it would be su -u <plugin>, but what permissions does it have? At first it would only be interaction with a system index, but we could create a mechanism for a cluster admin to define what the plugin can perform similar to a role definition. I believe that a cluster administrator should be empowered to declare what actions a plugin can perform and not by default give all plugins sudo privileges.


For these 2 PRs:

It will provide an off-ramp for plugins to remove usages of stashContext in favor of switchContext which is essentially identical, but it populates the threadcontext with plugin info so the security plugin can authz transport actions accordingly.

@reta
Copy link
Collaborator

reta commented Jul 16, 2024

The best analogy is that stashing the threadContext is equivalent to running a command as sudo from the existing security plugin perspective.

I think the sudo analogy is not right (for ThreadContext) and it is not serving the same purpose: it is specifically manages thread local context and only that (the fact security plugin may store the user / principal in thread context as well is implementation detail). The sudo analogy would be Subject::doAs where the identity is managed explicitly.

I believe that a cluster administrator should be empowered to declare what actions a plugin can perform and not by default give all plugins sudo privileges.

I still don't understand this, sorry, what kind of actions? We do have plugin security policy in place, plus permissions that security plugin manages. I think you have larger picture in mind but for some reasons reference to low level thread context hides it (it is just a means but not the solution).

@cwperks
Copy link
Member Author

cwperks commented Jul 16, 2024

I still don't understand this, sorry, what kind of actions?

Transport actions.

@reta
Copy link
Collaborator

reta commented Jul 16, 2024

Transport actions.

This is where we have permissions, right?

@cwperks
Copy link
Member Author

cwperks commented Jul 16, 2024

This is where we have permissions, right?

For users yes, but not when plugins stash the threadcontext.

When a plugin stashes the threadcontext, any action initiated in the block is implicitly trusted. In the security plugin, there is a bypass for this scenario so that a plugin can perform elevated actions such as writing to a system index.

@cwperks
Copy link
Member Author

cwperks commented Jul 16, 2024

Consider this pseudo code for creating an anomaly detector. Creating an anomaly detector indexes a document into the .opendistro-anomaly-detector-jobs index.

1. User makes REST call to create detector PUT /_plugins/ad/detector
2. Security plugin authenticates the request
3. After authentication, AD plugin's corresponding RestHandler's handleRequest is called
4. RestAction typically uses the node client to switch to transport layer (client.execute(Action.NAME, request, actionListener))

The steps below occur in the transport action to create a detector

5. AD stashes the threadcontext <- This is done to perform escalated (sudo) actions
    5.1. Index a document into .opendistro-anomaly-detector-jobs index with job metadata <- This is indented to indicate this is in a block where the threadcontext is stashed.

6. Respond to client success/fail

@reta
Copy link
Collaborator

reta commented Jul 16, 2024

For users yes, but not when plugins stash the threadcontext.

So it seems like the plugin have to be explicitly allowed to do that, if allowed at all, right? With that, it fits perfectly into the security policy model we have at the moment. Reject context stashing by default, allow when explicitly granted.

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2024

There could certainly be a permission entry in the plugin-security.policy file that governs whether a plugin can stash the threadcontext but why should stashing the threadcontext give a plugin the ability to perform any transport action?

@reta
Copy link
Collaborator

reta commented Jul 17, 2024

There could certainly be a permission entry in the plugin-security.policy file that governs whether a plugin can stash the threadcontext but why should stashing the threadcontext give a plugin the ability to perform any transport action?

I think transport actions and thread context are quite unrelated. To me, the proper implementation of the latter model would be to introduce identity to plugins and extensions, so we could apply the same permission model (that security plugin implements) uniformly.

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2024

That's more or less what this PR will accomplish: #14630

When switching contexts, it populates info about which plugin has switched contexts.

With the companion Security PR it demonstrates how that info would be used to more strongly protect system indices. With those 2 PRs, plugins would be limited to transport actions that directly interact with system indices registered by the respective plugin.

@reta
Copy link
Collaborator

reta commented Jul 17, 2024

I think we are still trying to "hack" the way though but not design the solution, I don't see anywhere:

  • what is plugin / extension identity?
  • how it is assigned or/and exposed?
  • how the permission model applies to plugins / extensions using its identity?

@peternied
Copy link
Member

[Triage - attendees 1 2 3]
@cwperks Thanks for creating this issue. While this proposal addresses an issue - identity's association with the thread context, this doesn't improve the posture of OpenSearch or the plugins, it puts syntactic sugar around a bad use case. For that reason we are not accepting this issue.

An alternative could be building up the identity framework so that plugin/extensions can take on its identity of itself vs the user using high level concept.

@cwperks cwperks reopened this Jul 17, 2024
@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2024

@peternied re-opening this issue to continue discussion on a solution

@peternied
Copy link
Member

@cwperks I would recommend creating an RFC to describe the problem and build consensus. This issue as filed was rejected and should stay closed.

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2024

what is plugin / extension identity?

For a plugin the identity would be the canonical class name for the class that extends Plugin.

how it is assigned or/and exposed?

It is not exposed. The PluginsService has a list of the installed plugins and is able to keep track of all installed plugins and their canonical class names.

how the permission model applies to plugins / extensions using its identity?

It does not (at least with the open PRs). When a plugin switches contexts it would only be permitted to perform actions that interact with its registered system indices. Its definitely possible to map the plugins identity to permissions to permit it to do more if there's demand for allowing plugins to perform additional actions outside of an authenticated user context.

The Open PRs provide an off-ramp, not an end state solution. There will be refactoring in plugins that need to take place. I don't want to expose the raw thread context to plugins and instead expose APIs within the PluginAwareNodeClient that can be sanitized to prevent plugins from populating certain headers.

@reta
Copy link
Collaborator

reta commented Jul 17, 2024

@peternied re-opening this issue to continue discussion on a solution

@cwperks I would like to +1 @peternied suggestion: this issue will not bring us to any solution since the problem statement is not set at the right level.

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2024

@peternied There is an open RFC: opensearch-project/security#4439

Please chime in with your thoughts.

@reta
Copy link
Collaborator

reta commented Jul 17, 2024

@peternied There is an open RFC: opensearch-project/security#4439

Again, the RFC focuses on very low level details, it is not designing (as per #14733 (comment)) any base abstractions to approach the problem and later on, suggest the APIs / implementation changes required (it looks reversed to me, sorry)

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2024

Thank you for all the feedback. I will rework the RFC to reflect the latest thinking around PluginAwareNodeClient, how its passed to plugins and what it will expose.

I've been iterating trying different ways to solve the problem so I probably started too low level. Hopefully the problem statement is clear that plugins are implicitly trusted and there should be better mechanisms in place. I've been thinking a lot about extensibility and how to simplify the plugin developer experience when working with Security and Job Scheduler while making the ecosystem more secure simultaneously.

@reta
Copy link
Collaborator

reta commented Jul 17, 2024

I've been iterating trying different ways to solve the problem so I probably started too low level. Hopefully the problem statement is clear that plugins are implicitly trusted and there should be better mechanisms in place.

Correct, so we need to define the identity model for plugins and extensions (please do not ignore them, unless we make a decision to phase them out, those are equally important and need to be represented).

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2024

Sorry if it was unclear from earlier replies. The open PRs put forward a proposal to populate a header called _plugin_execution_context which has canonical class name like org.opensearch.security.OpenSearchSecurityPlugin which could be considered the identity since its unique to a plugin. When the context is switched, this header would be populated.

While not done in that PR itself, I would also like to add higher-level APIs around the threadcontext to prevent certain headers from being populated by plugins.

Come to think of it, I may be able to move this check into an ActionFilter in core and have core provide the protection even if security is not installed.

@cwperks
Copy link
Member Author

cwperks commented Jul 17, 2024

Out-of-process extensions have a separate flow for differentiating requests made on-behalf-of the authenticated user and when the plugins extensions operate independently to interact with indices reserved for an extension. Extensions that want to use opensearch to store metadata in system indices are given a service account token during the handshake process between an extension and OpenSearch. When making requests to interact with reserved indices they will pass the service account token to authenticate and authorize the request.

Related META issue: opensearch-project/security#2944

@reta
Copy link
Collaborator

reta commented Jul 17, 2024

Related META issue: opensearch-project/security#2944

Thanks @cwperks (I forgot about that), I believe the plugins should have an similar kind of identity (or even the same kind of service account based identity), which would be granted based on the execution flow (since plugin could have user initiated flow or not).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request Plugins untriaged
Projects
None yet
Development

No branches or pull requests

3 participants