Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] Move calculation of rule source outside of applyRuleUpdate #199720

Closed
wants to merge 6 commits into from
Closed

[Security Solution] Move calculation of rule source outside of applyRuleUpdate #199720

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5b67f44
Move calculation of rule source outside of applyRuleUpdate
rylnd Nov 11, 2024
da7f9c8
Merge branch 'main' into rylnd/rule_import_improvements
rylnd Nov 14, 2024
2966af2
Merge branch 'main' into rylnd/rule_import_improvements
rylnd Nov 19, 2024
9e52e57
Merge branch 'main' into rylnd/rule_import_improvements
rylnd Nov 22, 2024
e760dd4
Merge branch 'main' into rylnd/rule_import_improvements
elasticmachine Nov 26, 2024
dc3e349
Merge branch 'main' into rylnd/rule_import_improvements
elasticmachine Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions ...etection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,14 @@ import type {
RuleResponse,
RuleUpdateProps,
} from '../../../../../../../common/api/detection_engine/model/rule_schema';
import type { IPrebuiltRuleAssetsClient } from '../../../../prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client';
import { applyRuleDefaults } from './apply_rule_defaults';
import { calculateRuleSource } from './rule_source/calculate_rule_source';

interface ApplyRuleUpdateProps {
prebuiltRuleAssetClient: IPrebuiltRuleAssetsClient;
existingRule: RuleResponse;
ruleUpdate: RuleUpdateProps;
}

export const applyRuleUpdate = async ({
prebuiltRuleAssetClient,
existingRule,
ruleUpdate,
}: ApplyRuleUpdateProps): Promise<RuleResponse> => {
Expand All @@ -43,10 +39,5 @@ export const applyRuleUpdate = async ({
created_by: existingRule.created_by,
};

nextRule.rule_source = await calculateRuleSource({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this call is removed, applyRuleUpdate doesn't have to be async anymore.

rule: nextRule,
prebuiltRuleAssetClient,
});

return nextRule;
};
14 changes: 11 additions & 3 deletions .../lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { validateMlAuth, toggleRuleEnabledOnUpdate } from '../utils';
import { createRule } from './create_rule';
import { getRuleByRuleId } from './get_rule_by_rule_id';
import { createRuleImportErrorObject } from '../../import/errors';
import { calculateRuleSource } from '../mergers/rule_source/calculate_rule_source';

interface ImportRuleOptions {
actionsClient: ActionsClient;
Expand Down Expand Up @@ -57,12 +58,19 @@ export const importRule = async ({

if (existingRule && overwriteRules) {
let ruleWithUpdates = await applyRuleUpdate({
prebuiltRuleAssetClient,
existingRule,
ruleUpdate: rule,
});
// applyRuleUpdate prefers the existing rule's values for `rule_source` and `immutable`, but we want to use the importing rule's calculated values
ruleWithUpdates = { ...ruleWithUpdates, ...overrideFields };

// If no override fields are provided, we calculate the rule source
if (overrideFields == null) {
ruleWithUpdates.rule_source = await calculateRuleSource({
rule: ruleWithUpdates,
prebuiltRuleAssetClient,
});
} else {
ruleWithUpdates = { ...ruleWithUpdates, ...overrideFields };
}
Comment on lines +65 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change raises some concerns about safety that I think we should discuss.

  • Previously, updating a rule triggered a rule source recalculation, ensuring the rule source was always in sync with the rule content.
  • Now, in some cases, we delegate the rule source recalculation to client users. If the applyRuleUpdate method usage isn’t closely monitored, we might see inconsistencies, with some rule sources being updated correctly while others might not.

This change seems to trade off implementation correctness for performance improvements, which could introduce potential issues. The root cause appears to be the splitting of the rule import logic between two clients: RuleSourceImporter and DetectionRulesClient. My suggestion is to adjust the DetectionRulesClient's import method so the overrideFields escape hatch is no longer necessary, and ensure that rule source recalculations are fully handled within a single method.

While performance improvements are important, it might be worth waiting until the rule customization feature flag is enabled by default before considering optimizations. This would also allow us to remove the legacy import method support and combine the RuleSourceImporter and DetectionRulesClient. But untill that, without concrete evidence that the performance is being impacted, premature optimizations might introduce more complexity than benefits.

@banderror, would love to hear your thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, updating a rule triggered a rule source recalculation, ensuring the rule source was always in sync with the rule content.

This is only true because we happen to call applyRuleUpdate in those instances, right? From the perspective of the DetectionRulesClient, nothing has changed here: #importRule, #updateRule, and upgradeRule all follow the same logic as before, it's only the internal applyRuleUpdate whose responsibility has changed. Are you arguing that applyRuleUpdate needs to contain all of that logic?

Now, in some cases, we delegate the rule source recalculation to client users.

I see this as an optimization rather than an inconsistency: in the special case of importing rules, rule_source is calculated in bulk as it's much more efficient.

If the general argument is that these extraneous calculations are acceptable/negligible: this code path is only hit when you're overwriting existing rules, which was about 45% slower than creating new rules in my recent testing. So: an edge case, but it's slower, but not to the point where it's timing out (even at 4000 rules). 🤷


const updatedRule = await rulesClient.update({
id: existingRule.id,
Expand Down
6 changes: 5 additions & 1 deletion .../lib/detection_engine/rule_management/logic/detection_rules_client/methods/update_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { RulesClient } from '@kbn/alerting-plugin/server';
import type { RuleResponse } from '../../../../../../../common/api/detection_engine/model/rule_schema';
import type { MlAuthz } from '../../../../../machine_learning/authz';
import { applyRuleUpdate } from '../mergers/apply_rule_update';
import { calculateRuleSource } from '../mergers/rule_source/calculate_rule_source';
import { getIdError } from '../../../utils/utils';
import { validateNonCustomizableUpdateFields } from '../../../utils/validate';
import { convertRuleResponseToAlertingRule } from '../converters/convert_rule_response_to_alerting_rule';
Expand Down Expand Up @@ -54,10 +55,13 @@ export const updateRule = async ({
validateNonCustomizableUpdateFields(ruleUpdate, existingRule);

const ruleWithUpdates = await applyRuleUpdate({
prebuiltRuleAssetClient,
existingRule,
ruleUpdate,
});
ruleWithUpdates.rule_source = await calculateRuleSource({
rule: ruleWithUpdates,
prebuiltRuleAssetClient,
});

const updatedRule = await rulesClient.update({
id: existingRule.id,
Expand Down
6 changes: 5 additions & 1 deletion ...tion_engine/rule_management/logic/detection_rules_client/methods/upgrade_prebuilt_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type { IPrebuiltRuleAssetsClient } from '../../../../prebuilt_rules/logic
import { convertAlertingRuleToRuleResponse } from '../converters/convert_alerting_rule_to_rule_response';
import { convertRuleResponseToAlertingRule } from '../converters/convert_rule_response_to_alerting_rule';
import { applyRuleUpdate } from '../mergers/apply_rule_update';
import { calculateRuleSource } from '../mergers/rule_source/calculate_rule_source';
import { ClientError, validateMlAuth } from '../utils';
import { createRule } from './create_rule';
import { getRuleByRuleId } from './get_rule_by_rule_id';
Expand Down Expand Up @@ -70,10 +71,13 @@ export const upgradePrebuiltRule = async ({

// Else, recreate the rule from scratch with the passed payload.
const updatedRule = await applyRuleUpdate({
prebuiltRuleAssetClient,
existingRule,
ruleUpdate: ruleAsset,
});
updatedRule.rule_source = await calculateRuleSource({
rule: updatedRule,
prebuiltRuleAssetClient,
});

const updatedInternalRule = await rulesClient.update({
id: existingRule.id,
Expand Down