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.

Sign up for GitHub

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
Merged

[SIEM] move away from Joi for importing/exporting timeline #62125

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ee11235
move away from joi
angorayc Mar 31, 2020
419b083
update schema for filterQuery
angorayc Apr 1, 2020
1097bb1
fix types
angorayc Apr 2, 2020
63ed91e
update schemas
angorayc Apr 2, 2020
6e84ffb
Merge remote-tracking branch 'upstream/master' into update-schemas
angorayc Apr 2, 2020
7314490
remove boom
angorayc Apr 2, 2020
b752c0d
remove redundant params
angorayc Apr 2, 2020
26235e5
reuse utils from case
angorayc Apr 2, 2020
0fd8ec7
update schemas for query params and body
angorayc Apr 8, 2020
a50e769
fix types
angorayc Apr 8, 2020
dffeda6
Merge remote-tracking branch 'upstream/master' into update-schemas
angorayc Apr 8, 2020
58b757b
update validation schema
angorayc Apr 8, 2020
546badc
fix unit test
angorayc Apr 8, 2020
f591358
update description for test cases
angorayc Apr 9, 2020
d34f645
remove import from case
angorayc Apr 9, 2020
a9bc45d
lifting common libs
angorayc Apr 9, 2020
9283eff
fix dependency
angorayc Apr 9, 2020
f8b3b03
lifting validation builder function
angorayc Apr 14, 2020
4655aea
Merge remote-tracking branch 'upstream/master' into update-schemas
angorayc Apr 14, 2020
81b9859
add unit test
angorayc Apr 14, 2020
cb05d5d
fix for code review
angorayc Apr 15, 2020
4f45e17
reve comments
angorayc Apr 15, 2020
ddf4409
rename common utils
angorayc Apr 15, 2020
2689f3b
fix types
angorayc Apr 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 8 additions & 12 deletions x-pack/legacy/plugins/siem/server/lib/timeline/create_timelines_stream_from_ndjson.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
*/

import { Transform } from 'stream';
import { identity } from 'fp-ts/lib/function';
import Boom from 'boom';
angorayc marked this conversation as resolved.
Show resolved Hide resolved
import { pipe } from 'fp-ts/lib/pipeable';
import { fold } from 'fp-ts/lib/Either';
import {
createConcatStream,
createSplitStream,
Expand All @@ -15,22 +19,14 @@ import {
filterExportedCounts,
createLimitStream,
} from '../detection_engine/rules/create_rules_stream_from_ndjson';
import { importTimelinesSchema } from './routes/schemas/import_timelines_schema';
import { BadRequestError } from '../detection_engine/errors/bad_request_error';

import { ImportTimelineResponse } from './routes/utils/import_timelines';
import { throwErrors } from '../../../../../../plugins/case/common/api';
import { importTimelinesSchema } from './routes/schemas/import_timelines_schema';

export const validateTimelines = (): Transform => {
return createMapStream((obj: ImportTimelineResponse) => {
if (!(obj instanceof Error)) {
const validated = importTimelinesSchema.validate(obj);
if (validated.error != null) {
return new BadRequestError(validated.error.message);
} else {
return validated.value;
}
} else {
return obj;
}
return pipe(importTimelinesSchema.decode(obj), fold(throwErrors(Boom.badRequest), identity));
});
};

Expand Down
26 changes: 11 additions & 15 deletions x-pack/legacy/plugins/siem/server/lib/timeline/routes/export_timelines_route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,29 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { schema } from '@kbn/config-schema';

import { set as _set } from 'lodash/fp';
import { IRouter } from '../../../../../../../../src/core/server';
import { LegacyServices } from '../../../types';
import { ExportTimelineRequestParams } from '../types';

import {
transformError,
buildRouteValidation,
buildSiemResponse,
} from '../../detection_engine/routes/utils';
import { transformError, buildSiemResponse } from '../../detection_engine/routes/utils';
import { TIMELINE_EXPORT_URL } from '../../../../common/constants';

import {
exportTimelinesSchema,
exportTimelinesQuerySchema,
} from './schemas/export_timelines_schema';

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 •
edited
Loading

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.

Sign in to view

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 •
edited
Loading

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 •
edited
Loading

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.

exportTimelinesQuerySchema
),
body: buildRouteValidation<ExportTimelineRequestParams['body']>(exportTimelinesSchema),
query: schema.object({
file_name: schema.string(),
exclude_export_details: schema.boolean(),
}),
body: schema.object({
ids: schema.arrayOf(schema.string()),
}),
},
options: {
tags: ['access:siem'],
Expand All @@ -42,6 +37,7 @@ export const exportTimelinesRoute = (router: IRouter, config: LegacyServices['co
const siemResponse = buildSiemResponse(response);
const savedObjectsClient = context.core.savedObjects.client;
const exportSizeLimit = config().get<number>('savedObjects.maxImportExportSize');

if (request.body?.ids != null && request.body.ids.length > exportSizeLimit) {
return siemResponse.error({
statusCode: 400,
Expand Down
52 changes: 31 additions & 21 deletions x-pack/legacy/plugins/siem/server/lib/timeline/routes/import_timelines_route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,13 @@

import { extname } from 'path';
import { chunk, omit, set } from 'lodash/fp';
import Boom from 'boom';
import { pipe } from 'fp-ts/lib/pipeable';
import { fold } from 'fp-ts/lib/Either';
import { identity } from 'fp-ts/lib/function';
import { schema } from '@kbn/config-schema';
import { Readable } from 'stream';
import {
buildRouteValidation,
buildSiemResponse,
createBulkErrorObject,
BulkError,
Expand All @@ -23,7 +28,6 @@ import {
isBulkError,
isImportRegular,
ImportTimelineResponse,
ImportTimelinesRequestParams,
ImportTimelinesSchema,
PromiseFromStreams,
} from './utils/import_timelines';
Expand All @@ -38,6 +42,7 @@ import { LegacyServices } from '../../../types';
import { Timeline } from '../saved_object';
import { validate } from '../../detection_engine/routes/rules/validate';
import { FrameworkRequest } from '../../framework';
import { throwErrors } from '../../../../../../../plugins/case/common/api';

const CHUNK_PARSED_OBJECT_SIZE = 10;

Expand All @@ -52,9 +57,7 @@ export const importTimelinesRoute = (
{
path: `${TIMELINE_IMPORT_URL}`,
validate: {
body: buildRouteValidation<ImportTimelinesRequestParams['body']>(
importTimelinesPayloadSchema
),
body: schema.object({}, { unknowns: 'allow' }),
},
options: {
tags: ['access:siem'],
Expand All @@ -65,28 +68,34 @@ export const importTimelinesRoute = (
},
},
async (context, request, response) => {
const siemResponse = buildSiemResponse(response);
const savedObjectsClient = context.core.savedObjects.client;
if (!savedObjectsClient) {
return siemResponse.error({ statusCode: 404 });
}
const { filename } = request.body.file.hapi;
try {
const siemResponse = buildSiemResponse(response);
const savedObjectsClient = context.core.savedObjects.client;
if (!savedObjectsClient) {
return siemResponse.error({ statusCode: 404 });
}

const fileExtension = extname(filename).toLowerCase();
const { file } = pipe(
angorayc marked this conversation as resolved.
Show resolved Hide resolved
importTimelinesPayloadSchema.decode(request.body),
fold(throwErrors(Boom.badRequest), identity)
);

if (fileExtension !== '.ndjson') {
return siemResponse.error({
statusCode: 400,
body: `Invalid file extension ${fileExtension}`,
});
}
const { filename } = file.hapi;

const objectLimit = config().get<number>('savedObjects.maxImportExportSize');
const fileExtension = extname(filename).toLowerCase();

if (fileExtension !== '.ndjson') {
return siemResponse.error({
statusCode: 400,
body: `Invalid file extension ${fileExtension}`,
});
}

const objectLimit = config().get<number>('savedObjects.maxImportExportSize');

try {
const readStream = createTimelinesStreamFromNdJson(objectLimit);
const parsedObjects = await createPromiseFromStreams<PromiseFromStreams[]>([
request.body.file,
(file as unknown) as Readable,
angorayc marked this conversation as resolved.
Show resolved Hide resolved
...readStream,
]);
const [duplicateIdErrors, uniqueParsedObjects] = getTupleDuplicateErrorsAndUniqueTimeline(
Expand Down Expand Up @@ -215,6 +224,7 @@ export const importTimelinesRoute = (
}
} catch (err) {
const error = transformError(err);
const siemResponse = buildSiemResponse(response);

return siemResponse.error({
body: error.message,
Expand Down
17 changes: 6 additions & 11 deletions x-pack/legacy/plugins/siem/server/lib/timeline/routes/schemas/export_timelines_schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import Joi from 'joi';
import * as runtimeTypes from 'io-ts';

/* eslint-disable @typescript-eslint/camelcase */
import { ids, exclude_export_details, file_name } from './schemas';
/* eslint-disable @typescript-eslint/camelcase */
const ids = runtimeTypes.array(runtimeTypes.string);
export const exportTimelinesSchema = runtimeTypes.type({ ids });

export const exportTimelinesSchema = Joi.object({
ids,
}).min(1);

export const exportTimelinesQuerySchema = Joi.object({
file_name: file_name.default('export.ndjson'),
exclude_export_details: exclude_export_details.default(false),
export const exportTimelinesQuerySchema = runtimeTypes.type({
file_name: runtimeTypes.string,
exclude_export_details: runtimeTypes.boolean,
});
73 changes: 23 additions & 50 deletions x-pack/legacy/plugins/siem/server/lib/timeline/routes/schemas/import_timelines_schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,55 +3,28 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import Joi from 'joi';
import {
columns,
created,
createdBy,
dataProviders,
dateRange,
description,
eventNotes,
eventType,
favorite,
filters,
globalNotes,
kqlMode,
kqlQuery,
savedObjectId,
savedQueryId,
sort,
title,
updated,
updatedBy,
version,
pinnedEventIds,
} from './schemas';
import * as runtimeTypes from 'io-ts';

export const importTimelinesPayloadSchema = Joi.object({
file: Joi.object().required(),
});
import { eventNotes, globalNotes, pinnedEventIds } from './schemas';
import { SavedTimelineRuntimeType } from '../../types';

export const importTimelinesSchema = Joi.object({
columns,
created,
createdBy,
dataProviders,
dateRange,
description,
eventNotes,
eventType,
filters,
favorite,
globalNotes,
kqlMode,
kqlQuery,
savedObjectId,
savedQueryId,
sort,
title,
updated,
updatedBy,
version,
pinnedEventIds,
});
const file = runtimeTypes.intersection([
runtimeTypes.UnknownRecord,
runtimeTypes.type({
hapi: runtimeTypes.type({ filename: runtimeTypes.string }),
}),
]);
export const importTimelinesPayloadSchema = runtimeTypes.type({ file });

export const importTimelinesSchema = runtimeTypes.intersection([
SavedTimelineRuntimeType,
runtimeTypes.type({
savedObjectId: runtimeTypes.string,
version: runtimeTypes.string,
}),
runtimeTypes.type({
globalNotes,
eventNotes,
pinnedEventIds,
}),
]);
Loading