[Security Solution] Prevent non-customizable fields from updating for…

… Prebuilt rule types (elastic#195318) ## Summary Addresses elastic#180273 Adds validation in the `detectionRulesClient` to prevent the updating of non-customizable fields in Prebuilt rule types (i.e. external `rule_source`). Returns a `400` error if `author` or `license` fields are updated via `PUT` and `PATCH` endpoints for external rules. Also updates related test utils to reflect this new logic ### Checklist Delete any items that are not applicable to this PR. - [ ] [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit 0004217)
kibanamachine · Oct 10, 2024 · 1d19d9f · 1d19d9f
1 parent e435c47
commit 1d19d9f
Show file tree

Hide file tree

Showing 13 changed files with 218 additions and 13 deletions.
diff --git a/.../server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.mock.ts b/.../server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.mock.ts
@@ -18,6 +18,7 @@ export const getPrebuiltRuleMock = (rewrites?: Partial<PrebuiltRuleAsset>): Preb
     language: 'kuery',
     rule_id: 'rule-1',
     version: 1,
+    author: [],
     ...rewrites,
   } as PrebuiltRuleAsset);
 
@@ -51,6 +52,7 @@ export const getPrebuiltThreatMatchRuleMock = (): PrebuiltRuleAsset => ({
   language: 'kuery',
   rule_id: 'rule-1',
   version: 1,
+  author: [],
   threat_query: '*:*',
   threat_index: ['list-index'],
   threat_mapping: [

diff --git a/...ne/rule_management/logic/detection_rules_client/detection_rules_client.patch_rule.test.ts b/...ne/rule_management/logic/detection_rules_client/detection_rules_client.patch_rule.test.ts
@@ -277,6 +277,27 @@ describe('DetectionRulesClient.patchRule', () => {
     expect(rulesClient.create).not.toHaveBeenCalled();
   });
 
+  it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
+    // Mock the existing rule
+    const existingRule = {
+      ...getRulesSchemaMock(),
+      rule_source: { type: 'external', is_customized: true },
+    };
+    (getRuleByRuleId as jest.Mock).mockResolvedValueOnce(existingRule);
+
+    // Mock the rule update
+    const rulePatch = getCreateRulesSchemaMock('query-rule-id');
+    rulePatch.license = 'new license';
+
+    // Mock the rule returned after update; not used for this test directly but
+    // needed so that the patchRule method does not throw
+    rulesClient.update.mockResolvedValue(getRuleMock(getQueryRuleParams()));
+
+    await expect(detectionRulesClient.patchRule({ rulePatch })).rejects.toThrow(
+      'Cannot update "license" field for prebuilt rules'
+    );
+  });
+
   describe('actions', () => {
     it("updates the rule's actions if provided", async () => {
       // Mock the existing rule

diff --git a/...e/rule_management/logic/detection_rules_client/detection_rules_client.update_rule.test.ts b/...e/rule_management/logic/detection_rules_client/detection_rules_client.update_rule.test.ts
@@ -498,5 +498,26 @@ describe('DetectionRulesClient.updateRule', () => {
         })
       );
     });
+
+    it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
+      // Mock the existing rule
+      const existingRule = {
+        ...getRulesSchemaMock(),
+        rule_source: { type: 'external', is_customized: true },
+      };
+
+      (getRuleByRuleId as jest.Mock).mockResolvedValueOnce(existingRule);
+
+      // Mock the rule update
+      const ruleUpdate = { ...getCreateRulesSchemaMock(), author: ['new user'] };
+
+      // Mock the rule returned after update; not used for this test directly but
+      // needed so that the patchRule method does not throw
+      rulesClient.update.mockResolvedValue(getRuleMock(getQueryRuleParams()));
+
+      await expect(detectionRulesClient.updateRule({ ruleUpdate })).rejects.toThrow(
+        'Cannot update "author" field for prebuilt rules'
+      );
+    });
   });
 });
diff --git a/...r/lib/detection_engine/rule_management/logic/detection_rules_client/methods/patch_rule.ts b/...r/lib/detection_engine/rule_management/logic/detection_rules_client/methods/patch_rule.ts
@@ -16,6 +16,7 @@ import type { MlAuthz } from '../../../../../machine_learning/authz';
 import type { IPrebuiltRuleAssetsClient } from '../../../../prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client';
 import { applyRulePatch } from '../mergers/apply_rule_patch';
 import { getIdError } from '../../../utils/utils';
+import { validateNonCustomizablePatchFields } from '../../../utils/validate';
 import { convertAlertingRuleToRuleResponse } from '../converters/convert_alerting_rule_to_rule_response';
 import { convertRuleResponseToAlertingRule } from '../converters/convert_rule_response_to_alerting_rule';
 import { ClientError, toggleRuleEnabledOnUpdate, validateMlAuth } from '../utils';
@@ -51,6 +52,8 @@ export const patchRule = async ({
 
   await validateMlAuth(mlAuthz, rulePatch.type ?? existingRule.type);
 
+  validateNonCustomizablePatchFields(rulePatch, existingRule);
+
   const patchedRule = await applyRulePatch({
     prebuiltRuleAssetClient,
     existingRule,

diff --git a/.../lib/detection_engine/rule_management/logic/detection_rules_client/methods/update_rule.ts b/.../lib/detection_engine/rule_management/logic/detection_rules_client/methods/update_rule.ts
@@ -11,6 +11,7 @@ import type { RuleResponse } from '../../../../../../../common/api/detection_eng
 import type { MlAuthz } from '../../../../../machine_learning/authz';
 import { applyRuleUpdate } from '../mergers/apply_rule_update';
 import { getIdError } from '../../../utils/utils';
+import { validateNonCustomizableUpdateFields } from '../../../utils/validate';
 import { convertRuleResponseToAlertingRule } from '../converters/convert_rule_response_to_alerting_rule';
 
 import { ClientError, toggleRuleEnabledOnUpdate, validateMlAuth } from '../utils';
@@ -50,6 +51,8 @@ export const updateRule = async ({
     throw new ClientError(error.message, error.statusCode);
   }
 
+  validateNonCustomizableUpdateFields(ruleUpdate, existingRule);
+
   const ruleWithUpdates = await applyRuleUpdate({
     prebuiltRuleAssetClient,
     existingRule,

diff --git a/...k/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts b/...k/plugins/security_solution/server/lib/detection_engine/rule_management/utils/validate.ts
@@ -15,6 +15,7 @@ import {
   RuleResponse,
   type RuleResponseAction,
   type RuleUpdateProps,
+  type RulePatchProps,
 } from '../../../../../common/api/detection_engine';
 import {
   RESPONSE_ACTION_API_COMMAND_TO_CONSOLE_COMMAND_MAP,
@@ -25,6 +26,7 @@ import { CustomHttpRequestError } from '../../../../utils/custom_http_request_er
 import { hasValidRuleType, type RuleAlertType, type RuleParams } from '../../rule_schema';
 import { type BulkError, createBulkErrorObject } from '../../routes/utils';
 import { internalRuleToAPIResponse } from '../logic/detection_rules_client/converters/internal_rule_to_api_response';
+import { ClientError } from '../logic/detection_rules_client/utils';
 
 export const transformValidateBulkError = (
   ruleId: string,
@@ -117,3 +119,31 @@ function rulePayloadContainsResponseActions(rule: RuleCreateProps | RuleUpdatePr
 function ruleObjectContainsResponseActions(rule?: RuleAlertType) {
   return rule != null && 'params' in rule && 'responseActions' in rule?.params;
 }
+
+export const validateNonCustomizableUpdateFields = (
+  ruleUpdate: RuleUpdateProps,
+  existingRule: RuleResponse
+) => {
+  // We don't allow non-customizable fields to be changed for prebuilt rules
+  if (existingRule.rule_source && existingRule.rule_source.type === 'external') {
+    if (!isEqual(ruleUpdate.author, existingRule.author)) {
+      throw new ClientError(`Cannot update "author" field for prebuilt rules`, 400);
+    } else if (ruleUpdate.license !== existingRule.license) {
+      throw new ClientError(`Cannot update "license" field for prebuilt rules`, 400);
+    }
+  }
+};
+
+export const validateNonCustomizablePatchFields = (
+  rulePatch: RulePatchProps,
+  existingRule: RuleResponse
+) => {
+  // We don't allow non-customizable fields to be changed for prebuilt rules
+  if (existingRule.rule_source && existingRule.rule_source.type === 'external') {
+    if (rulePatch.author && !isEqual(rulePatch.author, existingRule.author)) {
+      throw new ClientError(`Cannot update "author" field for prebuilt rules`, 400);
+    } else if (rulePatch.license != null && rulePatch.license !== existingRule.license) {
+      throw new ClientError(`Cannot update "license" field for prebuilt rules`, 400);
+    }
+  }
+};
diff --git a/...ections_response/rules_management/rule_patch/basic_license_essentials_tier/patch_rules.ts b/...ections_response/rules_management/rule_patch/basic_license_essentials_tier/patch_rules.ts
@@ -16,6 +16,9 @@ import {
   removeServerGeneratedPropertiesIncludingRuleId,
   getSimpleRuleOutputWithoutRuleId,
   updateUsername,
+  createHistoricalPrebuiltRuleAssetSavedObjects,
+  installPrebuiltRules,
+  createRuleAssetSavedObject,
 } from '../../../utils';
 import {
   createAlertsIndex,
@@ -238,6 +241,25 @@ export default ({ getService }: FtrProviderContext) => {
         });
       });
 
+      it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
+        // Install base prebuilt detection rule
+        await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
+          createRuleAssetSavedObject({ rule_id: 'rule-1', author: ['elastic'] }),
+        ]);
+        await installPrebuiltRules(es, supertest);
+
+        const { body } = await securitySolutionApi
+          .patchRule({
+            body: {
+              rule_id: 'rule-1',
+              author: ['new user'],
+            },
+          })
+          .expect(400);
+
+        expect(body.message).toEqual('Cannot update "author" field for prebuilt rules');
+      });
+
       describe('max signals', () => {
         afterEach(async () => {
           await deleteAllRules(supertest, log);

diff --git a/...ns_response/rules_management/rule_patch/basic_license_essentials_tier/patch_rules_bulk.ts b/...ns_response/rules_management/rule_patch/basic_license_essentials_tier/patch_rules_bulk.ts
@@ -16,6 +16,9 @@ import {
   getSimpleRuleOutputWithoutRuleId,
   removeServerGeneratedPropertiesIncludingRuleId,
   updateUsername,
+  createHistoricalPrebuiltRuleAssetSavedObjects,
+  installPrebuiltRules,
+  createRuleAssetSavedObject,
 } from '../../../utils';
 import {
   createAlertsIndex,
@@ -347,6 +350,41 @@ export default ({ getService }: FtrProviderContext) => {
           },
         ]);
       });
+
+      it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
+        // Install base prebuilt detection rule
+        await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
+          createRuleAssetSavedObject({ rule_id: 'rule-1', author: ['elastic'] }),
+          createRuleAssetSavedObject({ rule_id: 'rule-2', license: 'basic' }),
+        ]);
+        await installPrebuiltRules(es, supertest);
+
+        const { body } = await securitySolutionApi
+          .bulkPatchRules({
+            body: [
+              { rule_id: 'rule-1', author: ['new user'] },
+              { rule_id: 'rule-2', license: 'new license' },
+            ],
+          })
+          .expect(200);
+
+        expect([body[0], body[1]]).toEqual([
+          {
+            error: {
+              message: 'Cannot update "author" field for prebuilt rules',
+              status_code: 400,
+            },
+            rule_id: 'rule-1',
+          },
+          {
+            error: {
+              message: 'Cannot update "license" field for prebuilt rules',
+              status_code: 400,
+            },
+            rule_id: 'rule-2',
+          },
+        ]);
+      });
     });
   });
 };
diff --git a/...tions_response/rules_management/rule_update/basic_license_essentials_tier/update_rules.ts b/...tions_response/rules_management/rule_update/basic_license_essentials_tier/update_rules.ts
@@ -18,6 +18,9 @@ import {
   getSimpleMlRuleUpdate,
   getSimpleRule,
   updateUsername,
+  createHistoricalPrebuiltRuleAssetSavedObjects,
+  installPrebuiltRules,
+  createRuleAssetSavedObject,
 } from '../../../utils';
 import {
   createAlertsIndex,
@@ -309,6 +312,33 @@ export default ({ getService }: FtrProviderContext) => {
           expect(updatedRuleResponse).toMatchObject(expectedRule);
         });
       });
+
+      it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
+        // Install base prebuilt detection rule
+        await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
+          createRuleAssetSavedObject({ rule_id: 'rule-1', license: 'elastic' }),
+        ]);
+        await installPrebuiltRules(es, supertest);
+
+        const { body: existingRule } = await securitySolutionApi
+          .readRule({
+            query: { rule_id: 'rule-1' },
+          })
+          .expect(200);
+
+        const { body } = await securitySolutionApi
+          .updateRule({
+            body: getCustomQueryRuleParams({
+              ...existingRule,
+              rule_id: 'rule-1',
+              id: undefined,
+              license: 'new license',
+            }),
+          })
+          .expect(400);
+
+        expect(body.message).toEqual('Cannot update "license" field for prebuilt rules');
+      });
     });
   });
 };
diff --git a/..._response/rules_management/rule_update/basic_license_essentials_tier/update_rules_bulk.ts b/..._response/rules_management/rule_update/basic_license_essentials_tier/update_rules_bulk.ts
@@ -17,6 +17,9 @@ import {
   getSimpleRuleUpdate,
   getSimpleRule,
   updateUsername,
+  createHistoricalPrebuiltRuleAssetSavedObjects,
+  installPrebuiltRules,
+  createRuleAssetSavedObject,
 } from '../../../utils';
 import {
   createAlertsIndex,
@@ -370,6 +373,30 @@ export default ({ getService }: FtrProviderContext) => {
           },
         ]);
       });
+
+      it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
+        // Install base prebuilt detection rule
+        await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
+          createRuleAssetSavedObject({ rule_id: 'rule-1', author: ['elastic'] }),
+        ]);
+        await installPrebuiltRules(es, supertest);
+
+        const { body } = await securitySolutionApi
+          .bulkUpdateRules({
+            body: [getCustomQueryRuleParams({ rule_id: 'rule-1', author: ['new user'] })],
+          })
+          .expect(200);
+
+        expect([body[0]]).toEqual([
+          {
+            error: {
+              message: 'Cannot update "author" field for prebuilt rules',
+              status_code: 400,
+            },
+            rule_id: 'rule-1',
+          },
+        ]);
+      });
     });
   });
 };