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

Add ability to specify CORS accepted origins #84316

Merged
merged 25 commits into from
Dec 10, 2020
Merged

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Nov 25, 2020

Resolves #16714.

Summary

The current implementation is based on a discussion in the parent issue.
Kibana supports only 3 CORS options at the moment:

  • server.cors.enabled Set to true to allow cross-origin API calls. Default: false
  • server.cors.credentials Set to true to allow browser code to access response body whenever request performed with user credentials. Default: false
  • server.cors.origin List of origins permitted to access resources. You must specify server.cors.origin when server.cors.credentials: true. Default: "*"

Kibana extend Access-Control-Allow-Headers list with the next headers: ['Accept', 'Authorization', 'Content-Type', 'If-None-Match', 'kbn-xsrf']

Checklist

Delete any items that are not applicable to this PR.

Release Notes

Added experimental support for configuring CORS policy:

  • server.cors.enabled Set to true to allow cross-origin API calls. Default: false
  • server.cors.allowCredentials Set to true to allow browser code to access response body whenever request performed with user credentials. Default: false
  • server.cors.allowOrigin List of origins permitted to access resources. You must specify server.cors.allowOrigin when server.cors.allowCredentials: true. Default: ["*"]

@mshustov mshustov added release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc enhancement New value added to drive a business result v8.0.0 v7.11.0 labels Nov 25, 2020
@@ -445,6 +445,15 @@ deprecation warning at startup. This setting cannot end in a slash (`/`).
| [[server-compression]] `server.compression.enabled:`
| Set to `false` to disable HTTP compression for all responses. *Default: `true`*

| `server.cors.enabled:`
| experimental[] Set to `true` to allow cross-origin API calls. *Default:* `false`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kobelb We should start with marking it as experimental, I suppose

Copy link
Member

Choose a reason for hiding this comment

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

If we're marking the old server.cors setting as deprecated, then I'm not sure we should replace it with an experimental setting. Is there a reason to believe this won't be stable enough to mark as GA?

@mshustov mshustov added the docs label Nov 26, 2020
@mshustov mshustov marked this pull request as ready for review November 26, 2020 10:27
@mshustov mshustov requested a review from a team as a code owner November 26, 2020 10:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@mshustov mshustov changed the title [WIP] Add ability to specify CORS accepted origins Add ability to specify CORS accepted origins Nov 26, 2020
Comment on lines +16 to +28
it('Communicates to Kibana with configured CORS', async () => {
const args: string[] = config.get('kbnTestServer.serverArgs');
const originSetting = args.find((str) => str.includes('server.cors.origin'));
if (!originSetting) {
throw new Error('Cannot find "server.cors.origin" argument');
}
const [, value] = originSetting.split('=');
const url = JSON.parse(value);

await browser.navigateTo(url[0]);
const element = await find.byCssSelector('p');
expect(await element.getVisibleText()).to.be('content from kibana');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

:notbad:

Copy link
Member

Choose a reason for hiding this comment

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

++ Nice test!

@mshustov mshustov requested a review from legrego December 1, 2020 13:48
src/core/server/http/http_config.test.ts Outdated Show resolved Hide resolved
Comment on lines +16 to +28
it('Communicates to Kibana with configured CORS', async () => {
const args: string[] = config.get('kbnTestServer.serverArgs');
const originSetting = args.find((str) => str.includes('server.cors.origin'));
if (!originSetting) {
throw new Error('Cannot find "server.cors.origin" argument');
}
const [, value] = originSetting.split('=');
const url = JSON.parse(value);

await browser.navigateTo(url[0]);
const element = await find.byCssSelector('p');
expect(await element.getVisibleText()).to.be('content from kibana');
});
Copy link
Member

Choose a reason for hiding this comment

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

++ Nice test!

Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, @restrry.

Without the ability to customize the access-control-allow-headers response header and Kibana not responding to the OPTIONS pre-flights, the user will be limited to the APIs that they can call using CORS. Do we intend to add those in a separate PR, or is there a reason why we shouldn't do so?

docs/setup/settings.asciidoc Outdated Show resolved Hide resolved
@mshustov
Copy link
Contributor Author

mshustov commented Dec 3, 2020

Without the ability to customize the access-control-allow-headers response header and Kibana not responding to the OPTIONS pre-flights, the user will be limited to the APIs that they can call using CORS. Do we intend to add those in a separate PR, or is there a reason why we shouldn't do so?

@kobelb What kind of API is cannot be used with the current implementation? I can think of xsrf protection only, but that might be configured via server.xsrf.allowlist.

@kobelb
Copy link
Contributor

kobelb commented Dec 3, 2020

@kobelb What kind of API is cannot be used with the current implementation? I can think of xsrf protection only, but that might be configured via server.xsrf.allowlist.

We shouldn't be adding APIs to the server.xsrf.allowlist for them to work with CORS. CORS allows us to relax the same-origin restrictions in a selective manner, based on the origin, headers, etc.

Currently, all non-GET HTTP APIs in Kibana don't work with CORS. Kibana requires that all non-GET HTTP requests have the kbn-xsrf header specified for our XSRF protection. This custom header causes the user's browser to send a pre-flight OPTIONS request, that includes the origin, to Kibana. This allows Kibana to selectively determine which origins we're comfortable relaxing the same-origin restrictions and accepting requests from. However, these OPTIONS requests are failing right now:

Screen Shot 2020-12-03 at 7 50 05 AM

FWIW, my prior statement about Kibana not responding to OPTIONS requests was incorrect. In prior versions of Kibana, I recall Kibana not responding to any HTTP requests using the OPTIONS verb, but it appears that Kibana is now.

@mshustov
Copy link
Contributor Author

mshustov commented Dec 8, 2020

@kobelb I adopted the code to allow kbn-xsrf header to be set on CORS request 3e19081, PTAL

@mshustov mshustov requested a review from kobelb December 8, 2020 17:51
@kobelb
Copy link
Contributor

kobelb commented Dec 9, 2020

Thanks for making these changes, @restrry. This is looking great!

@mshustov mshustov merged commit 44688d9 into elastic:master Dec 10, 2020
@mshustov mshustov deleted the add-cors branch December 10, 2020 14:14
mshustov added a commit to mshustov/kibana that referenced this pull request Dec 10, 2020
* add settings

* update abab package to version with types

* add test case for CORS

* add tests for cors config

* fix jest tests

* add deprecation message

* tweak deprecation

* make test runable on Cloud

* add docs

* fix type error

* add test to throw on invalid URL

* address comments

* Update src/core/server/http/http_config.test.ts

Co-authored-by: Larry Gregory <[email protected]>

* Update docs/setup/settings.asciidoc

Co-authored-by: Brandon Kobel <[email protected]>

* allow kbn-xsrf headers to be set on CORS request

Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: Brandon Kobel <[email protected]>
# Conflicts:
#	src/core/server/config/deprecation/core_deprecations.ts
#	x-pack/scripts/functional_tests.js
mshustov added a commit that referenced this pull request Dec 10, 2020
* add settings

* update abab package to version with types

* add test case for CORS

* add tests for cors config

* fix jest tests

* add deprecation message

* tweak deprecation

* make test runable on Cloud

* add docs

* fix type error

* add test to throw on invalid URL

* address comments

* Update src/core/server/http/http_config.test.ts

Co-authored-by: Larry Gregory <[email protected]>

* Update docs/setup/settings.asciidoc

Co-authored-by: Brandon Kobel <[email protected]>

* allow kbn-xsrf headers to be set on CORS request

Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: Brandon Kobel <[email protected]>
# Conflicts:
#	src/core/server/config/deprecation/core_deprecations.ts
#	x-pack/scripts/functional_tests.js
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 46990 47750 +760

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs enhancement New value added to drive a business result release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to specify CORS accepted origins
6 participants