Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SIEM][Detection Engine] Converts from joi to use io-ts and moves the types to common #68127

Merged
merged 34 commits into from
Jun 9, 2020

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Jun 3, 2020

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.

@FrankHassanabad 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 FrankHassanabad self-assigned this Jun 4, 2020
@FrankHassanabad FrankHassanabad added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.9.0 labels Jun 4, 2020
@FrankHassanabad FrankHassanabad marked this pull request as ready for review June 4, 2020 20:05
@FrankHassanabad FrankHassanabad requested review from a team as code owners June 4, 2020 20:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

@FrankHassanabad 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 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
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
options: {
tags: ['access:securitySolution'],
tags: ['access:siem'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad conflict will fix and re-push.


export const createRulesBulkRoute = (router: IRouter, ml: SetupPlugins['ml']) => {
router.post(
{
path: `${DETECTION_ENGINE_RULES_URL}/_bulk_create`,
validate: {
body: buildRouteValidation<RuleAlertParamsRest[]>(createRulesBulkSchema),
body: buildRouteValidation<typeof createRulesBulkSchema, CreateRulesBulkSchemaDecoded>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

riskScore: 80,
severity: 'low',
tags: [],
threat: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's that structure we were talking about today 😉

@@ -87,6 +87,7 @@ export const schema: FormSchema = {
},
riskScore: {
type: FIELD_TYPES.RANGE,
serializer: (input: string) => Number(input),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* - 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>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

💚 Build Succeeded

History

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

@FrankHassanabad FrankHassanabad merged commit d99cf75 into elastic:master Jun 9, 2020
@FrankHassanabad FrankHassanabad deleted the add-create-schema branch June 9, 2020 01:54
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Jun 9, 2020
… 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
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Jun 9, 2020
FrankHassanabad added a commit that referenced this pull request Jun 9, 2020
… 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
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 9, 2020
* 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)
  ...
FrankHassanabad added a commit that referenced this pull request Jun 10, 2020
## 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
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Jun 10, 2020
## 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
FrankHassanabad added a commit that referenced this pull request Jun 10, 2020
)

## 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
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants