Copy http headers to ThreadContext strictly #45945

jkakavas · 2019-08-25T14:29:47Z

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.

Previous behavior while copying HTTP headers to the ThreadContext, would allow multiple HTTP headers with the same name, handling only the first occurence 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.

elasticmachine · 2019-08-25T14:29:49Z

Pinging @elastic/es-core-infra

jkakavas · 2019-08-25T14:35:30Z

For 7.x, we can refrain from making any breaking changes and minimize the changes required by only introducing RestRequest#getSingleValuedHeader and replacing RestRequest#header in https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/RestController.java#L259.

This would make the assumption that all registered headers are meant to be single-valued, but I think it is a fair assumption for all currently registered headers in all plugins, since we would only read the first of the values either way.

…ctly

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

jkakavas · 2019-08-26T07:03:12Z

@elasticmachine run elasticsearch-ci/1

jkakavas · 2019-09-04T18:36:41Z

@jasontedor I dont mean to rush things, just doing a round on my open PRs.

tvernum · 2019-09-25T06:28:57Z

server/src/main/java/org/elasticsearch/rest/RestController.java

            }
+        } catch (IllegalStateException e){


Suggested change

} catch (IllegalStateException e){

} catch (IllegalStateException e) {

tvernum · 2019-09-25T06:47:19Z

...curity-example-spi-extension/src/main/java/org/elasticsearch/example/SpiExtensionPlugin.java

+    public Collection<RestHeaderDefinition> getRestHeaders() {
+        return Arrays.asList(
+            new RestHeaderDefinition(CustomRealm.USER_HEADER, true),
+            new RestHeaderDefinition(CustomRealm.PW_HEADER, true));


Shouldn't these be false ?
I know it doesn't matter for testing purposes, but we point people to this as an example, and I think we want the default expaction for security examples to be single-valued headers.

…ctly

jkakavas · 2019-09-25T07:35:51Z

@jasontedor ping for a review 🙏

jasontedor

@jkakavas I'm so sorry I lost track of this one, it's unacceptable.

Overall the approach looks good, I left a comment for consideration.

jasontedor · 2019-10-08T11:04:49Z

server/src/main/java/org/elasticsearch/rest/RestController.java

            }
+        } catch (IllegalStateException e) {


It's too bad we have to use an unchecked exception for flow control here. Can we look for another way? For example, one suggestion would be to pull the multi-valued validation into this method instead of holding it in RestRequest#getSingleValuedHeader.

Or, if this is not palatable, or if we can't find another way, can RestRequest#getSingleValuedHeader at least declare this exception in its Javadocs?

…ctly

jasontedor

Looking good, I left one more observation, and a minor nit.

jasontedor · 2019-10-08T14:42:45Z

server/src/main/java/org/elasticsearch/rest/RestController.java

+                            createSimpleErrorResponse(channel, BAD_REQUEST, "multiple values for single-valued header [" + name + "]."));
+                    return;
+                } else {
+                    threadContext.putHeader(name, headerValues.stream().distinct().collect(Collectors.joining(",")));


This is effectively going to collect allocate a set to collect the header values twice in case multi-value headers are not allowed. Once to collect the header values into a set to see if its size is one, and then again (when the set is size one) to do the trivial join. Since we always need to collect the distinct values (either to check if we should fail the request, or put the header in the context), I think we should refactor this slightly to avoid collecting twice and all the allocations that come with it.

jasontedor · 2019-10-08T14:42:58Z

server/src/main/java/org/elasticsearch/rest/RestController.java

-            String httpHeader = request.header(key);
-            if (httpHeader != null) {
-                threadContext.putHeader(key, httpHeader);
+        for (RestHeaderDefinition restHeader : headersToCopy) {


The loop iterator can be final.

jasontedor

That looks great @jkakavas. Thanks for the iterations.

…ctly

jkakavas · 2019-10-11T12:34:31Z

@elasticmachine run elasticsearch-ci/packaging-sample-matrix

jkakavas · 2019-10-11T14:14:15Z

packaging sample was actually successful : https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request+packaging-tests-sample/8348/

BUILD SUCCESSFUL in 1h 28m 56s
--
339 actionable tasks: 321 executed, 17 from cache, 1 up-to-date

jkakavas · 2019-10-11T14:21:31Z

@elasticmachine run elasticsearch-ci/packaging-sample

jkakavas · 2019-10-11T14:36:47Z

17:34:54 OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x00007f8371dd0000, 65536, 1) failed; error='Cannot allocate memory' (errno=12)
17:34:54 #
17:34:54 # There is insufficient memory for the Java Runtime Environment to continue.
17:34:54 # Native memory allocation (mmap) failed to map 65536 bytes for committing reserved memory.
17:34:54 # An error report file with more information is saved as:
17:34:54 # /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request+packaging-tests-sample/hs_err_pid2493731.log

:(

jkakavas · 2019-10-12T11:10:35Z

@elasticmachine run elasticsearch-ci/packaging-sample

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 assuming all HTTP headers are single valued and throwing an error if a header with many values is sent. Backport of elastic#45945 without the breaking change in ActionPlugin#getRestHeaders

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.

jkakavas added >enhancement :Core/Infra/REST API REST infrastructure and utilities >breaking-java v8.0.0 labels Aug 25, 2019

Merge remote-tracking branch 'origin/master' into handle-headers-stri…

91f4eca

…ctly

jasontedor self-requested a review August 25, 2019 19:12

bizybot reviewed Aug 25, 2019

View reviewed changes

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java Show resolved Hide resolved

jkakavas requested a review from tvernum September 4, 2019 18:33

tvernum reviewed Sep 25, 2019

View reviewed changes

jkakavas added 2 commits September 25, 2019 10:08

address feedback

0f5cb21

Merge remote-tracking branch 'origin/master' into handle-headers-stri…

8bb69ec

…ctly

tvernum approved these changes Sep 25, 2019

View reviewed changes

jasontedor reviewed Oct 8, 2019

View reviewed changes

jkakavas added 3 commits October 8, 2019 16:38

address feedback

6fdb920

Merge remote-tracking branch 'origin/master' into handle-headers-stri…

97d1da4

…ctly

Remove unused import

a94639d

jasontedor reviewed Oct 8, 2019

View reviewed changes

further feedback

04ef2c3

jasontedor approved these changes Oct 8, 2019

View reviewed changes

jkakavas added 2 commits October 11, 2019 14:11

Merge remote-tracking branch 'origin/master' into handle-headers-stri…

545a64d

…ctly

Do not set headers with empty values in the thread context

081959b

jkakavas merged commit 1fc1df9 into elastic:master Oct 30, 2019

jkakavas mentioned this pull request Oct 30, 2019

Copy http headers to ThreadContext strictly #48675

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy http headers to ThreadContext strictly #45945

Copy http headers to ThreadContext strictly #45945

jkakavas commented Aug 25, 2019

elasticmachine commented Aug 25, 2019

jkakavas commented Aug 25, 2019

jkakavas commented Aug 26, 2019

jkakavas commented Sep 4, 2019

tvernum Sep 25, 2019

tvernum Sep 25, 2019

jkakavas commented Sep 25, 2019

jasontedor left a comment

jasontedor Oct 8, 2019

jasontedor left a comment

jasontedor Oct 8, 2019

jasontedor Oct 8, 2019

jasontedor left a comment

jkakavas commented Oct 11, 2019

jkakavas commented Oct 11, 2019

jkakavas commented Oct 11, 2019

jkakavas commented Oct 11, 2019

jkakavas commented Oct 12, 2019

	} catch (IllegalStateException e){
	} catch (IllegalStateException e) {

Copy http headers to ThreadContext strictly #45945

Copy http headers to ThreadContext strictly #45945

Conversation

jkakavas commented Aug 25, 2019

elasticmachine commented Aug 25, 2019

jkakavas commented Aug 25, 2019

jkakavas commented Aug 26, 2019

jkakavas commented Sep 4, 2019

tvernum Sep 25, 2019

Choose a reason for hiding this comment

tvernum Sep 25, 2019

Choose a reason for hiding this comment

jkakavas commented Sep 25, 2019

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Oct 8, 2019

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Oct 8, 2019

Choose a reason for hiding this comment

jasontedor Oct 8, 2019

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

jkakavas commented Oct 11, 2019

jkakavas commented Oct 11, 2019

jkakavas commented Oct 11, 2019

jkakavas commented Oct 11, 2019

jkakavas commented Oct 12, 2019