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.

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] Adds normalization for query fields before diff algorithm comparison #203482

Merged
merged 5 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ const extractDiffableCommonFields = (
version: rule.version,

// Main domain fields
name: rule.name,
name: rule.name.trim(),
tags: rule.tags ?? [],
description: rule.description,
severity: rule.severity,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { KqlQueryType } from '../../../api/detection_engine';
import {
extractRuleEqlQuery,
extractRuleEsqlQuery,
extractRuleKqlQuery,
} from './extract_rule_data_query';

describe('extract rule data queries', () => {
describe('extractRuleKqlQuery', () => {
it('extracts a trimmed version of the query field for inline query types', () => {
const extractedKqlQuery = extractRuleKqlQuery('\nevent.kind:alert\n', 'kuery', [], undefined);

expect(extractedKqlQuery).toEqual({
type: KqlQueryType.inline_query,
query: 'event.kind:alert',
language: 'kuery',
filters: [],
});
});
});

describe('extractRuleEqlQuery', () => {
it('extracts a trimmed version of the query field', () => {
const extractedEqlQuery = extractRuleEqlQuery({
query: '\n\nquery where true\n\n',
language: 'eql',
filters: [],
eventCategoryOverride: undefined,
timestampField: undefined,
tiebreakerField: undefined,
});

expect(extractedEqlQuery).toEqual({
query: 'query where true',
language: 'eql',
filters: [],
event_category_override: undefined,
timestamp_field: undefined,
tiebreaker_field: undefined,
});
});
});

describe('extractRuleEsqlQuery', () => {
it('extracts a trimmed version of the query field', () => {
const extractedEsqlQuery = extractRuleEsqlQuery('\nFROM * where true\t\n', 'esql');

expect(extractedEsqlQuery).toEqual({
query: 'FROM * where true',
language: 'esql',
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export const extractInlineKqlQuery = (
): InlineKqlQuery => {
return {
type: KqlQueryType.inline_query,
query: query ?? '',
query: query?.trim() ?? '',
language: language ?? 'kuery',
filters: filters ?? [],
};
Expand All @@ -63,7 +63,7 @@ interface ExtractRuleEqlQueryParams {

export const extractRuleEqlQuery = (params: ExtractRuleEqlQueryParams): RuleEqlQuery => {
return {
query: params.query,
query: params.query.trim(),
language: params.language,
filters: params.filters ?? [],
event_category_override: params.eventCategoryOverride,
Expand All @@ -77,7 +77,7 @@ export const extractRuleEsqlQuery = (
language: EsqlQueryLanguage
): RuleEsqlQuery => {
return {
query,
query: query.trim(),
language,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,46 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});

it('should trim all whitespace before version comparison', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(es, getRuleAssetSavedObjects());
await installPrebuiltRules(es, supertest);

// Customize an eql_query field on the installed rule
await updateRule(supertest, {
...getPrebuiltRuleMock(),
rule_id: 'rule-1',
type: 'eql',
query: '\nquery where true\n',
language: 'eql',
filters: [],
} as RuleUpdateProps);

// Add a v2 rule asset to make the upgrade possible, do NOT update the related eql_query field, and create the new rule assets
const updatedRuleAssetSavedObjects = [
createRuleAssetSavedObject({
rule_id: 'rule-1',
version: 2,
type: 'eql',
query: '\nquery where true',
language: 'eql',
filters: [],
}),
];
await createHistoricalPrebuiltRuleAssetSavedObjects(es, updatedRuleAssetSavedObjects);

// Call the upgrade review prebuilt rules endpoint and check that there is 1 rule eligible for update but eql_query field is NOT returned
const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest);
const fieldDiffObject = reviewResponse.rules[0].diff.fields as AllFieldsDiff;
expect(fieldDiffObject.eql_query).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); // `version` is considered an updated field
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});

describe("when rule field doesn't have an update but has a custom value - scenario ABA", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,44 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});

it('should trim all whitespace before version comparison', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(es, getRuleAssetSavedObjects());
await installPrebuiltRules(es, supertest);

// Customize an esql_query field on the installed rule
await updateRule(supertest, {
...getPrebuiltRuleMock(),
rule_id: 'rule-1',
type: 'esql',
query: '\tFROM query WHERE true\t',
language: 'esql',
} as RuleUpdateProps);

// Add a v2 rule asset to make the upgrade possible, do NOT update the related esql_query field, and create the new rule assets
const updatedRuleAssetSavedObjects = [
createRuleAssetSavedObject({
rule_id: 'rule-1',
version: 2,
type: 'esql',
query: '\n\nFROM query WHERE true\n\n',
language: 'esql',
}),
];
await createHistoricalPrebuiltRuleAssetSavedObjects(es, updatedRuleAssetSavedObjects);

// Call the upgrade review prebuilt rules endpoint and check that there is 1 rule eligible for update but esql_query field is NOT returned
const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest);
const fieldDiffObject = reviewResponse.rules[0].diff.fields as AllFieldsDiff;
expect(fieldDiffObject.esql_query).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); // `version` is considered an updated field
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});

describe("when rule field doesn't have an update but has a custom value - scenario ABA", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,52 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});

describe('when all query versions have different surrounding whitespace', () => {
it('should not show in the upgrade/_review API response', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(
es,
getQueryRuleAssetSavedObjects()
);
await installPrebuiltRules(es, supertest);

// Customize a kql_query field on the installed rule
await updateRule(supertest, {
...getPrebuiltRuleMock(),
rule_id: 'rule-1',
type: 'query',
query: '\nquery string = true',
language: 'kuery',
filters: [],
saved_id: undefined,
} as RuleUpdateProps);

// Add a v2 rule asset to make the upgrade possible, do NOT update the related kql_query field, and create the new rule assets
const updatedRuleAssetSavedObjects = [
createRuleAssetSavedObject({
rule_id: 'rule-1',
version: 2,
type: 'query',
query: 'query string = true\n',
language: 'kuery',
filters: [],
}),
];
await createHistoricalPrebuiltRuleAssetSavedObjects(es, updatedRuleAssetSavedObjects);

// Call the upgrade review prebuilt rules endpoint and check that there is 1 rule eligible for update but kql_query field is NOT returned
const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest);
const fieldDiffObject = reviewResponse.rules[0].diff.fields as AllFieldsDiff;
expect(fieldDiffObject.kql_query).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); // `version` is considered an updated field
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
});

describe("when rule field doesn't have an update but has a custom value - scenario ABA", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,41 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});

it('should trim all whitespace before version comparison', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(es, getRuleAssetSavedObjects());
await installPrebuiltRules(es, supertest);

// Customize a single line string field on the installed rule
await patchRule(supertest, log, {
rule_id: 'rule-1',
name: 'A\n',
});

// Increment the version of the installed rule, do NOT update the related single line string field, and create the new rule assets
const updatedRuleAssetSavedObjects = [
createRuleAssetSavedObject({
rule_id: 'rule-1',
name: '\nA',
version: 2,
}),
];
await createHistoricalPrebuiltRuleAssetSavedObjects(es, updatedRuleAssetSavedObjects);

// Call the upgrade review prebuilt rules endpoint and check that there is 1 rule eligible for update
// but single line string field (name) is NOT returned
const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest);
expect(reviewResponse.rules[0].diff.fields.name).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});

describe("when rule field doesn't have an update but has a custom value - scenario ABA", () => {
Expand Down
Loading