Skip to content

Commit

Permalink
[Security Solution] Adds normalization for query fields before diff…
Browse files Browse the repository at this point in the history
… algorithm comparison (elastic#203482)

## Summary

Fixes elastic#203151

Adds a normalization for the `kql_query`, `eql_query`, and `esql_query`
fields that trims the whitespace from the beginning and end of query
strings for a more robust comparison in the diff algorithms. Since
whitespace before or after the query string is purely a formatting
choice and doesn't impact the query itself, we discard the excess
whitespace characters before the direct string comparison.

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [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] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
  • Loading branch information
dplumlee authored Dec 13, 2024
1 parent abfd590 commit 0294838
Show file tree
Hide file tree
Showing 7 changed files with 224 additions and 4 deletions.
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

0 comments on commit 0294838

Please sign in to comment.