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] Modify threshold rule synthetic signal generation to use data from last hit in bucket #82444

Merged
merged 10 commits into from
Nov 11, 2020

Conversation

madirey
Copy link
Contributor

@madirey madirey commented Nov 3, 2020

Summary

Fixes #77253 by using the top_hits aggregation to return the most recent hit for each bucket which exceeded the given threshold (or by using the returned search results for threshold rules that are not bucketed by any field). This hit is used to populate the synthetic signal(s) that is generated.

This ensures that our signal contains only valid field values, as opposed to potential wildcards or CIDR IP ranges that were used for the original search.

For example, 192.168.0.0/16 (the CIDR ranged used by the rule to search) becomes 192.168.0.16 (the host IP from the most recent event captured on the interval).

Checklist

For maintainers

@madirey madirey added release_note:fix v8.0.0 Feature:Detection Rules Anything related to Security Solution's Detection Rules v7.11.0 labels Nov 3, 2020
@madirey madirey requested review from a team as code owners November 3, 2020 15:17
@madirey madirey changed the title Modify threshold rule synthetic signal generation to use data from first hit [Security Solution][Detections] Modify threshold rule synthetic signal generation to use data from first hit Nov 10, 2020
@madirey madirey changed the title [Security Solution][Detections] Modify threshold rule synthetic signal generation to use data from first hit [Security Solution][Detections] Modify threshold rule synthetic signal generation to use data from last hit in bucket Nov 10, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

const source = {
'@timestamp': new Date().toISOString(),
'@timestamp': get(timestampOverride ?? '@timestamp', hit._source),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this set the threshold signal date time to be the one from the hit?

I ask because previously there has been some debate previously about what a signal's @timestamp should be as some people prefer the source signal timestamp instead of the current date time that the signal is created but the decision so far has been that the @timestamp for a signal should be the current date time stamp and then if a rule has an original_timestamp it would be stored underneath signal.original_time

Threshold is unique in that is synthetic but the @timestamp should still be the timestamp of when the signal is created for consistency. That doesn't mean that we can't change them all as part of

Screen Shot 2020-11-10 at 7 04 55 PM

Copy link
Contributor Author

@madirey madirey Nov 11, 2020

Choose a reason for hiding this comment

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

@FrankHassanabad I thought that too initially, but the signal timestamp is created later. This timestamp is used to populate signal.original_time (it will be the original time of the last event in the timespan...

) and the signal timestamp is created here ( ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, thanks. Yeah, that makes sense. I get it.

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

Code looks clean, checked out the branch and ran through some tests, everything looks 👍

@madirey madirey merged commit f4126ea into elastic:master Nov 11, 2020
@madirey madirey deleted the cidr-house-rules branch November 11, 2020 03:28
madirey added a commit to madirey/kibana that referenced this pull request Nov 11, 2020
…l generation to use data from last hit in bucket (elastic#82444)

* Fix threshold rule synthetic signal generation

* Use top_hits aggregation

* Add timestampOverride

* Account for when threshold.field is not supplied

* Ensure we're getting the last event when threshold.field is not provided

* Add missing import
madirey added a commit that referenced this pull request Nov 12, 2020
…l generation to use data from last hit in bucket (#82444) (#83213)

* Fix threshold rule synthetic signal generation

* Use top_hits aggregation

* Add timestampOverride

* Account for when threshold.field is not supplied

* Ensure we're getting the last event when threshold.field is not provided

* Add missing import
@spong spong added the Feature:Threshold Rule Security Solution Threshold Rule feature label Dec 4, 2020
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 Feature:Threshold Rule Security Solution Threshold Rule feature release_note:fix v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution][Detection Engine] Threshold rules do not create signals with CIDR IP's
4 participants