Invalidate alert API Key when generating a new one by mikecote · Pull Request #53732 · elastic/kibana

mikecote · 2019-12-20T21:31:11Z

Resolves #45144

In this PR, I'm invalidating API keys after generating a new one. This happens within the update, enable, updateApiKey calls as well as after deleting an alert.

I'm also renaming a variable within CreateAPIKeyResult interface to be more clear (created -> apiKeysEnabled). This is to be consistent with the new interface InvalidateAPIKeyResult.

There are some notes worth considering (created an issue to track them #53868):

If the underlying alert task is running while the API key is invalidated, the task may throw an error if it needed that key (authentication failure, invalid key, etc)
The same applies to actions if they are running while the API key is invalidated
If actions are scheduled but not executed before the API key is invalidated, the action will throw an error if it depended on that key
If an action is in a retry state and depended on that key, it will keep failing
The invalidated API keys still return from the Elasticsearch API Key API. They are returned with invalidated: true as an attribute

elasticmachine · 2019-12-20T21:31:14Z

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

mikecote · 2019-12-31T12:59:25Z

@elasticmachine merge upstream

kibanamachine · 2019-12-31T14:13:24Z

💔 Build Failed

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

History

💔 Build #17032 failed f271908

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

kibanamachine · 2019-12-31T16:06:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 7d140e3

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

…t-api-key-cleanup

mikecote · 2020-01-02T13:34:30Z

@elasticmachine merge upstream

pmuellr

LGTM; added a comment/question about whether we need the new namespace param, whether we can just calculate as needed or at initialization based on spaceId

pmuellr · 2020-01-02T15:34:53Z

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

  alertTypeRegistry: AlertTypeRegistry;
+  encryptedSavedObjectsPlugin: EncryptedSavedObjectsStartContract;
  spaceId?: string;
+  namespace?: string;


What's the difference between spaceId and namespace? Looks like spaceId is only ever used in scheduleAlert(). I see some a conversion function used elsewhere in files in the PR - spaceIdToNamespace(), wondering if we should just use spaceIdToNamespace(this.spaceId) instead of adding the namespace parameter?

I'm not sure myself of the difference but I have been told by the security team that there is a difference. Also that we should use spaceId as our source of truth to convert into namespace and basePath.

We can use the function spaceIdToNamespace to get the value but I would have to pass the spaceIdToNamespace function as an argument to alerts client instead. So I figured I might as well pass the result of the function. I'm indifferent on the approach, thoughts?

eh, I worry about two highly related values being stored like that, in case one gets "out of sync" with the other, but I don't think that's really an issue since you can't really change the space of a SO anyway. And having to send more functions in doesn't sound all that great either. Seems like leaving it the way it is, is fine.

I'm hoping eventually we can just pass around the setup and start objects instead of sub-functions / properties. Though, not sure if that's good or bad but would reduce the number of things being passed around drastically.

gmmorris · 2020-01-03T14:11:17Z

x-pack/legacy/plugins/alerting/server/lib/alerts_client_factory.ts

        }
        return {
-          created: true,
+          apiKeysEnabled: true,


👍 for the rename

gmmorris · 2020-01-03T14:22:57Z

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

+    const apiKeyId = Buffer.from(apiKey, 'base64')
+      .toString()
+      .split(':')[0];
+    const response = await this.invalidateAPIKey({ id: apiKeyId });


If this throws (instead of returning an error_count > 0) then we wo't get the error message that says we failed to invalidate the key. Is it worth catching and logging accordingly?

The log message will still be there from the security plugin: https://github.com/elastic/kibana/blob/master/x-pack/plugins/security/server/authentication/api_keys.ts#L161.

Though I went ahead and also wrapped it ourselves to swallow the error in that case: 4a4aad9.

gmmorris

LGTM.
Left a couple of notes about testing for the unhappy path of invalidating keys, but other than that, looks good.

gmmorris · 2020-01-03T14:38:31Z

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

+  alertsClientParams.invalidateAPIKey.mockResolvedValue({
+    apiKeysEnabled: true,
+    result: { error_count: 0 },
+  });


Feels like we're testing the happy path here, it might be worth adding a couple of tests that checks what happens when these api calls fail (throw an error, etc)

…t-api-key-cleanup

kibanamachine · 2020-01-03T18:39:28Z

💚 Build Succeeded

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

History

💚 Build #17809 succeeded 10becbb
💚 Build #17561 succeeded 4b457b5
💚 Build #17504 succeeded 6473ee3

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

* Initial work to auto cleanup old API keys * Fix ESLint error * Rename confusing variables * Add test to ensure thrown errors are swallowed * Add more tests Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Initial work to auto cleanup old API keys * Fix ESLint error * Rename confusing variables * Add test to ensure thrown errors are swallowed * Add more tests Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798)

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793)

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793) [SR] Enable component integration tests (elastic#53893)

…nsole-dependencies * 'master' of github.com:elastic/kibana: (33 commits) adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768) increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793) [SR] Enable component integration tests (elastic#53893) Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777) Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) ... # Conflicts: # yarn.lock

Initial work to auto cleanup old API keys

f271908

mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// labels Dec 20, 2019

mikecote self-assigned this Dec 20, 2019

Merge branch 'master' into alerting/alert-api-key-cleanup

b71a787

elastic deleted a comment from elasticmachine Dec 31, 2019

mikecote added 2 commits December 31, 2019 09:37

Fix ESLint error

f06c1e5

Rename confusing variables

7d140e3

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

6473ee3

…t-api-key-cleanup

Merge branch 'master' into alerting/alert-api-key-cleanup

4b457b5

mikecote marked this pull request as ready for review January 2, 2020 13:34

mikecote requested a review from a team as a code owner January 2, 2020 13:34

mikecote requested a review from a team January 2, 2020 13:34

pmuellr approved these changes Jan 2, 2020

View reviewed changes

kobelb approved these changes Jan 2, 2020

View reviewed changes

gmmorris self-requested a review January 3, 2020 14:08

gmmorris reviewed Jan 3, 2020

View reviewed changes

gmmorris approved these changes Jan 3, 2020

View reviewed changes

mikecote added 3 commits January 3, 2020 10:10

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

2cf5671

…t-api-key-cleanup

Add test to ensure thrown errors are swallowed

4a4aad9

Add more tests

10becbb

Merge with master

0037957

mikecote merged commit 8cc778a into elastic:master Jan 3, 2020

mikecote mentioned this pull request Jan 3, 2020

[7.x] Invalidate alert API Key when generating a new one (#53732) #53963

Merged

FrankHassanabad mentioned this pull request Jan 6, 2020

Initial prototype of adding more details to the alerts and API Keys #53705

Closed

7 tasks

mikecote mentioned this pull request Mar 18, 2020

Fix race condition in flaky alerting test #60438

Merged

Conversation

mikecote commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 20, 2019

Uh oh!

mikecote commented Dec 31, 2019

Uh oh!

kibanamachine commented Dec 31, 2019

💔 Build Failed

History

Uh oh!

kibanamachine commented Dec 31, 2019

💔 Build Failed

Uh oh!

mikecote commented Jan 2, 2020

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 3, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mikecote commented Dec 20, 2019 •

edited

Loading