mirror of
https://github.com/elastic/kibana.git
synced 2025-04-24 01:38:56 -04:00
[Security Solution] Normalizes filters
field before rule diff comparison (#206344)
**Fixes: https://github.com/elastic/kibana/issues/202966** **Fixes: https://github.com/elastic/kibana/issues/206527** ## Summary The issue that causes the overarching problem mentioned in the ticket is that we add an extra `alias: null` property to the filter via the kibana filter utils instead of keeping the `alias` field unset. This is functionally the same rule but since the prebuilt rule objects are technically different (`alias` is set to `undefined` instead of `null`), we mark these rules as customized and causes the query fields to show as a modified field on update. To address this, since changing the kibana util filter would be very invasive and touching a lot of code, we instead normalize the field on our side before version comparison. This fixes the bug reported and improves resiliency of rule upgrades in the future. ### Testing (copied from ticket) - Ensure the `prebuiltRulesCustomizationEnabled` feature flag is enabled - Allow internal APIs via adding `server.restrictInternalApis: false` to `kibana.dev.yaml` - Clear Elasticsearch data - Run Elasticsearch and Kibana locally (do not open Kibana in a web browser) - Install an outdated version of the `security_detection_engine` Fleet package ```bash curl -X POST --user elastic:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: 123' -H "elastic-api-version: 2023-10-31" -d '{"force":true}' http://localhost:5601/kbn/api/fleet/epm/packages/security_detection_engine/8.14.1 ``` - Install prebuilt rules ```bash curl -X POST --user elastic:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: 123' -H "elastic-api-version: 1" -d '{"mode":"ALL_RULES"}' http://localhost:5601/kbn/internal/detection_engine/prebuilt_rules/installation/_perform ``` - Open a `threat_match` rule for editing. For example `Threat Intel Hash Indicator Match` with rule_id `aab184d3-72b3-4639-b242-6597c99d8bca`. With this fix, users should **NOT** see any extra fields in the rule upgrade flyout, nor should the rule be marked as "Modified" if opened and saved with no other modifications ### 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 --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
This commit is contained in:
parent
d9d6f52303
commit
a5b4570cd8
6 changed files with 182 additions and 6 deletions
|
@ -75,7 +75,7 @@ export function mapFilter(filter: Filter) {
|
|||
mappedFilter.meta.params = mapped.params;
|
||||
mappedFilter.meta.disabled = Boolean(mappedFilter.meta.disabled);
|
||||
mappedFilter.meta.negate = Boolean(mappedFilter.meta.negate);
|
||||
mappedFilter.meta.alias = mappedFilter.meta.alias || null;
|
||||
mappedFilter.meta.alias = mappedFilter.meta.alias;
|
||||
|
||||
return mappedFilter;
|
||||
}
|
||||
|
|
|
@ -44,7 +44,7 @@ describe('filter manager utilities', () => {
|
|||
"params": Array [
|
||||
Object {
|
||||
"meta": Object {
|
||||
"alias": null,
|
||||
"alias": undefined,
|
||||
"disabled": false,
|
||||
"index": "logstash-*",
|
||||
"key": "bytes",
|
||||
|
|
|
@ -5,6 +5,7 @@
|
|||
* 2.0.
|
||||
*/
|
||||
|
||||
import type { Filter } from '@kbn/es-query';
|
||||
import { KqlQueryType } from '../../../api/detection_engine';
|
||||
import {
|
||||
extractRuleEqlQuery,
|
||||
|
@ -12,6 +13,24 @@ import {
|
|||
extractRuleKqlQuery,
|
||||
} from './extract_rule_data_query';
|
||||
|
||||
const mockFilter: Filter = {
|
||||
meta: {
|
||||
alias: null,
|
||||
negate: false,
|
||||
disabled: false,
|
||||
type: 'phrase',
|
||||
key: 'test',
|
||||
params: {
|
||||
query: 'value',
|
||||
},
|
||||
},
|
||||
query: {
|
||||
term: {
|
||||
field: 'value',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
describe('extract rule data queries', () => {
|
||||
describe('extractRuleKqlQuery', () => {
|
||||
it('extracts a trimmed version of the query field for inline query types', () => {
|
||||
|
@ -24,6 +43,39 @@ describe('extract rule data queries', () => {
|
|||
filters: [],
|
||||
});
|
||||
});
|
||||
|
||||
it('normalizes filters', () => {
|
||||
const extractedKqlQuery = extractRuleKqlQuery(
|
||||
'event.kind:alert',
|
||||
'kuery',
|
||||
[mockFilter],
|
||||
undefined
|
||||
);
|
||||
|
||||
expect(extractedKqlQuery).toEqual({
|
||||
type: KqlQueryType.inline_query,
|
||||
query: 'event.kind:alert',
|
||||
language: 'kuery',
|
||||
filters: [
|
||||
{
|
||||
meta: {
|
||||
negate: false,
|
||||
disabled: false,
|
||||
type: 'phrase',
|
||||
key: 'test',
|
||||
params: {
|
||||
query: 'value',
|
||||
},
|
||||
},
|
||||
query: {
|
||||
term: {
|
||||
field: 'value',
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('extractRuleEqlQuery', () => {
|
||||
|
|
|
@ -5,6 +5,7 @@
|
|||
* 2.0.
|
||||
*/
|
||||
|
||||
import type { Filter } from '@kbn/es-query';
|
||||
import type {
|
||||
EqlQueryLanguage,
|
||||
EsqlQueryLanguage,
|
||||
|
@ -48,7 +49,7 @@ export const extractInlineKqlQuery = (
|
|||
type: KqlQueryType.inline_query,
|
||||
query: query?.trim() ?? '',
|
||||
language: language ?? 'kuery',
|
||||
filters: filters ?? [],
|
||||
filters: normalizeFilterArray(filters),
|
||||
};
|
||||
};
|
||||
|
||||
|
@ -65,7 +66,7 @@ export const extractRuleEqlQuery = (params: ExtractRuleEqlQueryParams): RuleEqlQ
|
|||
return {
|
||||
query: params.query.trim(),
|
||||
language: params.language,
|
||||
filters: params.filters ?? [],
|
||||
filters: normalizeFilterArray(params.filters),
|
||||
event_category_override: params.eventCategoryOverride,
|
||||
timestamp_field: params.timestampField,
|
||||
tiebreaker_field: params.tiebreakerField,
|
||||
|
@ -81,3 +82,22 @@ export const extractRuleEsqlQuery = (
|
|||
language,
|
||||
};
|
||||
};
|
||||
|
||||
/**
|
||||
* Removes the null `alias` field that gets appended from the internal kibana filter util for comparison
|
||||
* Relevant issue: https://github.com/elastic/kibana/issues/202966
|
||||
*/
|
||||
const normalizeFilterArray = (filters: RuleFilterArray | undefined): RuleFilterArray => {
|
||||
if (!filters?.length) {
|
||||
return [];
|
||||
}
|
||||
return (filters as Filter[]).map((filter) => ({
|
||||
...filter,
|
||||
meta: filter.meta
|
||||
? {
|
||||
...filter.meta,
|
||||
alias: filter.meta.alias ?? undefined,
|
||||
}
|
||||
: undefined,
|
||||
}));
|
||||
};
|
||||
|
|
|
@ -175,8 +175,8 @@ export const QueryBarField = ({
|
|||
// if saved query fetched, reset values in queryBar input and filters to saved query's values
|
||||
useEffect(() => {
|
||||
if (resetToSavedQuery && savedQuery) {
|
||||
const newFiledValue = savedQueryToFieldValue(savedQuery);
|
||||
setFieldValue(newFiledValue);
|
||||
const newFieldValue = savedQueryToFieldValue(savedQuery);
|
||||
setFieldValue(newFieldValue);
|
||||
}
|
||||
}, [resetToSavedQuery, savedQuery, setFieldValue]);
|
||||
|
||||
|
|
|
@ -179,6 +179,110 @@ export default ({ getService }: FtrProviderContext): void => {
|
|||
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('when all query versions have filters with alias fields set to null', () => {
|
||||
it('should not show in the upgrade/_review API response', async () => {
|
||||
// Install base prebuilt detection rule
|
||||
await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
|
||||
createRuleAssetSavedObject({
|
||||
rule_id: 'rule-1',
|
||||
version: 1,
|
||||
type: 'query',
|
||||
query: 'query string = true',
|
||||
language: 'kuery',
|
||||
filters: [
|
||||
{
|
||||
meta: {
|
||||
negate: false,
|
||||
disabled: false,
|
||||
type: 'phrase',
|
||||
key: 'test',
|
||||
params: {
|
||||
query: 'value',
|
||||
},
|
||||
},
|
||||
query: {
|
||||
term: {
|
||||
field: 'value',
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
}),
|
||||
]);
|
||||
await installPrebuiltRules(es, supertest);
|
||||
|
||||
// Customize a kql_query field on the installed rule
|
||||
await updateRule(supertest, {
|
||||
...getPrebuiltRuleMock(),
|
||||
rule_id: 'rule-1',
|
||||
type: 'query',
|
||||
query: 'query string = true',
|
||||
language: 'kuery',
|
||||
filters: [
|
||||
{
|
||||
meta: {
|
||||
alias: null,
|
||||
negate: false,
|
||||
disabled: false,
|
||||
type: 'phrase',
|
||||
key: 'test',
|
||||
params: {
|
||||
query: 'value',
|
||||
},
|
||||
},
|
||||
query: {
|
||||
term: {
|
||||
field: 'value',
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
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',
|
||||
language: 'kuery',
|
||||
filters: [
|
||||
{
|
||||
meta: {
|
||||
negate: false,
|
||||
disabled: false,
|
||||
type: 'phrase',
|
||||
key: 'test',
|
||||
params: {
|
||||
query: 'value',
|
||||
},
|
||||
},
|
||||
query: {
|
||||
term: {
|
||||
field: 'value',
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
}),
|
||||
];
|
||||
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", () => {
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue