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

[Canvas] - Allow timelion data source to use configured certificateAuthorities #26809

Merged
merged 6 commits into from Dec 19, 2018

Conversation

@legrego
Copy link
Contributor

commented Dec 7, 2018

@monfera

This enables the Timelion datasource when Kibana is configured to use https with a custom server.ssl.certificateAuthorities list.

The solution is to configure Axios to use a custom httpsAgent, which itself is configured to use the custom certificate authorities, if provided.

I tested this with a properly configured Kibana with SSL enabled with a certificate/ca pair, but I have not tested other configurations. This will likely need fine tuning to work with all configurations (ssl configured but disabled, ssl not configured, certificate configured with a system trusted CA)

Fixes #26308
Potentially solves #25742

TODO:
We we should remove this code once we have a common mechanism for augmenting the CA list. Ideally, we should use NODE_EXTRA_CA_CERTS or a similar mechanism to allow the entire runtime to trust user-supplied CAs via kibana.yml

import { fetch } from '../../../../common/lib/fetch';
import { buildBoolArray } from '../../../../server/lib/build_bool_array';

const readFile = file => readFileSync(file, 'utf8');

function parseConfig(sslConfig = {}) {

This comment has been minimized.

@legrego

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

@@ -27,6 +27,9 @@ export const createHandlers = (request, server) => {

return {
environment: 'server',
// A temporary measure to allow the timelion data source to negotiate secure connections to the Kibana server.
// See https://github.com/elastic/kibana/pull/26809 and https://github.com/elastic/kibana/issues/26812
__dangerouslyUnsupportedSslConfig: server.config().get('server.ssl'),

This comment has been minimized.

Copy link
@rashidkpc

rashidkpc Dec 7, 2018

Member

As noted by the comments, this needs to go.

We are very careful about expanding our API, especially as it relates to handlers. Handlers should never contain complex undocumented objects or methods. If we can get away with having nothing at all on handlers we should.

The only function we currently expose on handlers is the elasticsearch client (as wrapped by callWithRequest) and while we ere somewhat uncomfortable with it went ahead because the client is specced, staffed, versioned, documented, and pegged to the version of Elasticsearch that Kibana requires.

@legrego and I agreed on this only as a very temporary measure and agreed that the right solution here is for Kibana's node environment to support trust configuration. Hence the naming.

This comment has been minimized.

Copy link
@clintandrewhall

clintandrewhall Dec 18, 2018

Contributor

@legrego I'm fine to stamp this PR, but I'd like a follow-up issue, as a blocker to 6.7, and mentioned here.

@rashidkpc rashidkpc requested review from rashidkpc and w33ble Dec 7, 2018

@rashidkpc

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

I tested that this works without SSL, and @legrego tested that it works with SSL. We're not confident that it will work with all possible SSL configurations, nor even that it actually works on Cloud. This will need someone to coordinate a cloud instance with this pull installed so we can verify. @alexfrancoeur do you have time to help coordinate?

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

@rashidkpc rashidkpc removed their request for review Dec 11, 2018

@rayafratkina

This comment has been minimized.

Copy link

commented Dec 18, 2018

@legrego any update on this one?
cc @clintandrewhall

@legrego

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

I think we're waiting to see if this works on cloud, but I'm not sure of the best way to do that. @alexfrancoeur do you know what that process looks like? Do we need to merge this first, or is is possible to get a build on cloud from here?

@alexfrancoeur

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

@legrego I believe we need to merge in order to test on a cloud nightly snapshot. We've done some custom things in the past to build off a feature branch but I don't believe it's common. @joegallo is there another process here I'm not aware of?

@legrego legrego changed the title [WIP - Canvas] - Allow timelion data source to use configured certificateAuthorities [Canvas] - Allow timelion data source to use configured certificateAuthorities Dec 18, 2018

@legrego

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

@w33ble @rashidkpc @clintandrewhall

Unless @joegallo has options, are there any objections to merging this to master in order to test this on cloud? If not, a LGTM would be awesome 😄

As Rashid noted above in his review, this is a temporary measure, and shouldn't be considered long-lived code.

@elasticmachine

This comment was marked as outdated.

Copy link
Contributor

commented Dec 18, 2018

@legrego legrego requested a review from elastic/kibana-canvas as a code owner Dec 18, 2018

@elasticmachine

This comment was marked as outdated.

Copy link
Contributor

commented Dec 18, 2018

@clintandrewhall
Copy link
Contributor

left a comment

I'm stamping this to unblock, but please add a follow-up task as a blocker to 6.7 so we can be sure this is handled later.

Update src/legacy/core_plugins/interpreter/server/lib/create_handlers.js
Co-Authored-By: legrego <lgregorydev@gmail.com>
@legrego

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

Added #27437.
I'll merge once CI goes green, and backport to 6.x

@clintandrewhall

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

Thanks @legrego ...!

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

@w33ble

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

You don't need to wait for me if you want to merge this sooner than later, but I'd still like to learn more about this and what the more "optimal" fix would be. Planning to chat with @legrego tomorrow.

@legrego legrego merged commit 2bbc821 into elastic:master Dec 19, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

@legrego legrego deleted the legrego:canvas/trust-custom-cas branch Dec 19, 2018

legrego added a commit to legrego/kibana that referenced this pull request Dec 19, 2018
[Canvas] - Allow timelion data source to use configured certificateAu…
…thorities (elastic#26809)

* allow timelion data source to use configured certificateAuthorities

* make it obvious that this is a temporary measure

* fix lint error

* Update src/legacy/core_plugins/interpreter/server/lib/create_handlers.js

Co-Authored-By: legrego <lgregorydev@gmail.com>

* fix lint error from comment updated via GH
legrego added a commit that referenced this pull request Dec 19, 2018
[Canvas] - Allow timelion data source to use configured certificateAu…
…thorities (#26809) (#27487)

* allow timelion data source to use configured certificateAuthorities

* make it obvious that this is a temporary measure

* fix lint error

* Update src/legacy/core_plugins/interpreter/server/lib/create_handlers.js

Co-Authored-By: legrego <lgregorydev@gmail.com>

* fix lint error from comment updated via GH
@alexfrancoeur

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

@legrego

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

We made 6.6, but I'm not aware of another 6.5 patch release. If there is one then I don't mind backporting further.

@alexfrancoeur

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

w33ble added a commit to w33ble/kibana that referenced this pull request Jan 2, 2019
chore: revert elastic#26809
it was a temp fix meant to be removed anyway
w33ble added a commit that referenced this pull request Jan 3, 2019
Fix: Make timelion a browser function (#27944)
* chore: revert #26809

it was a temp fix meant to be removed anyway

* chore: convert timelion to private browser function

allows access to ui/chrome, required for determining the server basepath
w33ble added a commit to w33ble/kibana that referenced this pull request Jan 3, 2019
Fix: Make timelion a browser function (elastic#27944)
* chore: revert elastic#26809

it was a temp fix meant to be removed anyway

* chore: convert timelion to private browser function

allows access to ui/chrome, required for determining the server basepath
w33ble added a commit to w33ble/kibana that referenced this pull request Jan 3, 2019
Fix: Make timelion a browser function (elastic#27944)
* chore: revert elastic#26809

it was a temp fix meant to be removed anyway

* chore: convert timelion to private browser function

allows access to ui/chrome, required for determining the server basepath
w33ble added a commit that referenced this pull request Jan 3, 2019
Fix: Make timelion a browser function (#27944) (#28015)
* chore: revert #26809

it was a temp fix meant to be removed anyway

* chore: convert timelion to private browser function

allows access to ui/chrome, required for determining the server basepath
w33ble added a commit that referenced this pull request Jan 3, 2019
Fix: Make timelion a browser function (#27944) (#28014)
* chore: revert #26809

it was a temp fix meant to be removed anyway

* chore: convert timelion to private browser function

allows access to ui/chrome, required for determining the server basepath
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.