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] Deprecate the usage of ThreadContext.stashContext in plugins #2487

Open
cwperks opened this issue Feb 27, 2023 · 1 comment · May be fixed by opensearch-project/OpenSearch#17296
Open
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. v3.0.0

Comments

@cwperks
Copy link
Member

cwperks commented Feb 27, 2023

Is your feature request related to a problem?

The usage of ThreadContext.stashContext() is widely used in OpenSearch plugins to stash the current thread context and assume a "local direct" mode which escalates the plugin's privileges to perform any action on an OpenSearch cluster. Plugin developers often wrap code paths that interact with system indices to skip privilege evaluation and write documents into the plugin's own indices. When stashing the context, information that was previously populated into the threadcontext from the security plugin is removed and information about the original caller is unavailable within the new context.

Stashing the ThreadContext should be deprecated and a new way of evaluating actions that originate from plugins developed. Plugins should only be able to interact with their own system indices.


Below is a detailed trace of the code path to understand how stashing the context makes it so that plugins can assume a "local direct" mode to skip privilege evaluation.

From my understanding of how stashContext works in a block like this (Example in AD):

try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
    resolveUserAndExecute(user, detectorId, method, listener, (detector) -> adExecute(request, user, detector, context, listener));
} catch (Exception e) {
    LOG.error(e);
    listener.onFailure(e);
}

is that resolveUserAndExecute in this context will run with no threadcontext headers unless you explicitly take values out before stashing and add them back in within this block.

When a REST request hits a cluster with the security plugin installed, the security plugin wraps the REST handler to authenticate the request and upon successful authentication the security plugin will populate the threadcontext headers like this:

ThreadContext

transientHeaders:

_opendistro_security_user: <User obj>

headers:

_opendistro_security_origin: REST

then after the request is authenticated it goes from the REST-layer -> Transport-layer at which point the action filters are applied. The security plugin performs authorization at this level and within the SecurityFilter it populates another ThreadContext header with the user object serialized for plugins to read from with common-utils. That looks like:

ThreadContext

transientHeaders:

_opendistro_security_user: <User obj>

headers:

_opendistro_security_origin: REST
_opendistro_security_user_info: username|backend_role1,backend_role2 // This is a pipe-delimited serialized representation of a User

There are a few other headers the security plugin will use, but these are 3 of the major ones. When a plugin registers a new REST handler with a corresponding REST Action and Transport Action, these headers will already be populated by the security plugin when you are inside the transport action for the plugin. When plugin's need to bypass privilege evaluation they can stash the context and operate without these headers in place.

When that happens, the SecurityFilter is still applied before the transport action is executed but the SecurityFilter at that point is pass-through because the action is implicitly trusted as its considered a "local direct" request:

What solution would you like?

Actions originating from plugins should be identified and authorized accordingly. Reliance on stashing the context should be deprecated and eventually removed/forbidden.

@cwperks cwperks added enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized labels Feb 27, 2023
@stephen-crawford
Copy link
Contributor

[Triage] @cwperks would you be willing to share your thoughts about converting this into a campaign or what is needed next? Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. v3.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants