[Security Solution] Handle specific fields in /upgrade/_review upgrade workflow

[jpdjere]

Epics: https://github.com/elastic/security-team/issues/1974 (internal), #174168

Summary

Based on the discussions that took place in #147239, we need to treat different rule fields in different ways in the context of the upgrade workflow.

For each field we must decide:

• Do we need to remove it from the security-rule asset schema?: In cases where we have certainty that they won't ever be included in prebuilt rule assets, the field should be removed from the schema. E.g. actions, response_actions.
  • ✏️ : needs to be removed from schema
  • ➖ : not present in schema
  • ✅ : stays in schema
• Does it need to be removed from the response of /upgrade/_review?: does the field need to be removed of the diffs object that is generated for each rule in the endpoint?
  • ✏️ : needs to be filtered out/removed from diffs object
  • ➖ : not calculated in diff object (won't be calculated if removed from security-rule schema)
  • ✅ : stays in diff object

Field list

Field name	Remove from schema?	Remove from /upgrade/_review?
id	➖	➖
rule_source	➖	➖
immutable	➖	➖
version	✅	✅
revision	➖	➖
enabled	✅	➖
execution_summary	➖	➖
alert_suppression*	✅	✏️ (remove diff from code)
actions	✏️	➖ (remove diff from code)
throttle	✏️	➖ (remove diff from code)
response_actions	✏️	➖
meta	✏️	➖ (remove diff from code)
output_index	✏️	➖
namespace	✏️	➖
alias_purpose	✏️	➖
alias_target_id	✏️	➖
outcome	✏️	➖
created_at	➖	➖
created_by	➖	➖
updated_at	➖	➖
updated_by	➖	➖
author	✅	✅
license	✅	✅
concurrent_searches (IM Rules)	✅	✅
items_per_search (IM Rules)	✅	✅
rule_id	✅	✅
name	✅	✅
tags	✅	✅
description	✅	✅
severity	✅	✅
severity_mapping	✅	✅
risk_score	✅	✅
risk_score_mapping	✅	✅
references	✅	✅
false_positives	✅	✅
threat	✅	✅
note	✅	✅
setup	✅	✅
related_integrations	✅	✅
required_fields	✅	✅
max_signals	✅	✅
building_block_type	✅	✅
from (rule_schedule)	✅	✅
interval (rule_schedule)	✅	✅
exceptions_list*	✅	✅
rule_name_override	✅	✅
timestamp_override	✅	✅
timestamp_override_fallback_disabled	✅	✅
timeline_id (timeline_template)	✅	✅
timeline_title (timeline_template)	✅	✅
index (data_source)	✅	✅
data_view_id (data_source)	✅	✅
query	✅	✅
language	✅	✅
filters	✅	✅
saved_id	✅	✅
machine_learning_job_id (ML Rules)	✅	✅
anomaly_threshold (ML Rules)	✅	✅
threat_filters (IM Rules)	✅	✅
threat_query (IM Rules)	✅	✅
threat_mapping (IM Rules)	✅	✅
threat_language (IM Rules)	✅	✅
threat_index (IM Rules)	✅	✅
threat_indicator_path (IM Rules)	✅	✅
new_terms_fields (New Terms Rules)	✅	✅
history_window_start (New Terms Rules)	✅	✅

Notes on fields

1. exceptions_list: The Endpoint Security rule includes an exception list value, so this update/customization case needs to be handled. (That's the only prebuilt rule with an exception list as of now)
2. enabled: must be part of Prebuilt Asset schema as some important rules have their default value set to true. But it's not part of the diffing logic anyways.

Details

• security-rule asset schema: x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.ts
  • if we omit a property from the schema, it won't be calculated as part of diff in /upgrade/_review
• list of fields that are included in diffs object: x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/logic/diff/calculation/calculate_rule_fields_diff.ts. We can add/remove them here and define which algorithm is used to calculate the diff.

Work left over from this ticket

• Handling fields in the /upgrade/_perform endpoint. All of this needs to be done after the refactoring of the endpoint handler is done. As of now, it always installs the full target version, so the changes needed are not possible now. Moving the work to a separate ticket.
• Making the version, author and license fields visible but read-only in the Three Way Diff component. Addings this TO-DO as a requisite in the Three-Way-Diff component ticket.

[elasticmachine]

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

[elasticmachine]

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

[elasticmachine]

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

[jpdjere]

@approksiu @banderror

I finished evaluating how to handle all the fields in a rule and marked them in the table above.

Just wanted to call your attention to alert_suppression. I decided:

• remove the field from the Prebuilt Rules security-asset schema. The field does not exist in any prebuilt rules and there doesn't seem to be any plans to ever have prebuilt rules come with defined alert_suppresion (similarly to actions)
• the /upgrade/_review endpoint won't calculate the diff for this field.
• the /upgrade/_perform endpoint will handle this field under the hood, updating its value always to the current value, i.e. no changes during update.
• the field will always be hidden in the diff UI

[jpdjere]

@approksiu @banderror

I finished evaluating how to handle all the fields in a rule and marked them in the table above.

Just wanted to call your attention to alert_suppression. I decided:

• remove the field from the Prebuilt Rules security-asset schema. The field does not exist in any prebuilt rules and there doesn't seem to be any plans to ever have prebuilt rules come with defined alert_suppresion (similarly to actions)
• the /upgrade/_review endpoint won't calculate the diff for this field.
• the /upgrade/_perform endpoint will handle this field under the hood, updating its value always to the current value, i.e. no changes during update.
• the field will always be hidden in the diff UI

Update: the field was not removed from the Prebuilt Rules schema since it is extensively used in the code as part of it. We still won't calculate the diff for this field and the /upgrade/_perform endpoint will handle this field under the hood, updating its value always to the current value.

[banderror]

@jpdjere Thanks for the update. I'm still not convinced that we should handle this field in a special way. Why can't we simply enable it in the ThreeWayDiff UI? Especially because it stays in the prebuilt rule asset schema. We can do a quick zoom on that.

[jpdjere]

@banderror We definitely can enable it in the ThreeWayDiff UI; it just doesn't seem necessary since no Prebuilt Detection Rules have an alert_suppression field, and AFAIK there's no plan to have them in the short term.

But given having it as part of the UI does not mean any additional complexity, let's change this decision and include it as part of the UI and the /upgrade/_perform. This will cover us in the future in case the TRaDE team decides to start using this field in future prebuilt rules.

[approksiu]

@jpdjere in the future we might have the suppression fields set for prebuilt rules (depends on additional suppression fields like suppression_enabled).
I support the idea to have it in Diff. Thank you!
cc @banderror

[banderror]

@approksiu @jpdjere ++, and we just got a PR that is related to that: elastic/detection-rules#3986

Let's enable diff calculation for this field and show it in the UI. @jpdjere What do we need to do for that, exactly?

[jpdjere]

@banderror @approksiu

Created a ticket for adding both AlertSuppression and InvestigationFields to the upgrade workflow: #190597

It lists what needs to be done to include the fields in the workflow.

InvestigationFields had been left out since it was added to the rule schema after we had created our diffing logic.

[Mikaayenson]

@banderror We definitely can enable it in the ThreeWayDiff UI; it just doesn't seem necessary since no Prebuilt Detection Rules have an alert_suppression field, and AFAIK there's no plan to have them in the short term.

Just something to think about; now that we've opened our DAC feature to beta, it allows customers to use our logic to import/export rules using our schemas to their stack. In other words, even if we never create prebuilt rules that use the new alert suppression fields, other customers will still be able to. In fact, a recent customer / partner just reached out about adding support to our repo. They've also tested our implementation to confirm it worked for them and their custom rules.

[banderror]

even if we never create prebuilt rules that use the new alert suppression fields, other customers will still be able to

@Mikaayenson Good point, this will be supported - users will be able to customize prebuilt rules via the import endpoint and the DAC tool.