5990: Fix security plugin issue

[Deepti24]

Description

We are trying to fix: opensearch-project/security#5990
We need to pass transients that should be copied. This change addresses that

Related Issues

Resolves #[Issue number to be closed when this PR is merged]
opensearch-project/security#5990
Testing was done as follows:

• Open search was built with these changes using ./gradlew :distribution:archives:linux-arm64-tar:assemble -x test
• Security plugin was build such that is uses changes published by above ./gradlew assemble -Dopensearch.version=3.5.0-SNAPSHOT -Dbuild.snapshot=true -x test
• Then it was run and traces were validated.

A couple of points in PR:

• A new method getTransients is added to ActionPlugin class. We could have reused headers but felt like it would be unclear which fields are transients to recover in security plugin within headers map. Also, responsibility wise both looked different. Do let me know if my understanding is incorrect
• Ideally OTelTelemetryPlugin should implements getTransients method but it does not extend ActionPlugin currently. Do let me know if modification should be made there appropriately
• Change to ThreadContextBasedTracerContextStorage.java was made as a bug was identified in it where SpanReference was not null but span within it was null. Due to this, security plugin would propagate incorrect empty span reference. Hence, added this check to avoid that. Will add screenshot of same in this PR to highlight where we observed this

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b06469a)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Fix null span check in ThreadContextBasedTracerContextStorage Relevant files: server/src/main/java/org/opensearch/telemetry/tracing/ThreadContextBasedTracerContextStorage.java server/src/test/java/org/opensearch/telemetry/tracing/ThreadContextBasedTracerContextStorageTests.java Sub-PR theme: Add transients propagation support to ActionPlugin and ActionModule Relevant files: server/src/main/java/org/opensearch/action/ActionModule.java server/src/main/java/org/opensearch/plugins/ActionPlugin.java server/src/test/java/org/opensearch/action/ActionModuleTests.java

⚡ Recommended focus areas for review Deprecated API The new 3-argument getRestHandlerWrapper method delegates to the deprecated 2-argument version by default, which in turn delegates to the 1-argument version. This chain means the headersToCopy and transients parameters are silently ignored in the default implementation. Plugins that only override the 2-argument version will also silently ignore transients. The deprecation chain should be clearly documented and validated. default UnaryOperator<RestHandler> getRestHandlerWrapper( ThreadContext threadContext, Set<RestHeaderDefinition> headersToCopy, Set<String> transients ) { return this.getRestHandlerWrapper(threadContext); } @Deprecated(forRemoval = true) default UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext, Set<RestHeaderDefinition> headersToCopy) { return this.getRestHandlerWrapper(threadContext); } Raw Array Type The test uses a raw array type Set<String>[] receivedTransients = new Set[1] to capture values from a lambda, which generates an unchecked cast warning. Consider using an AtomicReference<Set<String>> instead for type safety. final Set<String>[] receivedTransients = new Set[1]; Missing Assertion The test testTransientsCollectedFromPluginsAndCore verifies that receivedTransients[0] is not null and contains expected values, but does not assert that the ActionModule was constructed successfully or that the RestController is properly wired. If the plugin's getRestHandlerWrapper is never called (e.g., due to a code path change), receivedTransients[0] would be null and the test would fail with a misleading message rather than pointing to the root cause. assertNotNull("Plugin should have received transients", receivedTransients[0]); assertTrue("Should contain custom transient from plugin", receivedTransients[0].contains("custom_transient")); assertTrue("Should contain core CURRENT_SPAN", receivedTransients[0].contains(TracerContextStorage.CURRENT_SPAN));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b06469a
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect delegation losing headers and transients The new 3-argument getRestHandlerWrapper delegates to the 1-argument version, completely ignoring headersToCopy and transients. This means any plugin that only overrides the 2-argument version will also silently lose the transients parameter. The default should delegate to the 2-argument version to preserve backward compatibility and pass the headers correctly. server/src/main/java/org/opensearch/plugins/ActionPlugin.java [159-165] default UnaryOperator<RestHandler> getRestHandlerWrapper( ThreadContext threadContext, Set<RestHeaderDefinition> headersToCopy, Set<String> transients ) { - return this.getRestHandlerWrapper(threadContext); + return this.getRestHandlerWrapper(threadContext, headersToCopy); } Suggestion importance[1-10]: 8 __ Why: The new 3-argument getRestHandlerWrapper delegates to the 1-argument version, skipping headersToCopy entirely. It should delegate to the 2-argument version to preserve backward compatibility and correctly pass headersToCopy. This is a real bug in the default implementation chain.	Medium
General	Replace raw array with type-safe reference holder Using a raw Set array (new Set[1]) causes an unchecked cast warning and is not type-safe. Consider using an AtomicReference<Set> instead, which is also more idiomatic for capturing values from lambdas/anonymous classes. server/src/test/java/org/opensearch/action/ActionModuleTests.java [287] -final Set<String>[] receivedTransients = new Set[1]; +final java.util.concurrent.atomic.AtomicReference<Set<String>> receivedTransients = new java.util.concurrent.atomic.AtomicReference<>(); Suggestion importance[1-10]: 3 __ Why: Using a raw Set[] array is a minor code quality issue in test code. While AtomicReference is more idiomatic, this is a low-impact style improvement that doesn't affect correctness or functionality.	Low

---

Previous suggestions

Suggestions up to commit 25fdcdc

Category	Suggestion	Impact
Possible issue	Fix incorrect delegation in overloaded method The new getRestHandlerWrapper overload with transients falls back to calling getRestHandlerWrapper(threadContext), which skips the headersToCopy parameter. This means plugins that override the two-parameter version (with headersToCopy) will not have their implementation called when the three-parameter version is invoked. The fallback should delegate to getRestHandlerWrapper(threadContext, headersToCopy) to preserve existing behavior. server/src/main/java/org/opensearch/plugins/ActionPlugin.java [159-161] default UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext, Set<RestHeaderDefinition> headersToCopy, Set<String> transients) { - return this.getRestHandlerWrapper(threadContext); + return this.getRestHandlerWrapper(threadContext, headersToCopy); } Suggestion importance[1-10]: 8 __ Why: The new three-parameter getRestHandlerWrapper delegates to getRestHandlerWrapper(threadContext) instead of getRestHandlerWrapper(threadContext, headersToCopy), which would skip the headersToCopy parameter for plugins that override the two-parameter version. This is a real behavioral bug in the delegation chain.	Medium
General	Ensure thread pool is always shut down If an exception is thrown before the try block containing threadPool.shutdown(), the thread pool will not be shut down. The threadPool should be shut down in a finally block that wraps the entire test body, not just the ActionModule construction. Consider restructuring so threadPool.shutdown() is always called regardless of where an exception occurs. server/src/test/java/org/opensearch/action/ActionModuleTests.java [331-336] -} catch (IOException e) { - throw new RuntimeException(e); -} -finally { +} finally { threadPool.shutdown(); } Suggestion importance[1-10]: 4 __ Why: The threadPool is initialized before the try block, so if an exception occurs before threadPool.shutdown() in the finally block, it would still be called since the finally is at the same level. However, the suggestion's improved_code doesn't actually show a meaningful structural change, making it marginally useful.	Low
	Move assertions inside try block The assertions are placed outside the try block but before finally, meaning if ActionModule construction throws an exception, the assertions are skipped and the test may pass silently without verifying the expected behavior. Move the assertions inside the try block to ensure they are always executed when the module is successfully created. server/src/test/java/org/opensearch/action/ActionModuleTests.java [326-330] +actionModule.getRestController(); // trigger initialization if needed +// Verify transients were passed to the plugin assertNotNull("Plugin should have received transients", receivedTransients[0]); assertTrue("Should contain custom transient from plugin", receivedTransients[0].contains("custom_transient")); assertTrue("Should contain core CURRENT_SPAN", receivedTransients[0].contains(TracerContextStorage.CURRENT_SPAN)); Suggestion importance[1-10]: 3 __ Why: The assertions are already inside the try block in the actual code (lines 326-330 are within the try that starts at line 285), so the concern about assertions being skipped is not accurate. The improved_code also adds an unrelated actionModule.getRestController() call that isn't clearly necessary.	Low

[github-actions]

❌ Gradle check result for 25fdcdc: 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]

Persistent review updated to latest commit b06469a

[github-actions]

❌ Gradle check result for b06469a: 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?

[Deepti24]

/check

[cwperks]

@Deepti24 I think we can simplify the changes. I believe cwperks#345 would solve the issue.

If I understand correctly, the issue is specifically on transient headers that have registered propagators. The code correctly handles the case where these headers are propagated from parent context -> stashed context but there is an issue on the restore workflow. In the PR I linked to above it modifies the restore logic to check for the transients with propagators from within the stashed context and ensures they are propagated back to the parent context.

I think that would be all the needed changes too because CURRENT_SPAN already had a thread context propagator defined in ThreadContextBasedTracerContextStorage

[Deepti24]

@cwperks Yes this should work but just curious we could have done same for rest headers as well ? Just trying to understand why did we go with headersToCopy approach there ?
class SecurityRestFilter-> headersToCopy

[cwperks]

@cwperks Yes this should work but just curious we could have done same for rest headers as well ? Just trying to understand why did we go with headersToCopy approach there ? class SecurityRestFilter-> headersToCopy

There's a few different types of ThreadContext headers:

1. (Request) headers - These are plain String values
2. Transient headers - These are Java Object values
3. Persistent headers - These are like 1), but cannot be stashed

headersToCopy predates persistent headers and was done specifically to carry X-Opaque-Id from parent to child context. This is a special request id to identify requests coming from Kibana/OSD as I understand it.

Its since moved on to allowing more headers and actually allows plugins to also define headers (1) to copy when stashing the context.

btw stashing the context is done in cases where the system needs to change from user-context -> system context for performing system operations like reading/writing to a system index.

I definitely think there could be cleanup in this area and would even start to think about removal of headersToCopy and the extension point ActionPlugin.getRestHeaders() in favor of using persistent headers.

[Deepti24]

Thanks for the detailed response @cwperks
Just one more observation, I guess there is some other issue due to which a span reference is being put with null span as seen in the screenshot.
Will try to debug where is such span coming from. Basically value of current_span in thread local and original context are different somehow. Tried testing with your code changes (but anyways this is independent of code changes, have seen this before as well)

[reta]

@Deepti24 this change is unclear to me: the span, if propagated, should be set in thread context (transient headers), there should be no custom logic involved (ideally) to carry it forward

[Deepti24]

Hey @reta , @cwperks has opened a separate PR to fix this issue in thread propagation itself : https://github.com/cwperks/OpenSearch/pull/345/changes
I think that would be smaller fix for this

But to explain the issue, there is a bug in security plugin (due to how restore and stash Context work).
So I tried to follow the pattern of how headersToCopy are used, tried doing same for transientsToCopy. Then would use that in security plugin as mentioned below:
https://github.com/opensearch-project/security/pull/6000/changes
But yes, ideally we should follow the pattern you mentioned.

[cwperks]

@reta there's a bug on the restore path described here: #20822 (comment)

The transient headers are correctly propagating from parent tc -> stashed tc, but not from stashed tc -> restored parent tc.

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.