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] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule #191065

Merged
merged 1 commit into from
Aug 26, 2024
Merged

[Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule #191065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
234 changes: 87 additions & 147 deletions ...solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,25 @@ describe('duplicateRule', () => {
meta: undefined,
maxSignals: 100,
responseActions: [],
relatedIntegrations: [],
requiredFields: [],
relatedIntegrations: [
{
package: 'aws',
version: '~1.2.3',
integration: 'route53',
},
],
requiredFields: [
{
name: 'event.action',
type: 'keyword',
ecs: true,
},
],
riskScore: 42,
riskScoreMapping: [],
severity: 'low',
severityMapping: [],
setup: 'Some setup guide.',
setup: `## Config\n\nThe 'Audit Detailed File Share' audit policy must be configured...`,
threat: [],
to: 'now',
references: [],
Expand Down Expand Up @@ -94,152 +106,64 @@ describe('duplicateRule', () => {
jest.clearAllMocks();
});

it('returns an object with fields copied from a given rule', async () => {
const rule = createTestRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual({
name: expect.anything(), // covered in a separate test
params: {
...rule.params,
ruleSource: {
type: 'internal',
},
ruleId: expect.anything(), // covered in a separate test
},
tags: rule.tags,
alertTypeId: rule.alertTypeId,
consumer: rule.consumer,
schedule: rule.schedule,
actions: rule.actions,
systemActions: rule.actions,
enabled: false, // covered in a separate test
});
});

it('appends [Duplicate] to the name', async () => {
const rule = createTestRule();
rule.name = 'PowerShell Keylogging Script';
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
name: 'PowerShell Keylogging Script [Duplicate]',
})
);
});

it('generates a new ruleId', async () => {
const rule = createTestRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
ruleId: 'new ruleId',
}),
})
);
});

it('makes sure the duplicated rule is disabled', async () => {
const rule = createTestRule();
rule.enabled = true;
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
enabled: false,
})
);
});

describe('when duplicating a prebuilt (immutable) rule', () => {
const createPrebuiltRule = () => {
describe('when duplicating any kind of rule', () => {
it('appends [Duplicate] to the name', async () => {
const rule = createTestRule();
rule.params.immutable = true;
return rule;
};

it('transforms it to a custom (mutable) rule', async () => {
const rule = createPrebuiltRule();
rule.name = 'PowerShell Keylogging Script';
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
immutable: false,
}),
name: 'PowerShell Keylogging Script [Duplicate]',
})
);
});

it('resets related integrations to an empty array', async () => {
const rule = createPrebuiltRule();
rule.params.relatedIntegrations = [
{
package: 'aws',
version: '~1.2.3',
integration: 'route53',
},
];

it('generates a new ruleId', async () => {
const rule = createTestRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
relatedIntegrations: [],
ruleId: 'new ruleId',
}),
})
);
});

it('resets required fields to an empty array', async () => {
const rule = createPrebuiltRule();
rule.params.requiredFields = [
{
name: 'event.action',
type: 'keyword',
ecs: true,
},
];

it('makes sure the duplicated rule is disabled', async () => {
const rule = createTestRule();
rule.enabled = true;
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
requiredFields: [],
}),
enabled: false,
})
);
});
});

describe('when duplicating a custom (mutable) rule', () => {
const createCustomRule = () => {
describe('when duplicating a prebuilt rule', () => {
const createPrebuiltRule = () => {
const rule = createTestRule();
rule.params.immutable = false;
rule.params.immutable = true;
rule.params.ruleSource = {
type: 'external',
isCustomized: false,
};
return rule;
};

it('keeps it custom', async () => {
const rule = createCustomRule();
it('transforms it to a custom rule', async () => {
const rule = createPrebuiltRule();
const result = await duplicateRule({
rule,
});
Expand All @@ -248,71 +172,87 @@ describe('duplicateRule', () => {
expect.objectContaining({
params: expect.objectContaining({
immutable: false,
ruleSource: {
type: 'internal',
},
}),
})
);
});

it('copies related integrations as is', async () => {
const rule = createCustomRule();
rule.params.relatedIntegrations = [
{
package: 'aws',
version: '~1.2.3',
integration: 'route53',
},
];

it('copies fields from the original rule', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The same as before should it be renamed copies fields from the original rule -> copies all fields from the original rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#191065 (comment)

const rule = createPrebuiltRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
relatedIntegrations: rule.params.relatedIntegrations,
}),
})
);
expect(result).toEqual({
name: expect.anything(), // covered in a separate test
params: {
...rule.params,
ruleId: expect.anything(), // covered in a separate test
immutable: expect.anything(), // covered in a separate test
ruleSource: expect.anything(), // covered in a separate test
},
tags: rule.tags,
alertTypeId: rule.alertTypeId,
consumer: rule.consumer,
schedule: rule.schedule,
actions: rule.actions,
systemActions: rule.actions,
enabled: false, // covered in a separate test
});
});
});

it('copies required fields as is', async () => {
const rule = createCustomRule();
rule.params.requiredFields = [
{
name: 'event.action',
type: 'keyword',
ecs: true,
},
];
describe('when duplicating a custom rule', () => {
const createCustomRule = () => {
const rule = createTestRule();
rule.params.immutable = false;
rule.params.ruleSource = {
type: 'internal',
};
return rule;
};

it('keeps it custom', async () => {
const rule = createCustomRule();
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It should be simpler to assert result.params directly

Suggested change
expect(result).toEqual(
expect(result.params).toEqual(
expect.objectContaining({
requiredFields: rule.params.requiredFields,
immutable: false,
ruleSource: {
type: 'internal',
},
}),
})
);

expect.objectContaining({
params: expect.objectContaining({
requiredFields: rule.params.requiredFields,
immutable: false,
ruleSource: {
type: 'internal',
},
}),
})
);
});

it('copies setup guide as is', async () => {
it('copies fields from the original rule', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I get it correctly that we wanna test for coping of ALL fields in that unit test? And it's a desired behavior to have this test failed when a new field got added for example by Response Ops team?

WDYT about changing the test name to copies all fields from the original rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I get it correctly that we wanna test for coping of ALL fields in that unit test?

@maximpn Strictly speaking, we don't copy all rule fields. We copy all parameters + some of the fields that Alerting Framework owns. Some fields get adjusted (e.g. name), some fields get regenerated. I don't think it would be correct to rename the test to copies all fields from the original rule.

And it's a desired behavior to have this test failed when a new field got added for example by Response Ops team?

Yes, because if they add a new top-level framework's field we need to make sure it is correctly handled by the rule duplication logic: copied, adjusted, or regenerated.

const rule = createCustomRule();
rule.params.setup = `## Config\n\nThe 'Audit Detailed File Share' audit policy must be configured...`;
const result = await duplicateRule({
rule,
});

expect(result).toEqual(
expect.objectContaining({
params: expect.objectContaining({
setup: rule.params.setup,
}),
})
);
expect(result).toEqual({
name: expect.anything(), // covered in a separate test
params: {
...rule.params,
ruleId: expect.anything(), // covered in a separate test
},
tags: rule.tags,
alertTypeId: rule.alertTypeId,
consumer: rule.consumer,
schedule: rule.schedule,
actions: rule.actions,
systemActions: rule.actions,
enabled: false, // covered in a separate test
});
});
});
});
28 changes: 12 additions & 16 deletions ...rity_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,18 @@ interface DuplicateRuleParams {

export const duplicateRule = async ({ rule }: DuplicateRuleParams): Promise<InternalRuleCreate> => {
// Generate a new static ruleId
const ruleId = uuidv4();

// If it's a prebuilt rule, reset Related Integrations, Required Fields and Setup Guide.
// We do this because for now we don't allow the users to edit these fields for custom rules.
const isPrebuilt = rule.params.immutable;
const relatedIntegrations = isPrebuilt ? [] : rule.params.relatedIntegrations;
const requiredFields = isPrebuilt ? [] : rule.params.requiredFields;

const actions = transformToActionFrequency(rule.actions, rule.throttle);
const ruleId: InternalRuleCreate['params']['ruleId'] = uuidv4();

// Duplicated rules are always considered custom rules
const immutable = false;
const immutable: InternalRuleCreate['params']['immutable'] = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what's the point to explicitly specify types here? Usually TS is able to infer types pretty accurately (especially such simple ones) and check wether these match return type (since it's specified for the function this also the reason why it's better to have function's return type). If something is wrong TS will complain on lines 51-52.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maximpn There are sometimes instances when TS allows to return an object from a function if the object fulfills a certain type or interface, but has additional fields as well that are not part of this type. I've seen it multiple times but I can't give you a concrete example immediately.

Here maybe you're right that these type annotations are not necessary, but I figured I'd still add them because here we return a complex rule object from the function and it's probably better if we add extra type annotations. It shouldn't hurt, anyway.

const ruleSource: InternalRuleCreate['params']['ruleSource'] = {
type: 'internal',
};

const actions: InternalRuleCreate['actions'] = transformToActionFrequency(
rule.actions,
rule.throttle
);

return {
name: `${rule.name} [${DUPLICATE_TITLE}]`,
Expand All @@ -47,13 +47,9 @@ export const duplicateRule = async ({ rule }: DuplicateRuleParams): Promise<Inte
consumer: SERVER_APP_ID,
params: {
...rule.params,
immutable,
ruleSource: {
type: 'internal',
},
ruleId,
relatedIntegrations,
requiredFields,
immutable,
ruleSource,
exceptionsList: [],
},
schedule: rule.schedule,
Expand Down