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] move away from Joi for importing/exporting timeline #62125

Merged
merged 24 commits into from
Apr 16, 2020

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Apr 1, 2020

Summary

  1. This PR is to remove the schemas created with Joi and reuse the existing io-ts schemas
  2. After migrating to io-ts, this case should work as well
    timelines_export_noFilterQuery.ndjson.txt
    (To verify this, please download the attachment and rename it with .ndjson)
  3. The validation error is updated accordingly due to the migration:

e.g:

Request body takes:

{
  ids: string[];
}

but we send { id: 'someId' } instead

Before the PR, the error message was

'"id" is not allowed'

After the PR, the error message becomes

'Invalid value undefined supplied to : { ids: Array<string> }/ids: Array<string>'

filter_query

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@angorayc angorayc requested a review from a team as a code owner April 1, 2020 10:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

expression: allowEmptyString,
}),
serializedQuery: allowEmptyString,
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.allow(null)

@angorayc angorayc added the v8.0.0 label Apr 2, 2020
@angorayc angorayc added the release_note:skip Skip the PR/issue when compiling release notes label Apr 2, 2020
import { getExportTimelineByObjectIds } from './utils/export_timelines';

export const exportTimelinesRoute = (router: IRouter, config: LegacyServices['config']) => {
router.post(
{
path: TIMELINE_EXPORT_URL,
validate: {
query: buildRouteValidation<ExportTimelineRequestParams['query']>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@angorayc I was imagining that once Joi was gone, this buildRouteValidation function would be modified to take an io-ts schema and perform whatever decoding/validation needs to happen.

It looks like we're currently loosely typing the request with @kbn/config-schema in the validation step, and then refining that further within the route handler with io-ts; is it possible to simply use io-ts in the validation step, similar to what buildRouteValidation was doing? If we can we should, as that removes the possibility of interacting with these "loose" types in the handler and losing type safety there (and removes the double validation).

Copy link
Contributor

@FrankHassanabad FrankHassanabad Apr 4, 2020

Choose a reason for hiding this comment

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

@rylnd , @angorayc Here is my draft PR where I am doing what @rylnd is suggesting for detection engine:

Example of where I validate incoming using io-ts to search for:

validate: {
        body: buildRouteValidationIoTS<CreateListsSchema>(createListsSchema),
      },

My function which performs an io-ts validation similar to the one above it that @rylnd is using:
https://github.com/elastic/kibana/pull/62552/files#diff-8b8c8b8c0404c58c669ab9551d6f2e8bL238-L239

export const buildRouteValidationIoTS = <T>(schema: t.Mixed): RouteValidationFunction<T> => (
  payload: object,
  { ok, badRequest }
) => {
  const decoded = schema.decode(payload);
  const checked = exactCheck(payload, decoded);
  const left = (errors: t.Errors): RouteValidationFunctionReturn<T> => {
    return badRequest(formatErrors(errors).join(','));
  };
  const right = (output: T): RouteValidationFunctionReturn<T> => {
    return ok(output);
  };
  return pipe(checked, fold(left, right));
};

This is very draft so again, refactoring/changing things afterwards is good but if it helps I can begin pulling in some of these utilities as soon as this the beginning of this week @rylnd to start helping reducing our usage of schemas and establishing better patterns.

It might be best I begin pulling in my pieces from draft mode in smaller PR's Monday and getting those reviewed. Then we can migrate any other things like this after 7.7.0 is released as the time is running out quickly?

I don't want to hold anything up though. Refactoring can happen after this PR too and my pieces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's beautiful @FrankHassanabad, @rylnd I've been trying to figure this out for a while! If I'm going to reuse it in timeline's route once it's merged, would you recommend importing from this file or there's better way of how we could reuse it? As currently I use lots of stuff from detection engine etc., not sure if we could have an utils for all the routes? Or is it possible if we implement this part in middleware so we can just provide the validation schemas in each route without implementing buildRouteValidationIoTS ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's tricky at the moment but we should be promoting things "up" as you are suggesting and out of detection engine as they see more re-use. In your PR's you can move things around as you see fit or if we have some duplication we can deconflict as well.

It's tricky as we are all working in the same areas but getting code in first and working is probably easier and then refactors second once people move to other areas maybe?

I am fine with either approach and I like your ideas. The middle ware idea is a good one but I haven't written those before with Hapi or the Kibana platform but am open to any and all ideas if you want to give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @FrankHassanabad , at the same time I found some code useful for validation,
https://elastic.slack.com/archives/CCJQY1NS0/p1584130424322000
It matches what I thought and fixes the incorrect error message I had, what do you think?

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: if you need to override the output type above (A), you'll have to do something like:

buildRouteValidation<t.Any, OverridingResponseType>(schema)

It feels like there should be a way to support overriding the type with a single generic argument, but I hit the limits of my typescript knowledge trying to make that work.

Copy link
Contributor Author

@angorayc angorayc Apr 14, 2020

Choose a reason for hiding this comment

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

Thanks @rylnd , I've applied the suggested type and it works well. I agree that we should warning user if their input got dropped, and given that @FrankHassanabad has implemented the solution nicely, I'd like to park here and creating an issue to keep track of this.
https://github.com/elastic/siem-team/issues/615

Copy link
Contributor Author

@angorayc angorayc Apr 15, 2020

Choose a reason for hiding this comment

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

I found in x-pack/plugins/case/common/api/runtime_types.ts Line. 37
Seems it's doing similar thing as exact check, which means when validation error occurs, it return the original object.

e.g.:
I updated the buildValidation:

export const buildRouteValidation = <T extends rt.Mixed, A = rt.TypeOf<T>>(
  schema: T
): RouteValidationFunction<A> => (
  inputValue: unknown,
  validationResult: RouteValidationResultFactory
) =>
  pipe(
    excess(schema).decode(inputValue),
    fold<rt.Errors, A, RequestValidationResult<A>>(
      (errors: rt.Errors) => validationResult.badRequest(failure(errors).join('\n')),
      (validatedInput: A) => validationResult.ok(validatedInput)
    )
  );

Use case:
I sent this as request body:
{ id: 'someId' }
but it's expecting
{ id: string[] }

The error message was
"Invalid value undefined supplied to : { ids: Array }/ids: Array"
It becomes
Received: "Invalid value {"id":"someId"} supplied to : { ids: Array }, excess properties: ["id"]"

Given that io-ts by default drops the redundant keys and provides the error messages not easy to read, I can see why we have patches for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, we can address this as part of #63525.

@angorayc angorayc closed this Apr 8, 2020
@angorayc angorayc reopened this Apr 8, 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.

@angorayc it looks like this PR is still a WIP so these are just comments, but I think we can make this even slicker than it is right now!

import { getExportTimelineByObjectIds } from './utils/export_timelines';

export const exportTimelinesRoute = (router: IRouter, config: LegacyServices['config']) => {
router.post(
{
path: TIMELINE_EXPORT_URL,
validate: {
query: buildRouteValidation<ExportTimelineRequestParams['query']>(

This comment was marked as resolved.

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.

I think we've got some redundant code and types that should be cleaned up, but we're so close! Thank you for adding/updating tests and for taking on establishment of this validation pattern 🙏

},
});
};

// TODO: Capture both the line number and the rule_id if you have that information for the error message
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment (and the function that follows it) are now moved elsewhere, and can be deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

@angorayc I think the createRulesStreamFromNdJson definition below is dead code, but we can clean it up in a subsequent PR if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I shouldn't have put createRulesStreamFromNdJson into server/utils as this is only consumed by import_rules_route, I'll remove the one in server/utils and keep this one here.

Regarding to the file create_rules_stream_from_ndjson.ts in server/utils, I renamed it to create_stream_from_ndjson.ts.

@@ -334,7 +334,10 @@ describe('import timelines', () => {
const result = server.validate(request);

expect(result.badRequest).toHaveBeenCalledWith(
'child "file" fails because ["file" is required]'
[
'Invalid value undefined supplied to : { file: (ReadableRt & { hapi: { filename: string } }) }/file: (ReadableRt & { hapi: { filename: string } })/0: ReadableRt',
Copy link
Contributor

Choose a reason for hiding this comment

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

😅 that's quite the error message ... harder to parse, but more info than before!

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 see how you mean, I found some example in case as well, please find this comment #62125 (comment)

x-pack/legacy/plugins/siem/server/lib/timeline/types.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/siem/server/lib/timeline/types.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/siem/server/lib/timeline/types.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/siem/server/lib/timeline/types.ts Outdated Show resolved Hide resolved
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.

LGTM. I think we still have some dead code but up to you whether you want to clean that up here or subsequently. Thanks for the feedback changes!

},
});
};

// TODO: Capture both the line number and the rule_id if you have that information for the error message
Copy link
Contributor

Choose a reason for hiding this comment

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

@angorayc I think the createRulesStreamFromNdJson definition below is dead code, but we can clean it up in a subsequent PR if you'd like.

import { getExportTimelineByObjectIds } from './utils/export_timelines';

export const exportTimelinesRoute = (router: IRouter, config: LegacyServices['config']) => {
router.post(
{
path: TIMELINE_EXPORT_URL,
validate: {
query: buildRouteValidation<ExportTimelineRequestParams['query']>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, we can address this as part of #63525.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@angorayc angorayc merged commit 7b74aa9 into elastic:master Apr 16, 2020
angorayc added a commit to angorayc/kibana that referenced this pull request Apr 16, 2020
…2125)

* move away from joi

* update schema for filterQuery

* fix types

* update schemas

* remove boom

* remove redundant params

* reuse utils from case

* update schemas for query params and body

* fix types

* update validation schema

* fix unit test

* update description for test cases

* remove import from case

* lifting common libs

* fix dependency

* lifting validation builder function

* add unit test

* fix for code review

* reve comments

* rename common utils

* fix types
angorayc added a commit that referenced this pull request Apr 16, 2020
…63673)

* move away from joi

* update schema for filterQuery

* fix types

* update schemas

* remove boom

* remove redundant params

* reuse utils from case

* update schemas for query params and body

* fix types

* update validation schema

* fix unit test

* update description for test cases

* remove import from case

* lifting common libs

* fix dependency

* lifting validation builder function

* add unit test

* fix for code review

* reve comments

* rename common utils

* fix types
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 17, 2020
* master: (56 commits)
  [i18n] Update CODEOWNERS (elastic#63354)
  add platform team definition of done (elastic#59993)
  [SIEM] move away from Joi for importing/exporting timeline (elastic#62125)
  Fix discover preserve url (elastic#63580)
  [alerting] Adds an alertServices mock and uses it in siem, monitoring and uptime (elastic#63489)
  Closes elastic#63109 for Service Map by resetting edges styles for the selected node (elastic#63655)
  MIgrated index_header to react (elastic#63490)
  Index pattern management UI -> TypeScript and New Platform Ready (indexed_fields_table) (elastic#63364)
  [SIEM] [Cases] Insert timeline and reporters/tags in table bug fixes (elastic#63642)
  [Reporting] Make usable default element positions (elastic#63191)
  [Reporting] Switch Serverside Config Wrapper to NP (elastic#62500)
  [Reporting] Add "warning" status as an alternate type of completed job (elastic#63498)
  Split action types into own page (elastic#63516)
  [Lens] Only show copy on save for previously saved docs (elastic#63535)
  Update README.md (elastic#63622)
  Bugfix clear saved query crashes kibana on Discover in some cases (elastic#63554)
  Add uptime CODEOWNER entries. (elastic#63616)
  [ML] Extract apiDoc params from the schema definitions (elastic#62933)
  Fix alerting documentation encryption key requirement (elastic#63512)
  Fix CODEOWNERS and sass lint paths (elastic#63552)
  ...
@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 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants