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

[Security Solution] Can't overwrite actions to [] when importing a rule with "Overwrite existing detection rules with conflicting Rule ID" #118166

Closed
ghost opened this issue Nov 10, 2021 · 11 comments · Fixed by #176166
Closed

[Security Solution] Can't overwrite actions to [] when importing a rule with "Overwrite existing detection rules with conflicting Rule ID" #118166

ghost opened this issue Nov 10, 2021 · 11 comments · Fixed by #176166
Assignees
@maximpn
Labels
8.13 candidate bug Fixes for quality problems that affect the customer experience Feature:Rule Actions Security Solution Detection Rule Actions area Feature:Rule Import/Export Security Solution Detection Rule Import & Export workflow Feature:Rule Management Security Solution Detection Rule Management area fixed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. 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.13.0 v8.14.0

Comments

@ghost
Copy link

ghost commented Nov 10, 2021 •
edited by banderror
Loading

Describe the bug
Actions are still exist when import the rule by selecting "Overwrite existing detection rules with conflicting Rule ID" after upgrade the build from 7.15 to 7.16.0

UPD: see for the details #118166 (comment)

Build Details:
Version: 7.16.0 BC3
Build: 45816
COMMIT: acaa761

Browser Details:
N/A

Preconditions

  1. 7.15 build should be exist.
  2. Rule should be created.
  3. Action should be added in rule

Steps to Reproduce

  1. Navigate to Rules tab of security tab
  2. Export the rule in 7.15 that contains the action.
  3. Upgrade the build to 7.16.0
  4. Do not delete the rule.
  5. Imported the exported rule in 7.16.0 by selecting the "Overwrite existing detection rules with conflicting Rule ID"
  6. Edit the rule and Observe that rule action still exist.

Actual Result
Actions are still exist when import the rule by selecting "Overwrite existing detection rules with conflicting Rule ID" after upgrade the build from 7.15 to 7.16.0

Expected Result
Actions should not be exist when import the rule by selecting "Overwrite existing detection rules with conflicting Rule ID" after upgrade the build from 7.15 to 7.16.0

What's Working

  • This issue is not occurring if we delete the rule and then import the exported rule

What's Not Working

  • N/A

Screen-Shot
7.15.0_rule.zip

import_rule.mp4
@ghost ghost added bug Fixes for quality problems that affect the customer experience triage_needed Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Nov 10, 2021
@elasticmachine
Copy link
Contributor

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

@ghost ghost added the v7.16.0 label Nov 10, 2021
@ghost ghost assigned ghost and MadameSheema and unassigned ghost Nov 10, 2021
@MadameSheema MadameSheema added Team:Security Solution Platform Security Solution Platform Team and removed triage_needed v7.16.0 labels Nov 10, 2021
@MadameSheema MadameSheema removed their assignment Nov 10, 2021
@MadameSheema
Copy link
Member

@yctercero @jethr0null can you please take a look at this issue and decide which should be the expected behaviour as well as in which version should be addressed?

@yctercero
Copy link
Contributor

yctercero commented Nov 10, 2021 •
edited
Loading

To summarize, the the two user scenarios considered here are:

  1. [Existing behavior] import rule with no actions specified --> select overwrite on import modal --> an existing rule with matching rule_id is found --> existing rule is overwritten with imported rule, but actions remain untouched
  2. import rule with no actions specified --> select overwrite on import modal --> an existing rule with matching rule_id is found --> existing rule is overwritten with imported rule and any existing actions are removed

For context, I believe that right now, the following is true:

  • On UPDATE, if actions are undefined or [], rule is updated to have actions of []
  • On PATCH, if actions are not defined, rule actions are left intact

I would assume that a rule with overwrite: true would follow the UPDATE behavior?

@jethr0null
Copy link

Thanks for the summary @yctercero. I feel like I might be missing some context here. Can you help me to understand why option 2 is being proposed? What would the argument be for changing the existing behavior?

@yctercero
Copy link
Contributor

Thanks for the summary @yctercero. I feel like I might be missing some context here. Can you help me to understand why option 2 is being proposed? What would the argument be for changing the existing behavior?

I think the second scenario is being considered here because that would match the behavior for the other params. And I think I'm trying to understand what the user expectation would be here. If I select overwrite I would assume that means that the rule will now be exactly what I imported, not a meshing of the imported and existing rule. But maybe that isn't the right expectation here?

@yctercero
Copy link
Contributor

@jethr0null I could see us taking this as a feature request where a user can specify whether to overwrite rules, actions and exceptions separately? Not sure if the user thinks of the rules and actions as completely separate entities where I might still want to overwrite, but not overwrite my actions.

@yctercero yctercero mentioned this issue Nov 10, 2021
Closed
2 tasks
@yctercero
Copy link
Contributor

Ignore the PR linked right above (#118221 )

@marshallmain
Copy link
Contributor

Related issue - #93342

@peluja1012 peluja1012 added the Team:Detections and Resp Security Detection Response Team label Aug 4, 2022
@elasticmachine
Copy link
Contributor

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

@yctercero
Copy link
Contributor

@peluja1012 I think this is now more under the rules domain as it's a broader question about the rule override behavior?

@yctercero yctercero added Team:Detection Rule Management Security Detection Rule Management Team and removed Team:Security Solution Platform Security Solution Platform Team labels Apr 3, 2023
@banderror
Copy link
Contributor

Yeah, I think we should replace the PATCH semantics with the UPDATE semantics, as @yctercero noted in #118166 (comment) and @marshallmain noted in #93342

@banderror banderror changed the title [Security Solution] Actions are still exist when import the rule by selecting "Overwrite existing detection rules with conflicting Rule ID" after upgrade the build to 7.16.0 [Security Solution] Can't overwrite actions to [] when importing a rule with "Overwrite existing detection rules with conflicting Rule ID" May 5, 2023
@banderror banderror added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Feature:Rule Actions Security Solution Detection Rule Actions area Feature:Rule Management Security Solution Detection Rule Management area Feature:Rule Import/Export Security Solution Detection Rule Import & Export workflow labels May 5, 2023
@banderror banderror removed their assignment May 5, 2023
@maximpn maximpn self-assigned this Feb 5, 2024
@banderror banderror mentioned this issue Feb 8, 2024
Merged
2 tasks
maximpn added a commit that referenced this issue Feb 19, 2024
@maximpn
[Security Solution] Fix not complete existing rule overwrite when imp…
53aaab4 
…orting rules (#176166)

**Fixes: #93342
**Fixes: #118166

## Summary

This PR fixes not complete existing rule overwrite when importing rules.

## Details

When importing a rule and attempting to overwrite an existing rule, if the new rule does not define a field that the existing rule did define then the newly imported rule will include the field from the existing rule. This can cause issues if we want to overwrite a rule with a rule of a different type, e.g. going from saved_query to query we would provide a new rule that doesn't have a saved_id but since saved_id was defined on the old saved_query rule it will be included in the new query rule.

The fix simply swaps out the `patchRules()` for `updateRules()`. Patching rules preserves previous field values if an incoming update doesn't have such fields while updating doesn't do that. The diff in `import_rules_utils.test.ts` looks bigger due to removing unnecessary `else` clause.

### 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
- [x] Ran successfully in Flaky test runner ([basic/essentials license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5166) and [trial/complete tier license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5167))
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Feb 19, 2024
@maximpn
[Security Solution] Fix not complete existing rule overwrite when imp…
d4184b8 
…orting rules (elastic#176166)

**Fixes: elastic#93342
**Fixes: elastic#118166

## Summary

This PR fixes not complete existing rule overwrite when importing rules.

## Details

When importing a rule and attempting to overwrite an existing rule, if the new rule does not define a field that the existing rule did define then the newly imported rule will include the field from the existing rule. This can cause issues if we want to overwrite a rule with a rule of a different type, e.g. going from saved_query to query we would provide a new rule that doesn't have a saved_id but since saved_id was defined on the old saved_query rule it will be included in the new query rule.

The fix simply swaps out the `patchRules()` for `updateRules()`. Patching rules preserves previous field values if an incoming update doesn't have such fields while updating doesn't do that. The diff in `import_rules_utils.test.ts` looks bigger due to removing unnecessary `else` clause.

### 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
- [x] Ran successfully in Flaky test runner ([basic/essentials license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5166) and [trial/complete tier license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5167))

(cherry picked from commit 53aaab4)
@maximpn maximpn added the fixed label Feb 19, 2024
kibanamachine referenced this issue Feb 19, 2024
@kibanamachine @maximpn
[8.13] [Security Solution] Fix not complete existing rule overwrite w…
2bbf6c3 
…hen importing rules (#176166) (#177196)

# Backport

This will backport the following commits from `main` to `8.13`:
- [[Security Solution] Fix not complete existing rule overwrite when
importing rules
(#176166)](#176166)

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

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

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-02-19T13:35:32Z","message":"[Security
Solution] Fix not complete existing rule overwrite when importing rules
(#176166)\n\n**Fixes:
https://github.com/elastic/kibana/issues/93342**\r\n**Fixes:
https://github.com/elastic/kibana/issues/118166**\r\n\r\n##
Summary\r\n\r\nThis PR fixes not complete existing rule overwrite when
importing rules.\r\n\r\n## Details\r\n\r\nWhen importing a rule and
attempting to overwrite an existing rule, if the new rule does not
define a field that the existing rule did define then the newly imported
rule will include the field from the existing rule. This can cause
issues if we want to overwrite a rule with a rule of a different type,
e.g. going from saved_query to query we would provide a new rule that
doesn't have a saved_id but since saved_id was defined on the old
saved_query rule it will be included in the new query rule.\r\n\r\nThe
fix simply swaps out the `patchRules()` for `updateRules()`. Patching
rules preserves previous field values if an incoming update doesn't have
such fields while updating doesn't do that. The diff in
`import_rules_utils.test.ts` looks bigger due to removing unnecessary
`else` clause.\r\n\r\n### Checklist\r\n\r\n- [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\r\n- [x] Ran
successfully in Flaky test runner ([basic/essentials license FTR
tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5166)
and [trial/complete tier license FTR
tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5167))","sha":"53aaab47322fd15ad232d71a1749fd2df8a5dde4","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","impact:medium","Team:Detections
and Resp","Team: SecuritySolution","Feature:Rule
Management","Team:Detection Rule Management","Feature:Rule
Import/Export","v8.13.0","v8.12.2","v8.14.0"],"title":"[Security
Solution] Fix not complete existing rule overwrite when importing
rules","number":176166,"url":"https://github.com/elastic/kibana/pull/176166","mergeCommit":{"message":"[Security
Solution] Fix not complete existing rule overwrite when importing rules
(#176166)\n\n**Fixes:
https://github.com/elastic/kibana/issues/93342**\r\n**Fixes:
https://github.com/elastic/kibana/issues/118166**\r\n\r\n##
Summary\r\n\r\nThis PR fixes not complete existing rule overwrite when
importing rules.\r\n\r\n## Details\r\n\r\nWhen importing a rule and
attempting to overwrite an existing rule, if the new rule does not
define a field that the existing rule did define then the newly imported
rule will include the field from the existing rule. This can cause
issues if we want to overwrite a rule with a rule of a different type,
e.g. going from saved_query to query we would provide a new rule that
doesn't have a saved_id but since saved_id was defined on the old
saved_query rule it will be included in the new query rule.\r\n\r\nThe
fix simply swaps out the `patchRules()` for `updateRules()`. Patching
rules preserves previous field values if an incoming update doesn't have
such fields while updating doesn't do that. The diff in
`import_rules_utils.test.ts` looks bigger due to removing unnecessary
`else` clause.\r\n\r\n### Checklist\r\n\r\n- [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\r\n- [x] Ran
successfully in Flaky test runner ([basic/essentials license FTR
tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5166)
and [trial/complete tier license FTR
tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5167))","sha":"53aaab47322fd15ad232d71a1749fd2df8a5dde4"}},"sourceBranch":"main","suggestedTargetBranches":["8.13","8.12"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.12","label":"v8.12.2","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","branchLabelMappingKey":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/176166","number":176166,"mergeCommit":{"message":"[Security
Solution] Fix not complete existing rule overwrite when importing rules
(#176166)\n\n**Fixes:
https://github.com/elastic/kibana/issues/93342**\r\n**Fixes:
https://github.com/elastic/kibana/issues/118166**\r\n\r\n##
Summary\r\n\r\nThis PR fixes not complete existing rule overwrite when
importing rules.\r\n\r\n## Details\r\n\r\nWhen importing a rule and
attempting to overwrite an existing rule, if the new rule does not
define a field that the existing rule did define then the newly imported
rule will include the field from the existing rule. This can cause
issues if we want to overwrite a rule with a rule of a different type,
e.g. going from saved_query to query we would provide a new rule that
doesn't have a saved_id but since saved_id was defined on the old
saved_query rule it will be included in the new query rule.\r\n\r\nThe
fix simply swaps out the `patchRules()` for `updateRules()`. Patching
rules preserves previous field values if an incoming update doesn't have
such fields while updating doesn't do that. The diff in
`import_rules_utils.test.ts` looks bigger due to removing unnecessary
`else` clause.\r\n\r\n### Checklist\r\n\r\n- [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\r\n- [x] Ran
successfully in Flaky test runner ([basic/essentials license FTR
tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5166)
and [trial/complete tier license FTR
tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5167))","sha":"53aaab47322fd15ad232d71a1749fd2df8a5dde4"}}]}]
BACKPORT-->

Co-authored-by: Maxim Palenov <[email protected]>
@maximpn maximpn mentioned this issue Feb 20, 2024
Merged
maximpn added a commit to maximpn/kibana that referenced this issue Feb 20, 2024
@maximpn
[Security Solution] Fix not complete existing rule overwrite when imp…
7c0e3d3 
…orting rules (elastic#176166)

**Fixes: elastic#93342
**Fixes: elastic#118166

## Summary

This PR fixes not complete existing rule overwrite when importing rules.

## Details

When importing a rule and attempting to overwrite an existing rule, if the new rule does not define a field that the existing rule did define then the newly imported rule will include the field from the existing rule. This can cause issues if we want to overwrite a rule with a rule of a different type, e.g. going from saved_query to query we would provide a new rule that doesn't have a saved_id but since saved_id was defined on the old saved_query rule it will be included in the new query rule.

The fix simply swaps out the `patchRules()` for `updateRules()`. Patching rules preserves previous field values if an incoming update doesn't have such fields while updating doesn't do that. The diff in `import_rules_utils.test.ts` looks bigger due to removing unnecessary `else` clause.

### 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
- [x] Ran successfully in Flaky test runner ([basic/essentials license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5166) and [trial/complete tier license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5167))

(cherry picked from commit 53aaab4)

# Conflicts:
#	x-pack/test/detection_engine_api_integration/basic/tests/export_rules.ts
#	x-pack/test/detection_engine_api_integration/basic/tests/import_rules.ts
#	x-pack/test/detection_engine_api_integration/basic/tests/import_rules_with_overwrite.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/group1/export_rules.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/group10/import_connectors.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/group10/import_export_rules.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/group10/import_rules.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/group10/import_rules_with_overwrite.ts
#	x-pack/test/detection_engine_api_integration/utils/get_rules_as_ndjson.ts
#	x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/assignments/index.ts
#	x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_import_export/trial_license_complete_tier/export_rules.ts
#	x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_import_export/trial_license_complete_tier/import_rules_ess.ts
#	x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_import_export/trial_license_complete_tier/index.ts
#	x-pack/test/security_solution_api_integration/test_suites/detections_response/utils/rules/index.ts
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
@maximpn @fkanout
[Security Solution] Fix not complete existing rule overwrite when imp…
9207d71 
…orting rules (elastic#176166)

**Fixes: elastic#93342
**Fixes: elastic#118166

## Summary

This PR fixes not complete existing rule overwrite when importing rules.

## Details

When importing a rule and attempting to overwrite an existing rule, if the new rule does not define a field that the existing rule did define then the newly imported rule will include the field from the existing rule. This can cause issues if we want to overwrite a rule with a rule of a different type, e.g. going from saved_query to query we would provide a new rule that doesn't have a saved_id but since saved_id was defined on the old saved_query rule it will be included in the new query rule.

The fix simply swaps out the `patchRules()` for `updateRules()`. Patching rules preserves previous field values if an incoming update doesn't have such fields while updating doesn't do that. The diff in `import_rules_utils.test.ts` looks bigger due to removing unnecessary `else` clause.

### 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
- [x] Ran successfully in Flaky test runner ([basic/essentials license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5166) and [trial/complete tier license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5167))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maximpn maximpn

Labels
8.13 candidate bug Fixes for quality problems that affect the customer experience Feature:Rule Actions Security Solution Detection Rule Actions area Feature:Rule Import/Export Security Solution Detection Rule Import & Export workflow Feature:Rule Management Security Solution Detection Rule Management area fixed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. 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.13.0 v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Security Solution] Fix not complete existing rule overwrite when importing rules maximpn/kibana
8 participants
@peluja1012 @jethr0null @maximpn @banderror @yctercero @elasticmachine @MadameSheema @marshallmain