[Timeline][RBAC] - Add RBAC logic to timeline alerts search strategy #105333

yctercero · 2021-07-12T21:13:43Z

Summary

Adds RBAC layer to timeline plugin search strategy for alerts. Alerts RBAC is built off of the alerting plugin's authorization class. A few things that I came across:

Users will not have ES privileges - they will have Kibana privileges - so need to query using Kibana user

The existing search strategy implementation does not expose a method for querying using the Kibana user. Upon discussions with @lukasolson figured out a simple way to expose this option to us in a way that does not expose it to existing search strategies. There may be a need to revisit this code, but seemed like a good option to keep things moving. A comment is added in the code to explain why this option was added.

Because of the need to query alerts using Kibana user, can't query other non-alerts indices at the same time (those need to be queried using `asCurrentUser`)

For the time being, this logic will be separated. So you can either query for alerts by specifying the entityType: EntityType.ALERTS or for events by specifying entityType: EntityType.EVENTS.

Timeline search strategy needs access to alerting's auth class and full Kibana request

@lukasolson was kind enough to expose the full Kibana request in the search strategy dependencies in a previous #98566. To get the alerting auth class, added alerting as a dependency. Should be added as optional as security can be disabled, in which case no auth is performed.

Client should be able to specify that they want to get more than one solution's alerts

Added a prop to timeline search strategy - alertsConsumers - an array of alert consumers for which solution alerts to search/return. An enum (non enum) of all alertsConsumers options can be found in a kbn package - packages/kbn-rule-data-utils/src/alerts_as_data_rbac.ts. Based off of this array, we determine which indices to query.

TO DO

Update integration tests to expect 403 instead of 500 for unauthorized users after [Search Strategy] - Errors being swallowed and reported as 500 #106005 is fixed
Update timeline search strategy types to specify that when querying alerts, you do not need to specify an index
Add logic for determining custom alerts index names
Timeline event details search strategy parser needs to be updated to expect new alert fields @angorayc
Move alerting plugin to optional in timelines plugin and add test to ensure auth is not conducted when alerting client is not included (@dhurley14 )

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—users should only have access to alerts in their space.	High	High	Integration tests will verify that: when user with access to SPACE-1 queries for alerts, they only receive alerts from SPACE-1, when user without access to SPACE-1 queries for alerts, they do not receive SPACE-1 alerts, when a user with access to SPACE-1 and SPACE-2 queries alerts in SPACE-1 they receive SPACE-1 alerts and when they query alerts in SPACE-2 they receive alerts in SPACE-2.
Multiple Solutions—users can have access to alerts from multiple solutions.	High	High	Integration tests will verify that when user with access to SOLUTION-1 queries for alerts, they only receive alerts from SOLUTION-1, when a user has access to multiple solutions they receive alerts from multiple solutions.
Code should handle case where security plugin is disabled.	Medium	High	Integration tests will verify that when security plugin is disabled, users can still access alerts (no auth check conducted).
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

x-pack/plugins/rule_registry/server/utils/create_persistence_rule_type_factory.ts

…, and a bit of cleanup

test/scripts/jenkins_security_solution_cypress_rac.sh

x-pack/plugins/alerting/server/authorization/alerting_authorization_kuery.ts

x-pack/plugins/rule_registry/common/mapping_from_field_map.ts

…h strategy all, need to update details search strategy

x-pack/plugins/alerting/server/authorization/alerting_authorization_kuery.test.ts

x-pack/plugins/alerting/server/authorization/alerting_authorization_kuery.ts

…ng internal user. does not affect or expose to existing search strategies

x-pack/plugins/rule_registry/server/utils/create_persistence_rule_type_factory.ts

x-pack/plugins/security_solution/common/constants.ts

x-pack/plugins/security_solution/server/lib/detection_engine/reference_rules/query.ts

dhurley14 · 2021-07-20T21:30:00Z

x-pack/plugins/timelines/kibana.json

@@ -6,6 +6,6 @@
 "extraPublicDirs": ["common"],
 "server": true,
 "ui": true,
- "requiredPlugins": ["data", "dataEnhanced", "kibanaReact", "kibanaUtils"],
+ "requiredPlugins": ["alerting", "data", "dataEnhanced", "kibanaReact", "kibanaUtils"],


Would timeline still work if alerting was optional instead of required?

That's a good question. So it's kind of interesting. If security is enabled, then yes, alerting is required because we have to do auth. But if security is disabled, then no, it's not required.

Guess I should move this to optional and add a test.

TBD in follow up pr.

dhurley14 · 2021-07-20T21:44:45Z

x-pack/plugins/timelines/server/search_strategy/timeline/index.ts

+
+ const queryFactory: TimelineFactory<T> = timelineFactory[factoryQueryType];
+
+ if (entityType != null && entityType === EntityType.ALERTS) {


Is there a way to force entityType to be required on the request so we don't have to do these null checks?

Yes! So my plan was to have a separate PR to update the front end code and that's when I would flip this to required. This allows me to not have to do it all in one PR>

or you can do something simple like that [EntityType.ALERTS].includes(entityType)

x-pack/plugins/timelines/server/search_strategy/timeline/index.ts

dhurley14

This is still in draft so mostly just pointed out a few questions I had, a few nits. Otherwise this looks great!

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

XavierM

Good to go!!! and good job on it much appreciated!

…trategy

yctercero · 2021-07-26T22:29:44Z

jenkins test this

…trategy

ymao1 · 2021-07-27T17:21:19Z

@yctercero #106519 just got merged, looks like there are merge conflicts

x-pack/plugins/timelines/server/search_strategy/timeline/index.ts

…trategy

lukasolson

Code LGTM

kibanamachine · 2021-07-28T05:36:48Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`infra`	939	940	+1

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
`securitySolution`	1245	1247	+2
`timelines`	763	768	+5
total			+7

Async chunks

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

id	before	after	diff
`infra`	1.7MB	1.7MB	+6.0B

Page load bundle

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

id	before	after	diff
`infra`	145.3KB	146.2KB	+880.0B
`securitySolution`	208.2KB	208.2KB	-2.0B
`timelines`	320.0KB	320.3KB	+243.0B
total			+1.1KB

Unknown metric groups

API count

id	before	after	diff
`data`	3716	3717	+1
`securitySolution`	1296	1298	+2
`timelines`	882	887	+5
total			+8

References to deprecated APIs

id	before	after	diff
`alerting`	32	23	-9

History

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

cc @yctercero

kibanamachine · 2021-07-28T05:39:38Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 105333

…lastic#105333) ## Summary Adds RBAC layer to timeline plugin search strategy for alerts. # Conflicts: # x-pack/plugins/alerting/server/alerting_authorization_client_factory.ts

…105333) (#107005) ## Summary Adds RBAC layer to timeline plugin search strategy for alerts. # Conflicts: # x-pack/plugins/alerting/server/alerting_authorization_client_factory.ts Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…lastic#105333) ## Summary Adds RBAC layer to timeline plugin search strategy for alerts.

yctercero force-pushed the alerts_as_data_tgrid branch from 14582e6 to fb865ab Compare July 14, 2021 01:12

yctercero commented Jul 14, 2021

View reviewed changes

x-pack/plugins/rule_registry/server/utils/create_persistence_rule_type_factory.ts Outdated Show resolved Hide resolved

yctercero added 3 commits July 19, 2021 20:29

changes made to search strategy to allow for internal user

00cf943

adds spaceId filter as option to alerting auth find filter constructor

a2cd92c

updates security solution custom rule script, integration test alerts…

cd764da

…, and a bit of cleanup