Skip to content

Commit 34d0212

Browse files
slobodanadamovicslobodanadamovic
committed
Sanitize transient headers from thread context
1 parent 88bc195 commit 34d0212

File tree

4 files changed

+177
-10
lines changed

4 files changed

+177
-10
lines changed

‎server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@
1616
import org.elasticsearch.common.io.stream.StreamInput;
1717
import org.elasticsearch.common.io.stream.StreamOutput;
1818
import org.elasticsearch.common.io.stream.Writeable;
19+
import org.elasticsearch.common.settings.SecureString;
1920
import org.elasticsearch.common.settings.Setting;
2021
import org.elasticsearch.common.settings.Setting.Property;
2122
import org.elasticsearch.common.settings.Settings;
2223
import org.elasticsearch.common.util.Maps;
2324
import org.elasticsearch.common.util.set.Sets;
25+
import org.elasticsearch.core.IOUtils;
2426
import org.elasticsearch.core.Releasable;
2527
import org.elasticsearch.core.Tuple;
2628
import org.elasticsearch.http.HttpTransportSettings;
@@ -773,6 +775,31 @@ public void sanitizeHeaders() {
773775
// intentionally not storing prior context to avoid restoring unwanted headers
774776
}
775777

778+
/**
779+
* Removes unneeded transient headers from the thread context. Does not store prior context.
780+
*/
781+
public void sanitizeTransientHeaders() {
782+
final ThreadContextStruct originalContext = threadLocal.get();
783+
final Map<String, Object> newTransientHeaders = Maps.newMapWithExpectedSize(originalContext.transientHeaders.size());
784+
for (var entry : originalContext.transientHeaders.entrySet()) {
785+
if (entry.getValue() instanceof SecureString secureString) {
786+
IOUtils.closeWhileHandlingException(secureString);
787+
} else {
788+
newTransientHeaders.put(entry.getKey(), entry.getValue());
789+
}
790+
}
791+
792+
final ThreadContextStruct newContext = new ThreadContextStruct(
793+
originalContext.requestHeaders,
794+
originalContext.responseHeaders,
795+
newTransientHeaders,
796+
originalContext.isSystemContext,
797+
originalContext.warningHeadersSize
798+
);
799+
threadLocal.set(newContext);
800+
// intentionally not storing prior context to avoid restoring unwanted headers
801+
}
802+
776803
@FunctionalInterface
777804
public interface StoredContext extends AutoCloseable, Releasable {
778805
default void restore() {

‎server/src/test/java/org/elasticsearch/common/util/concurrent/ThreadContextTests.java‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import org.elasticsearch.common.ReferenceDocs;
1313
import org.elasticsearch.common.io.stream.BytesStreamOutput;
1414
import org.elasticsearch.common.logging.HeaderWarning;
15+
import org.elasticsearch.common.settings.SecureString;
1516
import org.elasticsearch.common.settings.Settings;
1617
import org.elasticsearch.core.Tuple;
1718
import org.elasticsearch.http.HttpTransportSettings;
@@ -38,6 +39,7 @@
3839
import static org.hamcrest.Matchers.anEmptyMap;
3940
import static org.hamcrest.Matchers.contains;
4041
import static org.hamcrest.Matchers.containsInAnyOrder;
42+
import static org.hamcrest.Matchers.containsString;
4143
import static org.hamcrest.Matchers.equalTo;
4244
import static org.hamcrest.Matchers.hasItem;
4345
import static org.hamcrest.Matchers.hasSize;
@@ -1157,6 +1159,58 @@ public void testSanitizeHeaders() {
11571159
}
11581160
}
11591161

1162+
public void testSanitizeTransientHeaders() {
1163+
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
1164+
1165+
// add transient headers
1166+
final SecureString secureString1 = new SecureString("password1".toCharArray());
1167+
final String regularTransient1 = "string-value";
1168+
final Integer regularTransient2 = 42;
1169+
final Object regularTransient3 = new Object();
1170+
threadContext.putTransient("secure.key1", secureString1);
1171+
threadContext.putTransient("regular.key1", regularTransient1);
1172+
threadContext.putTransient("regular.key2", regularTransient2);
1173+
threadContext.putTransient("regular.key3", regularTransient3);
1174+
1175+
// add request and response headers that should be preserved
1176+
threadContext.putHeader("request-header", "request-value");
1177+
threadContext.addResponseHeader("response-header", "response-value");
1178+
1179+
threadContext.sanitizeTransientHeaders();
1180+
1181+
// verify secure string is removed from transient headers and closed
1182+
assertThat(threadContext.getTransient("secure.key1"), nullValue());
1183+
expectThrows(IllegalStateException.class, containsString("SecureString has already been closed"), secureString1::getChars);
1184+
1185+
// other headers should be unchanged
1186+
assertThat(threadContext.getTransient("regular.key1"), equalTo(regularTransient1));
1187+
assertThat(threadContext.getTransient("regular.key2"), equalTo(regularTransient2));
1188+
assertThat(threadContext.getTransient("regular.key3"), sameInstance(regularTransient3));
1189+
assertThat(threadContext.getHeader("request-header"), equalTo("request-value"));
1190+
assertThat(threadContext.getResponseHeaders().get("response-header").getFirst(), equalTo("response-value"));
1191+
}
1192+
1193+
public void testSanitizeTransientHeadersWithOnlySecureStrings() {
1194+
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
1195+
final SecureString secureString1 = new SecureString("password1".toCharArray());
1196+
threadContext.putTransient("secure.key1", secureString1);
1197+
1198+
threadContext.sanitizeTransientHeaders();
1199+
1200+
assertThat(threadContext.getTransient("secure.key1"), nullValue());
1201+
expectThrows(IllegalStateException.class, containsString("SecureString has already been closed"), secureString1::getChars);
1202+
assertThat(threadContext.getTransientHeaders(), is(anEmptyMap()));
1203+
}
1204+
1205+
public void testSanitizeTransientHeadersWithEmptyTransients() {
1206+
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
1207+
assertThat(threadContext.getTransientHeaders(), is(anEmptyMap()));
1208+
1209+
threadContext.sanitizeTransientHeaders();
1210+
1211+
assertThat(threadContext.getTransientHeaders(), is(anEmptyMap()));
1212+
}
1213+
11601214
public void testNewEmptyContext() {
11611215
final var threadContext = new ThreadContext(Settings.EMPTY);
11621216
final var header = randomBoolean() ? randomIdentifier() : randomFrom(HEADERS_TO_COPY);

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -100,20 +100,28 @@ public void intercept(RestRequest request, RestChannel channel, RestHandler targ
100100

101101
private void doHandleRequest(RestRequest request, RestChannel channel, RestHandler targetHandler, ActionListener<Boolean> listener) {
102102
threadContext.sanitizeHeaders();
103-
// operator privileges can short circuit to return a non-successful response
104-
if (operatorPrivilegesService.checkRest(targetHandler, request, channel, threadContext)) {
105-
listener.onResponse(Boolean.TRUE);
106-
} else {
107-
// The service sends its own response if it returns `false`.
108-
// That's kind of ugly, and it would be better if we throw an exception and let the rest controller serialize it as normal
109-
listener.onResponse(Boolean.FALSE);
103+
try {
104+
// operator privileges can short circuit to return a non-successful response
105+
if (operatorPrivilegesService.checkRest(targetHandler, request, channel, threadContext)) {
106+
listener.onResponse(Boolean.TRUE);
107+
} else {
108+
// The service sends its own response if it returns `false`.
109+
// That's kind of ugly, and it would be better if we throw an exception and let the rest controller serialize it as normal
110+
listener.onResponse(Boolean.FALSE);
111+
}
112+
} finally {
113+
threadContext.sanitizeTransientHeaders();
110114
}
111115
}
112116

113117
protected void handleException(RestRequest request, Exception e, ActionListener<?> listener) {
114-
logger.debug(() -> format("failed for REST request [%s]", request.uri()), e);
115-
threadContext.sanitizeHeaders();
116-
listener.onFailure(e);
118+
try {
119+
logger.debug(() -> format("failed for REST request [%s]", request.uri()), e);
120+
threadContext.sanitizeHeaders();
121+
listener.onFailure(e);
122+
} finally {
123+
threadContext.sanitizeTransientHeaders();
124+
}
117125
}
118126

119127
// for testing

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.elasticsearch.action.support.PlainActionFuture;
1515
import org.elasticsearch.client.internal.node.NodeClient;
1616
import org.elasticsearch.common.bytes.BytesArray;
17+
import org.elasticsearch.common.settings.SecureString;
1718
import org.elasticsearch.common.settings.Settings;
1819
import org.elasticsearch.common.util.concurrent.ThreadContext;
1920
import org.elasticsearch.http.HttpChannel;
@@ -346,5 +347,82 @@ public void maybeInterceptRequest(ThreadContext threadContext, TransportRequest
346347
}
347348
}
348349

350+
public void testSanitizeTransientHeaders() throws Exception {
351+
for (boolean failRequest : List.of(true, false)) {
352+
RestRequest request = mock(RestRequest.class);
353+
when(request.getHttpChannel()).thenReturn(mock(HttpChannel.class));
354+
HttpRequest httpRequest = mock(HttpRequest.class);
355+
when(request.getHttpRequest()).thenReturn(httpRequest);
356+
Authentication authentication = AuthenticationTestHelper.builder().build();
357+
doAnswer((i) -> {
358+
@SuppressWarnings("unchecked")
359+
ActionListener<Authentication> callback = (ActionListener<Authentication>) i.getArguments()[1];
360+
if (failRequest) {
361+
callback.onFailure(new RuntimeException());
362+
} else {
363+
callback.onResponse(authentication);
364+
}
365+
return Void.TYPE;
366+
}).when(authcService).authenticate(eq(httpRequest), anyActionListener());
367+
368+
final SecureString secureString = new SecureString("password".toCharArray());
369+
threadContext.putTransient("secure.key", secureString);
370+
371+
PlainActionFuture<Boolean> future = new PlainActionFuture<>() {
372+
@Override
373+
public void onResponse(Boolean result) {
374+
assertThat(threadContext.getTransient("secure.key"), equalTo(secureString));
375+
super.onResponse(result);
376+
}
377+
378+
@Override
379+
public void onFailure(Exception e) {
380+
assertThat(threadContext.getTransient("secure.key"), equalTo(secureString));
381+
super.onFailure(e);
382+
}
383+
};
384+
filter.intercept(request, channel, restHandler, future);
385+
assertThat(future.get(), is(Boolean.TRUE));
386+
387+
// verify SecureString instance is removed and closed
388+
assertThat(threadContext.getTransient("secure.key"), nullValue());
389+
expectThrows(IllegalStateException.class, containsString("SecureString has already been closed"), secureString::getChars);
390+
}
391+
}
392+
393+
public void testSanitizeTransientHeadersAfterException() throws Exception {
394+
RestRequest request = mock(RestRequest.class);
395+
when(request.getHttpChannel()).thenReturn(mock(HttpChannel.class));
396+
HttpRequest httpRequest = mock(HttpRequest.class);
397+
when(request.getHttpRequest()).thenReturn(httpRequest);
398+
399+
final ElasticsearchSecurityException testException = new ElasticsearchSecurityException("test exception");
400+
doAnswer((i) -> {
401+
@SuppressWarnings("unchecked")
402+
ActionListener<Authentication> callback = (ActionListener<Authentication>) i.getArguments()[1];
403+
callback.onFailure(testException);
404+
return Void.TYPE;
405+
}).when(authcService).authenticate(eq(httpRequest), anyActionListener());
406+
407+
final SecureString secureString = new SecureString("password".toCharArray());
408+
threadContext.putTransient("secure.key", secureString);
409+
410+
PlainActionFuture<Boolean> future = new PlainActionFuture<>() {
411+
@Override
412+
public void onResponse(Boolean result) {
413+
assertThat(threadContext.getTransient("secure.key"), equalTo(secureString));
414+
throw testException;
415+
}
416+
};
417+
filter.intercept(request, channel, restHandler, future);
418+
419+
var ese = expectThrows(ElasticsearchSecurityException.class, future::actionGet);
420+
assertThat(ese, sameInstance(testException));
421+
422+
// verify SecureString is removed and closed
423+
assertThat(threadContext.getTransient("secure.key"), nullValue());
424+
expectThrows(IllegalStateException.class, containsString("SecureString has already been closed"), secureString::getChars);
425+
}
426+
349427
private interface FilteredRestHandler extends RestHandler, RestRequestFilter {}
350428
}

0 commit comments

Comments
 (0)