Skip to content

Commit

Permalink
[Security Solution] Fix not complete existing rule overwrite when imp…
Browse files Browse the repository at this point in the history 
…orting rules (elastic#176166)

**Fixes: elastic#93342
**Fixes: elastic#118166

## Summary

This PR fixes not complete existing rule overwrite when importing rules.

## Details

When importing a rule and attempting to overwrite an existing rule, if the new rule does not define a field that the existing rule did define then the newly imported rule will include the field from the existing rule. This can cause issues if we want to overwrite a rule with a rule of a different type, e.g. going from saved_query to query we would provide a new rule that doesn't have a saved_id but since saved_id was defined on the old saved_query rule it will be included in the new query rule.

The fix simply swaps out the `patchRules()` for `updateRules()`. Patching rules preserves previous field values if an incoming update doesn't have such fields while updating doesn't do that. The diff in `import_rules_utils.test.ts` looks bigger due to removing unnecessary `else` clause.

### Checklist

- [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] Ran successfully in Flaky test runner ([basic/essentials license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5166) and [trial/complete tier license FTR tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5167))
  • Loading branch information
@maximpn @fkanout
maximpn authored and fkanout committed Mar 4, 2024
1 parent 1d0fa6c commit 9207d71
Show file tree
Hide file tree
Showing 19 changed files with 1,757 additions and 1,617 deletions.
3 changes: 3 additions & 0 deletions .../security_solution/server/lib/detection_engine/rule_management/logic/crud/update_rules.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ export interface UpdateRulesOptions {
rulesClient: RulesClient;
existingRule: RuleAlertType | null | undefined;
ruleUpdate: RuleUpdateProps;
allowMissingConnectorSecrets?: boolean;
}

export const updateRules = async ({
rulesClient,
existingRule,
ruleUpdate,
allowMissingConnectorSecrets,
}: UpdateRulesOptions): Promise<PartialRule<RuleParams> | null> => {
if (existingRule == null) {
return null;
Expand Down Expand Up @@ -81,6 +83,7 @@ export const updateRules = async ({
const update = await rulesClient.update({
id: existingRule.id,
data: newInternalRule,
allowMissingConnectorSecrets,
});

if (existingRule.enabled && enabled === false) {
Expand Down
67 changes: 57 additions & 10 deletions ...ution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@ import {
getRuleMock,
getEmptyFindResult,
getFindResultWithSingleHit,
getFindResultWithMultiHits,
} from '../../../routes/__mocks__/request_responses';

import { createRules } from '../crud/create_rules';
import { patchRules } from '../crud/patch_rules';
import { updateRules } from '../crud/update_rules';
import { importRules } from './import_rules_utils';

jest.mock('../crud/create_rules');
jest.mock('../crud/patch_rules');
jest.mock('../crud/update_rules');

describe('importRules', () => {
const mlAuthz = {
Expand Down Expand Up @@ -84,7 +85,7 @@ describe('importRules', () => {

expect(result).toEqual([{ rule_id: 'rule-1', status_code: 200 }]);
expect(createRules).toHaveBeenCalled();
expect(patchRules).not.toHaveBeenCalled();
expect(updateRules).not.toHaveBeenCalled();
});

it('reports error if "overwriteRules" is "false" and matching rule found', async () => {
Expand All @@ -106,10 +107,10 @@ describe('importRules', () => {
},
]);
expect(createRules).not.toHaveBeenCalled();
expect(patchRules).not.toHaveBeenCalled();
expect(updateRules).not.toHaveBeenCalled();
});

it('patches rule if "overwriteRules" is "true" and matching rule found', async () => {
it('updates rule if "overwriteRules" is "true" and matching rule found', async () => {
clients.rulesClient.find.mockResolvedValue(getFindResultWithSingleHit());

const result = await importRules({
Expand All @@ -129,7 +130,53 @@ describe('importRules', () => {

expect(result).toEqual([{ rule_id: 'rule-1', status_code: 200 }]);
expect(createRules).not.toHaveBeenCalled();
expect(patchRules).toHaveBeenCalled();
expect(updateRules).toHaveBeenCalled();
});

/**
* Existing rule may have nullable fields set to a value (e.g. `timestamp_override` is set to `some.value`) but
* a rule to import doesn't have these fields set (e.g. `timestamp_override` is NOT present at all in the ndjson file).
* We expect the updated rule won't have such fields preserved (e.g. `timestamp_override` will be removed).
*
* Unit test is only able to check `updateRules()` receives a proper update object.
*/
it('ensures overwritten rule DOES NOT preserve fields missed in the imported rule when "overwriteRules" is "true" and matching rule found', async () => {
const existingRule = getRuleMock(
getQueryRuleParams({
timestampOverride: 'some.value',
})
);

clients.rulesClient.find.mockResolvedValue(
getFindResultWithMultiHits({ data: [existingRule] })
);

const result = await importRules({
ruleChunks: [
[
{
...getImportRulesSchemaMock(),
rule_id: 'rule-1',
},
],
],
rulesResponseAcc: [],
mlAuthz,
overwriteRules: true,
rulesClient: context.alerting.getRulesClient(),
existingLists: {},
});

expect(result).toEqual([{ rule_id: 'rule-1', status_code: 200 }]);
expect(createRules).not.toHaveBeenCalled();
expect(updateRules).toHaveBeenCalledWith(
expect.objectContaining({
ruleUpdate: expect.not.objectContaining({
timestamp_override: expect.anything(),
timestampOverride: expect.anything(),
}),
})
);
});

it('reports error if rulesClient throws', async () => {
Expand All @@ -154,7 +201,7 @@ describe('importRules', () => {
},
]);
expect(createRules).not.toHaveBeenCalled();
expect(patchRules).not.toHaveBeenCalled();
expect(updateRules).not.toHaveBeenCalled();
});

it('reports error if "createRules" throws', async () => {
Expand All @@ -180,8 +227,8 @@ describe('importRules', () => {
]);
});

it('reports error if "patchRules" throws', async () => {
(patchRules as jest.Mock).mockRejectedValue(new Error('error patching rule'));
it('reports error if "updateRules" throws', async () => {
(updateRules as jest.Mock).mockRejectedValue(new Error('import rule error'));
clients.rulesClient.find.mockResolvedValue(getFindResultWithSingleHit());

const result = await importRules({
Expand All @@ -196,7 +243,7 @@ describe('importRules', () => {
expect(result).toEqual([
{
error: {
message: 'error patching rule',
message: 'import rule error',
status_code: 400,
},
rule_id: 'rule-1',
Expand Down
156 changes: 77 additions & 79 deletions ...y_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import type { ImportRuleResponse } from '../../../routes/utils';
import { createBulkErrorObject } from '../../../routes/utils';
import { createRules } from '../crud/create_rules';
import { readRules } from '../crud/read_rules';
import { patchRules } from '../crud/patch_rules';
import { updateRules } from '../crud/update_rules';
import type { MlAuthz } from '../../../../machine_learning/authz';
import { throwAuthzError } from '../../../../machine_learning/validation';
import { checkRuleExceptionReferences } from './check_rule_exception_references';
Expand Down Expand Up @@ -68,96 +68,94 @@ export const importRules = async ({
// otherwise we would output we are success importing 0 rules.
if (ruleChunks.length === 0) {
return importRuleResponse;
} else {
while (ruleChunks.length) {
const batchParseObjects = ruleChunks.shift() ?? [];
const newImportRuleResponse = await Promise.all(
batchParseObjects.reduce<Array<Promise<ImportRuleResponse>>>((accum, parsedRule) => {
const importsWorkerPromise = new Promise<ImportRuleResponse>(async (resolve, reject) => {
}

while (ruleChunks.length) {
const batchParseObjects = ruleChunks.shift() ?? [];
const newImportRuleResponse = await Promise.all(
batchParseObjects.reduce<Array<Promise<ImportRuleResponse>>>((accum, parsedRule) => {
const importsWorkerPromise = new Promise<ImportRuleResponse>(async (resolve, reject) => {
try {
if (parsedRule instanceof Error) {
// If the JSON object had a validation or parse error then we return
// early with the error and an (unknown) for the ruleId
resolve(
createBulkErrorObject({
statusCode: 400,
message: parsedRule.message,
})
);
return null;
}

try {
if (parsedRule instanceof Error) {
// If the JSON object had a validation or parse error then we return
// early with the error and an (unknown) for the ruleId
resolve(
createBulkErrorObject({
statusCode: 400,
message: parsedRule.message,
})
);
return null;
}
const [exceptionErrors, exceptions] = checkRuleExceptionReferences({
rule: parsedRule,
existingLists,
});

try {
const [exceptionErrors, exceptions] = checkRuleExceptionReferences({
rule: parsedRule,
existingLists,
});
importRuleResponse = [...importRuleResponse, ...exceptionErrors];

importRuleResponse = [...importRuleResponse, ...exceptionErrors];
throwAuthzError(await mlAuthz.validateRuleType(parsedRule.type));
const rule = await readRules({
rulesClient,
ruleId: parsedRule.rule_id,
id: undefined,
});

throwAuthzError(await mlAuthz.validateRuleType(parsedRule.type));
const rule = await readRules({
if (rule == null) {
await createRules({
rulesClient,
ruleId: parsedRule.rule_id,
id: undefined,
params: {
...parsedRule,
exceptions_list: [...exceptions],
},
allowMissingConnectorSecrets,
});

if (rule == null) {
await createRules({
rulesClient,
params: {
...parsedRule,
exceptions_list: [...exceptions],
},
allowMissingConnectorSecrets,
});
resolve({
rule_id: parsedRule.rule_id,
status_code: 200,
});
} else if (rule != null && overwriteRules) {
await patchRules({
rulesClient,
existingRule: rule,
nextParams: {
...parsedRule,
exceptions_list: [...exceptions],
},
allowMissingConnectorSecrets,
shouldIncrementRevision: false,
});
resolve({
rule_id: parsedRule.rule_id,
status_code: 200,
});
} else if (rule != null) {
resolve(
createBulkErrorObject({
ruleId: parsedRule.rule_id,
statusCode: 409,
message: `rule_id: "${parsedRule.rule_id}" already exists`,
})
);
}
} catch (err) {
resolve({
rule_id: parsedRule.rule_id,
status_code: 200,
});
} else if (rule != null && overwriteRules) {
await updateRules({
rulesClient,
existingRule: rule,
ruleUpdate: {
...parsedRule,
exceptions_list: [...exceptions],
},
});
resolve({
rule_id: parsedRule.rule_id,
status_code: 200,
});
} else if (rule != null) {
resolve(
createBulkErrorObject({
ruleId: parsedRule.rule_id,
statusCode: err.statusCode ?? 400,
message: err.message,
statusCode: 409,
message: `rule_id: "${parsedRule.rule_id}" already exists`,
})
);
}
} catch (error) {
reject(error);
} catch (err) {
resolve(
createBulkErrorObject({
ruleId: parsedRule.rule_id,
statusCode: err.statusCode ?? 400,
message: err.message,
})
);
}
});
return [...accum, importsWorkerPromise];
}, [])
);
importRuleResponse = [...importRuleResponse, ...newImportRuleResponse];
}

return importRuleResponse;
} catch (error) {
reject(error);
}
});
return [...accum, importsWorkerPromise];
}, [])
);
importRuleResponse = [...importRuleResponse, ...newImportRuleResponse];
}

return importRuleResponse;
};
Loading

0 comments on commit 9207d71

Please sign in to comment.