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] JSON diffs for prebuilt rules: Technical / runtime properties are displayed #174844

Closed
Tracked by #174167
nikitaindik opened this issue Jan 15, 2024 · 4 comments · Fixed by #174789
Closed
Tracked by #174167
Assignees
Labels
8.13 candidate bug Fixes for quality problems that affect the customer experience Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.12.1

Comments

@nikitaindik
Copy link
Contributor

nikitaindik commented Jan 15, 2024

Scherm­afbeelding 2024-01-15 om 14 43 03

Kibana version:
v8.12.0.

Describe the bug:
The JSON diff displays some technical/runtime fields we shouldn't expose to the user.

Live, installed rules might have additional properties that are not present in prebuilt rules.

Unwanted fields that might make it into the diff display:

  • enabled. Most prebuilt rules are disabled by default ("enabled": false). Once you install and enable a disabled rule, enabled becomes true, and we end up with a diff for this field.
  • exceptions_list. Never present in prebuilt rule updates. Generates diff if user adds exceptions to installed rule.
  • execution_summary. Once a rule runs, the execution_summary object gets added to the RuleResponse, which we use for diffing.
  • actions. Gets added when you add an action to a prebuilt rule and then save it.
  • filters. Equal to []. Gets added when you save a prebuilt rule.
  • timestamp_override_fallback_disabled, meta. Gets added when you save a prebuilt rule.
  • from. Gets added when you save a prebuilt rule.
  • updated_at, updated_by, created_at, created_by. Not relevant to the update flow (at least before we implement rule customisation)
  • revision. Can be confused with "version" by users. It's already hidden, listing for completeness.

Steps to reproduce:

  1. Install a prebuilt rule that is disabled by default (any rule except "Endpoint Security" or "Container Workload Protection")
  2. Enable the rule, let it run
  3. Create an update for this installed prebuilt rule
  4. enabled and execution_summary properties are displayed in the update diff.
  5. Add an action to this prebuilt rule and save
  6. actions, filters, meta and timestamp_override_fallback_disabled are displayed in the update diff.
  7. Also notice that after saving, from field is converted from minutes to seconds: now-10m -> now-600s, which also creates diff.

Expected behaviour:
These fields should be hidden. We need to display the diff only for fields that were added by the TRADE team.

List of all the fields that rules can have: #147239

@nikitaindik nikitaindik added bug Fixes for quality problems that affect the customer experience Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area v8.12.1 labels Jan 15, 2024
@nikitaindik nikitaindik self-assigned this Jan 15, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@nikitaindik nikitaindik changed the title [Security Solution] JSON diffs for prebuilt rules: Technical / runtime fields are displayed [Security Solution] JSON diffs for prebuilt rules: Technical / runtime properties are displayed Jan 15, 2024
@nikitaindik
Copy link
Contributor Author

@joepeeples Can we mention this in the known issues for v8.12 release?

@banderror banderror added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. 8.13 candidate labels Jan 16, 2024
nikitaindik added a commit that referenced this issue Jan 25, 2024
… properties (#174789)

**Resolves: #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
<img width="1271" alt="Scherm­afbeelding 2024-01-16 om 13 50 00"
src="https://github.com/elastic/kibana/assets/15949146/f72283dc-9a8a-4c95-a9b6-daa84d9356da">



#### After
<img width="1271" alt="Scherm­afbeelding 2024-01-16 om 13 50 36"
src="https://github.com/elastic/kibana/assets/15949146/080ef2ea-c108-4d05-8814-0a2ce7f5a0b0">
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Jan 25, 2024
… properties (elastic#174789)

**Resolves: elastic#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
<img width="1271" alt="Scherm­afbeelding 2024-01-16 om 13 50 00"
src="https://github.com/elastic/kibana/assets/15949146/f72283dc-9a8a-4c95-a9b6-daa84d9356da">

#### After
<img width="1271" alt="Scherm­afbeelding 2024-01-16 om 13 50 36"
src="https://github.com/elastic/kibana/assets/15949146/080ef2ea-c108-4d05-8814-0a2ce7f5a0b0">

(cherry picked from commit 5bf935b)
kibanamachine referenced this issue Jan 25, 2024
…nternal properties (#174789) (#175625)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[Security Solution] Rule upgrade JSON diff: Hide runtime and internal
properties (#174789)](#174789)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nikita
Indik","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-01-25T17:25:23Z","message":"[Security
Solution] Rule upgrade JSON diff: Hide runtime and internal properties
(#174789)\n\n**Resolves:
https://github.com/elastic/kibana/issues/174844**\r\n\r\n##
Summary\r\nHides technical/runtime fields that shouldn't be displayed in
the JSON\r\ndiff view.\r\nWe used to hide only the `revision` field
because it can be confused\r\nwith `version`. This PR hides more
fields.\r\n\r\nProperties that might be displayed as having diff, but
shouldn't:\r\n- `actions`: shown as diff if user defined an action for a
rule\r\n- `exceptions_list`: shown as diff if user defined an exception
list for\r\na rule\r\n- `execution_summary`: shown as diff if user has
enabled a rule and it\r\nexecuted at least once\r\n- `enabled`: shown as
diff if user enabled a rule that's disabled by\r\ndefault (or vice
versa)\r\n- `updated_at`: always shown as diff because its value is a
timestamp of\r\nwhen an API request made\r\n- `from`: might be shown as
diff if user has clicked \"save\" after\r\nediting a rule, because edit
screen's FE code converts value to a\r\ndifferent time unit, like 2h ->
7200s\r\n- `note`: shown as diff if an old version of a rule didn't
define this\r\nproperty, but a new version of a rule has it defined as
''\r\n- `threat`: might be shown as diff if user has clicked \"save\"
after\r\nediting a rule, because edit screen's FE code adds empty arrays
as\r\ndefaults if threats/techniques/subtechniques weren't set by the
user\r\n- `machine_learning_job_id`: might be shown as diff if a
prebuilt rule\r\nuses the legacy string format for this property. On
installation, the\r\nvalue is converted into a new array format, which
creates a difference\r\nbetween the installed rule (array format) and
the update (string format)\r\n- `threat_filters`: might be shown as diff
if user has clicked \"save\"\r\nafter editing a rule, because edit
screen's FE code adds null as a\r\ndefault value for \"meta\"
subproperty\r\n- `filters`: might be shown as diff if user has clicked
\"save\" after\r\nediting a rule, because edit screen's FE code adds []
as a default value\r\n- `timestamp_override_fallback_disabled`: might be
shown as diff if user\r\nhas clicked \"save\" after editing a rule\r\n-
`meta`: might be shown as diff if user has clicked \"save\"
after\r\nediting a rule\r\n- `output_index`: unused, shouldn't be
shown\r\n- `updated_at`, `updated_by`, `created_at`, `created_by`:
should be\r\nhidden because these are not relevant for the upgrade
flow\r\n\r\n\r\n\r\n#### Before\r\n<img width=\"1271\"
alt=\"Scherm­afbeelding 2024-01-16 om 13 50
00\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/f72283dc-9a8a-4c95-a9b6-daa84d9356da\">\r\n\r\n\r\n\r\n####
After\r\n<img width=\"1271\" alt=\"Scherm­afbeelding 2024-01-16 om 13 50
36\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/080ef2ea-c108-4d05-8814-0a2ce7f5a0b0\">","sha":"5bf935b5c30dd489ce381fc337e674443349940c","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","v8.12.1","v8.13.0"],"title":"[Security Solution] Rule upgrade
JSON diff: Hide runtime and internal
properties","number":174789,"url":"https://github.com/elastic/kibana/pull/174789","mergeCommit":{"message":"[Security
Solution] Rule upgrade JSON diff: Hide runtime and internal properties
(#174789)\n\n**Resolves:
https://github.com/elastic/kibana/issues/174844**\r\n\r\n##
Summary\r\nHides technical/runtime fields that shouldn't be displayed in
the JSON\r\ndiff view.\r\nWe used to hide only the `revision` field
because it can be confused\r\nwith `version`. This PR hides more
fields.\r\n\r\nProperties that might be displayed as having diff, but
shouldn't:\r\n- `actions`: shown as diff if user defined an action for a
rule\r\n- `exceptions_list`: shown as diff if user defined an exception
list for\r\na rule\r\n- `execution_summary`: shown as diff if user has
enabled a rule and it\r\nexecuted at least once\r\n- `enabled`: shown as
diff if user enabled a rule that's disabled by\r\ndefault (or vice
versa)\r\n- `updated_at`: always shown as diff because its value is a
timestamp of\r\nwhen an API request made\r\n- `from`: might be shown as
diff if user has clicked \"save\" after\r\nediting a rule, because edit
screen's FE code converts value to a\r\ndifferent time unit, like 2h ->
7200s\r\n- `note`: shown as diff if an old version of a rule didn't
define this\r\nproperty, but a new version of a rule has it defined as
''\r\n- `threat`: might be shown as diff if user has clicked \"save\"
after\r\nediting a rule, because edit screen's FE code adds empty arrays
as\r\ndefaults if threats/techniques/subtechniques weren't set by the
user\r\n- `machine_learning_job_id`: might be shown as diff if a
prebuilt rule\r\nuses the legacy string format for this property. On
installation, the\r\nvalue is converted into a new array format, which
creates a difference\r\nbetween the installed rule (array format) and
the update (string format)\r\n- `threat_filters`: might be shown as diff
if user has clicked \"save\"\r\nafter editing a rule, because edit
screen's FE code adds null as a\r\ndefault value for \"meta\"
subproperty\r\n- `filters`: might be shown as diff if user has clicked
\"save\" after\r\nediting a rule, because edit screen's FE code adds []
as a default value\r\n- `timestamp_override_fallback_disabled`: might be
shown as diff if user\r\nhas clicked \"save\" after editing a rule\r\n-
`meta`: might be shown as diff if user has clicked \"save\"
after\r\nediting a rule\r\n- `output_index`: unused, shouldn't be
shown\r\n- `updated_at`, `updated_by`, `created_at`, `created_by`:
should be\r\nhidden because these are not relevant for the upgrade
flow\r\n\r\n\r\n\r\n#### Before\r\n<img width=\"1271\"
alt=\"Scherm­afbeelding 2024-01-16 om 13 50
00\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/f72283dc-9a8a-4c95-a9b6-daa84d9356da\">\r\n\r\n\r\n\r\n####
After\r\n<img width=\"1271\" alt=\"Scherm­afbeelding 2024-01-16 om 13 50
36\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/080ef2ea-c108-4d05-8814-0a2ce7f5a0b0\">","sha":"5bf935b5c30dd489ce381fc337e674443349940c"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/174789","number":174789,"mergeCommit":{"message":"[Security
Solution] Rule upgrade JSON diff: Hide runtime and internal properties
(#174789)\n\n**Resolves:
https://github.com/elastic/kibana/issues/174844**\r\n\r\n##
Summary\r\nHides technical/runtime fields that shouldn't be displayed in
the JSON\r\ndiff view.\r\nWe used to hide only the `revision` field
because it can be confused\r\nwith `version`. This PR hides more
fields.\r\n\r\nProperties that might be displayed as having diff, but
shouldn't:\r\n- `actions`: shown as diff if user defined an action for a
rule\r\n- `exceptions_list`: shown as diff if user defined an exception
list for\r\na rule\r\n- `execution_summary`: shown as diff if user has
enabled a rule and it\r\nexecuted at least once\r\n- `enabled`: shown as
diff if user enabled a rule that's disabled by\r\ndefault (or vice
versa)\r\n- `updated_at`: always shown as diff because its value is a
timestamp of\r\nwhen an API request made\r\n- `from`: might be shown as
diff if user has clicked \"save\" after\r\nediting a rule, because edit
screen's FE code converts value to a\r\ndifferent time unit, like 2h ->
7200s\r\n- `note`: shown as diff if an old version of a rule didn't
define this\r\nproperty, but a new version of a rule has it defined as
''\r\n- `threat`: might be shown as diff if user has clicked \"save\"
after\r\nediting a rule, because edit screen's FE code adds empty arrays
as\r\ndefaults if threats/techniques/subtechniques weren't set by the
user\r\n- `machine_learning_job_id`: might be shown as diff if a
prebuilt rule\r\nuses the legacy string format for this property. On
installation, the\r\nvalue is converted into a new array format, which
creates a difference\r\nbetween the installed rule (array format) and
the update (string format)\r\n- `threat_filters`: might be shown as diff
if user has clicked \"save\"\r\nafter editing a rule, because edit
screen's FE code adds null as a\r\ndefault value for \"meta\"
subproperty\r\n- `filters`: might be shown as diff if user has clicked
\"save\" after\r\nediting a rule, because edit screen's FE code adds []
as a default value\r\n- `timestamp_override_fallback_disabled`: might be
shown as diff if user\r\nhas clicked \"save\" after editing a rule\r\n-
`meta`: might be shown as diff if user has clicked \"save\"
after\r\nediting a rule\r\n- `output_index`: unused, shouldn't be
shown\r\n- `updated_at`, `updated_by`, `created_at`, `created_by`:
should be\r\nhidden because these are not relevant for the upgrade
flow\r\n\r\n\r\n\r\n#### Before\r\n<img width=\"1271\"
alt=\"Scherm­afbeelding 2024-01-16 om 13 50
00\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/f72283dc-9a8a-4c95-a9b6-daa84d9356da\">\r\n\r\n\r\n\r\n####
After\r\n<img width=\"1271\" alt=\"Scherm­afbeelding 2024-01-16 om 13 50
36\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/080ef2ea-c108-4d05-8814-0a2ce7f5a0b0\">","sha":"5bf935b5c30dd489ce381fc337e674443349940c"}}]}]
BACKPORT-->

Co-authored-by: Nikita Indik <[email protected]>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
… properties (elastic#174789)

**Resolves: elastic#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
<img width="1271" alt="Scherm­afbeelding 2024-01-16 om 13 50 00"
src="https://github.com/elastic/kibana/assets/15949146/f72283dc-9a8a-4c95-a9b6-daa84d9356da">



#### After
<img width="1271" alt="Scherm­afbeelding 2024-01-16 om 13 50 36"
src="https://github.com/elastic/kibana/assets/15949146/080ef2ea-c108-4d05-8814-0a2ce7f5a0b0">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.13 candidate bug Fixes for quality problems that affect the customer experience Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.12.1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants