[Security Solution][Detections][7.12] Critical Threshold Rule Fixes #92667

madirey · 2021-02-24T17:34:13Z

Summary

This PR addresses critical issues identified in https://github.com/elastic/security-team/issues/839

Tighten up the schema for threshold rules (https://github.com/elastic/kibana/pull/90826/files#diff-8e7f5246a8edf50094b3ff0330e97e46b6dd3f762b37155a78c7272e0ee63159R472-R476)
Reenable Cypress test (https://github.com/elastic/kibana/pull/90826/files#diff-2b32187e91fc5fe185dd2beb2465db870f4c0bd11086b984b47da76b5f91cd62R84)
Rule query is not generated properly if cardinality is supplied without a terms aggregation (https://github.com/elastic/kibana/pull/90826/files#diff-d786a549ed9c6b30ca9607f5371487239bc483ffd898e02df76f622c7947364eR89)
Fixes bug that caused upgrading from 7.11 threshold rules to fail

Additionally, it adds several new unit tests to cover these cases.

We wanted to avoid a rule migration, so we've introduced a new type ThresholdNormalized which will be used for all internal calls from the detection engine. The threshold field will remain stored as a string for legacy threshold rules, but will be an array for all rules created through the UI. The cardinality field has been normalized to be an array to avoid a future migration when we support multiple cardinality fields.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
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

…2.-fixes

elasticmachine · 2021-02-28T02:04:17Z

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

elasticmachine · 2021-02-28T02:04:18Z

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

…2.-fixes

x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts

...plugins/security_solution/common/search_strategy/security_solution/matrix_histogram/index.ts

x-pack/plugins/security_solution/public/common/components/matrix_histogram/types.ts

marshallmain

great improvements overall, just a couple more questions

x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts

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

marshallmain · 2021-03-01T18:48:39Z

...lugins/security_solution/server/lib/detection_engine/signals/threshold_get_bucket_filters.ts

- .field,
+ field: (((hit._source.signal?.rule as RulesSchema).threshold as unknown) as {
+ field: string;
+ }).field,


might be easier to use lodash get here and then check if we got the value successfully rather than triple casting

You don't like my triple cast? 😂

Propose we leave this for now and I can update in a follow-up... just want to focus on getting the fixes in for BC3.

…2.-fixes

marshallmain

LGTM. I was able to create a threshold rules that use cardinality with and without Group by fields.

One more thing we may want to address in a follow up is some weirdness around how we exclude documents that were part of previous threshold alerts. When building the filters, we sort of mix the current state of the rule with the alerts that the rule already generated (e.g. here we use bucketByFields from the current state of the rule, but also check if the current fields are in the existing alerts). This can create some duplicate alerts if the rule is edited. I think we want to rely only on the alert documents to create the filters so that the filters will be consistent even if the rule is edited.

marshallmain · 2021-03-01T21:33:15Z

...ugins/security_solution/server/lib/detection_engine/signals/bulk_create_threshold_signals.ts

@@ -146,13 +107,11 @@ const getTransformedHits = (
 field,
 value: bucket.key,
 },
- ],
- cardinality: !isEmpty(threshold.cardinality_field)
+ ].filter((term) => term.field != null),


for follow up: field is typed as string in the function definition but used as string | null - looks like aggParts is being typed as any so we're losing type checking there

madirey · 2021-03-01T21:56:30Z

Thanks @marshallmain ... the bucketByFields fix is on the list, but didn't make this PR. Want to get this merged to address the upgrade issue and then I'll have yet another PR with the other fixes. :D

marshallmain · 2021-03-01T22:05:05Z

@madirey sounds good!

kibanamachine · 2021-03-01T22:07:06Z

💚 Build Succeeded

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.8MB	7.8MB	+1.2KB
`triggersActionsUi`	1.6MB	1.5MB	-23.9KB
total			-22.7KB

Page load bundle

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

id	before	after	diff
`securitySolution`	235.8KB	236.5KB	+766.0B
`triggersActionsUi`	104.0KB	104.1KB	+82.0B
total			+848.0B

Unknown metric groups

async chunk count

id	before	after	diff
`triggersActionsUi`	41	42	+1

History

💚 Build #110054 succeeded a8a8654
💛 Build #109788 was flaky d6dc1e2
💔 Build #109787 failed 493411d
💔 Build #109783 failed 26e05b0
💔 Build #109780 failed 8726a4d

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

…lastic#92667) * Threshold cardinality validation * Remove comments * Fix legacy threshold signal dupe mitigation * Add find_threshold_signals tests * remove comment * bug fixes * Fix edit form value initialization for cardinality_value * Fix test * Type and test fixes * Tests/types * Reenable threshold cypress test * Schema fixes * Types and tests, normalize threshold field util * Continue cleaning up types * Some more pre-7.12 tests * Limit cardinality_field to length 1 for now * Cardinality to array * Cardinality to array * Tests/types * cardinality can be null * Handle empty threshold field in bulk_create_threshold_signals * Remove cardinality_field, cardinality_value

…92667) (#93140) * Threshold cardinality validation * Remove comments * Fix legacy threshold signal dupe mitigation * Add find_threshold_signals tests * remove comment * bug fixes * Fix edit form value initialization for cardinality_value * Fix test * Type and test fixes * Tests/types * Reenable threshold cypress test * Schema fixes * Types and tests, normalize threshold field util * Continue cleaning up types * Some more pre-7.12 tests * Limit cardinality_field to length 1 for now * Cardinality to array * Cardinality to array * Tests/types * cardinality can be null * Handle empty threshold field in bulk_create_threshold_signals * Remove cardinality_field, cardinality_value

…92667) (#93141) * Threshold cardinality validation * Remove comments * Fix legacy threshold signal dupe mitigation * Add find_threshold_signals tests * remove comment * bug fixes * Fix edit form value initialization for cardinality_value * Fix test * Type and test fixes * Tests/types * Reenable threshold cypress test * Schema fixes * Types and tests, normalize threshold field util * Continue cleaning up types * Some more pre-7.12 tests * Limit cardinality_field to length 1 for now * Cardinality to array * Cardinality to array * Tests/types * cardinality can be null * Handle empty threshold field in bulk_create_threshold_signals * Remove cardinality_field, cardinality_value Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (42 commits) [Lens] Introduces new chart switcher (elastic#91844) [Lens] fix selection when dragging (elastic#93034) Converts usage collection README to .mdx (elastic#92982) Fix expanding document when using saved search data grid (elastic#92999) [SECURITY SOLUTIONS] Bug case connector (elastic#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942) [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139) [DOCS] Fixes links for machine learning alerts (elastic#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667) Bump ems landing page to 7.12 (elastic#93065) [App Search] Implement various Relevance Tuning states and form actions (elastic#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092) Hide instances latency distribution chart (elastic#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087) [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (elastic#92932) ...

… playwright-ftr-e2e * 'playwright-ftr-e2e' of github.com:shahzad31/kibana: (38 commits) [chore] Enable core's eslint rule: `@ts-expect-error` (elastic#93086) [Lens] Introduces new chart switcher (elastic#91844) [Lens] fix selection when dragging (elastic#93034) Converts usage collection README to .mdx (elastic#92982) Fix expanding document when using saved search data grid (elastic#92999) [SECURITY SOLUTIONS] Bug case connector (elastic#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942) [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139) [DOCS] Fixes links for machine learning alerts (elastic#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667) Bump ems landing page to 7.12 (elastic#93065) [App Search] Implement various Relevance Tuning states and form actions (elastic#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092) Hide instances latency distribution chart (elastic#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087) ...

… ilm/rollup-v2-action * 'ilm/rollup-v2-action' of github.com:elastic/kibana: (30 commits) Fix expanding document when using saved search data grid (#92999) [SECURITY SOLUTIONS] Bug case connector (#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (#92942) [APM] Fix hidden search bar in error pages while loading (#84476) (#93139) [DOCS] Fixes links for machine learning alerts (#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (#92667) Bump ems landing page to 7.12 (#93065) [App Search] Implement various Relevance Tuning states and form actions (#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (#93092) Hide instances latency distribution chart (#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (#93087) [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (#92932) [Security Solution][Detecttions] Indicator enrichment tweaks (#92989) [Maps] fix fit to data on heatmap not working (#92697) [Security Solution][Endpoint][Admin] Fixes policy sticky footer save test (#92919) ...

madirey added 9 commits February 23, 2021 16:36

Threshold cardinality validation

f66ac13

Remove comments

dbc1c20

Fix legacy threshold signal dupe mitigation

3188382

Merge branch 'master' of github.com:elastic/kibana into threshold-7.1…

21708cf

…2.-fixes

Add find_threshold_signals tests

f697baa

remove comment

8a53bf3

bug fixes

66d6d7e

Fix edit form value initialization for cardinality_value

c699fbb

Fix test

2a86995

madirey added 4 commits February 25, 2021 01:51

Type and test fixes

ea53ab1

Tests/types

ed9efaa

Reenable threshold cypress test

6357718

Schema fixes

3aca5d6

madirey marked this pull request as ready for review February 28, 2021 02:04

madirey requested a review from a team as a code owner February 28, 2021 02:04

madirey added 5 commits February 28, 2021 14:25

Types and tests, normalize threshold field util

748066d

Merge branch 'master' of github.com:elastic/kibana into threshold-7.1…

8c1187e

…2.-fixes

Continue cleaning up types

078e35f

Some more pre-7.12 tests

3e8d423

Limit cardinality_field to length 1 for now

8726a4d

marshallmain reviewed Feb 28, 2021

View reviewed changes

x-pack/plugins/security_solution/common/detection_engine/schemas/common/schemas.ts Outdated Show resolved Hide resolved

Cardinality to array

26e05b0

madirey commented Feb 28, 2021

View reviewed changes

...plugins/security_solution/common/search_strategy/security_solution/matrix_histogram/index.ts Show resolved Hide resolved

madirey commented Feb 28, 2021

View reviewed changes

x-pack/plugins/security_solution/public/common/components/matrix_histogram/types.ts Outdated Show resolved Hide resolved

madirey added 3 commits March 1, 2021 02:05

Cardinality to array

0807755

Tests/types

493411d

cardinality can be null

d6dc1e2

marshallmain reviewed Mar 1, 2021

View reviewed changes

madirey added 3 commits March 1, 2021 19:44

Handle empty threshold field in bulk_create_threshold_signals

feb0cd2

Merge branch 'master' of github.com:elastic/kibana into threshold-7.1…

a8a8654

…2.-fixes

Remove cardinality_field, cardinality_value

478b7af

marshallmain approved these changes Mar 1, 2021

View reviewed changes

madirey merged commit cb053f4 into elastic:master Mar 1, 2021

This was referenced Mar 1, 2021

[7.x] [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (#92667) #93140

Merged

[7.12] [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (#92667) #93141

Merged

spong mentioned this pull request Mar 3, 2021

[Security Solution][Detections] Pre-packaged Threshold Rules schema out of date resulting in errors on Rule Management page #93357

Closed

This was referenced Mar 4, 2021

Update schema for threshold rule type for 7.12 elastic/detection-rules#975

Closed

[FR] New and tuned rules leveraging new threshold rule features elastic/detection-rules#977

Closed

madirey deleted the threshold-7.12.-fixes branch March 7, 2021 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections][7.12] Critical Threshold Rule Fixes #92667

[Security Solution][Detections][7.12] Critical Threshold Rule Fixes #92667

madirey commented Feb 24, 2021 •

edited

Loading

elasticmachine commented Feb 28, 2021

elasticmachine commented Feb 28, 2021

marshallmain left a comment

marshallmain Mar 1, 2021

madirey Mar 1, 2021

madirey Mar 1, 2021

marshallmain left a comment •

edited

Loading

marshallmain Mar 1, 2021

madirey commented Mar 1, 2021

marshallmain commented Mar 1, 2021

kibanamachine commented Mar 1, 2021

async chunk count

[Security Solution][Detections][7.12] Critical Threshold Rule Fixes #92667

[Security Solution][Detections][7.12] Critical Threshold Rule Fixes #92667

Conversation

madirey commented Feb 24, 2021 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 28, 2021

elasticmachine commented Feb 28, 2021

marshallmain left a comment

Choose a reason for hiding this comment

marshallmain Mar 1, 2021

Choose a reason for hiding this comment

madirey Mar 1, 2021

Choose a reason for hiding this comment

madirey Mar 1, 2021

Choose a reason for hiding this comment

marshallmain left a comment • edited Loading

Choose a reason for hiding this comment

marshallmain Mar 1, 2021

Choose a reason for hiding this comment

madirey commented Mar 1, 2021

marshallmain commented Mar 1, 2021

kibanamachine commented Mar 1, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

async chunk count

History

madirey commented Feb 24, 2021 •

edited

Loading

marshallmain left a comment •

edited

Loading