Skip to content

Commit c352ab6

Browse files
marshallmainkibanamachine
authored andcommitted
[Security Solution][Rules Management] Separate actions import logic from rules import (elastic#216380)
## Summary Redo of elastic#193471 Closes elastic/security-team#8644 > Fixes a bug where importing a rule fails with a connector into a space where (1) the connector already exists, and (2) the existing connector was exported and re-imported from another space. The import logic in this scenario effectively tries to convert the action ID on the rule import twice. The second conversion attempt tries to use the old action ID to look up the correct new action ID in a map, however, in this test scenario the action ID has already been updated by legacy SO ID migration logic and there is no map entry with the new ID as a key. The result is that the second attempt sets the action ID to undefined, resulting in an import failure. The root cause of the bug is that we have two different places in the rule import logic where action IDs are migrated. The first ID migration was done by `migrateLegacyActionsIds` prior to importing rule actions, and the second migration was done by `importRuleActionConnectors` after importing the actions. `importRuleActionConnectors` used a lookup table to convert old IDs to new IDs, but if the connector already existed and had an `originId` then the rule action would already be migrated by `migrateLegacyActionsIds`. The lookup table used by `importRuleActionConnectors` does not have entries for migrated IDs, only the original IDs, so in that case the result of the lookup is `undefined` which we assign to the action ID. This PR reworks the logic to create a clean separation between action and rule import. We now import the connectors first, ignoring the rules, then migrate action IDs on the rules afterwards. This handles connectors changing IDs in any way, either through the 7.x->8.0 migration long ago or IDs changing on import if there are ID conflicts. Only after the connectors are imported and rule actions are migrated do we then verify if each rule action references a connector ID that actually exists with the new `checkRuleActions` function, replacing `checkIfActionsHaveMissingConnectors` and related functions that were also buggy. Finally, as a nice side effect this rework removes "rule action connector missing" errors out of the `action_connector_errors` part of the response. `action_connector_errors` is reserved for errors importing connectors specifically. If a rule action is missing a connector and therefore we don't import the rule, that's a rule error and it's represented in the `errors` part of the response. Since the shape of the response is not changing, I don't consider this a breaking change but rather a bug fix. ## Repro Steps Repro Steps 1. Download the export file below and change the extension back to .ndjson from .json (github does not allow .ndjson files [rules_export.json](https://github.com/user-attachments/files/17065272/rules_export.json) 2. Import the rule and connector into a space (default is fine) 3. Create a new space 4. Import the rule and connector into the new space 5. Import the rule and connector into the new space again, but check the `Overwrite existing connectors with conflicting action "id"` box. Observe the failure. --------- Co-authored-by: kibanamachine <[email protected]>
1 parent d8e950e commit c352ab6

File tree

13 files changed

+335
-1873
lines changed

13 files changed

+335
-1873
lines changed

x-pack/solutions/security/plugins/security_solution/server/lib/detection_engine/routes/utils.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ export const createBulkErrorObject = ({
7272
};
7373
} else {
7474
return {
75-
rule_id: '(unknown id)',
7675
error: {
7776
status_code: statusCode,
7877
message,

x-pack/solutions/security/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,6 @@ describe.skip('Import rules route', () => {
202202
message: `Unexpected token 'h', "this is not"... is not valid JSON`,
203203
status_code: 400,
204204
},
205-
rule_id: '(unknown id)',
206205
},
207206
],
208207
success: false,
@@ -350,14 +349,12 @@ describe.skip('Import rules route', () => {
350349
message: 'rule_id: Required',
351350
status_code: 400,
352351
},
353-
rule_id: '(unknown id)',
354352
},
355353
{
356354
error: {
357355
message: 'rule_id: Required',
358356
status_code: 400,
359357
},
360-
rule_id: '(unknown id)',
361358
},
362359
],
363360
success: false,

x-pack/solutions/security/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.ts

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {
2626
} from '../../../../routes/utils';
2727
import { createPrebuiltRuleAssetsClient } from '../../../../prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client';
2828
import { importRuleActionConnectors } from '../../../logic/import/action_connectors/import_rule_action_connectors';
29+
import { validateRuleActions } from '../../../logic/import/action_connectors/validate_rule_actions';
2930
import { createRuleSourceImporter } from '../../../logic/import/rule_source_importer';
3031
import { importRules } from '../../../logic/import/import_rules';
3132

@@ -123,13 +124,9 @@ export const importRulesRoute = (router: SecuritySolutionPluginRouter, config: C
123124
maxExceptionsImportSize: objectLimit,
124125
});
125126
// report on duplicate rules
126-
const [duplicateIdErrors, parsedObjectsWithoutDuplicateErrors] =
127-
getTupleDuplicateErrorsAndUniqueRules(rules, request.query.overwrite);
128-
129-
const migratedParsedObjectsWithoutDuplicateErrors = await migrateLegacyActionsIds(
130-
parsedObjectsWithoutDuplicateErrors,
131-
actionSOClient,
132-
actionsClient
127+
const [duplicateIdErrors, rulesToImportOrErrors] = getTupleDuplicateErrorsAndUniqueRules(
128+
rules,
129+
request.query.overwrite
133130
);
134131

135132
// import actions-connectors
@@ -138,20 +135,17 @@ export const importRulesRoute = (router: SecuritySolutionPluginRouter, config: C
138135
success: actionConnectorSuccess,
139136
warnings: actionConnectorWarnings,
140137
errors: actionConnectorErrors,
141-
rulesWithMigratedActions,
142138
} = await importRuleActionConnectors({
143139
actionConnectors,
144-
actionsClient,
145140
actionsImporter,
146-
rules: migratedParsedObjectsWithoutDuplicateErrors,
147141
overwrite: request.query.overwrite_action_connectors,
148142
});
149143

150-
// rulesWithMigratedActions: Is returned only in case connectors were exported from different namespace and the
151-
// original rules actions' ids were replaced with new destinationIds
152-
const parsedRuleStream = actionConnectorErrors.length
153-
? []
154-
: rulesWithMigratedActions || migratedParsedObjectsWithoutDuplicateErrors;
144+
const migratedRulesToImportOrErrors = await migrateLegacyActionsIds(
145+
rulesToImportOrErrors,
146+
actionSOClient,
147+
actionsClient
148+
);
155149

156150
const ruleSourceImporter = createRuleSourceImporter({
157151
config,
@@ -160,8 +154,20 @@ export const importRulesRoute = (router: SecuritySolutionPluginRouter, config: C
160154
prebuiltRuleObjectsClient: createPrebuiltRuleObjectsClient(rulesClient),
161155
});
162156

163-
const [parsedRules, parsedRuleErrors] = partition(isRuleToImport, parsedRuleStream);
164-
const ruleChunks = chunk(CHUNK_PARSED_OBJECT_SIZE, parsedRules);
157+
const [parsedRules, parsedRuleErrors] = partition(
158+
isRuleToImport,
159+
migratedRulesToImportOrErrors
160+
);
161+
162+
// After importing the actions and migrating action IDs on rules to import,
163+
// validate that all actions referenced by rules exist
164+
// Filter out rules that reference non-existent actions
165+
const { validatedActionRules, missingActionErrors } = await validateRuleActions({
166+
actionsClient,
167+
rules: parsedRules,
168+
});
169+
170+
const ruleChunks = chunk(CHUNK_PARSED_OBJECT_SIZE, validatedActionRules);
165171

166172
const importRuleResponse = await importRules({
167173
ruleChunks,
@@ -180,9 +186,9 @@ export const importRulesRoute = (router: SecuritySolutionPluginRouter, config: C
180186
const importErrors = importRuleResponse.filter(isBulkError);
181187
const errors = [
182188
...parseErrors,
183-
...actionConnectorErrors,
184189
...duplicateIdErrors,
185190
...importErrors,
191+
...missingActionErrors,
186192
];
187193

188194
const successes = importRuleResponse.filter((resp) => {

0 commit comments

Comments
 (0)