elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.71k stars 8.12k forks source link

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

Closed yctercero closed 5 months ago

yctercero commented 1 year ago

Describe the bug:

On both the rule creation flow and in timeline, when a timestamp override field is selected for an EQL rule, it is not honored. In some cases, it results in an EQL validation error, blocking user.

Kibana/Elasticsearch Stack version: Found in 8.7 but looks like possibly it's a 10 month old bug,

Steps to reproduce:

Create an index that uses timestamp instead of @timestamp.

  1. Go to create rule
  2. Select EQL
  3. Input a valid eql query
  4. Notice validation error about timestamp
  5. Select timestamp override to be timestamp
  6. Notice nothing changes

Current behavior: Timestamp override is not respected.

Expected behavior: Timestamp override is respected and no validation error occurs.

Screenshots (if relevant): image (1) image (2)

Any additional context (logs, chat logs, magical formulas, etc.): @kqualters-elastic found one area where the timestamp field is hardcoded to @timestamp. Also need to make sure it's passed through for the query validation

elasticmachine commented 1 year ago

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

elasticmachine commented 1 year ago

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

rylnd commented 6 months ago

I looked into this briefly, and there looks to be an EQL-specific field that we use for this purpose: timestampField, and that timestampOverride is effectively unused.

At the product level, I'm not sure if we want to support both of these rule options, and whether they're equivalent in functionality (and precedence). It would certainly be simplest to just remove timestampField and use timestampOverride instead; I think this bug is indicative of the fact that the former is unexpected and differs from the rest of the rule types, unless I'm misunderstanding the semantics of that field.

marshallmain commented 6 months ago

timestampField controls the field that is used for ordering events within a sequence, whereas the timestamp override controls which events are in range of the rule's query. We have a short blurb about it here. The idea is that with late arriving events you may want the rule to always query the events that just arrived, but run sequence logic on the events that depends on the order they actually occurred on the source.

rylnd commented 6 months ago

Thanks @marshallmain, that's useful context. I wrote some integration tests to document the current behavior/interaction of these two fields, with the following conclusions:

  1. If @timestamp is present, then neither of the fields are required since they both fall back to @timestamp
  2. If @timestamp is absent:
    1. Both fields are required, as neither has an appropriate fallback
    2. Omitting timestamp_field results in an error, and no alerts are generated
    3. Omitting timestamp_override results in a warning (actually, two identical warnings), and no alerts are generated
    4. The UI does not allow you to create a valid EQL rule, as you cannot proceed past the Define step to set the required timestamp_override

Focusing on the UI piece (the timeline hardcoding seems like a related but separate issue): I think the quickest fix here would be to link the two and set timestamp_override to the value of timestamp_field if it's unspecified; that would provide the expected behavior documented here. However, that might be unexpected/undesirable if one does have a @timestamp field to fall back on (and I'm not sure if we can make that determination in the UI, or whether there's precedent for that).

marshallmain commented 6 months ago

It looks like the timestamp_field is not being passed into the EQL validation request here, as I'm not seeing timestamp_field show up in the actual network request. If we pass it in then the query validation should work and allow the rule to be created even without timestamp_override being set. I think we should avoid linking the 2 fields together if possible so they stay independent.

rylnd commented 6 months ago

Good call; I realized there might be some preview- or validation-specific behavior contributing to this, but neglected to mention that above.

I'll make these changes and update those added tests 👍

yctercero commented 6 months ago

Not sure if it's already the case but this would be a great technical design decision to add to the dev docs as well.

rylnd commented 5 months ago

Closed by 5be91c9; backported to 8.13 in https://github.com/elastic/kibana/pull/178994.