[Security Solution][Detections] Updates Rule Details to show all historical alerts for a given Rule #120053

spong · 2021-12-01T02:26:58Z

Summary

Resolves Part II of #118935 which fixes a bug where pre-8.x alerts are not displayed on the Rule Details page if the Rule SO ID had been re-generated as part of the upgrade to 8.x. As discussed with the team and relevant product folks, we're resolving this issue by updating the alerts query to filter by ruleId instead of the Rule SO ID, which will result in a slight workflow change where all alerts for a given ruleId will be displayed rather than just the latest instance -- i.e. create pre-package rule, generate alerts, delete/re-create pre-package rule, generate alerts, alerts are displayed from both Rule instances instead of just the most recent. This also updates the exceptions workflow such that if the user specifies close all associated alerts when creating a Rule Exception, it will now close all alerts matching the exception for all historical instances of the rule, maintaining consistent behavior when managing alerts from a rule.

The Previous Rule Deleted functionality stays mostly the same -- if you navigate to Rule Details from an older alert whose rule has been deleted, the Rule Details page will still re-construct the rule's details from the alert, and now show all alerts (as above) instead of just that instances' alerts.

Also cleaned up a lot of duplicate/redundant code over in default_config.tsx that was leftover from the RuleRegsitry feature flag. Now condensed back to one function each for generating filters/columns/config.

Test Instructions:

Clear es-data, and start 7.16 ES

rm -rf $DEV_HOME/es-data
es snapshot --license trial -E xpack.security.authc.api_key.enabled=true -E path.data=$DEV_HOME/es-data --version=7.16.0

Navigate to kibana-7.16, checkout and yarn start
Login, create a rule, then inject some data

node x-pack/plugins/security_solution/scripts/endpoint/resolver_generator.js --node http://elastic:[email protected]:9200 --kibana http://elastic:changeme@localhost:5601/kbn --numHosts=5 --numDocs=2  --fleet

Verify Alerts, kill kibana, kill es, then start es 8.1.0 (run from kibana-main or use --version=8.1.0)

es snapshot --license trial -E xpack.security.authc.api_key.enabled=true -E path.data=$DEV_HOME/es-data

Navigate to kibana-main, checkout and yarn start
Login, navigate to previous rule (toggling enable/disable to bypass this bug), then inject some data (previous resolver_generator data-gen command should've created enough future events, but if not re-run w/o --fleet flag)

Verify `7.16` and `8.1` alerts are visible in both the Alerts Histogram and Alerts Table:

Note, Severity/Risk Score will not display for pre-8.x alerts see this comment for details.

Verify `7.16` and `8.1` alerts are closed when adding a Rule Exception (that of course matches all alerts ;) and selecting `Close all alerts that match this exception`.

Note: Initial approach was to plumb through the legacyId through the Read Rules route (#119031), so we can always revisit that method if necessary.

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

spong · 2021-12-01T02:29:26Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts

@@ -238,7 +238,7 @@ export const getFailingRules = async (
  try {
    const errorRules = await Promise.all(
      ids.map(async (id) =>
-        rulesClient.get({
+        rulesClient.resolve({


We had a bug where the RuleStatus wouldn't be found because it was querying with the old legacyId. This would result in an error toast along with the redirect toast:

That's a catch! I see that now with this PR getFailingRules is called only from the INTERNAL_DETECTION_ENGINE_RULE_STATUS_URL route, and I think we should be able to get rid of it completely when we rewrite this route to only return events from the rule execution log.

…leId

yctercero · 2021-12-03T00:54:37Z

Hey @spong , I come in peace I promise 😅 I'd been chatting with @dhurley14 and @rylnd about id vs rule_id stuff this week as platform's been working on some related id regeneration stuff. I know this PR is limited in scope to the filter query for the alerts table but there are a few questions that came up in thinking through all this.

In thinking about RAC/the goal of unified rules/alerts - how, if at all, does observability use rule_id or the concept of SIDs? Would using rule_id here in any way hinder the idea of unified rules/alerts in the scenario where lets say a user would be viewing alerts across solutions? My context might be outdated here, but it seemed back in August we'd been hoping that with RAC we'd move away from our own endpoints to a shared alerts/rule client?
Would we need to implement logic around space-awareness with rule_id in order to align with where Kibana core is heading with shared SOs? That would worry me a little bit just seeing how much work we are currently needing to do in order to align with share-ability when it comes to our own import/export APIs and exposing the origin id. We're still trying to make the move over to the SOM as soon as possible, but with that also comes the uncertainty of being able to have import logic that relies on rule_id like our current implementation.

Please don't take this as me saying rule_id should not be used - it's clear SIDs are important within the security context and rule sharing, but I'm hesitant about using it in a programatic sense here and relying on it to correlate rules <--> signals. I know it's been discussed a ton already, but I'd love to hear what you think about some of this stuff as I haven't been as involved in the rule registry/RAC for a few months and maybe there's context I'm missing.

spong · 2021-12-03T02:33:19Z

Peace offering accepted, haha! 🕊️ 😅 No worries at all @yctercero -- thank you for taking the time to think through the implications and potential externalities here, I really appreciate the extra 👀's and 🧠🔋!

To summarize from our conversation for others (we chatted offline a bit), regardless of the path forward to resolve #118935 (whether pushing down legacyId to the front end, or querying via ruleId), we're going to have to deal with the two concerns outlined above since Security Solution already supports this notion of a secondary static id/SID (signature ID) for our rules. So whether we're aligning for RAC with Stack, O11y and other solutions, or adding support for shared SO's and exposing Security Rules in the SOM, these are bridges we're going to have to cross.

Zooming out, the commonality that surfaced when we were discussing this is a platform need (of the SO Client most likely) to support specifying additional static field(s) when creating SO's. This pattern of specifying secondary static id's exists in quite a few places already within Security Solution (Timeline Templates, Exceptions/Trusted Apps, Rules), and all of these objects are going to need special care when SO sharing or SOM support are introduced if this functionality isn't added.

At the very least, this change specifically is reversible, and if we need to go back to querying alerts via rule.id at some point in the future, we'll have that flexibility. Now if we were making more sweeping (non read-only) correlations between rules <--> alerts, then I would have a bit more concern about the reversibility of these changes, but since it's a read-only operation, and we're still maintaining all the same data as before, I have less hesitation that this change would be reasonable to make at this time.

As an output, we discussed auditing Security Solution for two things:

Auditing the Rules API's for usage of rule.id vs rule.rule_id to ensure we have a solid map of where these touchpoints are and so we can better understand the implications of around questions 1&2 above when that time comes.
Auditing the usage of SO's leveraging secondary static id's so we can get a better picture for a platform requirement for these types of SO's and have a universal way of getting them to work with SO sharing and importing/exporting via SOM (so additional solution-specific middleware validation isn't necessary).

elasticmachine · 2021-12-03T03:50:13Z

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

elasticmachine · 2021-12-03T03:50:15Z

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

x-pack/plugins/security_solution/public/detections/components/alerts_table/default_config.tsx

spong · 2021-12-03T04:05:50Z

...k/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx

-            ...buildShowBuildingBlockFilter(showBuildingBlockAlerts),
-            ...buildAlertStatusFilter(filterGroup),
-          ]),
+      ...buildAlertsRuleIdFilter(rule?.rule_id ?? ''),


Would be nice to refactor this component so that these checks aren't necessary. Right now we're fetching the rule via useRuleWithFallback and have the page in a loading state while rule == null, and all these filters that are being generated aren't needed till the rule returns anyway.

…leId

…to use new filter

…leId

kibana-ci · 2021-12-07T03:55:29Z

💚 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
`@kbn/rule-data-utils`	71	72	+1

Async chunks

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

id	before	after	diff
`cases`	297.4KB	297.5KB	+103.0B
`infra`	990.9KB	991.3KB	+412.0B
`observability`	362.4KB	362.5KB	+103.0B
`securitySolution`	4.6MB	4.5MB	-1.6KB
total			-1.0KB

Page load bundle

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

id	before	after	diff
`apm`	29.3KB	29.4KB	+103.0B
`securitySolution`	243.1KB	243.2KB	+103.0B
`timelines`	164.5KB	164.6KB	+103.0B
`uptime`	23.2KB	23.3KB	+103.0B
total			+412.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/rule-data-utils`	74	75	+1

History

💛 Build #11502 was flaky 5599fe2
💚 Build #11453 succeeded e0285b4
💚 Build #10853 succeeded 4ad6ec0
💚 Build #10572 succeeded d9694ae
💔 Build #10230 failed ca5975c

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

cc @spong

banderror

LGTM 👍 Thank you so much for all the thought put into solving this issue, finding a better solution than just passing the legacy id to the FE, adjusting the exceptions logic, for the cleanup and super helpful comments in the code. So much great stuff in this PR!

I went through the code changes and didn't find any issues. Checked the filter used in useRuleWithFallback for fetching the last alert for the rule - seems like @madirey already updated it in her previous PR.

Sorry I didn't test the PR though, I hope that folks could help with that.

banderror · 2021-12-07T19:48:20Z

x-pack/plugins/security_solution/public/common/components/exceptions/use_add_exception.tsx

+          const alertStatusFilter = buildAlertStatusesFilter([
+            'open',
+            'acknowledged',
+            'in-progress',
+          ]);


Garrett, do you know if a go/no-go decision has been made regarding going live with rule_registry in 8.0? There's still some ongoing discussions regarding AAD schema which is extremely concerning considering where we are in the release cycle. Wondering what are chances of us being asked to revert this stuff back to .siem-signals-* (which means we'd need the ruleRegistryEnabled feature flag in the code):

// TODO: Once we are past experimental phase this code should be removed const alertStatusFilter = ruleRegistryEnabled ? buildAlertStatusesFilterRuleRegistry(['open', 'acknowledged', 'in-progress']) : buildAlertStatusesFilter(['open', 'acknowledged', 'in-progress']);

Verified RuleRegistry is a go for security in 8.0, so all good here 🙂

banderror · 2021-12-07T19:52:20Z

x-pack/plugins/security_solution/public/detections/components/alerts_table/default_config.tsx

                term: {
-                  'kibana.alert.workflow_status': status,
+                  [ALERT_WORKFLOW_STATUS]: status,
                },
              },
              {
                term: {
-                  'kibana.alert.workflow_status': 'in-progress',
+                  [ALERT_WORKFLOW_STATUS]: 'in-progress',


Thank you for all the clean up 🙏

banderror · 2021-12-07T19:54:24Z

x-pack/plugins/security_solution/public/detections/components/alerts_table/default_config.tsx

+/**
+ * Builds Kuery filter for fetching alerts for a specific rule. Takes the rule's
+ * static id, i.e. `rule.ruleId` (not rule.id), so that alerts for _all
+ * historical instances_ of the rule are returned.
+ *
+ * @param ruleStaticId Rule's static id: `rule.ruleId`
+ */
+export const buildAlertsFilter = (ruleStaticId: string | null): Filter[] =>


And thank you for the detailed comments on ruleStaticId explaining what it is and what are the implications of using it ("so that alerts for all historical instances of the rule are returned"). This is really helpful!

Of course! It gets so confusing between rule.id and rule.rule_id I needed to start clearing things up with differentiating nomenclature. If we're consistent going forward with naming rule.rule_id params as ruleStaticId it should make things a bit easier to grok.

banderror · 2021-12-07T20:12:37Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts

@@ -238,7 +238,7 @@ export const getFailingRules = async (
  try {
    const errorRules = await Promise.all(
      ids.map(async (id) =>
-        rulesClient.get({
+        rulesClient.resolve({


That's a catch! I see that now with this PR getFailingRules is called only from the INTERNAL_DETECTION_ENGINE_RULE_STATUS_URL route, and I think we should be able to get rid of it completely when we rewrite this route to only return events from the rule execution log.

banderror · 2021-12-07T20:50:37Z

Oh sorry, one more thing

Note, Severity/Risk Score will not display for pre-8.x alerts see this comment for details.

So is this a known issue that is going to be fixed? Do we have a ticket?

spong · 2021-12-07T20:54:59Z

Oh sorry, one more thing

Note, Severity/Risk Score will not display for pre-8.x alerts see this comment for details.

So is this a known issue that is going to be fixed? Do we have a ticket?

This is known (found during that review) and @marshallmain said he would have a fix for this in a follow-up PR, so no specific issue that I'm aware of. I'll check with Marshall to see if there's a draft PR floating around and create an issue if not just so we don't lose it. 👍

edit: Yup, this was added and being tracked as part of this issue/comment: https://github.com/elastic/security-team/issues/1018#issuecomment-937323352

Severity and risk_score overrides are being written to kibana.alert.rule.<severity | risk_score instead of kibana.alert.<severity | risk_score>

…y RuleId instead of Rule SO ID (elastic#120053) * Switches RuleDetails to query alerts by ruleId instead of SO id * Increases integrity of test outputs * Cleans up duplicate RuleRegistry functions * Removes support for rule.id for alerts filter and updates exceptions to use new filter

kibanamachine · 2021-12-08T00:17:10Z

💚 Backport successful

Status	Branch	Result
✅	8.0

This backport PR will be merged automatically after passing CI.

…y RuleId instead of Rule SO ID (elastic#120053) * Switches RuleDetails to query alerts by ruleId instead of SO id * Increases integrity of test outputs * Cleans up duplicate RuleRegistry functions * Removes support for rule.id for alerts filter and updates exceptions to use new filter

…y RuleId instead of Rule SO ID (#120053) (#120710) * Switches RuleDetails to query alerts by ruleId instead of SO id * Increases integrity of test outputs * Cleans up duplicate RuleRegistry functions * Removes support for rule.id for alerts filter and updates exceptions to use new filter

spong · 2021-12-16T22:30:19Z

As per elastic/security-docs#1321 (comment), updating title and adding to release notes so the behavioral changes are appropriately communicated.

…y RuleId instead of Rule SO ID (elastic#120053) * Switches RuleDetails to query alerts by ruleId instead of SO id * Increases integrity of test outputs * Cleans up duplicate RuleRegistry functions * Removes support for rule.id for alerts filter and updates exceptions to use new filter

## Closing alerts from flyout effect only alerts related to this rule Fix: #145675 For the exceptions component, we need to have `rule.rule_id` which wasn't initially in the timeline response. We can't safely use `rule.id`, it is [described here](#120053). Co-authored-by: Kibana Machine <[email protected]>

## Closing alerts from flyout effect only alerts related to this rule Fix: elastic#145675 For the exceptions component, we need to have `rule.rule_id` which wasn't initially in the timeline response. We can't safely use `rule.id`, it is [described here](elastic#120053). Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 6102f0e)

# Backport This will backport the following commits from `main` to `8.6`: - [Fix close alerts from flyout (#145939)](#145939)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Khristinin Nikita <[email protected]>

Switches RuleDetails to query alerts by ruleId instead of SO id

ca5975c

spong self-assigned this Dec 1, 2021

spong commented Dec 1, 2021

View reviewed changes

spong added 3 commits December 1, 2021 22:12

Increases integrity of test outputs

3191eec

Merge branch 'main' of github.com:elastic/kibana into rule-details-ru…

d9694ae

…leId

Merge branch 'main' of github.com:elastic/kibana into rule-details-ru…

f96a6c8

…leId

Cleans up duplicate RuleRegistry functions

4ad6ec0

spong requested review from a team December 3, 2021 03:49

spong marked this pull request as ready for review December 3, 2021 03:50

spong requested a review from a team as a code owner December 3, 2021 03:50

spong commented Dec 3, 2021

View reviewed changes

x-pack/plugins/security_solution/public/detections/components/alerts_table/default_config.tsx Outdated Show resolved Hide resolved

spong commented Dec 3, 2021

View reviewed changes

spong added 3 commits December 6, 2021 10:06

Merge branch 'main' of github.com:elastic/kibana into rule-details-ru…

e0285b4

…leId

Merge branch 'main' of github.com:elastic/kibana into rule-details-ru…

5599fe2

…leId

Removes support for rule.id for alerts filter and updates exceptions …

ef2c3d6

…to use new filter

spong added 2 commits December 6, 2021 19:10

Merge branch 'main' of github.com:elastic/kibana into rule-details-ru…

e97e910

…leId

Merge branch 'main' of github.com:elastic/kibana into rule-details-ru…

24b226b

…leId

dhurley14 mentioned this pull request Dec 7, 2021

[Security Solution] [Platform] Updates read rules route to include legacy id by default #119031

Closed

2 tasks

banderror approved these changes Dec 7, 2021

View reviewed changes

spong merged commit 3d9bfe2 into elastic:main Dec 8, 2021

spong deleted the rule-details-ruleId branch December 8, 2021 00:13

kibanamachine mentioned this pull request Dec 8, 2021

[8.0] [Security Solution][Detections] Updates RuleDetails to query alerts by RuleId instead of Rule SO ID (#120053) #120704

Closed

spong mentioned this pull request Dec 8, 2021

[8.0] [Security Solution][Detections] Updates RuleDetails to query alerts by RuleId instead of Rule SO ID (#120053) #120710

Merged

spong changed the title ~~[Security Solution][Detections] Updates RuleDetails to query alerts by RuleId instead of Rule SO ID~~ [Security Solution][Detections] Updates Rule Details to show all historical alerts for a given Rule Dec 16, 2021

spong added release_note:feature Makes this part of the condensed release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 16, 2021

spong mentioned this pull request Mar 2, 2022

[DOCS] Clarify note about exported case alerts elastic/security-docs#1637

Merged

nkhristinin mentioned this pull request Nov 22, 2022

Fix close alerts from flyout #145939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Updates Rule Details to show all historical alerts for a given Rule #120053

[Security Solution][Detections] Updates Rule Details to show all historical alerts for a given Rule #120053

spong commented Dec 1, 2021 •

edited

Loading

spong Dec 1, 2021

banderror Dec 7, 2021

yctercero commented Dec 3, 2021

spong commented Dec 3, 2021

elasticmachine commented Dec 3, 2021

elasticmachine commented Dec 3, 2021

spong Dec 3, 2021

kibana-ci commented Dec 7, 2021

API count

banderror left a comment

banderror Dec 7, 2021

spong Dec 8, 2021

banderror Dec 7, 2021

banderror Dec 7, 2021

spong Dec 8, 2021

banderror Dec 7, 2021

banderror commented Dec 7, 2021

spong commented Dec 7, 2021 •

edited

Loading

kibanamachine commented Dec 8, 2021

spong commented Dec 16, 2021

[Security Solution][Detections] Updates Rule Details to show all historical alerts for a given Rule #120053

[Security Solution][Detections] Updates Rule Details to show all historical alerts for a given Rule #120053

Conversation

spong commented Dec 1, 2021 • edited Loading

Summary

Test Instructions:

Verify 7.16 and 8.1 alerts are visible in both the Alerts Histogram and Alerts Table:

Verify 7.16 and 8.1 alerts are closed when adding a Rule Exception (that of course matches all alerts ;) and selecting Close all alerts that match this exception.

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yctercero commented Dec 3, 2021

spong commented Dec 3, 2021

elasticmachine commented Dec 3, 2021

elasticmachine commented Dec 3, 2021

Choose a reason for hiding this comment

kibana-ci commented Dec 7, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

banderror left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror commented Dec 7, 2021

spong commented Dec 7, 2021 • edited Loading

kibanamachine commented Dec 8, 2021

💚 Backport successful

spong commented Dec 16, 2021

spong commented Dec 1, 2021 •

edited

Loading

Verify `7.16` and `8.1` alerts are visible in both the Alerts Histogram and Alerts Table:

Verify `7.16` and `8.1` alerts are closed when adding a Rule Exception (that of course matches all alerts ;) and selecting `Close all alerts that match this exception`.

spong commented Dec 7, 2021 •

edited

Loading