-
Notifications
You must be signed in to change notification settings - Fork 474
[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) |
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
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
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 along
with 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 <101238137+muthu-mps@users.noreply.github.com>
💚 Build Succeeded
History
|
|
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.email
Done, whenever the value contains
@
.Suggestion:
azure.signinlogs.properties.service_principal_name
->service.name
Done, whenever it's populated.
Suggestion:
azure.identityprotection.properties.user_principal_name
->user.email
Suggestion:
azure.identityprotection.properties.user_display_name
->user.full_name
Suggestion:
azure.identityprotection.properties.user_id
->user.id
All done. Matched to the handling in the
signinlogs
data stream.Suggestion:
o365.audit.OriginatingDomain
->url.domain
Suggestion:
o365.audit.Application
->process.name
Suggestion:
o365.audit.Sha1
->file.hash.sha1
Suggestion:
o365.audit.Sha256
->file.hash.sha256
Suggestion:
o365.audit.FileExtension
->file.extension
Suggestion:
o365.audit.Parameters.From
->email.from.address
All done. Field defintion and mapping logic added.
Suggestion:
o365.audit.DeviceName
->host.name
Done.
DeviceName
is now the first source forhost.name
and two other sources will be tried if it’s empty.Suggestion:
o365.audit.TargetFilePath
->file.path
Done. Also
FilePath
.Suggestion:
o365.audit.FileSize
->file.size
Done. Field definition for
FileSize
added. New mapping logic triesFileSizeBytes
first and falls back toFileSize
.Suggestion:
o365.audit.Parameters.ForwardAsAttachmentTo
->email.to.address
Suggestion:
o365.audit.Parameters.ForwardTo
->email.to.address
Suggestion:
o365.audit.Parameters.RedirectTo
->email.to.address
All done. Mapping logic added.
Suggested mappings not done:
Suggestion:
o365.audit.UserId
->user.email
No change.
UserId
was already mapped touser.id
here, and set inuser.email
if the value has@
in it, here. Theuser.email
field can get other values ifUserId
is not set.Suggestion:
m365_defender.incident.alerts.evidence.user_account.azure_ad_user_id
->user.id
Suggestion:
m365_defender.incident.alerts.evidence.user_account.user_sid
->user.id
Suggestion:
m365_defender.incident.alerts.evidence.user_account.user_principal_name
->user.email
Not 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_name
Suggestion:
m365_defender.alert.evidence.p2_sender.display_name
->user.full_name
Not done. As above, these are already added to
related.user
. This is the case in both thealert
andincident
data streams.Suggestion:
m365_defender.incident.alerts.evidence.display_name
->user.full_name
Not 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_name
Added this to
related.user
, to match the handling ofazure_ad_user_id
,user_sid
, anduser_principal_name
.Checklist
changelog.yml
file.Related issues