[RAC][Security Solution] Pull Gap Remediation out of search_after_bulk_create #102104

madirey · 2021-06-14T17:26:40Z

Summary

Fixes #100181 (aside from Threat Match per-timerange bug where maxSignals can be exceeded)

~~NOTE: This branch includes commits from #101544, which will be removed once that PR is merged.~~

In order to ensure that maxSignals is enforced per time-range tuple, avoiding the potential silencing of signals when gap remediation is used, this PR modifies searchAfterAndBulkCreate so that it accepts only a single time-range tuple. Accordingly, the logic in signal_rule_alert_type was modified to loop over the tuples for each rule type and invoke N instances of the executor for N time ranges. This enforces maxSignals for each time range, with the exception of Threat Match rule invocations, which can still generate maxSignals * M signals per time range, where M is the number of parallel searches performed.

~~To address this, there is an optional commit that adds some synchronization code to searchAfterAndBulkCreate: 81105cf~~ (removed in favor of a better solution, to come in a future PR if prioritized) @MikePaquette Is this something we want to fix for 7.14?

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

…executions" This reverts commit ba3b2f7.

…iation-2

kibanamachine · 2021-06-15T19:02:55Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

marshallmain

LGTM, this is an exciting improvement - thanks!

…k_create (elastic#102104) * Modify threshold rules to receive a single date range tuple * Modify threat match rules to receive a single date range tuple * Modify custom query rules to receive a single date range tuple * Fix up tests (partially) * Change log message to indicate single tuple instead of array * Bad test? * Prevent max_signals from being exceeded on threat match rule executions * Revert "Prevent max_signals from being exceeded on threat match rule executions" This reverts commit ba3b2f7. * Modify EQL rules to use date range tuple * Modify ML rules to use date range tuple * Fix ML/EQL tests * Use dateMath to parse moments in ML/Threshold tests * Add mocks for threshold test * Use dateMath for eql tests

kibanamachine · 2021-06-18T15:38:17Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 102104 or prevent reminders by adding the backport:skip label.

…k_create (elastic#102104) * Modify threshold rules to receive a single date range tuple * Modify threat match rules to receive a single date range tuple * Modify custom query rules to receive a single date range tuple * Fix up tests (partially) * Change log message to indicate single tuple instead of array * Bad test? * Prevent max_signals from being exceeded on threat match rule executions * Revert "Prevent max_signals from being exceeded on threat match rule executions" This reverts commit ba3b2f7. * Modify EQL rules to use date range tuple * Modify ML rules to use date range tuple * Fix ML/EQL tests * Use dateMath to parse moments in ML/Threshold tests * Add mocks for threshold test * Use dateMath for eql tests

…k_create (#102104) (#102739) * Modify threshold rules to receive a single date range tuple * Modify threat match rules to receive a single date range tuple * Modify custom query rules to receive a single date range tuple * Fix up tests (partially) * Change log message to indicate single tuple instead of array * Bad test? * Prevent max_signals from being exceeded on threat match rule executions * Revert "Prevent max_signals from being exceeded on threat match rule executions" This reverts commit ba3b2f7. * Modify EQL rules to use date range tuple * Modify ML rules to use date range tuple * Fix ML/EQL tests * Use dateMath to parse moments in ML/Threshold tests * Add mocks for threshold test * Use dateMath for eql tests

FrankHassanabad · 2021-06-21T19:01:37Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/executors/eql.test.ts

    },
    references: [],
  };
+  const tuple = {
+    from: dateMath.parse(params.from)!,
+    to: dateMath.parse(params.to)!,


I know this is merged but instead of turning off the checks here can we do a small follow up where we do this similar pattern here that @marshallmain did a while back?

https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts#L468

I think that would be good to intentionally throw if for some reason these aren't parseable rather than turning off the typescript check for it.

Later if refactoring or mistakes are made and we get an SDH or issue it would be easier to track down where and what happened from a custom error message than somewhere else where the from and to have become null/undefined.

@FrankHassanabad Is this necessary in test files? Looks like we're only doing this in the test files... the code/test will fail when the check is done here, right? https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/server/lib/detection_engine/signals/search_after_bulk_create.ts#L52-L58

Oh I didn't see it was in a test file. For test files it's optional, I typically still avoid it if I can even in test files, but that's just me probably because I really don't like that TypeScript allows type assertions to be turned off compared to other languages with strict types.

madirey added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 Theme: rac label obsolete labels Jun 14, 2021

madirey requested review from ecezalp and marshallmain June 14, 2021 17:26

madirey force-pushed the rac-gap-remediation branch from 81105cf to ba3b2f7 Compare June 14, 2021 17:48

madirey added 8 commits June 14, 2021 16:46

Modify threshold rules to receive a single date range tuple

c61395b

Modify threat match rules to receive a single date range tuple

74ac5eb

Modify custom query rules to receive a single date range tuple

1c0b81d

Fix up tests (partially)

0578cc5

Change log message to indicate single tuple instead of array

d147330

Bad test?

1bd5eea

Prevent max_signals from being exceeded on threat match rule executions

b301793

Revert "Prevent max_signals from being exceeded on threat match rule …

4752688

…executions" This reverts commit ba3b2f7.

madirey force-pushed the rac-gap-remediation branch from ba3b2f7 to 4752688 Compare June 14, 2021 20:52

madirey added 3 commits June 14, 2021 16:56

Modify EQL rules to use date range tuple

5a0739a

Modify ML rules to use date range tuple

1038103

Fix ML/EQL tests

2f9ac90

madirey marked this pull request as ready for review June 14, 2021 21:09

madirey requested a review from a team as a code owner June 14, 2021 21:09

madirey changed the title ~~[RAC][Security Solution] Pull Gap Remediation out of search_after_bulk_create and update Threat Match maxSignals calculation~~ [RAC][Security Solution] Pull Gap Remediation out of search_after_bulk_create Jun 14, 2021

madirey added 4 commits June 15, 2021 12:39

Use dateMath to parse moments in ML/Threshold tests

956963e

Add mocks for threshold test

feba6b5

Merge branch 'master' of github.com:elastic/kibana into rac-gap-remed…

1a3bcf1

…iation-2

Use dateMath for eql tests

e3f8f8d

marshallmain approved these changes Jun 15, 2021

View reviewed changes

madirey merged commit c5e74d8 into elastic:master Jun 16, 2021

madirey deleted the rac-gap-remediation branch June 16, 2021 15:35

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 18, 2021

madirey mentioned this pull request Jun 21, 2021

[7.x] [RAC][Security Solution] Pull Gap Remediation out of search_after_bulk_create (#102104) #102739

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 21, 2021

FrankHassanabad reviewed Jun 21, 2021

View reviewed changes

ecezalp mentioned this pull request Jul 12, 2021

[Security Solution][Detections] Inconsistent handling of gap detection and max signals #100181

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RAC][Security Solution] Pull Gap Remediation out of search_after_bulk_create #102104

[RAC][Security Solution] Pull Gap Remediation out of search_after_bulk_create #102104

madirey commented Jun 14, 2021 •

edited

Loading

kibanamachine commented Jun 15, 2021

marshallmain left a comment

kibanamachine commented Jun 18, 2021

FrankHassanabad Jun 21, 2021 •

edited

Loading

madirey Jun 22, 2021

FrankHassanabad Jun 24, 2021

[RAC][Security Solution] Pull Gap Remediation out of search_after_bulk_create #102104

[RAC][Security Solution] Pull Gap Remediation out of search_after_bulk_create #102104

Conversation

madirey commented Jun 14, 2021 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

kibanamachine commented Jun 15, 2021

💚 Build Succeeded

Metrics [docs]

History

marshallmain left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 18, 2021

FrankHassanabad Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

madirey Jun 22, 2021

Choose a reason for hiding this comment

FrankHassanabad Jun 24, 2021

Choose a reason for hiding this comment

madirey commented Jun 14, 2021 •

edited

Loading

FrankHassanabad Jun 21, 2021 •

edited

Loading