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 Solutions][Detection Engine] Fixes timestamp bugs within source indexes when the formats are not ISO8601 format #101349

Merged

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Jun 3, 2021

Summary

We have a few bugs where when the source index for detections is not "strict_date_optional_time" it is possible that we will misinterpret the format to be epoch milliseconds when it could be epoch seconds or another ambiguous format or blow up when trying to write out the signals index. This fixes it to where we query for the source index format as an ISO8601 and when we copy the date time format we copy it back out as ISO8601 and insert it into the signal index as ISO8601.

See this gist for more details of how this was accidentally introduced when we added support for runtime fields and the general idea of the fix.

  • Removes docvalue_field and we now only use fields in detection engine search requests
  • Splits out the timestamp e2e tests into their own file for timestamps file
  • Adds more tests to ensure we copy what we expect and we are converting to ISO8601 in the signals
  • Removes ts-expect-error in a lot of areas including tests and then I fix the types and issues once it is removed.

Checklist

@FrankHassanabad FrankHassanabad requested review from delvedor and removed request for delvedor June 4, 2021 13:28
@FrankHassanabad FrankHassanabad self-assigned this Jun 4, 2021
@FrankHassanabad FrankHassanabad changed the title Fix timestamp source bug [Security Solutions][Detection Engine] Fixes timestamp bugs within source indexes when the formats are not ISO8601 format Jun 4, 2021
@FrankHassanabad FrankHassanabad marked this pull request as ready for review June 4, 2021 18:29
@FrankHassanabad FrankHassanabad requested a review from a team as a code owner June 4, 2021 18:29
@@ -6,7 +6,7 @@
"message": "hello world 4",
"@timestamp": "2020-12-16T15:16:18.570Z",
"event": {
"ingested": "2020-12-16T15:16:18.570Z"
"ingested": "2020-12-16T16:16:18.570Z"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Slight change just to ensure the tests still pass since event.ingested is usually a different value.

@@ -3,6 +3,7 @@
"value": {
"index": "myfakeindex-3",
"mappings": {
"dynamic": "strict",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Just added this to make sure I didn't add any bugs so it will blow up if it cannot insert the correct data.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @FrankHassanabad

Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

Reading through the gist referenced in the PR description my understanding is that the introduction of the field property into the query is overriding the formatting forced by the docvalue_fields which can lead to parsing error bugs. And that by moving the formatted timestamp / timestamp override fields (originally the docvalue_fields) into the field property we are able to continue to ensure proper formatting. This looks great! Thanks for catching this and the extra work around removing the ts-expect-error and the additional integration tests.

LGTM!

TL;DR from the gist:

Old and bad / buggy way
# Old and BAD because formatting doesn't stick
GET packetbeat*/_search
{
  "size": 1,
  "docvalue_fields": [
    {
      "field": "@timestamp",
      "format": "YYYY"
    }],
    "fields": [
      {"field": "*",
      "include_unmapped": "true"
      }
    ]
}

will return 

"@timestamp" : [
   "2021-06-04T21:07:21.951Z"
],
New and not buggy results
GET packetbeat*/_search
{
  "size": 1,
    "fields": [
      {"field": "*",
      "include_unmapped": "true"
      },
      {"field": "@timestamp",
      "format": "YYYY"}
    ]
}

will result in 

"@timestamp" : [
   "2021"
],

@@ -122,6 +119,7 @@ export const buildEventsSearchQuery = ({
field: '*',
include_unmapped: true,
},
...docFields,
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@@ -0,0 +1,208 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for moving these relevant tests into their own file!

* partial errors happen correctly
*/
describe('timestamp tests', () => {
describe('Signals generated from events with a timestamp in seconds is converted correctly into the forced ISO8601 format when copying', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

💥 Tests for this formatting bug. Nice addition.

@FrankHassanabad FrankHassanabad merged commit e5944a3 into elastic:master Jun 4, 2021
@FrankHassanabad FrankHassanabad added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 4, 2021
@FrankHassanabad FrankHassanabad deleted the fix-timestamp-source-bug branch June 4, 2021 22:20
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 4, 2021
…urce indexes when the formats are not ISO8601 format (elastic#101349)

## Summary

We have a few bugs where when the source index for detections is not `"strict_date_optional_time"` it is possible that we will misinterpret the format to be epoch milliseconds when it could be epoch seconds or another ambiguous format or blow up when trying to write out the signals index. This fixes it to where we query for the source index format as an ISO8601 and when we copy the date time format we copy it back out as ISO8601 and insert it into the signal index as ISO8601.

See this [gist](https://gist.github.com/FrankHassanabad/f614ec9762d59cd1129b3269f5bae41c) for more details of how this was accidentally introduced when we added support for runtime fields and the general idea of the fix.

* Removes `docvalue_field` and we now only use `fields` in detection engine search requests
* Splits out the timestamp e2e tests into their own file for `timestamps` file
* Adds more tests to ensure we copy what we expect and we are converting to ISO8601 in the signals
* Removes `ts-expect-error` in a lot of areas including tests and then I fix the types and issues once it is removed. 

### Checklist

- [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
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 5, 2021
…urce indexes when the formats are not ISO8601 format (#101349) (#101440)

## Summary

We have a few bugs where when the source index for detections is not `"strict_date_optional_time"` it is possible that we will misinterpret the format to be epoch milliseconds when it could be epoch seconds or another ambiguous format or blow up when trying to write out the signals index. This fixes it to where we query for the source index format as an ISO8601 and when we copy the date time format we copy it back out as ISO8601 and insert it into the signal index as ISO8601.

See this [gist](https://gist.github.com/FrankHassanabad/f614ec9762d59cd1129b3269f5bae41c) for more details of how this was accidentally introduced when we added support for runtime fields and the general idea of the fix.

* Removes `docvalue_field` and we now only use `fields` in detection engine search requests
* Splits out the timestamp e2e tests into their own file for `timestamps` file
* Adds more tests to ensure we copy what we expect and we are converting to ISO8601 in the signals
* Removes `ts-expect-error` in a lot of areas including tests and then I fix the types and issues once it is removed. 

### Checklist

- [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

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 7, 2021
* master: (90 commits)
  Fix UI breaks on providing long search keyword in 'Search Box' (elastic#101385)
  Adds css class to EuiDescriptionListDescription in order to break word on exception details card (elastic#101481)
  [Lens] Increase timings for drag and drop tests (elastic#101380)
  [Lens] Fix editor react error on configuration panel (elastic#101367)
  [Fleet] Move integrations to a separate app (elastic#99848)
  Fix incorrect message displayed on importing Timeline Templates (elastic#101288)
  [Cases] RBAC (elastic#95058)
  [APM] Visual improvements for new APM layout with left navigation (elastic#101360)
  [master] More precise alerts matching (elastic#99820)
  [Lens] Value in legend (elastic#101353)
  Revert "[Reporting] ILM policy for managing reporting indices (elastic#100130)" (elastic#101358)
  [Discover] Fix header row of data grid in Firefox (elastic#101374)
  Add link to advanced setting in Discover (elastic#101154)
  Url service locators (elastic#101045)
  [Timelion] Update the removal message to mention the exact version (elastic#100994)
  [Security Solution][Detection Engine] Test cases for alias failure test cases where we don't copy aliases correctly (elastic#101437)
  [Event Log] Adding `type_id` to saved object array in event log (elastic#100939)
  [Reporting] Add `location.url` info to console message logs (elastic#101427)
  [Security Solutions][Detection Engine] Fixes timestamp bugs within source indexes when the formats are not ISO8601 format (elastic#101349)
  Improve Task Manager instrumentation (elastic#99160)
  ...
@timroes timroes added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Jun 28, 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
auto-backport Deprecated - use backport:version if exact versions are needed release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants