Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More robust handling of CORS HTTP Access Control #16436

Merged
merged 3 commits into from Feb 4, 2016

Conversation

Projects
None yet
7 participants
@abeyad
Copy link
Contributor

commented Feb 4, 2016

Uses a refactored version of Netty's CORS implementation to provide more
robust cross-origin resource request functionality. The CORS specific
Elasticsearch parameters remain the same, just the underlying
implementation has changed.

It has also been refactored in a way that allows dropping in Netty's
CORS handler as a replacement once Elasticsearch is upgraded to Netty 4.

@abeyad abeyad added the review label Feb 4, 2016

@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2016

@spinscale I had to make a few changes to use Java 7 only features for 2.x, so if you can review before merging. All tests pass locally.

@spinscale

View changes

core/src/test/java/org/elasticsearch/rest/CorsRegexIT.java Outdated
@@ -66,10 +68,11 @@ public void testThatRegularExpressionWorksOnMatch() throws Exception {
assertThat(response.getHeaders().get("Access-Control-Allow-Credentials"), is("true"));
}

@Test

This comment has been minimized.

Copy link
@spinscale

spinscale Feb 4, 2016

Member

just leave that here for consistency? the other tests have it as well

This comment has been minimized.

Copy link
@abeyad

abeyad Feb 4, 2016

Author Contributor

fixed

Ali Beyad
More robust handling of CORS HTTP Access Control
Uses a refactored version of Netty's CORS implementation to provide more
robust cross-origin resource request functionality.  The CORS specific
Elasticsearch parameters remain the same, just the underlying
implementation has changed.

It has also been refactored in a way that allows dropping in Netty's
CORS handler as a replacement once Elasticsearch is upgraded to Netty 4.

@abeyad abeyad force-pushed the abeyad:cors-handling-v2.x branch to 56f2a12 Feb 4, 2016

@spinscale

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

LGTM

@abeyad abeyad removed the review label Feb 4, 2016

@abeyad abeyad changed the title Cors handling v2.x More robust handling of CORS HTTP Access Control Feb 4, 2016

abeyad pushed a commit that referenced this pull request Feb 4, 2016

Ali Beyad
Merge pull request #16436 from abeyad/cors-handling-v2.x
More robust handling of CORS HTTP Access Control

@abeyad abeyad merged commit e20b207 into elastic:2.x Feb 4, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details
@clintongormley

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

@abeyad this should go into master as well, no?

@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2016

@clintongormley

This comment has been minimized.

Copy link
Member

commented Feb 13, 2016

@abeyad ah thanks - i missed that

@aleybovich

This comment has been minimized.

Copy link

commented Apr 4, 2016

Looks like it broke CORS support - after upgrading 2.2.2 -> 2.3.1 I am getting this error:

XMLHttpRequest cannot load http://xxx.xxx.xxx.xxx:9200/_search. Request header field Content-Type is not allowed by Access-Control-Allow-Headers in preflight response.

I have the same http settings in elasticsearch.yml as 2.2.2:

http.cors.enabled: true
http.cors.allow-methods: OPTIONS, HEAD, GET, POST, PUT, DELETE
http.cors.allow-headers: X-Requested-With,X-Auth-Token,Content-Type,Content-Length
http.cors.allow-origin: "*"

Any changes in settings required for 2.3?

@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2016

@aleybovich Just to be clear, you sent an OPTIONS http request with the Origin and Access-Control-Request-Method set in the http headers?

And what was the error response / message exactly?

@aleybovich

This comment has been minimized.

Copy link

commented Apr 4, 2016

I use SearchKit UI running locally; ES is also running locally. When I run ES2.3 and searchkit does a search, I see the following in dev console:
The request type is OPTOINS (instead of POST when ES 2.2 is running);

Request URL:http://localhost:9200/_search
Request Method:OPTIONS
Status Code:200 OK
Remote Address:127.0.0.1:9200
Response Headers
view source
Access-Control-Allow-Methods:
Access-Control-Allow-Origin:*
Access-Control-Max-Age:1728000
content-length:0
date:Mon, 04 Apr 2016 23:04:50 GMT
Request Headers
view source
Accept:/
Accept-Encoding:gzip, deflate, sdch
Accept-Language:en-US,en;q=0.8
Access-Control-Request-Headers:accept, content-type
Access-Control-Request-Method:POST
Cache-Control:max-age=0
Connection:keep-alive
Host:localhost:9200
Origin:http://localhost:8080
Referer:http://localhost:8080/
User-Agent:Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.110 Safari/537.36

Request return 200 but in JS console I see:

XMLHttpRequest cannot load http://xxx.xxx.xxx.xxx:9200/_search. Request header field Content-Type is not allowed by Access-Control-Allow-Headers in preflight response

preflightHeaders.put("content-length", new ConstantValueGenerator("0"));
}
return new CorsConfig(this);
}

This comment has been minimized.

Copy link
@BobChao87

BobChao87 Apr 4, 2016

Suspect problem with CORS requests is from this function, since I am seeing "content-length" and "date" in my response as lowercase, which they appear as here. Should be "Content-Length", "Date", and include a "Content-Type" and possibly the "X-Requested-With", though that doesn't seem to be the cause of the break, to match the defaults as mentioned on NettyHttpServerTransport#L109

For the purposes of seeing changes, prior to 2.3.x, response contained
Content-Type: text/plain; charset=UTF-8
and did not include "Date" or "X-Requested-With".

@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2016

@aleybovich Thank you for finding this issue! It is indeed a legitimate issue and I created a fix for it at #17524 (also created a fix for master).

@aleybovich

This comment has been minimized.

Copy link

commented Apr 5, 2016

Thank you for addressing it! Do you know when a build with this fix would
be released?

On Tuesday, April 5, 2016, Ali Beyad notifications@github.com wrote:

@aleybovich https://github.com/aleybovich Thank you for finding this
issue! It is indeed a legitimate issue and I created a fix for it at
#17524 #17524 (also
created a fix for master).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#16436 (comment)

@marcoplaut

This comment has been minimized.

Copy link

commented Apr 11, 2016

are there any work around to avoid the problem currently ?

@aleybovich

This comment has been minimized.

Copy link

commented Apr 11, 2016

We have to roll back to 2.2.2 until the CORS issue is fixed.
On Apr 11, 2016 7:15 AM, "marcoplaut" notifications@github.com wrote:

are there any work around to avoid the problem currently ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#16436 (comment)

@marcoplaut

This comment has been minimized.

Copy link

commented Apr 11, 2016

if that could help someone, having nginx as reverse proxy in front of my elasticsearch, i solve the problem following this post: http://enable-cors.org/server_nginx.html

@abeyad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

If you depend on CORS pre-flight requests working correctly, @aleybovich 's suggestion is correct to roll back until 2.3.2 is released.

@hackel

This comment has been minimized.

Copy link

commented Apr 19, 2016

I just got hit by this as well, and wasted quite a bit of time debugging before I landed here. This is a really critical bug. Would be great if you added a big, fat warning about this issue at least to the release notes, if not to the homepage of elastic.io. This basically disables remote elasticsearch functionality entirely.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

Would be great if you added a big, fat warning about this issue at least to the release notes,

@hackel, we did: https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-2.3.html

@hackel

This comment has been minimized.

Copy link

commented Apr 19, 2016

Aha, thanks I missed that. I was looking at https://www.elastic.co/guide/en/elasticsearch/reference/current/release-notes-2.3.0.html I didn't realise there was a separate page for (more?) breaking changes. My bad, I guess.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

@hackel i've added a link to the relevant breaking changes section from each page of release notes: f9a64ea

Should help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.