[CTI] Filters alerts table by presence of threat (elastic/security-team#907) #96096

ecezalp · 2021-04-01T20:27:48Z

Closes elastic/security-team#907.

Summary

This change enables alert table filtering by the presence of signal.rule.threat_mapping. Previously we have discussed the usage of threat.indicator.matched to detect whether an alert is a "threat" or not, but I was unable to get hits on threat and any fields that would be under it, even though I was able to see the fields when I queried my siem-signals index with a match all query. So, this implementation detail (using signal.rule.threat_mapping instead of threat.indicator.matched is up for discussion.

Another interesting finding was around the default filters, it appeared that the filters were reversed, in the sense that I had to use a must_not query to obtain the results containing the threats, as opposed to a must. I found this peculiar, so please let me know if there are any improvements to be made in this area.

Lastly, a few unit tests were added, but so far there are no integration tests, I wasn't able to find if the Alerts Table is currently covered with integration tests, so I would appreciate a nudge towards that direction.

Images

not applied (Detections Home)

applied (Detections Home)

also available on the Rules page

Checklist

Delete any items that are not applicable to this PR.

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…am#907)

...ecurity_solution/public/detections/components/alerts_table/alerts_utility_bar/index.test.tsx

rylnd

So, this implementation detail (using signal.rule.threat_mapping instead of threat.indicator.matched is up for discussion.

My initial thinking was that we need a canonical definition for a threat match alert, and that "has threat match fields" was the simplest answer. To that end, I'd be happy to pair to see if we can't figure out a query that works; I'm guessing it's either the nested fields thing, or a consequence of the fields being requested.

I was also going to claim that this logic would also catch old, pre-enriched indicator alerts (which I don't think we want), however, we didn't have mappings for these fields until 7.12, so (I think) both enrichment and the ability to query threat_mapping happened in the same release, so this should work? Regardless, I think we should try to make the former work before settling on this solution.

You've got some type failures which look related to one of my questions, so let's discuss this again on Monday :)

...ecurity_solution/public/detections/components/alerts_table/alerts_utility_bar/index.test.tsx

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

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

filter instead to achieve the same result. An old TODO has been addressed by adding an optional boolean exists to Filter type

ecezalp · 2021-04-06T15:59:01Z

@kibana-app-services you have been summoned because I temporarily removed a TODO that was left in the codebase, which suppressed a ts-error regarding the Filter not having an "exists" field. I briefly added an optional field to the Filter object in the data plugin, but that seemed to result in a lot of type errors in this build, so I decided to bring back the TODO -- nothing to see here. Creating a new ticket for the removal of the TODO for the future, will attach to this comment shortly.

rylnd

The filter functionality works great! The only things to address are the label copy and the space between the two labels.

rylnd · 2021-04-07T03:20:55Z

...ins/security_solution/public/detections/components/alerts_table/alerts_utility_bar/index.tsx

@@ -159,6 +167,20 @@ const AlertsUtilityBarComponent: React.FC<AlertsUtilityBarProps> = ({
          label={i18n.ADDITIONAL_FILTERS_ACTIONS_SHOW_BUILDING_BLOCK}
        />
      </BuildingBlockContainer>
+      <ThreatMatchContainer>


nit:

Suggested change

<ThreatMatchContainer>

<ThreatMatchFilterContainer>

rylnd · 2021-04-07T03:26:48Z

...curity_solution/public/detections/components/alerts_table/alerts_utility_bar/translations.ts

+export const ADDITIONAL_FILTERS_ACTIONS_SHOW_THREAT_MATCHES_ONLY = i18n.translate(
+  'xpack.securitySolution.detectionEngine.alerts.utilityBar.additionalFiltersActions.showThreatMatchesOnly',
+  {
+    defaultMessage: 'Show threat indicator matches only',


Suggested change

defaultMessage: 'Show threat indicator matches only',

defaultMessage: 'Show only threat match alerts',

Thoughts: I would emphasize the entities that this filter is acting on (alerts), and move "only" to the beginning of the label since it's logically different than the existing building block query.

RE qualifying alert, I like "threat indicator alert" more than "threat indicator match alert" as that seems redundant, but we should probably rope product in for the final say here. /cc @MikePaquette @devonakerr

I think "threat indicator match alert" is awkward, and "threat indicator alert" is more concise without losing meaning.

updating as Show only threat indicator alerts

...ins/security_solution/public/detections/components/alerts_table/alerts_utility_bar/index.tsx

rylnd

I think this PR unintentionally inverted the logic of the building block filter; please do fix that before merging. Other than that, this looks great!

rylnd · 2021-04-08T00:20:49Z

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

@@ -286,10 +288,11 @@ const RuleDetailsPageComponent = () => {

  const alertDefaultFilters = useMemo(
    () => [
-      ...(ruleId != null ? buildAlertsRuleIdFilter(ruleId) : []),


Can we safely remove this ternary that was here before? What was its purpose?

Update: never mind, I see that you moved this logic into the function itself 👍

Would be great to add a quick unit test for that 😉

rylnd · 2021-04-08T00:22:55Z

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

  onShowBuildingBlockAlertsChanged: (showBuildingBlockAlerts: boolean) => void;
+  onShowThreatMatchesOnlyChanged: (showBuildingBlockAlerts: boolean) => void;


nit:

Suggested change

onShowThreatMatchesOnlyChanged: (showBuildingBlockAlerts: boolean) => void;

onShowThreatMatchesOnlyChanged: (showThreatMatchesOnly: boolean) => void;

rylnd · 2021-04-08T00:34:17Z

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


-export const buildShowBuildingBlockFilter = (showBuildingBlockAlerts: boolean): Filter[] => [


I think the logic was inverted here; we were previously returning an empty array if showBuildingBlockAlerts was truthy.

...ins/security_solution/public/detections/components/alerts_table/alerts_utility_bar/index.tsx

kibanamachine · 2021-04-12T21:08:32Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: 5ae850c
Storybooks Preview
Flaky suites:
- xpack-securitySolutionCypressChrome

Test Failures

Kibana Pipeline / general / "before all" hook for "should open a modal".Open timeline Open timeline modal "before all" hook for "should open a modal"

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: Expected to find element: `[data-test-subj="title-924470b0-9bca-11eb-bf7d-539c5070ec2a"]`, but never found it.

Because this error occurred during a `before all` hook we are skipping the remaining tests in the current suite: `Open timeline`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
    at Object.openTimelineById (http://localhost:6121/__cypress/tests?p=cypress/integration/timelines/open_timeline.spec.ts:16092:15)
    at Context.eval (http://localhost:6121/__cypress/tests?p=cypress/integration/timelines/open_timeline.spec.ts:15046:28)

Metrics [docs]

Async chunks

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

id	before	after	diff
`securitySolution`	7.3MB	7.3MB	+2.9KB

History

💛 Build #118914 was flaky 945101b
💔 Build #117984 failed a956112
💔 Build #117939 failed 1d3b581
💔 Build #117645 failed 8a63480
💔 Build #117615 failed 0ac8f87

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

…am#907) (elastic#96096) [CTI] Filters alerts table by presence of threat (elastic/security-team#907)

…am#907) (#96096) (#96897) [CTI] Filters alerts table by presence of threat (elastic/security-team#907)

[CTI] Filters alerts table by presence of threat (elastic/security-te…

077ac2a

…am#907)

ecezalp added 7.13 candidate Team: CTI labels Apr 1, 2021

ecezalp requested review from rylnd and a team April 1, 2021 20:27

ecezalp added release_note:feature Makes this part of the condensed release notes v7.13.0 labels Apr 1, 2021

ecezalp commented Apr 1, 2021

View reviewed changes

...ecurity_solution/public/detections/components/alerts_table/alerts_utility_bar/index.test.tsx Show resolved Hide resolved

rylnd reviewed Apr 3, 2021

View reviewed changes

ecezalp and others added 2 commits April 5, 2021 11:35

fixes type error, updates meta values of filters

66be4d2

Merge branch 'master' into threat-filter

f997f01

rylnd reviewed Apr 5, 2021

View reviewed changes

...k/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx Outdated Show resolved Hide resolved

ecezalp requested a review from a team as a code owner April 6, 2021 14:45

Reverts change to buildAlertsRuleIdFilter, and combines the threat …

20c3466

filter instead to achieve the same result. An old TODO has been addressed by adding an optional boolean exists to Filter type

ecezalp force-pushed the threat-filter branch from 1b0acb4 to 20c3466 Compare April 6, 2021 15:35

ecezalp and others added 2 commits April 6, 2021 12:55

Adds ts-ignore to default_config.test.ts

0ac8f87

Merge branch 'master' into threat-filter

8a63480

rylnd reviewed Apr 7, 2021

View reviewed changes

ecezalp and others added 3 commits April 7, 2021 12:37

updates AdditionalFilterContent gutter size and message

6052993

Merge branch 'master' into threat-filter

1d3b581

Merge branch 'master' into threat-filter

a956112

rylnd approved these changes Apr 8, 2021

View reviewed changes

ecezalp and others added 3 commits April 12, 2021 14:13

fixes reverted buildShowBuildingBlockFilter logic

cce193f

Merge branch 'master' into threat-filter

945101b

rename commit

5ae850c

ecezalp merged commit e7ecad7 into elastic:master Apr 12, 2021

ecezalp added the backport label Apr 12, 2021

ecezalp added a commit to ecezalp/kibana that referenced this pull request Apr 12, 2021

[CTI] Filters alerts table by presence of threat (elastic/security-te…

befe374

…am#907) (elastic#96096) [CTI] Filters alerts table by presence of threat (elastic/security-team#907)

ecezalp mentioned this pull request Apr 12, 2021

[7.x] [CTI] Filters alerts table by presence of threat (elastic/security-team#907) (#96096) #96897

Merged

ecezalp added a commit that referenced this pull request Apr 12, 2021

[CTI] Filters alerts table by presence of threat (elastic/security-te…

6958776

…am#907) (#96096) (#96897) [CTI] Filters alerts table by presence of threat (elastic/security-team#907)

jmikell821 mentioned this pull request Apr 30, 2021

[DOCS] Update "Managing detection alerts" topic elastic/security-docs#664

Closed

nastasha-solomon mentioned this pull request May 3, 2021

[DOCS] Updating "Manage detection alerts" topic elastic/security-docs#666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CTI] Filters alerts table by presence of threat (elastic/security-team#907) #96096

[CTI] Filters alerts table by presence of threat (elastic/security-team#907) #96096

ecezalp commented Apr 1, 2021 •

edited

Loading

rylnd left a comment

ecezalp commented Apr 6, 2021

rylnd left a comment

rylnd Apr 7, 2021

rylnd Apr 7, 2021

devonakerr Apr 7, 2021

ecezalp Apr 7, 2021 •

edited

Loading

rylnd left a comment

rylnd Apr 8, 2021

rylnd Apr 8, 2021

rylnd Apr 8, 2021

rylnd Apr 8, 2021

rylnd Apr 8, 2021

kibanamachine commented Apr 12, 2021

Stack Trace

	defaultMessage: 'Show threat indicator matches only',
	defaultMessage: 'Show only threat match alerts',

		onShowBuildingBlockAlertsChanged: (showBuildingBlockAlerts: boolean) => void;
		onShowThreatMatchesOnlyChanged: (showBuildingBlockAlerts: boolean) => void;

	onShowThreatMatchesOnlyChanged: (showBuildingBlockAlerts: boolean) => void;
	onShowThreatMatchesOnlyChanged: (showThreatMatchesOnly: boolean) => void;


		export const buildShowBuildingBlockFilter = (showBuildingBlockAlerts: boolean): Filter[] => [

[CTI] Filters alerts table by presence of threat (elastic/security-team#907) #96096

[CTI] Filters alerts table by presence of threat (elastic/security-team#907) #96096

Conversation

ecezalp commented Apr 1, 2021 • edited Loading

Summary

Images

Checklist

For maintainers

rylnd left a comment

Choose a reason for hiding this comment

ecezalp commented Apr 6, 2021

rylnd 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

ecezalp Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

rylnd 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

kibanamachine commented Apr 12, 2021

💛 Build succeeded, but was flaky

Stack Trace

Metrics [docs]

Async chunks

History

ecezalp commented Apr 1, 2021 •

edited

Loading

ecezalp Apr 7, 2021 •

edited

Loading