[Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists #80592

FrankHassanabad · 2020-10-14T21:33:13Z

Summary

Fixes a bug where when you update your pre-packaged rules you could end up removing any existing exception lists the user might have already added. See: #80417

Fixes the merge logic so that any exception lists from pre-packaged rules will be additive if they do not already exist on the rule. User based exception lists will not be lost.
Added new backend integration tests for exception lists that did not exist before including ones that test the functionality of exception lists
Refactored some of the code in the get_rules_to_update.ts
Refactored some of the integration tests to use helper utils of countDownES, and countDownTest which simplify the retry logic within the integration tests
Added unit tests to exercise the bug and then the fix.
Added integration tests that fail this logic and then fixed the logic

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

…st scenarios

elasticmachine · 2020-10-14T23:35:38Z

Pinging @elastic/siem (Team:SIEM)

x-pack/plugins/security_solution/server/lib/detection_engine/rules/get_rules_to_update.test.ts

yctercero

LGreatTM! Thanks so much for all the added tests. I pulled down and tested loading prebuilt rules, adding an exception to one, incrementing it's version, updating it, and confirming that my exception was still there. 💯

Comments are super super nit.

x-pack/plugins/security_solution/server/lib/detection_engine/rules/get_rules_to_update.test.ts

yctercero · 2020-10-15T11:37:22Z

x-pack/plugins/security_solution/server/lib/detection_engine/rules/get_rules_to_update.ts

@@ -7,15 +7,67 @@
 import { AddPrepackagedRulesSchemaDecoded } from '../../../../common/detection_engine/schemas/request/add_prepackaged_rules_schema';
 import { RuleAlertType } from './types';

+/**


This is awesome, thanks for the added context.

yctercero · 2020-10-15T11:42:54Z

x-pack/plugins/security_solution/server/lib/detection_engine/rules/get_rules_to_update.test.ts

+      expect(update.exceptions_list).toEqual(ruleFromFileSystem1.exceptions_list);
+    });
+
+    test('should not remove an additional exception_list if an additional one was added by the end user on an immutable rule during an upgrade', () => {


Curious, is there a scenario where the rules have mismatching endpoint lists? I don't know exactly what the scenario would be, but curious as to how we might deal with that since right now we can only have one global endpoint list.

You cannot have a pre-packaged rule which overwrites an existing list or deletes a list only add a new list to the array. Talked to @spong and I think we need to have a way to associate/de-associate lists as well as delete them from a separate UI/UX such as a table very very soon.

yctercero · 2020-10-15T11:52:17Z

x-pack/plugins/security_solution/server/lib/detection_engine/rules/get_rules_to_update.ts

+    if (installedRule != null && installedRule.params.exceptionsList != null) {
+      const installedExceptionList = installedRule.params.exceptionsList;
+      const fileSystemExceptions = ruleFromFileSystem.exceptions_list.filter((potentialDuplicate) =>
+        installedExceptionList.every((item) => item.list_id !== potentialDuplicate.list_id)


I think this answers my question from earlier. So in the case of endpoint exception lists, it would flag it as duplicate because we control the endpoint exception list id and nothing should change for the user.

Yeah, I use list_id as what it should skip if it already exists on the list so we don't accidentally add duplicates. If we end up messing this part up and adding duplicates we are totally 🍞 (toast) as we don't have UI/UX to de-associate lists really as well as delete lists.

kibanamachine · 2020-10-15T19:08:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 946f5ae

Metrics [docs]

✅ unchanged

History

💚 Build #81767 succeeded 1cda07d
💚 Build #81762 succeeded 814821a
💚 Build #81755 succeeded c1a2678

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

… contain exception lists to not overwrite user defined lists (elastic#80592) ## Summary Fixes a bug where when you update your pre-packaged rules you could end up removing any existing exception lists the user might have already added. See: elastic#80417 * Fixes the merge logic so that any exception lists from pre-packaged rules will be additive if they do not already exist on the rule. User based exception lists will not be lost. * Added new backend integration tests for exception lists that did not exist before including ones that test the functionality of exception lists * Refactored some of the code in the `get_rules_to_update.ts` * Refactored some of the integration tests to use helper utils of `countDownES`, and `countDownTest` which simplify the retry logic within the integration tests * Added unit tests to exercise the bug and then the fix. * Added integration tests that fail this logic and then fixed the logic ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

… contain exception lists to not overwrite user defined lists (#80592) (#80734) ## Summary Fixes a bug where when you update your pre-packaged rules you could end up removing any existing exception lists the user might have already added. See: #80417 * Fixes the merge logic so that any exception lists from pre-packaged rules will be additive if they do not already exist on the rule. User based exception lists will not be lost. * Added new backend integration tests for exception lists that did not exist before including ones that test the functionality of exception lists * Refactored some of the code in the `get_rules_to_update.ts` * Refactored some of the integration tests to use helper utils of `countDownES`, and `countDownTest` which simplify the retry logic within the integration tests * Added unit tests to exercise the bug and then the fix. * Added integration tests that fail this logic and then fixed the logic ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

… contain exception lists to not overwrite user defined lists (#80592) (#80733) ## Summary Fixes a bug where when you update your pre-packaged rules you could end up removing any existing exception lists the user might have already added. See: #80417 * Fixes the merge logic so that any exception lists from pre-packaged rules will be additive if they do not already exist on the rule. User based exception lists will not be lost. * Added new backend integration tests for exception lists that did not exist before including ones that test the functionality of exception lists * Refactored some of the code in the `get_rules_to_update.ts` * Refactored some of the integration tests to use helper utils of `countDownES`, and `countDownTest` which simplify the retry logic within the integration tests * Added unit tests to exercise the bug and then the fix. * Added integration tests that fail this logic and then fixed the logic ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

* master: (115 commits) [ML] Transforms/DF Analytics: Fix data grid column sorting. (elastic#80618) added brace import to vis editor (elastic#80652) Fix error rate sorting in services list (elastic#80764) Emit info log when using custom registry URL (elastic#80768) [Reporting] Config Schema Validation for rules[N].protocol strings (elastic#80766) Add Storybook a11y addon (elastic#80069) Fix anomaly alert selection text (elastic#80746) [Security Solution] [Maps] Kibana index pattern, comma bug fix (elastic#80208) [kbn/optimizer] tweak split chunks options (elastic#80444) update template to use the new team label (elastic#80748) [Security Solution] Fix the Field dropdown in Timeline data providers resets when scrolled (elastic#80718) Adjusts observability alerting perms to require "all" (elastic#79896) [Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists (elastic#80592) [data.ui] Fix flaky test & lazy loading rendering artifacts. (elastic#80612) Licensed feature usage for connectors (elastic#77679) [Security Solution] Cypress template creation (elastic#80180) [APM] Hide service if only data is from ML (elastic#80145) Fix role mappings test for ESS (elastic#80604) [Maps] Add support for envelope (elastic#80614) [Security Solution] Update button text according to status (elastic#80389) ...

FrankHassanabad added 5 commits October 14, 2020 15:23

Fixes the bugs with unit and integration tests added for exception li…

01f5f55

…st scenarios

Added additional tests

c1a2678

Merge branch 'master' into fix-exception-merge-bug

2493832

Add back other tests from the security_and_spaces

814821a

Refactored part of the code and tests

1cda07d

FrankHassanabad self-assigned this Oct 14, 2020

FrankHassanabad added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.11.0 v7.10.0 Feature:Detection Rules Security Solution rules and Detection Engine Team:Detections and Resp Security Detection Response Team labels Oct 14, 2020

FrankHassanabad marked this pull request as ready for review October 14, 2020 23:35

FrankHassanabad requested review from a team as code owners October 14, 2020 23:35

FrankHassanabad added the Team:SIEM label Oct 14, 2020

yctercero reviewed Oct 15, 2020

View reviewed changes

x-pack/plugins/security_solution/server/lib/detection_engine/rules/get_rules_to_update.test.ts Outdated Show resolved Hide resolved

yctercero reviewed Oct 15, 2020

View reviewed changes

x-pack/plugins/security_solution/server/lib/detection_engine/rules/get_rules_to_update.test.ts Outdated Show resolved Hide resolved

yctercero reviewed Oct 15, 2020

View reviewed changes

x-pack/plugins/security_solution/server/lib/detection_engine/rules/get_rules_to_update.test.ts Outdated Show resolved Hide resolved

yctercero approved these changes Oct 15, 2020

View reviewed changes

FrankHassanabad changed the title ~~Fix exception merge bug~~ [Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists Oct 15, 2020

spong added the bug Fixes for quality problems that affect the customer experience label Oct 15, 2020

FrankHassanabad added 3 commits October 15, 2020 11:25

Merge branch 'master' into fix-exception-merge-bug

3faee3b

Fixed comments from PR review

981099c

Fixed from code review

946f5ae

FrankHassanabad merged commit e1aec17 into elastic:master Oct 15, 2020

FrankHassanabad deleted the fix-exception-merge-bug branch October 15, 2020 19:21

FrankHassanabad mentioned this pull request Oct 15, 2020

[7.x] [Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists (#80592) #80733

Merged

FrankHassanabad mentioned this pull request Oct 15, 2020

[7.10] [Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists (#80592) #80734

Merged

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Oct 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists #80592

[Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists #80592

FrankHassanabad commented Oct 14, 2020 •

edited

Loading

elasticmachine commented Oct 14, 2020

yctercero left a comment

yctercero Oct 15, 2020

yctercero Oct 15, 2020

FrankHassanabad Oct 15, 2020

yctercero Oct 15, 2020

FrankHassanabad Oct 15, 2020

kibanamachine commented Oct 15, 2020

[Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists #80592

[Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists #80592

Conversation

FrankHassanabad commented Oct 14, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Oct 14, 2020

yctercero left a comment

Choose a reason for hiding this comment

yctercero Oct 15, 2020

Choose a reason for hiding this comment

yctercero Oct 15, 2020

Choose a reason for hiding this comment

FrankHassanabad Oct 15, 2020

Choose a reason for hiding this comment

yctercero Oct 15, 2020

Choose a reason for hiding this comment

FrankHassanabad Oct 15, 2020

Choose a reason for hiding this comment

kibanamachine commented Oct 15, 2020

💚 Build Succeeded

Metrics [docs]

History

FrankHassanabad commented Oct 14, 2020 •

edited

Loading