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 Higher-Level APIs for Plugins to switch contexts #14931

Closed
cwperks opened this issue Jul 23, 2024 · 8 comments · Fixed by #15016
Closed

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

cwperks opened this issue Jul 23, 2024 · 8 comments · Fixed by #15016
Assignees
Labels
enhancement Enhancement or improvement to existing feature or request Identity PR/Issues associated with Authentication or Authorization Plugins security Anything security related v2.17.0 v3.0.0 Issues and PRs related to version 3.0.0

Comments

@cwperks
Copy link
Member

cwperks commented Jul 23, 2024

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 cwperks added enhancement Enhancement or improvement to existing feature or request untriaged labels Jul 23, 2024
@cwperks
Copy link
Member Author

cwperks commented Jul 23, 2024

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

cwperks commented Jul 24, 2024

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
Copy link
Member Author

cwperks commented Jul 24, 2024

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 peternied added security Anything security related Identity PR/Issues associated with Authentication or Authorization and removed untriaged labels Jul 24, 2024
@peternied
Copy link
Member

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

@dblock
Copy link
Member

dblock commented Aug 12, 2024

[Catch All Triage - 1, 2, 3]

@cwperks
Copy link
Member Author

cwperks commented Aug 29, 2024

Closing this issue now that #14630 is merged.

@cwperks cwperks closed this as completed Aug 29, 2024
@reta reta added v3.0.0 Issues and PRs related to version 3.0.0 v2.17.0 labels Aug 29, 2024
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 Identity PR/Issues associated with Authentication or Authorization Plugins security Anything security related v2.17.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
4 participants