-
Notifications
You must be signed in to change notification settings - Fork 387
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
[Darktrace] Initial Release for the Darktrace #4001
Conversation
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
🌐 Coverage report
|
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.
Query: There are a lot of object arrays in the documents produced here under the darktrace key path. Is this a concern?
Hey @efd6, Yes the Darktrace API sends the data in such a way. And We can't flatten in as it would result in conflicts in index mappings. So that doesn't seem to be a good idea to flatten it. |
Thanks |
@andrewkroh @efd6 could we prioritise review on this integration next please? Thanks! |
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 am a bit concerned about the amount of arrays in the resulting data, but I don't feel there is any good way around it, since creating single documents for each would simply be too many.
I presume there has not been any big challenges in still using them for visualizations?
I am also a bit unsure how this impacts possibility to create SIEM rules for certain content, seeing as they are all stored in arrays.
If this is something that has already been discussed/thought about, feel free to ignore that.
Outside of that I added a few small comments, the rest seems good to go, great work! :)
@@ -0,0 +1,13 @@ | |||
rules: | |||
- path: /modelbreaches | |||
methods: ["GET"] |
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.
Does this API use any sort of authentication? Any way we could add that in as well?
@@ -0,0 +1,48 @@ | |||
config_version: 2 | |||
interval: {{interval}} | |||
request.timeout: 5m |
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.
Could this be a configurable advanced option which defaults to 5min? We usually allow users to override this in some niche usecases.
@@ -0,0 +1,51 @@ | |||
config_version: 2 | |||
interval: {{interval}} | |||
request.timeout: 5m |
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.
Same as above, please make this configurable
I think maybe we have to discuss the duplicate custom fields, as has been brought up in some other PR comments as well |
/test |
🚀 Benchmarks reportTo see the full report comment with |
What does this PR do?
Integration release checklist
This checklist is intended for integrations maintainers to ensure consistency
when creating or updating a Package, Module or Dataset for an Integration.
All changes
New Package
Dashboards changes
Log dataset changes
How to test this PR locally
Related issues
Screenshots