-
Notifications
You must be signed in to change notification settings - Fork 444
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
[m365_defender] Add support for Incident data-stream with new Security Graph API #4435
[m365_defender] Add support for Incident data-stream with new Security Graph API #4435
Conversation
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
🚀 Benchmarks reportTo see the full report comment with |
🌐 Coverage report
|
@@ -0,0 +1,20 @@ | |||
- name: data_stream.type |
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.
Move the fields to ecs.yml and use external definitions where possible.
data_stream.type
data_stream.dataset
data_stream.namespace
@timestamp
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.
Hey @efd6 ,
Also, there is an agent.yml
which contains an ECS field description. So, should we remove that file and provide those field descriptions in ecs.yml
?
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.
Yeah, that has confused me in the past. @P1llus WDYT?
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.
base-fields.yml
is the correct approach, all integrations currently have those, so we shouldn't move them out of that file, unless there is any specific plans to do this on all integrations.
agent.yml
needs to stay and usually have all the agent fields populated by the agent, so it should be fairly large. If its very small for some reason then feel free to copy it from any of the other integrations.
if (!eventType.isEmpty()) { | ||
ctx.event.type = eventType; | ||
} | ||
if (!eventCategory.isEmpty()) { | ||
ctx.event.category = eventCategory; | ||
} |
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've found that this can be a cause of test failures due to changes in iteration order in the HashSet type. See #4296. The fix is to copy the elements to an array list and then sort it (this is done in the linked PR).
if (!cloudProvider.isEmpty()) { | ||
ctx.cloud.provider = cloudProvider; | ||
} | ||
if (!groupName.isEmpty()) { | ||
ctx.group.name = groupName; | ||
} | ||
if (!hostId.isEmpty()) { | ||
ctx.host.id = hostId; | ||
} | ||
if (!userDomain.isEmpty()) { | ||
ctx.user.domain = userDomain; | ||
} | ||
if (!userId.isEmpty()) { | ||
ctx.user.id = userId; | ||
} | ||
if (!userName.isEmpty()) { | ||
ctx.user.name = userName; | ||
} | ||
if (!userEmail.isEmpty()) { | ||
ctx.user.email = userEmail; | ||
} | ||
if (!processUserId.isEmpty()) { | ||
ctx.process.user.id = processUserId; | ||
} | ||
if (!processUserName.isEmpty()) { | ||
ctx.process.user.name = processUserName; | ||
} |
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 issue here. Given the number of cases a helper function to convert a hash set to a canonically ordered list would probably be best.
if (!fileSize.isEmpty()) { | ||
ctx.file.size = fileSize; | ||
} | ||
if (!processPid.isEmpty()) { | ||
ctx.process.pid = processPid; | ||
} | ||
if (!processParentPid.isEmpty()) { | ||
ctx.process.parent.pid = processParentPid; | ||
} |
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.
And here.
if (!cloudProvider.isEmpty()) { | ||
ctx.cloud.provider = cloudProvider; | ||
} | ||
if (!groupName.isEmpty()) { | ||
ctx.group.name = groupName; | ||
} | ||
if (!hostId.isEmpty()) { | ||
ctx.host.id = hostId; | ||
} | ||
if (!userDomain.isEmpty()) { | ||
ctx.user.domain = userDomain; | ||
} | ||
if (!userId.isEmpty()) { | ||
ctx.user.id = userId; | ||
} | ||
if (!userName.isEmpty()) { | ||
ctx.user.name = userName; | ||
} | ||
if (!userEmail.isEmpty()) { | ||
ctx.user.email = userEmail; | ||
} | ||
if (!processUserId.isEmpty()) { | ||
ctx.process.user.id = processUserId; | ||
} | ||
if (!processUserName.isEmpty()) { | ||
ctx.process.user.name = processUserName; | ||
} |
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.
Ordering.
if (!fileSize.isEmpty()) { | ||
ctx.file.size = fileSize; | ||
} | ||
if (!processPid.isEmpty()) { | ||
ctx.process.pid = processPid; | ||
} | ||
if (!processParentPid.isEmpty()) { | ||
ctx.process.parent.pid = processParentPid; | ||
} |
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.
Ordering.
packages/m365_defender/changelog.yml
Outdated
@@ -1,4 +1,9 @@ | |||
# newer versions go on top | |||
- version: "2.0.0" |
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.
Is there a reason this needs to bump the major version? I don't see any breaking change here.
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.
As discussed, the existing log data stream will be deprecated and will be replaced by a new alert and incident data stream that supports the latest Microsoft Security Graph API.
@P1llus Should we include a note in the readme or do you have other suggestions?
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.
Yeah this is a breaking change and would need a major bump, but I believe there is also a few other things that needs to be discussed.
ctx.cloud.provider = convertToOrderedArray(cloudProvider); | ||
ctx.group.name = convertToOrderedArray(groupName); | ||
ctx.host.id = convertToOrderedArray(hostId); | ||
ctx.user.domain = convertToOrderedArray(userDomain); | ||
ctx.user.id = convertToOrderedArray(userId); | ||
ctx.user.name = convertToOrderedArray(userName); | ||
ctx.user.email = convertToOrderedArray(userEmail); | ||
ctx.process.user.id = convertToOrderedArray(processUserId); | ||
ctx.process.user.name = convertToOrderedArray(processUserName); |
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 these and the similar calls below should still be conditional to avoid fruitless work here and in the empty value cleaning processor.
packages/m365_defender/data_stream/alert/_dev/test/pipeline/test-alert.log
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/alert/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/alert/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/alert/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/alert/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/alert/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
Outside of the comments from @efd6 I have nothing else to add here, it seems good to me 👍 |
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.
LGTM after conflict resolved.
What does this PR do?
Checklist
changelog.yml
file.manifest.yml
file to point to the latest Elastic stack release (e.g.^7.16.0
).How to test this PR locally
Related issues
Screenshots