[Security Solution] Incorporates EQL options in EQL query validation on both Rule Creation and Timeline #178468

rylnd · 2024-03-12T03:14:20Z

Summary

This PR updates the Detection Rule Creation and Timeline forms to account for the new(er) EQL options fields (timestamp_field, event_category_field, and tiebreaker_field) when validating the EQL query. While the rule query and timeline query (respectively) would correctly persist and use these options, they were unused during EQL validation, meaning that in certain situations it was impossible to produce a "valid" (from the perspective of the form) EQL query if your data necessitated one of those options (see #158326 for more details).

Changes

The above was accomplished by:

Adding a hidden form field to the UI for eqlOptions
- A lack of a field component meant that formData never held any value for this field
- This meant that the form-based validations (i.e. eqlValidator) would not have access to eqlOptions
Updating the onOptionsChange hook to additionally inform the form of changes to eqlOptions
- This allows the form to have updated values for eqlOptions with minimal changes
- Not ideal from a data-flow perspective, since the field changes are updating two copies of eqlOptions. However, we're only using the "form copy" for the validation call, and everything else works as it did before.

Additional changes

Update: I've reverted the described changes to the default timeline values below, and moved that to a separate issue.

In the course of performing the above tasks for timeline, I found the behavior there to be slightly different: we were specifying default values for these fields, which was redundant in some cases (timestamp_field, event_category_field), and invalid in the case of tiebreaker_field. You can see here how the tiebreaker_field must be changed to make the query valid:

Screen.Recording.2024-03-11.at.10.12.28.PM.mov

I opted to remove these default values, as well as the defaulting behavior from the search strategy call (see 10a47fd). While these values may be persisted in existing saved timelines, this makes the EQL workflow much cleaner for new users, and those users with persisted EQL options can now modify and validate those options on the fly.

Steps to Review

For @elastic/kibana-visualizations and/or @elastic/kibana-data-discovery : I've added a few tests to the EQL search strategy that document existing behavior RE EQL option fields. Nothing about the search strategy itself was changed. If you think the tests aren't useful, I'm happy to delete them.

Create an index without a @timestamp field, and insert some data:

Dev Tools

PUT timestamp/
{
  "mappings": {
    "dynamic": "strict",
    "properties": {
      "timestamp": {
        "type": "date",
        "format": "epoch_second"
      },
      "locale": {
        "type": "keyword"
      },
      "created_at": {
        "type": "date",
        "format": "epoch_second"
      },
      "event": {
        "properties": {
          "category": {
            "ignore_above": 1024,
            "type": "keyword"
          }
        }
      }
    }
  }
}

PUT timestamp/_doc/4
{
  "timestamp": 1710211630,
  "locale": "es",
  "created_at": 1710211630,
  "event.category": "configuration"
}

Create a new EQL detection rule, and choose timestamp as the index pattern
Type any where true as the query, and observe a validation error (Found 1 problem line -1:-1: Unknown column [@timestamp])
Open EQL Settings popover, and select timestamp from the Timestamp field dropdown
Observe that the query has been revalidated, and found valid.

New Behavior:

Screen.Recording.2024-03-11.at.10.06.19.PM.mov

Related Issues

Addresses [Detection Engine] - EQL timestamp override not taking as expected #158326

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
Flaky Test Runner was used on any tests changed

For maintainers

This was checked for breaking API changes and was labeled appropriately

@timestamp

The conclusion is that we currently need _both_ of these fields for the EQL rule to operate if the data doesn't have a @timestamp at all. However, the UI does not allow you to set both of these fields.

…rameters This is a sanity check to allowing our EQL validation functionality to leverage these same options.

This will break our calls, which is what we want (they need to pass options now).

Previously, the eqlOptions field was referenced in the schema of the Define Step form, but as there was no component controlling the field, it was unknown to the form data, and consequently unavailable to the queryBar validator (which is ultimately our goal here). By adding a component for the eqlOptions field (albeit hidden), and updating our onOptionsChange callback to additionally set that field data, the rule form now has eqlOptions available during validation of the EQL query.

This is one case where the form lib makes that super easy 👍.

This was mostly a sanity check, but good to document this API IMO.

By adding the field to the form itself (as a hidden field), and modifying the onOptionsChange callback to additionally update the form value, we now have that field value available to us when validating the EQL query itself. This is all near-identical to the changes just made to the Define Step rule creation form.

* The default value of `''` for `tiebreaker_field` is invalid and must be changed by the user * The redux store selector was using default values that both didn't make sense and were never used * Removes defaulting behavior for options in the timeline EQL search strategy, since sending `timestamp_field: undefined` is equivalent to not specifying any value (and thus falls back to the equivalent EQL default)

rylnd · 2024-03-12T14:10:47Z

/ci

@timestamp

While the data I wrote was "non-ECS," that's not a necessary feature of the bug here. It simply needs to _not_ have a @timestamp field. This also updates the corresponding FTR tests to not be exclusive (whoops).

While the '@' in the file/folder seems to work, I'm a little dubious of the portability. Using normal characters here just to be safe.

This is retrieving the general rule index pattern input (of which there is no other reference); I think the naming was due to it originally being introduced while testing the IM rule type.

rylnd · 2024-03-12T22:15:21Z

/ci

This automates the process that I've been using to test this: given data without an `@timestamp` (the default time field for EQL), you'll see errors when we attempt to validate the EQL query UNTIL you choose another timestamp field via the EQL options.

rylnd · 2024-03-12T22:43:27Z

/ci

An empty string is the default value for the EQL option `tiebreaker_field` in the timeline EQL form. However, if this is passed directly to ES an error will be thrown, as this is never a valid value for that option. A similar guard exists in the timeline EQL search strategy; we're adding one to our validation call as well now that it has to deal with those values.

These additional parameters do not need to be specified in order to produce a valid query, but I am requiring that the key be specified for sanity's sake.

rylnd · 2024-03-13T18:21:00Z

@kqualters-elastic I split off the defaulting conversation into https://github.com/elastic/security-team/issues/8892. On this PR, I've reverted the defaulting changes, and added the necessary guard to prevent the behavior in the above video.

rylnd · 2024-03-13T18:33:22Z

...rity_solution/public/detection_engine/rule_creation_ui/components/step_define_rule/index.tsx

@@ -814,17 +827,16 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
    <>
      <StepContentWrapper addPadding={!isUpdateView}>
        <Form form={form} data-test-subj="stepDefineRule">
-          <StyledVisibleContainer isVisible={false}>


This is wholly unrelated to this PR, but react was throwing dev warnings on this page and I couldn't help but fix it. See 3a9d040 for more details.

rylnd · 2024-03-13T18:34:37Z

...s/cypress/e2e/detection_response/detection_engine/rule_creation/event_correlation_rule.cy.ts

@@ -174,4 +183,34 @@ describe('EQL rules', { tags: ['@ess', '@serverless'] }, () => {
        });
    });
  });
+
+  describe('with source data requiring EQL overrides', () => {


This is the one true 🔴 -> 🟢 test in this PR; the rest are just documenting existing behavior.

kqualters-elastic · 2024-03-13T19:34:19Z

@rylnd cool, I ran everything locally before that commit and all lgtm 👍 I think an easy solution (and could probably revert to how you had it) would be to just add the default field to either the form field description label or the input placeholder, and not have any defaults in the app state at all. Solves 2 and lets it work how you initially had it I think.

lukasolson

DataDiscovery code changes LGTM!

In elastic#178724, we added the ability to retrieve archives from another location (namely, where our FTR archives live). This allows me to delete the duplicated archive specific for the cypress usage, and share the FTR archive between both. Nice!

vitaliidm · 2024-03-18T09:49:41Z

x-pack/plugins/security_solution/public/common/hooks/eql/api.ts

 }: Params): Promise<{ valid: boolean; errors: string[] }> => {
  const { rawResponse: response } = await firstValueFrom(
    data.search.search<EqlSearchStrategyRequest, EqlSearchStrategyResponse>(
      {
        params: {
          index: dataViewTitle,
          body: { query, runtime_mappings: runtimeMappings, size: 0 },
+          // @ts-expect-error top-level keys are unexpected in {@link EqlSearchRequest}


why this error happens here?
I don't see any top level await introduced here

These @ts-expect-errors were due to an incompatibility between the actual EQL endpoint, and the types in the search strategy. I managed to extend the type to make this all work without the need for the @ts-expect-error: 6767182.

I'm not sure what you were referring to with the await comment, though.

vitaliidm · 2024-03-18T09:53:50Z

...s/cypress/e2e/detection_response/detection_engine/rule_creation/event_correlation_rule.cy.ts

+      cy.task('esArchiverUnload', { archiveName: 'no_at_timestamp_field', type: 'ftr' });
+    });
+
+    it('includes EQL options in query validation', () => {


can you please run flaky test runner to ensure this test is not flaky?

https://ci-stats.kibana.dev/trigger_flaky_test_runner/1

Of course.

Do you see anything in the test itself that looks suspect?

nothing suspicious, but when adding new test we usually run them in flaky test runner to ensure they are not flaky, as an effort to reduce overall Cypress tests flakiness.
Since even perfectly written code does not guarantee tests won't be flaky unfortunately

vitaliidm · 2024-03-18T10:39:23Z

...nse/detection_engine/rule_execution_logic/trial_license_complete_tier/execution_logic/eql.ts

+
+        expect(_log.errors).to.be.empty();
+        expect(_log.warnings).to.eql([
+          'This rule reached the maximum alert limit for the rule execution. Some alerts were not created.',


test title suggests no alerts were generated, but they were
specifying only timestamp_field results in a warning, and no alerts are generated

Oops! Good catch. f3e55d5

vitaliidm · 2024-03-18T10:40:11Z

...nse/detection_engine/rule_execution_logic/trial_license_complete_tier/execution_logic/eql.ts

+          logs: [_log],
+        } = await previewRule({ supertest, rule });
+
+        expect(_log.errors).to.be.empty();


test title suggests no alerts were generated and error encountered
no error in test and alert were generated
specifying only timestamp_override results in an error, and no alerts are generated

vitaliidm · 2024-03-18T10:42:21Z

...nse/detection_engine/rule_execution_logic/trial_license_complete_tier/execution_logic/eql.ts

+        expect(previewAlerts.length).to.be.greaterThan(0);
+      });
+
+      it('specifying both timestamp_override and timestamp_field behaves as expected', async () => {


can we add more details what is expected in this case.

Good catch, thank you! f3e55d5

The actual ES client/EQL endpoint allows for specifying options at both the top level _and_ within the request's `body` parameter. However, we had only typed the latter in our original implementation. This aligns the types with the actual (demonstrable, as evidenced by all the @ts-expect-errors) behavior of the server, using a union type cribbed from responseops (see elastic#171348). Consequently, all these `@ts-expect-error`s can now be removed.

A few of these were updated in the course of development without the title being updated. This clarifies the behavior in the case of data with `@timestamp`, as well as replacing the vague "behaves as expected" descriptions with more helpful ones.

rylnd · 2024-03-18T22:42:40Z

@vitaliidm thanks for the diligence in your review! I fixed the type error, as well as the test descriptions, and the flaky runner is running now. Ready for another review!

kibana-ci · 2024-03-18T23:41:05Z

💚 Build Succeeded

Buildkite Build
Commit: f3e55d5

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	15.5MB	15.5MB	+1.3KB

History

💔 Build #197869 failed 7ac409a
💛 Build #197760 was flaky f6739dd
💛 Build #197515 was flaky e837f19
💔 Build #197309 failed 0d8ad8b
💔 Build #197165 failed 2676ada

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

cc @rylnd

vitaliidm

Detection Engine area LGTM
@rylnd thank you for fixing it, adding extensive test coverage and addressing my review comments

@timestamp

…on both Rule Creation and Timeline (elastic#178468) ## Summary This PR updates the Detection Rule Creation and Timeline forms to account for the new(er) EQL options fields (`timestamp_field`, `event_category_field`, and `tiebreaker_field`) when validating the EQL query. While the rule query and timeline query (respectively) would correctly persist and use these options, they were unused during EQL validation, meaning that in certain situations it was impossible to produce a "valid" (from the perspective of the form) EQL query if your data necessitated one of those options (see elastic#158326 for more details). ### Changes The above was accomplished by: 1. Adding a hidden form field to the UI for `eqlOptions` * A lack of a field component meant that `formData` never held any value for this field * This meant that the form-based validations (i.e. `eqlValidator`) would not have access to `eqlOptions` 2. Updating the `onOptionsChange` hook to additionally inform the form of changes to `eqlOptions` * This allows the form to have updated values for `eqlOptions` with minimal changes * Not ideal from a data-flow perspective, since the field changes are updating two copies of `eqlOptions`. However, we're only using the "form copy" for the validation call, and everything else works as it did before. ### Additional changes __Update__: I've reverted the described changes to the default timeline values below, and moved that to a [separate issue](elastic/security-team#8892). In the course of performing the above tasks for timeline, I found the behavior there to be slightly different: we were specifying default values for these fields, which was redundant in some cases (`timestamp_field`, `event_category_field`), and invalid in the case of `tiebreaker_field`. You can see here how the `tiebreaker_field` must be changed to make the query valid: https://github.com/elastic/kibana/assets/657252/556bc998-c147-4825-8bde-5d8d03873e75 ~~I opted to remove these default values, as well as the defaulting behavior from the search strategy call (see 10a47fd). While these values may be persisted in existing saved timelines, this makes the EQL workflow much cleaner for new users, and those users with persisted EQL options can now modify and validate those options on the fly.~~ #### Steps to Review For @elastic/kibana-visualizations and/or @elastic/kibana-data-discovery : I've added a few tests to the EQL search strategy that document existing behavior RE EQL option fields. Nothing about the search strategy itself was changed. If you think the tests aren't useful, I'm happy to delete them. 1. Create an index without a `@timestamp` field, and insert some data: <details> <summary>Dev Tools</summary> PUT timestamp/ { "mappings": { "dynamic": "strict", "properties": { "timestamp": { "type": "date", "format": "epoch_second" }, "locale": { "type": "keyword" }, "created_at": { "type": "date", "format": "epoch_second" }, "event": { "properties": { "category": { "ignore_above": 1024, "type": "keyword" } } } } } } PUT timestamp/_doc/4 { "timestamp": 1710211630, "locale": "es", "created_at": 1710211630, "event.category": "configuration" } </details> 2. Create a new EQL detection rule, and choose `timestamp` as the index pattern 3. Type `any where true` as the query, and observe a validation error (`Found 1 problem line -1:-1: Unknown column [@timestamp]`) 4. Open EQL Settings popover, and select `timestamp` from the `Timestamp field` dropdown 5. Observe that the query has been revalidated, and found valid. ### New Behavior: https://github.com/elastic/kibana/assets/657252/22a63363-d597-4dfd-8f9a-647f4084ec0e ### Related Issues * Addresses elastic#158326 ### Checklist Delete any items that are not applicable to this PR. - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 5be91c9)

kibanamachine · 2024-03-19T17:57:12Z

💚 All backports created successfully

Status	Branch	Result
✅	8.13

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@timestamp

…dation on both Rule Creation and Timeline (#178468) (#178994) # Backport This will backport the following commits from `main` to `8.13`: - [[Security Solution] Incorporates EQL options in EQL query validation on both Rule Creation and Timeline (#178468)](#178468)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  --------- Co-authored-by: Ryland Herrick <[email protected]>

rylnd added 10 commits February 23, 2024 12:32

Document current EQL rule behavior with timestamp fields

02a0c50

The conclusion is that we currently need _both_ of these fields for the EQL rule to operate if the data doesn't have a @timestamp at all. However, the UI does not allow you to set both of these fields.

Add similar tests for data with @timestamp

7838039

Add tests validating that our EQL search strategy respects certain pa…

413ed8c

…rameters This is a sanity check to allowing our EQL validation functionality to leverage these same options.

EQL validation function accepts EQL options as a parameter

cedecbb

This will break our calls, which is what we want (they need to pass options now).

Ensure that changes to EQL options cause the EQL query to be revalidated

3dc18c1

This is one case where the form lib makes that super easy 👍.

Add a test validating how onOptionsChange is called

4105e69

This was mostly a sanity check, but good to document this API IMO.

Remove unused interface

39d005b

rylnd added the Team:Detection Engine Security Solution Detection Engine Area label Mar 12, 2024

rylnd self-assigned this Mar 12, 2024

rylnd added Feature:Timeline Security Solution Timeline feature Feature:Event Correlation (EQL) Rule Security Solution Event Correlation (EQL) rule type Feature:Rule Creation Security Solution Detection Rule Creation workflow labels Mar 12, 2024

Merge branch 'main' into rylnd/bugfix/eql_rule_timestamp_override

2676ada

rylnd added 3 commits March 12, 2024 13:50

Rename new archive to be more accurate

bbd6a8d

While the data I wrote was "non-ECS," that's not a necessary feature of the bug here. It simply needs to _not_ have a @timestamp field. This also updates the corresponding FTR tests to not be exclusive (whoops).

Rename archive, again

fa03d22

While the '@' in the file/folder seems to work, I'm a little dubious of the portability. Using normal characters here just to be safe.

More accurate cypress selector

b2aec76

This is retrieving the general rule index pattern input (of which there is no other reference); I think the naming was due to it originally being introduced while testing the IM rule type.

rylnd marked this pull request as ready for review March 13, 2024 00:12

rylnd requested review from a team as code owners March 13, 2024 00:12

rylnd added the release_note:fix label Mar 13, 2024

validateEql options parameter is optional

e837f19

These additional parameters do not need to be specified in order to produce a valid query, but I am requiring that the key be specified for sanity's sake.

rylnd commented Mar 13, 2024

View reviewed changes

kqualters-elastic approved these changes Mar 13, 2024

View reviewed changes

lukasolson approved these changes Mar 13, 2024

View reviewed changes

semd approved these changes Mar 14, 2024

View reviewed changes

rylnd added 3 commits March 14, 2024 12:16

Merge branch 'main' into rylnd/bugfix/eql_rule_timestamp_override

f6739dd

Merge branch 'main' into rylnd/bugfix/eql_rule_timestamp_override

c970f74

vitaliidm reviewed Mar 18, 2024

View reviewed changes

rylnd added 3 commits March 18, 2024 17:07

Merge branch 'main' into rylnd/bugfix/eql_rule_timestamp_override

92a2bd2

rylnd requested a review from vitaliidm March 18, 2024 22:41

vitaliidm approved these changes Mar 19, 2024

View reviewed changes

rylnd added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Mar 19, 2024

rylnd merged commit 5be91c9 into elastic:main Mar 19, 2024
38 checks passed

rylnd deleted the rylnd/bugfix/eql_rule_timestamp_override branch March 19, 2024 17:52

kibanamachine added the v8.14.0 label Mar 19, 2024

kibanamachine mentioned this pull request Mar 19, 2024

[8.13] [Security Solution] Incorporates EQL options in EQL query validation on both Rule Creation and Timeline (#178468) #178994

Merged

kibanamachine added the v8.13.0 label Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Incorporates EQL options in EQL query validation on both Rule Creation and Timeline #178468

[Security Solution] Incorporates EQL options in EQL query validation on both Rule Creation and Timeline #178468

rylnd commented Mar 12, 2024 •

edited by kibanamachine

Loading

rylnd commented Mar 12, 2024

rylnd commented Mar 12, 2024

rylnd commented Mar 12, 2024

rylnd commented Mar 13, 2024

rylnd Mar 13, 2024

rylnd Mar 13, 2024

kqualters-elastic commented Mar 13, 2024 •

edited

Loading

lukasolson left a comment

vitaliidm Mar 18, 2024

rylnd Mar 18, 2024

vitaliidm Mar 18, 2024

rylnd Mar 18, 2024

vitaliidm Mar 19, 2024

vitaliidm Mar 18, 2024

rylnd Mar 18, 2024

vitaliidm Mar 18, 2024

rylnd Mar 18, 2024

vitaliidm Mar 18, 2024

rylnd Mar 18, 2024

rylnd commented Mar 18, 2024

kibana-ci commented Mar 18, 2024

vitaliidm left a comment

kibanamachine commented Mar 19, 2024

[Security Solution] Incorporates EQL options in EQL query validation on both Rule Creation and Timeline #178468

[Security Solution] Incorporates EQL options in EQL query validation on both Rule Creation and Timeline #178468

Conversation

rylnd commented Mar 12, 2024 • edited by kibanamachine Loading

Summary

Changes

Additional changes

Steps to Review

New Behavior:

Related Issues

Checklist

For maintainers

rylnd commented Mar 12, 2024

rylnd commented Mar 12, 2024

rylnd commented Mar 12, 2024

rylnd commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqualters-elastic commented Mar 13, 2024 • edited Loading

lukasolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylnd commented Mar 18, 2024

kibana-ci commented Mar 18, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

History

vitaliidm left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 19, 2024

💚 All backports created successfully

Questions ?

rylnd commented Mar 12, 2024 •

edited by kibanamachine

Loading

kqualters-elastic commented Mar 13, 2024 •

edited

Loading