From 2f946d46149616bf3eb06ba3fc67cab7a945f075 Mon Sep 17 00:00:00 2001 From: "Devin W. Hurley" Date: Wed, 12 Feb 2020 21:13:43 -0500 Subject: [PATCH] [SIEM] [Detection Engine] Reject if duplicate rule_id in request payload (#57057) * prevents creation of rules when duplicate rule_id is present * adds unit test to reflect change * genericizes duplicate discovery functions, allows creation of non-duplicated rules even when duplicates are discovered, keeps same return type signature, updates relevant test for duplicates in request payload * utilizes countBy and removes reduce in favor of a filter on getDuplicates function * fix type * removes skip from e2e test for duplicates on bulk create, updates expected response in e2e test, fixes bug where duplicated error messages appeared for each instance of a duplicated rule_id (found this one through the e2e tests)! Adds unit test to catch this case. * getDuplicate returns empty array instead of null, removes unnecessary return logic * removes null coalescing from includes in filter Co-authored-by: Elastic Machine --- .../rules/create_rules_bulk_route.test.ts | 32 ++++ .../routes/rules/create_rules_bulk_route.ts | 159 ++++++++++-------- .../routes/rules/utils.test.ts | 22 +++ .../detection_engine/routes/rules/utils.ts | 9 + .../tests/create_rules_bulk.ts | 10 +- 5 files changed, 156 insertions(+), 76 deletions(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts index 5cf6d8955d8b2a..f1169442484c6b 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts @@ -20,6 +20,8 @@ import { } from '../__mocks__/request_responses'; import { DETECTION_ENGINE_RULES_URL } from '../../../../../common/constants'; import { createRulesBulkRoute } from './create_rules_bulk_route'; +import { BulkError } from '../utils'; +import { OutputRuleAlertRest } from '../../types'; describe('create_rules_bulk', () => { let { server, alertsClient, actionsClient, elasticsearch } = createMockServer(); @@ -124,4 +126,34 @@ describe('create_rules_bulk', () => { expect(statusCode).toBe(400); }); }); + + test('returns 409 if duplicate rule_ids found in request payload', async () => { + alertsClient.find.mockResolvedValue(getFindResult()); + alertsClient.get.mockResolvedValue(getResult()); + actionsClient.create.mockResolvedValue(createActionResult()); + alertsClient.create.mockResolvedValue(getResult()); + const request: ServerInjectOptions = { + method: 'POST', + url: `${DETECTION_ENGINE_RULES_URL}/_bulk_create`, + payload: [typicalPayload(), typicalPayload()], + }; + const { payload } = await server.inject(request); + const output: Array> = JSON.parse(payload); + expect(output.some(item => item.error?.status_code === 409)).toBeTruthy(); + }); + + test('returns one error object in response when duplicate rule_ids found in request payload', async () => { + alertsClient.find.mockResolvedValue(getFindResult()); + alertsClient.get.mockResolvedValue(getResult()); + actionsClient.create.mockResolvedValue(createActionResult()); + alertsClient.create.mockResolvedValue(getResult()); + const request: ServerInjectOptions = { + method: 'POST', + url: `${DETECTION_ENGINE_RULES_URL}/_bulk_create`, + payload: [typicalPayload(), typicalPayload()], + }; + const { payload } = await server.inject(request); + const output: Array> = JSON.parse(payload); + expect(output.length).toBe(1); + }); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts index 0ffa61e2e2bedf..e7145d2a6f0559 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts @@ -5,14 +5,14 @@ */ import Hapi from 'hapi'; -import { isFunction } from 'lodash/fp'; +import { isFunction, countBy } from 'lodash/fp'; import uuid from 'uuid'; import { DETECTION_ENGINE_RULES_URL } from '../../../../../common/constants'; import { createRules } from '../../rules/create_rules'; import { BulkRulesRequest } from '../../rules/types'; import { ServerFacade } from '../../../../types'; import { readRules } from '../../rules/read_rules'; -import { transformOrBulkError } from './utils'; +import { transformOrBulkError, getDuplicates } from './utils'; import { getIndexExists } from '../../index/get_index_exists'; import { callWithRequestFactory, @@ -48,94 +48,109 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou return headers.response().code(404); } + const ruleDefinitions = request.payload; + const mappedDuplicates = countBy('rule_id', ruleDefinitions); + const dupes = getDuplicates(mappedDuplicates); + const rules = await Promise.all( - request.payload.map(async payloadRule => { - const { - description, - enabled, - false_positives: falsePositives, - from, - query, - language, - output_index: outputIndex, - saved_id: savedId, - meta, - filters, - rule_id: ruleId, - index, - interval, - max_signals: maxSignals, - risk_score: riskScore, - name, - severity, - tags, - threat, - to, - type, - references, - timeline_id: timelineId, - timeline_title: timelineTitle, - version, - } = payloadRule; - const ruleIdOrUuid = ruleId ?? uuid.v4(); - try { - const finalIndex = outputIndex != null ? outputIndex : getIndex(request, server); - const callWithRequest = callWithRequestFactory(request, server); - const indexExists = await getIndexExists(callWithRequest, finalIndex); - if (!indexExists) { - return createBulkErrorObject({ - ruleId: ruleIdOrUuid, - statusCode: 400, - message: `To create a rule, the index must exist first. Index ${finalIndex} does not exist`, - }); - } - if (ruleId != null) { - const rule = await readRules({ alertsClient, ruleId }); - if (rule != null) { - return createBulkErrorObject({ - ruleId, - statusCode: 409, - message: `rule_id: "${ruleId}" already exists`, - }); - } - } - const createdRule = await createRules({ - alertsClient, - actionsClient, + ruleDefinitions + .filter(rule => rule.rule_id == null || !dupes.includes(rule.rule_id)) + .map(async payloadRule => { + const { description, enabled, - falsePositives, + false_positives: falsePositives, from, - immutable: false, query, language, - outputIndex: finalIndex, - savedId, - timelineId, - timelineTitle, + output_index: outputIndex, + saved_id: savedId, meta, filters, - ruleId: ruleIdOrUuid, + rule_id: ruleId, index, interval, - maxSignals, - riskScore, + max_signals: maxSignals, + risk_score: riskScore, name, severity, tags, + threat, to, type, - threat, references, + timeline_id: timelineId, + timeline_title: timelineTitle, version, - }); - return transformOrBulkError(ruleIdOrUuid, createdRule); - } catch (err) { - return transformBulkError(ruleIdOrUuid, err); - } - }) + } = payloadRule; + const ruleIdOrUuid = ruleId ?? uuid.v4(); + try { + const finalIndex = outputIndex != null ? outputIndex : getIndex(request, server); + const callWithRequest = callWithRequestFactory(request, server); + const indexExists = await getIndexExists(callWithRequest, finalIndex); + if (!indexExists) { + return createBulkErrorObject({ + ruleId: ruleIdOrUuid, + statusCode: 400, + message: `To create a rule, the index must exist first. Index ${finalIndex} does not exist`, + }); + } + if (ruleId != null) { + const rule = await readRules({ alertsClient, ruleId }); + if (rule != null) { + return createBulkErrorObject({ + ruleId, + statusCode: 409, + message: `rule_id: "${ruleId}" already exists`, + }); + } + } + const createdRule = await createRules({ + alertsClient, + actionsClient, + description, + enabled, + falsePositives, + from, + immutable: false, + query, + language, + outputIndex: finalIndex, + savedId, + timelineId, + timelineTitle, + meta, + filters, + ruleId: ruleIdOrUuid, + index, + interval, + maxSignals, + riskScore, + name, + severity, + tags, + to, + type, + threat, + references, + version, + }); + return transformOrBulkError(ruleIdOrUuid, createdRule); + } catch (err) { + return transformBulkError(ruleIdOrUuid, err); + } + }) ); - return rules; + return [ + ...rules, + ...dupes.map(ruleId => + createBulkErrorObject({ + ruleId, + statusCode: 409, + message: `rule_id: "${ruleId}" already exists`, + }) + ), + ]; }, }; }; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts index 7e7d67333e78d9..fb3262c476b402 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts @@ -15,6 +15,7 @@ import { transformRulesToNdjson, transformAlertsToRules, transformOrImportError, + getDuplicates, } from './utils'; import { getResult } from '../__mocks__/request_responses'; import { INTERNAL_IDENTIFIER } from '../../../../../common/constants'; @@ -1202,4 +1203,25 @@ describe('utils', () => { expect(output).toEqual(expected); }); }); + + describe('getDuplicates', () => { + test("returns array of ruleIds showing the duplicate keys of 'value2' and 'value3'", () => { + const output = getDuplicates({ + value1: 1, + value2: 2, + value3: 2, + }); + const expected = ['value2', 'value3']; + expect(output).toEqual(expected); + }); + test('returns null when given a map of no duplicates', () => { + const output = getDuplicates({ + value1: 1, + value2: 1, + value3: 1, + }); + const expected: string[] = []; + expect(output).toEqual(expected); + }); + }); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts index abb94c10209dca..df9e3021e400f2 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts @@ -5,6 +5,7 @@ */ import { pickBy } from 'lodash/fp'; +import { Dictionary } from 'lodash'; import { SavedObject } from 'kibana/server'; import { INTERNAL_IDENTIFIER } from '../../../../../common/constants'; import { @@ -215,3 +216,11 @@ export const transformOrImportError = ( }); } }; + +export const getDuplicates = (lodashDict: Dictionary): string[] => { + const hasDuplicates = Object.values(lodashDict).some(i => i > 1); + if (hasDuplicates) { + return Object.keys(lodashDict).filter(key => lodashDict[key] > 1); + } + return []; +}; diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts index dfa297c85dfb88..be008a34343c4f 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts @@ -80,7 +80,7 @@ export default ({ getService }: FtrProviderContext): void => { }); // TODO: This is a valid issue and will be fixed in an upcoming PR and then enabled once that PR is merged - it.skip('should return a 200 ok but have a 409 conflict if we attempt to create the same rule_id twice', async () => { + it('should return a 200 ok but have a 409 conflict if we attempt to create the same rule_id twice', async () => { const { body } = await supertest .post(`${DETECTION_ENGINE_RULES_URL}/_bulk_create`) .set('kbn-xsrf', 'true') @@ -89,9 +89,11 @@ export default ({ getService }: FtrProviderContext): void => { expect(body).to.eql([ { - error: 'Conflict', - message: 'rule_id: "rule-1" already exists', - statusCode: 409, + error: { + message: 'rule_id: "rule-1" already exists', + status_code: 409, + }, + rule_id: 'rule-1', }, ]); });