[Security Solution][Detection Engine] Fix chunking logic so importing many rules with exception lists works (#190447)

## Summary

@Mikaayenson reported that rule import fails when importing many rules
with exceptions. We are only fetching the first 100 exception lists for
reference verification after importing exceptions from the rule import,
so if there are more than 100 exception lists then they will be imported
but the rules will appear to have a missing exception list reference.

Since we're already processing the rules in chunks, this PR changes the
logic so we fetch exception lists for each chunk of 50 rules
independently. There's still a possibility that 50 rules could have more
than 1000 exception lists referenced and we might run into the same
problem again. To fix the problem permanently we need to update the
exception lists client to support paging through exception lists so we
can reliably fetch all exception lists referenced by a chunk of rules.
However that's a more involved fix that'll have to wait for a follow up
PR.

### Testing
The rule export file below contains 210 rules, each with an exception
list. It triggers the failure when imported on `main` but works with
this PR. Rename the file to `.ndjson` from `.json` - github doesn't
allow uploading `.ndjson` files so I renamed it before uploading here.
[rules_export
(1).json](https://github.com/user-attachments/files/16603839/rules_export.1.json)
This commit is contained in:
Marshall Main 2024-08-15 13:15:05 -07:00 committed by GitHub
parent ff5be94c13
commit 10f4c199ee
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 102 additions and 29 deletions

View file

@ -64,7 +64,7 @@ interface ImportExceptionListAndItemsAsStreamOptions {
}
export type ExceptionsImport = Array<ImportExceptionListItemSchema | ImportExceptionsListSchema>;
export const CHUNK_PARSED_OBJECT_SIZE = 100;
export const CHUNK_PARSED_OBJECT_SIZE = 1000;
/**
* Import exception lists parent containers and items as stream. The shape of the list and items

View file

@ -67,7 +67,7 @@ describe('find_all_exception_list_item_types', () => {
filter:
'((exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "detection_list_id") AND (exception-list-agnostic.attributes.item_id:(1)))',
page: undefined,
perPage: 100,
perPage: 1000,
sortField: undefined,
sortOrder: undefined,
type: ['exception-list-agnostic'],
@ -85,7 +85,7 @@ describe('find_all_exception_list_item_types', () => {
filter:
'((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "detection_list_id") AND (exception-list.attributes.item_id:(1)))',
page: undefined,
perPage: 100,
perPage: 1000,
sortField: undefined,
sortOrder: undefined,
type: ['exception-list'],
@ -103,7 +103,7 @@ describe('find_all_exception_list_item_types', () => {
filter:
'((exception-list.attributes.list_type: item AND exception-list.attributes.list_id: "detection_list_id") AND (exception-list.attributes.item_id:(2))) OR ((exception-list-agnostic.attributes.list_type: item AND exception-list-agnostic.attributes.list_id: "detection_list_id") AND (exception-list-agnostic.attributes.item_id:(1)))',
page: undefined,
perPage: 100,
perPage: 1000,
sortField: undefined,
sortOrder: undefined,
type: ['exception-list', 'exception-list-agnostic'],

View file

@ -62,7 +62,7 @@ describe('find_all_exception_list_item_types', () => {
filter: 'exception-list-agnostic.attributes.list_id:(1)',
namespaceType: ['agnostic'],
page: undefined,
perPage: 100,
perPage: 1000,
savedObjectsClient,
sortField: undefined,
sortOrder: undefined,
@ -76,7 +76,7 @@ describe('find_all_exception_list_item_types', () => {
filter: 'exception-list.attributes.list_id:(1)',
namespaceType: ['single'],
page: undefined,
perPage: 100,
perPage: 1000,
savedObjectsClient,
sortField: undefined,
sortOrder: undefined,
@ -95,7 +95,7 @@ describe('find_all_exception_list_item_types', () => {
'exception-list-agnostic.attributes.list_id:(1) OR exception-list.attributes.list_id:(2)',
namespaceType: ['single', 'agnostic'],
page: undefined,
perPage: 100,
perPage: 1000,
savedObjectsClient,
sortField: undefined,
sortOrder: undefined,

View file

@ -23,7 +23,6 @@ import type { BulkError, ImportRuleResponse } from '../../../../routes/utils';
import { buildSiemResponse, isBulkError, isImportRegular } from '../../../../routes/utils';
import { importRuleActionConnectors } from '../../../logic/import/action_connectors/import_rule_action_connectors';
import { createRulesAndExceptionsStreamFromNdJson } from '../../../logic/import/create_rules_stream_from_ndjson';
import { getReferencedExceptionLists } from '../../../logic/import/gather_referenced_exceptions';
import type { RuleExceptionsPromiseFromStreams } from '../../../logic/import/import_rules_utils';
import { importRules as importRulesHelper } from '../../../logic/import/import_rules_utils';
import { importRuleExceptions } from '../../../logic/import/import_rule_exceptions';
@ -143,12 +142,6 @@ export const importRulesRoute = (router: SecuritySolutionPluginRouter, config: C
? []
: rulesWithMigratedActions || migratedParsedObjectsWithoutDuplicateErrors;
// gather all exception lists that the imported rules reference
const foundReferencedExceptionLists = await getReferencedExceptionLists({
rules: parsedRules,
savedObjectsClient,
});
const chunkParseObjects = chunk(CHUNK_PARSED_OBJECT_SIZE, parsedRules);
const importRuleResponse: ImportRuleResponse[] = await importRulesHelper({
@ -156,8 +149,8 @@ export const importRulesRoute = (router: SecuritySolutionPluginRouter, config: C
rulesResponseAcc: [...actionConnectorErrors, ...duplicateIdErrors],
overwriteRules: request.query.overwrite,
detectionRulesClient,
existingLists: foundReferencedExceptionLists,
allowMissingConnectorSecrets: !!actionConnectors.length,
savedObjectsClient,
});
const errorsResp = importRuleResponse.filter((resp) => isBulkError(resp)) as BulkError[];

View file

@ -5,6 +5,8 @@
* 2.0.
*/
import type { SavedObjectsClientContract } from '@kbn/core/server';
import { savedObjectsClientMock } from '@kbn/core/server/mocks';
import { getImportRulesSchemaMock } from '../../../../../../common/api/detection_engine/rule_management/mocks';
import { getRulesSchemaMock } from '../../../../../../common/api/detection_engine/model/rule_schema/rule_response_schema.mock';
import { requestContextMock } from '../../../routes/__mocks__';
@ -16,8 +18,12 @@ describe('importRules', () => {
const { clients, context } = requestContextMock.createTools();
const ruleToImport = getImportRulesSchemaMock();
let savedObjectsClient: jest.Mocked<SavedObjectsClientContract>;
beforeEach(() => {
jest.clearAllMocks();
savedObjectsClient = savedObjectsClientMock.create();
});
it('returns an empty rules response if no rules to import', async () => {
@ -26,7 +32,7 @@ describe('importRules', () => {
rulesResponseAcc: [],
overwriteRules: false,
detectionRulesClient: context.securitySolution.getDetectionRulesClient(),
existingLists: {},
savedObjectsClient,
});
expect(result).toEqual([]);
@ -38,7 +44,7 @@ describe('importRules', () => {
rulesResponseAcc: [],
overwriteRules: false,
detectionRulesClient: context.securitySolution.getDetectionRulesClient(),
existingLists: {},
savedObjectsClient,
});
expect(result).toEqual([
@ -67,7 +73,7 @@ describe('importRules', () => {
rulesResponseAcc: [],
overwriteRules: false,
detectionRulesClient: context.securitySolution.getDetectionRulesClient(),
existingLists: {},
savedObjectsClient,
});
expect(result).toEqual([
@ -93,7 +99,7 @@ describe('importRules', () => {
rulesResponseAcc: [],
overwriteRules: false,
detectionRulesClient: context.securitySolution.getDetectionRulesClient(),
existingLists: {},
savedObjectsClient,
});
expect(result).toEqual([{ rule_id: ruleToImport.rule_id, status_code: 200 }]);

View file

@ -5,11 +5,10 @@
* 2.0.
*/
import type { SavedObject } from '@kbn/core/server';
import type { SavedObject, SavedObjectsClientContract } from '@kbn/core/server';
import type {
ImportExceptionsListSchema,
ImportExceptionListItemSchema,
ExceptionListSchema,
} from '@kbn/securitysolution-io-ts-list-types';
import type { RuleToImport } from '../../../../../../common/api/detection_engine/rule_management';
@ -17,6 +16,7 @@ import type { ImportRuleResponse } from '../../../routes/utils';
import { createBulkErrorObject } from '../../../routes/utils';
import { checkRuleExceptionReferences } from './check_rule_exception_references';
import type { IDetectionRulesClient } from '../detection_rules_client/detection_rules_client_interface';
import { getReferencedExceptionLists } from './gather_referenced_exceptions';
export type PromiseFromStreams = RuleToImport | Error;
export interface RuleExceptionsPromiseFromStreams {
@ -44,15 +44,15 @@ export const importRules = async ({
rulesResponseAcc,
overwriteRules,
detectionRulesClient,
existingLists,
allowMissingConnectorSecrets,
savedObjectsClient,
}: {
ruleChunks: PromiseFromStreams[][];
rulesResponseAcc: ImportRuleResponse[];
overwriteRules: boolean;
detectionRulesClient: IDetectionRulesClient;
existingLists: Record<string, ExceptionListSchema>;
allowMissingConnectorSecrets?: boolean;
savedObjectsClient: SavedObjectsClientContract;
}) => {
let importRuleResponse: ImportRuleResponse[] = [...rulesResponseAcc];
@ -64,6 +64,10 @@ export const importRules = async ({
while (ruleChunks.length) {
const batchParseObjects = ruleChunks.shift() ?? [];
const existingLists = await getReferencedExceptionLists({
rules: batchParseObjects,
savedObjectsClient,
});
const newImportRuleResponse = await Promise.all(
batchParseObjects.reduce<Array<Promise<ImportRuleResponse>>>((accum, parsedRule) => {
const importsWorkerPromise = new Promise<ImportRuleResponse>(async (resolve, reject) => {

View file

@ -6,6 +6,7 @@
*/
import expect from 'expect';
import { range } from 'lodash';
import { EXCEPTION_LIST_ITEM_URL, EXCEPTION_LIST_URL } from '@kbn/securitysolution-list-constants';
import { getCreateExceptionListMinimalSchemaMock } from '@kbn/lists-plugin/common/schemas/request/create_exception_list_schema.mock';
@ -16,6 +17,7 @@ import {
getImportExceptionsListItemNewerVersionSchemaMock,
} from '@kbn/lists-plugin/common/schemas/request/import_exceptions_schema.mock';
import {
combineArraysToNdJson,
combineToNdJson,
fetchRule,
getCustomQueryRuleParams,
@ -1376,6 +1378,61 @@ export default ({ getService }: FtrProviderContext): void => {
});
});
it('should resolve 100 or more exception references when importing into a clean slate', async () => {
const rules = range(150).map((idx) =>
getCustomQueryRuleParams({
rule_id: `${idx}`,
exceptions_list: [
{
id: `${idx}`,
list_id: `${idx}`,
type: 'detection',
namespace_type: 'single',
},
],
})
);
const exceptionLists = range(150).map((idx) => ({
...getImportExceptionsListSchemaMock(`${idx}`),
id: `${idx}`,
type: 'detection',
namespace_type: 'single',
}));
const exceptionItems = range(150).map((idx) => ({
description: 'some description',
entries: [
{
field: 'some.not.nested.field',
operator: 'included',
type: 'match',
value: 'some value',
},
],
item_id: `item_id_${idx}`,
list_id: `${idx}`,
name: 'Query with a rule id',
type: 'simple',
}));
const importPayload = combineArraysToNdJson(rules, exceptionLists, exceptionItems);
const { body } = await supertest
.post(`${DETECTION_ENGINE_RULES_URL}/_import`)
.set('kbn-xsrf', 'true')
.set('elastic-api-version', '2023-10-31')
.attach('file', Buffer.from(importPayload), 'rules.ndjson')
.expect(200);
expect(body).toMatchObject({
success: true,
success_count: 150,
rules_count: 150,
errors: [],
exceptions_errors: [],
exceptions_success: true,
exceptions_success_count: 150,
});
});
it('@skipInServerless should resolve exception references that include comments', async () => {
// So importing a rule that references an exception list
// Keep in mind, no exception lists or rules exist yet

View file

@ -8,3 +8,11 @@
export function combineToNdJson(...parts: unknown[]): string {
return parts.map((p) => JSON.stringify(p)).join('\n');
}
export function combineArrayToNdJson(parts: unknown[]): string {
return parts.map((p) => JSON.stringify(p)).join('\n');
}
export function combineArraysToNdJson(...arrays: unknown[][]): string {
return arrays.map((array) => combineArrayToNdJson(array)).join('\n');
}

View file

@ -7,6 +7,7 @@
import type SuperTest from 'supertest';
import { v4 as uuidv4 } from 'uuid';
import limit from 'p-limit';
import type {
Type,
@ -229,12 +230,16 @@ export const deleteAllExceptionsByType = async (
.set('kbn-xsrf', 'true')
.send();
const ids: string[] = body.data.map((exception: ExceptionList) => exception.id);
for await (const id of ids) {
await supertest
.delete(`${EXCEPTION_LIST_URL}?id=${id}&namespace_type=${type}`)
.set('kbn-xsrf', 'true')
.send();
}
const limiter = limit(10);
const promises = ids.map((id) =>
limiter(() =>
supertest
.delete(`${EXCEPTION_LIST_URL}?id=${id}&namespace_type=${type}`)
.set('kbn-xsrf', 'true')
.send()
)
);
await Promise.all(promises);
const { body: finalCheck } = await supertest
.get(`${EXCEPTION_LIST_URL}/_find?namespace_type=${type}`)
.set('kbn-xsrf', 'true')