Skip to content

Commit

Permalink
[SIEM] [Detection Engine] Reject if duplicate rule_id in request payl…
Browse files Browse the repository at this point in the history 
…oad (#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 <[email protected]>
  • Loading branch information
@dhurley14 @elasticmachine
dhurley14 and elasticmachine authored Feb 13, 2020
1 parent bc8a41a commit ee6386b
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 76 deletions.
32 changes: 32 additions & 0 deletions ...acy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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<BulkError | Partial<OutputRuleAlertRest>> = 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<BulkError | Partial<OutputRuleAlertRest>> = JSON.parse(payload);
expect(output.length).toBe(1);
});
});
159 changes: 87 additions & 72 deletions ...k/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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`,
})
),
];
},
};
};
Expand Down
22 changes: 22 additions & 0 deletions x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
transformRulesToNdjson,
transformAlertsToRules,
transformOrImportError,
getDuplicates,
} from './utils';
import { getResult } from '../__mocks__/request_responses';
import { INTERNAL_IDENTIFIER } from '../../../../../common/constants';
Expand Down Expand Up @@ -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);
});
});
});
9 changes: 9 additions & 0 deletions x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -215,3 +216,11 @@ export const transformOrImportError = (
});
}
};

export const getDuplicates = (lodashDict: Dictionary<number>): string[] => {
const hasDuplicates = Object.values(lodashDict).some(i => i > 1);
if (hasDuplicates) {
return Object.keys(lodashDict).filter(key => lodashDict[key] > 1);
}
return [];
};
10 changes: 6 additions & 4 deletions x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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',
},
]);
});
Expand Down

0 comments on commit ee6386b

Please sign in to comment.