Add cors support to NioHttpServerTransport #30827

Tim-Brooks · 2018-05-23T23:12:37Z

This is related to #28898. This commit adds cors support to the nio http
transport. Most of the work is copied directly from the netty module
implementation. Additionally, this commit adds tests for the nio http
channel.

elasticmachine · 2018-05-23T23:12:39Z

Pinging @elastic/es-core-infra

Tim-Brooks · 2018-05-23T23:16:06Z

This mostly just copies work over from the netty module. In the future I will work on extracting some of this code into a common classes. But the most straightforward way to implement this at the moment is to copy (as it requires a netty dependency which cannot live in server).

s1monw · 2018-05-28T10:35:06Z

@elastic/es-core-infra @jasontedor @tbrooks8 I don't think I can review this I am lacking knowledge of CORS.

tvernum

I left a few comments.

tvernum · 2018-05-28T10:47:15Z

plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/cors/NioCorsConfig.java

+    }
+
+    /**
+     * Determines if cookies are supported for CORS requests.


I hate to get pedantic on Javadoc, but within CORS, credentials refers to more than just cookies.

Per: https://www.w3.org/TR/cors/#user-credentials

The term user credentials for the purposes of this specification means cookies, HTTP authentication, and client-side SSL certificates that would be sent based on the user agent's previous interactions with the origin

tvernum · 2018-05-28T10:59:10Z

plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/cors/NioCorsHandler.java

+            final String originHeaderVal;
+            if (config.isAnyOriginSupported()) {
+                originHeaderVal = ANY_ORIGIN;
+            } else if (config.isOriginAllowed(originHeader) || isSameOrigin(originHeader, request.headers().get(HttpHeaderNames.HOST))) {


This isSameOrigin check seems out of place.
It seems that we implicity treat any origins on the same host as being allowed, even if the browser thinks it's cross-origin.
Since an origin consists of the triplet ( scheme , host , port ), this would allow any web-app running on the same host to have access the ES server (if CORS is enabled) regardless of its actual origin (scheme/port).

Opened an issue (#30988)

tvernum · 2018-05-28T11:10:48Z

plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/cors/NioCorsHandler.java

+
+    private boolean validateOrigin() {
+        if (config.isAnyOriginSupported()) {
+            return true;


This will include null origins, which I think is the right thing to do, but probably should be explicitly stated in the javadocs for isAnyOriginSupported.

I updated the java doc.

tvernum · 2018-05-28T11:10:49Z

plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/cors/NioCorsHandler.java

+        }
+
+        if ("null".equals(origin) && config.isNullOriginAllowed()) {
+            return true;


I think this should be something more like:

if ("null".equals(origin)) { return config.isNullOriginAllowed(); }

I don't think it makes sense to fall through to pattern checking the origin is null, and null is not allowed.
That is, if we have an explicit config option for null handling, then that should be the only way to determine whether null is allowed.

I actually do not think this is correct based on how we have implemented Cors. Our setup, means that config.isNullOriginAllowed will always be false. We never modify that config. We only allow configuring config.isOriginAllowed(...) through http.cors.allow-origin. So if the user wants to allow null, that must be in the regex. We will only evaluate that regex if we fall through.

I actually do not think this is correct based on how we have implemented Cors.

This is an intricacy of the fact that the CorsConfig class that we have copied over from netty allows a lot of different configs. But we really only use the pattern matching.

I have a (non-blocking) concern about copying code across with the obligations to maintain it, but only use part of it.
Is there an argument for why we're keeping parts of this that we don't actually use?

I'm not sure why we took the approach we did. It is on my roadmap to change at some point.

I have started playing around with implementing Cors on our side opposed to Netty's side (https://github.com/tbrooks8/elasticsearch/tree/cors_without_netty). Essentially we would do the Cors logic AFTER we have turned a netty http request into a RestRequest. And then nio / netty4 modules would use the same code base (as the logic would live in server). If that turns out to be too complicated, I can explore paring down what we use from netty.

Tim-Brooks · 2018-05-31T04:18:13Z

Thanks for the feedback @tvernum. I want to provide some context for this PR. It is just copying our work form the netty module and modifying it to work with nio facilities. All the logic is the same.

NioCorsHandler = Netty4CorsHandler
NioCorsConfig = Netty4CorsConfig
...etc

Additionally these are pretty much pulled from the netty project (CorsConfig, CorsHandler). The java docs are from netty.

I guess my point here is: all of the feedback you provided applies to both implementations. I opened an issue for one of your points. I think that should be addressed separately. On the two documentation points I made slight modifications to the documentation from netty. As that was pretty simple.

tvernum

I'm happy with this as is, but I do think we should go through and remove the bits that we don't actually need/use. But that doesn't have to happen in this PR.

* elastic/master: Add cors support to NioHttpServerTransport (elastic#30827) [DOCS] Fixes security example (elastic#31082) Allow terms query in _rollup_search (elastic#30973) Removing erroneous repeat Adapt bwc versions after backporting elastic#30983 to 6.4

* elastic/master: [DOCS] Creates rest-api folder in docs [Rollup] Disallow index patterns that match the rollup index (#30491) Add cors support to NioHttpServerTransport (#30827) [DOCS] Fixes security example (#31082) Allow terms query in _rollup_search (#30973)

* ccr: [DOCS] Creates rest-api folder in docs [Rollup] Disallow index patterns that match the rollup index (elastic#30491) Replace exact numDocs by soft-del count in SegmentInfo (elastic#31086) Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (elastic#31073) Add cors support to NioHttpServerTransport (elastic#30827) [DOCS] Fixes security example (elastic#31082) Allow terms query in _rollup_search (elastic#30973) Removing erroneous repeat Adapt bwc versions after backporting elastic#30983 to 6.4 [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (elastic#30978) Fix docs build. Only auto-update license signature if all nodes ready (elastic#30859) Add BlobContainer.writeBlobAtomic() (elastic#30902) Add a doc value format to binary fields. (elastic#30860)

Tim-Brooks added 3 commits May 23, 2018 14:43

Bring over files

554e04c

Work on Cors

aeff0d2

Add cors and tests to nio

42a94b8

Tim-Brooks added >enhancement review :Distributed/Network Http and internode communication implementations v7.0.0 labels May 23, 2018

Tim-Brooks requested review from s1monw and jasontedor May 23, 2018 23:12

tvernum reviewed May 28, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into add_cors_support

0ed278b

Tim-Brooks added 3 commits May 30, 2018 22:21

Documentation updates

1e4445d

Fix compile

5667ca9

Merge remote-tracking branch 'upstream/master' into add_cors_support

208ba87

Tim-Brooks requested a review from tvernum June 4, 2018 15:29

tvernum approved these changes Jun 5, 2018

View reviewed changes

Tim-Brooks merged commit 05ee0f8 into elastic:master Jun 5, 2018

Tim-Brooks deleted the add_cors_support branch November 14, 2018 14:54

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cors support to NioHttpServerTransport #30827

Add cors support to NioHttpServerTransport #30827

Tim-Brooks commented May 23, 2018

elasticmachine commented May 23, 2018

Tim-Brooks commented May 23, 2018

s1monw commented May 28, 2018

tvernum left a comment

tvernum May 28, 2018

tvernum May 28, 2018

Tim-Brooks May 31, 2018

tvernum May 28, 2018

Tim-Brooks May 31, 2018

tvernum May 28, 2018

Tim-Brooks May 31, 2018

Tim-Brooks May 31, 2018

tvernum Jun 5, 2018

Tim-Brooks Jun 5, 2018

Tim-Brooks commented May 31, 2018

tvernum left a comment

Add cors support to NioHttpServerTransport #30827

Add cors support to NioHttpServerTransport #30827

Conversation

Tim-Brooks commented May 23, 2018

elasticmachine commented May 23, 2018

Tim-Brooks commented May 23, 2018

s1monw commented May 28, 2018

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Brooks commented May 31, 2018

tvernum left a comment

Choose a reason for hiding this comment