opensearch-project / OpenSearch

🔎 Open source distributed and RESTful search engine.
https://opensearch.org/docs/latest/opensearch/index/
Apache License 2.0
9.72k stars 1.8k forks source link

[Feature Request] Create Higher-Level APIs for Plugins to switch contexts #14931

Closed cwperks closed 2 months ago

cwperks commented 3 months ago

Is your feature request related to a problem? Please describe

As a plugin developer, its necessary to differentiate between performing transport actions within 1) an authenticated user context and 2) outside of an authenticated user context.

When using the OpenSearch Security plugin, plugin developers use the threadPool.getThreadContext().stashContext() method to accomplish this as the security plugin uses headers in the ThreadContext to determine whether a transport action is being executed within the context of an authenticated user or not.

stashContext allows a plugin to execute code in a fresh threadContext and the original context is restored upon closure.

stashContext is a low-level API that requires a plugin developer to have specific knowledge about the implementation of security. Ideally, there is a higher-level API that plugin developers can use to "switch contexts".

Note: Switching contexts is necessary for plugins that have system indices. Stashing the context is required before system index interaction.

Describe the solution you'd like

I am thinking about introducing a new parameter that is passed into plugin.createComponents. The new parameter would be an instance of an interface like ContextSwitcher:

public interface ContextSwitcher {
    ThreadContext.StoredContext switchContext();
}

There are 2 types of ContextSwitchers:

  1. SystemContextSwitcher -> when calling switch context is will by default call markAsSystemContext (currently a public method of ThreadContext). This would be an@Internal class and can only be used in this repo. Not by plugins.
  2. PluginContextSwitcher -> When stashing the context, this will also populate a special header (_plugin_execution_context) with the canonical class name of the class that extends Plugin. Canonical class name is chosen because its unique to every plugin and should match the key to this map.
    • This special header will have special protections to prevent it from being written to from higher-level ThreadContext APIs. A notion of FORBIDDEN_HEADERS would be introduced into the ThreadContext class which will prevent populating these headers directly.

Along with the ContextSwitcher, the ThreadContext class will be revisited to determine what level of access each method should have. For instance, the markAsSystemContext() method is currently public which means its accessible to plugins. I propose making markAsSystemContext and all stashContext methods to be package-private.

Related component

Plugins

Describe alternatives you've considered

Modify the NodeClient that is passed to plugins and expose select methods.

Additional context

stashContext nullifies all request + transient headers except for transient headers that have a registered propagator.

cwperks commented 3 months ago

@reta @peternied I opened up another issue that discusses the issue and proposed solution on a higher-level. Let me know your thoughts. Does this capture enough detail or not enough?

peternied commented 3 months ago

As a plugin developer, it's necessary to differentiate between performing transport actions within 1) an authenticated user context and 2) outside of an authenticated user context.

2 as written doesn't make sense to me, is this operating as root, an admin, or as the plugin? I think it should be as the plugin and I need clear scenarios to understand the boundaries of what we are agreeing to build.

IMO Plugins should have ways to access their system index. Can you draw out what a plugin needs to do for this second scenario, how can it be fenced in such a way that allows for access controls to be added and shaped over time.

Let's not worry about the internal implementation in core but instead focused on the plugin developer experience, what APIs can be called that make this clear.

peternied commented 3 months ago

For example, what do you think about 2 node clients that can only be used one at a time. The user client and the plugin client, and unless inside a try block you cannot using the plugin client like so:

userClient.readIndex(...); // OK
pluginClient.readIndex(...); // throws CrossClientUsageException

try (pluginClient.activate()){
   pluginClient.readIndex(...); // OK
   userClient.readIndex(...); // throws CrossClientUsageException
} // deactivates the plugin client

Note: I don't like the side effect based relationship between the clients, but it's a starting point.

What do you think?

cwperks commented 3 months ago

For example, what do you think about 2 node clients that can only be used one at a time.

Definitely, that looks nice and readable and I see a clear path to replacing threadContext.stashContext with pluginClient.activate().

In general, the access modifiers of the methods in ThreadContext need to be revisited. Many of those methods should not be public, especially the methods around stashing the thread context (stashContext, stashWithOrigin and stashAndMergeHeaders) and marking the context as system context (markAsSystemContext).

cwperks commented 3 months ago

2 as written doesn't make sense to me, is this operating as root, an admin, or as the plugin?

For plugins, it would always operate in the context of the plugin. So if ISM switches contexts, then all transport actions would need to be authorized as if ISM itself is performing the transport action.

In the core repo, there are still instances of stashing the context (example). The core repo will retain the ability to switch contexts and operate as root, but that will be restricted for plugins.

peternied commented 3 months ago

[Triage - attendees 1 2] @cwperks Thanks for creating this issue.

dblock commented 2 months ago

[Catch All Triage - 1, 2, 3]

cwperks commented 2 months ago

Closing this issue now that https://github.com/opensearch-project/OpenSearch/pull/14630 is merged.