Skip to content

Commit

Permalink
[Security Solution] Recalculate isCustomized when bulk editing rules (#…
Browse files Browse the repository at this point in the history
…190041)

**Resolves: #187706

## Summary

Added the `isCustomized` field recalculation after a bulk edit operation
on rules as part of the [rules customization
epic](elastic/security-team#1974).

**Background**
The `isCustomized` field is a rule parameter indicating if a prebuilt
Elastic rule has been modified by a user. This field is extensively used
in the prebuilt rule upgrade workflow. It's essential to ensure any rule
modification operation recalculates this field to keep its value in sync
with the rule content. Most of the rule CRUD operations were already
covered in a previous PR: [Calculate and save ruleSource.isCustomized in
API endpoint handlers](#180145).
This PR addresses the remaining bulk rule modification operations
performed using the `rulesClient.bulkEdit` method.

**`rulesClient.bulkEdit` changes**

The `isCustomized` calculation is based on the entire rule object (i.e.,
rule params and attributes) and should be performed after all bulk
operations have been applied to the rule - after `operations` and
`paramsModifier`. To support this, I changed the `paramsModifier` to
accept entire rule object:

```diff
export type ParamsModifier<Params extends RuleParams> = (
-  params: Params
+  rule: Rule<Params>
) => Promise<ParamsModifierResult<Params>>;
```

**Security Solution Bulk Endpoint changes**

The `/api/detection_engine/rules/_bulk_action` endpoint now handles bulk
edit actions a bit differently. Previously, most of the bulk action was
delegated to the rules client. Now, we need to do some preparatory work:

1. Fetch the affected rules in memory first, regardless of whether we
received a query or rule IDs as input (previously delegated to
Alerting).
2. Identify all prebuilt rules among the fetched rules.
3. Fetch base versions of the prebuilt rules.
4. Provide base versions to `ruleModifier` for the `isCustomized`
calculation.

These changes add a few extra roundtrips to Elasticsearch and make the
bulk edit endpoint less efficient than before. However, this seems
justified given the added functionality of the customization epic. In
the future, we might consider optimizing to reduce the number of
database requests. Ideally, for Security Solution use cases, we would
need a more generic method than `bulkEdit`, such as `bulkUpdate`,
allowing us to implement any required rule update logic fully on the
solution side.
  • Loading branch information
xcrzx authored Aug 29, 2024
1 parent 96a3504 commit 5cf9522
Show file tree
Hide file tree
Showing 14 changed files with 404 additions and 77 deletions.
1 change: 1 addition & 0 deletions .buildkite/ftr_security_serverless_configs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ enabled:
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/large_prebuilt_rules_package/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/update_prebuilt_rules_package/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_bulk_actions/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_delete/trial_license_complete_tier/configs/serverless.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_delete/basic_license_essentials_tier/configs/serverless.config.ts
Expand Down
1 change: 1 addition & 0 deletions .buildkite/ftr_security_stateful_configs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ enabled:
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/large_prebuilt_rules_package/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/update_prebuilt_rules_package/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/prebuilt_rule_customization/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_bulk_actions/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_delete/trial_license_complete_tier/configs/ess.config.ts
- x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_delete/basic_license_essentials_tier/configs/ess.config.ts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3214,7 +3214,8 @@ describe('bulkEdit()', () => {
value: ['test-1'],
},
],
paramsModifier: async (params) => {
paramsModifier: async (rule) => {
const params = rule.params;
params.index = ['test-index-*'];

return { modifiedParams: params, isParamsUpdateSkipped: false, skipReasons: [] };
Expand Down Expand Up @@ -3431,7 +3432,8 @@ describe('bulkEdit()', () => {
value: ['test-1'],
},
],
paramsModifier: async (params) => {
paramsModifier: async (rule) => {
const params = rule.params;
params.index = ['test-index-*'];

return { modifiedParams: params, isParamsUpdateSkipped: false, skipReasons: [] };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
SavedObjectsUpdateResponse,
} from '@kbn/core/server';
import { validateAndAuthorizeSystemActions } from '../../../../lib/validate_authorize_system_actions';
import { RuleAction, RuleSystemAction } from '../../../../../common';
import { Rule, RuleAction, RuleSystemAction } from '../../../../../common';
import { RULE_SAVED_OBJECT_TYPE } from '../../../../saved_objects';
import { BulkActionSkipResult } from '../../../../../common/bulk_edit';
import { RuleTypeRegistry } from '../../../../types';
Expand Down Expand Up @@ -505,7 +505,8 @@ async function updateRuleAttributesAndParamsInMemory<Params extends RuleParams>(
validateScheduleInterval(context, updatedRule.schedule.interval, ruleType.id, rule.id);

const { modifiedParams: ruleParams, isParamsUpdateSkipped } = paramsModifier
? await paramsModifier(updatedRule.params)
? // TODO (http-versioning): Remove the cast when all rule types are fixed
await paramsModifier(updatedRule as Rule<Params>)
: {
modifiedParams: updatedRule.params,
isParamsUpdateSkipped: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@ import {
bulkEditOperationsSchema,
bulkEditOperationSchema,
} from '../schemas';
import { RuleParams, RuleDomain, Rule } from '../../../types';
import { RuleParams, RuleDomain } from '../../../types';
import { Rule } from '../../../../../../common';

export type BulkEditRuleSnoozeSchedule = TypeOf<typeof bulkEditRuleSnoozeScheduleSchema>;
export type BulkEditOperation = TypeOf<typeof bulkEditOperationSchema>;
export type BulkEditOperations = TypeOf<typeof bulkEditOperationsSchema>;

export type ParamsModifier<Params extends RuleParams> = (
params: Params
rule: Rule<Params>
) => Promise<ParamsModifierResult<Params>>;

interface ParamsModifierResult<Params extends RuleParams> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,19 @@ import { initPromisePool } from '../../../../../../utils/promise_pool';
import type { RuleAlertType } from '../../../../rule_schema';
import { readRules } from '../../../logic/detection_rules_client/read_rules';
import { findRules } from '../../../logic/search/find_rules';
import { MAX_RULES_TO_PROCESS_TOTAL } from './route';

export const fetchRulesByQueryOrIds = async ({
query,
ids,
rulesClient,
abortSignal,
maxRules,
}: {
query: string | undefined;
ids: string[] | undefined;
rulesClient: RulesClient;
abortSignal: AbortSignal;
maxRules: number;
}): Promise<PromisePoolOutcome<string, RuleAlertType>> => {
if (ids) {
return initPromisePool({
Expand All @@ -43,17 +44,17 @@ export const fetchRulesByQueryOrIds = async ({

const { data, total } = await findRules({
rulesClient,
perPage: MAX_RULES_TO_PROCESS_TOTAL,
perPage: maxRules,
filter: query,
page: undefined,
sortField: undefined,
sortOrder: undefined,
fields: undefined,
});

if (total > MAX_RULES_TO_PROCESS_TOTAL) {
if (total > maxRules) {
throw new BadRequestError(
`More than ${MAX_RULES_TO_PROCESS_TOTAL} rules matched the filter query. Try to narrow it down.`
`More than ${maxRules} rules matched the filter query. Try to narrow it down.`
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { IKibanaResponse, Logger } from '@kbn/core/server';
import { AbortError } from '@kbn/kibana-utils-plugin/common';
import { transformError } from '@kbn/securitysolution-es-utils';
import { buildRouteValidationWithZod } from '@kbn/zod-helpers';
import type { BulkActionSkipResult } from '@kbn/alerting-plugin/common';
import type { ConfigType } from '../../../../../../config';
import type { PerformRulesBulkActionResponse } from '../../../../../../../common/api/detection_engine/rule_management';
import {
Expand Down Expand Up @@ -42,8 +43,12 @@ import { buildBulkResponse } from './bulk_actions_response';
import { bulkEnableDisableRules } from './bulk_enable_disable_rules';
import { fetchRulesByQueryOrIds } from './fetch_rules_by_query_or_ids';
import { bulkScheduleBackfill } from './bulk_schedule_rule_run';
import { createPrebuiltRuleAssetsClient } from '../../../../prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client';

export const MAX_RULES_TO_PROCESS_TOTAL = 10000;
const MAX_RULES_TO_PROCESS_TOTAL = 10000;
// Set a lower limit for bulk edit as the rules client might fail with a "Query
// contains too many nested clauses" error
const MAX_RULES_TO_BULK_EDIT = 2000;
const MAX_ROUTE_CONCURRENCY = 5;

export const performBulkActionRoute = (
Expand Down Expand Up @@ -126,6 +131,7 @@ export const performBulkActionRoute = (
const savedObjectsClient = ctx.core.savedObjects.client;
const actionsClient = ctx.actions.getActionsClient();
const detectionRulesClient = ctx.securitySolution.getDetectionRulesClient();
const prebuiltRuleAssetClient = createPrebuiltRuleAssetsClient(savedObjectsClient);

const { getExporter, getClient } = ctx.core.savedObjects;
const client = getClient({ includedHiddenTypes: ['action'] });
Expand All @@ -141,38 +147,23 @@ export const performBulkActionRoute = (

const query = body.query !== '' ? body.query : undefined;

// handling this action before switch statement as bulkEditRules fetch rules within
// rulesClient method, hence there is no need to use fetchRulesByQueryOrIds utility
if (body.action === BulkActionTypeEnum.edit && !isDryRun) {
const { rules, errors, skipped } = await bulkEditRules({
actionsClient,
rulesClient,
filter: query,
ids: body.ids,
actions: body.edit,
mlAuthz,
experimentalFeatures: config.experimentalFeatures,
});

return buildBulkResponse(response, {
updated: rules,
skipped,
errors,
});
}

const fetchRulesOutcome = await fetchRulesByQueryOrIds({
rulesClient,
query,
ids: body.ids,
abortSignal: abortController.signal,
maxRules:
body.action === BulkActionTypeEnum.edit
? MAX_RULES_TO_BULK_EDIT
: MAX_RULES_TO_PROCESS_TOTAL,
});

const rules = fetchRulesOutcome.results.map(({ result }) => result);
const errors: BulkActionError[] = [...fetchRulesOutcome.errors];
let updated: RuleAlertType[] = [];
let created: RuleAlertType[] = [];
let deleted: RuleAlertType[] = [];
let skipped: BulkActionSkipResult[] = [];

switch (body.action) {
case BulkActionTypeEnum.enable: {
Expand Down Expand Up @@ -307,25 +298,40 @@ export const performBulkActionRoute = (
// will be processed only when isDryRun === true
// during dry run only validation is getting performed and rule is not saved in ES
case BulkActionTypeEnum.edit: {
const bulkActionOutcome = await initPromisePool({
concurrency: MAX_RULES_TO_UPDATE_IN_PARALLEL,
items: rules,
executor: async (rule) => {
await dryRunValidateBulkEditRule({
mlAuthz,
rule,
edit: body.edit,
experimentalFeatures: config.experimentalFeatures,
});
if (isDryRun) {
const bulkActionOutcome = await initPromisePool({
concurrency: MAX_RULES_TO_UPDATE_IN_PARALLEL,
items: rules,
executor: async (rule) => {
await dryRunValidateBulkEditRule({
mlAuthz,
rule,
edit: body.edit,
experimentalFeatures: config.experimentalFeatures,
});

return rule;
},
abortSignal: abortController.signal,
});
errors.push(...bulkActionOutcome.errors);
updated = bulkActionOutcome.results
.map(({ result }) => result)
.filter((rule): rule is RuleAlertType => rule !== null);
return rule;
},
abortSignal: abortController.signal,
});
errors.push(...bulkActionOutcome.errors);
updated = bulkActionOutcome.results
.map(({ result }) => result)
.filter((rule): rule is RuleAlertType => rule !== null);
} else {
const bulkEditResult = await bulkEditRules({
actionsClient,
rulesClient,
prebuiltRuleAssetClient,
rules,
actions: body.edit,
mlAuthz,
experimentalFeatures: config.experimentalFeatures,
});
updated = bulkEditResult.rules;
skipped = bulkEditResult.skipped;
errors.push(...bulkEditResult.errors);
}
break;
}

Expand All @@ -352,6 +358,7 @@ export const performBulkActionRoute = (
updated,
deleted,
created,
skipped,
errors,
isDryRun,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,30 @@
* 2.0.
*/

import type { RulesClient } from '@kbn/alerting-plugin/server';
import type { ActionsClient } from '@kbn/actions-plugin/server';
import type { RulesClient } from '@kbn/alerting-plugin/server';

import type { ExperimentalFeatures } from '../../../../../../common';
import type { BulkActionEditPayload } from '../../../../../../common/api/detection_engine/rule_management';

import type { MlAuthz } from '../../../../machine_learning/authz';

import { enrichFilterWithRuleTypeMapping } from '../search/enrich_filter_with_rule_type_mappings';
import type { RuleAlertType } from '../../../rule_schema';
import type { RuleAlertType, RuleParams } from '../../../rule_schema';

import type { IPrebuiltRuleAssetsClient } from '../../../prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client';
import { convertAlertingRuleToRuleResponse } from '../detection_rules_client/converters/convert_alerting_rule_to_rule_response';
import { calculateIsCustomized } from '../detection_rules_client/mergers/rule_source/calculate_is_customized';
import { bulkEditActionToRulesClientOperation } from './action_to_rules_client_operation';
import { ruleParamsModifier } from './rule_params_modifier';
import { splitBulkEditActions } from './split_bulk_edit_actions';
import { validateBulkEditRule } from './validations';
import { bulkEditActionToRulesClientOperation } from './action_to_rules_client_operation';

export interface BulkEditRulesArguments {
actionsClient: ActionsClient;
rulesClient: RulesClient;
prebuiltRuleAssetClient: IPrebuiltRuleAssetsClient;
actions: BulkActionEditPayload[];
filter?: string;
ids?: string[];
rules: RuleAlertType[];
mlAuthz: MlAuthz;
experimentalFeatures: ExperimentalFeatures;
}
Expand All @@ -41,27 +43,70 @@ export interface BulkEditRulesArguments {
export const bulkEditRules = async ({
actionsClient,
rulesClient,
ids,
prebuiltRuleAssetClient,
rules,
actions,
filter,
mlAuthz,
experimentalFeatures,
}: BulkEditRulesArguments) => {
// Split operations
const { attributesActions, paramsActions } = splitBulkEditActions(actions);
const operations = attributesActions
.map((attribute) => bulkEditActionToRulesClientOperation(actionsClient, attribute))
.flat();
const result = await rulesClient.bulkEdit({
...(ids ? { ids } : { filter: enrichFilterWithRuleTypeMapping(filter) }),

// Check if there are any prebuilt rules and fetch their base versions
const prebuiltRules = rules.filter((rule) => rule.params.immutable);
const baseVersions = await prebuiltRuleAssetClient.fetchAssetsByVersion(
prebuiltRules.map((rule) => ({
rule_id: rule.params.ruleId,
version: rule.params.version,
}))
);
const baseVersionsMap = new Map(
baseVersions.map((baseVersion) => [baseVersion.rule_id, baseVersion])
);

const result = await rulesClient.bulkEdit<RuleParams>({
ids: rules.map((rule) => rule.id),
operations,
paramsModifier: async (ruleParams: RuleAlertType['params']) => {
paramsModifier: async (rule) => {
const ruleParams = rule.params;

await validateBulkEditRule({
mlAuthz,
ruleType: ruleParams.type,
edit: actions,
immutable: ruleParams.immutable,
experimentalFeatures,
});
return ruleParamsModifier(ruleParams, paramsActions, experimentalFeatures);
const { modifiedParams, isParamsUpdateSkipped } = ruleParamsModifier(
ruleParams,
paramsActions,
experimentalFeatures
);

// Update rule source
const updatedRule = {
...rule,
params: modifiedParams,
};
const ruleResponse = convertAlertingRuleToRuleResponse(updatedRule);
const ruleSource =
ruleResponse.immutable === true
? {
type: 'external' as const,
isCustomized: calculateIsCustomized(
baseVersionsMap.get(ruleResponse.rule_id),
ruleResponse
),
}
: {
type: 'internal' as const,
};
modifiedParams.ruleSource = ruleSource;

return { modifiedParams, isParamsUpdateSkipped };
},
});

Expand Down
Loading

0 comments on commit 5cf9522

Please sign in to comment.