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
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
import { createMockConfig, requestContextMock, serverMock, requestMock } from '../__mocks__';
import { importRulesRoute } from './import_rules_route';
import { DEFAULT_SIGNALS_INDEX } from '../../../../../common/constants';
import * as createRulesStreamFromNdJson from '../../rules/create_rules_stream_from_ndjson';
import * as createRulesStreamFromNdJson from '../../../../utils/read_stream/create_rules_stream_from_ndjson';
import { setFeatureFlagsForTestsOnly, unSetFeatureFlagsForTestsOnly } from '../../feature_flags';

describe('import_rules_route', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
buildSiemResponse,
validateLicenseForRuleType,
} from '../utils';
import { createRulesStreamFromNdJson } from '../../rules/create_rules_stream_from_ndjson';
import { createRulesStreamFromNdJson } from '../../../../utils/read_stream/create_rules_stream_from_ndjson';
import { ImportRuleAlertRest } from '../../types';
import { patchRules } from '../../rules/patch_rules';
import { importRulesQuerySchema, importRulesPayloadSchema } from '../schemas/import_rules_schema';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
transformTags,
getIdBulkError,
transformOrBulkError,
transformDataToNdjson,
transformAlertsToRules,
transformOrImportError,
getDuplicates,
Expand All @@ -22,9 +21,8 @@ import { getResult } from '../__mocks__/request_responses';
import { INTERNAL_IDENTIFIER } from '../../../../../common/constants';
import { ImportRuleAlertRest, RuleAlertParamsRest, RuleTypeParams } from '../../types';
import { BulkError, ImportSuccessError } from '../utils';
import { sampleRule } from '../../signals/__mocks__/es_results';
import { getSimpleRule, getOutputRuleAlertForRest } from '../__mocks__/utils';
import { createRulesStreamFromNdJson } from '../../rules/create_rules_stream_from_ndjson';
import { createRulesStreamFromNdJson } from '../../../../utils/read_stream/create_rules_stream_from_ndjson';
import { createPromiseFromStreams } from '../../../../../../../../../src/legacy/utils/streams';
import { PartialAlert } from '../../../../../../../../plugins/alerting/server';
import { SanitizedAlert } from '../../../../../../../../plugins/alerting/server/types';
Expand Down Expand Up @@ -396,47 +394,6 @@ describe('utils', () => {
});
});

describe('transformDataToNdjson', () => {
test('if rules are empty it returns an empty string', () => {
const ruleNdjson = transformDataToNdjson([]);
expect(ruleNdjson).toEqual('');
});

test('single rule will transform with new line ending character for ndjson', () => {
const rule = sampleRule();
const ruleNdjson = transformDataToNdjson([rule]);
expect(ruleNdjson.endsWith('\n')).toBe(true);
});

test('multiple rules will transform with two new line ending characters for ndjson', () => {
const result1 = sampleRule();
const result2 = sampleRule();
result2.id = 'some other id';
result2.rule_id = 'some other id';
result2.name = 'Some other rule';

const ruleNdjson = transformDataToNdjson([result1, result2]);
// this is how we count characters in JavaScript :-)
const count = ruleNdjson.split('\n').length - 1;
expect(count).toBe(2);
});

test('you can parse two rules back out without errors', () => {
const result1 = sampleRule();
const result2 = sampleRule();
result2.id = 'some other id';
result2.rule_id = 'some other id';
result2.name = 'Some other rule';

const ruleNdjson = transformDataToNdjson([result1, result2]);
const ruleStrings = ruleNdjson.split('\n');
const reParsed1 = JSON.parse(ruleStrings[0]);
const reParsed2 = JSON.parse(ruleStrings[1]);
expect(reParsed1).toEqual(result1);
expect(reParsed2).toEqual(result2);
});
});

describe('transformAlertsToRules', () => {
test('given an empty array returns an empty array', () => {
expect(transformAlertsToRules([])).toEqual([]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,15 +152,6 @@ export const transformAlertToRule = (
});
};

export const transformDataToNdjson = (data: unknown[]): string => {
if (data.length !== 0) {
const dataString = data.map(rule => JSON.stringify(rule)).join('\n');
return `${dataString}\n`;
} else {
return '';
}
};

export const transformAlertsToRules = (
alerts: RuleAlertType[]
): Array<Partial<OutputRuleAlertRest>> => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,39 +4,19 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { Transform } from 'stream';
import { has, isString } from 'lodash/fp';
import { ImportRuleAlertRest } from '../types';
import {
createSplitStream,
createMapStream,
createFilterStream,
createConcatStream,
} from '../../../../../../../../src/legacy/utils/streams';
import { importRulesSchema } from '../routes/schemas/import_rules_schema';
import { BadRequestError } from '../errors/bad_request_error';

export interface RulesObjectsExportResultDetails {
/** number of successfully exported objects */
exportedCount: number;
}

export const parseNdjsonStrings = (): Transform => {
return createMapStream((ndJsonStr: string) => {
if (isString(ndJsonStr) && ndJsonStr.trim() !== '') {
try {
return JSON.parse(ndJsonStr);
} catch (err) {
return err;
}
}
});
};

export const filterExportedCounts = (): Transform => {
return createFilterStream<ImportRuleAlertRest | RulesObjectsExportResultDetails>(
obj => obj != null && !has('exported_count', obj)
);
};
import {
parseNdjsonStrings,
filterExportedCounts,
createLimitStream,
} from '../../../utils/read_stream/create_rules_stream_from_ndjson';

export const validateRules = (): Transform => {
return createMapStream((obj: ImportRuleAlertRest) => {
Expand All @@ -53,21 +33,6 @@ export const validateRules = (): Transform => {
});
};

// Adaptation from: saved_objects/import/create_limit_stream.ts
export const createLimitStream = (limit: number): Transform => {
let counter = 0;
return new Transform({
objectMode: true,
async transform(obj, _, done) {
if (counter >= limit) {
return done(new Error(`Can't import more than ${limit} rules`));
}
counter++;
done(undefined, obj);
},
});
};

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

// eventually and then pass it down so we can give error messages on the line number

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
import { AlertsClient } from '../../../../../../../plugins/alerting/server';
import { getNonPackagedRules } from './get_existing_prepackaged_rules';
import { getExportDetailsNdjson } from './get_export_details_ndjson';
import { transformAlertsToRules, transformDataToNdjson } from '../routes/rules/utils';
import { transformAlertsToRules } from '../routes/rules/utils';
import { transformDataToNdjson } from '../../../utils/read_stream/create_rules_stream_from_ndjson';

export const getExportAll = async (
alertsClient: AlertsClient
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import { AlertsClient } from '../../../../../../../plugins/alerting/server';
import { getExportDetailsNdjson } from './get_export_details_ndjson';
import { isAlertType } from '../rules/types';
import { readRules } from './read_rules';
import { transformDataToNdjson, transformAlertToRule } from '../routes/rules/utils';
import { transformAlertToRule } from '../routes/rules/utils';
import { OutputRuleAlertRest } from '../types';
import { transformDataToNdjson } from '../../../utils/read_stream/create_rules_stream_from_ndjson';

interface ExportSuccesRule {
statusCode: 200;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import * as rt from 'io-ts';
import { Transform } from 'stream';
import { pipe } from 'fp-ts/lib/pipeable';
import { fold } from 'fp-ts/lib/Either';
import { failure } from 'io-ts/lib/PathReporter';
import { identity } from 'fp-ts/lib/function';
import {
createConcatStream,
createSplitStream,
Expand All @@ -14,26 +18,28 @@ import {
parseNdjsonStrings,
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';
} from '../../utils/read_stream/create_rules_stream_from_ndjson';

import { ImportTimelineResponse } from './routes/utils/import_timelines';
import { ImportTimelinesSchemaRt } from './routes/schemas/import_timelines_schema';

type ErrorFactory = (message: string) => Error;

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;
}
});
export const createPlainError = (message: string) => new Error(message);

export const throwErrors = (createError: ErrorFactory) => (errors: rt.Errors) => {
throw createError(failure(errors).join('\n'));
};

export const decodeOrThrow = <A, O, I>(
runtimeType: rt.Type<A, O, I>,
createError: ErrorFactory = createPlainError
) => (inputValue: I) =>
pipe(runtimeType.decode(inputValue), fold(throwErrors(createError), identity));

export const validateTimelines = (): Transform =>
createMapStream((obj: ImportTimelineResponse) => decodeOrThrow(ImportTimelinesSchemaRt)(obj));

export const createTimelinesStreamFromNdJson = (ruleLimit: number) => {
return [
createSplitStream('\n'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,16 @@

import { TIMELINE_EXPORT_URL, TIMELINE_IMPORT_URL } from '../../../../../common/constants';
import { requestMock } from '../../../detection_engine/routes/__mocks__';

import stream from 'stream';
const readable = new stream.Readable();
export const getExportTimelinesRequest = () =>
requestMock.create({
method: 'get',
path: TIMELINE_EXPORT_URL,
query: {
file_name: 'mock_export_timeline.ndjson',
exclude_export_details: 'false',
},
body: {
ids: ['f0e58720-57b6-11ea-b88d-3f1a31716be8', '890b8ae0-57df-11ea-a7c9-3976b7f1cb37'],
},
Expand All @@ -22,7 +27,7 @@ export const getImportTimelinesRequest = (filename?: string) =>
path: TIMELINE_IMPORT_URL,
query: { overwrite: false },
body: {
file: { hapi: { filename: filename ?? 'filename.ndjson' } },
file: { ...readable, hapi: { filename: filename ?? 'filename.ndjson' } },
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,34 @@ describe('export timelines', () => {
});

describe('request validation', () => {
test('disallows singular id query param', async () => {
test('return validation error for request body', async () => {
const request = requestMock.create({
method: 'get',
path: TIMELINE_EXPORT_URL,
body: { id: 'someId' },
});
const result = server.validate(request);

expect(result.badRequest).toHaveBeenCalledWith('"id" is not allowed');
expect(result.badRequest.mock.calls[0][0]).toEqual(
'Invalid value undefined supplied to : { ids: Array<string> }/ids: Array<string>'
);
});

test('return validation error for request params', async () => {
const request = requestMock.create({
method: 'get',
path: TIMELINE_EXPORT_URL,
body: { id: 'someId' },
});
const result = server.validate(request);

expect(result.badRequest.mock.calls[1][0]).toEqual(
[
'Invalid value undefined supplied to : { file_name: string, exclude_export_details: ("true" | "false") }/file_name: string',
'Invalid value undefined supplied to : { file_name: string, exclude_export_details: ("true" | "false") }/exclude_export_details: ("true" | "false")/0: "true"',
'Invalid value undefined supplied to : { file_name: string, exclude_export_details: ("true" | "false") }/exclude_export_details: ("true" | "false")/1: "false"',
].join('\n')
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,29 @@
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 { getExportTimelineByObjectIds } from './utils/export_timelines';
import {
exportTimelinesSchema,
exportTimelinesQuerySchema,
exportTimelinesRequestBodySchema,
buildRouteValidation,
} from './schemas/export_timelines_schema';

import { getExportTimelineByObjectIds } from './utils/export_timelines';
import { ExportTimelinesQuery, ExportTimelinesRequestBody } from '../types';

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.

query: buildRouteValidation<ExportTimelinesQuery, ExportTimelinesQuery, unknown>(
exportTimelinesQuerySchema
),
body: buildRouteValidation<ExportTimelineRequestParams['body']>(exportTimelinesSchema),
body: buildRouteValidation<ExportTimelinesRequestBody, ExportTimelinesRequestBody, unknown>(
exportTimelinesRequestBodySchema
),
},
options: {
tags: ['access:siem'],
Expand All @@ -42,6 +40,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
Original file line number Diff line number Diff line change
Expand Up @@ -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)

'Invalid value undefined supplied to : { file: (ReadableRt & { hapi: { filename: string } }) }/file: (ReadableRt & { hapi: { filename: string } })/1: { hapi: { filename: string } }',
].join('\n')
);
});
});
Expand Down
Loading