Asset criticality alert enrichment #171241

oatkiller · 2023-11-14T20:38:37Z

Summary

This will allow analysts to filter alerts by the analyst-defined criticality of the related host or user.

This introduces two new kibana fields to alerts. These fields allow us to model the criticality of the alert's most relevant host and user, in context of analyst workflows.

field name	type	description	example
`kibana.alert.host.criticality_level`	keyword	Contains an enum describing the criticality of the host, as defined by the analyst in a previously used workflow. Used by analysts to filter alerts by host criticality.	'very important'
`kibana.alert.user.criticality_level`	keyword	Contains an enum describing the criticality of the user, as defined by the analyst in a previously used workflow. Used by analysts to filter alerts by user criticality.	'very important'

Design

Analysts can assign criticality to their assets. These are stored in the asset criticality index (.asset-criticality.asset-criticality-${space}). This PR will detect user names and host names in events, query the asset criticality index for the associated criticality (if any) and then add that value to the resulting alert under kibana.alert.host.criticality_level or kibana.alert.user.criticality_level

Design Exploration

What if we want to filter by other criticalities?

We may want to allow analysts to filter on the criticality of other entities. For example, alerts can refer to multiple users and multiple hosts. Also, we may introduce other types of entities, e.g. IP addresses, files, registry keys.

If we were to follow the same approach as we did here, that could lead to a mapping explosion if we had a lot. However:

we don't think we'll add very many classes of entities. less than 8 more.
we don't necessarily want to filter alerts by other ones.

Using keyword vs nested or flattened

We could use a flattened or nested field here to store more criticalities and have them all indexed. However we don't need to sort or order alerts by their entity's criticalities so we don't see a need.

KQL support

The fields we proposed here are of keyword type. This type of field works intuitively for KQL and since analyst workflows are the focus of this change, this lines up well.

How to test

until asset criticality UI is not enabled let's create a mapping for asset criticality and index some docs

PUT .asset-criticality.asset-criticality-default
{
  "mappings": {
    "properties": {
      "id_value": {
        "type": "keyword"
      },
      "id_field": {
        "type": "keyword"
      },
      "criticality_level": {
        "type": "keyword"
      },
      "@timestamp": {
        "type": "date"
      },
      "updated_at": {
        "type": "date"
      }
    }
  }
}

POST .asset-criticality.asset-criticality-default/_doc
{
  "id_field": "user.name",
  "id_value": "User 1",
  "criticality_level": "very important",
  "@timestamp": 1701860267617
}

POST .asset-criticality.asset-criticality-default/_doc
{
  "id_field": "host.name",
  "id_value": "Host 3",
  "criticality_level": "normal",
  "@timestamp": 1701860267617
}

Then create rules, which have alerts from events with host.name or user.name which match asset criticality documents.

Then add fields to the alerts table, you should see some values in those columns

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

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

nkhristinin · 2023-12-01T11:18:03Z

@elasticmachine merge upstream

nkhristinin · 2023-12-04T14:32:59Z

@elasticmachine merge upstream

…-fix'

nkhristinin · 2023-12-06T10:15:25Z

@elasticmachine merge upstream

nkhristinin · 2023-12-12T07:19:21Z

@elasticmachine merge upstream

nkhristinin · 2023-12-12T10:05:33Z

@elasticmachine merge upstream

nkhristinin · 2023-12-13T09:45:37Z

@elasticmachine merge upstream

e40pud

LGTM!

e40pud · 2023-12-13T10:37:03Z

...curity_solution/server/lib/detection_engine/rule_types/utils/enrichments/utils/transforms.ts

@@ -23,7 +23,7 @@ export const applyEnrichmentsToEvents: ApplyEnrichmentsToEvents = ({
 enrichmentsList,
 logger,
 }) => {
- const mergedEnrichments = mergeEnrichments(enrichmentsList);
+ const mergedEnrichments: EventsMapByEnrichments = mergeEnrichments(enrichmentsList);


do we need : EventsMapByEnrichments here? it looks like mergeEnrichments return type is explicitly set to EventsMapByEnrichments, so the type should be deduced automatically?

nkhristinin · 2023-12-19T10:00:59Z

@elasticmachine merge upstream

rylnd

I made another pass here; enrichment additions make sense and look good (although I can't speak to performance here; how many more ES queries does this add to each rule execution?)

My broad concern is still the fields in which we're placing these enrichments: I made this comment a few weeks ago that wasn't addressed. If we define e.g. host.risk.criticality_level to generally be "the criticality level of the relevant entity at the time the document was created," that should cover both of these use cases and eliminate the need for these additional non-ECS fields.

rylnd · 2023-12-20T18:49:45Z

...er/lib/detection_engine/rule_types/utils/enrichments/create_single_field_match_enrichment.ts

+ // gets just the events we will enrich
 const eventsWithField = events.filter((event) => getEventValue(event, mappingField.eventField));


@oatkiller were these meant to stick around, or were these just notes for you? Typically when I see comments like this in the code, it's making up for an unhelpful variable name. IMO these variables are good at describing what they are, but not why they are. Perhaps the comments wouldn't be needed if the intention was more clear?

Suggested change

// gets just the events we will enrich

const eventsWithField = events.filter((event) => getEventValue(event, mappingField.eventField));

const eventsToEnrich = events.filter((event) => getEventValue(event, mappingField.eventField));

rylnd · 2023-12-20T19:01:03Z

...security_solution/server/lib/detection_engine/rule_types/utils/enrichments/utils/requests.ts

@@ -26,6 +31,7 @@ export const makeSingleFieldMatchQuery: MakeSingleFieldMatchQuery = ({ values, s
 query: {
 bool: {
 should: shouldClauses,


@nkhristinin I know this isn't part of the PR, but I was exploring how best to construct a query like this for asset criticality, and found that wrapping the should in a filter query has the same behavior with the performance benefit of not scoring the results.

rylnd · 2023-12-20T20:10:32Z

...suites/detections_response/default_license/rule_execution_logic/configs/serverless.config.ts

@@ -17,6 +17,7 @@ export default createTestConfig({
 '/testing_regex*/',
 ])}`, // See tests within the file "ignore_fields.ts" which use these values in "alertIgnoreFields"
 `--xpack.securitySolution.enableExperimental=${JSON.stringify([
+ 'entityAnalyticsAssetCriticalityEnabled',


Do these flags actually apply in serverless? If not, they should be deleted. If they do, but serverless doesn't provide an actual way to set these in production, then they should also be deleted.

But as I understand those tests will run in a serverless environment anyway, which allows us to test and integrate our functionality during development, and catch some serverless issues earlier.

If we remove the flag and skip tests, then in time when we enable it and try to run it serverless we can have some problems. For example we didn't have serverless tests for risk engine enablement, and then there were errors for permissions

rylnd · 2023-12-20T20:15:35Z

.../test_suites/detections_response/default_license/rule_execution_logic/execution_logic/eql.ts

+ const previewAlerts = await getPreviewAlerts({ es, previewId });
+ const fullAlert = previewAlerts[0]._source;
+ if (!fullAlert) {
+ return expect(fullAlert).to.be.ok();


Why is the early return needed here? At first I thought it was so that line 645 doesn't throw a ReferenceError, but shouldn't the optional chaining prevent that anyway?

rylnd · 2023-12-20T20:21:17Z

...detections_response/default_license/rule_execution_logic/execution_logic/machine_learning.ts

+
+ describe('with asset criticality', async () => {
+ before(async () => {
+ await esArchiver.load('x-pack/test/functional/es_archives/asset_criticality');


If these tests hinge on a specific foreign key across these data sources, it would be good to call that out (similar to how @marshallmain does with ID here.

rylnd · 2023-12-20T20:28:50Z

...er/lib/detection_engine/rule_types/utils/enrichments/create_single_field_match_enrichment.ts

 .map((result) => (result as PromiseFulfilledResult<EnrichmentType[]>)?.value);

+ // search hits.
 const enrichments = flatten(enrichmentsResults);


Suggested change

.map((result) => (result as PromiseFulfilledResult<EnrichmentType[]>)?.value);

// search hits.

const enrichments = flatten(enrichmentsResults);

.flatMap((result) => (result as PromiseFulfilledResult<EnrichmentType[]>)?.value);

(you'd also need to rename enrichmentResults to just enrichments, but I couldn't include that line in this comment)

rylnd · 2023-12-20T20:42:18Z

...ins/security_solution/server/lib/detection_engine/rule_types/utils/enrichments/index.test.ts

+jest.mock('./enrichment_by_type/asset_criticality', () => ({
+ ...jest.requireActual('./enrichment_by_type/asset_criticality'),
+ doesAssetCriticalityIndexExist: jest.fn(),
+}));


To me this is an indication that doesAssetCriticalityIndexExists (sic) should live independent to the enrichments themselves: we're trying to mock a file that we also want to test.

nkhristinin · 2023-12-21T09:32:12Z

My broad concern is still the fields in which we're placing these enrichments: I made this comment a few weeks ago that wasn't addressed if we define e.g. host.risk.criticality_level to generally be "the criticality level of the relevant entity at the time the document was created," that should cover both of these use cases and eliminate the need for these additional non-ECS fields.

@oatkiller @kobelb I know that you have some discussion about field names, can you provide here some additional context?

nkhristinin · 2023-12-21T13:14:11Z

@elasticmachine merge upstream

kibana-ci · 2023-12-21T18:41:47Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 5c1de12

Failed CI Steps

FTR Configs #37

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`	11.3MB	11.3MB	+2.6KB

History

💔 Build #184772 failed c296a4c
💔 Build #184278 failed 81ca783
💔 Build #184188 failed 08611bf
💔 Build #183380 failed 81142da
💔 Build #183356 failed 9ca2fa7
💔 Build #183127 failed 0695181

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

maryam-saeidi

LGTM!

oatkiller force-pushed the asset-criticality-alert-enrichment branch 2 times, most recently from 95bb2e1 to 577bc3d Compare November 28, 2023 15:54

oatkiller added 10 commits November 30, 2023 10:52

Add new fields to alertsFieldMap

ee2fac5

Add new fields to alerts model including BaseFieldsLatest, etc

5f13e85

Add new alert columns to table

37e1dbf

refactoring alert enrichment code.

243c117

fix makeSingleFieldMatchQuery snapshot

8fd21de

restore create_single_field_match_enrichment to main version

c692d33

remove references to should_minimum_match

887764a

add new extraFilter option to createSingleMatchEnrichment

0f6d337

put back all the stuff abount minimum_should_match

29f7513

work in progress

12aa184

nkhristinin force-pushed the asset-criticality-alert-enrichment branch from 3777033 to 12aa184 Compare November 30, 2023 09:54

nkhristinin added 3 commits November 30, 2023 16:59

Add alert enrichment

e3b7020

Add integrations tests

c9b2cf1

remove owners.csv

2111edf

kibanamachine and others added 5 commits December 1, 2023 06:18

Merge branch 'main' into asset-criticality-alert-enrichment

219ba82

Schema changes

070b43d

fix unit tests

0b77a96

Add more unit tests

b31beb6

Merge branch 'main' into asset-criticality-alert-enrichment

f862201

kibanamachine added 2 commits December 4, 2023 09:33

Merge branch 'main' into asset-criticality-alert-enrichment

78a3694

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

c4a461e

…-fix'

kibanamachine and others added 4 commits December 6, 2023 05:15

Merge branch 'main' into asset-criticality-alert-enrichment

c97c330

Change alert schema

d3c5122

change how to set field to alert

fe33f17

add more tests

10ee114

nkhristinin added 2 commits December 11, 2023 12:33

remoe comments

4cf765e

Fix types

744aace

Merge branch 'main' into asset-criticality-alert-enrichment

7b2e1ea

kibanamachine and others added 3 commits December 12, 2023 05:05

Merge branch 'main' into asset-criticality-alert-enrichment

00fdef5

Fix path

fa8df5c

Move feature flag

0695181

Merge branch 'main' into asset-criticality-alert-enrichment

9ca2fa7

e40pud approved these changes Dec 13, 2023

View reviewed changes

Remove type annotation

81142da

kibanamachine and others added 2 commits December 19, 2023 05:01

Merge branch 'main' into asset-criticality-alert-enrichment

08611bf

fix tests

81ca783

rylnd reviewed Dec 20, 2023

View reviewed changes

PR fixes

ffeb96e

nkhristinin added 3 commits December 21, 2023 10:34

Clean tests

35a9be3

Add utility function to check if index exist

c296a4c

Rename file

b44541a

kibanamachine and others added 3 commits December 21, 2023 08:14

Merge branch 'main' into asset-criticality-alert-enrichment

ed77987

specify test parameters

84c8da9

Merge branch 'main' into asset-criticality-alert-enrichment

5c1de12

maryam-saeidi approved these changes Dec 21, 2023

View reviewed changes

nkhristinin merged commit cf9ff4d into elastic:main Dec 22, 2023
40 checks passed

kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset criticality alert enrichment #171241

Asset criticality alert enrichment #171241

oatkiller commented Nov 14, 2023 •

edited by nkhristinin

Loading

nkhristinin commented Dec 1, 2023

nkhristinin commented Dec 4, 2023

nkhristinin commented Dec 6, 2023

nkhristinin commented Dec 12, 2023

nkhristinin commented Dec 12, 2023

nkhristinin commented Dec 13, 2023

e40pud left a comment

e40pud Dec 13, 2023

nkhristinin commented Dec 19, 2023

rylnd left a comment

rylnd Dec 20, 2023

rylnd Dec 20, 2023

rylnd Dec 20, 2023

nkhristinin Dec 21, 2023

rylnd Dec 20, 2023

rylnd Dec 20, 2023

rylnd Dec 20, 2023

rylnd Dec 20, 2023

nkhristinin commented Dec 21, 2023

nkhristinin commented Dec 21, 2023

kibana-ci commented Dec 21, 2023

maryam-saeidi left a comment

		// gets just the events we will enrich
		const eventsWithField = events.filter((event) => getEventValue(event, mappingField.eventField));

	// gets just the events we will enrich
	const eventsWithField = events.filter((event) => getEventValue(event, mappingField.eventField));
	const eventsToEnrich = events.filter((event) => getEventValue(event, mappingField.eventField));

Asset criticality alert enrichment #171241

Asset criticality alert enrichment #171241

Conversation

oatkiller commented Nov 14, 2023 • edited by nkhristinin Loading

Summary

Design

Design Exploration

What if we want to filter by other criticalities?

Using keyword vs nested or flattened

KQL support

How to test

Checklist

Risk Matrix

For maintainers

nkhristinin commented Dec 1, 2023

nkhristinin commented Dec 4, 2023

nkhristinin commented Dec 6, 2023

nkhristinin commented Dec 12, 2023

nkhristinin commented Dec 12, 2023

nkhristinin commented Dec 13, 2023

e40pud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkhristinin commented Dec 19, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkhristinin commented Dec 21, 2023

nkhristinin commented Dec 21, 2023

kibana-ci commented Dec 21, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

maryam-saeidi left a comment

Choose a reason for hiding this comment

oatkiller commented Nov 14, 2023 •

edited by nkhristinin

Loading