Skip to content

Commit

Permalink
[SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule mi…
Browse files Browse the repository at this point in the history 
…scounts (#56709)

## Summary

* Found multiple issues with how unstable finds can occur where iterating over multiple pages of find API with saved objects might return the same results per page and omit things as you try to figure out which pre-packaged rules are installed and which ones are not.
* This makes a distinct trade off of doing more JSON.parse() on the event loop by querying all the pre-packaged rules at one time. This however gives a stable and accurate count
* Fixed the tags aggregation to do the same thing.
* Fixes https://github.com/elastic/siem-team/issues/506

### 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)
  • Loading branch information
@FrankHassanabad
FrankHassanabad authored Feb 4, 2020
1 parent bd3491c commit 9e3aa2a
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 219 deletions.
191 changes: 39 additions & 152 deletions ...acy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,32 +35,7 @@ describe('get_existing_prepackaged_rules', () => {
expect(rules).toEqual([getResult()]);
});

test('should return 2 items over two pages, one per page', async () => {
const alertsClient = alertsClientMock.create();

const result1 = getResult();
result1.params.immutable = true;
result1.id = '4baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result2 = getResult();
result2.params.immutable = true;
result2.id = '5baa53f8-96da-44ee-ad58-41bccb7f9f3d';

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result1], perPage: 1, page: 1, total: 2 })
);
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result2], perPage: 1, page: 2, total: 2 })
);

const unsafeCast: AlertsClient = (alertsClient as unknown) as AlertsClient;
const rules = await getExistingPrepackagedRules({
alertsClient: unsafeCast,
});
expect(rules).toEqual([result1, result2]);
});

test('should return 3 items with over 3 pages one per page', async () => {
test('should return 3 items over 1 page with all on one page', async () => {
const alertsClient = alertsClientMock.create();

const result1 = getResult();
Expand All @@ -75,40 +50,17 @@ describe('get_existing_prepackaged_rules', () => {
result3.params.immutable = true;
result3.id = 'f3e1bf0b-b95f-43da-b1de-5d2f0af2287a';

// first result mock which is for returning the total
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result1], perPage: 1, page: 1, total: 3 })
);

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result2], perPage: 1, page: 2, total: 3 })
);

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result3], perPage: 1, page: 2, total: 3 })
getFindResultWithMultiHits({
data: [result1],
perPage: 1,
page: 1,
total: 3,
})
);

const unsafeCast: AlertsClient = (alertsClient as unknown) as AlertsClient;
const rules = await getExistingPrepackagedRules({
alertsClient: unsafeCast,
});
expect(rules).toEqual([result1, result2, result3]);
});

test('should return 3 items over 1 pages with all on one page', async () => {
const alertsClient = alertsClientMock.create();

const result1 = getResult();
result1.params.immutable = true;
result1.id = '4baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result2 = getResult();
result2.params.immutable = true;
result2.id = '5baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result3 = getResult();
result3.params.immutable = true;
result3.id = 'f3e1bf0b-b95f-43da-b1de-5d2f0af2287a';

// second mock which will return all the data on a single page
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({
data: [result1, result2, result3],
Expand Down Expand Up @@ -137,7 +89,7 @@ describe('get_existing_prepackaged_rules', () => {
expect(rules).toEqual([getResult()]);
});

test('should return 2 items over two pages, one per page', async () => {
test('should return 2 items over 1 page', async () => {
const alertsClient = alertsClientMock.create();

const result1 = getResult();
Expand All @@ -146,11 +98,19 @@ describe('get_existing_prepackaged_rules', () => {
const result2 = getResult();
result2.id = '5baa53f8-96da-44ee-ad58-41bccb7f9f3d';

// first result mock which is for returning the total
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result1], perPage: 1, page: 1, total: 2 })
getFindResultWithMultiHits({
data: [result1],
perPage: 1,
page: 1,
total: 2,
})
);

// second mock which will return all the data on a single page
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result2], perPage: 1, page: 2, total: 2 })
getFindResultWithMultiHits({ data: [result1, result2], perPage: 2, page: 1, total: 2 })
);

const unsafeCast: AlertsClient = (alertsClient as unknown) as AlertsClient;
Expand All @@ -160,7 +120,7 @@ describe('get_existing_prepackaged_rules', () => {
expect(rules).toEqual([result1, result2]);
});

test('should return 3 items with over 3 pages one per page', async () => {
test('should return 3 items over 1 page with all on one page', async () => {
const alertsClient = alertsClientMock.create();

const result1 = getResult();
Expand All @@ -172,37 +132,17 @@ describe('get_existing_prepackaged_rules', () => {
const result3 = getResult();
result3.id = 'f3e1bf0b-b95f-43da-b1de-5d2f0af2287a';

// first result mock which is for returning the total
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result1], perPage: 1, page: 1, total: 3 })
);

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result2], perPage: 1, page: 2, total: 3 })
);

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result3], perPage: 1, page: 2, total: 3 })
getFindResultWithMultiHits({
data: [result1],
perPage: 3,
page: 1,
total: 3,
})
);

const unsafeCast: AlertsClient = (alertsClient as unknown) as AlertsClient;
const rules = await getNonPackagedRules({
alertsClient: unsafeCast,
});
expect(rules).toEqual([result1, result2, result3]);
});

test('should return 3 items over 1 pages with all on one page', async () => {
const alertsClient = alertsClientMock.create();

const result1 = getResult();
result1.id = '4baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result2 = getResult();
result2.id = '5baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result3 = getResult();
result3.id = 'f3e1bf0b-b95f-43da-b1de-5d2f0af2287a';

// second mock which will return all the data on a single page
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({
data: [result1, result2, result3],
Expand Down Expand Up @@ -241,80 +181,27 @@ describe('get_existing_prepackaged_rules', () => {
const result2 = getResult();
result2.id = '5baa53f8-96da-44ee-ad58-41bccb7f9f3d';

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result1], perPage: 1, page: 1, total: 2 })
);
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result2], perPage: 1, page: 2, total: 2 })
);

const unsafeCast: AlertsClient = (alertsClient as unknown) as AlertsClient;
const rules = await getRules({
alertsClient: unsafeCast,
filter: '',
});
expect(rules).toEqual([result1, result2]);
});

test('should return 3 items with over 3 pages one per page', async () => {
const alertsClient = alertsClientMock.create();

const result1 = getResult();
result1.id = '4baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result2 = getResult();
result2.id = '5baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result3 = getResult();
result3.id = 'f3e1bf0b-b95f-43da-b1de-5d2f0af2287a';

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result1], perPage: 1, page: 1, total: 3 })
);

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result2], perPage: 1, page: 2, total: 3 })
);

alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result3], perPage: 1, page: 2, total: 3 })
);

const unsafeCast: AlertsClient = (alertsClient as unknown) as AlertsClient;
const rules = await getRules({
alertsClient: unsafeCast,
filter: '',
});
expect(rules).toEqual([result1, result2, result3]);
});

test('should return 3 items over 1 pages with all on one page', async () => {
const alertsClient = alertsClientMock.create();

const result1 = getResult();
result1.id = '4baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result2 = getResult();
result2.id = '5baa53f8-96da-44ee-ad58-41bccb7f9f3d';

const result3 = getResult();
result3.id = 'f3e1bf0b-b95f-43da-b1de-5d2f0af2287a';

// first result mock which is for returning the total
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({
data: [result1, result2, result3],
perPage: 3,
data: [result1],
perPage: 1,
page: 1,
total: 3,
total: 2,
})
);

// second mock which will return all the data on a single page
alertsClient.find.mockResolvedValueOnce(
getFindResultWithMultiHits({ data: [result1, result2], perPage: 2, page: 1, total: 2 })
);

const unsafeCast: AlertsClient = (alertsClient as unknown) as AlertsClient;
const rules = await getRules({
alertsClient: unsafeCast,
filter: '',
});
expect(rules).toEqual([result1, result2, result3]);
expect(rules).toEqual([result1, result2]);
});
});

Expand Down
55 changes: 13 additions & 42 deletions ...k/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { AlertsClient } from '../../../../../alerting';
import { RuleAlertType, isAlertTypes } from './types';
import { findRules } from './find_rules';

export const DEFAULT_PER_PAGE = 100;
export const FILTER_NON_PREPACKED_RULES = `alert.attributes.tags: "${INTERNAL_IMMUTABLE_KEY}:false"`;
export const FILTER_PREPACKED_RULES = `alert.attributes.tags: "${INTERNAL_IMMUTABLE_KEY}:true"`;

Expand All @@ -33,84 +32,56 @@ export const getRulesCount = async ({
filter,
perPage: 1,
page: 1,
sortField: 'createdAt',
sortOrder: 'desc',
});
return firstRule.total;
};

export const getRules = async ({
alertsClient,
perPage = DEFAULT_PER_PAGE,
filter,
}: {
alertsClient: AlertsClient;
perPage?: number;
filter: string;
}): Promise<RuleAlertType[]> => {
const firstPrepackedRules = await findRules({
const count = await getRulesCount({ alertsClient, filter });
const rules = await findRules({
alertsClient,
filter,
perPage,
perPage: count,
page: 1,
sortField: 'createdAt',
sortOrder: 'desc',
});
const totalPages = Math.ceil(firstPrepackedRules.total / firstPrepackedRules.perPage);
if (totalPages <= 1) {
if (isAlertTypes(firstPrepackedRules.data)) {
return firstPrepackedRules.data;
} else {
// If this was ever true, you have a really messed up system.
// This is keep typescript happy since we have an unknown with data
return [];
}
} else {
const returnPrepackagedRules = await Array(totalPages - 1)
.fill({})
.map((_, page) => {
// page index starts at 2 as we already got the first page and we have more pages to go
return findRules({
alertsClient,
filter,
perPage,
page: page + 2,
});
})
.reduce<Promise<object[]>>(async (accum, nextPage) => {
return [...(await accum), ...(await nextPage).data];
}, Promise.resolve(firstPrepackedRules.data));

if (isAlertTypes(returnPrepackagedRules)) {
return returnPrepackagedRules;
} else {
// If this was ever true, you have a really messed up system.
// This is keep typescript happy since we have an unknown with data
return [];
}
if (isAlertTypes(rules.data)) {
return rules.data;
} else {
// If this was ever true, you have a really messed up system.
// This is keep typescript happy since we have an unknown with data
return [];
}
};

export const getNonPackagedRules = async ({
alertsClient,
perPage = DEFAULT_PER_PAGE,
}: {
alertsClient: AlertsClient;
perPage?: number;
}): Promise<RuleAlertType[]> => {
return getRules({
alertsClient,
perPage,
filter: FILTER_NON_PREPACKED_RULES,
});
};

export const getExistingPrepackagedRules = async ({
alertsClient,
perPage = DEFAULT_PER_PAGE,
}: {
alertsClient: AlertsClient;
perPage?: number;
}): Promise<RuleAlertType[]> => {
return getRules({
alertsClient,
perPage,
filter: FILTER_PREPACKED_RULES,
});
};
Loading

0 comments on commit 9e3aa2a

Please sign in to comment.