From 1fc1df9499a07cfe7729a0aa12e13e16898c13fb Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 30 Oct 2019 08:38:50 +0200 Subject: [PATCH] Copy http headers to ThreadContext strictly (#45945) Previous behavior while copying HTTP headers to the ThreadContext, would allow multiple HTTP headers with the same name, handling only the first occurrence and disregarding the rest of the values. This can be confusing when dealing with multiple Headers as it is not obvious which value is read and which ones are silently dropped. According to RFC-7230, a client must not send multiple header fields with the same field name in a HTTP message, unless the entire field value for this header is defined as a comma separated list or this specific header is a well-known exception. This commits changes the behavior in order to be more compliant to the aforementioned RFC by requiring the classes that implement ActionPlugin to declare if a header can be multi-valued or not when registering this header to be copied over to the ThreadContext in ActionPlugin#getRestHeaders. If the header is allowed to be multivalued, then all such headers are read from the HTTP request and their values get concatenated in a comma-separated string. If the header is not allowed to be multivalued, and the HTTP request contains multiple such Headers with different values, the request is rejected with a 400 status. --- .../ReindexFromRemoteWithAuthTests.java | 6 ++- .../elasticsearch/action/ActionModule.java | 5 +- .../elasticsearch/plugins/ActionPlugin.java | 3 +- .../elasticsearch/rest/RestController.java | 22 ++++++--- .../rest/RestHeaderDefinition.java | 46 +++++++++++++++++++ .../rest/RestControllerTests.java | 37 ++++++++++++++- .../core/LocalStateCompositeXPackPlugin.java | 5 +- .../xpack/security/Security.java | 11 +++-- .../example/SpiExtensionPlugin.java | 7 ++- 9 files changed, 121 insertions(+), 21 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/rest/RestHeaderDefinition.java diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFromRemoteWithAuthTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFromRemoteWithAuthTests.java index 3a7b0b253bc38..4cab64e1bc9c4 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFromRemoteWithAuthTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFromRemoteWithAuthTests.java @@ -43,6 +43,7 @@ import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.rest.RestHeaderDefinition; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.script.ScriptService; import org.elasticsearch.tasks.Task; @@ -161,8 +162,9 @@ public List getActionFilters() { } @Override - public Collection getRestHeaders() { - return Arrays.asList(TestFilter.AUTHORIZATION_HEADER, TestFilter.EXAMPLE_HEADER); + public Collection getRestHeaders() { + return Arrays.asList(new RestHeaderDefinition(TestFilter.AUTHORIZATION_HEADER, false), + new RestHeaderDefinition(TestFilter.EXAMPLE_HEADER, false)); } } diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index 81d5bd17c33df..9098f280da708 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -230,6 +230,7 @@ import org.elasticsearch.plugins.ActionPlugin.ActionHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; +import org.elasticsearch.rest.RestHeaderDefinition; import org.elasticsearch.rest.action.RestFieldCapabilitiesAction; import org.elasticsearch.rest.action.RestMainAction; import org.elasticsearch.rest.action.admin.cluster.RestAddVotingConfigExclusionAction; @@ -390,9 +391,9 @@ public ActionModule(Settings settings, IndexNameExpressionResolver indexNameExpr actionFilters = setupActionFilters(actionPlugins); autoCreateIndex = new AutoCreateIndex(settings, clusterSettings, indexNameExpressionResolver); destructiveOperations = new DestructiveOperations(settings, clusterSettings); - Set headers = Stream.concat( + Set headers = Stream.concat( actionPlugins.stream().flatMap(p -> p.getRestHeaders().stream()), - Stream.of(Task.X_OPAQUE_ID) + Stream.of(new RestHeaderDefinition(Task.X_OPAQUE_ID, false)) ).collect(Collectors.toSet()); UnaryOperator restWrapper = null; for (ActionPlugin plugin : actionPlugins) { diff --git a/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java b/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java index ee6e026d5a535..ef9861f266222 100644 --- a/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java +++ b/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java @@ -36,6 +36,7 @@ import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; +import org.elasticsearch.rest.RestHeaderDefinition; import java.util.Collection; import java.util.Collections; @@ -91,7 +92,7 @@ default List getRestHandlers(Settings settings, RestController rest /** * Returns headers which should be copied through rest requests on to internal requests. */ - default Collection getRestHeaders() { + default Collection getRestHeaders() { return Collections.emptyList(); } diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 49b6754c6f715..1c174c89df608 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -50,6 +50,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; import java.util.function.UnaryOperator; +import java.util.stream.Collectors; import static org.elasticsearch.rest.BytesRestResponse.TEXT_CONTENT_TYPE; import static org.elasticsearch.rest.RestStatus.BAD_REQUEST; @@ -71,10 +72,10 @@ public class RestController implements HttpServerTransport.Dispatcher { private final CircuitBreakerService circuitBreakerService; /** Rest headers that are copied to internal requests made during a rest request. */ - private final Set headersToCopy; + private final Set headersToCopy; private final UsageService usageService; - public RestController(Set headersToCopy, UnaryOperator handlerWrapper, + public RestController(Set headersToCopy, UnaryOperator handlerWrapper, NodeClient client, CircuitBreakerService circuitBreakerService, UsageService usageService) { this.headersToCopy = headersToCopy; this.usageService = usageService; @@ -255,10 +256,19 @@ private void sendContentTypeErrorMessage(@Nullable List contentTypeHeade } private void tryAllHandlers(final RestRequest request, final RestChannel channel, final ThreadContext threadContext) throws Exception { - for (String key : headersToCopy) { - String httpHeader = request.header(key); - if (httpHeader != null) { - threadContext.putHeader(key, httpHeader); + for (final RestHeaderDefinition restHeader : headersToCopy) { + final String name = restHeader.getName(); + final List headerValues = request.getAllHeaderValues(name); + if (headerValues != null && headerValues.isEmpty() == false) { + final List distinctHeaderValues = headerValues.stream().distinct().collect(Collectors.toList()); + if (restHeader.isMultiValueAllowed() == false && distinctHeaderValues.size() > 1) { + channel.sendResponse( + BytesRestResponse. + createSimpleErrorResponse(channel, BAD_REQUEST, "multiple values for single-valued header [" + name + "].")); + return; + } else { + threadContext.putHeader(name, String.join(",", distinctHeaderValues)); + } } } // error_trace cannot be used when we disable detailed errors diff --git a/server/src/main/java/org/elasticsearch/rest/RestHeaderDefinition.java b/server/src/main/java/org/elasticsearch/rest/RestHeaderDefinition.java new file mode 100644 index 0000000000000..6fb95ba80dd5f --- /dev/null +++ b/server/src/main/java/org/elasticsearch/rest/RestHeaderDefinition.java @@ -0,0 +1,46 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.rest; + +/** + * A definition for an http header that should be copied to the {@link org.elasticsearch.common.util.concurrent.ThreadContext} when + * reading the request on the rest layer. + */ +public final class RestHeaderDefinition { + private final String name; + /** + * This should be set to true only when the syntax of the value of the Header to copy is defined as a comma separated list of String + * values. + */ + private final boolean multiValueAllowed; + + public RestHeaderDefinition(String name, boolean multiValueAllowed) { + this.name = name; + this.multiValueAllowed = multiValueAllowed; + } + + public String getName() { + return name; + } + + public boolean isMultiValueAllowed() { + return multiValueAllowed; + } +} diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 95cb56cd97c4f..eb3d76bc82ae2 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -105,7 +105,8 @@ public void setup() { public void testApplyRelevantHeaders() throws Exception { final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - Set headers = new HashSet<>(Arrays.asList("header.1", "header.2")); + Set headers = new HashSet<>(Arrays.asList(new RestHeaderDefinition("header.1", true), + new RestHeaderDefinition("header.2", true))); final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService); Map> restHeaders = new HashMap<>(); restHeaders.put("header.1", Collections.singletonList("true")); @@ -138,6 +139,40 @@ public MethodHandlers next() { assertNull(threadContext.getHeader("header.3")); } + public void testRequestWithDisallowedMultiValuedHeader() { + final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); + Set headers = new HashSet<>(Arrays.asList(new RestHeaderDefinition("header.1", true), + new RestHeaderDefinition("header.2", false))); + final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService); + Map> restHeaders = new HashMap<>(); + restHeaders.put("header.1", Collections.singletonList("boo")); + restHeaders.put("header.2", List.of("foo", "bar")); + RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(restHeaders).build(); + AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.BAD_REQUEST); + restController.dispatchRequest(fakeRequest, channel, threadContext); + assertTrue(channel.getSendResponseCalled()); + } + + public void testRequestWithDisallowedMultiValuedHeaderButSameValues() { + final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); + Set headers = new HashSet<>(Arrays.asList(new RestHeaderDefinition("header.1", true), + new RestHeaderDefinition("header.2", false))); + final RestController restController = new RestController(headers, null, null, circuitBreakerService, usageService); + Map> restHeaders = new HashMap<>(); + restHeaders.put("header.1", Collections.singletonList("boo")); + restHeaders.put("header.2", List.of("foo", "foo")); + RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(restHeaders).withPath("/bar").build(); + restController.registerHandler(RestRequest.Method.GET, "/bar", new RestHandler() { + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + channel.sendResponse(new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); + } + }); + AssertingChannel channel = new AssertingChannel(fakeRequest, false, RestStatus.OK); + restController.dispatchRequest(fakeRequest, channel, threadContext); + assertTrue(channel.getSendResponseCalled()); + } + public void testRegisterAsDeprecatedHandler() { RestController controller = mock(RestController.class); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java index e334f72bc4a8a..6952342040c5d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java @@ -62,6 +62,7 @@ import org.elasticsearch.repositories.Repository; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; +import org.elasticsearch.rest.RestHeaderDefinition; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.ScriptService; import org.elasticsearch.threadpool.ExecutorBuilder; @@ -152,8 +153,8 @@ public Collection createComponents(Client client, ClusterService cluster } @Override - public Collection getRestHeaders() { - List headers = new ArrayList<>(); + public Collection getRestHeaders() { + List headers = new ArrayList<>(); headers.addAll(super.getRestHeaders()); filterPlugins(ActionPlugin.class).stream().forEach(p -> headers.addAll(p.getRestHeaders())); return headers; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 9c9aea7111442..a0e07907a8712 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -57,6 +57,7 @@ import org.elasticsearch.plugins.Plugin; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; +import org.elasticsearch.rest.RestHeaderDefinition; import org.elasticsearch.script.ScriptService; import org.elasticsearch.threadpool.ExecutorBuilder; import org.elasticsearch.threadpool.FixedExecutorBuilder; @@ -621,14 +622,14 @@ public static List> getSettings(List securityExten } @Override - public Collection getRestHeaders() { - Set headers = new HashSet<>(); - headers.add(UsernamePasswordToken.BASIC_AUTH_HEADER); + public Collection getRestHeaders() { + Set headers = new HashSet<>(); + headers.add(new RestHeaderDefinition(UsernamePasswordToken.BASIC_AUTH_HEADER, false)); if (XPackSettings.AUDIT_ENABLED.get(settings)) { - headers.add(AuditTrail.X_FORWARDED_FOR_HEADER); + headers.add(new RestHeaderDefinition(AuditTrail.X_FORWARDED_FOR_HEADER, true)); } if (AuthenticationServiceField.RUN_AS_ENABLED.get(settings)) { - headers.add(AuthenticationServiceField.RUN_AS_USER_HEADER); + headers.add(new RestHeaderDefinition(AuthenticationServiceField.RUN_AS_USER_HEADER, false)); } return headers; } diff --git a/x-pack/qa/security-example-spi-extension/src/main/java/org/elasticsearch/example/SpiExtensionPlugin.java b/x-pack/qa/security-example-spi-extension/src/main/java/org/elasticsearch/example/SpiExtensionPlugin.java index 9314b6a675056..eedb06f2c1bad 100644 --- a/x-pack/qa/security-example-spi-extension/src/main/java/org/elasticsearch/example/SpiExtensionPlugin.java +++ b/x-pack/qa/security-example-spi-extension/src/main/java/org/elasticsearch/example/SpiExtensionPlugin.java @@ -9,6 +9,7 @@ import org.elasticsearch.example.realm.CustomRealm; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.rest.RestHeaderDefinition; import org.elasticsearch.xpack.core.security.authc.RealmSettings; import java.util.ArrayList; @@ -22,8 +23,10 @@ public class SpiExtensionPlugin extends Plugin implements ActionPlugin { @Override - public Collection getRestHeaders() { - return Arrays.asList(CustomRealm.USER_HEADER, CustomRealm.PW_HEADER); + public Collection getRestHeaders() { + return Arrays.asList( + new RestHeaderDefinition(CustomRealm.USER_HEADER, false), + new RestHeaderDefinition(CustomRealm.PW_HEADER, false)); } @Override