From 6a9a2d44dad1373493e04902536ee29dad5e54c2 Mon Sep 17 00:00:00 2001 From: Deepti Chauhan Date: Thu, 5 Mar 2026 20:50:34 +0530 Subject: [PATCH 1/5] 20517: Fix for security plugin not working Signed-off-by: Deepti Chauhan Signed-off-by: Deepti24 --- .../security/filter/SecurityRestFilter.java | 5 +++ .../filter/SecurityRestFilterUnitTests.java | 35 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 210d3f20c5..69ea54edb0 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -71,6 +71,7 @@ import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.HTTPHelper; import org.opensearch.security.user.User; +import org.opensearch.telemetry.tracing.TracerContextStorage; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.client.node.NodeClient; @@ -159,6 +160,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c tmpHeaders.put(header.getName(), value); } } + final Object currentSpan = threadContext.getTransient(TracerContextStorage.CURRENT_SPAN); storedContext.restore(); if (tmpHeaders != null) { @@ -167,6 +169,9 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } threadContext.putHeader(OPENSEARCH_SECURITY_REQUEST_HEADERS, String.join(",", tmpHeaders.keySet())); } + if (currentSpan != null && threadContext.getTransient(TracerContextStorage.CURRENT_SPAN) == null) { + threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, currentSpan); + } }); NettyAttribute.popFrom(request, Netty4HttpRequestHeaderVerifier.UNCONSUMED_PARAMS).ifPresent(unconsumedParams -> { diff --git a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java index 1cbb4eea1a..320f5ed71f 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java +++ b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java @@ -31,10 +31,13 @@ import org.opensearch.security.configuration.CompatConfig; import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator; import org.opensearch.security.ssl.transport.PrincipalExtractor; +import org.opensearch.telemetry.tracing.Span; +import org.opensearch.telemetry.tracing.TracerContextStorage; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.client.node.NodeClient; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doReturn; @@ -104,4 +107,36 @@ public void testDoesCallDelegateOnSuccessfulAuthorization() throws Exception { // unit tests for restPathMatches are in RestPathMatchesTests.java + /** + * Test that current_span transient is preserved after context restoration. + * We have avoided static mock here hence, we are just checking if our fix helps with the bug + */ + @Test + public void testCurrentSpanTransientPreservedAfterRestore() throws Exception { + ThreadPool tp = spy(new ThreadPool(Settings.builder().put("node.name", "mock").build())); + ThreadContext threadContext = new ThreadContext(Settings.EMPTY); + doReturn(threadContext).when(tp).getThreadContext(); + + Span mockSpan = mock(Span.class); + + // Create a stored context without current_span (simulates stashContext clearing it) + ThreadContext.StoredContext storedContext = threadContext.newStoredContext(false); + + // Now add current_span to the current context (simulates it being set after stash) + threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, mockSpan); + + // Save the span before restore + final Object currentSpan = threadContext.getTransient(TracerContextStorage.CURRENT_SPAN); + + // Restore the stored context (this wipes current_span) + storedContext.restore(); + + // Apply the fix: restore current_span if it was wiped + if (currentSpan != null && threadContext.getTransient(TracerContextStorage.CURRENT_SPAN) == null) { + threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, currentSpan); + } + + assertNotNull("current_span should be preserved", threadContext.getTransient(TracerContextStorage.CURRENT_SPAN)); + } + } From d968f47e9218fa8f12a202863e1902a341d8ea1d Mon Sep 17 00:00:00 2001 From: Deepti Chauhan Date: Sun, 8 Mar 2026 21:53:38 +0530 Subject: [PATCH 2/5] 20517: Fix for security plugin not working Signed-off-by: Deepti24 --- .../security/OpenSearchSecurityPlugin.java | 4 +-- .../security/filter/SecurityRestFilter.java | 27 ++++++++++++++----- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index db738ff536..e5ead03552 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -729,13 +729,13 @@ public List getRestHandlers( } @Override - public UnaryOperator getRestHandlerWrapper(final ThreadContext threadContext, Set headersToCopy) { + public UnaryOperator getRestHandlerWrapper(final ThreadContext threadContext, Set headersToCopy, Set transientsToCopy) { if (client || disabled || SSLConfig.isSslOnlyMode()) { return (rh) -> rh; } - return (rh) -> securityRestHandler.wrap(rh, adminDns, headersToCopy); + return (rh) -> securityRestHandler.wrap(rh, adminDns, headersToCopy, transientsToCopy); } @Override diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 69ea54edb0..7df76010d5 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -128,11 +128,13 @@ public SecurityRestFilter( class AuthczRestHandler extends DelegatingRestHandler { private final AdminDNs adminDNs; private final Set headersToCopy; + private final Set transientsToCopy; - public AuthczRestHandler(RestHandler original, AdminDNs adminDNs, Set headersToCopy) { + public AuthczRestHandler(RestHandler original, AdminDNs adminDNs, Set headersToCopy, Set transientsToCopy) { super(original); this.adminDNs = adminDNs; this.headersToCopy = headersToCopy; + this.transientsToCopy = transientsToCopy; } @Override @@ -160,7 +162,18 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c tmpHeaders.put(header.getName(), value); } } - final Object currentSpan = threadContext.getTransient(TracerContextStorage.CURRENT_SPAN); + + Map trasients = null; + for (String transientValue : transientsToCopy) { + final String value = threadContext.getHeader(transientValue); + if (value != null) { + if (trasients == null) { + trasients = new HashMap<>(); + } + trasients.put(transientValue, value); + } + } + storedContext.restore(); if (tmpHeaders != null) { @@ -169,8 +182,10 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } threadContext.putHeader(OPENSEARCH_SECURITY_REQUEST_HEADERS, String.join(",", tmpHeaders.keySet())); } - if (currentSpan != null && threadContext.getTransient(TracerContextStorage.CURRENT_SPAN) == null) { - threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, currentSpan); + if(trasients != null) { + for (Map.Entry transientVal : trasients.entrySet()) { + threadContext.putTransient(transientVal.getKey(), transientVal.getValue()); + } } }); @@ -268,8 +283,8 @@ RestRequest maybeFilterRestRequest(RestRequest request) throws IOException { * See {@link AllowlistApiAction} for the implementation of this API. * SuperAdmin is identified by credentials, which can be passed in the curl request. */ - public RestHandler wrap(RestHandler original, AdminDNs adminDNs, Set headersToCopy) { - return new AuthczRestHandler(original, adminDNs, headersToCopy); + public RestHandler wrap(RestHandler original, AdminDNs adminDNs, Set headersToCopy, Set transientsToCopy) { + return new AuthczRestHandler(original, adminDNs, headersToCopy, transientsToCopy); } /** From c721ba8e43eceb22968e914286282c4e4bba694a Mon Sep 17 00:00:00 2001 From: Deepti Chauhan Date: Mon, 9 Mar 2026 00:25:37 +0530 Subject: [PATCH 3/5] 20517: adding junit Signed-off-by: Deepti24 --- .../security/filter/SecurityRestFilter.java | 2 +- .../filter/SecurityRestFilterUnitTests.java | 24 ++++++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 7df76010d5..b57fabfb8e 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -165,7 +165,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c Map trasients = null; for (String transientValue : transientsToCopy) { - final String value = threadContext.getHeader(transientValue); + final String value = threadContext.getTransient(transientValue); if (value != null) { if (trasients == null) { trasients = new HashMap<>(); diff --git a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java index 320f5ed71f..495f121757 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java +++ b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java @@ -12,7 +12,7 @@ package org.opensearch.security.filter; import java.nio.file.Path; -import java.util.HashSet; +import java.util.*; import org.junit.Before; import org.junit.Test; @@ -84,7 +84,7 @@ public void setUp() throws NoSuchMethodException { public void testSecurityRestFilterWrap() throws Exception { AdminDNs adminDNs = mock(AdminDNs.class); - RestHandler wrappedRestHandler = sf.wrap(testRestHandler, adminDNs, new HashSet<>()); + RestHandler wrappedRestHandler = sf.wrap(testRestHandler, adminDNs, new HashSet<>(), new HashSet<>()); assertTrue(wrappedRestHandler instanceof SecurityRestFilter.AuthczRestHandler); assertFalse(wrappedRestHandler instanceof TestRestHandler); @@ -96,7 +96,7 @@ public void testDoesCallDelegateOnSuccessfulAuthorization() throws Exception { AdminDNs adminDNs = mock(AdminDNs.class); RestHandler testRestHandlerSpy = spy(testRestHandler); - RestHandler wrappedRestHandler = filterSpy.wrap(testRestHandlerSpy, adminDNs, new HashSet<>()); + RestHandler wrappedRestHandler = filterSpy.wrap(testRestHandlerSpy, adminDNs, new HashSet<>(), new HashSet<>()); doReturn(false).when(filterSpy).userIsSuperAdmin(any(), any()); @@ -119,6 +119,7 @@ public void testCurrentSpanTransientPreservedAfterRestore() throws Exception { Span mockSpan = mock(Span.class); + Set transientsToCopy = new HashSet<>(List.of(TracerContextStorage.CURRENT_SPAN)); // Create a stored context without current_span (simulates stashContext clearing it) ThreadContext.StoredContext storedContext = threadContext.newStoredContext(false); @@ -126,14 +127,25 @@ public void testCurrentSpanTransientPreservedAfterRestore() throws Exception { threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, mockSpan); // Save the span before restore - final Object currentSpan = threadContext.getTransient(TracerContextStorage.CURRENT_SPAN); + Map trasients = null; + for (String transientValue : transientsToCopy) { + final Object value = threadContext.getTransient(transientValue); + if (value != null) { + if (trasients == null) { + trasients = new HashMap<>(); + } + trasients.put(transientValue, value); + } + } // Restore the stored context (this wipes current_span) storedContext.restore(); // Apply the fix: restore current_span if it was wiped - if (currentSpan != null && threadContext.getTransient(TracerContextStorage.CURRENT_SPAN) == null) { - threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, currentSpan); + if(trasients != null) { + for (Map.Entry transientVal : trasients.entrySet()) { + threadContext.putTransient(transientVal.getKey(), transientVal.getValue()); + } } assertNotNull("current_span should be preserved", threadContext.getTransient(TracerContextStorage.CURRENT_SPAN)); From 419c72bf47cc46a236432d51edbe668ccce08e0e Mon Sep 17 00:00:00 2001 From: Deepti Chauhan Date: Mon, 9 Mar 2026 23:40:16 +0530 Subject: [PATCH 4/5] 20517: adding junits Signed-off-by: Deepti24 --- .../security/filter/SecurityRestFilter.java | 6 +- .../filter/SecurityRestFilterUnitTests.java | 103 +++++++++++------- 2 files changed, 65 insertions(+), 44 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index b57fabfb8e..17a1663db6 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -165,7 +165,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c Map trasients = null; for (String transientValue : transientsToCopy) { - final String value = threadContext.getTransient(transientValue); + final Object value = threadContext.getTransient(transientValue); if (value != null) { if (trasients == null) { trasients = new HashMap<>(); @@ -184,7 +184,9 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } if(trasients != null) { for (Map.Entry transientVal : trasients.entrySet()) { - threadContext.putTransient(transientVal.getKey(), transientVal.getValue()); + if (threadContext.getTransient(transientVal.getKey()) == null) { + threadContext.putTransient(transientVal.getKey(), transientVal.getValue()); + } } } }); diff --git a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java index 495f121757..5f820947ba 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java +++ b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java @@ -21,24 +21,20 @@ import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.rest.RestStatus; -import org.opensearch.rest.BytesRestResponse; -import org.opensearch.rest.RestChannel; -import org.opensearch.rest.RestHandler; -import org.opensearch.rest.RestRequest; +import org.opensearch.rest.*; import org.opensearch.security.auditlog.AuditLog; import org.opensearch.security.auth.BackendRegistry; import org.opensearch.security.configuration.AdminDNs; import org.opensearch.security.configuration.CompatConfig; import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator; import org.opensearch.security.ssl.transport.PrincipalExtractor; +import org.opensearch.security.ssl.http.netty.Netty4HttpRequestHeaderVerifier; import org.opensearch.telemetry.tracing.Span; import org.opensearch.telemetry.tracing.TracerContextStorage; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.client.node.NodeClient; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -49,6 +45,7 @@ public class SecurityRestFilterUnitTests { SecurityRestFilter sf; RestHandler testRestHandler; + ThreadPool threadPool; class TestRestHandler implements RestHandler { @@ -63,6 +60,7 @@ public void setUp() throws NoSuchMethodException { testRestHandler = new TestRestHandler(); ThreadPool tp = spy(new ThreadPool(Settings.builder().put("node.name", "mock").build())); + this.threadPool = tp; doReturn(new ThreadContext(Settings.EMPTY)).when(tp).getThreadContext(); sf = new SecurityRestFilter( @@ -107,48 +105,69 @@ public void testDoesCallDelegateOnSuccessfulAuthorization() throws Exception { // unit tests for restPathMatches are in RestPathMatchesTests.java - /** - * Test that current_span transient is preserved after context restoration. - * We have avoided static mock here hence, we are just checking if our fix helps with the bug - */ + //Test that current_span transient is preserved after context restoration. @Test public void testCurrentSpanTransientPreservedAfterRestore() throws Exception { - ThreadPool tp = spy(new ThreadPool(Settings.builder().put("node.name", "mock").build())); - ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - doReturn(threadContext).when(tp).getThreadContext(); - - Span mockSpan = mock(Span.class); + ThreadContext threadContext = threadPool.getThreadContext(); + // Handler verifies span is present + RestHandler testHandler = (request, channel, client) -> { + assertNotNull("CURRENT_SPAN should be preserved", + threadContext.getTransient(TracerContextStorage.CURRENT_SPAN)); + }; Set transientsToCopy = new HashSet<>(List.of(TracerContextStorage.CURRENT_SPAN)); - // Create a stored context without current_span (simulates stashContext clearing it) - ThreadContext.StoredContext storedContext = threadContext.newStoredContext(false); - - // Now add current_span to the current context (simulates it being set after stash) - threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, mockSpan); - - // Save the span before restore - Map trasients = null; - for (String transientValue : transientsToCopy) { - final Object value = threadContext.getTransient(transientValue); - if (value != null) { - if (trasients == null) { - trasients = new HashMap<>(); - } - trasients.put(transientValue, value); - } - } + RestHandler wrappedRestHandler = sf.wrap(testHandler, mock(AdminDNs.class), new HashSet<>(), transientsToCopy); + RestRequest request = addRelevantMocksAndGetRequest(threadContext); - // Restore the stored context (this wipes current_span) - storedContext.restore(); + threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, mock(Span.class)); + wrappedRestHandler.handleRequest(request, mock(RestChannel.class), mock(NodeClient.class)); - // Apply the fix: restore current_span if it was wiped - if(trasients != null) { - for (Map.Entry transientVal : trasients.entrySet()) { - threadContext.putTransient(transientVal.getKey(), transientVal.getValue()); - } - } + assertNotNull("current_span should be preserved after handleRequest completes", + threadContext.getTransient(TracerContextStorage.CURRENT_SPAN)); + + } + + // Current span is present in context ,not in transientsToCopy, hence we should NOT find it later + @Test + public void testCurrentSpanTransientNotPreservedAfterRestore() throws Exception { + ThreadContext threadContext = threadPool.getThreadContext(); + + // Handler verifies span is absent + RestHandler testHandler = (request, channel, client) -> { + assertNull("CURRENT_SPAN should NOT be preserved", + threadContext.getTransient(TracerContextStorage.CURRENT_SPAN)); + }; + + Set transientsToCopy = new HashSet<>(); + RestHandler wrappedRestHandler = sf.wrap(testHandler, mock(AdminDNs.class), new HashSet<>(), transientsToCopy); + RestRequest request = addRelevantMocksAndGetRequest(threadContext); - assertNotNull("current_span should be preserved", threadContext.getTransient(TracerContextStorage.CURRENT_SPAN)); + threadContext.putTransient(TracerContextStorage.CURRENT_SPAN, mock(Span.class)); + + wrappedRestHandler.handleRequest(request, mock(RestChannel.class), mock(NodeClient.class)); + assertNull("current_span should NOT be preserved after handleRequest completes as its not present in transientsToCopy", + threadContext.getTransient(TracerContextStorage.CURRENT_SPAN)); } + + @SuppressWarnings("unchecked") + private RestRequest addRelevantMocksAndGetRequest(ThreadContext threadContext ) { + // Mock Netty attributes + RestRequest request = mock(RestRequest.class); + org.opensearch.http.HttpChannel httpChannel = mock(org.opensearch.http.HttpChannel.class); + io.netty.channel.Channel nettyChannel = mock(io.netty.channel.Channel.class); + + doReturn(httpChannel).doReturn(httpChannel).doReturn(null).when(request).getHttpChannel(); + doReturn(Optional.of(nettyChannel)).when(httpChannel).get("channel", io.netty.channel.Channel.class); + + io.netty.util.Attribute contextAttr = mock(io.netty.util.Attribute.class); + io.netty.util.Attribute earlyResponseAttr = mock(io.netty.util.Attribute.class); + doReturn(contextAttr).when(nettyChannel).attr(Netty4HttpRequestHeaderVerifier.CONTEXT_TO_RESTORE); + doReturn(earlyResponseAttr).when(nettyChannel).attr(Netty4HttpRequestHeaderVerifier.EARLY_RESPONSE); + doReturn(null).when(earlyResponseAttr).getAndSet(null); + + ThreadContext.StoredContext storedContext = threadContext.newStoredContext(true); + doReturn(storedContext).when(contextAttr).getAndSet(null); + return request; + } } From 11c2d200fe2b41b98f20532daccfb2beaddb0b13 Mon Sep 17 00:00:00 2001 From: Deepti Chauhan Date: Mon, 9 Mar 2026 23:42:57 +0530 Subject: [PATCH 5/5] 20517: adding junits Signed-off-by: Deepti24 --- .../security/filter/SecurityRestFilterUnitTests.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java index 5f820947ba..959b91872c 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java +++ b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java @@ -12,7 +12,10 @@ package org.opensearch.security.filter; import java.nio.file.Path; -import java.util.*; +import java.util.HashSet; +import java.util.Optional; +import java.util.Set; +import java.util.List; import org.junit.Before; import org.junit.Test;