Disable creating alert client instances when ESO plugin is using an ephemeral encryption key #56676

mikecote · 2020-02-03T20:46:36Z

Resolves the alerting portion of #56420.

In this PR, the alerts client will throw an error whenever trying to create an instance of the client (getAlertsClient via getAlertsClientWithRequest) and the encrypted saved objects plugin is using an ephemeral encryption key.

Error message:

Unable to create alerts client due to the Encrypted Saved Objects plugin using an ephemeral encryption key. Please set xpack.encryptedSavedObjects.encryptionKey in kibana.yml

elasticmachine · 2020-02-03T20:46:38Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

peterschretlen

I realize it's not much of a concern in 7.6, but wouldn't we want to disable the creation of actions/connectors as well if an ephemeral key is used?

mikecote · 2020-02-04T13:19:54Z

@peterschretlen

I realize it's not much of a concern in 7.6, but wouldn't we want to disable the creation of actions/connectors as well if an ephemeral key is used?

I agree, I was thinking of doing those in a separate PR and only backport this PR (alert APIs) into 7.6? Happy to backport both otherwise. I will keep the main GH issue #56420 open until both API sets are fixed and backported properly.

mikecote · 2020-02-04T14:03:40Z

x-pack/legacy/plugins/alerting/server/plugin.ts

@@ -119,6 +121,7 @@ export class Plugin {
      taskManager: plugins.taskManager,
      securityPluginSetup: plugins.security,
      encryptedSavedObjectsPlugin: plugins.encryptedSavedObjects,
+      isESOUsingEphemeralEncryptionKey: this.isESOUsingEphemeralEncryptionKey!,


Should we move the logic from the alerts client and move it into getAlertsClientWithRequest function below? 🤔 There's also the alerts client factory that could handle it.

This would mean we throw this error at a higher level with more ways around it but a lot less code.

Seems like the answer would be yes. Unless the encryption key can be set in the config "live" via the NP plugin live config bits. In that case, we'd need to rebuild the alerts client at the same time, at which point it would be happy with the key.

seconded 👍
That makes more sense to me too

The one holdback I have is that we expose the AlertsClient class (https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/alerting/index.ts#L14) so others who use the class directly wouldn't be protected anymore. 🤔 We should probably just expose the interface instead of the class.

++ to pulling it up to getAlertsClientWithRequest

Ya, exposing an interface would be better, eg we could have a "disabled" version of the class which just threw errors for all the API calls, which we returned in this case, instead of the "real" one.

But it also makes me wonder if there are some APIs we should actually allow. find() and get() seem ok to me, and I'm wondering if that would be useful to have available in this case. Probably doesn't matter, since you can't really do anything else at this point anyway without fixing the encryption key config ...

…ephemeral encryption key

mikecote · 2020-02-04T18:41:42Z

@gmmorris @peterschretlen @pmuellr let me know your thoughts. I've made the changes based on the discussion we've had here: #56676 (comment).

pmuellr

LGTM

Initially, this felt kind of "blunt", I guess I was thinking we'd return an alert client that implemented all it's methods as things that threw the exception, instead of throwing when trying to get the alert client. But after seeing this, I think it's probably better, as it let's a plugin using alerting to be able to make the determination up front, instead of at every point they use the alert client. I guess we'll find out.

Was looking at our routes, and I suspect we will want them enhanced. Eg

kibana/x-pack/legacy/plugins/alerting/server/routes/create.ts

Lines 68 to 71 in 885b79e

    
           async handler(request: ScheduleRequest) { 
        
             const alertsClient = request.getAlertsClient!(); 
        
             return await alertsClient.create({ data: request.payload }); 
        
           },

Won't it throw something in that reques.getAlertsClient!() call now, that it wouldn't before? I think the result would be that you'd get a 400 "invalid request" kind of result, but it would be nice to actually indicate what the error was. Or maybe since it's security-related, it would be best NOT to. Worth noodling over a bit (separate issue), I think it's certainly survivable as-is for now.

kibanamachine · 2020-02-04T19:44:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b34fa68

History

💚 Build #24307 succeeded bce4018e55a70c17e145787e9cc81579eac2f9ac
💔 Build #24139 failed 4b511a8784d244f7234387976eed29979d0ce09d

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

peterschretlen

LGTM 👍 Tested it out with the management UI and detection engine and confirmed you can't create or list alerts. Dev mode works as before with the 'aaaaa....' key, so dev experience is unchanged which is great.

mikecote · 2020-02-04T19:54:05Z

@pmuellr thanks for the review! I will create a follow up issue for your comments: #56676 (review).

…ephemeral encryption key (elastic#56676)

…ng an ephemeral encryption key (#56676) (#56791) * Fix merge conflicts * Fix * Fix test failures

* master: (23 commits) Properly handle password change for users authenticated with provider other than `basic`. (elastic#55206) Improve pull request template proposal (elastic#56756) Only change handlers as the element changes (elastic#56782) [SIEM][Detection Engine] Final final rule changes (elastic#56806) [SIEM][Detection Engine] critical blocker, wrong ilm policy, need to match beats ilm policy Move ui/agg_types in to shim data plugin (elastic#56353) [SIEM] Fixes Signals count spinner (elastic#56797) [docs] Update upgrade version path (elastic#56658) [Canvas] Use unique Id for Canvas Embeddables (elastic#56783) [Rollups] Adjust max width for job detail panel (elastic#56674) Prevent http client from converting our form data (elastic#56772) Disable creating alerts client instances when ESO plugin is using an ephemeral encryption key (elastic#56676) Bumps terser-webpack-plugin to 2.3.4 (elastic#56662) Advanced settings component registry ⇒ kibana platform plugin (elastic#55940) [Endpoint] EMT-67: add kql support for endpoint list (elastic#56328) Implement UI for Create Alert form (elastic#55232) Fix: Filter pill base coloring (elastic#56761) fix open close signal on detail page (elastic#56757) [Search service] Move loadingCount to sync search strategy (elastic#56335) Rollup TSVB integration: Add test and fix warning text (elastic#56639) ...

…ephemeral encryption key (#56676) (#56790) Co-authored-by: Elastic Machine <[email protected]>

mikecote added review Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Feb 3, 2020

mikecote self-assigned this Feb 3, 2020

peterschretlen reviewed Feb 3, 2020

View reviewed changes

mikecote commented Feb 4, 2020

View reviewed changes

Disable creating alerts client instances when ESO plugin is using an …

b34fa68

…ephemeral encryption key

mikecote force-pushed the alerting/disable-alert-APIs-when-eso-gen-key branch from bce4018 to b34fa68 Compare February 4, 2020 18:11

mikecote changed the title ~~Disable alerting APIs when ESO plugin is using an ephemeral encryption key~~ Disable creating alert client instances when ESO plugin is using an ephemeral encryption key Feb 4, 2020

pmuellr approved these changes Feb 4, 2020

View reviewed changes

mikecote mentioned this pull request Feb 4, 2020

Disable alerting or alerting APIs when no encryptionKey is set for the encrypted saved objects plugin #56420

Closed

peterschretlen approved these changes Feb 4, 2020

View reviewed changes

mikecote merged commit e74ab19 into elastic:master Feb 4, 2020

mikecote added a commit to mikecote/kibana that referenced this pull request Feb 4, 2020

Disable creating alerts client instances when ESO plugin is using an …

c6386f7

…ephemeral encryption key (elastic#56676)

This was referenced Feb 4, 2020

[7.x] Disable creating alerts client instances when ESO plugin is using an ephemeral encryption key (#56676) #56790

Merged

[7.6] Disable creating alerts client instances when ESO plugin is using an ephemeral encryption key (#56676) #56791

Merged

mikecote added a commit that referenced this pull request Feb 5, 2020

[7.6] Disable creating alerts client instances when ESO plugin is usi…

09a2c2c

…ng an ephemeral encryption key (#56676) (#56791) * Fix merge conflicts * Fix * Fix test failures

mikecote added the v7.6.0 label Feb 5, 2020

mikecote added a commit that referenced this pull request Feb 5, 2020

Disable creating alerts client instances when ESO plugin is using an …

b5b610e

…ephemeral encryption key (#56676) (#56790) Co-authored-by: Elastic Machine <[email protected]>

mikecote mentioned this pull request Feb 6, 2020

Remove hacks from alert plugin.test.ts once migrated to new platform #57043

Closed

mikecote added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2020

ymao1 mentioned this pull request Jul 13, 2021

[alerting] decrypt errors during migration yield unmigrated alert saved objects #101582

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable creating alert client instances when ESO plugin is using an ephemeral encryption key #56676

Disable creating alert client instances when ESO plugin is using an ephemeral encryption key #56676

mikecote commented Feb 3, 2020 •

edited

Loading

elasticmachine commented Feb 3, 2020

peterschretlen left a comment

mikecote commented Feb 4, 2020

mikecote Feb 4, 2020 •

edited

Loading

pmuellr Feb 4, 2020

gmmorris Feb 4, 2020

mikecote Feb 4, 2020

peterschretlen Feb 4, 2020 •

edited

Loading

pmuellr Feb 4, 2020

mikecote commented Feb 4, 2020

pmuellr left a comment

kibanamachine commented Feb 4, 2020

peterschretlen left a comment

mikecote commented Feb 4, 2020

	async handler(request: ScheduleRequest) {
	const alertsClient = request.getAlertsClient!();
	return await alertsClient.create({ data: request.payload });
	},

Disable creating alert client instances when ESO plugin is using an ephemeral encryption key #56676

Disable creating alert client instances when ESO plugin is using an ephemeral encryption key #56676

Conversation

mikecote commented Feb 3, 2020 • edited Loading

elasticmachine commented Feb 3, 2020

peterschretlen left a comment

Choose a reason for hiding this comment

mikecote commented Feb 4, 2020

mikecote Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

pmuellr Feb 4, 2020

Choose a reason for hiding this comment

gmmorris Feb 4, 2020

Choose a reason for hiding this comment

mikecote Feb 4, 2020

Choose a reason for hiding this comment

peterschretlen Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

pmuellr Feb 4, 2020

Choose a reason for hiding this comment

mikecote commented Feb 4, 2020

pmuellr left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 4, 2020

💚 Build Succeeded

History

peterschretlen left a comment

Choose a reason for hiding this comment

mikecote commented Feb 4, 2020

mikecote commented Feb 3, 2020 •

edited

Loading

mikecote Feb 4, 2020 •

edited

Loading

peterschretlen Feb 4, 2020 •

edited

Loading