[SIEM][Detections] Restrict ML rule modification to ML Admins #65583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These revolve around capabilities/permissions checks which were previously only used on the client. Now that we have need to make similar checks on the server, we can/should move these up to be shared.
There was already some drift between these types preventing our helpers from being used with the ML services; this will prevent further drift.
Some of this responsibility will move to ML services in the near future, but for now we still need to restrict SIEM users from performing certain actions.
The tests were a little tricky because the use of spyOn/clear was preventing (rather, clearing the mocks from) the use of jest.mock(). I found a workaround with mockRestore(), which was easy to verify because the mock would throw an error if it wasn't removed, and we'd import multiple rules if a default mock was used. The threading through of ML can go away if/when ML adds their services to the request handler context.
This is now unused and redundant with the mlAuthz module.
These were missed when the helpers were moved to common/, but are also unneeded.
|
Pinging @elastic/siem (Team:SIEM) |
|
I think there are going to be cases in high division-of-labor environments where SOC personnel are senior enough to be tasked with managing all rules but not senior enough to be tasked or authorized ML admins. |
@randomuserid I agree that we should support that use case. Until alerting's permissions model supports that, this will prevent said rule managers from unintentionally breaking ML rules. @spong I think we need to add this requirement to #50222 as the "refresh permissions" case does not cover it: this would require a break from alerting's current "you touched it last" model. |
Conflicts: x-pack/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts
A recent upstream refactor in ML added top-level exports; this uses them where possible.
This prevents us from unnecessarily calling ml services if e.g. we're not dealing with an ML rule. This also adds a failing test for the next-to-be-implemented feature: cashing the async validation for subsequent validator calls.
The purpose of the `buildMlAuthz` function is to store state (request, license, ml). Since `validateMlAuthz` should be idempotent for the duration of this object's lifecycle, we should cache the result the first time we call it; this is effectively memoization since the arguments do not change.
| @@ -4,10 +4,14 @@ | |||
| * you may not use this file except in compliance with the Elastic License. | |||
| */ | |||
|
|
|||
| import { MlCapabilities } from './types'; | |||
| import { MlCapabilitiesResponse } from '../../../ml/common/types/capabilities'; | |||
There was a problem hiding this comment.
Nice they have types we can use 👍 !
| updateRulesRoute(router); | ||
| patchRulesRoute(router); | ||
| updateRulesRoute(router, ml); | ||
| patchRulesRoute(router, ml); | ||
| deleteRulesRoute(router); | ||
| findRulesRoute(router); | ||
|
|
||
| addPrepackedRulesRoute(router); |
There was a problem hiding this comment.
Prepackaged rules will contain ML types, do you want to add ML checks and block adding any rule additions from occurring if the user does not have privileges or maybe we have to do something more involved with it where only the non-ML rules are processed?
I am fine if you don't add it for now, but maybe just a TODO or NOTE above this as a reminder for down the road?
There was a problem hiding this comment.
I believe this one was unguarded previously because the prepackaged rule still has to be enabled after installation.
This PR is meant to prevent non-ML admins from creating/editing enabled ML rules. I could go either way on this one, but my inclination is just to guard against situations where a non-ML admin could break the execution of an ML Rule. If in the future alerting gives us a way to "persist" the rule creators permissions, most of these guards will become unnecessary (and in their current form would even break the use case craig described above).
Within patch it does call Optionally, we could pull the Just a suggestion ... Up to you on it. Not picky on it with this PR. |
FrankHassanabad
approved these changes
May 8, 2020
There was a problem hiding this comment.
Looked it over, did not do in-depth testing with the different roles and permutations but I did do testing using my platinum license and regular user account and everything looks to function correctly.
Added some unit tests and played with the code and the request caching looks good as well. Going to go ahead and approve.
If we somehow generate an empty message string, the validation should fail as we were attempting to assign _something_ as a failure message.
valid: message !== null was the opposite of what I wanted; a validation is valid if it has no message (i.e. it's undefined).
This required refactoring patchRules to accept the rule to be patched, so that we can check its attributes before performing the update.
patchRules no longer does the fetch; we need to perform this ourselves.
This notably synchronizes the entirety of the updates, as our tests were failing due to the asynchronous nature of the updates.
Instead of fetching the rule within patchRules, we now pass it in.
|
Reviewed again and still 👍 |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
May 11, 2020
…c#65583) * Move common ML types and functions into siem/common These revolve around capabilities/permissions checks which were previously only used on the client. Now that we have need to make similar checks on the server, we can/should move these up to be shared. * Use ML's Capabilities type in lieu of our own There was already some drift between these types preventing our helpers from being used with the ML services; this will prevent further drift. * Add authorization helpers for ML Some of this responsibility will move to ML services in the near future, but for now we still need to restrict SIEM users from performing certain actions. * Use mlAuthz on our import rule route The tests were a little tricky because the use of spyOn/clear was preventing (rather, clearing the mocks from) the use of jest.mock(). I found a workaround with mockRestore(), which was easy to verify because the mock would throw an error if it wasn't removed, and we'd import multiple rules if a default mock was used. The threading through of ML can go away if/when ML adds their services to the request handler context. * Add mlAuthz checks to remaining rule routes * Remove validateLicenseForRuleType This is now unused and redundant with the mlAuthz module. * Fix failing tests These were missed when the helpers were moved to common/, but are also unneeded. * Cleanup: fixing type errors * Clean up some types from ML A recent upstream refactor in ML added top-level exports; this uses them where possible. * Refactor mlAuthz to defer authz validation until validator is called This prevents us from unnecessarily calling ml services if e.g. we're not dealing with an ML rule. This also adds a failing test for the next-to-be-implemented feature: cashing the async validation for subsequent validator calls. * Cache validation promise The purpose of the `buildMlAuthz` function is to store state (request, license, ml). Since `validateMlAuthz` should be idempotent for the duration of this object's lifecycle, we should cache the result the first time we call it; this is effectively memoization since the arguments do not change. * Make our result caching more explicit Extracts a caching helper function. * Add additional unit tests around some edge cases This is the best form of documentation, thanks Frank! * Remove redundant test setup * Empty messages are invalid If we somehow generate an empty message string, the validation should fail as we were attempting to assign _something_ as a failure message. * Fix validity logic valid: message !== null was the opposite of what I wanted; a validation is valid if it has no message (i.e. it's undefined). * Prevent patching of ML rules by non-ML admins This required refactoring patchRules to accept the rule to be patched, so that we can check its attributes before performing the update. * Fix our update_prepackaged_rules route patchRules no longer does the fetch; we need to perform this ourselves. * Fix update_prepackaged_rules tests This notably synchronizes the entirety of the updates, as our tests were failing due to the asynchronous nature of the updates. * Remove id and ruleId from patchRules parameters Instead of fetching the rule within patchRules, we now pass it in. Co-authored-by: Elastic Machine <[email protected]>
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
May 11, 2020
…c#65583) * Move common ML types and functions into siem/common These revolve around capabilities/permissions checks which were previously only used on the client. Now that we have need to make similar checks on the server, we can/should move these up to be shared. * Use ML's Capabilities type in lieu of our own There was already some drift between these types preventing our helpers from being used with the ML services; this will prevent further drift. * Add authorization helpers for ML Some of this responsibility will move to ML services in the near future, but for now we still need to restrict SIEM users from performing certain actions. * Use mlAuthz on our import rule route The tests were a little tricky because the use of spyOn/clear was preventing (rather, clearing the mocks from) the use of jest.mock(). I found a workaround with mockRestore(), which was easy to verify because the mock would throw an error if it wasn't removed, and we'd import multiple rules if a default mock was used. The threading through of ML can go away if/when ML adds their services to the request handler context. * Add mlAuthz checks to remaining rule routes * Remove validateLicenseForRuleType This is now unused and redundant with the mlAuthz module. * Fix failing tests These were missed when the helpers were moved to common/, but are also unneeded. * Cleanup: fixing type errors * Clean up some types from ML A recent upstream refactor in ML added top-level exports; this uses them where possible. * Refactor mlAuthz to defer authz validation until validator is called This prevents us from unnecessarily calling ml services if e.g. we're not dealing with an ML rule. This also adds a failing test for the next-to-be-implemented feature: cashing the async validation for subsequent validator calls. * Cache validation promise The purpose of the `buildMlAuthz` function is to store state (request, license, ml). Since `validateMlAuthz` should be idempotent for the duration of this object's lifecycle, we should cache the result the first time we call it; this is effectively memoization since the arguments do not change. * Make our result caching more explicit Extracts a caching helper function. * Add additional unit tests around some edge cases This is the best form of documentation, thanks Frank! * Remove redundant test setup * Empty messages are invalid If we somehow generate an empty message string, the validation should fail as we were attempting to assign _something_ as a failure message. * Fix validity logic valid: message !== null was the opposite of what I wanted; a validation is valid if it has no message (i.e. it's undefined). * Prevent patching of ML rules by non-ML admins This required refactoring patchRules to accept the rule to be patched, so that we can check its attributes before performing the update. * Fix our update_prepackaged_rules route patchRules no longer does the fetch; we need to perform this ourselves. * Fix update_prepackaged_rules tests This notably synchronizes the entirety of the updates, as our tests were failing due to the asynchronous nature of the updates. * Remove id and ruleId from patchRules parameters Instead of fetching the rule within patchRules, we now pass it in. Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # x-pack/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts
rylnd
added a commit
that referenced
this pull request
May 12, 2020
#66102) * Move common ML types and functions into siem/common These revolve around capabilities/permissions checks which were previously only used on the client. Now that we have need to make similar checks on the server, we can/should move these up to be shared. * Use ML's Capabilities type in lieu of our own There was already some drift between these types preventing our helpers from being used with the ML services; this will prevent further drift. * Add authorization helpers for ML Some of this responsibility will move to ML services in the near future, but for now we still need to restrict SIEM users from performing certain actions. * Use mlAuthz on our import rule route The tests were a little tricky because the use of spyOn/clear was preventing (rather, clearing the mocks from) the use of jest.mock(). I found a workaround with mockRestore(), which was easy to verify because the mock would throw an error if it wasn't removed, and we'd import multiple rules if a default mock was used. The threading through of ML can go away if/when ML adds their services to the request handler context. * Add mlAuthz checks to remaining rule routes * Remove validateLicenseForRuleType This is now unused and redundant with the mlAuthz module. * Fix failing tests These were missed when the helpers were moved to common/, but are also unneeded. * Cleanup: fixing type errors * Clean up some types from ML A recent upstream refactor in ML added top-level exports; this uses them where possible. * Refactor mlAuthz to defer authz validation until validator is called This prevents us from unnecessarily calling ml services if e.g. we're not dealing with an ML rule. This also adds a failing test for the next-to-be-implemented feature: cashing the async validation for subsequent validator calls. * Cache validation promise The purpose of the `buildMlAuthz` function is to store state (request, license, ml). Since `validateMlAuthz` should be idempotent for the duration of this object's lifecycle, we should cache the result the first time we call it; this is effectively memoization since the arguments do not change. * Make our result caching more explicit Extracts a caching helper function. * Add additional unit tests around some edge cases This is the best form of documentation, thanks Frank! * Remove redundant test setup * Empty messages are invalid If we somehow generate an empty message string, the validation should fail as we were attempting to assign _something_ as a failure message. * Fix validity logic valid: message !== null was the opposite of what I wanted; a validation is valid if it has no message (i.e. it's undefined). * Prevent patching of ML rules by non-ML admins This required refactoring patchRules to accept the rule to be patched, so that we can check its attributes before performing the update. * Fix our update_prepackaged_rules route patchRules no longer does the fetch; we need to perform this ourselves. * Fix update_prepackaged_rules tests This notably synchronizes the entirety of the updates, as our tests were failing due to the asynchronous nature of the updates. * Remove id and ruleId from patchRules parameters Instead of fetching the rule within patchRules, we now pass it in. Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
rylnd
added a commit
that referenced
this pull request
May 12, 2020
…65583) (#66104) * [SIEM][Detections] Restrict ML rule modification to ML Admins (#65583) * Move common ML types and functions into siem/common These revolve around capabilities/permissions checks which were previously only used on the client. Now that we have need to make similar checks on the server, we can/should move these up to be shared. * Use ML's Capabilities type in lieu of our own There was already some drift between these types preventing our helpers from being used with the ML services; this will prevent further drift. * Add authorization helpers for ML Some of this responsibility will move to ML services in the near future, but for now we still need to restrict SIEM users from performing certain actions. * Use mlAuthz on our import rule route The tests were a little tricky because the use of spyOn/clear was preventing (rather, clearing the mocks from) the use of jest.mock(). I found a workaround with mockRestore(), which was easy to verify because the mock would throw an error if it wasn't removed, and we'd import multiple rules if a default mock was used. The threading through of ML can go away if/when ML adds their services to the request handler context. * Add mlAuthz checks to remaining rule routes * Remove validateLicenseForRuleType This is now unused and redundant with the mlAuthz module. * Fix failing tests These were missed when the helpers were moved to common/, but are also unneeded. * Cleanup: fixing type errors * Clean up some types from ML A recent upstream refactor in ML added top-level exports; this uses them where possible. * Refactor mlAuthz to defer authz validation until validator is called This prevents us from unnecessarily calling ml services if e.g. we're not dealing with an ML rule. This also adds a failing test for the next-to-be-implemented feature: cashing the async validation for subsequent validator calls. * Cache validation promise The purpose of the `buildMlAuthz` function is to store state (request, license, ml). Since `validateMlAuthz` should be idempotent for the duration of this object's lifecycle, we should cache the result the first time we call it; this is effectively memoization since the arguments do not change. * Make our result caching more explicit Extracts a caching helper function. * Add additional unit tests around some edge cases This is the best form of documentation, thanks Frank! * Remove redundant test setup * Empty messages are invalid If we somehow generate an empty message string, the validation should fail as we were attempting to assign _something_ as a failure message. * Fix validity logic valid: message !== null was the opposite of what I wanted; a validation is valid if it has no message (i.e. it's undefined). * Prevent patching of ML rules by non-ML admins This required refactoring patchRules to accept the rule to be patched, so that we can check its attributes before performing the update. * Fix our update_prepackaged_rules route patchRules no longer does the fetch; we need to perform this ourselves. * Fix update_prepackaged_rules tests This notably synchronizes the entirety of the updates, as our tests were failing due to the asynchronous nature of the updates. * Remove id and ruleId from patchRules parameters Instead of fetching the rule within patchRules, we now pass it in. Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # x-pack/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts * Fix ML type imports This is a backport where we do not have the consolidated ML exports. * Add missing argument to patchRules This was missed in the cherry-pick/backport.
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
May 12, 2020
* master: (46 commits) [Drilldowns][chore] Remove some any's from components. Remove `PlaceContext` from components (elastic#65854) [functional/services] import By/until from module (elastic#66015) [Drilldowns][IE] fix welcome bar layout in IE (elastic#65676) Inspect action shows on dashboard for every chart (elastic#65998) Fix heigt calc in calc issue for ie11 (elastic#66010) [Flights] Delay Bucket - Error notification on opening sample visualization (elastic#66028) [SIEM] [Security] unified code structure phase 0 (elastic#65965) [Maps] Organize layers into subfolders (elastic#65513) skip flaky suite (elastic#59849) Cleanup prefill and edit flow. (elastic#66105) Fix major severity service map ring colors (elastic#66124) [DOCS] Improves formatting in action types (elastic#65932) [DOCS] APM Agent config: Setting values must be string (elastic#65875) Change default cert age limit value. (elastic#65918) [DOCS] Removed saved object options (elastic#66072) [SIEM] [Cases] Case API tests (elastic#65777) Add example of of local plugin installation (elastic#65986) skip flaky suite (elastic#65741) [SIEM][Detections] Restrict ML rule modification to ML Admins (elastic#65583) [Reporting/Test] Add Functional test for download CSV (elastic#65401) ...
1 task
MindyRS
added
the
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Because an ML Rule needs ML Admin permissions to execute, and a Rule(Alert)'s permissions are determined by the last person to modify it, we need to prevent non-ML Admins from editing ML Rules so as to prevent them from causing those rules to fail.
Overview of Changes
MlCapabilities) and were preventing integration with ML servicesmlAuthzmodulemlAuthz, which checks:Reviewer notes
There is one situation that is not covered here: patching an existing ML rule. We handle a user trying to convert a rule to an ML rule via PATCH, but if they're modifying other fields that will succeed. We would need to read the rule before patching it to know whether it's an ML rule and prevent this behavior.addressed in e51c8ddmlAuthzwill eventually be offloaded to the ML Plugin itself.Example failure when attempting to import an ML Rule:
Checklist
Delete any items that are not applicable to this PR.
For maintainers