Skip to content

Fixes CORS headers needed by Elastic clients #85791

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

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

swallez
Copy link
Member

@swallez swallez commented Apr 11, 2022

This PR updates the default values for CORS headers so that Elasticsearch client libraries that can run in a web browser (e.g. JavaScript client, Go & Rust clients compiled to WebAssembly) work out of the box by just enabling http.cors.enabled: true and setting http.cors.allow-origin.

It adds the following headers to http.cors.allow-headers's default value: Authorization for webapp-level authentication, Accept, and User-Agent and X-Elastic-Client-Meta that are useful for telemetry.

Additionally, a new http.cors.expose-headers setting allows configuring headers that are exposed to the code in the browser, defaulting to X-elastic-product which allows clients to successfully run product checks.

See also elastic/elasticsearch-rs#200

Updates the default value for the `http.cors.allow-headers`
setting to include headers used by Elastic client libraries.

Also adds the `access-control-expose-headers` header to responses to
CORS requests so that clients can successfully perform their product
check.
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Apr 11, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added v8.3.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Apr 11, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @swallez, I've created a changelog YAML for you.

@rjernst rjernst self-requested a review May 3, 2022 15:21
@tvernum tvernum requested review from Tim-Brooks and tvernum June 2, 2022 13:10
@@ -110,9 +110,16 @@ Which methods to allow. Defaults to `OPTIONS, HEAD, GET, POST, PUT, DELETE`.
// tag::http-cors-allow-headers-tag[]
`http.cors.allow-headers` {ess-icon}::
(<<static-cluster-setting,Static>>)
Which headers to allow. Defaults to `X-Requested-With, Content-Type, Content-Length`.
Which headers to allow. Defaults to `X-Requested-With, Content-Type, Content-Length, Authorization, Accept, User-Agent, X-Elastic-Client-Meta`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it strange that we default access-control-allow-headers to include Authorization but access-control-allow-credentials to false?

Is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is intentional:

  • access-control-allow-headers, in a response to a preflight request, defines what headers (outside of the safelist) are allowed in CORS requests. Since clients set the Authorization header in their requests, it has to be allowed by the server in preflight responses.
  • Defaulting access-control-allow-credentials to false forbids the use of browser-level credentials by clients, and thus requires clients to be provided with credential information that is then set in the Authorization header.

CORS is sometimes confusing 😅

@elasticsearchmachine elasticsearchmachine changed the base branch from master to main July 22, 2022 23:07
@mark-vieira mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022
@csoulios csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022
@kingherc kingherc added v8.7.0 and removed v8.6.0 labels Nov 16, 2022
@swallez
Copy link
Member Author

swallez commented Jan 24, 2023

@tvernum I'm reviving this old PR. Can you please have look at my reply to your comments?

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but we should check whether @rjernst still wants to review.

@rjernst
Copy link
Member

rjernst commented Feb 6, 2023

I don't have any thoughts here.

@rjernst rjernst removed the v8.7.0 label Feb 8, 2023
@rjernst rjernst added the v8.8.0 label Feb 8, 2023
@swallez swallez merged commit 484d3f4 into elastic:main Feb 9, 2023
@swallez swallez deleted the fix-cors-for-clients branch February 9, 2023 15:44
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
7.17 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 85791

@swallez
Copy link
Member Author

swallez commented Feb 9, 2023

💚 All backports created successfully

Status Branch Result
7.17

Questions ?

Please refer to the Backport tool documentation

swallez added a commit to swallez/elasticsearch that referenced this pull request Feb 9, 2023
* Fixes CORS headers needed by Elastic clients

Updates the default value for the `http.cors.allow-headers`
setting to include headers used by Elastic client libraries.

Also adds the `access-control-expose-headers` header to responses to
CORS requests so that clients can successfully perform their product
check.

(cherry picked from commit 484d3f4)
swallez added a commit that referenced this pull request Feb 9, 2023
* Fixes CORS headers needed by Elastic clients

Updates the default value for the `http.cors.allow-headers`
setting to include headers used by Elastic client libraries.

Also adds the `access-control-expose-headers` header to responses to
CORS requests so that clients can successfully perform their product
check.

(cherry picked from commit 484d3f4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Core/Infra Meta label for core/infra team v7.17.10 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants