[BUG] SecurityRestFilter drops the headers from ThreadContext

[kaushalmahi12]

What is the bug? SecurityLayer should not drop information from ThreadContext which is a opensearch construct. In current setup SecurityRestFilter drops the request headers populated in ThreadContext for a request. OpenSearch process controls the valid list of headers that can be propagated from http layer to ThreadContext by defining them inside the ActionModule. But SecurityFilter does not consider all the whitelisted headers and abruptly drops all headers except X_OPAQUE_ID.

Problematic code line: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java#L139C13-L142C19

How can one reproduce the bug? Steps to reproduce the behavior:

1. Make a request with a custom http header which is defined in ActionModule curl -X GET https://localhost:9200/_search -u 'admin:passwd123456789@' --insecure -H 'queryGroupId: 9oguoImmRMy1qYe2M6W3dA'
2. try to consume this header from ThreadContext anywhere starting from your RestHandler (RestSearchAction)

What is the expected behavior? Security plugin should retain all whitelisted headers.

What is your host/environment?

• OS: [e.g. iOS]
• Version [e.g. 22]
• Plugins

Do you have any screenshots? If applicable, add screenshots to help explain your problem.

Do you have any additional context? Add any other context about the problem.

[kkhatua]

@cwperks would it make sense to allow additional headers be supported instead of just X_OPAQUE_ID ?

[cwperks]

@cwperks would it make sense to allow additional headers be supported instead of just X_OPAQUE_ID ?

Yes definitely, this is a bug.

These are my initial thoughts, I will analyze this issue further and provide more updates later today.

A couple of options:

1. Look into the usage of persistent TC headers for the request headers that are copied to the thread context. Persistent headers are not stashable

   • I was unfamiliar with the ActionPlugin.getRestHeaders() extension point before this issue was filed and tbh I think it should be deprecated in its current implementation. The problem with this extension point is that there is no concept of forbidden headers, so any plugin can register a header to get copied to the ThreadContext. It just so happens that no plugins in the default distribution of OpenSearch implement this extension point. Based on the PR that introduced the extension point it looks like it may have been used by x-pack security at one point and predates the fork. For a contrived example of why this extension point can be problematic, a plugin can specify a known TC header that would be populated downstream of here. If someone creates a request with this header, there's a risk of a runtime exception when handling the request because a header can only be set once

2. Feed the list of headers from core -> Security plugin so that security plugin can restore the headers in the SecurityRestFilter. This line in ActionModule is specific to the security plugin , we could either look at modifying the existing extension point (unlikely since its breaking change) or introduce similar extension point to feed the headers to the same plugin that implements the getRestHandlerWrapper

3. (Preferred) Make this area of the security plugin more general. Before restoring the context, 1) temporarily get all headers using threadContext.getHeaders(), 2) restore previous context from header_verifier, 3) copy the headers back into new context

   • I can raise a PR to explore this approach

[cwperks]

For additional context, I think this is a bug introduced in 2.11 but it was not noticed since the only headerToCopy has been X-Opaque-Id until now.

The change that the Security plugin introduced in 2.11 was to move auth further up the netty pipeline before decompression: https://github.com/opensearch-project/OpenSearch/pull/10261

The reason that security has to restore a previous context is that core has a call to stashContext in the last handler of the netty pipeline here. Before 2.11, security would authenticate a request after the call to stashContext and in the process of authenticating the request it would populate a bunch of TC headers including transient headers like the object corresponding to the authenticated user and other information such as the IP address where the request originated.

After authentication was moved up the netty pipeline, security would populate the TC before the call to stashContext from above. To get around this issue the security plugin uses netty channel attributes to carry the context down the pipeline and restore it in the SecurityRestFilter.

[kaushalmahi12]

Thanks @cwperks! for suggesting the approaches. I agree the change in extension point is intrusive and causes wider compatibility issues. I think approach 3 seems to be best so far since it implicitly leverages http server layer trust for valid headers hence avoid the dependency on core to get whitelisted headers.

[cwperks]

If Option 3 is implemented, then there should be a corresponding integTest that asserts the correct behavior. Writing an integ test will take a little more effort, but I think I can dig up some examples. IMO for an integ test we should create an EchoPlugin that creates a REST Api that returns the list of headersToCopy in the response. headersToCopy should include X-Opaque-Id, queryGroupId and any header registered by an ActionPlugin with getRestHeaders() extension point. The test can create a REST Request and pass in a header to copy and verify its contained in the output of the RestResponse.