[Security Solution] Create Map of upgradable rule fields by type #190128
Conversation
jpdjere
changed the title
jpdjere
added
release_note:skip
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
| // the type of TypeSpecificFields is actually z.ZodEffects, and therefore does | ||
| // not have access to an options property. We need to transform it back into | ||
| // a z.ZodDiscriminatedUnion, accessing its underlying schema. | ||
| const TypeSpecificFieldsSchema = TypeSpecificFields._def.schema; |
There was a problem hiding this comment.
If I understand correctly, we have a zod transform that removes response_actions from certain rule types. Therefore, I expect response_actions not to be part of UPGRADABLE_RULES_FIELDS_BY_TYPE_MAP. However, _def.schema actually accesses the initial definitions where response_actions are included so they become upgradeable fields.
There was a problem hiding this comment.
I think what you need is to use omit instead of relying on transform:
export type TypeSpecificFields = z.infer<typeof TypeSpecificFields>;
export const TypeSpecificFields = z.discriminatedUnion('type', [
EqlRuleCreateFields,
QueryRuleCreateFields.omit({ response_actions: true }),
SavedQueryRuleCreateFields.omit({ response_actions: true }),
ThresholdRuleCreateFields,
ThreatMatchRuleCreateFields,
MachineLearningRuleCreateFields,
NewTermsRuleCreateFields,
EsqlRuleCreateFields,
]);There was a problem hiding this comment.
I expect response_actions not to be part of UPGRADABLE_RULES_FIELDS_BY_TYPE_MAP
Yes, thanks for that catch. It indeed includes response_actions, which is exactly what I'm trying to prevent.
The solution you proposed above would be the easiest way forward, and would solve the issue with the response_action field being present in the Map, but the reason we initially went with the more complex Zod .transform() is because we discussed with @banderror that we want to avoid having and additional place where the rule types are listed, and which needs to be modified if a new rule type is added.
I think it makes sense: when ES|QL was added, and the Detection Engine team added the functionality for a corresponding security rule, but we did not go through our codebase to check where rule types where listed and if we needed to take some action.
But, yeah, not sure how strict we want/need to be with this decision. I'll try to find another way of generating the Map without the response_action field using the output of the transform.
One possibility is just filtering out unwanted fields from the type specific fields within the function that creates the map. Something like:
const TYPE_SPECIFIC_FIELDS_TO_OMIT = new Set(['response_actions']);
function createUpgradableRuleFieldsByTypeMap() {
const baseFields = Object.keys(
BaseCreateProps.omit(BASE_PROPS_REMOVED_FROM_PREBUILT_RULE_ASSET).shape
);
return new Map(
TypeSpecificFieldsSchema.options.map((option) => {
const typeName = option.shape.type.value;
const typeSpecificFields = Object.keys(option.shape).filter(field => !TYPE_SPECIFIC_FIELDS_TO_OMIT.has(field));
return [typeName, [...baseFields, ...typeSpecificFields]];
})
);
}And also use TYPE_SPECIFIC_FIELDS_TO_OMIT in the transform() function somehow, to avoid repeating the list of fields to omit.
I'll give that a try, otherwise we can fallback to recreating the discriminated union as you proposed above.
There was a problem hiding this comment.
@xcrzx and FYI @banderror
OK, the apporach I proposed above is technically feasible, and we are still able to generate the derived types without listing all the rule types.
I pushed the changes. Take a look and let me know what you think.
There was a problem hiding this comment.
It looks like the transformation now turns all type-specific props into Record<string, any>:
There was a problem hiding this comment.
I think it makes sense: when ES|QL was added, and the Detection Engine team added the functionality for a corresponding security rule, but we did not go through our codebase to check where rule types where listed and if we needed to take some action.
Can we use the power of Typescript to ensure those types are always in sync?
import type { IsEqual } from 'type-fest';
// Make sure the type-specific fields contain all the same rule types as the type-specific rule params
// Here will be a type error if the types are not equal
const areTypesEqual: IsEqual<
typeof TypeSpecificRuleParams._type.type,
typeof TypeSpecificFields._type.type
> = true;There was a problem hiding this comment.
Another option, just use tests:
expect(TypeSpecificRuleParams._type.type).toEqual(TypeSpecificFields._type.type);
|
@xcrzx Thanks for all the suggestions. I applied both checks for extra security, the static TS check and an additional test to the test suite. Type behaves as it should now:
|
...solution/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.ts
Outdated
Show resolved
Hide resolved
...solution/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.ts
Outdated
Show resolved
Hide resolved
...solution/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.ts
Outdated
Show resolved
Hide resolved
| @@ -83,3 +105,31 @@ export const PrebuiltRuleAsset = BaseCreateProps.omit(BASE_PROPS_REMOVED_FROM_PR | |||
| version: RuleVersion, | |||
There was a problem hiding this comment.
Rule version should also be part of upgradeable rule fields.
There was a problem hiding this comment.
@xcrzx Not sure I understand why, can you elaborate?
From my perspective, version should be returned from the upgrade/_review endpoint as part of the diff (as it currently is), but shouldn't be accepted by the upgrade/_perform endpoint as part of the fields object. We pass it separately for concurrency control reasons, and it should be always picked from the target version by the endpoint handler.
There was a problem hiding this comment.
By upgradeable I meant that the version field can be upgraded to the target version. But in the context of the upgrade/_perform API request, I agree that it shouldn’t be part of the fields structure. So it really depends on how the upgradeable field list is intended to be used.
...solution/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.ts
Outdated
Show resolved
Hide resolved
| // Filter out type-specific fields that should not be part of the upgradable fields | ||
| (field) => !specificTypesToOmit.includes(field) | ||
| ); | ||
| return [typeName, [...baseFields, ...typeSpecificFields]]; |
There was a problem hiding this comment.
Another thought, maybe we should derive the upgradeable fields from the PrebuiltRuleAsset schema minus rule_id? Instead of composing it from base + type-specific fields.
There was a problem hiding this comment.
The reason I don't start of directly from PrebuiltRuleAsset is because it has a type of ZodIntersection, which does not have the options or shape operands that Zod discriminated unions have.
Also, if I wanna do something like:
PrebuiltRuleAsset.transform((data) => { ...that data is an object of only the properties common for all types (type-specific props are missing)
| import { getPrebuiltRuleMock, getPrebuiltThreatMatchRuleMock } from './prebuilt_rule_asset.mock'; | ||
| import { TypeSpecificCreateProps } from '@kbn/security-solution-plugin/common/api/detection_engine'; | ||
|
|
||
| describe('TypeSpecificFields', () => { |
There was a problem hiding this comment.
Why does it have its own top-level describe block? Isn't it in fact a part of 'Prebuilt rule asset schema'?
There was a problem hiding this comment.
Moved within Prebuilt rule asset schema
...ion/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.test.ts
Outdated
Show resolved
Hide resolved
...ion/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.test.ts
Outdated
Show resolved
Hide resolved
...solution/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.ts
Show resolved
Hide resolved
| ); | ||
| } | ||
|
|
||
| export const UPGRADABLE_RULES_FIELDS_BY_TYPE_MAP = createUpgradableRuleFieldsByTypeMap(); |
There was a problem hiding this comment.
Not sure I understand how and where this map is gonna be used, can you elaborate?
Also, what I'm missing in this PR is changes to the upgrade/_perform request schema. We talked that its fields property's schema should be derived from the rule schema, I think this needs to be done in this PR:
There was a problem hiding this comment.
Not sure I understand how and where this map is gonna be used, can you elaborate?
The idea is to take the type of the target rule to retrieve from our Map the list of upgradable fields that we should use to create a payload to send over to our Detections Client. For example, the user is updating a new_term rules to its next version, and our Map says that the upgradable fields for a new_terms rule are:
[
"new_terms",
[
"name",
"description",
"risk_score",
"severity",
"rule_name_override",
"timestamp_override",
"timestamp_override_fallback_disabled",
"timeline_id",
"timeline_title",
"license",
"note",
"building_block_type",
"investigation_fields",
"version",
"tags",
"enabled",
"risk_score_mapping",
"severity_mapping",
"interval",
"from",
"to",
"exceptions_list",
"author",
"false_positives",
"references",
"max_signals",
"threat",
"setup",
"related_integrations",
"required_fields",
"type",
"query",
"new_terms_fields",
"history_window_start",
"index",
"data_view_id",
"filters",
"alert_suppression",
"language"
]
],So we retrieve these fields from the Map, so can loop over them, and based on the payload of the request, create a value to update them to. Using this list, we'd create an object like:
{
"name": "Juan" // calculated from the diffs if pick_version was MERGED
"description": "My Description" // passed in the payload since pick_version for field was RESOLVED
"risk_score": 11 // retrieved from BASE version since pick_version for field was BASW
"severity": 'hi', //etc
// ...etc including all upgradable fields
}This object will be created for each rule, and then the whole payload passed over to the Detection Client's upgradePrebuiltRule() method.
Also, this map allows us to do additional validation: if we are upgrading a new_terms rule and the user includes in the fields object of the payload a anomaly_threshold field (or any non-matching) field, we can throw for that rule.
There was a problem hiding this comment.
Also, what I'm missing in this PR is changes to the upgrade/_perform request schema. We talked that its fields property's schema should be derived from the rule schema, I think this needs to be done in this PR:
Yes, you're right, I'm trying to do some Zod acrobatics to get this schema created. It's WIP, but I will include it in this PR.
jpdjere
force-pushed
the
create-upgradable-rule-fields-map
branch
from
2f6773d to
5b3e6c1
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @jpdjere |
There was a problem hiding this comment.
Very quickly looked through the diff and the comments, LGTM 👍
@jpdjere I'll probably take a deeper look tomorrow and maybe leave a few comments post-merge, but I wouldn't like to block the PR for another 24 hours, so approving the PR ✅ Thank you!
… to Rule Upgrade workflow (#195499) Resolves: #190597 ## Summary Adds `AlertSuppression` and `Investigation Fields` to Rule Upgrade workflow: - Fields had already been added to DiffableRule schema and diffing algorithms in #190128 - Current PR adds them to the UI field list so they get displayed in the diff ## Screenshots #### Investigation Fields  #### Alert Suppression  ## Testing Little bit tricky: no prebuilt rules have these fields, so no matter which packages you install you wont' see this upgrade. You'll need to tinker with the security-rule assets, for example: ```ts POST .kibana_security_solution/_update_by_query { "script": { "source": """ ctx._source['security-rule']['alert_suppression'] = [ 'group_by': ['agent.hostname'], 'missing_fields_strategy': 'suppress' ]; """, "lang": "painless" }, "query": { "bool": { "must": [ { "term": { "type": { "value": "security-rule" } } }, { "term": { "security-rule.rule_id": { "value": "0564fb9d-90b9-4234-a411-82a546dc1343" } } }, { "term": { "security-rule.version": { "value": "111" } } } ] } } } ``` ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
… to Rule Upgrade workflow (elastic#195499) Resolves: elastic#190597 ## Summary Adds `AlertSuppression` and `Investigation Fields` to Rule Upgrade workflow: - Fields had already been added to DiffableRule schema and diffing algorithms in elastic#190128 - Current PR adds them to the UI field list so they get displayed in the diff ## Screenshots #### Investigation Fields  #### Alert Suppression  ## Testing Little bit tricky: no prebuilt rules have these fields, so no matter which packages you install you wont' see this upgrade. You'll need to tinker with the security-rule assets, for example: ```ts POST .kibana_security_solution/_update_by_query { "script": { "source": """ ctx._source['security-rule']['alert_suppression'] = [ 'group_by': ['agent.hostname'], 'missing_fields_strategy': 'suppress' ]; """, "lang": "painless" }, "query": { "bool": { "must": [ { "term": { "type": { "value": "security-rule" } } }, { "term": { "security-rule.rule_id": { "value": "0564fb9d-90b9-4234-a411-82a546dc1343" } } }, { "term": { "security-rule.version": { "value": "111" } } } ] } } } ``` ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit ed144bd)
Summary
Partially addresses [Security Solution] Extend the
POST /upgrade/_performAPI endpoint's contract and functionality #166376 (see step 1 of plan)Partially addresses: [Security Solution] Add InvestigationFields and AlertSuppression fields to the upgrade workflow #190597
Creates a Map of the fields that are upgradable during the Upgrade workflow, by type.
/upgrade/_performendpoint handler logic to build the payload of fields that will be upgraded to their different versions (BASE,CURRENT,TARGET,MERGED,RESOLVED)Creates
RuleFieldsToUpgradeZod schema andFieldUpgradeSpecifiertype, part of the/upgrade/_performpayload, which defines which fields can be upgraded and how.See output: UPGRADABLE_RULES_FIELDS_BY_TYPE_MAP
### For maintainers