-
Notifications
You must be signed in to change notification settings - Fork 136
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
Email object and Email Activity updates. Deprecate Email URL Activity and Email File Activity. #1259
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Paul Agbabian <[email protected]>
…favor of an updated email_activity class. Updated the email object to include domains, files, urls arrays. Updated the email_activity class to add the message_trace_uid ID. Updated the email_activity class to use the references[] for the Trace activity_id instead of the description URL. Updated the email_activity class description to reflect its SMTP protocol and the possible URLs and files attachments. Signed-off-by: Paul Agbabian <[email protected]>
Signed-off-by: Paul Agbabian <[email protected]>
…on to fail!! Signed-off-by: Paul Agbabian <[email protected]>
…ed an at_least_one constraint on all the to and from attributes. Not all email logs have the 'to' and 'from' but must have at least those or 'smtp_to' and 'smtp_from' in the log. Signed-off-by: Paul Agbabian <[email protected]>
Signed-off-by: Paul Agbabian <[email protected]>
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 great
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 requested changes are only the grammatical things in descriptions. My other comments are just suggestions or observations.
dictionary.json
Outdated
@@ -1781,6 +1781,12 @@ | |||
"type": "domain_contact", | |||
"is_array": true | |||
}, | |||
"domains": { | |||
"caption": "Domains", | |||
"description": "The domains that pertain to the event or 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.
Consistency: The vast majority of descriptions in this dictionary end with a full stop (period). This comment applies to all of the descriptions in this PR.
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.
Good catch.
dictionary.json
Outdated
@@ -1781,6 +1781,12 @@ | |||
"type": "domain_contact", | |||
"is_array": true | |||
}, | |||
"domains": { | |||
"caption": "Domains", | |||
"description": "The domains that pertain to the event or 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.
The word domains is quite general in meaning, e.g. I work in the security domain. Presumably the meaning in our dictionary with be DNS domains. Consider if this attribute would be more appropriately named domain_names
or dns_domains
.
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 fact that domains
is general in meaning might be a good thing, then we have a higher likelihood that we can re-use that dictionary attribute. Maybe we should add See specific usage.
to the description.
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 fact that
domains
is general in meaning might be a good thing, then we have a higher likelihood that we can re-use that dictionary attribute. Maybe we should addSee specific usage.
to the description.
That's a very constructive way of seeing the generality. I like it.
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 fact that
domains
is general in meaning might be a good thing, then we have a higher likelihood that we can re-use that dictionary attribute. Maybe we should addSee specific usage.
to the description.
Agreed on making the dictionary description generalized and using the more specific description where applied. This is a common practice that has worked very well to better future-proof OCSF.
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.
Yes, we've had many discussions on domain, subdomain, path, hostname, suffix etc. domain
is in the dictionary already and in use. We have a subdomain
and subdomains
array and so I want to be consistent with that. Interestingly in URL
we have an example that tries to indicate what we mean by domain
as a string, but it could be a see specific usage
to be more general. In the email class, it should be similar in meaning to the URL
meaning.
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 dictionary description is almost tautological but I added See specific usage.
to both domain
and domains
. Note that the descriptions for subdomain
and subdomains
are more specific, and relate to URLs.
Note that this now creates warnings for about 5 or 6 other classes where domain
is used. I don't want to only do See specific usage.
for domains
which is new, and not do it for domain
but that is the side effect that will need to be addressed. In a separate PR though, as we need to give it some thought as to how to specifically define it for those classes.
@@ -3083,6 +3095,11 @@ | |||
"description": "The description of the event/finding, as defined by the source.", | |||
"type": "string_t" | |||
}, | |||
"message_trace_uid": { | |||
"caption": "Message Trace UID", | |||
"description": "The identifier that tracks a message that travels through multiple points of a messaging service.", |
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.
Would it be helpful to give examples of what this maps to in real email products and/or protocol exchanges? One of the challenges I often have when working with the OCSF schema is that it's not clear to me what exactly is intended to go in a particular attribute.
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.
@davemcatcisco I agree, examples help! @pagbabian-splunk if you choose to add an example to this description, here is one from o365 Message Tracing: MessageTraceId = ae4ad8f6-7613-411c-e67e-08cfc740629
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.
With Adam's proposal we will have a trace object (and profile) and a trace.uid
within it. Are we trying to keep this attribute at the highest level?
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.
Yes, I looked at that first, but I didn't want to use a trace
object for this - in fact if I wanted to be more specific to Microsoft (which I'm trying to avoid), the MessageTraceId
would be the source
and it is directly in the event. Adam's PR is mirroring the OTel Trace
concept which is related but likely its description and definitely usage will be different. MS has their ID looking like a UUID/GUID but I don't want to be that specific given there may be other implementations.
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 going to add Mike's Microsoft link example to a references
for the attribute, although I will say "For example..." so as to not make it authoritative.
@@ -74,6 +74,7 @@ Thankyou! --> | |||
1. Added new `11: Basic Authentication` enum value to `auth_protocol_id`. #1239 | |||
1. Added `values` as an array of `string_t`. #1251 | |||
1. Added `kernel_release` as a `string_t`. | |||
1. Added `domains` `files` `urls` and `message_trace_uid`. #1259 |
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.
Put commas between the attribute names, and optionally put an Oxford comma before the and. This comment applies to other additions to CHANGELOG.md in this PR.
dictionary.json
Outdated
@@ -2083,6 +2089,12 @@ | |||
"description": "The result of the file change. It should contain the new values of the changed attributes.", | |||
"type": "file" | |||
}, | |||
"files": { | |||
"caption": "Files", | |||
"description": "The files that are part of the event or 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.
Nit: Add a period to the dscription.
"message_trace_uid": { | ||
"group": "primary", | ||
"requirement": "recommended" | ||
}, | ||
"smtp_hello": { |
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.
Do we maybe want to make this more generic since we are encapsulating more than just SMTP with this event class? Maybe using command
?
SMTP Commands: HELO, MAIL FROM, RCPT TO, DATA, QUIT, etc. POP3 Commands: USER, PASS, RETR, DELE, QUIT, etc. IMAP Commands: LOGIN, SELECT, FETCH, LOGOUT, etc.
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 only issue I see is that if we make it smtp_command
or just command
we will have a value returned but we won't know which SMTP command it was. If we want to cover all commands, we'd probably want to have an SMTP Command
object and objects for POP3 and IMAP. Or a single object such as Email Command
that includes an enum that describes what the command is. Please think about that @Aniak5 and we can have a separate PR for it, but I'll just leave the current smtp_hello
intact for now.
@@ -2,7 +2,7 @@ | |||
"uid": 9, | |||
"caption": "Email Activity", | |||
"category": "network", | |||
"description": "Email events report activities of emails.", | |||
"description": "Email Activity events report SMTP protocol and email activities including those with embedded URLs and files. See the <code>Email</code> object for details.", | |||
"extends": "base_event", | |||
"name": "email_activity", | |||
"attributes": { |
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.
We should probably add a field to represent protocol
so we can specify SMTP
, POP3
, IMAP
, etc. We could either create a new protocol
field or change the description of protocol_name
in the dictionary and add See specific usage.
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's do this at the same time as figuring out the SMTP Command
approach since they are related, and we haven't really had much other discussion about it.
Good suggestions all - I will update and convert from DRAFT. |
…ific usage.` for the domain and domains attributes in the dictionary. Added another references[] to the dictionary definition of message_trace_uid. Updated O365 to Office 365 in email.json Trace references[] to be consistent. Signed-off-by: Paul Agbabian <[email protected]>
Signed-off-by: Paul Agbabian <[email protected]>
Related Issue: N/A
Description of changes:
Added
domains
files
urls
andmessage_trace_uid
to the dictionary for use with theemail
object andemail_activity
class.Deprecated the two other classes in favor of a single
email_activity
class where theemail
object can also hold files domains and urls.