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

[Docs]Updates detections API #70

Merged
merged 14 commits into from
Jul 26, 2020

Conversation

benskelker
Copy link
Contributor

@benskelker benskelker commented Jul 21, 2020

Adds new 7.9 rule fields and objects to the detections API.

Create rule preview

Signals endpoint preview

Update rule preview

Reviewers, please ignore any BEN ... in the files, this is just to remind me to add links when other stuff is ready (docs won't build when there are broken links). Also, no need to worry about terminology (signals vs alerts), we'll clean that up in a separate PR before the 7.9 release.

@benskelker benskelker linked an issue Jul 21, 2020 that may be closed by this pull request
@benskelker benskelker added the v7.9.0 Features in the 7.9 Release label Jul 21, 2020
Comment on lines 10 to 12
* Threshold rules: Searches the defined indices and creates an alert when the
number of times the specified event field is matched during a single execution
meets the threshold.
Copy link
Contributor

@dhurley14 dhurley14 Jul 22, 2020

Choose a reason for hiding this comment

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

Not a professional but could we switch the phrase "meets the threshold" and "matched during a single execution"? The sentence reads a little bit easier for me if they are swapped. The resulting description could be:

"Searches the defined indices and creates an alert when the number of times the specified event meets the threshold during a single execution."?

What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

++, It may be worth working in that results are grouped/bucketed/aggregated by the specified event field, and that a detection alert will be created for each bucket that surpasses the 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.

Thanks - @spong @dhurley14
I tried explaining this more generally so users who don't care/know about ES aggregations understand. Let me know if you have more suggestions or corrections. Thanks

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.

LGTM! Thanks @benskelker!

Comment on lines +172 to +175
|building_block_type |String |Determines if the rule acts as a building block.
By default, building-block alerts are not displayed in the UI. These rules are
used as a foundation for other rules that do generate alerts. Its value must be
`default`. For more information, see BEN - add link later.
Copy link
Member

Choose a reason for hiding this comment

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

@benskelker - I was chatting with @MikePaquette and he said he may have a good example or two for us to reference when speaking about Building Block Rules, so stay tuned 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much needed :)

Comment on lines 283 to 285
|timestamp_override |String |Determines the time field used to query indices.
When unspecified, rules query the `@timestamp` field. The source field
must an {es} date data type.
Copy link
Member

Choose a reason for hiding this comment

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

Similarly there is also the rule_name_override field (added in elastic/kibana#70288), which will take a string field and if the source event has a value for that field, will use that as the signal.rule.name when creating the detection alert. This is useful for detection rules whose sole purpose is bringing in external alerts into the detection engine, as this allows the user to expose a bit more information front and center on the alerts table.

We use this internally within the Elastic Endpoint rule to map the message field to Rule Name, as this contains details of the alert like Malware Prevention Alert.

Copy link
Contributor Author

@benskelker benskelker Jul 23, 2020

Choose a reason for hiding this comment

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

I'm adding this without too much background info. But your example is great, and I'll explain the idea when I document the UI - so thanks!

Comment on lines +268 to +271
* `field` (string, required): Source event field used to override the default
`risk_score`. This field must be an integer.
* `operator` (string, required): Must be `equals`.
* `value`(string, required): Must be an empty string (`""`).
Copy link
Member

Choose a reason for hiding this comment

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

Just a heads up -- I'm fixing some bugs around this area of the code, and will probably refine this structure a bit to clean up the API. It's looking like only field will be required, and we'll also add a risk_score prop as well (like the severity structure below) for future support of mapping specific values to specific a risk_score.

This is good for now though, and I'll ping you on that PR with changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great - thanks

"risk_score": 30,
"rule_id": "liv-win-ser-logins",
"severity": "low",
"severity_mapping": [ <2>
Copy link
Member

Choose a reason for hiding this comment

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

I'll need to verify the compatibility constraints of using the severity/risk_score mappings with Threshold Rules. I believe that for this mapping to be successful, any field used within the mapping will also have to be referenced in the rule's query so that there exists a unique value per bucket that can then be used for the mapping (otherwise it will fall back to the default value since the field will be missing).

Will keep you posted on this one 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh ok. If so, we'll document the constraint in the UI section, and I'll update the example.

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.

Noticed the rule_name_override field is missing, but other than that LGTM -- thanks @benskelker!

As mentioned in the other comments, will keep you posted on the results/changes from my severity/risk_score bugfixing adventures 🙂

@benskelker
Copy link
Contributor Author

@spong @dhurley14 I updated the Update rule section as well. Sorry, should of done this before requesting a review. Can you take a look. Thanks

Copy link
Contributor

@narcher7 narcher7 left a comment

Choose a reason for hiding this comment

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

A few small suggestions, otherwise, LGTM


* Query-based rules, which search the defined indices and creates a signal when
* Query rules: Searches the defined indices and creates an alert when
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we bold query and thershold?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not suggesting we bold these terms every time they are presented, just the first time in this bulleted list so readers know to look at for them throughout the rest of the doc.

+
For example, if the threshold `field` is `source.ip` and its `value` is `10`, an
alert is generated for every source IP address that appears in at least 10 of
the rule's search results. If you're interested, see
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete, "If you're interested."

* `1h`: Every hour
* `1d`: Every day
* `7d`: Every week

|Yes, when actions are used to send notifications.
Required when `actions` are used to send notifications.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change from passive to active. "Required when actions send notifications."

indices, see:
The signals endpoint is for retrieving, aggregating, and updating detection
alerts. For detailed information on how to retrieve and aggregate results from
the indices, see:

* {ref}/getting-started-search.html[Start searching]
Copy link
Contributor

Choose a reason for hiding this comment

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

"Start searching" seems general? Is this for site search? Searching through signals?

@benskelker benskelker merged commit ee87d96 into elastic:master Jul 26, 2020
@benskelker benskelker deleted the 7.9-rules-api-updates branch July 26, 2020 13:09
benskelker added a commit to benskelker/security-docs that referenced this pull request Jul 26, 2020
* adds new rule fields

* makes things more readable??

* updates terminology where possible

* typos and stuff

* more terminology changes

* corrections and new eample

* updates-signals-endpoint

* missing comma

* corrections after reiew

* typo

* wording and typos

* updates update rule endpoint

* terminology

* corrections after review
benskelker added a commit to benskelker/security-docs that referenced this pull request Jul 26, 2020
* adds new rule fields

* makes things more readable??

* updates terminology where possible

* typos and stuff

* more terminology changes

* corrections and new eample

* updates-signals-endpoint

* missing comma

* corrections after reiew

* typo

* wording and typos

* updates update rule endpoint

* terminology

* corrections after review
benskelker added a commit that referenced this pull request Jul 26, 2020
* adds new rule fields

* makes things more readable??

* updates terminology where possible

* typos and stuff

* more terminology changes

* corrections and new eample

* updates-signals-endpoint

* missing comma

* corrections after reiew

* typo

* wording and typos

* updates update rule endpoint

* terminology

* corrections after review
benskelker added a commit that referenced this pull request Jul 26, 2020
* adds new rule fields

* makes things more readable??

* updates terminology where possible

* typos and stuff

* more terminology changes

* corrections and new eample

* updates-signals-endpoint

* missing comma

* corrections after reiew

* typo

* wording and typos

* updates update rule endpoint

* terminology

* corrections after review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v7.9.0 Features in the 7.9 Release
Projects
None yet
4 participants