github-mirrors/kibana
1
0
Fork 0
mirror of https://github.com/elastic/kibana.git synced 2025-04-24 09:48:58 -04:00
Code Issues Projects Releases Packages Wiki Activity

[Security Solution] [Platform] Fix critical bug when migrating action within update route (#116512) (#116875)

Browse source 
* WIP - need to figure out how to delete old siem-detection action SO's after each test

* WIP - adds some fixes for the update rules utility that differ from patch rules utility

* fix type checks

* cleanup

* remove commented out code

* rename const to use capital snake case

* naming integration tests, adds expect for disabled rules that get migrated, adds expect for pre-migrated rules

Co-authored-by: Devin W. Hurley <devin.hurley@elastic.co>
This commit is contained in:
Kibana Machine 2021-10-30 00:58:30 -04:00 • committed by GitHub
parent 81608279ea
commit 003ad4cf0b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 317 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

5
x-pack/plugins/security_solution/common/constants.ts
View file

@ -225,6 +225,11 @@ export const INTERNAL_RULE_ID_KEY = `${INTERNAL_IDENTIFIER}_rule_id` as const;
export const INTERNAL_RULE_ALERT_ID_KEY = `${INTERNAL_IDENTIFIER}_rule_alert_id` as const;
export const INTERNAL_IMMUTABLE_KEY = `${INTERNAL_IDENTIFIER}_immutable` as const;
/**
* Internal actions route
*/
export const UPDATE_OR_CREATE_LEGACY_ACTIONS = '/internal/api/detection/legacy/notifications';
/**
* Detection engine routes
*/

4
x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts
View file

@ -78,7 +78,7 @@ export const updateRulesBulkRoute = (
id: payloadRule.id,
});
await legacyMigrate({
const migratedRule = await legacyMigrate({
rulesClient,
savedObjectsClient,
rule: existingRule,
@ -89,6 +89,8 @@ export const updateRulesBulkRoute = (
rulesClient,
ruleStatusClient,
defaultOutputIndex: siemClient.getSignalsIndex(),
existingRule,
migratedRule,
ruleUpdate: payloadRule,
isRuleRegistryEnabled,
});

4
x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/update_rules_route.ts
View file

@ -69,7 +69,7 @@ export const updateRulesRoute = (
id: request.body.id,
});
await legacyMigrate({
const migratedRule = await legacyMigrate({
rulesClient,
savedObjectsClient,
rule: existingRule,
@ -79,6 +79,8 @@ export const updateRulesRoute = (
isRuleRegistryEnabled,
rulesClient,
ruleStatusClient,
existingRule,
migratedRule,
ruleUpdate: request.body,
spaceId: context.securitySolution.getSpaceId(),
});

2
x-pack/plugins/security_solution/server/lib/detection_engine/rules/types.ts
View file

@ -274,6 +274,8 @@ export interface UpdateRulesOptions {
ruleStatusClient: IRuleExecutionLogClient;
rulesClient: RulesClient;
defaultOutputIndex: string;
existingRule: SanitizedAlert<RuleParams> | null | undefined;
migratedRule: SanitizedAlert<RuleParams> | null | undefined;
ruleUpdate: UpdateRulesSchema;
}

6
x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_rules.mock.ts
View file

@ -12,6 +12,8 @@ import {
getUpdateRulesSchemaMock,
} from '../../../../common/detection_engine/schemas/request/rule_schemas.mock';
import { ruleExecutionLogClientMock } from '../rule_execution_log/__mocks__/rule_execution_log_client';
import { getAlertMock } from '../routes/__mocks__/request_responses';
import { getQueryRuleParams } from '../schemas/rule_schemas.mock';
export const getUpdateRulesOptionsMock = (isRuleRegistryEnabled: boolean) => ({
spaceId: 'default',
@ -19,6 +21,8 @@ export const getUpdateRulesOptionsMock = (isRuleRegistryEnabled: boolean) => ({
ruleStatusClient: ruleExecutionLogClientMock.create(),
savedObjectsClient: savedObjectsClientMock.create(),
defaultOutputIndex: '.siem-signals-default',
existingRule: getAlertMock(isRuleRegistryEnabled, getQueryRuleParams()),
migratedRule: getAlertMock(isRuleRegistryEnabled, getQueryRuleParams()),
ruleUpdate: getUpdateRulesSchemaMock(),
isRuleRegistryEnabled,
});
@ -29,6 +33,8 @@ export const getUpdateMlRulesOptionsMock = (isRuleRegistryEnabled: boolean) => (
ruleStatusClient: ruleExecutionLogClientMock.create(),
savedObjectsClient: savedObjectsClientMock.create(),
defaultOutputIndex: '.siem-signals-default',
existingRule: getAlertMock(isRuleRegistryEnabled, getQueryRuleParams()),
migratedRule: getAlertMock(isRuleRegistryEnabled, getQueryRuleParams()),
ruleUpdate: getUpdateMachineLearningSchemaMock(),
isRuleRegistryEnabled,
});

40
x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_rules.ts
View file

@ -10,13 +10,19 @@ import { validate } from '@kbn/securitysolution-io-ts-utils';
import { DEFAULT_MAX_SIGNALS } from '../../../../common/constants';
import { transformRuleToAlertAction } from '../../../../common/detection_engine/transform_actions';
import { PartialAlert } from '../../../../../alerting/server';
import { readRules } from './read_rules';
import { UpdateRulesOptions } from './types';
import { addTags } from './add_tags';
import { typeSpecificSnakeToCamel } from '../schemas/rule_converters';
import { internalRuleUpdate, RuleParams } from '../schemas/rule_schemas';
import { enableRule } from './enable_rule';
import { maybeMute, transformToAlertThrottle, transformToNotifyWhen } from './utils';
import {
maybeMute,
transformToAlertThrottle,
transformToNotifyWhen,
updateActions,
updateThrottleNotifyWhen,
} from './utils';
class UpdateError extends Error {
public readonly statusCode: number;
@ -27,19 +33,14 @@ class UpdateError extends Error {
}
export const updateRules = async ({
isRuleRegistryEnabled,
spaceId,
rulesClient,
ruleStatusClient,
defaultOutputIndex,
existingRule,
migratedRule,
ruleUpdate,
}: UpdateRulesOptions): Promise<PartialAlert<RuleParams> | null> => {
const existingRule = await readRules({
isRuleRegistryEnabled,
rulesClient,
ruleId: ruleUpdate.rule_id,
id: ruleUpdate.id,
});
if (existingRule == null) {
return null;
}
@ -85,9 +86,24 @@ export const updateRules = async ({
...typeSpecificParams,
},
schedule: { interval: ruleUpdate.interval ?? '5m' },
actions: ruleUpdate.actions != null ? ruleUpdate.actions.map(transformRuleToAlertAction) : [],
throttle: transformToAlertThrottle(ruleUpdate.throttle),
notifyWhen: transformToNotifyWhen(ruleUpdate.throttle),
actions: updateActions(
transformRuleToAlertAction,
migratedRule?.actions,
existingRule.actions,
ruleUpdate?.actions
),
throttle: updateThrottleNotifyWhen(
transformToAlertThrottle,
migratedRule?.throttle,
existingRule.throttle,
ruleUpdate?.throttle
),
notifyWhen: updateThrottleNotifyWhen(
transformToNotifyWhen,
migratedRule?.notifyWhen,
existingRule.notifyWhen,
ruleUpdate?.throttle
),
};
const [validated, errors] = validate(newInternalRule, internalRuleUpdate);

50
x-pack/plugins/security_solution/server/lib/detection_engine/rules/utils.ts
View file

@ -5,7 +5,7 @@
* 2.0.
*/
import { pickBy, isEmpty } from 'lodash/fp';
import { pickBy, isEmpty, isEqual } from 'lodash/fp';
import type {
FromOrUndefined,
MachineLearningJobIdOrUndefined,
@ -64,10 +64,14 @@ import { RulesClient } from '../../../../../alerting/server';
// eslint-disable-next-line no-restricted-imports
import { LegacyRuleActions } from '../rule_actions/legacy_types';
import { FullResponseSchema } from '../../../../common/detection_engine/schemas/request';
import { transformAlertToRuleAction } from '../../../../common/detection_engine/transform_actions';
import {
transformAlertToRuleAction,
transformRuleToAlertAction,
} from '../../../../common/detection_engine/transform_actions';
// eslint-disable-next-line no-restricted-imports
import { legacyRuleActionsSavedObjectType } from '../rule_actions/legacy_saved_object_mappings';
import { LegacyMigrateParams } from './types';
import { RuleAlertAction } from '../../../../common/detection_engine/types';
export const calculateInterval = (
interval: string | undefined,
@ -331,6 +335,10 @@ export const legacyMigrate = async ({
}),
savedObjectsClient.find({
type: legacyRuleActionsSavedObjectType,
hasReference: {
type: 'alert',
id: rule.id,
},
}),
]);
@ -344,8 +352,10 @@ export const legacyMigrate = async ({
)
: null,
]);
const { id, ...restOfRule } = rule;
const migratedRule = {
...rule,
...restOfRule,
actions: siemNotification.data[0].actions,
throttle: siemNotification.data[0].schedule.interval,
notifyWhen: transformToNotifyWhen(siemNotification.data[0].throttle),
@ -354,7 +364,39 @@ export const legacyMigrate = async ({
id: rule.id,
data: migratedRule,
});
return migratedRule;
return { id: rule.id, ...migratedRule };
}
return rule;
};
export const updateThrottleNotifyWhen = (
transform: typeof transformToAlertThrottle | typeof transformToNotifyWhen,
migratedRuleThrottle: string | null | undefined,
existingRuleThrottle: string | null | undefined,
ruleUpdateThrottle: string | null | undefined
) => {
if (existingRuleThrottle == null && ruleUpdateThrottle == null && migratedRuleThrottle != null) {
return migratedRuleThrottle;
}
return transform(ruleUpdateThrottle);
};
export const updateActions = (
transform: typeof transformRuleToAlertAction,
migratedRuleActions: AlertAction[] | null | undefined,
existingRuleActions: AlertAction[] | null | undefined,
ruleUpdateActions: RuleAlertAction[] | null | undefined
) => {
// if the existing rule actions and the rule update actions are equivalent (aka no change)
// but the migrated actions and the ruleUpdateActions (or existing rule actions, associatively)
// are not equivalent, we know that the rules' actions were migrated and we need to apply
// that change to the update request so it is not overwritten by the rule update payload
if (
existingRuleActions?.length === 0 &&
ruleUpdateActions == null &&
!isEqual(existingRuleActions, migratedRuleActions)
) {
return migratedRuleActions;
}
return ruleUpdateActions != null ? ruleUpdateActions.map(transform) : [];
};

41
x-pack/test/detection_engine_api_integration/security_and_spaces/tests/patch_rules.ts
View file

@ -21,6 +21,7 @@ import {
getSimpleMlRuleOutput,
createRule,
getSimpleMlRule,
createLegacyRuleAction,
} from '../../utils';
// eslint-disable-next-line import/no-default-export
@ -187,6 +188,46 @@ export default ({ getService }: FtrProviderContext) => {
expect(bodyToCompare).to.eql(outputRule);
});
it('should return the rule with migrated actions after the enable patch', async () => {
const [connector, rule] = await Promise.all([
supertest
.post(`/api/actions/connector`)
.set('kbn-xsrf', 'foo')
.send({
name: 'My action',
connector_type_id: '.slack',
secrets: {
webhookUrl: 'http://localhost:1234',
},
}),
createRule(supertest, getSimpleRule('rule-1')),
]);
await createLegacyRuleAction(supertest, rule.id, connector.body.id);
// patch disable the rule
const patchResponse = await supertest
.patch(DETECTION_ENGINE_RULES_URL)
.set('kbn-xsrf', 'true')
.send({ id: rule.id, enabled: false })
.expect(200);
const outputRule = getSimpleRuleOutput();
outputRule.actions = [
{
action_type_id: '.slack',
group: 'default',
id: connector.body.id,
params: {
message:
'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
},
},
];
outputRule.throttle = '1m';
const bodyToCompare = removeServerGeneratedProperties(patchResponse.body);
expect(bodyToCompare).to.eql(outputRule);
});
it('should give a 404 if it is given a fake id', async () => {
const { body } = await supertest
.patch(DETECTION_ENGINE_RULES_URL)

50
x-pack/test/detection_engine_api_integration/security_and_spaces/tests/patch_rules_bulk.ts
View file

@ -19,6 +19,7 @@ import {
getSimpleRuleOutputWithoutRuleId,
removeServerGeneratedPropertiesIncludingRuleId,
createRule,
createLegacyRuleAction,
} from '../../utils';
// eslint-disable-next-line import/no-default-export
@ -126,6 +127,55 @@ export default ({ getService }: FtrProviderContext) => {
expect(bodyToCompare2).to.eql(outputRule2);
});
it('should bulk disable two rules and migrate their actions', async () => {
const [connector, rule1, rule2] = await Promise.all([
supertest
.post(`/api/actions/connector`)
.set('kbn-xsrf', 'foo')
.send({
name: 'My action',
connector_type_id: '.slack',
secrets: {
webhookUrl: 'http://localhost:1234',
},
}),
createRule(supertest, getSimpleRule('rule-1')),
createRule(supertest, getSimpleRule('rule-2')),
]);
await Promise.all([
createLegacyRuleAction(supertest, rule1.id, connector.body.id),
createLegacyRuleAction(supertest, rule2.id, connector.body.id),
]);
// patch a simple rule's name
const { body } = await supertest
.patch(`${DETECTION_ENGINE_RULES_URL}/_bulk_update`)
.set('kbn-xsrf', 'true')
.send([
{ id: rule1.id, enabled: false },
{ id: rule2.id, enabled: false },
])
.expect(200);
// @ts-expect-error
body.forEach((response) => {
const outputRule = getSimpleRuleOutput(response.rule_id, false);
outputRule.actions = [
{
action_type_id: '.slack',
group: 'default',
id: connector.body.id,
params: {
message:
'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
},
},
];
outputRule.throttle = '1m';
const bodyToCompare = removeServerGeneratedProperties(response);
expect(bodyToCompare).to.eql(outputRule);
});
});
it('should patch a single rule property of name using the auto-generated id', async () => {
const createdBody = await createRule(supertest, getSimpleRule('rule-1'));

50
x-pack/test/detection_engine_api_integration/security_and_spaces/tests/update_rules.ts
View file

@ -23,6 +23,7 @@ import {
getSimpleMlRuleUpdate,
createRule,
getSimpleRule,
createLegacyRuleAction,
} from '../../utils';
// eslint-disable-next-line import/no-default-export
@ -130,6 +131,55 @@ export default ({ getService }: FtrProviderContext) => {
expect(bodyToCompare).to.eql(outputRule);
});
it('should update a single rule property of name using an auto-generated rule_id and migrate the actions', async () => {
const rule = getSimpleRule('rule-1');
delete rule.rule_id;
const [connector, createRuleBody] = await Promise.all([
supertest
.post(`/api/actions/connector`)
.set('kbn-xsrf', 'foo')
.send({
name: 'My action',
connector_type_id: '.slack',
secrets: {
webhookUrl: 'http://localhost:1234',
},
}),
createRule(supertest, rule),
]);
await createLegacyRuleAction(supertest, createRuleBody.id, connector.body.id);
// update a simple rule's name
const updatedRule = getSimpleRuleUpdate('rule-1');
updatedRule.rule_id = createRuleBody.rule_id;
updatedRule.name = 'some other name';
delete updatedRule.id;
const { body } = await supertest
.put(DETECTION_ENGINE_RULES_URL)
.set('kbn-xsrf', 'true')
.send(updatedRule)
.expect(200);
const outputRule = getSimpleRuleOutputWithoutRuleId();
outputRule.name = 'some other name';
outputRule.version = 2;
outputRule.actions = [
{
action_type_id: '.slack',
group: 'default',
id: connector.body.id,
params: {
message:
'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
},
},
];
outputRule.throttle = '1m';
const bodyToCompare = removeServerGeneratedPropertiesIncludingRuleId(body);
expect(bodyToCompare).to.eql(outputRule);
});
it('should update a single rule property of name using the auto-generated id', async () => {
const createdBody = await createRule(supertest, getSimpleRule('rule-1'));

59
x-pack/test/detection_engine_api_integration/security_and_spaces/tests/update_rules_bulk.ts
View file

@ -20,6 +20,7 @@ import {
getSimpleRuleUpdate,
createRule,
getSimpleRule,
createLegacyRuleAction,
} from '../../utils';
// eslint-disable-next-line import/no-default-export
@ -94,6 +95,64 @@ export default ({ getService }: FtrProviderContext) => {
expect(bodyToCompare2).to.eql(outputRule2);
});
it('should update two rule properties of name using the two rules rule_id and migrate actions', async () => {
const [connector, rule1, rule2] = await Promise.all([
supertest
.post(`/api/actions/connector`)
.set('kbn-xsrf', 'foo')
.send({
name: 'My action',
connector_type_id: '.slack',
secrets: {
webhookUrl: 'http://localhost:1234',
},
}),
createRule(supertest, getSimpleRule('rule-1')),
createRule(supertest, getSimpleRule('rule-2')),
]);
await Promise.all([
createLegacyRuleAction(supertest, rule1.id, connector.body.id),
createLegacyRuleAction(supertest, rule2.id, connector.body.id),
]);
expect(rule1.actions).to.eql([]);
expect(rule2.actions).to.eql([]);
const updatedRule1 = getSimpleRuleUpdate('rule-1');
updatedRule1.name = 'some other name';
const updatedRule2 = getSimpleRuleUpdate('rule-2');
updatedRule2.name = 'some other name';
// update both rule names
const { body } = await supertest
.put(`${DETECTION_ENGINE_RULES_URL}/_bulk_update`)
.set('kbn-xsrf', 'true')
.send([updatedRule1, updatedRule2])
.expect(200);
// @ts-expect-error
body.forEach((response) => {
const outputRule = getSimpleRuleOutput(response.rule_id);
outputRule.name = 'some other name';
outputRule.version = 2;
outputRule.actions = [
{
action_type_id: '.slack',
group: 'default',
id: connector.body.id,
params: {
message:
'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
},
},
];
outputRule.throttle = '1m';
const bodyToCompare = removeServerGeneratedProperties(response);
expect(bodyToCompare).to.eql(outputRule);
});
});
it('should update a single rule property of name using an id', async () => {
const createRuleBody = await createRule(supertest, getSimpleRule('rule-1'));

24
x-pack/test/detection_engine_api_integration/utils.ts
View file

@ -48,6 +48,7 @@ import {
DETECTION_ENGINE_SIGNALS_MIGRATION_URL,
INTERNAL_IMMUTABLE_KEY,
INTERNAL_RULE_ID_KEY,
UPDATE_OR_CREATE_LEGACY_ACTIONS,
} from '../../plugins/security_solution/common/constants';
import { RACAlert } from '../../plugins/security_solution/server/lib/detection_engine/rule_types/types';
@ -516,6 +517,29 @@ export const createSignalsIndex = async (
}, 'createSignalsIndex');
};
export const createLegacyRuleAction = async (
supertest: SuperTest.SuperTest<SuperTest.Test>,
alertId: string,
connectorId: string
): Promise<unknown> =>
supertest
.post(`${UPDATE_OR_CREATE_LEGACY_ACTIONS}`)
.set('kbn-xsrf', 'true')
.query({ alert_id: alertId })
.send({
name: 'Legacy notification with one action',
interval: '1m',
actions: [
{
id: connectorId,
group: 'default',
params: {
message: 'Hourly\nRule {{context.rule.name}} generated {{state.signals_count}} alerts',
},
actionTypeId: '.slack',
},
],
});
/**
* Deletes the signals index for use inside of afterEach blocks of tests
* @param supertest The supertest client library