[Security Solution] Swap rule unions out for discriminated unions to improve validation error messages #171452

marshallmain · 2023-11-16T23:21:35Z

Epics: https://github.com/elastic/security-team/issues/8058, https://github.com/elastic/security-team/issues/6726 (internal)
Partially addresses: https://github.com/elastic/security-team/issues/7991 (internal)

Summary

The main benefit of this PR is shown in rule_request_schema.test.ts, where the error messages are now more accurate and concise. With regular unions, zod has to try validating the input against all schemas in the union and reports the errors from every schema in the union. Switching to discriminated unions, with type as the discriminator, allows zod to pick the right rule type schema from the union and only validate against that rule type. This means the error message reports that either the discriminator is invalid, in any case where type is not valid, or if type is valid but another field is wrong for that type of rule then the error message is the validation result from only that rule type.

To make it possible to use discriminated unions, we need to switch from using zod's .and() for intersections to .merge() because .and() returns an intersection type that is incompatible with discriminated unions in zod. Similarly, we need to remove the usage of .transform() because it returns a ZodEffect that is incompatible with .merge().

Instead of using .transform() to turn properties from optional to possibly undefined, we can use requiredOptional explicitly in specific places to convert the types. Similarly, the RequiredOptional type can be used on the return type of conversion functions between API and internal schemas to enforce that all properties are explicitly specified in the conversion.

Future work:

better alignment of codegen with OpenAPI definitions of anyOf/oneOf. https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#oneof oneOf requires that the input match exactly one schema from the list, which is different from z.union. anyOf should be z.union, oneOf should be z.discriminatedUnion
flatten the schema structure further to avoid Type instantiation is excessively deep and possibly infinite. Seems to be a common issue with zod (“Type instantiation is excessively deep and possibly infinite” but only in a large codebase microsoft/TypeScript#34933) Limiting the number of .merge and other zod operations needed to build a particular schema object seems to help resolve the error. Combining ResponseRequiredFields and ResponseOptionalFields into a single object rather than merging them solved the immediate problem. However, we may still be near the depth limit. Changing RuleResponse as seen below also solved the problem in testing, and may give us more headroom for future changes if we apply techniques like this here and in other places. The difference here is that SharedResponseProps is only intersected with the type specific schemas after they're combined in a discriminated union, whereas in main we merge SharedResponseProps with each individual schema then merge them all together.
combine other Required and Optional schemas, like QueryRuleRequiredFields and QueryRuleOptionalFields

export type RuleResponse = z.infer<typeof RuleResponse>;
export const RuleResponse = SharedResponseProps.and(z.discriminatedUnion('type', [
  EqlRuleResponseFields,
  QueryRuleResponseFields,
  SavedQueryRuleResponseFields,
  ThresholdRuleResponseFields,
  ThreatMatchRuleResponseFields,
  MachineLearningRuleResponseFields,
  NewTermsRuleResponseFields,
  EsqlRuleResponseFields,
]));

… deep' error

rylnd

Nice writeup; much obliged. Detection engine changes are minimal; mostly just the moved requiredOptional calls and all the test updates (which look great). Nice discriminators, as well 😉 .

LGTM

elasticmachine · 2023-11-22T13:10:05Z

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

elasticmachine · 2023-11-22T13:10:06Z

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

maximpn

@marshallmain thank you for improving error messages 🙏

Error messages look much better now 👍 And I'm really happy you got rid of x-modify: requireOptional.

We definitely need to keep an eye on our schemas complexity to avoid Type instantiation is excessively deep and possibly infinite. Zod is well know for complex typing and one of the slowest libraries for type inference due to huge flexibility. Interesting if TS can be tuned too to allow "deeper" types.

The only concern I have is related to params.meta as { [x: string]: {} | undefined }. Basically recursive RequiredOptional changes type from Record<string, unknown> to Record<string, {} | undefined>. It looks like it should be some solution like define SnakeCase types explicitly and define modified types there. Or refrain from recursive RequiredOptional.

(Btw there is an util type to transform camel case to snake case.)

maximpn · 2023-11-26T10:26:50Z

...olution/common/api/detection_engine/model/rule_response_actions/response_actions.schema.yaml

@@ -49,7 +49,6 @@ components:
      required:
        - id
        - query
-      x-modify: requiredOptional


Thank you for finally removing requiredOptional from the schema 👍

maximpn · 2023-11-26T11:45:41Z

packages/kbn-zod-helpers/src/required_optional.ts

@@ -28,7 +28,13 @@
 */
 export type RequiredOptional<T> = { [K in keyof T]-?: [T[K]] } extends infer U
  ? U extends Record<keyof U, [unknown]>
-    ? { [K in keyof U]: U[K][0] }
+    ? {
+        [K in keyof U]: Record<string, unknown> extends U[K][0]


Why did you need to make RequiredOptional recursive?

I was experimenting with this during the hackathon to make it easier to ensure that as we add more object types to the rule schema no fields get missed during the conversion between snake and camel case. Alert suppression fields, for example, are in the alert_suppression object and some of those sub-fields are optional. We can use the non-recursive RequiredOptional on the return type of convertAlertSuppressionToSnake to cover that conversion. But, it could be useful to have a recursive RequiredOptional so we don't have to specify RequiredOptional on all the conversion functions for sub-objects as well, only at the top level converter from camel-rule to snake-rule.

It's not necessary for this PR though so I'll remove it and we can try that separately.

I think this:

export type RequiredOptional<T> = { [K in keyof T]-?: [T[K]] } extends infer U ? U extends Record<keyof U, [unknown]> ? { [K in keyof U]: U[K][0] extends Record<string, unknown> | undefined ? undefined extends U[K][0] ? RequiredOptional<U[K][0]> | undefined : RequiredOptional<U[K][0]> : U[K][0]; } : never : never;

works better, but I'm still not 100% sure that it handles all the type edge cases correctly. Mostly putting it here for posterity in case I or someone else comes back to try this again later.

maximpn · 2023-11-26T11:52:43Z

...curity_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts

@@ -657,7 +657,7 @@ export const commonParamsCamelToSnake = (params: BaseRuleParams) => {
    output_index: params.outputIndex,
    timeline_id: params.timelineId,
    timeline_title: params.timelineTitle,
-    meta: params.meta,
+    meta: params.meta as { [x: string]: {} | undefined },


Type casting here is quite obscure. I only managed to understand why it's necessary after switching to the branch. Recursive RequiredOptional unintentionally updates types to be an object instead of unknown.

Can we restrain from making RequiredOptional recursive?

Another option to avoid this type casting is to update generator to generate something like .catchall(z.object({})) instead .catchall(z.unknown()).

Yeah, the recursive update was not necessary for the discriminated unions so I removed it for this PR

maximpn

@marshallmain Thank you for addressing my comments!

tomsonpl

Defend Workflows (Osquery) changes lgtm 👍

banderror · 2023-11-29T18:50:08Z

@elasticmachine merge upstream

kibana-ci · 2023-11-29T20:08:17Z

💚 Build Succeeded

Buildkite Build
Commit: 94e98fa

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	12.9MB	12.9MB	+241.0B

History

💚 Build #179344 succeeded fd64136
💔 Build #179319 failed f980f45
💛 Build #177048 was flaky aef13a2
💔 Build #176990 failed d615f62
💛 Build #176768 was flaky bbac159
💔 Build #176754 failed 8f6de42

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

cc @marshallmain

banderror

Reviewed the changes and copied the "future work" from the PR description to https://github.com/elastic/security-team/issues/7991.

This is a great improvement, thank you @marshallmain 🙏

marshallmain added 8 commits November 16, 2023 15:00

Swap rule unions out for discriminated unions

43b5a53

Update tests

0c0181b

Fix more tests

a101ae0

Flatten schema dependencies to fix 'Type instantiation is excessively…

6abde76

… deep' error

Fix more test

b4735c2

Fix formatting in zod_schema_item.handlebars

bc95708

Fix bad import

8f6de42

Fix test

bbac159

marshallmain marked this pull request as ready for review November 20, 2023 15:33

marshallmain requested review from a team as code owners November 20, 2023 15:33

marshallmain requested review from patrykkopycinski, szwarckonrad, rylnd and maximpn November 20, 2023 15:33

marshallmain and others added 2 commits November 20, 2023 07:34

Remove unneeded import

d615f62

Merge branch 'main' into rule-schema-discriminated-union

aef13a2

marshallmain added release_note:skip Skip the PR/issue when compiling release notes v8.12.0 Team:Detection Rule Management Security Detection Rule Management Team labels Nov 20, 2023

rylnd approved these changes Nov 20, 2023

View reviewed changes

banderror assigned marshallmain Nov 22, 2023

banderror added Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Nov 22, 2023

maximpn reviewed Nov 26, 2023

View reviewed changes

Switch RequiredOptional back to non-recursive

c217f51

marshallmain added 2 commits November 27, 2023 13:15

Merge branch 'main' into rule-schema-discriminated-union

f980f45

Merge branch 'main' into rule-schema-discriminated-union

fd64136

maximpn approved these changes Nov 29, 2023

View reviewed changes

tomsonpl approved these changes Nov 29, 2023

View reviewed changes

Merge branch 'main' into rule-schema-discriminated-union

94e98fa

banderror approved these changes Nov 29, 2023

View reviewed changes

banderror merged commit 6073eb6 into elastic:main Nov 29, 2023
30 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Nov 29, 2023

vitaliidm mentioned this pull request Dec 1, 2023

[Security Solution][Detection Engine] adds alerts Suppression to threshold rule #171423

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Swap rule unions out for discriminated unions to improve validation error messages #171452

[Security Solution] Swap rule unions out for discriminated unions to improve validation error messages #171452

marshallmain commented Nov 16, 2023 •

edited by banderror

Loading

rylnd left a comment

elasticmachine commented Nov 22, 2023

elasticmachine commented Nov 22, 2023

maximpn left a comment

maximpn Nov 26, 2023

maximpn Nov 26, 2023

marshallmain Nov 27, 2023

marshallmain Nov 27, 2023

maximpn Nov 26, 2023

marshallmain Nov 27, 2023

maximpn left a comment

tomsonpl left a comment

banderror commented Nov 29, 2023

kibana-ci commented Nov 29, 2023

banderror left a comment

[Security Solution] Swap rule unions out for discriminated unions to improve validation error messages #171452

[Security Solution] Swap rule unions out for discriminated unions to improve validation error messages #171452

Conversation

marshallmain commented Nov 16, 2023 • edited by banderror Loading

Summary

rylnd left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 22, 2023

elasticmachine commented Nov 22, 2023

maximpn left a comment

Choose a reason for hiding this comment

maximpn Nov 26, 2023

Choose a reason for hiding this comment

maximpn Nov 26, 2023

Choose a reason for hiding this comment

marshallmain Nov 27, 2023

Choose a reason for hiding this comment

marshallmain Nov 27, 2023

Choose a reason for hiding this comment

maximpn Nov 26, 2023

Choose a reason for hiding this comment

marshallmain Nov 27, 2023

Choose a reason for hiding this comment

maximpn left a comment

Choose a reason for hiding this comment

tomsonpl left a comment

Choose a reason for hiding this comment

banderror commented Nov 29, 2023

kibana-ci commented Nov 29, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

History

banderror left a comment

Choose a reason for hiding this comment

marshallmain commented Nov 16, 2023 •

edited by banderror

Loading