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

[SIEM][Detection Engine] critical bug, fixes duplicate tags

Browse source 
## Summary

You can get duplicate tags that keep growing with each update which is not good at all and will cause major data issues for customers.

<img width="467" alt="Screen Shot 2020-01-27 at 10 22 23 PM" src="https://user-images.githubusercontent.com/1151048/73242235-9d01de80-4161-11ea-9c5d-56cafc865dd5.png">

Testing:

Create a rule, then update it

```sh
./post_rule.sh
./update_rule.sh
```

Grab the id of the rule and run the `get_saved_objects.sh` like so:

```sh
./get_saved_objects.sh alert ${id}
```

You shouldn't have duplicates in tags, it should look like this:

```sh
    "tags": [
      "__internal_rule_id:query-rule-id",
      "__internal_immutable:false"
    ],
```


### Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)~~

~~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)~~

~~- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~~

- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

~~- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~~

### For maintainers

~~- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~

- [x] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
This commit is contained in:
Frank Hassanabad 2020-01-28 08:49:49 -07:00 committed by GitHub
parent 106ddf918c
commit 24a6eb2cde
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 31 additions and 79 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

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

@ -21,9 +21,11 @@ import {
jest.mock('../../rules/get_prepackaged_rules', () => {
return {
getPrepackagedRules: () => {
getPrepackagedRules: (): PrepackagedRules[] => {
return [
{
tags: [],
immutable: true,
rule_id: 'rule-1',
output_index: '.siem-signals',
risk_score: 50,
@ -43,6 +45,7 @@ jest.mock('../../rules/get_prepackaged_rules', () => {
});
import { addPrepackedRulesRoute } from './add_prepackaged_rules_route';
import { PrepackagedRules } from '../../types';
describe('add_prepackaged_rules_route', () => {
let { server, alertsClient, actionsClient, elasticsearch } = createMockServer();
@ -57,7 +60,7 @@ describe('add_prepackaged_rules_route', () => {
describe('status codes with actionClient and alertClient', () => {
test('returns 200 when creating a with a valid actionClient and alertClient', async () => {
alertsClient.find.mockResolvedValue(getFindResult());
alertsClient.find.mockResolvedValue(getFindResultWithSingleHit());
alertsClient.get.mockResolvedValue(getResult());
actionsClient.create.mockResolvedValue(createActionResult());
alertsClient.create.mockResolvedValue(getResult());

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

@ -105,7 +105,7 @@ export const createCreateRulesRoute = (server: ServerFacade): Hapi.ServerRoute =
timelineTitle,
meta,
filters,
ruleId: ruleId != null ? ruleId : uuid.v4(),
ruleId: ruleId ?? uuid.v4(),
index,
interval,
maxSignals,

66
x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/add_tags.test.ts
View file

@ -8,46 +8,6 @@ import { addTags } from './add_tags';
import { INTERNAL_RULE_ID_KEY, INTERNAL_IMMUTABLE_KEY } from '../../../../common/constants';
describe('add_tags', () => {
test('if given a null everything this returns a new array for tags', () => {
const tags = addTags(null, null, null);
expect(tags).toEqual([]);
});
test('if given a undefined everything this returns a new array for tags', () => {
const tags = addTags(undefined, undefined, undefined);
expect(tags).toEqual([]);
});
test('it should add a rule id as an internal structure to a single tag', () => {
const tags = addTags(['tag 1'], 'rule-1', null);
expect(tags).toEqual(['tag 1', `${INTERNAL_RULE_ID_KEY}:rule-1`]);
});
test('it should add a rule id as an internal structure to a single tag if the input tags is null', () => {
const tags = addTags(null, 'rule-1', null);
expect(tags).toEqual([`${INTERNAL_RULE_ID_KEY}:rule-1`]);
});
test('it should add a rule id as an internal structure to two tags', () => {
const tags = addTags(['tag 1', 'tag 2'], 'rule-1', null);
expect(tags).toEqual(['tag 1', 'tag 2', `${INTERNAL_RULE_ID_KEY}:rule-1`]);
});
test('it should add a rule id as an internal structure with empty tags', () => {
const tags = addTags([], 'rule-1', null);
expect(tags).toEqual([`${INTERNAL_RULE_ID_KEY}:rule-1`]);
});
test('it should add a immutable true as an internal structure with empty tags', () => {
const tags = addTags([], null, true);
expect(tags).toEqual([`${INTERNAL_IMMUTABLE_KEY}:true`]);
});
test('it should add a immutable false as an internal structure with empty tags', () => {
const tags = addTags([], null, false);
expect(tags).toEqual([`${INTERNAL_IMMUTABLE_KEY}:false`]);
});
test('it should add a rule id as an internal structure with immutable true', () => {
const tags = addTags([], 'rule-1', true);
expect(tags).toEqual([`${INTERNAL_RULE_ID_KEY}:rule-1`, `${INTERNAL_IMMUTABLE_KEY}:true`]);
@ -58,18 +18,22 @@ describe('add_tags', () => {
expect(tags).toEqual([`${INTERNAL_RULE_ID_KEY}:rule-1`, `${INTERNAL_IMMUTABLE_KEY}:false`]);
});
test('it should add not add an internal structure if only a tag is given', () => {
const tags = addTags(['tag 1'], undefined, null);
expect(tags).toEqual(['tag 1']);
test('it should not allow duplicate tags to be created', () => {
const tags = addTags(['tag-1', 'tag-1'], 'rule-1', false);
expect(tags).toEqual([
'tag-1',
`${INTERNAL_RULE_ID_KEY}:rule-1`,
`${INTERNAL_IMMUTABLE_KEY}:false`,
]);
});
test('it should add not add an internal structure if everything is null', () => {
const tags = addTags(['tag 1'], null, null);
expect(tags).toEqual(['tag 1']);
});
test('it should add not add an internal structure if everything is undefined', () => {
const tags = addTags(['tag 1'], undefined, undefined);
expect(tags).toEqual(['tag 1']);
test('it should not allow duplicate internal tags to be created when called two times in a row', () => {
const tags1 = addTags(['tag-1'], 'rule-1', false);
const tags2 = addTags(tags1, 'rule-1', false);
expect(tags2).toEqual([
'tag-1',
`${INTERNAL_RULE_ID_KEY}:rule-1`,
`${INTERNAL_IMMUTABLE_KEY}:false`,
]);
});
});

23
x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/add_tags.ts
View file

@ -6,23 +6,12 @@
import { INTERNAL_RULE_ID_KEY, INTERNAL_IMMUTABLE_KEY } from '../../../../common/constants';
export const addTags = (
tags: string[] | null | undefined,
ruleId: string | null | undefined,
immutable: boolean | null | undefined
): string[] => {
const defaultedTags = tags != null ? tags : [];
if (ruleId != null && immutable != null) {
return [
...defaultedTags,
export const addTags = (tags: string[], ruleId: string, immutable: boolean): string[] => {
return Array.from(
new Set([
...tags,
`${INTERNAL_RULE_ID_KEY}:${ruleId}`,
`${INTERNAL_IMMUTABLE_KEY}:${immutable}`,
];
} else if (ruleId != null && immutable == null) {
return [...defaultedTags, `${INTERNAL_RULE_ID_KEY}:${ruleId}`];
} else if (ruleId == null && immutable != null) {
return [...defaultedTags, `${INTERNAL_IMMUTABLE_KEY}:${immutable}`];
} else {
return defaultedTags;
}
])
);
};

4
x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/create_rules.ts
View file

@ -5,7 +5,7 @@
*/
import { APP_ID, SIGNALS_ID } from '../../../../common/constants';
import { RuleParams } from './types';
import { CreateRuleParams } from './types';
import { addTags } from './add_tags';
export const createRules = ({
@ -37,7 +37,7 @@ export const createRules = ({
type,
references,
version,
}: RuleParams) => {
}: CreateRuleParams) => {
return alertsClient.create({
data: {
name,

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

@ -163,7 +163,7 @@ export type DeleteRuleParams = Clients & {
ruleId: string | undefined | null;
};
export type RuleParams = RuleAlertParams & Clients;
export type CreateRuleParams = Omit<RuleAlertParams, 'ruleId'> & { ruleId: string } & Clients;
export interface ReadRuleParams {
alertsClient: AlertsClient;

6
x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/update_rules.ts
View file

@ -198,11 +198,7 @@ export const updateRules = async ({
return alertsClient.update({
id: rule.id,
data: {
tags: addTags(
tags != null ? tags : rule.tags, // Add tags as an update if it exists, otherwise re-use the older tags
rule.params.ruleId,
immutable != null ? immutable : rule.params.immutable // Add new one if it exists, otherwise re-use old one
),
tags: addTags(tags ?? rule.tags, rule.params.ruleId, immutable ?? rule.params.immutable),
name: calculateName({ updatedName: name, originalName: rule.name }),
schedule: {
interval: calculateInterval(interval, rule.schedule.interval),