[Security Solution] Allow importing of prebuilt rules via the API (el…

…astic#190198) ## Summary This PR introduces the backend functionality necessary to import prebuilt rules via our existing import API. The [RFC](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/docs/rfcs/detection_response/prebuilt_rules_customization.md) goes into detail, and the import-specific issue is described [here](elastic#180168), but at a high level we're adding two things in this PR: 1. The logic to calculate the new `rule_source` field on import, which contains the information about the rule's relationship to existing prebuilt rules. 2. A new code path for importing rules, which respects the calculated `rule_source` field. In order to maintain backwards compatibility with the existing import logic, and because the existing import implementation is hard to modify/extend, I opted to add a top-level switch on the feature flag in the import route itself, which calls either the existing import function (now named `importRulesLegacy`), or the new function, `importRules`, which ultimately calls the new `DetectionRulesClient` method, `importRules`. Neither knows about the feature flag, and thanks to great suggestions from @xcrzx there are nice, clean boundaries between the import functions and the client methods. I went down the path of trying to write the new import code by reusing the outer `importRules` function, but after discussion with the team we decided it was simplest to simply bifurcate the code at that point, so that we have: 1. The legacy import code, which: * only supports custom rules (`immutable: false`) * accepts `version` as an optional parameter * calculates a legacy `rule_source` value based on the `immutable` field 2. The new import code, which * Installs the prebuilt rules assets as necessary * Accepts both kinds of rules (`immutable: true/false`) * Requires `version` as a parameter for _prebuilt_ rules * Allows `version` to be optional for custom rules * calculates a proper `rule_source` (and `immutable`) ### Deprecation of `immutable` The RFC (and thus I) had initially thought that we'd be deprecating the `immutable` field as part of this PR/Epic. However, after [discussion](elastic#190198 (comment)) we have opted to continue supporting `immutable` until such time as we can drop it, compatibility-wise. ## Steps to Review 1. Enable the Feature Flag: `prebuiltRulesCustomizationEnabled` 2. Install the prebuilt rules package via fleet 3. Create and export a custom rule to serve as a template (or download one: curl -L -H 'Authorization: 8eef0fe5d7dfc52b' -o 'rules_export (1).ndjson' https://upload.elastic.co/d/4693e7fe4356ce7bcf7b7d6b72881a9fd189730c61bf5ef47c9930458d746979 ) 4. Install some prebuilt rules, and obtain a prebuilt rule's `rule_id`, e.g. `ac8805f6-1e08-406c-962e-3937057fa86f` 5. Edit the `rules_export.ndjson` to contain only the first line, and modify the `rule_id` with the prebuilt rule's 6. Import `rules_export.ndjson` and then retrieve the rule via the Dev Console: GET kbn:api/detection_engine/rules?rule_id=ac8805f6-1e08-406c-962e-3937057fa86f 7. Observe that the rule has the expected `rule_source` and `immutable` values 8. Test other permutations of import by modifying `rules_export.ndjson` and re-importing; see ([the test plan](elastic#191116) as well as a [reference table of scenarios](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/docs/rfcs/detection_response/prebuilt_rules_customization.md#importing-rules)) ### Checklist Delete any items that are not applicable to this PR. - [x] 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/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Georgii Gorbachev <[email protected]> (cherry picked from commit c0b1301)
kibanamachine · Oct 16, 2024 · 7015873 · 7015873
1 parent 2c833fd
commit 7015873
Show file tree

Hide file tree

Showing 50 changed files with 2,730 additions and 650 deletions.
diff --git a/..._solution/common/api/detection_engine/rule_management/import_rules/rule_to_import.mock.ts b/..._solution/common/api/detection_engine/rule_management/import_rules/rule_to_import.mock.ts
@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import type { RuleToImport } from './rule_to_import';
+import type { RuleToImport, ValidatedRuleToImport } from './rule_to_import';
 
 export const getImportRulesSchemaMock = (rewrites?: Partial<RuleToImport>): RuleToImport =>
   ({
@@ -15,12 +15,18 @@ export const getImportRulesSchemaMock = (rewrites?: Partial<RuleToImport>): Rule
     severity: 'high',
     type: 'query',
     risk_score: 55,
-    language: 'kuery',
     rule_id: 'rule-1',
     immutable: false,
     ...rewrites,
   } as RuleToImport);
 
+export const getValidatedRuleToImportMock = (
+  overrides?: Partial<ValidatedRuleToImport>
+): ValidatedRuleToImport => ({
+  version: 1,
+  ...getImportRulesSchemaMock(overrides),
+});
+
 export const getImportRulesWithIdSchemaMock = (ruleId = 'rule-1'): RuleToImport => ({
   id: '6afb8ce1-ea94-4790-8653-fd0b021d2113',
   description: 'some description',
@@ -29,7 +35,6 @@ export const getImportRulesWithIdSchemaMock = (ruleId = 'rule-1'): RuleToImport
   severity: 'high',
   type: 'query',
   risk_score: 55,
-  language: 'kuery',
   rule_id: ruleId,
   immutable: false,
 });
@@ -63,7 +68,6 @@ export const getImportThreatMatchRulesSchemaMock = (
     severity: 'high',
     type: 'threat_match',
     risk_score: 55,
-    language: 'kuery',
     rule_id: 'rule-1',
     threat_index: ['index-123'],
     threat_mapping: [{ entries: [{ field: 'host.name', type: 'mapping', value: 'host.name' }] }],

diff --git a/..._solution/common/api/detection_engine/rule_management/import_rules/rule_to_import.test.ts b/..._solution/common/api/detection_engine/rule_management/import_rules/rule_to_import.test.ts
@@ -550,7 +550,7 @@ describe('RuleToImport', () => {
     );
   });
 
-  test('You cannot set the immutable to a number when trying to create a rule', () => {
+  test('You cannot set immutable to a number', () => {
     const payload = getImportRulesSchemaMock({
       // @ts-expect-error assign unsupported value
       immutable: 5,
@@ -560,11 +560,11 @@ describe('RuleToImport', () => {
     expectParseError(result);
 
     expect(stringifyZodError(result.error)).toMatchInlineSnapshot(
-      `"immutable: Invalid literal value, expected false"`
+      `"immutable: Expected boolean, received number"`
     );
   });
 
-  test('You can optionally set the immutable to be false', () => {
+  test('You can optionally set immutable to false', () => {
     const payload: RuleToImportInput = getImportRulesSchemaMock({
       immutable: false,
     });
@@ -574,32 +574,14 @@ describe('RuleToImport', () => {
     expectParseSuccess(result);
   });
 
-  test('You cannot set the immutable to be true', () => {
+  test('You can optionally set immutable to true', () => {
     const payload = getImportRulesSchemaMock({
-      // @ts-expect-error assign unsupported value
       immutable: true,
     });
 
     const result = RuleToImport.safeParse(payload);
-    expectParseError(result);
-
-    expect(stringifyZodError(result.error)).toMatchInlineSnapshot(
-      `"immutable: Invalid literal value, expected false"`
-    );
-  });
-
-  test('You cannot set the immutable to be a number', () => {
-    const payload = getImportRulesSchemaMock({
-      // @ts-expect-error assign unsupported value
-      immutable: 5,
-    });
 
-    const result = RuleToImport.safeParse(payload);
-    expectParseError(result);
-
-    expect(stringifyZodError(result.error)).toMatchInlineSnapshot(
-      `"immutable: Invalid literal value, expected false"`
-    );
+    expectParseSuccess(result);
   });
 
   test('You cannot set the risk_score to 101', () => {
@@ -1091,5 +1073,16 @@ describe('RuleToImport', () => {
       expectParseSuccess(result);
       expect(result.data).toEqual(payload);
     });
+
+    describe('backwards compatibility', () => {
+      it('allows version to be absent', () => {
+        const payload = getImportRulesSchemaMock();
+        delete payload.version;
+
+        const result = RuleToImport.safeParse(payload);
+        expectParseSuccess(result);
+        expect(result.data).toEqual(payload);
+      });
+    });
   });
 });
diff --git a/...urity_solution/common/api/detection_engine/rule_management/import_rules/rule_to_import.ts b/...urity_solution/common/api/detection_engine/rule_management/import_rules/rule_to_import.ts
@@ -12,13 +12,14 @@ import {
   RequiredFieldInput,
   RuleSignatureId,
   TypeSpecificCreateProps,
+  RuleVersion,
 } from '../../model/rule_schema';
 
 /**
  * Differences from this and the createRulesSchema are
  *   - rule_id is required
  *   - id is optional (but ignored in the import code - rule_id is exclusively used for imports)
- *   - immutable is optional but if it is any value other than false it will be rejected
+ *   - immutable is optional (but ignored in the import code)
  *   - created_at is optional (but ignored in the import code)
  *   - updated_at is optional (but ignored in the import code)
  *   - created_by is optional (but ignored in the import code)
@@ -29,7 +30,6 @@ export type RuleToImportInput = z.input<typeof RuleToImport>;
 export const RuleToImport = BaseCreateProps.and(TypeSpecificCreateProps).and(
   ResponseFields.partial().extend({
     rule_id: RuleSignatureId,
-    immutable: z.literal(false).default(false),
     /*
       Overriding `required_fields` from ResponseFields because
       in ResponseFields `required_fields` has the output type,
@@ -40,3 +40,19 @@ export const RuleToImport = BaseCreateProps.and(TypeSpecificCreateProps).and(
     required_fields: z.array(RequiredFieldInput).optional(),
   })
 );
+
+/**
+ * This type represents new rules being imported once the prebuilt rule
+ * customization work is complete. In order to provide backwards compatibility
+ * with existing rules, and not change behavior, we now validate `version` in
+ * the route as opposed to the type itself.
+ *
+ * It differs from RuleToImport in that it requires a `version` field.
+ */
+export type ValidatedRuleToImport = z.infer<typeof ValidatedRuleToImport>;
+export type ValidatedRuleToImportInput = z.input<typeof ValidatedRuleToImport>;
+export const ValidatedRuleToImport = RuleToImport.and(
+  z.object({
+    version: RuleVersion,
+  })
+);
diff --git a/...ion/common/api/detection_engine/rule_management/import_rules/rule_to_import_validation.ts b/...ion/common/api/detection_engine/rule_management/import_rules/rule_to_import_validation.ts
@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import type { RuleToImport } from './rule_to_import';
+import type { RuleToImport, ValidatedRuleToImport } from './rule_to_import';
 
 /**
  * Additional validation that is implemented outside of the schema itself.
@@ -55,3 +55,6 @@ const validateThreshold = (rule: RuleToImport): string[] => {
   }
   return errors;
 };
+
+export const ruleToImportHasVersion = (rule: RuleToImport): rule is ValidatedRuleToImport =>
+  !!rule.version;
diff --git a/...rver/lib/detection_engine/prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client.ts b/...rver/lib/detection_engine/prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client.ts
@@ -20,7 +20,7 @@ const MAX_PREBUILT_RULES_COUNT = 10_000;
 export interface IPrebuiltRuleAssetsClient {
   fetchLatestAssets: () => Promise<PrebuiltRuleAsset[]>;
 
-  fetchLatestVersions(): Promise<RuleVersionSpecifier[]>;
+  fetchLatestVersions(ruleIds?: string[]): Promise<RuleVersionSpecifier[]>;
 
   fetchAssetsByVersion(versions: RuleVersionSpecifier[]): Promise<PrebuiltRuleAsset[]>;
 }
@@ -72,8 +72,12 @@ export const createPrebuiltRuleAssetsClient = (
       });
     },
 
-    fetchLatestVersions: (): Promise<RuleVersionSpecifier[]> => {
+    fetchLatestVersions: (ruleIds: string[] = []): Promise<RuleVersionSpecifier[]> => {
       return withSecuritySpan('IPrebuiltRuleAssetsClient.fetchLatestVersions', async () => {
+        const filter = ruleIds
+          .map((ruleId) => `${PREBUILT_RULE_ASSETS_SO_TYPE}.attributes.rule_id: ${ruleId}`)
+          .join(' OR ');
+
         const findResult = await savedObjectsClient.find<
           PrebuiltRuleAsset,
           {
@@ -83,6 +87,7 @@ export const createPrebuiltRuleAssetsClient = (
           }
         >({
           type: PREBUILT_RULE_ASSETS_SO_TYPE,
+          filter,
           aggs: {
             rules: {
               terms: {

diff --git a/...solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts b/...solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts
@@ -15,7 +15,7 @@ import {
 import { getRulesSchemaMock } from '../../../../../../../common/api/detection_engine/model/rule_schema/rule_response_schema.mock';
 
 import type { requestMock } from '../../../../routes/__mocks__';
-import { createMockConfig, requestContextMock, serverMock } from '../../../../routes/__mocks__';
+import { configMock, requestContextMock, serverMock } from '../../../../routes/__mocks__';
 import { buildHapiStream } from '../../../../routes/__mocks__/utils';
 import {
   getImportRulesRequest,
@@ -26,23 +26,30 @@ import {
   getBasicEmptySearchResponse,
 } from '../../../../routes/__mocks__/request_responses';
 
-import * as createRulesAndExceptionsStreamFromNdJson from '../../../logic/import/create_rules_stream_from_ndjson';
+import * as createPromiseFromRuleImportStream from '../../../logic/import/create_promise_from_rule_import_stream';
 import { getQueryRuleParams } from '../../../../rule_schema/mocks';
 import { importRulesRoute } from './route';
 import { HttpAuthzError } from '../../../../../machine_learning/validation';
+import { createPrebuiltRuleAssetsClient as createPrebuiltRuleAssetsClientMock } from '../../../../prebuilt_rules/logic/rule_assets/__mocks__/prebuilt_rule_assets_client';
 
 jest.mock('../../../../../machine_learning/authz');
 
+let mockPrebuiltRuleAssetsClient: ReturnType<typeof createPrebuiltRuleAssetsClientMock>;
+
+jest.mock('../../../../prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client', () => ({
+  createPrebuiltRuleAssetsClient: () => mockPrebuiltRuleAssetsClient,
+}));
+
 describe('Import rules route', () => {
-  let config: ReturnType<typeof createMockConfig>;
+  let config: ReturnType<typeof configMock.createDefault>;
   let server: ReturnType<typeof serverMock.create>;
   let request: ReturnType<typeof requestMock.create>;
   let { clients, context } = requestContextMock.createTools();
 
   beforeEach(() => {
     server = serverMock.create();
     ({ clients, context } = requestContextMock.createTools());
-    config = createMockConfig();
+    config = configMock.createDefault();
     const hapiStream = buildHapiStream(ruleIdsToNdJsonString(['rule-1']));
     request = getImportRulesRequest(hapiStream);
 
@@ -54,6 +61,7 @@ describe('Import rules route', () => {
     context.core.elasticsearch.client.asCurrentUser.search.mockResolvedValue(
       elasticsearchClientMock.createSuccessTransportRequestPromise(getBasicEmptySearchResponse())
     );
+    mockPrebuiltRuleAssetsClient = createPrebuiltRuleAssetsClientMock();
     importRulesRoute(server.router, config);
   });
 
@@ -112,9 +120,9 @@ describe('Import rules route', () => {
       });
     });
 
-    test('returns error if createRulesAndExceptionsStreamFromNdJson throws error', async () => {
+    test('returns error if createPromiseFromRuleImportStream throws error', async () => {
       const transformMock = jest
-        .spyOn(createRulesAndExceptionsStreamFromNdJson, 'createRulesAndExceptionsStreamFromNdJson')
+        .spyOn(createPromiseFromRuleImportStream, 'createPromiseFromRuleImportStream')
         .mockImplementation(() => {
           throw new Error('Test error');
         });
@@ -133,6 +141,30 @@ describe('Import rules route', () => {
       expect(response.status).toEqual(400);
       expect(response.body).toEqual({ message: 'Invalid file extension .html', status_code: 400 });
     });
+
+    describe('with prebuilt rules customization enabled', () => {
+      beforeEach(() => {
+        clients.detectionRulesClient.importRules.mockResolvedValueOnce([]);
+        server = serverMock.create(); // old server already registered this route
+        config = configMock.withExperimentalFeature(config, 'prebuiltRulesCustomizationEnabled');
+
+        importRulesRoute(server.router, config);
+      });
+
+      test('returns 500 if importing fails', async () => {
+        clients.detectionRulesClient.importRules
+          .mockReset()
+          .mockRejectedValue(new Error('test error'));
+
+        const response = await server.inject(request, requestContextMock.convertContext(context));
+
+        expect(response.status).toEqual(500);
+        expect(response.body).toMatchObject({
+          message: 'test error',
+          status_code: 500,
+        });
+      });
+    });
   });
 
   describe('single rule import', () => {