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] RFC for Prebuilt Rules Customization - Milestone 3 #171856

Merged
merged 62 commits into from
Apr 4, 2024

Conversation

jpdjere
Copy link
Contributor

@jpdjere jpdjere commented Nov 23, 2023

Resolves: #171309

Summary

  • Creates an RFC for Milestone 3 of the Prebuilt Rules Customization, including:

    • rule schema changes
    • mappings
    • migration strategy and technical implementation
    • exporting and importing rules
    • schema-related changes needed in endpoints
    • calculation of isCustomized field on endpoints that update/patch rules.
    • additional changes needed to /upgrade/_review and /upgrade/_perform endpoints
    • concrete diff algorithms
    • UI Changes
  • Creates x-pack/plugins/security_solution/docs/rfcs/detection_response folder and adds it to CODEOWNER file, with owners the Detection Engine and Rule Management teams.

@jpdjere jpdjere self-assigned this Nov 23, 2023
@jpdjere jpdjere force-pushed the rfc-prebuilt-rules-customization branch 6 times, most recently from 1d7a67e to d3398fd Compare December 4, 2023 11:29
@jpdjere jpdjere force-pushed the rfc-prebuilt-rules-customization branch from 6a35b46 to b7e5eb3 Compare December 5, 2023 15:29
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.

@jpdjere Here's the first chunk of comments after reviewing "Necessary rule schema changes".

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.

@jpdjere Reviewed:

  • "Migration strategy: perform migration on all write operations of the rules"
  • "Other Rule Management endpoints that will need to be adapted to new schema, but will not need migration logic:"

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.

Reviewed:

  • "Changes needed in rule_monitoring endpoints"
  • "Endpoints that will not need any changes"

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.

@jpdjere Reviewed:

  • "Technical implementation of migration"
  • "Exporting and importing rules"

@jpdjere jpdjere force-pushed the rfc-prebuilt-rules-customization branch from ad46411 to 3e2b9e5 Compare December 14, 2023 11:01
@jpdjere jpdjere force-pushed the rfc-prebuilt-rules-customization branch from d3bcd1d to 41ae3a9 Compare January 5, 2024 13:04
@jpdjere
Copy link
Contributor Author

jpdjere commented Jan 25, 2024

Hi @banderror @marshallmain @dplumlee @nikitaindik

I have finished rewriting what I call Part 1, which is everything from the beginning and up to the subtitle Customizing Prebuilt Rules, and includes everything related to rule schema changes, mapping, migrations, normalization and changes we need in endpoints (changes related to these topics, at least, some more are needed for rule customization).

But I basically rewrote the first part taking @marshallmain suggestion of having a top-level prebuilt object that stores all info related to prebuilt rules and is optional (so custom rules do not have it, and that's how we differentiate it).

I think it led to a much more streamlined data model, and I was able to encapsulate all normalization and migration logic in 4 helper methods, so that by using them, the changes needed in our endpoints are minimal.

So, I think that overall the RFC is in a much better state now, and I would like to move ahead and start breaking this first part into workable tickets. Of course, please take a look whenever you have time, and feedback/corrections are still welcome, but I'll just additionally correct any created tickets if we change something substantial in the RFC.

@banderror
Copy link
Contributor

banderror commented Feb 6, 2024

@jpdjere Could you please squash all the commits in your branch into one and rebase it on top of latest main? You have changes in the code in this PR (I guess you're checking if your ideas from the RFC actually work), and you're already far behind main at this point.

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.

Submitting a portion of comments. Still reviewing the RFC.

@jpdjere jpdjere force-pushed the rfc-prebuilt-rules-customization branch from 9aff922 to c0f2e05 Compare February 8, 2024 10:51
// [... file continues below ...]
```

### Importing rules
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should allow users to set the prebuilt (or external etc if we rename them) fields through the import API, or through any of our existing APIs. Instead I would suggest that we treat the prebuilt fields as "protected" and only allow them to be set when applying an update from a security-rule SO to the rule itself. This limits the edge cases we have to handle here around potentially incorrect values being passed in, like a customized prebuilt rule claiming it's not customized, an incorrect "source updated at" value, a rule that doesn't exist in the prebuilt repo, etc.

We could still allow prebuilt rules to be imported (as custom rules) and receive updates by using the rule_id as the single unique identifier. Prebuilt rule updates could be applied to any rule as long as the rule_id matches, and when the update gets applied we would create the prebuilt field on the rule and compute isCustomized and any other prebuilt sub-fields on the server to ensure they get the correct values. A user could thus import their customized prebuilt rule and immediately link it to the prebuilt object by loading the prebuilt repo and doing the equivalent of "accept current changes" to link the rule to the prebuilt repo while keeping their customizations. We could try to make this a one step process with an option like "automatically link imported rules to prebuilt by rule_id if they exist" or similar if multiple steps are too cumbersome for users.

This aligns with the long term vision for third party DaC as well. For any rule with an external source of truth, we want to have reliable information about the source so we know about what customizations have been made, if any updates are available, etc. That info about the external source is most reliable if we require it to come directly from the source (the security-rule SO).

Copy link
Contributor

Choose a reason for hiding this comment

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

Some updates per today's discussion at the Simplified Protections WG:

@jpdjere made a great point that we need to allow users to specify the version on import in order for upgrades to work correctly. If a user exports a prebuilt rule from one cluster at version 3 and imports it to another cluster where an updated package has been installed with version 4 of that rule, and we import the rule without specifying the version, we won't be able to easily tell if the imported rule is version 4 customized or version 3 un-customized. If we import the rule and treat it as version 4 customized, but it should have been version 3 un-customized, then upgrading the rule to version 5 would require the user to merge "their customizations" that they didn't actually make. If we instead allow the rule to be imported as version 3 un-customized, the upgrade flow is easier as there are no customizations to worry about merge and we can simply fast forward to version 5 when they want to upgrade.

With that case in mind, I think the combination of rule_id + version would be the unique identifier we use on rule import to load and populate the additional prebuilt fields like updated at, isCustomized, etc based on the security-rule assets. With the addition of version, we can pick the right asset from the history instead of always using the latest one as I had imagined previously.

The core requirement that I had in mind when I proposed that the import API should not allow users to set the prebuilt fields was that the prebuilt fields must always be in sync with the associated security-rule asset - we need to do validation at the API boundaries so our application within those boundaries can always assume that the data in those fields is reliable. But maybe it would be convenient to allow users to include prebuilt fields in the ndjson they import to mark which rules should be linked to security-rule assets, and during the import process we could load the prebuilt rules package (if it is not already loaded) and either overwrite or reject rules in the import JSON with prebuilt fields that don't match up with what's in the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marshallmain

But maybe it would be convenient to allow users to include prebuilt fields in the ndjson they import to mark which rules should be linked to security-rule assets

Not sure I understand why we'd need the data from the prebuilt field if we have the rule_id and the version and are fetching the rule asset based on those two values. If the prebuilt field can be completely recalculated from there, why would we allow prebuilt to be sent in the payload?

Copy link
Contributor

Choose a reason for hiding this comment

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

If prebuilt can be passed in the payload then we don't need an extra parameter on the API, separate from the rule import data, to control whether or not we try to link imported rules to prebuilt rule assets - each rule's JSON tells us if it should be linked or not. None of the data like isCustomized or sourceUpdatedAt from the prebuilt object would be copied directly into the created rule, but the existence of the prebuilt object would signal the user's intent to import a rule as a prebuilt rule instead of a custom rule that just coincidentally shares a rule_id and version with a prebuilt rule.

With the ruleSource changes we talked about, if ruleSource.type: 'external' (or ruleSource is missing and immutable: true) then we'd fetch the rest of the ruleSource data from the security rule asset. If ruleSource.type: 'internal (or ruleSource is missing and immutable: false) then we wouldn't need to fetch any other data from the security rule assets. I think with ruleSource.type now the idea is a bit clearer, whereas before we would look for prebuilt but recalculate all the fields in it so the prebuilt data seems redundant (we would just be looking for the existence of the object), with ruleSource we keep ruleSource.type and recalculate the rest of the values if type is external.

@jpdjere jpdjere changed the title [Security Solution] RFC for Prebuilt Rules Customization - Phase 3 [Security Solution] RFC for Prebuilt Rules Customization - Milestone 3 Apr 3, 2024
@jpdjere jpdjere marked this pull request as ready for review April 3, 2024 11:15
@jpdjere jpdjere requested a review from a team as a code owner April 3, 2024 11:15
@jpdjere jpdjere added release_note:skip Skip the PR/issue when compiling release notes docs 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 labels Apr 3, 2024
@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)

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

  • 💔 Build #182461 failed 404c445b3998affa871a99e5c1c85b47cb312cb8
  • 💔 Build #182101 failed 2267a7e808be3244f1a0a07f3bbeb2cbb24cf867
  • 💔 Build #181931 failed 2148c2f9992c214da2905bf842b6ddc8ec6af3ad
  • 💔 Build #181849 failed b7e5eb37c9516534037f43ab7a12f81e942249ac
  • 💔 Build #181386 failed 6a35b46429f5895b8150f1960ae54376ede5b2c2

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

cc @jpdjere

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.

...and after 2800 lines and 364 comments we can finally call it a day 😅

Huge effort and incredibly thorough technical design document as a result, answering the questions raised in the RFC ticket. This RFC allowed us to plan #174168 and move forward.

Thank you @jpdjere! Let's merge it! 🚀 🚀 🚀

Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@jpdjere thank you for making this huge effort addressing comments, discussing trade offs and polishing the RFC 👍

I had the last check and it looks good to me, approving it 🚀

@jpdjere
Copy link
Contributor Author

jpdjere commented Apr 4, 2024

Thanks for the patience for reviewing, correcting and giving feedback! ❤️ @banderror @marshallmain @maximpn @approksiu and @nikitaindik

Looking forward to finally moving ahead with this epic.

@jpdjere jpdjere merged commit de25d7c into elastic:main Apr 4, 2024
34 checks passed
@kibanamachine kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting docs 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.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Write an RFC for customizing prebuilt rules
9 participants