[SIEM][Detection Engine] Converts from joi to use io-ts and moves the types to common by FrankHassanabad · Pull Request #68127 · elastic/kibana

FrankHassanabad · 2020-06-03T16:25:02Z

Summary

https://github.com/elastic/siem-team/issues/646
Converts the detection rules and REST to use io-ts
Removes their joi counterparts
Updates all tests to use it
Fixes a bug with the risk_score that was being sent in as a string from the UI instead of a number
Fixes a bug within the exactCheck validating where it can now accept null value types for optional body messages.
Fixes a bug in the FindRoute where it did not send down fields from REST
Changes the lists plugin to utilize the io-ts types from siem rather than having them duplicated.
Makes some stronger validations
Adds a lot of codecs

Things to look out for:

Generic testing to ensure I didn't break something that was not part of the tests.
Fix for the risk_score from string to number is in:

x-pack/plugins/security_solution/public/alerts/components/rules/step_about_rule/index.test.tsx

Fix for the exact check (unit tests are written and added)

x-pack/plugins/security_solution/public/alerts/components/rules/step_about_rule/index.test.tsx

Within all the types I added are there any misspelled things or copy-pasta mistakes with strings:
x-pack/plugins/security_solution/common/detection_engine/schemas/types
Fix for find_rules_route.ts:58

x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.ts

Follow on things that this PR doesn't do we need to:

Add linter rule to forbid NodeJS code within common section
The [object Object] formatter issues seen in the code such as:

// TODO: Fix/Change the formatErrors to be better able to handle objects
'Invalid value "[object Object]" supplied to "note"',

Formatter issues such as: 'Invalid value "" supplied to ""'
Remove the hapi server object from lists plugin

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

… build

…ber and removed back end TODO

…the older joi schema and used newer mock methodologies

elasticmachine · 2020-06-04T20:05:47Z

Pinging @elastic/siem (Team:SIEM)

FrankHassanabad · 2020-06-04T20:06:54Z

...ins/security_solution/server/lib/detection_engine/scripts/create_ndjson_prepackaged_rules.sh

+  echo "" >> pre_packaged_rules.ndjson
+done
+echo "{\"exported_count\":$i,\"missing_rules\":[],\"missing_rules_count\":0}" >> pre_packaged_rules.ndjson
+


NOTE: This is just a helper script for ad-hoc creating pre packaged rules as ndjson and I just kind of snuck it in here with this :-)

…the create schema with regards to validation

…the stricter types for the tests

…ts from joi to io-ts

…ne since io-ts is slower than joi for importing

rylnd

Just adding some comments for now because a) I think they may be indicative of broader issues (or maybe just my misunderstanding :wink) and b) I've gotta run for the evening, but: this is an incredible amount of work that cleans things up TREMENDOUSLY and I want to thank you for the effort.

rylnd · 2020-06-08T22:05:02Z

...lugins/security_solution/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts

      },
      options: {
-        tags: ['access:securitySolution'],
+        tags: ['access:siem'],


Did we mean to revert these, or was this a bad conflict resolution?

Bad conflict will fix and re-push.

rylnd · 2020-06-08T22:45:23Z

...lugins/security_solution/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts

      path: `${DETECTION_ENGINE_RULES_URL}/_bulk_create`,
      validate: {
-        body: buildRouteValidation<RuleAlertParamsRest[]>(createRulesBulkSchema),
+        body: buildRouteValidation<typeof createRulesBulkSchema, CreateRulesBulkSchemaDecoded>(


Question: why do we need both createRulesBulkSchema and CreateRulesBulkSchemaDecoded? If our input is unknown and our output is CreateRulesBulkSchemaDecoded then we shouldn't need the intermediate type, right? (Okay, I realize why we need it right now, but having to specify both of these types feels like maybe we're doing something non-optimal here.)

At some point, I would expect to be able to specify a single codec that converts a request (I) into our expected form (A), but I'm happy to reset that expectation given new info as well :)

I would like that as well. I would like a way to specify the decoded type automatically without having to create one for it.

rylnd · 2020-06-08T22:55:17Z

x-pack/plugins/security_solution/public/alerts/components/rules/step_about_rule/index.test.tsx

+      riskScore: 80,
+      severity: 'low',
+      tags: [],
+      threat: [


Here's that structure we were talking about today 😉

rylnd · 2020-06-08T22:55:54Z

x-pack/plugins/security_solution/public/alerts/components/rules/step_about_rule/schema.tsx

  },
  riskScore: {
    type: FIELD_TYPES.RANGE,
+    serializer: (input: string) => Number(input),


Yeah, that was a good find. It makes sense, the HTML input must contain a string so they gave us a way to specify a serializer so we can convert from string to number

rylnd · 2020-06-08T23:03:09Z

x-pack/plugins/security_solution/common/detection_engine/schemas/types/only_false_allowed.ts

+ *   - If true is sent in then this will return an error
+ *   - If false is sent in then this will allow it only false
+ */
+export const OnlyFalseAllowed: OnlyFalseAllowedC = new t.Type<boolean, boolean, unknown>(


I don't think you need to re-specify the generic arguments here, those should be inferred by the arguments (and enforced by OnlyFalseAllowedC)

Yeah I can remove them in all the files, it was a pattern from io-ts I just modified from their examples.

It would be better to just use:

export const OnlyFalseAllowed = new t.Type<boolean, boolean, unknown>( ... code ); export type OnlyFalseAllowedC = typeof OnlyFalseAllowed;

And not re-type it twice everywhere. Will fix that.

rylnd · 2020-06-08T23:16:44Z

...ecurity_solution/common/detection_engine/schemas/request/add_prepackged_rules_schema.test.ts

+
+    const decoded = addPrepackagedRulesSchema.decode(payload);
+    const checked = exactCheck<AddPrepackagedRulesSchema>(
+      (payload as unknown) as AddPrepackagedRulesSchema,


This is due to exactCheck requiring that original is constrained to T, right? If it were changed to unknown:

export const exactCheck = <T>( original: unknown, decoded: Either<t.Errors, T> ): Either<t.Errors, T> => {

we wouldn't need to perform these casts. But perhaps I'm missing something?

I think you're right, I changed it locally and it looks good. I have changed all of these everywhere and I also was able to remove a lot of the templating in a lot of spots too.

Thank you! 🙏

nit: I think findDifferencesRecursive should also change to receive unknown, but if you wanna merge this tonight we can follow up after.

rylnd

Nothing else caught my eye, but given the size here someone else should probably take a look. Regardless, you've got my approval! 🚀 Onwards and upwards! 🚀

kibanamachine · 2020-06-09T01:42:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b06403c

History

💚 Build #52539 succeeded 8be7378
💚 Build #52300 succeeded fa51402
💚 Build #52265 succeeded df139c2
💚 Build #52258 succeeded 347c5a7
💚 Build #52250 succeeded ae07f4b

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

… types to common (elastic#68127) ## Summary * https://github.com/elastic/siem-team/issues/646 * Converts the detection rules and REST to use io-ts * Removes their joi counterparts * Updates all tests to use it * Fixes a bug with the risk_score that was being sent in as a string from the UI instead of a number * Fixes a bug within the exactCheck validating where it can now accept null value types for optional body messages. * Fixes a bug in the FindRoute where it did not send down fields from REST * Changes the lists plugin to utilize the io-ts types from siem rather than having them duplicated. * Makes some stronger validations * Adds a lot of codecs **Things to look out for:** * Generic testing to ensure I didn't break something that was not part of the tests. * Fix for the risk_score from string to number is in: ``` x-pack/plugins/security_solution/public/alerts/components/rules/step_about_rule/index.test.tsx ``` * Fix for the exact check (unit tests are written and added) ``` x-pack/plugins/security_solution/public/alerts/components/rules/step_about_rule/index.test.tsx ``` * Within all the types I added are there any misspelled things or copy-pasta mistakes with strings: x-pack/plugins/security_solution/common/detection_engine/schemas/types * Fix for `find_rules_route.ts:58` ``` x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.ts ``` **Follow on things that this PR doesn't do we need to:** * Add linter rule to forbid NodeJS code within common section * The `[object Object]` formatter issues seen in the code such as: ``` // TODO: Fix/Change the formatErrors to be better able to handle objects 'Invalid value "[object Object]" supplied to "note"', ``` * Formatter issues such as: `'Invalid value "" supplied to ""'` * Remove the hapi server object from lists plugin ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

… types to common (#68127) (#68611) ## Summary * https://github.com/elastic/siem-team/issues/646 * Converts the detection rules and REST to use io-ts * Removes their joi counterparts * Updates all tests to use it * Fixes a bug with the risk_score that was being sent in as a string from the UI instead of a number * Fixes a bug within the exactCheck validating where it can now accept null value types for optional body messages. * Fixes a bug in the FindRoute where it did not send down fields from REST * Changes the lists plugin to utilize the io-ts types from siem rather than having them duplicated. * Makes some stronger validations * Adds a lot of codecs **Things to look out for:** * Generic testing to ensure I didn't break something that was not part of the tests. * Fix for the risk_score from string to number is in: ``` x-pack/plugins/security_solution/public/alerts/components/rules/step_about_rule/index.test.tsx ``` * Fix for the exact check (unit tests are written and added) ``` x-pack/plugins/security_solution/public/alerts/components/rules/step_about_rule/index.test.tsx ``` * Within all the types I added are there any misspelled things or copy-pasta mistakes with strings: x-pack/plugins/security_solution/common/detection_engine/schemas/types * Fix for `find_rules_route.ts:58` ``` x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.ts ``` **Follow on things that this PR doesn't do we need to:** * Add linter rule to forbid NodeJS code within common section * The `[object Object]` formatter issues seen in the code such as: ``` // TODO: Fix/Change the formatErrors to be better able to handle objects 'Invalid value "[object Object]" supplied to "note"', ``` * Formatter issues such as: `'Invalid value "" supplied to ""'` * Remove the hapi server object from lists plugin ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

* master: (37 commits) [DOCS] Adds documentation for drilldowns (elastic#68122) [Telemetry] telemetry.sendUsageFrom: "server" by default (elastic#68071) [ML] Transforms: Support sub-aggregations (elastic#68306) Fixed pre-configured docs link points to the wrong page and functional tests configs (elastic#68606) [Ingest Manager] Update queries from `stream.*` to `dataset.*` (elastic#68322) Enable Watcher by default to fix bug in which Watcher doesn't render in the side nav (elastic#68602) Convert Index Templates API routes to snakecase. (elastic#68463) [SECURITY SOLUTION] [Detections] Add / Update e2e tests to ensure initial rule runs are successful (elastic#68441) [Ingest] OpenAPI spec file (elastic#68323) chore(NA): skip apis Endpoint plugin Endpoint policy api (elastic#68638) bumping makelogs version to v6.0.0 (elastic#66163) [SIEM] Add create template button (elastic#66613) Bump websocket-extensions from 0.1.3 to 0.1.4 (elastic#68414) [ML] Sample data modules - use event.dataset instead of index name (elastic#68538) [ML] Functional tests - fix job validation API test with maxModelMemoryLimit (elastic#68501) [ML] Functional tests - stabilize DFA job creation (elastic#68495) [TSVB] Allows the user to change the tooltip mode (elastic#67775) chore(NA): skip apis Endpoint plugin Endpoint alert API when data is in elasticsearch (elastic#68613) chore(NA): skip endpoint Endpoint Alert Page: when es has data and user has navigated to the page (elastic#68596) [SIEM][Detection Engine] Converts from joi to use io-ts and moves the types to common (elastic#68127) ...

## Summary * Smaller follow ups and bug fixes from: #68127 * Added unknown to `findDifferencesRecursive` * Added linter rule to catch NodeJS code in the common folders for both `lists` and `security_solution` * Removed the Hapi server type from the common folder of lists ### Checklist * Added unknown to the correct locations - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

## Summary * Smaller follow ups and bug fixes from: elastic#68127 * Added unknown to `findDifferencesRecursive` * Added linter rule to catch NodeJS code in the common folders for both `lists` and `security_solution` * Removed the Hapi server type from the common folder of lists ### Checklist * Added unknown to the correct locations - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

) ## Summary * Smaller follow ups and bug fixes from: #68127 * Added unknown to `findDifferencesRecursive` * Added linter rule to catch NodeJS code in the common folders for both `lists` and `security_solution` * Removed the Hapi server type from the common folder of lists ### Checklist * Added unknown to the correct locations - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

elasticmachine · 2021-09-23T14:34:03Z

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

FrankHassanabad added 12 commits June 1, 2020 16:36

WIP with create_rules_schema

52c53d6

Added createSchema and put it into place

c8d607f

Merge branch 'master' into add-create-schema

87740c9

Updated test to pass

1c31834

Added workaround for the UI sending down strings to get the server to…

427019b

… build

Fixed code for the unit test for the workaround

e7b6a5d

Merge branch 'master' into add-create-schema

c96a6e3

Fixed the code and tests for risk score on the front end to use a num…

1a8e004

…ber and removed back end TODO

Merge branch 'master' into add-create-schema

2bcfaf3

Removed duplicate files in favor of types existing within SIEM

c822956

Added the create rules bulk and fixed type errors as well as deleted …

115a803

…the older joi schema and used newer mock methodologies

Added forgotten query and language sections for the create rules bulk

f9849d7

FrankHassanabad changed the title ~~[SIEM][Detection Engine] Change create rules REST route to use io-ts and remove joi~~ [SIEM][Detection Engine] Changes create and create_bulk rules REST route to use io-ts and remove joi Jun 4, 2020

FrankHassanabad added 2 commits June 4, 2020 13:16

Merge branch 'master' into add-create-schema

2f78f59

Updated to the latest folder change

bb2c276

FrankHassanabad self-assigned this Jun 4, 2020

FrankHassanabad added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.9.0 labels Jun 4, 2020

FrankHassanabad marked this pull request as ready for review June 4, 2020 20:05

FrankHassanabad requested review from a team as code owners June 4, 2020 20:05

FrankHassanabad added the Team:SIEM label Jun 4, 2020

FrankHassanabad commented Jun 4, 2020

View reviewed changes

FrankHassanabad added 4 commits June 4, 2020 14:12

Deleted the old joi schemas

7105a2d

Added update rules and update rules bulk and fixed bugs found within …

c785a0d

…the create schema with regards to validation

Merge branch 'master' into add-create-schema

503c616

Fixed end to end tests where the schema is now stricter to work with …

0a809be

…the stricter types for the tests

FrankHassanabad changed the title ~~[SIEM][Detection Engine] Changes create and create_bulk rules REST route to use io-ts and remove joi~~ [SIEM][Detection Engine] Changes create, create_bulk, update, update_bulk REST routes to use io-ts and remove joi Jun 5, 2020

FrankHassanabad added 8 commits June 5, 2020 18:12

Added the patch conversion and updated tests

9378017

Merge branch 'master' into add-create-schema

d6cc52c

prepackaged rules converted

26b37dd

Converted the query schema which includes the read and delete endpoin…

09e50a4

…ts from joi to io-ts

Converted import from joi to io-ts, updated tests and had to remove o…

ae07f4b

…ne since io-ts is slower than joi for importing

Converted export from joi -> iots

347c5a7

Converted find from joi -> io-ts

a6ebe61

Converted the statuses and left overs from joi -> io-ts

df139c2

FrankHassanabad changed the title ~~[SIEM][Detection Engine] Changes create, create_bulk, update, update_bulk REST routes to use io-ts and remove joi~~ [SIEM][Detection Engine] Converts from joi to use io-ts and moves the types to common Jun 7, 2020

FrankHassanabad added 2 commits June 7, 2020 18:30

Merge branch 'master' into add-create-schema

fa51402

Merge branch 'master' into add-create-schema

8be7378

rylnd reviewed Jun 8, 2020

View reviewed changes

FrankHassanabad added 3 commits June 8, 2020 17:35

Fixed issue with bad conflict

3d41e5a

Merge branch 'master' into add-create-schema

d9993dd

Fixes from code review

b06403c

rylnd approved these changes Jun 9, 2020

View reviewed changes

FrankHassanabad merged commit d99cf75 into elastic:master Jun 9, 2020

FrankHassanabad deleted the add-create-schema branch June 9, 2020 01:54

FrankHassanabad mentioned this pull request Jun 9, 2020

[7.x] [SIEM][Detection Engine] Converts from joi to use io-ts and moves the types to common (#68127) #68611

Merged

FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Jun 9, 2020

Follow up PR with smaller things from PR elastic#68127

e258ecc

FrankHassanabad mentioned this pull request Jun 9, 2020

[SIEM][Detection Engine] Follow up issues from PR 68127 #68612

Merged

1 task

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Conversation

FrankHassanabad commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Jun 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankHassanabad Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankHassanabad Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 9, 2020

💚 Build Succeeded

History

Uh oh!

elasticmachine commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FrankHassanabad commented Jun 3, 2020 •

edited

Loading

FrankHassanabad Jun 8, 2020 •

edited

Loading

FrankHassanabad Jun 8, 2020 •

edited

Loading