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] Deleting value lists referential integrity #81302

Closed
MadameSheema opened this issue Oct 21, 2020 · 1 comment · Fixed by #85925
Closed

[Security Solution] Deleting value lists referential integrity #81302

MadameSheema opened this issue Oct 21, 2020 · 1 comment · Fixed by #85925
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Rule Value Lists Security Solution Detection Rule Value Lists area impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.11.0

Comments

@MadameSheema
Copy link
Member

Info:

  • Found during testing 7.9 branch
  • Commit 023bc67

Preconditon:

  • To have a rule created with an exception using an exception list

Steps to reproduce:

  1. Delete the value list used in the rule

Current behaviour:

  • The value list is not taken into account anymore
  • The value list is displayed in the rule
  • When you edit the exception
  • The value is empty and if you keep it empty
  • When you save the edition the old list is still displayed

Expected behaviour:

  • TBD
@MadameSheema MadameSheema added bug Fixes for quality problems that affect the customer experience Team:Detections and Resp Security Detection Response Team labels Oct 21, 2020
@MadameSheema MadameSheema reopened this Oct 21, 2020
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Oct 27, 2020
@peluja1012 peluja1012 added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Oct 28, 2020
@peluja1012 peluja1012 added the Feature:Rule Value Lists Security Solution Detection Rule Value Lists area label Nov 17, 2020
@marrasherrier
Copy link
Contributor

marrasherrier commented Nov 19, 2020

Figma

@spong spong self-assigned this Nov 19, 2020
@spong spong added the v7.11.0 label Dec 1, 2020
spong added a commit that referenced this issue Dec 16, 2020
…when deleting value lists (#85925)

## Summary

Resolves #77324, #77325, resolves #77325, and resolves #81302


This PR addresses referential integrity issues when deleting value lists. Previously when deleting value lists, any references in Exception Lists/Items would be left behind. This PR introduces a new confirmation modal when deleting value lists that are referenced in either space aware (`simple`) or space `agnostic` exception lists.

Also includes:

* Fixed Lists plugin `quick_start.sh` as it was using endpoint exception list + value lists (unsupported)
* Adds `quick_start_value_list_references.sh` to create exception lists/items, value lists, and references to easily test
* Add support to `findExceptionList` for searching for both `simple` and `agnostic` list types
* Two new query params have been added to the `deleteListRoute`
  * `ignoreReferences` (default:false) when true, maintains pre-7.11 behavior of deleting value list without performing any additional checks. 
    * NOTE: As written, this becomes an API breaking change as existing existing calls to the same API will `409` conflict if references exist. cc @jmikell821 @Donnater 
  * `deleteReferences` (default:false) to perform dry run and identify referenced exception lists/items

## Testing
To test, run `quick_start_value_list_references.sh` and it will create all the necessary resources/references to easily exercise the above functionality. The below diagram details the resources created and how the references are wired up.

> Creates three different exception lists and value lists, and associates as
> below to test referential integrity functionality.
>
> NOTE: Endpoint lists don't support value lists, and are not tested here
>
> EL: Exception list
> ELI Exception list Item
> VL: Value list
>
>      EL1        EL2 (Agnostic)   EL3
>       |          |                |
>      ELI1       ELI2             ELI3
>       |\        /|                |
>       | \      / |                |
>       |  \    /  |                |
>       |   \  /   |                |
>       |    \/    |                |
>       |    /\    |                |
>       |   /  \   |                |
>       |  /    \  |                |
>       | /      \ |                |
>       |/        \|                |
>      VL1        VL2              VL3        VL4
>      ips.txt  ip_range.txt       text.txt   hosts.txt
>

Corner cases to be aware of:

* An exception item may have multiple value list entries -- only referenced value list entries should be removed
  * There is no API for removing individual entries. If all entries are references the entire item is deleted. If only some entries are references, the item is updated via a `PUT` (no `PATCH` support for exception items)
* It's not possible via the UI to create a space agnostic list that has value list exception items (only agnostic endpoint exception lists can be created and they do not support value lists). Please use above script to exercise this behavior.


Additional notes:
* Once the Exception List table is introduced (#85465), we can add an enhancement for deeplinking to exception lists from the reference error modal.
* The `deleteListRoute` response has been updated to include the responses from the reference checks to provide maximum flexibility
* There is no bulk API for deleting exception list items, and so they are iterated over via the `deleteExceptionListItem` API.


##### Reference error modal
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/102199153-813e1e80-3e80-11eb-8a9b-af116ca13df9.gif" />
</p>




##### Overflow example
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/102199032-5784f780-3e80-11eb-81c7-17283d002ce4.gif" />
</p>

### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [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] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

### For maintainers

- [X] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
spong added a commit that referenced this issue Dec 16, 2020
…when deleting value lists (#85925) (#86075)

## Summary

Resolves #77324, #77325, resolves #77325, and resolves #81302


This PR addresses referential integrity issues when deleting value lists. Previously when deleting value lists, any references in Exception Lists/Items would be left behind. This PR introduces a new confirmation modal when deleting value lists that are referenced in either space aware (`simple`) or space `agnostic` exception lists.

Also includes:

* Fixed Lists plugin `quick_start.sh` as it was using endpoint exception list + value lists (unsupported)
* Adds `quick_start_value_list_references.sh` to create exception lists/items, value lists, and references to easily test
* Add support to `findExceptionList` for searching for both `simple` and `agnostic` list types
* Two new query params have been added to the `deleteListRoute`
  * `ignoreReferences` (default:false) when true, maintains pre-7.11 behavior of deleting value list without performing any additional checks. 
    * NOTE: As written, this becomes an API breaking change as existing existing calls to the same API will `409` conflict if references exist. cc @jmikell821 @Donnater 
  * `deleteReferences` (default:false) to perform dry run and identify referenced exception lists/items

## Testing
To test, run `quick_start_value_list_references.sh` and it will create all the necessary resources/references to easily exercise the above functionality. The below diagram details the resources created and how the references are wired up.

> Creates three different exception lists and value lists, and associates as
> below to test referential integrity functionality.
>
> NOTE: Endpoint lists don't support value lists, and are not tested here
>
> EL: Exception list
> ELI Exception list Item
> VL: Value list
>
>      EL1        EL2 (Agnostic)   EL3
>       |          |                |
>      ELI1       ELI2             ELI3
>       |\        /|                |
>       | \      / |                |
>       |  \    /  |                |
>       |   \  /   |                |
>       |    \/    |                |
>       |    /\    |                |
>       |   /  \   |                |
>       |  /    \  |                |
>       | /      \ |                |
>       |/        \|                |
>      VL1        VL2              VL3        VL4
>      ips.txt  ip_range.txt       text.txt   hosts.txt
>

Corner cases to be aware of:

* An exception item may have multiple value list entries -- only referenced value list entries should be removed
  * There is no API for removing individual entries. If all entries are references the entire item is deleted. If only some entries are references, the item is updated via a `PUT` (no `PATCH` support for exception items)
* It's not possible via the UI to create a space agnostic list that has value list exception items (only agnostic endpoint exception lists can be created and they do not support value lists). Please use above script to exercise this behavior.


Additional notes:
* Once the Exception List table is introduced (#85465), we can add an enhancement for deeplinking to exception lists from the reference error modal.
* The `deleteListRoute` response has been updated to include the responses from the reference checks to provide maximum flexibility
* There is no bulk API for deleting exception list items, and so they are iterated over via the `deleteExceptionListItem` API.


##### Reference error modal
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/102199153-813e1e80-3e80-11eb-8a9b-af116ca13df9.gif" />
</p>




##### Overflow example
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/102199032-5784f780-3e80-11eb-81c7-17283d002ce4.gif" />
</p>

### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [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] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

### For maintainers

- [X] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Rule Value Lists Security Solution Detection Rule Value Lists area impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.11.0
Projects
None yet
5 participants