Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Detections][EQL] EQL rule execution in detection engine #77419

Merged
merged 31 commits into from
Sep 25, 2020

Conversation

marshallmain
Copy link
Contributor

@marshallmain marshallmain commented Sep 14, 2020

Summary

Adds backend functionality for EQL rules in detection engine. Now updated to create a building block alert for each event within a sequence as well as an alert for the sequence as a whole. Non-sequence EQL queries work very similarly to KQL detection rules.

Chart of example sequence signal structure updated with building blocks

image

Work for follow up PRs:

  • Enrich shell alert by finding common field values across all events in a sequence
  • Add field for Detection Engine run instance (each time alert executor runs gets a unique id)
  • Add a schema version check before rule execution and if there is a version mis-match we'll throw an error and not create alerts, and then have documentation that references this error so users can search to find out how to upgrade their signals index
  • Add rule overrides for single event EQL queries

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@marshallmain marshallmain marked this pull request as ready for review September 22, 2020 07:02
@marshallmain marshallmain requested review from a team as code owners September 22, 2020 07:02
@marshallmain marshallmain added v7.10.0 v8.0.0 release_note:enhancement Feature:Detection Rules Anything related to Security Solution's Detection Rules Team:Endpoint Response Endpoint Response Team labels Sep 22, 2020
@elasticmachine
Copy link
Contributor

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

@marshallmain marshallmain changed the title [Detections] WIP First draft of EQL rules in detection engine [Detections][EQL] EQL rule execution in detection engine Sep 22, 2020
Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this enhancement @marshallmain! 🚀

Per the demo over zoom with @spong @peluja1012 and @XavierM, this PR LGTM WRT enabling the Investigate in timeline workflow, which should display (in Timeline) both the 🐢 "shell" detection, and the building block alerts representing the EQL sequences that triggered the shell detection.

More specifically, we validated that when a user clicks the Investigate in timeline action on a shell alert in Detections page, we can display both the shell and it's associated building blocks alerts in Timeline with the following (pseudo) query:

_id: id_of_the_shell_detection OR signal.group.id: id_of_the_shell_detection

@marshallmain
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@spong
Copy link
Member

spong commented Sep 24, 2020

Testing the field overrides with both sequence and non-sequence queries and doesn't look like they're working. No need to fix in this PR, but just commenting for tracking purposes. Not sure what we want to do here with sequence queries as there's no single source event (unless we want to use the shell alert with the common fields?), but queries which result in an alert created from a single event should be able to work with the existing override logic we have.

edit: Ahh yeah, I see in code now where you worked around the overrides... 🙂 This seems good to me for sequences, but shouldn't we be able to use the overrides for non-sequence results where one event results in one alert?

@@ -65,7 +65,7 @@ export const buildRule = ({

const meta = { ...ruleParams.meta, ...riskScoreMeta, ...severityMeta, ...ruleNameMeta };

const rule = pickBy<RulesSchema>((value: unknown) => value != null, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the last guard in place to remove any extraneous null/undefined's before writing the rule to the signals. In testing everything looks 👌 without (and I believe we've got tests covering this as well), but just want to verify the intent here.

Copy link
Contributor Author

@marshallmain marshallmain Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah converting the entire rule to Partial makes it harder to deal with when building the rest of the signal document since we need signal.rule.id later to compute the doc id. My thought is that we should be able to rely on the elasticsearch js library not to convert undefined fields to null or anything else so ES should never see undefined fields. Explicit null fields I think we should handle (usually reject) at the schema level instead of stripping them out later on. Some nulls are making it into the signal document since they're explicit in the rule SO.

I did more testing on this to figure out where the explicit null fields are coming from and we have some mismatches in the types between signalSchema and RuleTypeParams. For example, timestampOverride is schema.nullable(schema.string()) (so string | null) in signalSchema, but string | undefined in RuleTypeParams. So what happens when we don't define timestampOverride when creating a rule is it starts as undefined in the request but then the alert plugin validates using signalSchema which coerces the undefined into null and stores it in the SO. Then, when we get the rule SO back later to use in the alert executor, timestampOverride is string | null but RuleTypeParams says it's string | undefined and we use it as such. When it's null (which is when we originally sent undefined as part of rule creation), we end up with signal.rule.timestamp_override: null in the signal document.

The UI isn't displaying the null fields which is probably good. I think the best solution is to resolve the schema mismatches so that our undefined values stay undefined instead of being coerced into null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra sleuthing here! 🙂 And yeah, that was my worry with regards to the null's leaking into the signals. ++ to solving this via the schema mismatches to prevent that coercion in the first place.

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked out, tested locally and LGTM! Great clean code here @marshallmain, and tests to boot! 🙂

Congrats on all your hard work here to realize EQL within the Detection Engine, this is going to be an absolute game changer for our users! 🚀

@marshallmain
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id value diff baseline
securitySolution 10.2MB +4.3KB 10.2MB

page load bundle size

id value diff baseline
securitySolution 581.0KB +136.0B 580.9KB

distributable file count

id value diff baseline
default 45631 +1 45630

History

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

@marshallmain
Copy link
Contributor Author

@spong yeah I was so focused on sequences the single event case was kind of an afterthought 😆 I'll add the overrides for those in the next PR if it's not a blocker here

@marshallmain marshallmain merged commit faf4b04 into elastic:master Sep 25, 2020
@marshallmain marshallmain deleted the eql-rules branch September 25, 2020 20:23
marshallmain added a commit to marshallmain/kibana that referenced this pull request Sep 25, 2020
* First draft of EQL rules in detection engine

* Reorganize functions to separate files

* Start adding eventCategoryOverride option for EQL rules

* Add building block alerts for each event within sequence

* Use eql instead of eql_query for rule type

* Remove unused imports

* Fix tests

* Add basic tests for buildEqlSearchRequest

* Add rulesSchema tests for eql

* Add buildSignalFromSequence test

* Add threat rule fields to buildRuleWithoutOverrides

* Fix buildSignalFromSequence typecheck error

* Add more tests

* Add tests for wrapBuildingBlock and generateSignalId

* Use isEqlRule function and fix import error

* delete frank

* Move sequence interface to types.ts

* Fix import

* Remove EQL execution placeholder, add back language to eql rule type

* allow no indices for eql search

* Fix unit tests for language update

* Fix buildEqlSearchRequest tests

* Replace signal.child with signal.group

* remove unused import

* Move sequence signal group building to separate testable function

* Unbork the merge conflict resolution

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
marshallmain added a commit that referenced this pull request Sep 25, 2020
…8550)

* First draft of EQL rules in detection engine

* Reorganize functions to separate files

* Start adding eventCategoryOverride option for EQL rules

* Add building block alerts for each event within sequence

* Use eql instead of eql_query for rule type

* Remove unused imports

* Fix tests

* Add basic tests for buildEqlSearchRequest

* Add rulesSchema tests for eql

* Add buildSignalFromSequence test

* Add threat rule fields to buildRuleWithoutOverrides

* Fix buildSignalFromSequence typecheck error

* Add more tests

* Add tests for wrapBuildingBlock and generateSignalId

* Use isEqlRule function and fix import error

* delete frank

* Move sequence interface to types.ts

* Fix import

* Remove EQL execution placeholder, add back language to eql rule type

* allow no indices for eql search

* Fix unit tests for language update

* Fix buildEqlSearchRequest tests

* Replace signal.child with signal.group

* remove unused import

* Move sequence signal group building to separate testable function

* Unbork the merge conflict resolution

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 28, 2020
* master: (226 commits)
  [Enterprise Search] Added Logic for the Credentials View (elastic#77626)
  [CSM] Js errors (elastic#77919)
  Add the @kbn/apm-config-loader package (elastic#77855)
  [Security Solution] Refactor useSelector (elastic#75297)
  Implement tagcloud renderer (elastic#77910)
  [APM] Alerting: Add global option to create all alert types (elastic#78151)
  [Ingest pipelines] Upload indexed document to test a pipeline (elastic#77939)
  TypeScript cleanup in visualizations plugin (elastic#78428)
  Lazy load metric & mardown visualizations (elastic#78391)
  [Detections][EQL] EQL rule execution in detection engine (elastic#77419)
  Update tutorial-full-experience.asciidoc (elastic#75836)
  Update tutorial-define-index.asciidoc (elastic#75754)
  Add support for runtime field types to mappings editor. (elastic#77420)
  [Monitoring] Usage collection (elastic#75878)
  [Docs][Actions] Add docs for Jira and IBM Resilient (elastic#78316)
  [Security Solution][Resolver] Update @timestamp formatting (elastic#78166)
  [Security Solution] Fix app layout (elastic#76668)
  [Security Solution][Resolver] 2 new functions to DAL (elastic#78477)
  Adds new elasticsearch client to telemetry plugin (elastic#78046)
  skip flaky suite (elastic#78512) (elastic#78511) (elastic#78510) (elastic#78509) (elastic#78508) (elastic#78507) (elastic#78506) (elastic#78505) (elastic#78504) (elastic#78503) (elastic#78502) (elastic#78501) (elastic#78500)
  ...
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Rules Anything related to Security Solution's Detection Rules release_note:enhancement Team:Endpoint Response Endpoint Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants