-
Notifications
You must be signed in to change notification settings - Fork 515
[azure,o365,m365_defender] ECS mapping improvements #14085
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
[azure,o365,m365_defender] ECS mapping improvements #14085
Conversation
920c94b to
469ab90
Compare
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
efd6
left a comment
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 if you are happy with the queries?
packages/azure/data_stream/identity_protection/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/incident/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
| - gsub: | ||
| field: file.size | ||
| pattern: ',' | ||
| replacement: '' | ||
| ignore_missing: true |
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.
Are they always localized to non-European thousands separators?
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.
Not sure. One value was provided with the suggestion; "3,818,355".
In the MS documentation* I see:

In existing pipeline test input we have this (it's a different place in the document):
{
"ExchangeMetaData": {
"FileSize": 13405,
...
},
...
}
I think more resilient logic would be:
- convert to string (to add)
- remove non-digits (this gsub modified)
- convert to long (existing)
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.
Also changed the new field definition to keyword.
fae9079 to
e5a7d70
Compare
🚀 Benchmarks reportTo see the full report comment with |
2a59c2f to
3b51ff3
Compare
| - set: | ||
| field: user.email | ||
| copy_from: azure.identityprotection.properties.user_principal_name | ||
| if: "(ctx.azure?.identityprotection?.properties?.user_principal_name ?: '') =~ /^[^@]+@[^@]+$/" |
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'm disinclined to do more than checking for an @; "I am the @ man"@gmail.com is a valid email address per the RFC.
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.
Oh, good to know.
Switched back to the simpler check.
3b51ff3 to
0624b35
Compare
efd6
left a comment
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.
Still LGTM
muthu-mps
left a comment
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.
Let a comment to modify the changelog description.
Reviewed Azure signinlogs, LGTM!
| field: file.size | ||
| copy_from: o365audit.FileSize | ||
| ignore_empty_value: true | ||
| override: false |
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.
Can only one of filesize and filesizebytes can exist ?
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.
The current API documentation includes FileSize, with the description "Size for the file in bytes", so I think these fields have the same meaning.
The FileSizeBytes mapping was added in a community PR, based on production data. I don't see the field in the documentation. It may be that the field name changed or that it only exists in certain cases.
We have a user request to parse FileSize, so I want to prefer that but I to fall back to the original FileSizeBytes parsing in case it does appear with a different version or configuration, or in older data.
| - convert: | ||
| tag: convert_file_size_to_long | ||
| field: file.size | ||
| type: long |
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.
Converting to string before and then to long ?
Could you explain more ?
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.
The idea was to have logic that works for values from two sources:
FileSize: documented as a string, provided sample value of"3,818,355"FileSizeBytes: undocumented, mapped as alongwith no conversion
But on second thought, we can remove the string conversion and limit the removal of non-digit characters to values that are already strings, so I've done that.
Note: there is an issue (elastic/beats#43659, elastic/elasticsearch#128160) with converting numbers of a certain size to long, but both versions have that and it's being fixed in ES.
Co-authored-by: muthu-mps <[email protected]>
💚 Build Succeeded
History
|
|
zmoog
left a comment
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!
|
Package azure - 1.26.0 containing this change is available at https://epr.elastic.co/package/azure/1.26.0/ |
|
Package m365_defender - 3.9.0 containing this change is available at https://epr.elastic.co/package/m365_defender/3.9.0/ |
|
Package o365 - 2.18.0 containing this change is available at https://epr.elastic.co/package/o365/2.18.0/ |




Proposed commit message
Discussion
Can be read commit-by-commit.
Suggested mappings done:
Suggestion:
azure.signinlogs.properties.user_principal_name->user.emailDone, whenever the value contains
@.Suggestion:
azure.signinlogs.properties.service_principal_name->service.nameDone, whenever it's populated.
Suggestion:
azure.identityprotection.properties.user_principal_name->user.emailSuggestion:
azure.identityprotection.properties.user_display_name->user.full_nameSuggestion:
azure.identityprotection.properties.user_id->user.idAll done. Matched to the handling in the
signinlogsdata stream.Suggestion:
o365.audit.OriginatingDomain->url.domainSuggestion:
o365.audit.Application->process.nameSuggestion:
o365.audit.Sha1->file.hash.sha1Suggestion:
o365.audit.Sha256->file.hash.sha256Suggestion:
o365.audit.FileExtension->file.extensionSuggestion:
o365.audit.Parameters.From->email.from.addressAll done. Field defintion and mapping logic added.
Suggestion:
o365.audit.DeviceName->host.nameDone.
DeviceNameis now the first source forhost.nameand two other sources will be tried if it’s empty.Suggestion:
o365.audit.TargetFilePath->file.pathDone. Also
FilePath.Suggestion:
o365.audit.FileSize->file.sizeDone. Field definition for
FileSizeadded. New mapping logic triesFileSizeBytesfirst and falls back toFileSize.Suggestion:
o365.audit.Parameters.ForwardAsAttachmentTo->email.to.addressSuggestion:
o365.audit.Parameters.ForwardTo->email.to.addressSuggestion:
o365.audit.Parameters.RedirectTo->email.to.addressAll done. Mapping logic added.
Suggested mappings not done:
Suggestion:
o365.audit.UserId->user.emailNo change.
UserIdwas already mapped touser.idhere, and set inuser.emailif the value has@in it, here. Theuser.emailfield can get other values ifUserIdis not set.Suggestion:
m365_defender.incident.alerts.evidence.user_account.azure_ad_user_id->user.idSuggestion:
m365_defender.incident.alerts.evidence.user_account.user_sid->user.idSuggestion:
m365_defender.incident.alerts.evidence.user_account.user_principal_name->user.emailNot done. These aren't the user the event relates to. The incident can have related alerts, each with several pieces of related evidence, each with a related user account. These IDs/names are already copied to
related.user, which should facilitate matching in searches and rules. Duplicating this information inuser.*seems inappropriate.Suggestion:
m365_defender.alert.evidence.p1_sender.display_name->user.full_nameSuggestion:
m365_defender.alert.evidence.p2_sender.display_name->user.full_nameNot done. As above, these are already added to
related.user. This is the case in both thealertandincidentdata streams.Suggestion:
m365_defender.incident.alerts.evidence.display_name->user.full_nameNot done. This is not always a user name. For example, it's a group name in this case. This could be revisited if we have the data to know exactly when it's a user.
Suggested mappings done with adjustments:
m365_defender.incident.alerts.evidence.user_account.display_name->user.full_nameAdded this to
related.user, to match the handling ofazure_ad_user_id,user_sid, anduser_principal_name.Checklist
changelog.ymlfile.Related issues