[Security_solution][Detections] Refactor signal ancestry to allow multiple parents by marshallmain · Pull Request #76531 · elastic/kibana

marshallmain · 2020-09-02T16:01:04Z

Summary

Remaining issues:

On existing .signals mappings there will be no filtering/querying on signal.parents or signal.depth. We would have to ask the user to create a new space for full sequence functionality

TODO: Force mapping update so existing signal indices will add the new fields (signal.depth, signal.parents, signal.ancestors.index) to the mapping

This PR changes the way that signal.parent.rule and signal.ancestors.rule are populated, as well as signal.parent.depth and signal.ancestors.depth. They will contain the rule and depth of the parent event rather than the current event as they do now. It also deprecates signal.parent in favor of signal.parents which is expected to be an array of parent events/signals. It also refactors buildSignal to make it easy to build a signal from one or multiple events.

signal.parent currently contains a mix of information from the current signal and the parent signal/event - signal.parent.rule contains the current rule id (not the parent rule id), but the other fields such as signal.parent.id are from the parent signal/event.

Existing signal structure chart

New signal structure chart WITHOUT sequence based rule

New signal structure chart WITH sequence based rule (and using the signal above as an input)

Post 7.10 signals that are built on pre-7.10 signals will have some duplication in signal.ancestors.rule and signal.ancestors.depth. If a post-7.10 signal is built on a pre-7.10 chain of signals, then signal.ancestors will be

[
  {rule: 1, type: event, id: 0, depth: 1},
  {rule: 2, type: signal, id: 1, depth: 2},
  {rule: 3, type: signal, id: 2, depth: 3},
  {rule: 3, type: signal, id: 3, depth: 3}, -> first ancestors entry added by a post-7.10 signal
  {rule: 4, type: signal, id: 4, depth: 4} -> future signals increment depth correctly
]

The rule de-duplication logic that prevents a rule from triggering on a signal with that same rule in its ancestry still works in this case since no data is missing.

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

marshallmain · 2020-09-02T16:02:21Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/index/signals_mapping.json

              "rule": {
                "type": "keyword"
              },
+              "index": {


index was in parent but not here

marshallmain · 2020-09-02T16:05:30Z

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

  });
-  const signal = buildSignal(doc, rule);
+  const signal: Signal = {
+    ...buildSignal([doc], rule),


buildSignal now expects an array of documents and handles building the rule, parents, and ancestors. additionalSignalFields takes a single document and creates the fields for the special case where we only have 1 event triggering the signal (so we can copy the fields from the event into the signal). When we add signals based on EQL sequences we won't be able to copy every event from the sequence into the signal, instead relying on signal.parents to contain references to the sub-events.

marshallmain · 2020-09-02T16:07:02Z

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

    expect(rule).toEqual(expected);
  });
+
+  test('it builds a rule and removes internal tags', () => {


removeInternalTagsFromRule now happens inside buildRule so 1 additional test here.

marshallmain · 2020-09-02T16:08:24Z

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

+    };
+    expect(rule).toEqual(expected);
+  });
+


Next 4 tests are simply moved from build_signal.test.ts since removeInternalTagsFromRule was moved from build_signal.ts to build_rule.ts

marshallmain · 2020-09-02T16:09:03Z

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

+  return removeInternalTagsFromRule(rule);
+};
+
+export const removeInternalTagsFromRule = (rule: Partial<RulesSchema>): Partial<RulesSchema> => {


Cut/paste from build_signal.ts since this is rule-related code.

marshallmain · 2020-09-02T16:10:36Z

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

-    expect(signal).toEqual(expected);
-  });
-
-  test('it builds a signal as expected with original_event if is present and without internal tags in them', () => {


This test was deleted - buildSignal is no longer responsible for removing internal tags. Test is replaced with the similar test above in build_rule.test.ts

marshallmain · 2020-09-02T16:15:03Z

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

-    expect(signal).toEqual(expected);
-  });
-
-  test('it builds a ancestor correctly if the parent does exist without internal tags in them', () => {


Deleted this test - buildAncestor no longer needs the rule, so it doesn't need to test a rule with internal tags in it.

marshallmain · 2020-09-02T16:15:40Z

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

    expect(signal).toEqual(expected);
  });
-
-  test('it removes internal tags from a typical rule', () => {


Next 4 tests moved to build_rule.test.ts

marshallmain · 2020-09-02T16:16:59Z

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

+  if (doc._source.signal != null) {
    return {
-      rule: rule.id != null ? rule.id : '',
+      rule: doc._source.signal.rule.id,


Semantics change: signal.parents.rule now contains the parent rule rather than current signal rule.

marshallmain · 2020-09-02T16:17:35Z

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

    };
  } else {
    return {
-      rule: rule.id != null ? rule.id : '',


If the parent doc is not a signal, then there is no signal.parents.rule

marshallmain · 2020-09-02T16:20:19Z

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

-      return !doc._source.signal.ancestors.some((ancestor) => ancestor.rule === ruleId);
+      return !(
+        doc._source.signal.ancestors.some((ancestor) => ancestor.rule === ruleId) ||
+        doc._source.signal.rule.id === ruleId


Since signal.ancestors no longer contains the rule id of the current signal we need to check signal.rule.id as well as the ancestors.

marshallmain · 2020-09-02T16:21:32Z

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

-    parent: Ancestor;
+    // parent is deprecated: new signals should populate parents instead
+    // both are optional until all signals with parent are gone and we can safely remove it
+    parent?: Ancestor;


SignalSource represents the signal results coming from ES - we need to support pre-7.10 signals which may still have parent or may have parents.

elasticmachine · 2020-09-02T20:24:25Z

Pinging @elastic/endpoint-response (Team:Endpoint Response)

marshallmain · 2020-09-02T20:24:53Z

@elasticmachine merge upstream

madirey · 2020-09-03T16:40:49Z

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

+ * creating an array of N+1 ancestors.
+ * @param doc The parent signal/event for which to extend the ancestry.
+ */
+export const buildAncestorsSignal = (doc: SignalSourceHit): Signal['ancestors'] => {


Can we rename this to buildAncestors? For parity with buildParent, and since this isn't actually building a signal...

madirey

Let's get confirmation that the semantics changes are something we can get away with (for existing API users), but otherwise LGTM. Would also like to get that function renamed.

marshallmain · 2020-09-04T22:43:59Z

@elasticmachine merge upstream

yctercero · 2020-09-08T14:54:03Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/index/signals_mapping.json

          },
+          "parents": {
+            "properties": {
+              "rule": {


This is the rule.id, not rule.rule_id right? Curious if there's any upside to using one over the other. Like if users often import/export rules, would we lose tracing by using the id?

Right, this is rule.id. I think for the parents/ancestry we want to know which specific instance of a rule generated the alert so id is a good choice.

We could consider making signal.parent.rule an object where we could have id, rule_id, and any other rule fields that could be useful in signal.parent.rule.*, but that would be a breaking change with the existing signal.parent and signal.ancestors so I think we'd need a migration.

yctercero · 2020-09-08T15:10:25Z

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

-      depth: existingSignal.depth + 1,
+      // We first look for signal.depth and use that if it exists. If it doesn't exist, this should be a pre-7.10 signal
+      // and should have signal.parent.depth instead. signal.parent.depth in this case is treated as equivalent to signal.depth.
+      depth: doc._source.signal.depth ?? doc._source.signal?.parent?.depth ?? 1,


Why is the ? needed after doc._source.signal - seems confusing since it was checked on line 16. Should I expect that value to exist in this block of code?

Good point, it's not needed - I'll remove it.

yctercero · 2020-09-08T15:13:11Z

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

+ * creating an array of N+1 ancestors.
+ * @param doc The parent signal/event for which to extend the ancestry.
+ */
+export const buildAncestors = (doc: SignalSourceHit): Signal['ancestors'] => {


Nit: Are Signal['ancestors'] and Ancestor[] equivalent? If they are, should we use Ancestor[] for consistency, just so as a dev quickly looking through here it's clear buildParent and buildAncestors are returning the same type just one is array and the other isn't?

marshallmain · 2020-09-08T17:53:44Z

@elasticmachine merge upstream

kibanamachine · 2020-09-08T19:44:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 203c363

Build metrics

✅ unchanged

History

💚 Build #72782 succeeded 166f491
💚 Build #72704 succeeded 9143432
💚 Build #72688 succeeded 93094ab
💛 Build #72417 was flaky 2c9d038
💚 Build #72042 succeeded d818f21

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

dhurley14

LGTM. Created a rule (rule-1) which produced some signals on master, then checked out this branch and ran that rule + another rule (rule-2) to populate the "parents" field on signals generated by rule-2. Did not see an issue with migrations or anything on the signals table breaking. Looks good!

* master: (25 commits) [bugfix] Replace panel flyout opens 2 flyouts (elastic#76931) clean up test (elastic#76887) [Enterprise Search] Update shared API request handler (elastic#77112) [Maps] convert ESAggSource to TS (elastic#76999) Add plugin status API - take 2 (elastic#76732) Adds lens as a readable saved object for read-only dashboard users (elastic#77067) Skip checking for the reserved realm (elastic#76687) Reporting/diagnostics (elastic#74314) Reporting/Test: unskip non-screenshot tests (elastic#77088) Move metrics to setup and add cgroup metrics (elastic#76730) [Enterprise Search] Add Overview landing page/plugin (elastic#76734) First pass. Change TS type. Update OpenAPI (elastic#76434) [CI] Balance xpack ci groups a bit (elastic#77068) [Security_solution][Detections] Refactor signal ancestry to allow multiple parents (elastic#76531) [Maps] convert MetricsEditor to TS (elastic#76727) IndexMigrator: fix non blocking migration wrapper promise rejection (elastic#77018) [Enterprise Search] Update config data endpoint to v2 (elastic#76970) [ML] Add decision path charts to exploration results table (elastic#73561) Bump eventemitter3 from 4.0.0 to 4.0.7 (elastic#77016) [Ingest Pipelines] Add descriptions for ingest processors K-S (elastic#76981) ...

* master: (41 commits) [bugfix] Replace panel flyout opens 2 flyouts (elastic#76931) clean up test (elastic#76887) [Enterprise Search] Update shared API request handler (elastic#77112) [Maps] convert ESAggSource to TS (elastic#76999) Add plugin status API - take 2 (elastic#76732) Adds lens as a readable saved object for read-only dashboard users (elastic#77067) Skip checking for the reserved realm (elastic#76687) Reporting/diagnostics (elastic#74314) Reporting/Test: unskip non-screenshot tests (elastic#77088) Move metrics to setup and add cgroup metrics (elastic#76730) [Enterprise Search] Add Overview landing page/plugin (elastic#76734) First pass. Change TS type. Update OpenAPI (elastic#76434) [CI] Balance xpack ci groups a bit (elastic#77068) [Security_solution][Detections] Refactor signal ancestry to allow multiple parents (elastic#76531) [Maps] convert MetricsEditor to TS (elastic#76727) IndexMigrator: fix non blocking migration wrapper promise rejection (elastic#77018) [Enterprise Search] Update config data endpoint to v2 (elastic#76970) [ML] Add decision path charts to exploration results table (elastic#73561) Bump eventemitter3 from 4.0.0 to 4.0.7 (elastic#77016) [Ingest Pipelines] Add descriptions for ingest processors K-S (elastic#76981) ...

…rok/new-patterns-component-use-array * 'master' of github.com:elastic/kibana: (39 commits) [APM] Always load esarchives from common (elastic#77139) [Ingest Manager] Handle Legacy ES client errors (elastic#76865) [Docs] URL Drilldown (elastic#76529) [bugfix] Replace panel flyout opens 2 flyouts (elastic#76931) clean up test (elastic#76887) [Enterprise Search] Update shared API request handler (elastic#77112) [Maps] convert ESAggSource to TS (elastic#76999) Add plugin status API - take 2 (elastic#76732) Adds lens as a readable saved object for read-only dashboard users (elastic#77067) Skip checking for the reserved realm (elastic#76687) Reporting/diagnostics (elastic#74314) Reporting/Test: unskip non-screenshot tests (elastic#77088) Move metrics to setup and add cgroup metrics (elastic#76730) [Enterprise Search] Add Overview landing page/plugin (elastic#76734) First pass. Change TS type. Update OpenAPI (elastic#76434) [CI] Balance xpack ci groups a bit (elastic#77068) [Security_solution][Detections] Refactor signal ancestry to allow multiple parents (elastic#76531) [Maps] convert MetricsEditor to TS (elastic#76727) IndexMigrator: fix non blocking migration wrapper promise rejection (elastic#77018) [Enterprise Search] Update config data endpoint to v2 (elastic#76970) ... # Conflicts: # src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_array.ts

* master: (38 commits) Reporting/Test: unskip non-screenshot tests (elastic#77088) Move metrics to setup and add cgroup metrics (elastic#76730) [Enterprise Search] Add Overview landing page/plugin (elastic#76734) First pass. Change TS type. Update OpenAPI (elastic#76434) [CI] Balance xpack ci groups a bit (elastic#77068) [Security_solution][Detections] Refactor signal ancestry to allow multiple parents (elastic#76531) [Maps] convert MetricsEditor to TS (elastic#76727) IndexMigrator: fix non blocking migration wrapper promise rejection (elastic#77018) [Enterprise Search] Update config data endpoint to v2 (elastic#76970) [ML] Add decision path charts to exploration results table (elastic#73561) Bump eventemitter3 from 4.0.0 to 4.0.7 (elastic#77016) [Ingest Pipelines] Add descriptions for ingest processors K-S (elastic#76981) [Metrics UI] Replace Snapshot API with Metrics API (elastic#76253) legacy utils cleanup (elastic#76608) [ML] Account for "properties" layer in find_file_structure mappings (elastic#77035) fixed typo Upgrade to Kea 2.2 (elastic#77047) a11y tests on spaces home page including feature control (elastic#76515) [ML] Transforms list: persist pagination through refresh interval (elastic#76786) [ML] Replace all use of date_histogram interval with fixed_interval (elastic#76876) ...

kibanamachine · 2020-09-11T18:08:44Z

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

…tiple parents (elastic#76531) * Refactors signal ancestry to allow multiple parents * Fix depth calculation for 7.10+ signals on pre-7.10 signals * Comment build_signal functions * Rename buildAncestorsSignal to buildAncestors * Update detection engine depth test scripts and docs * Update halting test readme * Match up rule ids in readme * Continue populating signal.parent along with signal.parents * pr comments Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…tiple parents (#76531) (#77346) * Refactors signal ancestry to allow multiple parents * Fix depth calculation for 7.10+ signals on pre-7.10 signals * Comment build_signal functions * Rename buildAncestorsSignal to buildAncestors * Update detection engine depth test scripts and docs * Update halting test readme * Match up rule ids in readme * Continue populating signal.parent along with signal.parents * pr comments Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Refactors signal ancestry to allow multiple parents

39a5f49

marshallmain commented Sep 2, 2020

View reviewed changes

marshallmain added 2 commits September 2, 2020 16:03

Fix depth calculation for 7.10+ signals on pre-7.10 signals

de9debb

Comment build_signal functions

a3e1559

marshallmain added Feature:Detection Rules Security Solution rules and Detection Engine release_note:fix Team:Endpoint Response Endpoint Response Team v7.10.0 v8.0.0 labels Sep 2, 2020

marshallmain marked this pull request as ready for review September 2, 2020 20:24

marshallmain requested review from a team as code owners September 2, 2020 20:24

Merge branch 'master' into refactor-ancestry

d818f21

madirey reviewed Sep 3, 2020

View reviewed changes

madirey approved these changes Sep 3, 2020

View reviewed changes

Rename buildAncestorsSignal to buildAncestors

2c9d038

marshallmain added the release_note:breaking label Sep 4, 2020

marshallmain added 4 commits September 4, 2020 12:48

Update detection engine depth test scripts and docs

93094ab

Update halting test readme

08a2a4d

Match up rule ids in readme

9143432

Continue populating signal.parent along with signal.parents

f3b294e

Merge branch 'master' into refactor-ancestry

166f491

marshallmain removed the release_note:breaking label Sep 4, 2020

yctercero reviewed Sep 8, 2020

View reviewed changes

pr comments

0a8caa5

Merge branch 'master' into refactor-ancestry

203c363

dhurley14 approved these changes Sep 9, 2020

View reviewed changes

marshallmain merged commit 42a6934 into elastic:master Sep 9, 2020

marshallmain deleted the refactor-ancestry branch September 9, 2020 18:05

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 11, 2020

marshallmain mentioned this pull request Sep 14, 2020

[7.x] [Security_solution][Detections] Refactor signal ancestry to allow multiple parents (#76531) #77346

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 14, 2020

Conversation

marshallmain commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marshallmain Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marshallmain Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marshallmain Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marshallmain Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marshallmain Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 2, 2020

Uh oh!

marshallmain commented Sep 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madirey left a comment

Choose a reason for hiding this comment

Uh oh!

marshallmain commented Sep 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yctercero Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marshallmain commented Sep 8, 2020

Uh oh!

kibanamachine commented Sep 8, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

dhurley14 left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

marshallmain commented Sep 2, 2020 •

edited

Loading

marshallmain Sep 2, 2020 •

edited

Loading

marshallmain Sep 2, 2020 •

edited

Loading

marshallmain Sep 2, 2020 •

edited

Loading

marshallmain Sep 2, 2020 •

edited

Loading

marshallmain Sep 2, 2020 •

edited

Loading

yctercero Sep 8, 2020 •

edited

Loading