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 Solutions][Detections] - Fix exception list table referential deletion #87231

Merged
merged 6 commits into from
Jan 5, 2021

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Jan 4, 2021

Summary

This PR concentrates on fixing the deletion on the exceptions list table view. This fix is intermediary and a more thorough, backend solution is needed. Currently, if you delete an exception list, it deletes the exception list SO, but does not remove references to it from rules. This PR allows for a quick fix conducting this logic client side. This is not ideal as we want to provide referential integrity api side. The current logic is as follows upon hitting "delete" on the exceptions list table:

  • if exception list is not referenced elsewhere (dangling) - deleteExceptionList called
  • if exception list is referenced elsewhere - 1) modal pops up informing user that proceeding with deletion will remove exception list from the rules shown 2) rules are patched to remove exception list 3) upon successful completion of all rules being patched, exception list SO is deleted
  • if any rule patch fails, exception list SO is not deleted and user informed of failure

Testing

To test, navigate to the lists plugins scripts and run the following to create 3 exception lists:

./hard_reset.sh && \
# Create exception lists 1, 2 (agnostic), 3
./post_exception_list.sh ./exception_lists/new/references/exception_list_detection_1.json && \
./post_exception_list.sh ./exception_lists/new/references/exception_list_detection_2_agnostic.json && \
./post_exception_list.sh ./exception_lists/new/references/exception_list_detection_3.json && \
# Create exception list items
./post_exception_list_item.sh ./exception_lists/new/references/exception_list_item_1_non_value_list.json && \
./post_exception_list_item.sh ./exception_lists/new/references/exception_list_item_2_non_value_list.json && \
./post_exception_list_item.sh ./exception_lists/new/references/exception_list_item_3_non_value_list.json

Take note of the ids of the lists created above. You'll need to add the exception list ids to the files noted in the below script for creating the rules. Navigate to the detections plugins scripts and run the following to create 2 rules that make reference to the newly created exception lists:

./hard_reset.sh && \
# Create rules 1, 2
./post_rule.sh ./rules/queries/references/query_with_single_exception_list.json && \
./post_rule.sh ./rules/queries/references/query_with_multiple_exception_lists.json

EL: Exception list

 EL1        EL2 (Agnostic)   EL3       
  |              /|                
  |             / |                
  |            /  |                
  |           /   |               
  |          /    |                
  |         /     |             
  |        /      |             
  |       /       |             
  |      /        |            
  |     /         |             
RULE1           RULE2 

Additional notes:

  • Rule attributes are not searchable at this time, so unfortunately we have to fetch all rules and iterate through them in order to find which contain references to the exception list of interest
  • There is an issue and discussion about use of back links that would make these referential integrity issues much nicer to deal with
  • The ideal solution is to have the exceptions list delete endpoint take care of this logic

Screenshots

Modal shown when references exist

Screen Shot 2021-01-04 at 11 18 59 PM

Error shown if rule patch error occurs during reference deletion

Screen Shot 2021-01-04 at 11 55 57 PM

Checklist

@yctercero yctercero self-assigned this Jan 4, 2021
@yctercero yctercero added bug Fixes for quality problems that affect the customer experience release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team Team:SIEM v7.11.0 labels Jan 4, 2021
@spong
Copy link
Member

spong commented Jan 5, 2021

Tangential to this PR, but just commenting since I saw it in testing, but the [comma][space] join logic for the Rules assigned to column doesn't seem to be working:

I remember seeing a similar issue way back when with the Host Details description view, for which it looks like @FrankHassanabad ended up solving with a <Spacer> component using CSS margins, so may want to go that route.

Also, looks like that <LinkAnchor> component in the Rules assigned to column renderer will need a key attr as I'm seeing the Each child in a list should have a unique "key" prop. console error when navigating to the Exception lists tab.

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.

Checked out, tested locally, and performed code review -- LGTM! 👍 All worked as expected (thanks for the setup scripts! :), and even hairy corner cases like starting to delete an exception list with multiple references, then wiping out one of the referenced Rules in another tab worked as expected. Thanks @yctercero! 🙂

@yctercero yctercero marked this pull request as ready for review January 5, 2021 04:09
@yctercero yctercero requested review from a team as code owners January 5, 2021 04:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@yctercero
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 8.5MB 8.5MB +6.1KB

Distributable file count

id before after diff
default 47252 48016 +764

History

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

@yctercero yctercero merged commit 51efc19 into elastic:master Jan 5, 2021
yctercero added a commit to yctercero/kibana that referenced this pull request Jan 5, 2021
…al deletion (elastic#87231)

### Summary

This PR concentrates on fixing the deletion on the exceptions list table view. This fix is intermediary and a more thorough, backend solution is needed. Currently, if you delete an exception list, it deletes the exception list SO, but does not remove references to it from rules. This PR allows for a quick fix conducting this logic client side.
yctercero added a commit to yctercero/kibana that referenced this pull request Jan 5, 2021
…al deletion (elastic#87231)

### Summary

This PR concentrates on fixing the deletion on the exceptions list table view. This fix is intermediary and a more thorough, backend solution is needed. Currently, if you delete an exception list, it deletes the exception list SO, but does not remove references to it from rules. This PR allows for a quick fix conducting this logic client side.
@yctercero yctercero deleted the fix_exceptions_delete branch January 5, 2021 17:55
yctercero added a commit that referenced this pull request Jan 5, 2021
…al deletion (#87231) (#87357)

### Summary

This PR concentrates on fixing the deletion on the exceptions list table view. This fix is intermediary and a more thorough, backend solution is needed. Currently, if you delete an exception list, it deletes the exception list SO, but does not remove references to it from rules. This PR allows for a quick fix conducting this logic client side.
yctercero added a commit that referenced this pull request Jan 5, 2021
…al deletion (#87231) (#87358)

### Summary

This PR concentrates on fixing the deletion on the exceptions list table view. This fix is intermediary and a more thorough, backend solution is needed. Currently, if you delete an exception list, it deletes the exception list SO, but does not remove references to it from rules. This PR allows for a quick fix conducting this logic client side.
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 release_note:fix Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants