-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[SIEM][Detection Engine] Adds ecs threat properties to rules #51782
Conversation
Pinging @elastic/siem (Team:SIEM) |
], | ||
}).error | ||
).toBeTruthy(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add one or two unit tests to the update
tests, we might be getting to the point where we should possibly break this test file up into small test files but doesn't have to be this PR. It could be one later.
@@ -165,6 +191,7 @@ export const updateRulesSchema = Joi.object({ | |||
tags, | |||
to, | |||
type, | |||
threats: threats.optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just have threats
without having to put in optional()
since its default state is optional.
@@ -110,6 +135,7 @@ export const createRulesSchema = Joi.object({ | |||
tags: tags.default([]), | |||
to: to.required(), | |||
type: type.required(), | |||
threats, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: I would just default this on the way in as an empty array if it's not set like so:
threats: threats.default([])
Will make the typing and code easier. It's at least what I have done for references
below and tags
above when the user doesn't push anything in, I just default them to []
Update: If you require something something other than an empty then this is great and probably better than the ones I have and I should update mine at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I can default to an empty array and not throw any errors 👍 Will update this too.
@@ -1755,7 +1758,7 @@ | |||
"ignore_above": 1024, | |||
"type": "keyword" | |||
}, | |||
"threat": { | |||
"threats": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is directly from:
https://github.com/elastic/ecs/blob/master/generated/elasticsearch/7/template.json#L1759
And should be threat
as this part of the mapping is still part of the copying mechanism.
You don't want to change this to the word threats
here.
@@ -82,6 +82,9 @@ | |||
"tags": { | |||
"type": "keyword" | |||
}, | |||
"threats": { | |||
"type": "object" | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update:
I think really we should try to use a nested mapping here?
https://www.elastic.co/guide/en/elasticsearch/reference/current/nested.html
I haven't used one before and don't know how it works with KQL, etc... but I think that would be the best way to support indexing and KQL.
edit: Most of this below is wrong, I put strike outs where I could:
This part here should be directly from ECS rather than object so we get KQL and indexing support.
So you want this to be:
edit: Ignore this below:
"threat": {
"properties": {
"framework": {
"type": "keyword"
},
"tactic": {
"properties": {
"id": {
"type": "keyword"
},
"name": {
"type": "keyword"
},
"reference": {
"type": "keyword"
}
}
},
"technique": {
"properties": {
"id": {
"type": "keyword"
},
"name": {
"type": "keyword"
},
"reference": {
"type": "keyword"
}
}
}
}
},
edit: This is still a valid thing I think we want to do before shipping:
Before shipping we will probably change this file to be two different parts. One JSON which is directly from the ECS generated mapping as-is and then a TypeScript file that imports it and adds the extra signal mapping in as that will be more maintainable and upgrades should be simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without nesting this seems to work ok, so I am fine with it where it is at unless we see other issues.
@@ -44,6 +57,7 @@ export interface RuleAlertParams { | |||
severity: string; | |||
tags: string[]; | |||
to: string; | |||
threats: ThreatParams[] | undefined | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per comments below and how ECS
labels it as threat
I would keep it at threat
even though yes it is indeed multiple threats.
Actually this should be fine as threats
, I updated my comments below. I am wrong here as this is different than the ECS threats, this is a threat from a rule which would probably be a nested object.
💚 Build Succeeded |
…onal from update rules schema as it is implied, updates and adds relevant tests
a9aa95c
to
3466317
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with this example:
{
"rule_id": "rule-threat",
"risk_score": 1,
"description": "Detecting root and admin users",
"interval": "5m",
"name": "Detect Root/Admin Users",
"severity": "high",
"type": "query",
"from": "now-6m",
"to": "now",
"query": "user.name: root or user.name: admin",
"language": "kuery",
"references": ["http://www.example.com", "https://ww.example.com"],
"threats": [
{
"framework": "MITRE ATT&CK",
"tactic": {
"id": "TA0040",
"name": "impact",
"reference": "https://attack.mitre.org/tactics/TA0040/"
},
"technique": {
"id": "T1499",
"name": "endpoint denial of service",
"reference": "https://attack.mitre.org/techniques/T1499/"
}
},
{
"framework": "MITRE ATT&CK",
"tactic": {
"id": "T1020",
"name": "Automated Exfiltration",
"reference": "https://attack.mitre.org/techniques/T1020/"
},
"technique": {
"id": "T1002",
"name": "Data Compressed",
"reference": "https://attack.mitre.org/techniques/T1002/"
}
}
]
}
and everything works as expected. Optional: just add an example for us to use and test with and I think this is LGTM! 🦃 🦃 🦃 :-)
💚 Build Succeeded |
…#51782) * allows addition of ecs threat properties to rules and signals for mitre attack info * adds default empty array to threats on creation of rule, removes optional from update rules schema as it is implied, updates and adds relevant tests * adds sample rule with mitre attack threats property
…#51827) * allows addition of ecs threat properties to rules and signals for mitre attack info * adds default empty array to threats on creation of rule, removes optional from update rules schema as it is implied, updates and adds relevant tests * adds sample rule with mitre attack threats property
* upstream/7.x: Fix infinite redirect loop when multiple cookies are sent (elastic#50452) (elastic#51821) [Console] Proxy fallback (elastic#50185) (elastic#51814) Added endgame-* index and new heading 3 Elastic Endpoint SMP. (elastic#51071) (elastic#51828) [Maps] Added options to disable zoom, hide tool tips, widgets/overlays in embeddable maps (elastic#50663) (elastic#51811) Move errors and validate index pattern ⇒ NP (elastic#51805) (elastic#51831) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) (elastic#51827) [Lens] Remove client-side reference to server source code (elastic#51763) (elastic#51825) fixes drag and drop in tests (elastic#51806) (elastic#51813) [Uptime] Redesign/44541 new monitor list expanded row (elastic#46567) (elastic#51809) [7.x] [Telemetry] collector set to np (elastic#51618) (elastic#51787) [Uptime] added test for chart wrapper (elastic#50399) (elastic#51808) Expressions service fixes: better error and loading states handling (elastic#51183) (elastic#51800) Query String(Bar) Input - cleanup (elastic#51598) (elastic#51804) [ML] Adjust and re-enable categorization advanced wizard test (elastic#51005) (elastic#51017) fixes url state tests (elastic#51746) (elastic#51798) fixes browser field tests (elastic#51738) (elastic#51799) [Task Manager] Tests for the ability to run tasks of varying durations in parallel (elastic#51572) (elastic#51701) [ML] Fix anomaly detection test suite (elastic#51712) (elastic#51795) [SIEM] Fix Timeline drag and drop behavior (elastic#51558) (elastic#51793)
…ra/kibana into IS-46410_remove-@kbn/ui-framework * 'IS-46410_remove-@kbn/ui-framework' of github.com:mbondyra/kibana: (49 commits) [ML] Re-activate after method in transform test (elastic#51815) [SIEM] [Detection Engine] Add edit on rule creation (elastic#51670) De-angularize visLegend (elastic#50613) [SIEM][Detection Engine] Change security model to use SIEM permissions [Monitoring] Sass cleanup (elastic#51100) Move errors and validate index pattern ⇒ NP (elastic#51805) fixes pagination tests (elastic#51822) Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository (elastic#48882) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) [Lens] Remove client-side reference to server source code (elastic#51763) Fix infinite redirect loop when multiple cookies are sent (elastic#50452) fixes drag and drop in tests (elastic#51806) [Console] Proxy fallback (elastic#50185) Query String(Bar) Input - cleanup (elastic#51598) shim visualizations plugin (elastic#50624) Expressions service fixes: better error and loading states handling (elastic#51183) fixes url state tests (elastic#51746) fixes browser field tests (elastic#51738) [ML] Fix anomaly detection test suite (elastic#51712) [SIEM] Fix Timeline drag and drop behavior (elastic#51558) ...
…license-management * 'master' of github.com:elastic/kibana: (48 commits) Enable alerting and actions plugin by default (elastic#51254) Fix error returned when creating an alert with ES security disabled (elastic#51639) [Discover] Improve Percy functional tests (elastic#51699) fixes timeline data providers tests (elastic#51862) [Dependencies]: upgrade react to latest v16.12.0 (elastic#51145) Allow routes to define some payload config values (elastic#50783) Move saved queries service + language switcher ⇒ NP (elastic#51812) [ML] Re-activate after method in transform test (elastic#51815) [SIEM] [Detection Engine] Add edit on rule creation (elastic#51670) De-angularize visLegend (elastic#50613) [SIEM][Detection Engine] Change security model to use SIEM permissions [Monitoring] Sass cleanup (elastic#51100) Move errors and validate index pattern ⇒ NP (elastic#51805) fixes pagination tests (elastic#51822) Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository (elastic#48882) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) [Lens] Remove client-side reference to server source code (elastic#51763) Fix infinite redirect loop when multiple cookies are sent (elastic#50452) fixes drag and drop in tests (elastic#51806) [Console] Proxy fallback (elastic#50185) ...
…#51782) * allows addition of ecs threat properties to rules and signals for mitre attack info * adds default empty array to threats on creation of rule, removes optional from update rules schema as it is implied, updates and adds relevant tests * adds sample rule with mitre attack threats property
Summary
Allows addition of ecs threat properties for mitre attack info to rules / signals.
Summarize your PR. If it involves visual changes include a screenshot or gif.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers