Ensure all restHeaders from ActionPlugin.getRestHeaders are carried to threadContext for tracing #5396

cwperks · 2025-06-13T17:48:34Z

Description

This PR ensures that allowlisted headers from ActionPlugin.getRestHeaders() are carried from HTTP Headers -> ThreadContext headers in order to trace a request end-to-end through the cluster.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Bug fix

Issues Resolved

Resolves #4799

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
Commits are signed per the DCO using --signoff

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.

…o threadContext for tracing Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2025-06-13T17:50:57Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

+                requestHeadersToCopy,
+                getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REQUEST_HEADERS).split(",")
+            );
+            requestHeadersToCopy.remove(Task.X_OPAQUE_ID); // Special case where this header is preserved during stashContext.


The changes in this file are necessary because currently only X_OPAQUE_ID is not lost when stashing here: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/common/util/concurrent/ThreadContext.java#L157-L163

Ideally, this is solved another way through the core. Either:

Change X_OPAQUE_ID to a persistent header and remove the logic in stashContext. persistentHeaders are a special category of headers that are not stashable.

Update the logic in ThreadContext to not stash any of the headers from https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/action/ActionModule.java#L581-L586

IMO, option 1 is better.

cwperks · 2025-06-13T17:52:02Z

@sgup432 This PR would need additional tests, but this is what the general changes could look like if just done within the confines of the security plugin.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

codecov · 2025-06-20T22:18:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.77%. Comparing base (a581bc6) to head (560507d).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5396   +/-   ##
=======================================
  Coverage   72.76%   72.77%           
=======================================
  Files         397      397           
  Lines       24591    24606   +15     
  Branches     3741     3747    +6     
=======================================
+ Hits        17894    17906   +12     
- Misses       4867     4872    +5     
+ Partials     1830     1828    -2

Files with missing lines	Coverage Δ
...opensearch/security/filter/SecurityRestFilter.java	`85.40% <100.00%> (+0.32%)`	⬆️
...g/opensearch/security/support/ConfigConstants.java	`95.23% <ø> (ø)`
...search/security/transport/SecurityInterceptor.java	`77.10% <100.00%> (+1.00%)`	⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cwperks · 2025-06-23T17:44:23Z

src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java

@@ -130,7 +131,8 @@ public void testShouldAllowAtRestAndBlockAtTransport() {
            // granted at Rest layer
            auditLogsRule.assertExactlyOne(privilegePredicateRESTLayer(GRANTED_PRIVILEGES, REST_ONLY, GET, "/" + PROTECTED_API));
            // missing at Transport layer
-            auditLogsRule.assertExactlyOne(
+            auditLogsRule.assertExactlyScanAll(


It was tricky fixing the tests in this file. The reason these started to fail is because of the logic here to separate REST-Layer audit logs from Transport-Layer audit logs. We should find a more robust way to handle this and aim to have robust assertions in the tests.

Why scan all to only evaluate 1?

All of these assertions need to be revisited after this bugfix.

From what I've gathered, the test audit log tries to split messages into 2 distinct categories:

REST-Layer messages

Transport-Layer messages

The previous logic to sort these was not working properly and relied on the present of audit_transport_headers to determine if the message was from the transport-layer.

I'm using this method here because it was the only appropriate one I found, but I think the right fix is either not to separate the messages into each category or to make sure the logic to separate the messages is correct.

okay, would you mind opening an issue (good-first issue?) to address this in future.

DarshitChanpura · 2025-06-24T18:46:16Z

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

-                if (xOpaqueId != null) {
-                    threadContext.putHeader(Task.X_OPAQUE_ID, xOpaqueId);
+                for (Map.Entry<String, String> header : tmpHeaders.entrySet()) {
+                    threadContext.putHeader(header.getKey(), header.getValue());


qq, will values be null for any header?

no, null is not possible. There could be empty

does this need specific headers or all headers? Copying/populating all headers might be costly?

+1 Could this cause perf regression?

See the linked issue: #4799

There is a bug with ActionPlugin.getRestHeaders() where the headers are getting dropped because the security plugin is not carrying them from rest -> transport and then also propagating them internally in the transport layer.

Don't think this would have a high performance penalty.

DarshitChanpura · 2025-06-24T18:48:04Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

+                requestHeadersToCopy,
+                getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REQUEST_HEADERS).split(",")
+            );
+            requestHeadersToCopy.remove(Task.X_OPAQUE_ID); // Special case where this header is preserved during stashContext.


IMO, option 1 is better.

DarshitChanpura · 2025-06-24T18:49:04Z

src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java

@@ -130,7 +131,8 @@ public void testShouldAllowAtRestAndBlockAtTransport() {
            // granted at Rest layer
            auditLogsRule.assertExactlyOne(privilegePredicateRESTLayer(GRANTED_PRIVILEGES, REST_ONLY, GET, "/" + PROTECTED_API));
            // missing at Transport layer
-            auditLogsRule.assertExactlyOne(
+            auditLogsRule.assertExactlyScanAll(


Why scan all to only evaluate 1?

src/main/java/org/opensearch/security/support/ConfigConstants.java

nagarajg17 · 2025-06-25T19:08:51Z

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

-                if (xOpaqueId != null) {
-                    threadContext.putHeader(Task.X_OPAQUE_ID, xOpaqueId);
+                for (Map.Entry<String, String> header : tmpHeaders.entrySet()) {
+                    threadContext.putHeader(header.getKey(), header.getValue());


+1 Could this cause perf regression?

nagarajg17 · 2025-06-25T19:13:01Z

CHANGELOG.md

@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Create a mechanism for plugins to explicitly declare actions they need to perform with their assigned PluginSubject ([#5341](https://github.com/opensearch-project/security/pull/5341))

 ### Changed
+- Ensure all restHeaders from ActionPlugin.getRestHeaders are carried to threadContext for tracing ([#5396](https://github.com/opensearch-project/security/pull/5396))


What is the motivation for this change? Without this how hard is it be debug?

See the issue as described here: #4799

WLM (workload management) found this issue bc they have logic to tag queries and assign them to groups. It was noticed that after overridding ActionPlugin.getRestHeaders() the the headers they expected to trace around with the entire request handling process (i.e. from rest down to transport and carried node-to-node) were getting dropped bc of logic in the security repo to control what headers are sent internally. Not all HTTP Headers should be copied to the threadcontext and carried around, but the one's explicitly allowed via ActionPlugin.getRestHeaders() should be.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Ensure all restHeaders from ActionPlugin.getRestHeaders are carried t…

fed6098

…o threadContext for tracing Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks commented Jun 13, 2025

View reviewed changes

cwperks added 3 commits June 13, 2025 13:52

Add a CHANGELOG entry

f3e7f9d

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into fix-rest-headers

bb90012

Start to fix failing tests

2a34997

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks marked this pull request as ready for review June 23, 2025 17:43

cwperks requested review from DarshitChanpura, derek-ho, nibix, peternied, RyanL1997, reta, shikharj05 and willyborankin as code owners June 23, 2025 17:43

cwperks commented Jun 23, 2025

View reviewed changes

DarshitChanpura reviewed Jun 24, 2025

View reviewed changes

shikharj05 reviewed Jun 25, 2025

View reviewed changes

src/main/java/org/opensearch/security/support/ConfigConstants.java Outdated Show resolved Hide resolved

nagarajg17 reviewed Jun 25, 2025

View reviewed changes

cwperks and others added 5 commits July 20, 2025 20:37

Merge branch 'main' into fix-rest-headers

08f641d

Assert entire audit log

3aee8b1

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Rename to OpenSearch

a3183b1

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Fix assertion

243093e

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into fix-rest-headers

560507d

DarshitChanpura approved these changes Jul 24, 2025

View reviewed changes

nagarajg17 approved these changes Jul 25, 2025

View reviewed changes

Ensure all restHeaders from ActionPlugin.getRestHeaders are carried to threadContext for tracing #5396

Are you sure you want to change the base?

Ensure all restHeaders from ActionPlugin.getRestHeaders are carried to threadContext for tracing #5396

Uh oh!

Conversation

cwperks commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks commented Jun 13, 2025

Uh oh!

codecov bot commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cwperks commented Jun 13, 2025 •

edited

Loading

codecov bot commented Jun 20, 2025 •

edited

Loading