maxmustermann/kibana
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

[SIEM] [Detection Engine] Reject if duplicate rule_id in request payload (#57057) (#57526)

Browse source 
* 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 <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
This commit is contained in:
Devin W. Hurley 2020-02-13 08:48:24 -05:00 committed by GitHub
parent 25ae43acd7
commit 8ca0cef8a9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 156 additions and 76 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts
View file

@ -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<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
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts
View file

@ -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`,
})
),
];
},
};
};

22
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts
View file

@ -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);
});
});
});

9
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts
View file

@ -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<number>): string[] => {
const hasDuplicates = Object.values(lodashDict).some(i => i > 1);
if (hasDuplicates) {
return Object.keys(lodashDict).filter(key => lodashDict[key] > 1);
}
return [];
};

10
x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts
View file

@ -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',
},
]);
});