-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Allow to udate rule version for prebuilt rules #139287
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
69b45d2
to
0d75ffd
Compare
0d75ffd
to
fa07fc2
Compare
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts
Outdated
Show resolved
Hide resolved
fa07fc2
to
d3e11f0
Compare
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts
Outdated
Show resolved
Hide resolved
d3e11f0
to
d379927
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked out, reviewed the changes, and tested locally. LGTM 👍
Tested the fix twice in two different ways:
- by upgrading from 8.4 to 8.5 while having some prebuilt rules installed in 8.4
- by bumping the version in a filesystem rule
Left a few minor comments/nits.
@xcrzx do you think it's possible to write an integration test that would cover the logic of updating versions of prebuilt rules when addPrepackedRulesRoute
is called?
x-pack/plugins/security_solution/server/lib/detection_engine/rules/patch_rules.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts
Outdated
Show resolved
Hide resolved
// increment the version of mutable rules by 1. | ||
version: | ||
params.version ?? existingParams.immutable | ||
? existingParams.version | ||
: shouldUpdateVersion(params) | ||
? existingParams.version + 1 | ||
: existingParams.version, | ||
exceptionsList: params.exceptions_list ?? existingParams.exceptionsList, | ||
version: nextParams.version | ||
? nextParams.version | ||
: shouldUpdateVersion(nextParams, existingParams) | ||
? existingParams.version + 1 | ||
: existingParams.version, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like currently, our users are able to modify the rule version of prebuilt rules via CRUD endpoints. I think we need to create some follow-up tickets for disallowing that. Version updates should be done internally and controlled by the app on the server side.
Also, I believe this logic of incrementing the version should move to the routes like the add_prepackaged_rules_route.ts
one. Here in the convertPatchAPIToInternalSchema
it would become version: nextParams.version ?? existingParams.version
. We can leave it as is in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we should disallow users to modify prebuilt rules via API. But given that we're working actively on the prebuilt rule customization epic and the immutability flag will be removed, probably it's not worth spending time to add extra validation to the routes 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And as for the version increment logic, I hope we'll remove it once this task is finished.
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/schemas/rule_converters.test.ts
Outdated
Show resolved
Hide resolved
d379927
to
f4bd7b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect! Thank you for addressing the comments.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @xcrzx |
(cherry picked from commit 7429f78)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
Resolves: #139095
Resolves: #138876
Summary
Fixed logic that prevented rule version update for prebuilt rules and caused the rule upgrade banner to persist on the page.
Screen.Recording.2022-08-23.at.13.53.10.mov
Version update logic before:
After:
Now, if a new version is passed with the rule params, it gets applied unconditionally. As a result, rule converters are no more aware of the rule immutability concept. Therefore, prebuilt rule immutability should be enforced on the business logic layer where needed.
How to test
x-pack/plugins/security_solution/server/lib/detection_engine/rules/prepackaged_rules
, modify a rule: updatename
,risk_score
and any other fields, and bump up theversion
.Rules
page. The update callout should be visible on top.Update 1 Elastic prebuilt rule
button.