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.

Sign up for GitHub

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

Jump to bottom

[SecuritySolution][Threat Hunting] Fix a couple of field ids for highlighted fields #124941

Merged
merged 2 commits into from
Feb 15, 2022
Merged

[SecuritySolution][Threat Hunting] Fix a couple of field ids for highlighted fields #124941

merged 2 commits into from
Feb 15, 2022

Conversation

janmonschke
Copy link
Contributor

@janmonschke janmonschke commented Feb 8, 2022 •
edited
Loading

Summary

  1. We were using the wrong field id for DNS alerts in the alert flyout. The id should have been dns.question.name instead of dns.query.name.
  2. Instead of showing kibana.alert.rule.description we're supposed to show rule.description for behavior alerts.

This PR fixes these ids and adds test for all of them

Checklist

@janmonschke janmonschke added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 Team:Threat Hunting:Investigations Security Solution Investigations Team labels Feb 8, 2022
@janmonschke janmonschke self-assigned this Feb 8, 2022
@janmonschke janmonschke requested a review from a team as a code owner February 8, 2022 12:58
@elasticmachine
Copy link
Contributor

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

@janmonschke janmonschke changed the title [SecuritySolution][Threat Hunting] Use correct DNS field id for highlighted fields [SecuritySolution][Threat Hunting] Fix a couple of field ids for highlighted fields Feb 8, 2022
@janmonschke
Copy link
Contributor Author

@elasticmachine merge upstream

andrew-goldstein
andrew-goldstein reviewed Feb 9, 2022
View reviewed changes
.../plugins/security_solution/public/common/components/event_details/get_alert_summary_rows.tsx
@@ -101,7 +97,7 @@ function getFieldsByEventCode(
switch (eventCode) {
case EventCode.BEHAVIOR:
return [
{ id: ALERT_RULE_DESCRIPTION, label: ALERTS_HEADERS_RULE_DESCRIPTION },
{ id: 'rule.description', label: ALERTS_HEADERS_RULE_DESCRIPTION },
Copy link
Contributor

Choose a reason for hiding this comment

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

When viewing the same alert in a branch based off main (before the change), the alert description appears in the flyout, because the alert is populated with kibana.alert.rule.description, per the before screenshot below:

before

before

When running this PR branch, the dns.question.name now shows up as expected, but the rule description no longer appears, because the event does not contain rule.description, per the after screenshot below:

after

after

Is it possible that I'm missing a change in my local environment that would populate rule.description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is something that I was also struggling with to understand. From what @paulewing explained to me I understood that kibana.alert.rule.description is not the same as rule.description. I am not sure though, how it gets populated. Maybe Paul has some more insights into that.

andrew-goldstein
andrew-goldstein reviewed Feb 9, 2022
View reviewed changes
...plugins/security_solution/public/common/components/event_details/alert_summary_view.test.tsx
}) as TimelineEventsDetailsItem[]),
{
category: 'dns',
field: 'dns.question.name',
Copy link
Contributor

@andrew-goldstein andrew-goldstein Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does it make sense to also display dns.question.type in the flyout summary for parity with the DNS row renderer, shown in the screenshot below?

dns-row-renderer

CC @paulewing

@janmonschke
Copy link
Contributor Author

@elasticmachine merge upstream

@janmonschke janmonschke mentioned this pull request Feb 14, 2022
Closed
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #15 / machine learning model management trained models for ML user with read-only access renders expanded row content correctly for model with pipelines

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 4.7MB 4.7MB -4.0B

History

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

cc @janmonschke

andrew-goldstein
andrew-goldstein approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @janmonschke!
LGTM 🚀

@janmonschke janmonschke merged commit 8fabaf3 into main Feb 15, 2022
@janmonschke janmonschke deleted the security/fix-dns-field-id branch February 15, 2022 16:20
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 15, 2022
@kibanamachine
Copy link
Contributor

The following labels were identified as gaps in your version labels and will be added automatically:

  • v8.2.0

If any of these should not be on your pull request, please manually remove them.

@kibanamachine kibanamachine mentioned this pull request Feb 15, 2022
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 15, 2022
@janmonschke @kibanamachine
[SecuritySolution][Threat Hunting] Fix a couple of field ids for high…
6689dc4 
…lighted fields (elastic#124941)

* fix: use correct DNS field id

* fix: for behavior alerts we should display rule.description

(cherry picked from commit 8fabaf3)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.1

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Feb 21, 2022
@kibanamachine @janmonschke
[SecuritySolution][Threat Hunting] Fix a couple of field ids for high…
81f308c 
…lighted fields (#124941) (#125688)

* fix: use correct DNS field id

* fix: for behavior alerts we should display rule.description

(cherry picked from commit 8fabaf3)

Co-authored-by: Jan Monschke <jan.monschke@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrew-goldstein andrew-goldstein andrew-goldstein approved these changes

Assignees

@janmonschke janmonschke

Labels
auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Investigations Security Solution Investigations Team v8.1.0 v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@janmonschke @elasticmachine @kibana-ci @kibanamachine @andrew-goldstein