Add security support for alerts and actions by mikecote · Pull Request #41389 · elastic/kibana

mikecote · 2019-07-17T17:25:31Z

This PR makes alerting and actions plugin work when security is enabled. It takes advantage of Elasticsearch API keys to run requests on behalf of a user at future times.

The PR also contains the following changes:

Api integration tests have been refactored to follow the x-pack/test/ui_capabilities style with different permutations. Right now it tests only for spaces & security enabled, future PRs will add spaces only and security only.
Executing an action will create a saved object action_task_params before scheduling a task. This saved object encrypts the API key so it can be passed from an alert to an action. (to support executing actions on behalf of the creator of the alert).
getServices now takes a request instead of custom parameters. This facilitates passing a real request and a fake request using API keys.
System ensures user can read the alert / action before running the task (code is within execute.js for actions and within getCreateTaskRunnerFunction for alerting).
Uses registerFeature within init to allow tests to try different permutations for a user using the savedObjectsClient within an executor (create alert but can't read dashboard. This allows us to confirm the savedObjectsClient is scoped to the user).
Converts alerting to use encrypted saved objects (to store API keys).
Capture a createdBy on the alert in future scenario we want to know who's API keys are with the saved object.
Fix a bug with task manager removing a task before disabling an alert. Unauthorized users were able to remove tasks by calling disable API.

Fixes: #40021
Fixes: #42959

elasticmachine · 2019-07-17T17:25:33Z

Pinging @elastic/kibana-stack-services

pmuellr

From what I could understand, LGTM

x-pack/legacy/plugins/actions/server/create_fire_function.test.ts

x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts

…rity

pmuellr · 2019-08-19T15:49:32Z

from #41389 (comment)

@kobelb
Are we at risk of leaking the secret values here if they happen to fail the validation?

@mikecote
The validators use @kbn/config-schema and from what I understand it doesn't leak data when validating. @pmuellr can you confirm that is the case?

We're fine for now, but I'm going to open an issue for config-schema because there is a case that leaks data, but shouldn't affect us now.

Generally config-schema only reports the type of the data (eg, "expecting a string but you passed a number"), but there's one case I've seen that does leak, and that's with schema.maybe(). It actually prints the object. In practice, we only use schema.maybe with strings, and so if you use a number where a string field is expected, it will print the number (eg, "expecting a string but you passed 42").

But I'd like to have a safer contract (never print values, always print types) with schema.config, so will open an issue ... actually I'm going to create a backlog card because I'll have to create a test case first ...

bmcconaghy

Code LGTM.

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

x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts

pmuellr

Made a bunch of comments; only critical one is the misspelled defualt string.

x-pack/legacy/plugins/actions/server/create_execute_function.test.ts

x-pack/legacy/plugins/actions/server/create_execute_function.ts

x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts

x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts

x-pack/legacy/plugins/alerting/mappings.json

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

x-pack/test/alerting_api_integration/common/lib/space_test_utils.ts

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

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

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

…rity

elasticmachine · 2019-08-20T14:54:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

bmcconaghy · 2019-08-20T15:09:22Z

Newer changes LGTM

* Initial work * Cleanup add createAPIKey fn pt1 * Change getServices function to take request as parameter * Use API key when executing alerts * Revert task manager using encrypted saved objects * Store fired actions within a saved object to encrypt API keys * Fix fireActionId * Cleanup code, fix type check error * Add a type for getScopedSavedObjectsClient * Fix getBasePath and spaceIdToNamespace functions * Add safety check for API key and action * Fix integration tests * Fix broken jest tests * Cleanup * Rename generatedApiKey to apiKeyValue * Ensure access to action record * Cleanup * Add unit tests * Fix variable conflict * Revert task manager specific code (no longer needed) * Remove fire terminology * Move tests to spaces and security folder * Use ES Archiver to remove spaces (empty_kibana) * Fix missing pieces * Convert action tests to run per user * Convert alerting tests to run per user * Fix type check issue * Fix failing test * Add callCluster and savedObjectsClient authorization tests * Make savedObjectsClient return 403 for authorization tests * Cleanup * Fix test failure * Common function to get data from test index * Create ObjectRemover * Cleanup * useApiKey now provided to functions instead of relying on condition of two strings * Fix typo * Make tests it(...) start with should * Rename useApiKey to isSecurityEnabled * Merge apiKeyId and apiKeyValue into one * Update docs * Use feature controls for list alert / action types API * Remove need to add ! in TypeScript for required plugins * Fix ESLint issue * Include actions and alertTypeParams into AAD and genereate new API key on update * Generate random id for API key name attribute * Include interval in AAD * Send pre-encoded string * Fix ExecutorError * Fix apiKey snapshot * Fix 'default' typo * De-compose apiKey * Refresh API key when enabling / disabling an alert * Add updatedBy * Make unauthorized APIs return 404

mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Stack Services v7.4.0 labels Jul 17, 2019

mikecote self-assigned this Jul 17, 2019

This comment has been minimized.

Sign in to view

pmuellr approved these changes Jul 18, 2019

View reviewed changes

x-pack/legacy/plugins/actions/server/create_fire_function.test.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts Outdated Show resolved Hide resolved

mikecote mentioned this pull request Jul 26, 2019

Make alerting properly space aware #42081

Merged

mikecote added 2 commits August 7, 2019 14:15

Initial work

8fc2509

Cleanup add createAPIKey fn pt1

07b923f

mikecote force-pushed the alerting/security branch from 56cc5c5 to 07b923f Compare August 8, 2019 11:02

mikecote added 14 commits August 8, 2019 08:13

Change getServices function to take request as parameter

5954064

Use API key when executing alerts

1e821f2

Revert task manager using encrypted saved objects

37d1be5

Store fired actions within a saved object to encrypt API keys

28b2ae6

Merge with master

c8d742e

Fix fireActionId

6e9a6fe

Cleanup code, fix type check error

5700426

Add a type for getScopedSavedObjectsClient

c0205a2

Fix getBasePath and spaceIdToNamespace functions

b756589

Add safety check for API key and action

3c296b5

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

06a4c40

…rity

Fix integration tests

59f0807

Fix broken jest tests

c55cea5

Cleanup

72e0a87

This comment has been minimized.

Sign in to view

mikecote added 2 commits August 12, 2019 13:03

Rename generatedApiKey to apiKeyValue

b7f43d4

Ensure access to action record

32c5efc

Generate random id for API key name attribute

bdef9ed

This comment has been minimized.

Sign in to view

bmcconaghy approved these changes Aug 19, 2019

View reviewed changes

kobelb reviewed Aug 19, 2019

View reviewed changes

x-pack/legacy/plugins/alerting/server/init.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts Show resolved Hide resolved

pmuellr approved these changes Aug 19, 2019

View reviewed changes

mikecote added 7 commits August 20, 2019 06:17

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

888337a

…rity

Include interval in AAD

b2f67c7

Send pre-encoded string

66fac87

Fix ExecutorError

21e5f1f

Fix apiKey snapshot

422174c

Fix 'default' typo

e505a65

De-compose apiKey

243890e

This comment has been minimized.

Sign in to view

mikecote added 2 commits August 20, 2019 08:54

Refresh API key when enabling / disabling an alert

f99ab74

Add updatedBy

c6d5589

kobelb approved these changes Aug 20, 2019

View reviewed changes

Make unauthorized APIs return 404

f5eb263

mikecote merged commit 5734f1c into elastic:master Aug 20, 2019

mikecote mentioned this pull request Aug 20, 2019

[7.x] Add security support for alerts and actions (#41389) #43592

Merged

mikecote mentioned this pull request Aug 27, 2019

Disable alerting and actions plugin by default #44083

Merged

peterschretlen mentioned this pull request Sep 19, 2019

Stack Monitoring + Kibana Alerting #45571

Open

3 tasks

peterschretlen mentioned this pull request Oct 30, 2019

Allow creation of API keys on behalf of other users elastic/elasticsearch#48716

Closed

mikecote mentioned this pull request Oct 31, 2019

Add createdBy and updatedBy fields to action saved objects #49819

Closed

mikecote mentioned this pull request Nov 12, 2019

Open issue on config-schema to prevent data leakage #50252

Closed

mikecote mentioned this pull request Dec 12, 2019

Cleanup alerting / actions feature controls #52286

Merged

Conversation

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

Uh oh!

Uh oh!

elasticmachine commented Jul 17, 2019

Uh oh!

This comment has been minimized.

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

pmuellr commented Aug 19, 2019

Uh oh!

This comment has been minimized.

bmcconaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

elasticmachine commented Aug 20, 2019

💚 Build Succeeded

Uh oh!

bmcconaghy commented Aug 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mikecote commented Jul 17, 2019 •

edited

Loading