[Security Solution] Extend the `/upgrade/_perform` API endpoint's contract #189187

jpdjere · 2024-07-25T14:01:53Z

Partially addresses (contract change only): #166376

Summary

Creates OpenAPI schema YAML for the /upgrade/_perform endpoint.
Extends contract as described in the POC (search for Perform rule upgrade)
OAS includes list of fields that are user-customizable during the upgrade process, as listed in: [Security Solution] Handle specific fields in /upgrade/_perform endpoint upgrade workflow #186544
Autogenerated file is NOT yet used, as extending the functionality of the endpoint is TBD

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-07-26T11:29:56Z

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

elasticmachine · 2024-07-26T11:29:57Z

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

elasticmachine · 2024-07-26T11:29:58Z

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

xcrzx · 2024-07-29T10:09:57Z

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

+            - $ref: '../../model/rule_schema/common_attributes.schema.yaml#/components/schemas/RuleName'
+            - $ref: '../../model/rule_schema/common_attributes.schema.yaml#/components/schemas/RuleTagArray'
+            - $ref: '../../model/rule_schema/common_attributes.schema.yaml#/components/schemas/RuleDescription'


What is the reason for making this type a union? The rule name prop should have the resolved_value matching the ruleName type, not any of these. Can it be implemented that way?

I'm trying to reverse engineer these types. Notice the generic FieldUpgradeRequest type, which is what I tried to replicate with a union:

export interface PerformRuleUpgradeRequestBody { mode: 'ALL_RULES' | 'SPECIFIC_RULES'; pick_version?: 'BASE' | 'CURRENT' | 'TARGET' | 'MERGED'; rules: SingleRuleUpgradeRequest[]; // required if mode is SPECIFIC_RULES } export interface SingleRuleUpgradeRequest { id: RuleObjectId; pick_version?: 'BASE' | 'CURRENT' | 'TARGET' | 'MERGED'; fields?: { name?: FieldUpgradeRequest<RuleName>; description?: FieldUpgradeRequest<RuleDescription>; // etc // Every non-specified field will default to pick_version: 'MERGED'. // If pick_version is MERGED and there's a merge conflict the endpoint will throw. }; rule_revision: number; } export interface FieldUpgradeRequest<T> { pick_version: 'BASE' | 'CURRENT' | 'TARGET' | 'MERGED' | 'RESOLVED'; resolved_value: T; // required if pick_version is RESOLVED; type depends on the rule field type }

So each of the properties within fields in SingleRuleUpgradeRequest should be of type FieldUpgradeRequest because each property should be able to be updated to any of the pick_versions listed in the full enum (which includes RESOLVED).

Only if the pick_version value for that field is RESOLVED, then the resolved_value can take the value of the prop (RuleName for the case of the name property).

I'm not sure how to write the OAS in some other way without that union. Do you have in mind something more simple? Ideas welcome 😄

Maybe instead of having a "generic" FieldUpgradeRequest we could have something like RuleNameFieldUpgradeRequest, which directly refs to RuleName. And create a similar schema for each of the updatable fields.

With such an approach we wouldn't need a union, but it would mean a lot more schemas manually created.

What do you think?

I think we need to list every upgradeable field, something like this:

RuleUpgradeSpecifier: type: object properties: fields: type: object properties: name: type: object required: - pick_version properties: pick_version: $ref: '#/components/schemas/FieldPickVersionValues' resolved_value: $ref: '../../model/rule_schema/common_attributes.schema.yaml#/components/schemas/RuleName' tags: type: object required: - pick_version properties: pick_version: $ref: '#/components/schemas/FieldPickVersionValues' resolved_value: $ref: '../../model/rule_schema/common_attributes.schema.yaml#/components/schemas/RuleTagArray'

This approach is a bit more verbose than using a union with all possible values, but it will produce the most accurate type. I believe the effort is worth it.

wayneseymour

LGTM, cr only.

nikitaindik

Thanks @jpdjere! I've taken a look, left a few questions.

...ty_solution/docs/openapi/ess/security_solution_detections_api_2023_10_31.bundled.schema.yaml

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

nikitaindik · 2024-07-30T15:20:06Z

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

+              required:
+                - pick_version
+              properties:
+                pick_version:


Should it be oneOf? Either pick_version or resolved_value need to be specified?

No, resolved_value should only be defined if pick_version is RESOLVED. And it should contain the "resolved" value -as passed by the user in the request- that the field should be updated to.

Thanks, makes sense now! Do you think we could make the type more precise, something like:

{ pick_version: 'BASE' | 'CURRENT' | 'TARGET' | 'MERGED' } | { pick_version: 'RESOLVED', resolved_value: <value> }

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

xcrzx · 2024-07-31T10:02:10Z

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

+  title: Perform Rule Upgrade API endpoint
+  version: '2023-10-31'
+paths:
+  /api/detection_engine/rules/prebuilt/_perform_upgrade:


Suggested change

/api/detection_engine/rules/prebuilt/_perform_upgrade:

/internal/detection_engine/prebuilt_rules/upgrade/_perform:

xcrzx · 2024-07-31T10:02:25Z

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

+openapi: 3.0.0
+info:
+  title: Perform Rule Upgrade API endpoint
+  version: '2023-10-31'


Suggested change

version: '2023-10-31'

version: '1'

xcrzx · 2024-07-31T10:02:40Z

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

+paths:
+  /api/detection_engine/rules/prebuilt/_perform_upgrade:
+    post:
+      x-labels: [ess]


Suggested change

x-labels: [ess]

x-labels: [ess, serverless]

xcrzx · 2024-07-31T11:54:35Z

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

+        errors:
+          type: array
+          items:
+            $ref: '../../model/error_schema.schema.yaml#/components/schemas/ErrorSchema'


Shouldn't it be aggregated error?

kibana/x-pack/plugins/security_solution/common/api/detection_engine/prebuilt_rules/model/aggregated_prebuilt_rules_error.ts

Lines 8 to 15 in 329e696

export interface AggregatedPrebuiltRuleError {

message: string;

status_code?: number;

rules: Array<{

rule_id: string;

name?: string;

}>;

}

ErrorSchema is pretty different:

kibana/x-pack/plugins/security_solution/common/api/detection_engine/model/error_schema.gen.ts

Lines 21 to 33 in 329e696

export type ErrorSchema = z.infer<typeof ErrorSchema>;

export const ErrorSchema = z

.object({

id: z.string().optional(),

rule_id: RuleSignatureId.optional(),

list_id: z.string().min(1).optional(),

item_id: z.string().min(1).optional(),

error: z.object({

status_code: z.number().int().min(400),

message: z.string(),

}),

})

.strict();

xcrzx · 2024-07-31T11:59:45Z

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

+          description: |
+            Fields that can be customized during the upgrade workflow
+            as decided in: https://github.com/elastic/kibana/issues/186544
+            Fields listed here, which are not specified in the request body,
+            will default to a `pick_version` of `MERGED`.


Descriptions become a part of our public documentation. We should avoid putting technical details or links to GitHub issues in the description.

Also, is this part still accurate?

Fields listed here, which are not specified in the request body, will default to a `pick_version` of `MERGED`.

Shouldn't omitted fields default to the high-level pick_version?

Shouldn't omitted fields default to the high-level pick_version?

Yes and no. Omitted fields will default to the higher-level pick_version, but only if one is defined (notice that pick_version is not required at any higher level, only on each field within fields). So I think the behaviour should be:

Use the pick_version defined within each rule field.

If the field is not defined, update that field to:
2.1. MERGED if no higher-level pick_version is defined.
2.2. Otherwise: the pick_version passed on the RuleUpgradeSpecifier, if defined.
2.3. Otherwise: the pick_version passed on the UpgradeSpecificRulesRequest, if defined.
2.4. Otherwise (no pick_version passed on individual fields or at any level): use MERGED as default.

Same for UpgradeAllRulesRequest: here use MERGED as default, unless pick_version is explicitly passed.

What do you think? The alternative to this would be to make the top-level pick_version required so that we can default to that instead of MERGED.

xcrzx · 2024-07-31T12:02:17Z

.../detection_engine/prebuilt_rules/perform_rule_upgrade/perform_rule_upgrade_route.schema.yaml

+        rule_id:
+          $ref: '../../model/rule_schema/common_attributes.schema.yaml#/components/schemas/RuleSignatureId'
+        revision:
+          type: number
+        version:
+          $ref: '../../model/rule_schema/common_attributes.schema.yaml#/components/schemas/RuleVersion'
+        pick_version:
+          $ref: '#/components/schemas/PickVersionValues'


It would be super helpful to provide some description for other fields as well

Yes, added to the fields here. What do you think about the description for revision?

Rule's current revision number. Should match the rule's revision number returned from the Review Rule Upgrade API endpoint.

I think that is understandable by the user (mentioning the upgrade endpoint), but is not technically correct. If a request slips in between calling /upgrade and /perform, the second request will fail because the revision number won't match. But that's hard to explain here.

banderror

@jpdjere I would like to avoid migrating prebuilt rules API endpoints to OpenAPI at this stage. This can require significant effort, since a lot of types like ThreeWayDiff, RuleFieldsDiff, DiffableRule, diff algorithms are all generics. Some of them are a part of the API contract.

My proposal is to extend the perform's API request schema using the current tech used for Prebuilt Rules API. This will allow us to iterate on the perform endpoint implementation much sooner.

When we have implemented the API behavior required for Milestone 3 + a comprehensive integration test coverage for it, we will plan a migration to OpenAPI. Tests would be a safety net for this significant refactoring.

kibana-ci · 2024-08-01T12:14:32Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] Defend Workflows Cypress Tests #6 / Automated Response Actions From alerts "before all" hook for "should have generated endpoint and rule" "before all" hook for "should have generated endpoint and rule"
[job] [logs] Defend Workflows Cypress Tests on Serverless #4 / Automated Response Actions From alerts "before all" hook for "should have generated endpoint and rule" "before all" hook for "should have generated endpoint and rule"
[job] [logs] Defend Workflows Cypress Tests #16 / Document signing: "before all" hook for "should fail if data tampered" "before all" hook for "should fail if data tampered"
[job] [logs] Defend Workflows Cypress Tests on Serverless #1 / Endpoint generated alerts "before each" hook for "should create a Detection Engine alert from an endpoint alert" "before each" hook for "should create a Detection Engine alert from an endpoint alert"
[job] [logs] Defend Workflows Cypress Tests #11 / Endpoints page "before all" hook for "Shows endpoint on the list" "before all" hook for "Shows endpoint on the list"
[job] [logs] Defend Workflows Cypress Tests on Serverless #7 / Endpoints page "before all" hook for "Shows endpoint on the list" "before all" hook for "Shows endpoint on the list"
[job] [logs] Defend Workflows Cypress Tests on Serverless #14 / Response console Execute operations: "before all" hook for ""execute --command" - should execute a command" "before all" hook for ""execute --command" - should execute a command"
[job] [logs] Defend Workflows Cypress Tests #2 / Response console File operations: "before all" hook for ""get-file --path" - should retrieve a file" "before all" hook for ""get-file --path" - should retrieve a file"
[job] [logs] Defend Workflows Cypress Tests on Serverless #1 / Response console File operations: "before all" hook for ""get-file --path" - should retrieve a file" "before all" hook for ""get-file --path" - should retrieve a file"
[job] [logs] Defend Workflows Cypress Tests #17 / Response console From endpoint list "before all" hook for "should open responder" "before all" hook for "should open responder"
[job] [logs] Defend Workflows Cypress Tests #3 / Response console Host Isolation: "before all" hook for "should isolate a host from response console" "before all" hook for "should isolate a host from response console"
[job] [logs] Defend Workflows Cypress Tests on Serverless #4 / Response console Host Isolation: "before all" hook for "should release an isolated host via response console" "before all" hook for "should release an isolated host via response console"
[job] [logs] Defend Workflows Cypress Tests #6 / Response console Scan operation: "before all" hook for ""scan --path" - should scan a file" "before all" hook for ""scan --path" - should scan a file"
[job] [logs] Defend Workflows Cypress Tests on Serverless #5 / Response console Scan operation: "before all" hook for ""scan --path" - should scan a file" "before all" hook for ""scan --path" - should scan a file"
[job] [logs] Defend Workflows Cypress Tests #15 / Response console: From Alerts "before all" hook for "should open responder from alert details flyout" "before all" hook for "should open responder from alert details flyout"
[job] [logs] Defend Workflows Cypress Tests #20 / Unenroll agent from fleet changing agent policy when agent tamper protection is enabled but then is switched to a policy with it also enabled "before each" hook for "should unenroll from fleet without issues" "before each" hook for "should unenroll from fleet without issues"
[job] [logs] Defend Workflows Cypress Tests #18 / Unenroll agent from fleet when agent tamper protection is disabled but then is switched to a policy with it enabled "before each" hook for "should unenroll from fleet without issues" "before each" hook for "should unenroll from fleet without issues"
[job] [logs] Defend Workflows Cypress Tests #16 / Unenroll agent from fleet when agent tamper protection is enabled "before each" hook for "should unenroll from fleet without issues" "before each" hook for "should unenroll from fleet without issues"
[job] [logs] Defend Workflows Cypress Tests #14 / Unenroll agent from fleet with agent tamper protection is disabled "before each" hook for "should unenroll from fleet without issues" "before each" hook for "should unenroll from fleet without issues"
[job] [logs] Defend Workflows Cypress Tests #3 / Uninstall agent from host changing agent policy when agent tamper protection is enabled but then is switched to a policy with it also enabled "before each" hook for "should uninstall from host without issues" "before each" hook for "should uninstall from host without issues"
[job] [logs] Defend Workflows Cypress Tests #2 / Uninstall agent from host changing agent policy when agent tamper protection is enabled but then is switched to a policy with it disabled "before each" hook for "should uninstall from host without issues" "before each" hook for "should uninstall from host without issues"
[job] [logs] Defend Workflows Cypress Tests #15 / Uninstall agent from host when agent tamper protection is disabled "before each" hook for "should uninstall from host without issues" "before each" hook for "should uninstall from host without issues"
[job] [logs] Defend Workflows Cypress Tests #17 / Uninstall agent from host when agent tamper protection is enabled "before each" hook for "should uninstall from host with the uninstall token" "before each" hook for "should uninstall from host with the uninstall token"

Metrics [docs]

✅ unchanged

History

💚 Build #224954 succeeded 329e696
💔 Build #224714 failed c30b261
💛 Build #224122 was flaky d5eb5e4

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

cc @jpdjere

jpdjere · 2024-08-02T11:50:28Z

Closing in favour of: #189790

…tract migrating to Zod (#189790) Partially addresses (contract change only): #166376 Created in favour of: #189187 (closed) ## Summary - Extends contract as described in the [POC](#144060), migrating from `io-ts` to Zod (search for `Perform rule upgrade`) - Uses new types in endpoint, but functionality remains unchaged. ### 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)

jpdjere self-assigned this Jul 25, 2024

jpdjere changed the title ~~[Security Solution] Extend the /upgrade/_perform API endpoint's contract and functionality~~ [Security Solution] Extend the /upgrade/_perform API endpoint's contract Jul 26, 2024

jpdjere marked this pull request as ready for review July 26, 2024 11:29

jpdjere requested a review from a team as a code owner July 26, 2024 11:29

jpdjere requested a review from nikitaindik July 26, 2024 11:29

jpdjere requested a review from banderror July 26, 2024 11:30

elastic-vault-github-plugin-prod bot requested a review from a team as a code owner July 26, 2024 12:16

xcrzx reviewed Jul 29, 2024

View reviewed changes

jpdjere requested a review from a team as a code owner July 30, 2024 14:33

wayneseymour approved these changes Jul 30, 2024

View reviewed changes

jpdjere and others added 9 commits July 30, 2024 16:51

Created OpenAPI schema

26a13ac

Correct OAS

d8a9978

Expand OAS

4edf88c

Update description

c724a2f

[CI] Auto-commit changed files from 'yarn openapi:bundle:detections'

fc614c5

Rewrote schemas

c0fec4d

[CI] Auto-commit changed files from 'yarn openapi:bundle:detections'

32b6d4f

Rewrote schema again

da8713b

Update bundle

c30b261

jpdjere force-pushed the extend-perform-endpoint branch from af446ba to c30b261 Compare July 30, 2024 14:53

nikitaindik reviewed Jul 30, 2024

View reviewed changes

jpdjere requested a review from xcrzx July 30, 2024 15:51

jpdjere added 3 commits July 31, 2024 11:11

Merge branch 'main' into extend-perform-endpoint

4a441ad

Created union for each field

fc2bb62

Add bundle

329e696

xcrzx reviewed Jul 31, 2024

View reviewed changes

banderror requested changes Jul 31, 2024

View reviewed changes

Add descriptions

fbf5163

jpdjere mentioned this pull request Aug 2, 2024

[Security Solution] Extend the /upgrade/_perform API endpoint's contract migrating to Zod #189790

Merged

1 task

jpdjere closed this Aug 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Extend the `/upgrade/_perform` API endpoint's contract #189187

[Security Solution] Extend the `/upgrade/_perform` API endpoint's contract #189187

jpdjere commented Jul 25, 2024 •

edited

Loading

elasticmachine commented Jul 26, 2024

elasticmachine commented Jul 26, 2024

elasticmachine commented Jul 26, 2024

xcrzx Jul 29, 2024 •

edited

Loading

jpdjere Jul 29, 2024 •

edited

Loading

jpdjere Jul 29, 2024 •

edited

Loading

xcrzx Jul 30, 2024

wayneseymour left a comment

nikitaindik left a comment

nikitaindik Jul 30, 2024

jpdjere Jul 30, 2024 •

edited

Loading

nikitaindik Jul 31, 2024

xcrzx Jul 31, 2024

xcrzx Jul 31, 2024

xcrzx Jul 31, 2024

xcrzx Jul 31, 2024

xcrzx Jul 31, 2024

jpdjere Jul 31, 2024

xcrzx Jul 31, 2024

jpdjere Jul 31, 2024

banderror left a comment •

edited

Loading

kibana-ci commented Aug 1, 2024 •

edited

Loading

jpdjere commented Aug 2, 2024

	/api/detection_engine/rules/prebuilt/_perform_upgrade:
	/internal/detection_engine/prebuilt_rules/upgrade/_perform:

	export interface AggregatedPrebuiltRuleError {
	message: string;
	status_code?: number;
	rules: Array<{
	rule_id: string;
	name?: string;
	}>;
	}

	export type ErrorSchema = z.infer<typeof ErrorSchema>;
	export const ErrorSchema = z
	.object({
	id: z.string().optional(),
	rule_id: RuleSignatureId.optional(),
	list_id: z.string().min(1).optional(),
	item_id: z.string().min(1).optional(),
	error: z.object({
	status_code: z.number().int().min(400),
	message: z.string(),
	}),
	})
	.strict();

[Security Solution] Extend the /upgrade/_perform API endpoint's contract #189187

[Security Solution] Extend the /upgrade/_perform API endpoint's contract #189187

Conversation

jpdjere commented Jul 25, 2024 • edited Loading

Summary

For maintainers

elasticmachine commented Jul 26, 2024

elasticmachine commented Jul 26, 2024

elasticmachine commented Jul 26, 2024

xcrzx Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

jpdjere Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

jpdjere Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wayneseymour left a comment

Choose a reason for hiding this comment

nikitaindik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpdjere Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror left a comment • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Aug 1, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

History

jpdjere commented Aug 2, 2024

[Security Solution] Extend the `/upgrade/_perform` API endpoint's contract #189187

[Security Solution] Extend the `/upgrade/_perform` API endpoint's contract #189187

jpdjere commented Jul 25, 2024 •

edited

Loading

xcrzx Jul 29, 2024 •

edited

Loading

jpdjere Jul 29, 2024 •

edited

Loading

jpdjere Jul 29, 2024 •

edited

Loading

jpdjere Jul 30, 2024 •

edited

Loading

banderror left a comment •

edited

Loading

kibana-ci commented Aug 1, 2024 •

edited

Loading