From 9e3aa2a008199ff257c386d3a300c943fbde7ea3 Mon Sep 17 00:00:00 2001 From: Frank Hassanabad Date: Mon, 3 Feb 2020 18:12:50 -0700 Subject: [PATCH] [SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule miscounts (#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) --- .../get_existing_prepackaged_rules.test.ts | 191 ++++-------------- .../rules/get_existing_prepackaged_rules.ts | 55 ++--- .../lib/detection_engine/tags/read_tags.ts | 46 ++--- 3 files changed, 73 insertions(+), 219 deletions(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.test.ts index dc308263baab61..8d00ddb18be6b9 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.test.ts @@ -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(); @@ -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], @@ -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(); @@ -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; @@ -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(); @@ -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], @@ -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]); }); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.ts index b7ab6a97634a80..a48957da7aa942 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.ts @@ -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"`; @@ -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 => { - 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>(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 => { return getRules({ alertsClient, - perPage, filter: FILTER_NON_PREPACKED_RULES, }); }; export const getExistingPrepackagedRules = async ({ alertsClient, - perPage = DEFAULT_PER_PAGE, }: { alertsClient: AlertsClient; - perPage?: number; }): Promise => { return getRules({ alertsClient, - perPage, filter: FILTER_PREPACKED_RULES, }); }; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/tags/read_tags.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/tags/read_tags.ts index 0f973d816917fc..02456732df3b4e 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/tags/read_tags.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/tags/read_tags.ts @@ -9,8 +9,6 @@ import { INTERNAL_IDENTIFIER } from '../../../../common/constants'; import { AlertsClient } from '../../../../../alerting'; import { findRules } from '../rules/find_rules'; -const DEFAULT_PER_PAGE: number = 1000; - export interface TagType { id: string; tags: string[]; @@ -42,39 +40,37 @@ export const convertTagsToSet = (tagObjects: object[]): Set => { // Ref: https://www.elastic.co/guide/en/kibana/master/saved-objects-api.html export const readTags = async ({ alertsClient, - perPage = DEFAULT_PER_PAGE, }: { alertsClient: AlertsClient; - perPage?: number; }): Promise => { - const tags = await readRawTags({ alertsClient, perPage }); + const tags = await readRawTags({ alertsClient }); return tags.filter(tag => !tag.startsWith(INTERNAL_IDENTIFIER)); }; export const readRawTags = async ({ alertsClient, - perPage = DEFAULT_PER_PAGE, }: { alertsClient: AlertsClient; perPage?: number; }): Promise => { - const firstTags = await findRules({ alertsClient, fields: ['tags'], perPage, page: 1 }); - const firstSet = convertTagsToSet(firstTags.data); - const totalPages = Math.ceil(firstTags.total / firstTags.perPage); - if (totalPages <= 1) { - return Array.from(firstSet); - } else { - const returnTags = 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, fields: ['tags'], perPage, page: page + 2 }); - }) - .reduce>>(async (accum, nextTagPage) => { - const tagArray = convertToTags((await nextTagPage).data); - return new Set([...(await accum), ...tagArray]); - }, Promise.resolve(firstSet)); - - return Array.from(returnTags); - } + // Get just one record so we can get the total count + const firstTags = await findRules({ + alertsClient, + fields: ['tags'], + perPage: 1, + page: 1, + sortField: 'createdAt', + sortOrder: 'desc', + }); + // Get all the rules to aggregate over all the tags of the rules + const rules = await findRules({ + alertsClient, + fields: ['tags'], + perPage: firstTags.total, + sortField: 'createdAt', + sortOrder: 'desc', + page: 1, + }); + const tagSet = convertTagsToSet(rules.data); + return Array.from(tagSet); };