-
Notifications
You must be signed in to change notification settings - Fork 396
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
New Event Structure #2870
Comments
while this represent the complete event schema, I also think we should try to emit minimal events. For example, I suppose most people won't need the full description with every event, or the full stack trace. We already allow conditionally including some elements (stack trace, syscall, exec-env), and I think we need to expand this control to all event fields. for discussing this I'll create another issue. |
@itaysk Do you think we should maybe use the I've had this thought as well in relation to the recent |
Actually I'm not very familiar with how protocol is used now. I thought it's supposed to become obsolete in the unified binary approach. Can you elaborate on your proposal or perhaps add an example? |
The protocol is more a way to differentiate different payloads going into the rules engine. |
May I suggest we move the event definition to a |
I'm still not sure there's a use case for this, or I didn't fully understand it, but regardless seems like we reached the same conclusion that the "rule engine" is gone now. +1 for proto |
Has the event type serialization, using protobufs, ever been tested? I'm particularly concerned about:
I mean, we will not serialize eBPF events, for sure, but if we're considering converting the Tracee type to protobuf, in a high rate, coming from an external source, then we should do some measurement before taking the decision, IMO. |
I've actually written a PR (#2070) once where I added a ebpf -> rules serialization with protobuf. From my measurement in that PR, protobuf serialization was quicker than both json and gob ONLY if the struct was initially in the protobuf form already - conversion from |
Good to know that. |
I updated the struct to have process related context grouped together (pids, tids, comm, star time, namespaces, cgroup, uid). |
Adding this here for future reference and consideration: https://www.elastic.co/blog/ecs-elastic-common-schema-otel-opentelemetry-faq |
The field changes were causing compatibility issues for integrated products. We will re-introduce these fields as part of the new Context field in the new event structure (see aquasecurity#2870)
The field changes were causing compatibility issues for integrated products. We will re-introduce these fields as part of the new Context field in the new event structure (see aquasecurity#2870)
After talking with @yanivagman we thought about some slight changes:
TBD:
I've updated the description to reflect this |
Let's also take the opportunity to only expose the fields mentioned here to the user. Any other (internal) fields should not be part of the trace.Event. |
More detailed struct with some comments added:
|
why?
agree, but let's discuss in a separate issue as it's about adding an entirely new feature? (and won't break the event structure)
what's the motivation for adding this layer?
+1
since we're discussing the "event structure", having a field in the root implies it's an "event" field. In this case, I'm internally reading this as "event ancestors" but the meaning is "process ancestors". should we relocate it under process or prefix it with process to clarify?
agree, but let's discuss in a separate issue as it's about adding an entirely new feature?
if this is a kubernetes thing, should we add kubernetes to the name? if this is not, are we sure the structure will work for other pod implementation?
What does this mean? from reading the code I think I can gather that it is looking at container lable
This is the only field in the event that is Tracee-specific, and not meaningful by itself. For example, if I'm streaming all events to splunk and then someone else at another time sees this event there, all other field would make sense since they describe what happened, but to understand this field I need to understand trace and how it was configured started. |
Moved it to next release, the event structure is finished and merged, now it is missing integrating it on tracee internals. |
Few comments:
|
The data we need and is not in context currently:
|
This is available in the
@itaysk we should be able to add these through container labels like other kubernetes data we already get in container enrichment. I can open an issue for this if agreed. |
When process tree was being created there was a specific discussion about COMM versus BINARY PATH and where the info should go (https://github.com/aquasecurity/tracee/pull/3364/files#diff-773e2917cb050cc42ce31d36b08db6c9e3da89ab6dff75f8a9b3eba5171316d3R12). Alon and I agreed that COMM would be part of the TaskInfo structure (for the Process or Thread) and the Binary Path would be part of the File Info structure (for the Binary and the Interpreter of the BInary). The COMM (Process Name for the process tree) does not pick the args (its basically procfs comm field), that would come together with argv array (which is an argument for the exec event). We can introduce COMM + ARGS in the process tree if needed (or something else), no problem. |
Hey @mcherny, thank you for the questions:
Do you have an example of how this will be used? Because the Threat information is part of specific even (behaviour events), I'm considering this should be a part of the event definition as optional, and not have its own API. Thoughts?
We didn't add those because they are internal to Aqua, our plan was to actually remove all together for opensource signatures, and let the internal project handle the translation between those ids, and tracee ids.
Agree! Right now the ids are stable for base events but dynamic for signatures, I need to look into how we can make it always stable. |
Assuming we need this on each event that this may be relevant (e.x. file open), how would I consume by alternative means if I need it outside of tracee process, that is at gRPC client? |
In general, if someone needs info on something on each event of its kind, for a particular usecase, a signature/derived event is probably called for. |
That's why we wrote:
There is an advantage to include this information in the OSS project when a threat is detected (not so frequent) so user can get information about the threat without consulting the documentation
In continuation to the first point, this is also static data and can be added to the
Agree. We have an old issue opened for that #1098 |
Sounds like a reasonable addition to tracee that should be optional. We should discuss such additions in a separate issue and keep this issue for event structure to support the already existing features of tracee |
I've been doing some work in the capture area. Considering we want to merge it into the "everything is an event" scheme at some point, shouldn't we also include an |
Eventually capture will be an action taken by some event. In that case, the data about it will be part of policies->actions |
#2355 changed the primary user experience of Tracee to be event oriented (previously events were considered internal and hidden from the user). Therefore:
Following is the updated event schema based on the comments below:
timestamp
name
id
- machine readable id (integer). Note: current event id isn't good since it is architecture specificversion
- use semver where major is a breaking change in the event (e.g. one of the event's fields under data has been changed or removed), minor is a non breaking change (e.g. a new field was added to the event under data) and patch (e.g. a bug fix). Since this data is static, we may remove this or make optionaltags
- since this data is static, we may remove this or make optionallabels
- doesn't exist. For future use.policies
matched
actions
- doesn't exist, for future use - list of actions taken (currently the only action we have is print).context
process
executable
path
name
- the binary name (basename of the path) - doesn't exist, consider adding (in another issue)uniqueId
- unique id of the processpid
hostPid
executionTime
- time of last exec. Doesn't exist, consider adding (in another issue)realUser
id
name
- doesn't exist, consider adding (in another issue)user
- effective user. Doesn't exist, consider adding (in another issue)id
name
ancestors
- process ancestors array. Only direct parent will be populated by default with the following fields:uniqueId
pid
hostPid
thread
startTime
name
(aka "comm")tid
hostTid
capabilities
- doesn't exist, consider adding (in another issue)syscall
- the syscall that triggered this eventcompat
- boolean. moved fromflags.compat
userStackTrace
- if enabled, will be herecontainer
id
name
image
id
repoDigest
name
isRunning
- boolean. moved fromflags
startTime
- Timestamp of container start time. Doesn’t exist. Will replacestarted
pid
- entrypoint's pid. Doesn’t exists, consider addingk8s
pod
name
uid
labels
namespace
name
data
returnValue
(if relevant will appear here)triggeredBy
(will appear on threat detection events)name
id
data
threat
(if relevant will appear here) - static data about threats (can be omitted)description
mitre
tactic
name
technique
name
id
severity
We also discussed versioning the event schema, but not including the version with each event, for efficiency.
The text was updated successfully, but these errors were encountered: