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

[Security Solution] [Detections] EQL Rule Creation #76831

Merged
merged 17 commits into from
Sep 15, 2020

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Sep 4, 2020

Summary

This allows us to create simple EQL rules. While they can be activated, execution of an EQL rule will currently result in an (intentional) error. Overview:

  • New type/field during rule creation
    • Field is simply a textarea for now
    • EQL query reuses the existing query and language attributes
  • Integration test for EQL Rule Creation
  • Audit treatment of EQL Rules in UI
    • Verifying that EQL rules do not break the UI.
  • Audit switches based on rule type throughout app

Detections_-_Kibana

TBD:

  • UI Validation of EQL Syntax
  • Autocomplete of index fields?

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Without this, we get some bad behaviors:
* Cannot use React.memo'd components
* Cannot switch between UseField components (causes a "change in the
  order of hooks" error from React)
WIP because: they're probably not treated well in the UI, and they're certainly not
going to execute properly, and there are no tests.
@rylnd rylnd added Team:SIEM release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 4, 2020
@rylnd rylnd self-assigned this Sep 4, 2020
It's mostly just a glorified textarea for now.
* Does not assert the query language, as that is not displayed on Rule
  Details
* Does not exercise rule execution
This is to prevent undefined behavior until EQL execution is
implemented.
I changed the default value for the form field mock from an array to a
string; this fixes the few tests that were relying on it being an array.
I made a pass through our treatment of RuleType to verify that EQL rules
would be treated appropriately. Since the default/fallthrough case is
typically the Query rule, and since this rule has the same
attributes/behavior as the new EQL rule, not much had to change here.

I converted a few if statements to exhaustive switches where possible,
and used predicate helpers in places where it was not.
@rylnd rylnd added the v8.0.0 label Sep 9, 2020
@@ -102,7 +102,7 @@ function UseFieldComp<T = unknown>(props: Props<T>) {
);
}

return componentToRender({ field, ...propsToForward });
return <ComponentToRender {...{ field, ...propsToForward }} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebelga I haven't yet added a test to encompass this change, but you can read about the errant behavior in 9d5ce1c. My first thought was simply to assert that React.createElement is called when a custom component is used, but I'd appreciate your thoughts!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to test the different use cases instead of the implementation. So I would have a dummy form with 3 types of fields (or more if there are more use cases) and make sure they render correctly (are present in the DOM)

  • no component passed --> renders an input in the dom
  • component passed --> renders it
  • memoized component passed --> renders it

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind explaining what you mean by "Cannot switch between UseField components"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebelga in this PR we're swapping between two different field components based on which rule type the user has chosen. link. This causes react warnings if the components are not invoked with React.createElement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebelga tests added here: a9a6703

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool thanks for the tests @rylnd !

For the swap of components, we took the habit of adding a key to the UseField being swapped. To make sure the component unmounts and re-mounts.

@@ -245,7 +246,9 @@ export const signalRulesAlertType = ({
if (bulkCreateDuration) {
result.bulkCreateTimes.push(bulkCreateDuration);
}
} else if (type === 'threshold' && threshold) {
} else if (isEqlRule(type)) {
throw new Error('EQL Rules are under development, execution is not yet implemented');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marshallmain this is our placeholder until EQL evaluation is implemented.

} else if (type === 'threshold' && threshold) {
} else if (isEqlRule(type)) {
throw new Error('EQL Rules are under development, execution is not yet implemented');
} else if (isThresholdRule(type) && threshold) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patrykkopycinski Is it correct for isThresholdRule(type) && !threshold to fall through to the else clause here? I noticed this when auditing our rule type branching.

.eq(DEFINITION_CUSTOM_QUERY)
.invoke('text')
.should('eql', `${eqlRule.customQuery} `);
cy.get(DEFINITION_STEP).eq(2).invoke('text').should('eql', 'Event Correlation');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MadameSheema this is the same issue as mentioned here

@rylnd rylnd marked this pull request as ready for review September 9, 2020 21:34
@rylnd rylnd requested review from a team as code owners September 9, 2020 21:34
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

* 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)
  ...
@@ -294,6 +294,7 @@ export const toOrUndefined = t.union([to, t.undefined]);
export type ToOrUndefined = t.TypeOf<typeof toOrUndefined>;

export const type = t.keyof({
eql: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense from a schema perspective to have eql be its own type? vs just another language for query

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rw-access that's an interesting point. In general, a rule's type could be derived from its other fields, and we're mainly using it as a shortcut for that determination. I don't believe there's currently any ambiguity that the type field clarifies.

Another consideration is signal.rule.type, which we currently use to e.g. determine what actions can be taken on a given alert. We could compute the type when the alert is generated so as not to have to fetch the rule and derive its type, but that would introduce some inconsistency with signal.rule being a strict subset of the rule's fields.

Since both the querying and the alert generation are unique to EQL, I'm inclined to promote that "up" to the rule type as opposed to just query/language, but I'm happy to hear your thoughts.

Copy link
Contributor

@rw-access rw-access Sep 10, 2020

Choose a reason for hiding this comment

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

Since both the querying and the alert generation are unique to EQL, I'm inclined to promote that "up" to the rule type as opposed to just query/language, but I'm happy to hear your thoughts.

Yeah, I think EQL is very special snowflake compared to Lucene/KQL, because of the complexity in its response format. Like you said, I think that it makes sense that even though the schema of the rule itself is otherwise identical to KQL/Lucene, it's processed very differently. Great point.

I wonder if that adds some more precision to what type: query means. To me, I now interpret that as "find the documents that match this boolean condition". I think that helps me mentally understand the difference better.

Thanks, this gave me some mental clarity. It could be worthwhile to communicate some of that reasoning in the "Rule type" part of the UI. I think "Use KQL or Lucene to detect issues across indices." makes me think about cluster health. Maybe "Use KQL or Lucene to alert when documents match a condition" could help, but I'll defer to you and the UX team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are great points! I've opened a ticket to discuss our qualification of the different rule types and how we present that to the user: #77250

There was an issue previously where memoized components would not work;
these are primarily regression tests covering that use case.
@rylnd
Copy link
Contributor Author

rylnd commented Sep 14, 2020

@elasticmachine merge upstream

@sebelga sebelga self-requested a review September 14, 2020 15:35
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

Changes on the form lib look good! Thanks for adding this fix @rylnd ! 👍

When swapping between the Custom Query and EQL rule types, we want to
ensure that the corresponding input component coming from UseField fully
unmounts and remounts with the new component.
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 reviewed code -- LGTM! 👍 Thanks for the cleanup around the rule type checks, exhaustive switches, and hooks-form-lib changes as well. No nits on the code front, but do want to note that we may have to constrain some of the additional configuration options (e.g. field overrides) for EQL depending on how the responses shape up, but can address that later. Thanks @rylnd! 🙂

@rylnd
Copy link
Contributor Author

rylnd commented Sep 15, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 1959 +2 1957

async chunks size

id value diff baseline
securitySolution 10.0MB +3.5KB 10.0MB

page load bundle size

id value diff baseline
esUiShared 995.1KB +63.0B 995.0KB
securitySolution 793.3KB +18.0B 793.2KB
total +81.0B

distributable file count

id value diff baseline
default 45570 +1 45569

History

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

@rylnd rylnd merged commit 7f3ec3f into elastic:master Sep 15, 2020
@rylnd rylnd deleted the eql_rules_creation branch September 15, 2020 17:29
rylnd added a commit to rylnd/kibana that referenced this pull request Sep 15, 2020
* Use existing predicate helper to avoid hardcoded strings

* Render our field components with React.createElement

Without this, we get some bad behaviors:
* Cannot use React.memo'd components
* Cannot switch between UseField components (causes a "change in the
  order of hooks" error from React)

* WIP: EQL Rules can be created

WIP because: they're probably not treated well in the UI, and they're certainly not
going to execute properly, and there are no tests.

* Add unit tests for changes to schema + helpers

* Add unit tests for new EQL query input component

It's mostly just a glorified textarea for now.

* Add integration test for EQL Rule creation

* Does not assert the query language, as that is not displayed on Rule
  Details
* Does not exercise rule execution

* Use predicate helper

* Throw an error if an EQL Rule is executed

This is to prevent undefined behavior until EQL execution is
implemented.

* Fix failing tests

I changed the default value for the form field mock from an array to a
string; this fixes the few tests that were relying on it being an array.

* Audit our rule statements/switches

I made a pass through our treatment of RuleType to verify that EQL rules
would be treated appropriately. Since the default/fallthrough case is
typically the Query rule, and since this rule has the same
attributes/behavior as the new EQL rule, not much had to change here.

I converted a few if statements to exhaustive switches where possible,
and used predicate helpers in places where it was not.

* Add tests around use of custom components with UseField

There was an issue previously where memoized components would not work;
these are primarily regression tests covering that use case.

* Fix typo

* Add keys to UseField to ensure unmount

When swapping between the Custom Query and EQL rule types, we want to
ensure that the corresponding input component coming from UseField fully
unmounts and remounts with the new component.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
rylnd added a commit that referenced this pull request Sep 15, 2020
* Use existing predicate helper to avoid hardcoded strings

* Render our field components with React.createElement

Without this, we get some bad behaviors:
* Cannot use React.memo'd components
* Cannot switch between UseField components (causes a "change in the
  order of hooks" error from React)

* WIP: EQL Rules can be created

WIP because: they're probably not treated well in the UI, and they're certainly not
going to execute properly, and there are no tests.

* Add unit tests for changes to schema + helpers

* Add unit tests for new EQL query input component

It's mostly just a glorified textarea for now.

* Add integration test for EQL Rule creation

* Does not assert the query language, as that is not displayed on Rule
  Details
* Does not exercise rule execution

* Use predicate helper

* Throw an error if an EQL Rule is executed

This is to prevent undefined behavior until EQL execution is
implemented.

* Fix failing tests

I changed the default value for the form field mock from an array to a
string; this fixes the few tests that were relying on it being an array.

* Audit our rule statements/switches

I made a pass through our treatment of RuleType to verify that EQL rules
would be treated appropriately. Since the default/fallthrough case is
typically the Query rule, and since this rule has the same
attributes/behavior as the new EQL rule, not much had to change here.

I converted a few if statements to exhaustive switches where possible,
and used predicate helpers in places where it was not.

* Add tests around use of custom components with UseField

There was an issue previously where memoized components would not work;
these are primarily regression tests covering that use case.

* Fix typo

* Add keys to UseField to ensure unmount

When swapping between the Custom Query and EQL rule types, we want to
ensure that the corresponding input component coming from UseField fully
unmounts and remounts with the new component.

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

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 16, 2020
…s-for-710

* 'master' of github.com:elastic/kibana: (95 commits)
  log request body in new ES client (elastic#77150)
  use `navigateToUrl` to navigate to recent nav links (elastic#77446)
  Move core config service to `kbn/config` package (elastic#76874)
  [UBI] Copy license to /licenses folder (elastic#77563)
  Skip flaky Events Viewer Cypress test
  [Lens] Remove dynamic names in telemetry fields (elastic#76988)
  [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366)
  [Enterprise Search] Add flag to restrict width of layout (elastic#77539)
  [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967)
  [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090)
  [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058)
  [@kbn/utils] Adds missing dependency (elastic#77536)
  Add the Enterprise Search logo to the Overview search result (elastic#77514)
  [Logs UI] [Metrics UI] Remove saved object field mappings (elastic#75482)
  [Security Solution][Exceptions] Exception modal bulk close option only closes alerts generated by same rule (elastic#77402)
  Adds @kbn/utils package (elastic#76518)
  Map layout changes (elastic#77132)
  [Security Solution] [Detections] EQL Rule Creation (elastic#76831)
  Adding test user to maps tests under documents source folder  (elastic#77245)
  Suppress error logs when clients connect over HTTP instead of HTTPS (elastic#77397)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx
@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
release_note:skip Skip the PR/issue when compiling release notes 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.

7 participants