Fix enable and disable API to still work when AAD is out of sync #56634

mikecote · 2020-02-03T15:16:42Z

Solves the enable and disable API for #56619.

In this PR, I'm allowing alerts to be enabled and disabled even when the AAD is out of sync. I'm also refactoring the enable and disable unit tests of the alerts client.

The code to invalidate API keys was previously at the wrong place (enable API) when it should of been at the disable API.

elasticmachine · 2020-02-03T15:16:45Z

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

pmuellr

LGTM so far

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

pmuellr · 2020-02-04T13:59:09Z

x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/disable.ts

+                expect(e.status).to.eql(404);
+              }
+              // Ensure AAD isn't broken
+              await checkAAD({


I guess AAD is now "fixed" (it was "broken" just above), because the enable() call "fixed" it. Interesting. That seems fine for alerts, I wonder if it would end up being a security issue with actions, when we get to fixing it's related encryption key issues.

Yeah, this scenario we're lucky. For actions, I believe all the secrets have to be passed in for any sort of update to the SO. We probably won't have this issue of loading the decrypted object beforehand in the actions client, I think.

…le-disable-alerts-broken-aad

elasticmachine · 2020-02-06T21:43:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 88dbe03

History

💔 Build #24033 failed a349e24

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

peterschretlen

I think we still need to call invalidateApiKey() when enabling

peterschretlen · 2020-02-07T15:46:35Z

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

@@ -395,12 +390,32 @@ export class AlertsClient {
      );
      const scheduledTask = await this.scheduleAlert(id, attributes.alertTypeId);
      await this.savedObjectsClient.update('alert', id, { scheduledTaskId: scheduledTask.id });
-      await this.invalidateApiKey({ apiKey: attributes.apiKey });


In some cases we still need the invalidate. As-is, the code assumes that prior to calling enable() no API key exists.
I tested this with detection rules, and this results in dangling API keys because sometimes an alert is updated before calling enable():

Create a custom rule, with the rule disabled. This will call create() and generate an API key k1

Enable the rule

calls update(), this creates a new API key k2 and invalidates the old one k1

calls enable(), which in this case creates a new key k3. k2 doesn't get invalidated.

Even after fixing #57036, if update() is called before enable() you will end up with a dangling key.

Thanks for the review 🙏 I see what you mean, I've also noticed this strange behaviour when fixing #57036 and created #57099 from that. This would solve the problem, but it never hurts to add a check here as well so I will do the change 👍

Fixed in f1367ec.

…m:mikecote/kibana into alerting/enable-disable-alerts-broken-aad

peterschretlen

LGTM - thanks!

mikecote · 2020-02-07T18:47:31Z

@elasticmachine merge upstream

pmuellr

LGTM

mikecote · 2020-02-10T13:18:12Z

@elasticmachine merge upstream

kibanamachine · 2020-02-10T14:39:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0d1004f

History

💔 Build #25289 failed 76bcaab
💚 Build #25058 succeeded 88dbe03
💔 Build #24033 failed a349e24

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

…stic#56634) * Fix enable and disable API to still work when AAD is broken * Load SO once before fallback * Fix comment * Invalidate API key if any in enable API * Add missing integration test Co-authored-by: Elastic Machine <[email protected]>

* master: (34 commits) [Index management] Server-side NP ready (elastic#56829) Webhook action - make user and password secrets optional (elastic#56823) [DOCS] Removes reference to IRC (elastic#57245) [Monitoring] NP migration: Local angular module (elastic#51823) [SIEM] Adds ECS link to help menu (elastic#57104) Ensure http interceptors are shares across lifecycle methods (elastic#57150) [Remote clusters] Migrate server code out of legacy (elastic#56781) fixes render bug in alert list (elastic#57152) siem 7.6 updates (elastic#57169) Make the update alert API key API work when AAD is out of sync (elastic#56640) fix(NA): MaxListenersExceededWarning on getLoggerStream (elastic#57133) [Metrics UI] Setup commonly used time ranges in timepicker (elastic#56701) [Maps] set filter.meta.key to geoFieldName so query passes filterMatchesIndex when ignoreFilterIfFieldNotInIndex is true (elastic#56692) Create plugin mock for event log plugin (elastic#57048) fix ts error on master (elastic#57236) Don't create API key for disabled alerts when calling create API (elastic#57041) Fix enable and disable API to still work when AAD is out of sync (elastic#56634) [DOCS] Canvas embed objects (elastic#57156) Delete autocomplete namespace (elastic#57187) Security - Inject logout url (elastic#57201) ...

) (#57238) * Fix enable and disable API to still work when AAD is broken * Load SO once before fallback * Fix comment * Invalidate API key if any in enable API * Add missing integration test Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

Fix enable and disable API to still work when AAD is broken

a349e24

mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v7.6.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

mikecote mentioned this pull request Feb 3, 2020

Alerting APIs fail when alert fails to be decrypted #56619

Closed

mikecote removed the v7.6.0 label Feb 3, 2020

pmuellr approved these changes Feb 4, 2020

View reviewed changes

mikecote added 2 commits February 6, 2020 14:59

Merge branch 'master' of github.com:elastic/kibana into alerting/enab…

039d8a5

…le-disable-alerts-broken-aad

Load SO once before fallback

4331c37

mikecote mentioned this pull request Feb 6, 2020

API key gets created for disabled alerts (create API) #57036

Closed

mikecote marked this pull request as ready for review February 6, 2020 20:18

mikecote requested a review from a team as a code owner February 6, 2020 20:18

mikecote added the review label Feb 6, 2020

Fix comment

88dbe03

elastic deleted a comment from kibanamachine Feb 6, 2020

peterschretlen self-requested a review February 7, 2020 15:13

peterschretlen suggested changes Feb 7, 2020

View reviewed changes

mikecote added 2 commits February 7, 2020 11:05

Invalidate API key if any in enable API

f1367ec

Merge branch 'alerting/enable-disable-alerts-broken-aad' of github.co…

3de3156

…m:mikecote/kibana into alerting/enable-disable-alerts-broken-aad

peterschretlen approved these changes Feb 7, 2020

View reviewed changes

Add missing integration test

76bcaab

Merge branch 'master' into alerting/enable-disable-alerts-broken-aad

38186ea

pmuellr approved these changes Feb 7, 2020

View reviewed changes

Merge branch 'master' into alerting/enable-disable-alerts-broken-aad

0d1004f

mikecote merged commit 8155415 into elastic:master Feb 10, 2020

mikecote mentioned this pull request Feb 10, 2020

[7.x] Fix enable and disable API to still work when AAD is out of sync (#56634) #57238

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix enable and disable API to still work when AAD is out of sync #56634

Fix enable and disable API to still work when AAD is out of sync #56634

mikecote commented Feb 3, 2020 •

edited

Loading

elasticmachine commented Feb 3, 2020

pmuellr left a comment

pmuellr Feb 4, 2020

mikecote Feb 6, 2020

elasticmachine commented Feb 6, 2020

peterschretlen left a comment •

edited

Loading

peterschretlen Feb 7, 2020 •

edited

Loading

mikecote Feb 7, 2020

mikecote Feb 7, 2020

peterschretlen left a comment

mikecote commented Feb 7, 2020

pmuellr left a comment

mikecote commented Feb 10, 2020

kibanamachine commented Feb 10, 2020

Fix enable and disable API to still work when AAD is out of sync #56634

Fix enable and disable API to still work when AAD is out of sync #56634

Conversation

mikecote commented Feb 3, 2020 • edited Loading

elasticmachine commented Feb 3, 2020

pmuellr left a comment

Choose a reason for hiding this comment

pmuellr Feb 4, 2020

Choose a reason for hiding this comment

mikecote Feb 6, 2020

Choose a reason for hiding this comment

elasticmachine commented Feb 6, 2020

💚 Build Succeeded

History

peterschretlen left a comment • edited Loading

Choose a reason for hiding this comment

peterschretlen Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

mikecote Feb 7, 2020

Choose a reason for hiding this comment

mikecote Feb 7, 2020

Choose a reason for hiding this comment

peterschretlen left a comment

Choose a reason for hiding this comment

mikecote commented Feb 7, 2020

pmuellr left a comment

Choose a reason for hiding this comment

mikecote commented Feb 10, 2020

kibanamachine commented Feb 10, 2020

💚 Build Succeeded

History

mikecote commented Feb 3, 2020 •

edited

Loading

peterschretlen left a comment •

edited

Loading

peterschretlen Feb 7, 2020 •

edited

Loading