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] Handle missing base versions during rule upgrade #154571

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Apr 6, 2023

Resolves: #152836
Resolves: #148183
Resolves partially: #148189

Summary

Updates the rule status (prebuilt_rules/status) and upgrade review (prebuilt_rules/upgrade/_review) APIs to continue working when the base rule version is unavailable, for example, when a detection rules package is released without historical versions.

The logic applied when calculating rule diff is as follows:

  if (baseVersion === MissingVersion) {
    /**
     * We couldn't find the base version of the rule in the package, so further
     * version comparison is not possible. We assume that the rule is not
     * customized, and the value can be updated if there's an update.
     */
    return isEqual(currentVersion, targetVersion)
      ? ThreeWayDiffOutcome.StockValueNoUpdate
      : ThreeWayDiffOutcome.StockValueCanUpdate;
  }

The above logic has considerable implications. When a historical version of a rule is unavailable, we assume that the rule had no modifications. Therefore, in cases where it has been modified, the upgrade review endpoint would still return "no conflicts detected", and users risk losing all their edits.

{
  name: {
    current_version: "My Modified Version Test 123",
    target_version: "Attempt to Modify an Okta Policy Rule",
    merged_version: "Attempt to Modify an Okta Policy Rule",
    diff_outcome: "BASE=A, CURRENT=A, TARGET=B",
    merge_outcome: "TARGET",
    has_update: true,
    has_conflict: false,
  },
}

If we had the base versions, the result would be different:

{
  name: {
    current_version: "My Modified Version Test 123",
    target_version: "Attempt to Modify an Okta Policy Rule",
    merged_version: "My Modified Version Test 123",
    diff_outcome: "BASE=A, CURRENT=B, TARGET=A",
    merge_outcome: "CURRENT",
    has_update: false,
    has_conflict: false,
  },
}

Minor changes

This PR adds the RuleUpgradeStatsForReview.tags implementation.

Additionally, this PR removes some unused API fields:

  • PrebuiltRulesStatusStats.num_prebuilt_rules_total
  • RuleUpgradeStatsForReview.num_rules_to_upgrade_not_customized
  • RuleUpgradeStatsForReview.num_rules_to_upgrade_customized
  • RuleUpgradeStatsForReview.fields

@xcrzx xcrzx self-assigned this Apr 6, 2023
@xcrzx xcrzx changed the title [Security Solution] Rule upgrade workflow API follow-up [Security Solution] Handle missing base versions during rule upgrade Apr 17, 2023
@xcrzx xcrzx added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area v8.8.0 labels Apr 17, 2023
@xcrzx xcrzx marked this pull request as ready for review April 17, 2023 12:48
@xcrzx xcrzx requested a review from a team as a code owner April 17, 2023 12:48
@xcrzx xcrzx requested a review from maximpn April 17, 2023 12:48
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@xcrzx xcrzx force-pushed the rule-upgrade-workflow-api branch 2 times, most recently from 263fe0d to 409217e Compare April 17, 2023 15:42
@banderror banderror requested review from spong and removed request for maximpn April 18, 2023 12:30
@xcrzx xcrzx force-pushed the rule-upgrade-workflow-api branch from 409217e to b42bc1a Compare April 18, 2023 13:26
Comment on lines +58 to +60
if (hasBaseVersion) {
invariant(base_version.type === 'query', BASE_TYPE_ERROR);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe extract these invariants to a common function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to, but Typescript started complaining about the rule type mismatch. Unfortunately, I haven't found a clean way to fix that, so I left the checks as is.

Comment on lines +42 to +50
/**
* We couldn't find the base version of the rule in the package so further
* version comparison is not possible. We assume that the rule is not
* customized and the value can be updated if there's an update.
*/
return currentEqlTarget
? ThreeWayDiffOutcome.StockValueNoUpdate
: ThreeWayDiffOutcome.StockValueCanUpdate;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WRT

The above logic has considerable implications. When a historical version of a rule is unavailable, we assume that the rule had no modifications. Therefore, in cases where it has been modified, the upgrade review endpoint would still return "no conflicts detected", and users risk losing all their edits.

In these instances, I'm curious if we should have some UI/UX to help mitigate the issue in the event it actually happens. Whether that's encouraging the user to do a quick export of existing rules, a system-triggered temporary auto-backup of the rules in question, or something along those lines? Depending on how the storage of historical rules goes, perhaps this is uncommon enough to not even be necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that users could lose all their modifications without the ability to restore them, I think it makes sense to implement some safeguards on the client side. It's not a big concern, though, as long as we do not allow them to customize prebuilt rules. So, maybe we should wait and see how the adoption of historical rules goes. If we see a substantial number of packages without historical versions or not all versions available for every rule, then we'll need to implement some workarounds in the UI.

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM! 👍 Thanks @xcrzx 🙂

Left a couple nits and a discussion around losing user data when missing historical versions, but no changes required.

@xcrzx xcrzx force-pushed the rule-upgrade-workflow-api branch from b42bc1a to 8d9a34f Compare April 19, 2023 09:46
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 432 434 +2

Total ESLint disabled count

id before after diff
securitySolution 512 514 +2

History

  • 💛 Build #120701 was flaky b42bc1a320dbd40fe9cdf4291e8156c8262cc2b4
  • 💛 Build #120366 was flaky 409217e76ac49f51737de70f683aef7f6e0513d4
  • 💛 Build #120264 was flaky 263fe0d3e48a8bbb06a460b01cc89b72c07d7199
  • 💚 Build #118375 succeeded 062ba3cb0a67cce90b047e8e4628b6328d109d94

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @xcrzx

@xcrzx xcrzx merged commit 4e5ad09 into elastic:main Apr 19, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 19, 2023
@xcrzx xcrzx deleted the rule-upgrade-workflow-api branch April 19, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area Feature:Rule Management Security Solution Detection Rule Management area release_note:skip Skip the PR/issue when compiling release notes 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.8.0
Projects
None yet
5 participants