[Security Solution] [Detections] Add "read index" privilege check on rule execution #83134

dhurley14 · 2020-11-10T23:27:00Z

Summary

Adds a privilege check on each rule execution to ensure the user who's api key is associated with the rule has read privileges on the index.

Indices for testing

POST myfakeindex-1/_doc
{
  "message": "hello world fake",
  "@timestamp": "2020-12-14T22:31:01.726Z"
}


POST somefakeindex-1/_doc
{
  "message": "hello world some",
  "@timestamp": "2020-12-14T22:31:01.726Z"
}

Login to detections as a superuser and set up detections.
Write a rule to query *:* on index patterns myfa* and some*. Ensure the rule starts to run successfully. This rule will be using the api key of the superuser you logged in with and will have read privileges to the above index patterns.
Update the role for the t1_analyst user here x-pack/plugins/security_solution/server/lib/detection_engine/scripts/roles_users/t1_analyst/detections_role.json to have read privileges for the index pattern "myfa*". Save and execute the following within the detection_engine/scripts/roles_users/t1_analyst directory:

$ ./post_detections_role.sh && ./post_detections_user.sh

Login to detections as the t1_analyst and re-enable the rule (the rule is now running with the t1_analyst privileges). Once the rule completes running it should have the following partial error message on the rule details page.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…se lines into a util function to be used in create rules and use that check on the UI too

yctercero

LGTM - super minor comments that can all be addressed as follow up if need be. Am curious to chat about how we define "success" in rule runs.

...plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts

yctercero · 2020-12-22T16:04:18Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts

      await ruleStatusService.goingToRun();

+      // check if rule has permissions to access given index pattern


Is this a TO DO? Would definitely be great to move out for testing too.

I had written a function in the old multiple timestamps PR (which was closed) that would have been the home for this privilege check as well. Maybe in 7.12 I can bring that function back. I think once we add more types of pre-execution checks we can merge them into a single function to be executed at the top of the rule executor like this privilege check is now.

yctercero · 2020-12-22T16:12:22Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts

@@ -600,7 +625,7 @@ export const signalRulesAlertType = ({
              `[+] Finished indexing ${result.createdSignalsCount} signals into ${outputIndex}`
            )
          );
-          if (!hasError) {
+          if (!hasError && !wroteStatus) {


We'll only be reporting success if it checked all indices? Wondering if we should start better documenting/defining what "successful" rule run means. Like in this case if we successfully searched the indices we could, I might expect a success with a warning?

So, my thinking was if the rule can't search every index pattern provided because it's missing read privileges we would want to say "this partially failed" because we couldn't do everything the rule was written to accomplish https://github.com/elastic/kibana/pull/83134/files/b79dd8abf743a5451c3346f7dd004735b7beb732#diff-bd205efcb13acaf5f9552bd573f21422d37d60c6f2aa03ae7f98d18b95fbc6e4R194. We could re-work the partial failure status to just be a "warning" status maybe? Maybe that would clear things up. What do you think?

Yea, that makes sense too, if we weren't able to fulfill what the rule is asking. No need to change anything, maybe just an interesting convo to have at a team meeting about how to qualify different states.

…to ANY of the index patterns provided

kibanamachine · 2020-12-22T21:51:07Z

💚 Build Succeeded

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

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	47143	47903	+760

History

💚 Build #95873 succeeded b79dd8a
💚 Build #89134 succeeded efa3efc2abaf69e447efa6265dff6e78fc046ade
💔 Build #87250 failed e3bd9d617c541bd12e7227785c1bd717ea130788

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

…rule execution (elastic#83134) * adds privilege check in rule execution function, need to abstract these lines into a util function to be used in create rules and use that check on the UI too * fixes tests * cleanup code, adds a unit test * set rule to failure status if the rule does not have read privileges to ANY of the index patterns provided

…ck on rule execution (#83134) (#86849) * adds privilege check in rule execution function, need to abstract these lines into a util function to be used in create rules and use that check on the UI too * fixes tests * cleanup code, adds a unit test * set rule to failure status if the rule does not have read privileges to ANY of the index patterns provided

…eck on rule execution (#83134) (#86850) * adds privilege check in rule execution function, need to abstract these lines into a util function to be used in create rules and use that check on the UI too * fixes tests * cleanup code, adds a unit test * set rule to failure status if the rule does not have read privileges to ANY of the index patterns provided

dhurley14 changed the title ~~adds privilege check in rule execution function, need to abstract the…~~ [Security Solution] [Detections] Add "read index" privilege check on rule execution Nov 10, 2020

dhurley14 force-pushed the privilege-check-execution branch from e3bd9d6 to efa3efc Compare November 19, 2020 23:16

dhurley14 mentioned this pull request Nov 25, 2020

[Security Solution] [Detections] Create a 'partial failure' status for rules #84293

Merged

3 tasks

dhurley14 added 3 commits December 21, 2020 16:08

adds privilege check in rule execution function, need to abstract the…

1eb95af

…se lines into a util function to be used in create rules and use that check on the UI too

fixes tests

6539b60

cleanup code, adds a unit test

b79dd8a

dhurley14 force-pushed the privilege-check-execution branch from efa3efc to b79dd8a Compare December 21, 2020 22:23

dhurley14 self-assigned this Dec 21, 2020

dhurley14 linked an issue Dec 21, 2020 that may be closed by this pull request

[Security Solution] [Detections] Fail rule if rule doesn't have read privileges to every index pattern in rule definition #78491

Closed

dhurley14 marked this pull request as ready for review December 21, 2020 22:28

dhurley14 requested review from a team as code owners December 21, 2020 22:28

dhurley14 added release_note:enhancement review Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team v7.11.0 v7.12.0 v8.0.0 labels Dec 21, 2020

yctercero approved these changes Dec 22, 2020

View reviewed changes

madirey approved these changes Dec 22, 2020

View reviewed changes

set rule to failure status if the rule does not have read privileges …

b77f56a

…to ANY of the index patterns provided

dhurley14 merged commit 59298c5 into elastic:master Dec 22, 2020

dhurley14 deleted the privilege-check-execution branch December 22, 2020 22:01

This was referenced Dec 22, 2020

[7.x] [Security Solution] [Detections] Add "read index" privilege check on rule execution (#83134) #86849

Merged

[7.11] [Security Solution] [Detections] Add "read index" privilege check on rule execution (#83134) #86850

Merged

dhurley14 mentioned this pull request Jan 19, 2021

[Security Solution] [Detections] Update wording for read privilege check #88763

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] [Detections] Add "read index" privilege check on rule execution #83134

[Security Solution] [Detections] Add "read index" privilege check on rule execution #83134

dhurley14 commented Nov 10, 2020 •

edited

Loading

yctercero left a comment

yctercero Dec 22, 2020

dhurley14 Dec 22, 2020

yctercero Dec 22, 2020

dhurley14 Dec 22, 2020

yctercero Dec 22, 2020

kibanamachine commented Dec 22, 2020

		await ruleStatusService.goingToRun();

		// check if rule has permissions to access given index pattern

[Security Solution] [Detections] Add "read index" privilege check on rule execution #83134

[Security Solution] [Detections] Add "read index" privilege check on rule execution #83134

Conversation

dhurley14 commented Nov 10, 2020 • edited Loading

Summary

Checklist

For maintainers

yctercero left a comment

Choose a reason for hiding this comment

yctercero Dec 22, 2020

Choose a reason for hiding this comment

dhurley14 Dec 22, 2020

Choose a reason for hiding this comment

yctercero Dec 22, 2020

Choose a reason for hiding this comment

dhurley14 Dec 22, 2020

Choose a reason for hiding this comment

yctercero Dec 22, 2020

Choose a reason for hiding this comment

kibanamachine commented Dec 22, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

dhurley14 commented Nov 10, 2020 •

edited

Loading