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

[RAC] [RBAC] MVP RBAC for alerts as data #100705

Merged
merged 33 commits into from
Jul 8, 2021

Conversation

dhurley14
Copy link
Contributor

@dhurley14 dhurley14 commented May 26, 2021

++ in collaboration with @yctercero && @angorayc

An MVP of the RBAC work required for the "alerts as data" effort. An example of the existing implementation for alerts would be that of the security solution. The security solution stores its alerts generated from rules in a single data index - .siem-signals. In order to gain or restrict access to alerts, users do so by following the Elasticsearch privilege architecture. A user would need to go into the Kibana role access UI and give explicit read/write/manage permissions for the index itself.

Kibana as a whole is moving away from this model and instead having all user interactions run through the Kibana privilege model. When solutions use saved objects, this authentication layer is abstracted away for them. Because we have chosen to use data indices for alerts, we cannot rely on this abstracted out layer that saved objects provide - we need to provide our own RBAC! Instead of giving users explicit permission to an alerts index, users are instead given access to features. They don't need to know anything about indices, that work we do under the covers now.

An example of what a user will see in the UI:

Screen Shot 2021-06-21 at 10 45 01 AM

The alerting plugin, from which rules and alerts are generated (kinda - there's also a rule registry plugin, different convo) already had an RBAC infrastructure based on a rule producer, consumer, and rule type id. In an effort to keep things consistent (and given that alerts flow from corresponding rules), we are reusing the alerting plugin authorization client. The authorization client provides methods that interact with the security plugin API to provide the necessary authentication checks.

The MVP deals with the following use cases.

As a user, I would like to:
  • search my alerts
  • get an alert
  • update an alert status
As an admin, I would like to:
  • allow some users to read rules and alerts
  • allow some users to read/write rules and read/update alerts
  • allow some users to read rules and read/update alerts

The MVP also includes the work necessary to create alerts with the proper fields, however, alerts are created using the internal Kibana user.

Resources

Open discussions

Alerts as data client

Alerts as data can be interacted with using the AlertsClient api found in x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

This api includes public methods such as

  • getFullAssetName
  • getAlertsIndex: provides the alerts as data indices the user is authorized to read
  • get: get a single alert
  • update: update a single alert
  • bulkUpdate (TODO): update multiple alerts via query
  • find (TODO): retrieve multiple alerts

Migrate away from _source and utilize fields apis.

Checklist

@yctercero yctercero added Feature:RAC label obsolete Theme: rac label obsolete v7.14.0 v8.0.0 labels May 27, 2021
@dhurley14 dhurley14 force-pushed the squashed_alerts_rbac_mvp_backup branch 3 times, most recently from f4c0190 to 498e4c0 Compare June 2, 2021 22:13
@dhurley14 dhurley14 added Feature:Detection Alerts Security Solution Detection Alerts Feature release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team labels Jun 2, 2021
@dhurley14 dhurley14 marked this pull request as ready for review June 2, 2021 22:57
@dhurley14 dhurley14 requested review from a team as code owners June 2, 2021 22:57
@dhurley14 dhurley14 requested a review from a team June 2, 2021 22:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@dhurley14 dhurley14 changed the title Squashed alerts rbac mvp backup [RAC] [RBAC] MVP RBAC for alerts as data Jun 2, 2021
@legrego legrego self-requested a review June 2, 2021 23:11
Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stack Monitoring changes look good 👍

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 2, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@dhurley14 dhurley14 force-pushed the squashed_alerts_rbac_mvp_backup branch from dec3d22 to e3c3821 Compare June 3, 2021 15:48
yctercero and others added 21 commits July 8, 2021 10:38
* force any plugin registering rules to register the feature id into the master list which maps feature id to index names

* javascript is not python

* favor keyof instead of typeof as const, removes ts-expect-error

* updates typedocs

* adds rbac utils to utils folder and updates typedocs

* featureIds is already string[] and do not trust input for type guard
… integration coverage (#21)

* updated method to getAugmentedRuleTypesWithAuthorization and updated to use APM_SERVER_FEATURE_ID const instead of string

* abstract inferred return type in rule registry start plugin contract

* updated tests and resolved PR comments

* working on debugging integration tests, certain roles returning 403

* got it to 12 failing from 30+ - yay?

* missing rac api privileges in security solution feature registration

* restrict index level privileges in scripts roles and adds test script for security solution alerts

* forgot to add rac privilege to read section of security solution feature registration

* mismatch of test case and user/roles for two tests

* audit before executing update call

* security is an optional dependency, should be typed out as such

* remove unused / unrealistic roles + users from test cases, fix type check failures

* cleanup trial license roles + users

Co-authored-by: Devin Hurley <[email protected]>
…n alert that does not exist || an index name that does not exist
…ript runner where we added ... instead of ..
…re duplicate tests and puts them at the higher basic/ dir.
* adds spaces only tests

* remove unused imports

* remove auth section of request since security is disabled for these tests

* rename basic to trial for spaces only tests
…nges were merged into master regarding naming conventions for alerts as data index
@dhurley14 dhurley14 force-pushed the squashed_alerts_rbac_mvp_backup branch from c22e179 to 220a8e9 Compare July 8, 2021 16:38
Copy link
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security solution changes LGTM! I just skimmed over the rest of the PR. This is a huge step forward for RAC, thanks!


#### Defined in

[rule_registry/server/alert_data_client/alerts_client.ts:59](https://github.com/dhurley14/kibana/blob/d2173f5090e/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts#L59)
Copy link
Contributor

@marshallmain marshallmain Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These links all lead to a personal repo, should update to lead to elastic repo eventually

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we pulled this from cases and it looks like they have the same problem too.. I'll poke around at this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we'll need to regenerate the docs once it's in master.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in here: #104975

Copy link
Member

@cnasikas cnasikas Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhurley14 @yctercero Thank you for pointing it out! We will regenerate the docs also.

* in a new plugin will require updating the below data structure
* to include the index name where the alerts as data will be written to.
*/
export const mapConsumerToIndexName = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we could make a mechanism to register these feature ID - index name pairs in plugin setup instead of hardcoding them. Ideally we would integrate that registration with the index template creation and RuleDataClient initialization so when a plugin registers alerts-as-data it automatically sets up RBAC and provides the necessary client(s). The overall goal would be to keep the entire definition for a particular solution's alerts-as-data index inside the plugin that owns it. Right now the index names are specified in multiple places that have to be kept in sync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Def agree. This is an ongoing conversation. As the ruleRegistry right now is a library and meant to be stateless, we didn't have a "home" to store this data we needed in. We're opening up a ticket to discuss where best this all might live.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 208 228 +20
apm 37 38 +1
ruleRegistry 44 49 +5
total +26

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 4.3MB 4.3MB +333.0B
observability 483.4KB 483.8KB +437.0B
total +770.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
alerting 14 16 +2
ruleRegistry 7 8 +1
total +3

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
apm 39.8KB 40.1KB +300.0B
Unknown metric groups

API count

id before after diff
alerting 216 236 +20
apm 37 38 +1
ruleRegistry 44 49 +5
total +26

History

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

cc @dhurley14 @angorayc @yctercero

@dhurley14 dhurley14 merged commit c77c7fb into elastic:master Jul 8, 2021
dhurley14 added a commit that referenced this pull request Jul 9, 2021
* [RAC] [RBAC] MVP RBAC for alerts as data (#100705)

An MVP of the RBAC work required for the "alerts as data" effort. An example of the existing implementation for alerts would be that of the security solution. The security solution stores its alerts generated from rules in a single data index - .siem-signals. In order to gain or restrict access to alerts, users do so by following the Elasticsearch privilege architecture. A user would need to go into the Kibana role access UI and give explicit read/write/manage permissions for the index itself.

Kibana as a whole is moving away from this model and instead having all user interactions run through the Kibana privilege model. When solutions use saved objects, this authentication layer is abstracted away for them. Because we have chosen to use data indices for alerts, we cannot rely on this abstracted out layer that saved objects provide - we need to provide our own RBAC! Instead of giving users explicit permission to an alerts index, users are instead given access to features. They don't need to know anything about indices, that work we do under the covers now.

Co-authored-by: Yara Tercero <[email protected]>
Co-authored-by: Yara Tercero <[email protected]>

* fixes failed integration test

Co-authored-by: Yara Tercero <[email protected]>
Co-authored-by: Yara Tercero <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
@peluja1012 peluja1012 mentioned this pull request Jul 20, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Alerts Security Solution Detection Alerts Feature Feature:RAC label obsolete release_note:enhancement Team:APM All issues that need APM UI Team support Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team Theme: rac label obsolete v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.