From 5bf935b5c30dd489ce381fc337e674443349940c Mon Sep 17 00:00:00 2001 From: Nikita Indik Date: Thu, 25 Jan 2024 18:25:23 +0100 Subject: [PATCH] [Security Solution] Rule upgrade JSON diff: Hide runtime and internal properties (#174789) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Resolves: https://github.com/elastic/kibana/issues/174844** ## Summary Hides technical/runtime fields that shouldn't be displayed in the JSON diff view. We used to hide only the `revision` field because it can be confused with `version`. This PR hides more fields. Properties that might be displayed as having diff, but shouldn't: - `actions`: shown as diff if user defined an action for a rule - `exceptions_list`: shown as diff if user defined an exception list for a rule - `execution_summary`: shown as diff if user has enabled a rule and it executed at least once - `enabled`: shown as diff if user enabled a rule that's disabled by default (or vice versa) - `updated_at`: always shown as diff because its value is a timestamp of when an API request made - `from`: might be shown as diff if user has clicked "save" after editing a rule, because edit screen's FE code converts value to a different time unit, like 2h -> 7200s - `note`: shown as diff if an old version of a rule didn't define this property, but a new version of a rule has it defined as '' - `threat`: might be shown as diff if user has clicked "save" after editing a rule, because edit screen's FE code adds empty arrays as defaults if threats/techniques/subtechniques weren't set by the user - `machine_learning_job_id`: might be shown as diff if a prebuilt rule uses the legacy string format for this property. On installation, the value is converted into a new array format, which creates a difference between the installed rule (array format) and the update (string format) - `threat_filters`: might be shown as diff if user has clicked "save" after editing a rule, because edit screen's FE code adds null as a default value for "meta" subproperty - `filters`: might be shown as diff if user has clicked "save" after editing a rule, because edit screen's FE code adds [] as a default value - `timestamp_override_fallback_disabled`: might be shown as diff if user has clicked "save" after editing a rule - `meta`: might be shown as diff if user has clicked "save" after editing a rule - `output_index`: unused, shouldn't be shown - `updated_at`, `updated_by`, `created_at`, `created_by`: should be hidden because these are not relevant for the upgrade flow #### Before Scherm­afbeelding 2024-01-16 om 13 50 00 #### After Scherm­afbeelding 2024-01-16 om 13 50 36 --- .../components/rule_details/rule_diff_tab.tsx | 109 +++++++++++++++++- .../normalization/rule_converters.ts | 1 + 2 files changed, 107 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/rule_diff_tab.tsx b/x-pack/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/rule_diff_tab.tsx index 0da09e7e3d0ff..fc1fa754f9694 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/rule_diff_tab.tsx +++ b/x-pack/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/rule_diff_tab.tsx @@ -6,7 +6,7 @@ */ import React, { useMemo } from 'react'; -import { omit } from 'lodash'; +import { omit, pick } from 'lodash'; import stringify from 'json-stable-stringify'; import { EuiSpacer, @@ -16,13 +16,113 @@ import { EuiTitle, EuiIconTip, } from '@elastic/eui'; +import type { Filter } from '@kbn/es-query'; +import { normalizeMachineLearningJobIds } from '../../../../../common/detection_engine/utils'; +import { + formatScheduleStepData, + filterEmptyThreats, +} from '../../../rule_creation_ui/pages/rule_creation/helpers'; import type { RuleResponse } from '../../../../../common/api/detection_engine/model/rule_schema/rule_schemas.gen'; import { DiffView } from './json_diff/diff_view'; import * as i18n from './json_diff/translations'; +import { getHumanizedDuration } from '../../../../detections/pages/detection_engine/rules/helpers'; + +/* Inclding these properties in diff display might be confusing to users. */ +const HIDDEN_PROPERTIES = [ + /* + By default, prebuilt rules don't have any actions or exception lists. So if a user has defined actions or exception lists for a rule, it'll show up as diff. This looks confusing as the user might think that their actions and exceptions lists will get removed after the upgrade, which is not the case - they will be preserved. + */ + 'actions', + 'exceptions_list', + + /* + Most prebuilt rules are installed as "disabled". Once user enables a rule, it'll show as diff, which doesn't add value. + */ + 'enabled', + + /* Technical property. Hidden because it can be confused with "version" by users. */ + 'revision', + + /* + "updated_at" value is regenerated on every '/upgrade/_review' endpoint run + and will therefore always show a diff. It adds no value to display it to the user. + */ + 'updated_at', +]; const sortAndStringifyJson = (jsObject: Record): string => stringify(jsObject, { space: 2 }); +/** + * Normalizes the rule object, making it suitable for comparison with another normalized rule. + * + * Normalization is needed to avoid showing diffs for semantically equal property values. + * Saving a rule via the rule editing UI might change the format of some properties or set default values. + * This function compensates for those changes. + * + * @param {RuleResponse} originalRule - Rule of a RuleResponse type. + * @returns {RuleResponse} - The updated normalized rule object. + */ +const normalizeRule = (originalRule: RuleResponse): RuleResponse => { + const rule = { ...originalRule }; + + /* + Convert the "from" property value to a humanized duration string, like 'now-1m' or 'now-2h'. + Conversion is needed to skip showing the diff for the "from" property when the same + duration is represented in different time units. For instance, 'now-1h' and 'now-3600s' + indicate a one-hour duration. + The same helper is used in the rule editing UI to format "from" before submitting the edits. + So, after the rule is saved, the "from" property unit/value might differ from what's in the package. + */ + rule.from = formatScheduleStepData({ + interval: rule.interval, + from: getHumanizedDuration(rule.from, rule.interval), + to: rule.to, + }).from; + + /* + Default "note" to an empty string if it's not present. + Sometimes, in a new version of a rule, the "note" value equals an empty string, while + in the old version, it wasn't specified at all (undefined becomes ''). In this case, + it doesn't make sense to show diff, so we default falsy values to ''. + */ + rule.note = rule.note ?? ''; + + /* + Removes empty arrays (techniques, subtechniques) from the MITRE ATT&CK value. + The same processing is done in the rule editing UI before submitting the edits. + */ + rule.threat = filterEmptyThreats(rule.threat); + + /* + The "machine_learning_job_id" property is converted from the legacy string format + to the new array format during installation and upgrade. Thus, all installed rules + use the new format. For correct comparison, we must ensure that the rule update is + also in the new format before showing the diff. + */ + if ('machine_learning_job_id' in rule) { + rule.machine_learning_job_id = normalizeMachineLearningJobIds(rule.machine_learning_job_id); + } + + /* + Default the "alias" property to null for all threat filters that don't have it. + Setting a default is needed to match the behavior of the rule editing UI, + which also defaults the "alias" property to null. + */ + if (rule.type === 'threat_match' && Array.isArray(rule.threat_filters)) { + const threatFiltersWithDefaultMeta = (rule.threat_filters as Filter[]).map((filter) => { + if (!filter.meta.alias) { + return { ...filter, meta: { ...filter.meta, alias: null } }; + } + return filter; + }); + + rule.threat_filters = threatFiltersWithDefaultMeta; + } + + return rule; +}; + interface RuleDiffTabProps { oldRule: RuleResponse; newRule: RuleResponse; @@ -30,8 +130,11 @@ interface RuleDiffTabProps { export const RuleDiffTab = ({ oldRule, newRule }: RuleDiffTabProps) => { const [oldSource, newSource] = useMemo(() => { - const visibleOldRuleProperties = omit(oldRule, 'revision'); - const visibleNewRuleProperties = omit(newRule, 'revision'); + const visibleNewRuleProperties = omit(normalizeRule(newRule), ...HIDDEN_PROPERTIES); + const visibleOldRuleProperties = omit( + /* Only compare properties that are present in the update. */ + pick(normalizeRule(oldRule), Object.keys(visibleNewRuleProperties)) + ); return [ sortAndStringifyJson(visibleOldRuleProperties), diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts index 2402e9fdcdf33..2756c54b71951 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts @@ -745,6 +745,7 @@ export const convertPrebuiltRuleAssetToRuleResponse = ( related_integrations: [], required_fields: [], setup: '', + note: '', references: [], threat: [], tags: [],