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] Fixes data normalization in diff algorithms for threat and rule_schedule fields #200105

Merged
merged 8 commits into from
Nov 18, 2024

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Nov 13, 2024

Fixes #199629

Summary

Fixes the data normalization we do before comparison for the threat and rule_schedule fields so that they align with our prebuilt rule specs. Specifically:

  • Trims any extra optional nested fields in the threat field that were left as empty arrays
  • Removes the logic to use the from value in the meta field if it existed, so that we can normalize the time strings for rule_schedule

These errors were occurring when a rule was saved via the Rule Editing form in the UI and extra fields were added in the update API call. This PR makes the diff algorithms more robust against different field values that are represented differently but are logically the same.

This extra data added in the Rule Edit UI form was also causing rules to appear as modified when saved from the form, even if no fields had been modified.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

@dplumlee dplumlee added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes v9.0.0 Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area backport:version Backport to applied version labels v8.17.0 v8.16.1 labels Nov 13, 2024
@dplumlee dplumlee self-assigned this Nov 13, 2024
@banderror banderror added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Nov 14, 2024
@dplumlee dplumlee marked this pull request as ready for review November 14, 2024 18:43
@dplumlee dplumlee requested review from a team as code owners November 14, 2024 18:43
@dplumlee dplumlee requested a review from dhurley14 November 14, 2024 18:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@dplumlee dplumlee requested a review from banderror November 14, 2024 18:54
@dplumlee dplumlee force-pushed the fields-marked-as-modified-fix branch from e3eb3cc to ca33625 Compare November 15, 2024 15:20
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Code changes LGTM 👍

Tested the PR locally and everything worked correctly.

I started with testing it with the feature flag turned OFF:

  • Adding one or more actions to a prebuilt rule via the Rule Editing page doesn't mark it as customized. It stays non-customized.
  • Removing actions from a prebuilt rule via the Rule Editing page doesn't mark it as customized. It stays non-customized.
  • Adding actions to a prebuilt rule via the bulk add rule actions menu doesn't mark it as customized. It stays non-customized.

So we should be good with fixing it in 8.16.1 and 8.17.0 🎉

Then, I turned the feature flag ON and tested:

  • Adding actions to a prebuilt rule via the Rule Editing page still doesn't mark it as customized. It stays non-customized.
  • Removing actions from a prebuilt rule via the Rule Editing page still doesn't mark it as customized. It stays non-customized.
  • Adding actions to a prebuilt rule via the bulk add rule actions menu still doesn't mark it as customized. It stays non-customized.
  • Editing rule name, description, or query via the Rule Editing page does mark the rule as customized. Editing and reverting it back to the original values marks it back as non-customized.
  • Bulk adding index patterns to prebuilt rules marks them as customized. Bulk removing these index patterns marks them back as non-customized.
  • Same for bulk editing tags.

The fix looks very solid, thank you @dplumlee for quickly opening this PR! 🙌

I hope it will make it to v8.16.1.

@dplumlee dplumlee enabled auto-merge (squash) November 18, 2024 18:58
@dplumlee dplumlee merged commit a8fd0c9 into elastic:main Nov 18, 2024
43 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11899937451

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 6212 6213 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 13.4MB 13.4MB -16.0B

History

cc @dplumlee

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 18, 2024
…threat` and `rule_schedule` fields (elastic#200105)

**Fixes elastic#199629

## Summary

Fixes the data normalization we do before comparison for the `threat`
and `rule_schedule` fields so that they align with our prebuilt rule
specs. Specifically:

- Trims any extra optional nested fields in the `threat` field that were
left as empty arrays
- Removes the logic to use the `from` value in the `meta` field if it
existed, so that we can normalize the time strings for `rule_schedule`

These errors were occurring when a rule was saved via the Rule Editing
form in the UI and extra fields were added in the update API call. This
PR makes the diff algorithms more robust against different field values
that are represented differently but are logically the same.

This extra data added in the Rule Edit UI form was also causing rules to
appear as modified when saved from the form, even if no fields had been
modified.

### Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

- [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

(cherry picked from commit a8fd0c9)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 18, 2024
…threat` and `rule_schedule` fields (elastic#200105)

**Fixes elastic#199629

## Summary

Fixes the data normalization we do before comparison for the `threat`
and `rule_schedule` fields so that they align with our prebuilt rule
specs. Specifically:

- Trims any extra optional nested fields in the `threat` field that were
left as empty arrays
- Removes the logic to use the `from` value in the `meta` field if it
existed, so that we can normalize the time strings for `rule_schedule`

These errors were occurring when a rule was saved via the Rule Editing
form in the UI and extra fields were added in the update API call. This
PR makes the diff algorithms more robust against different field values
that are represented differently but are logically the same.

This extra data added in the Rule Edit UI form was also causing rules to
appear as modified when saved from the form, even if no fields had been
modified.

### Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

- [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

(cherry picked from commit a8fd0c9)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.16
8.x

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

Questions ?

Please refer to the Backport tool documentation

@dplumlee dplumlee deleted the fields-marked-as-modified-fix branch November 18, 2024 19:53
kibanamachine added a commit that referenced this pull request Nov 18, 2024
… for `threat` and `rule_schedule` fields (#200105) (#200646)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Fixes data normalization in diff algorithms for
`threat` and `rule_schedule` fields
(#200105)](#200105)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"56367316+dplumlee@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-11-18T19:48:14Z","message":"[Security
Solution] Fixes data normalization in diff algorithms for `threat` and
`rule_schedule` fields (#200105)\n\n**Fixes
https://github.com/elastic/kibana/issues/199629**\r\n\r\n##
Summary\r\n\r\nFixes the data normalization we do before comparison for
the `threat`\r\nand `rule_schedule` fields so that they align with our
prebuilt rule\r\nspecs. Specifically:\r\n\r\n- Trims any extra optional
nested fields in the `threat` field that were\r\nleft as empty
arrays\r\n- Removes the logic to use the `from` value in the `meta`
field if it\r\nexisted, so that we can normalize the time strings for
`rule_schedule`\r\n\r\nThese errors were occurring when a rule was saved
via the Rule Editing\r\nform in the UI and extra fields were added in
the update API call. This\r\nPR makes the diff algorithms more robust
against different field values\r\nthat are represented differently but
are logically the same.\r\n\r\nThis extra data added in the Rule Edit UI
form was also causing rules to\r\nappear as modified when saved from the
form, even if no fields had been\r\nmodified.\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nCheck the PR satisfies following conditions.
\r\n\r\nReviewers should verify this PR satisfies this list as
well.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"a8fd0c95148ab42411e5ad8e6a65df0634f67dbe","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","impact:high","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","backport:version","v8.17.0","v8.16.1"],"title":"[Security
Solution] Fixes data normalization in diff algorithms for `threat` and
`rule_schedule`
fields","number":200105,"url":"https://github.com/elastic/kibana/pull/200105","mergeCommit":{"message":"[Security
Solution] Fixes data normalization in diff algorithms for `threat` and
`rule_schedule` fields (#200105)\n\n**Fixes
https://github.com/elastic/kibana/issues/199629**\r\n\r\n##
Summary\r\n\r\nFixes the data normalization we do before comparison for
the `threat`\r\nand `rule_schedule` fields so that they align with our
prebuilt rule\r\nspecs. Specifically:\r\n\r\n- Trims any extra optional
nested fields in the `threat` field that were\r\nleft as empty
arrays\r\n- Removes the logic to use the `from` value in the `meta`
field if it\r\nexisted, so that we can normalize the time strings for
`rule_schedule`\r\n\r\nThese errors were occurring when a rule was saved
via the Rule Editing\r\nform in the UI and extra fields were added in
the update API call. This\r\nPR makes the diff algorithms more robust
against different field values\r\nthat are represented differently but
are logically the same.\r\n\r\nThis extra data added in the Rule Edit UI
form was also causing rules to\r\nappear as modified when saved from the
form, even if no fields had been\r\nmodified.\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nCheck the PR satisfies following conditions.
\r\n\r\nReviewers should verify this PR satisfies this list as
well.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"a8fd0c95148ab42411e5ad8e6a65df0634f67dbe"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200105","number":200105,"mergeCommit":{"message":"[Security
Solution] Fixes data normalization in diff algorithms for `threat` and
`rule_schedule` fields (#200105)\n\n**Fixes
https://github.com/elastic/kibana/issues/199629**\r\n\r\n##
Summary\r\n\r\nFixes the data normalization we do before comparison for
the `threat`\r\nand `rule_schedule` fields so that they align with our
prebuilt rule\r\nspecs. Specifically:\r\n\r\n- Trims any extra optional
nested fields in the `threat` field that were\r\nleft as empty
arrays\r\n- Removes the logic to use the `from` value in the `meta`
field if it\r\nexisted, so that we can normalize the time strings for
`rule_schedule`\r\n\r\nThese errors were occurring when a rule was saved
via the Rule Editing\r\nform in the UI and extra fields were added in
the update API call. This\r\nPR makes the diff algorithms more robust
against different field values\r\nthat are represented differently but
are logically the same.\r\n\r\nThis extra data added in the Rule Edit UI
form was also causing rules to\r\nappear as modified when saved from the
form, even if no fields had been\r\nmodified.\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nCheck the PR satisfies following conditions.
\r\n\r\nReviewers should verify this PR satisfies this list as
well.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"a8fd0c95148ab42411e5ad8e6a65df0634f67dbe"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
kibanamachine added a commit that referenced this pull request Nov 18, 2024
…s for &#x60;threat&#x60; and &#x60;rule_schedule&#x60; fields (#200105) (#200645)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[Security Solution] Fixes data normalization in diff algorithms for
&#x60;threat&#x60; and &#x60;rule_schedule&#x60; fields
(#200105)](#200105)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"56367316+dplumlee@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-11-18T19:48:14Z","message":"[Security
Solution] Fixes data normalization in diff algorithms for `threat` and
`rule_schedule` fields (#200105)\n\n**Fixes
https://github.com/elastic/kibana/issues/199629**\r\n\r\n##
Summary\r\n\r\nFixes the data normalization we do before comparison for
the `threat`\r\nand `rule_schedule` fields so that they align with our
prebuilt rule\r\nspecs. Specifically:\r\n\r\n- Trims any extra optional
nested fields in the `threat` field that were\r\nleft as empty
arrays\r\n- Removes the logic to use the `from` value in the `meta`
field if it\r\nexisted, so that we can normalize the time strings for
`rule_schedule`\r\n\r\nThese errors were occurring when a rule was saved
via the Rule Editing\r\nform in the UI and extra fields were added in
the update API call. This\r\nPR makes the diff algorithms more robust
against different field values\r\nthat are represented differently but
are logically the same.\r\n\r\nThis extra data added in the Rule Edit UI
form was also causing rules to\r\nappear as modified when saved from the
form, even if no fields had been\r\nmodified.\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nCheck the PR satisfies following conditions.
\r\n\r\nReviewers should verify this PR satisfies this list as
well.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"a8fd0c95148ab42411e5ad8e6a65df0634f67dbe","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","impact:high","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","backport:version","v8.17.0","v8.16.1"],"title":"[Security
Solution] Fixes data normalization in diff algorithms for `threat` and
`rule_schedule`
fields","number":200105,"url":"https://github.com/elastic/kibana/pull/200105","mergeCommit":{"message":"[Security
Solution] Fixes data normalization in diff algorithms for `threat` and
`rule_schedule` fields (#200105)\n\n**Fixes
https://github.com/elastic/kibana/issues/199629**\r\n\r\n##
Summary\r\n\r\nFixes the data normalization we do before comparison for
the `threat`\r\nand `rule_schedule` fields so that they align with our
prebuilt rule\r\nspecs. Specifically:\r\n\r\n- Trims any extra optional
nested fields in the `threat` field that were\r\nleft as empty
arrays\r\n- Removes the logic to use the `from` value in the `meta`
field if it\r\nexisted, so that we can normalize the time strings for
`rule_schedule`\r\n\r\nThese errors were occurring when a rule was saved
via the Rule Editing\r\nform in the UI and extra fields were added in
the update API call. This\r\nPR makes the diff algorithms more robust
against different field values\r\nthat are represented differently but
are logically the same.\r\n\r\nThis extra data added in the Rule Edit UI
form was also causing rules to\r\nappear as modified when saved from the
form, even if no fields had been\r\nmodified.\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nCheck the PR satisfies following conditions.
\r\n\r\nReviewers should verify this PR satisfies this list as
well.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"a8fd0c95148ab42411e5ad8e6a65df0634f67dbe"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200105","number":200105,"mergeCommit":{"message":"[Security
Solution] Fixes data normalization in diff algorithms for `threat` and
`rule_schedule` fields (#200105)\n\n**Fixes
https://github.com/elastic/kibana/issues/199629**\r\n\r\n##
Summary\r\n\r\nFixes the data normalization we do before comparison for
the `threat`\r\nand `rule_schedule` fields so that they align with our
prebuilt rule\r\nspecs. Specifically:\r\n\r\n- Trims any extra optional
nested fields in the `threat` field that were\r\nleft as empty
arrays\r\n- Removes the logic to use the `from` value in the `meta`
field if it\r\nexisted, so that we can normalize the time strings for
`rule_schedule`\r\n\r\nThese errors were occurring when a rule was saved
via the Rule Editing\r\nform in the UI and extra fields were added in
the update API call. This\r\nPR makes the diff algorithms more robust
against different field values\r\nthat are represented differently but
are logically the same.\r\n\r\nThis extra data added in the Rule Edit UI
form was also causing rules to\r\nappear as modified when saved from the
form, even if no fields had been\r\nmodified.\r\n\r\n\r\n\r\n###
Checklist\r\n\r\nCheck the PR satisfies following conditions.
\r\n\r\nReviewers should verify this PR satisfies this list as
well.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests
changed","sha":"a8fd0c95148ab42411e5ad8e6a65df0634f67dbe"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
…threat` and `rule_schedule` fields (elastic#200105)

**Fixes elastic#199629

## Summary

Fixes the data normalization we do before comparison for the `threat`
and `rule_schedule` fields so that they align with our prebuilt rule
specs. Specifically:

- Trims any extra optional nested fields in the `threat` field that were
left as empty arrays
- Removes the logic to use the `from` value in the `meta` field if it
existed, so that we can normalize the time strings for `rule_schedule`

These errors were occurring when a rule was saved via the Rule Editing
form in the UI and extra fields were added in the update API call. This
PR makes the diff algorithms more robust against different field values
that are represented differently but are logically the same.

This extra data added in the Rule Edit UI form was also causing rules to
appear as modified when saved from the form, even if no fields had been
modified.



### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels bug Fixes for quality problems that affect the customer experience Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.16.1 v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Rules mistakenly marked as customized
5 participants