Skip to content
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

chore(events): export parse functions as a go module #4096

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlonZivony
Copy link
Contributor

1. Explain what the PR does

Export the parse functions from Tracee, which would allow signatures to use them.

2. Explain how to test it

3. Other comments

go.mod Outdated
@@ -1,16 +1,15 @@
module github.com/aquasecurity/tracee

go 1.21

toolchain go1.21.5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was recently added by @geyslan to make builds use a known toolchain, why removing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't remove it, just ran go mod tidy.
I will check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geyslan is the toolchain still a requirement?
It is automatically removed by my IDE (Goland).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the PR is still open, I didn't have time to resolve its conflicts yet - mostly because it has to be coordinated with an internal tests bump (types).

@geyslan is the toolchain still a requirement?

I think that it is a sane requirement since we (contributors) will always have the same go version for tests/debugging.

It is automatically removed by my IDE (Goland).

Check if the cli go mod tidy removes it. In my case it doesn't.

image

I suppose that the maximum that go will enforce you is to download (automatically) the toolchain for that specific go.mod project. When you leave tracee folder, go version will be system's.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked it in this PR branch?
Because it is reproduced in my machine with go mod tidy.
My go version is go version go1.21.6 linux/amd64

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So go mod tidy reinserted the toolchain, right?

What is the output of go version in other folder (without a go.mod)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go version go1.21.6 linux/amd64

@@ -179,3 +178,5 @@ require (
gopkg.in/yaml.v3 v3.0.1 // indirect
kernel.org/pub/linux/libs/security/libcap/psx v1.2.68 // indirect
)

replace github.com/aquasecurity/tracee/pkg/events/parsers => ./pkg/events/parsers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only temporary, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, will need follow-up PR to remove it

return nil
}

func parseArgsFDs(event *trace.Event, origTimestamp uint64, fdArgPathMap *bpf.BPFMap) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be part of the parsers package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.
This function name is misleading.
It is not a parsing function, but a resolving function. It uses the real-time info from a BPF map.
Hence, it is part of the eBPF logic in user mode. Not a parsing function of known values.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rename it to getPathFromFD then?

@@ -103,7 +104,7 @@ func (t *Tracee) processNetCapEvent(event *trace.Event) {

// sanity checks

payloadArg := events.GetArg(event, "payload")
payloadArg := parsers.GetArg(event, "payload")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is more intuitive for the GetArg function to be in the events or pipeline package (once we will extract it from the ebpf package) and not under parsers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this concerns the ArgVal function as well (which resides both in the signatures/helpers package and in the events/params package).
I only move the code as it is, without trying to restructure the whole package.
I think all the arguments extraction functions places should be determined in its own PR and discussion. This PR won't be affected by the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add that for now both GetArg and SetArgValue are only used by the parsers package.

@@ -287,7 +288,7 @@ func (t *Tracee) processHookedProcFops(event *trace.Event) error {
}
hookedFops = append(hookedFops, trace.HookedSymbolData{SymbolName: functionName, ModuleOwner: hookingFunction.Owner})
}
err = events.SetArgValue(event, hookedFopsPointersArgName, hookedFops)
err = parsers.SetArgValue(event, hookedFopsPointersArgName, hookedFops)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -8,9 +8,8 @@ import (
"strings"
"sync/atomic"

"github.com/moby/moby/pkg/parsers/kernel"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this external package and not use our own environment package?
If it's because it is not exposed as a module - then maybe we should

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is mainly because of the external module issue.
However, we coupled that environment into Tracee by using the logger and errfmt packages there.
As the function only use the kernel version and not other OS information, I determined that it will be easier to use the version as a string. As I needed to parse it I thought that using such a known third-party package might be easier than trying to export many parts of the code as modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlonZivony do you want me to move environment as its own module? I think we should since it's required by different projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do.
I think we might also want to put the logger in another module, but this way we divide the project to too many pieces.
For now I think using this external library is good enough.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case let's take the logger and err handling out of the environment package (it should be only a few lines to change).
It doesn't make sense to use an external package for logic we already have (actually, we need to do the opposite - make as less dependencies on external packages as possible)

Comment on lines -38 to +33
switch ID(event.EventID) {
case MemProtAlert:
switch event.EventName {
case "mem_prot_alert":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep event.EventID here and use the existing consts from the events package.
In the future, we should change it to the new EventID enum, exported as part of the new proto API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use the events.ID values because we can't import the events package.
If this package is imported by the signatures, and the signatures are used by Tracee, then it will cause a circular dependency to import Tracee for the events IDs.
The only solution is to have an external package that will translate the IDs to an informative value. I think that we agreed that it will be possible once we use the new event's structure and the protobuf values could be used.

Comment on lines +52 to +64
// TODO: Add support for syscall id to name parsing
// case "sys_enter", "sys_exit":
// if syscallArg := GetArg(event, "syscall"); syscallArg != nil {
// if id, isInt32 := syscallArg.Value.(int32); isInt32 {
// if events.Core.IsDefined(events.ID(id)) {
// eventDefinition := events.Core.GetDefinitionByID(events.ID(id))
// if eventDefinition.IsSyscall() {
// syscallArg.Value = eventDefinition.GetName()
// syscallArg.Type = "string"
// }
// }
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's impossible to translate the syscall ID to its name in a small effort way which will support both AMD64 and ARM64.
As using the events package is not possible, for now I determined that this was out of scope to support this parsing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a regression.
I think we should merge this PR only after introducing the new pipeline event structure (to be done in the coming milestone)

@AlonZivony AlonZivony force-pushed the chore/move-parse-to-new-module branch from 0ac73f4 to 5643a7d Compare June 4, 2024 13:46
pkg/events/parsers/go.mod Outdated Show resolved Hide resolved
@@ -8,9 +8,8 @@ import (
"strings"
"sync/atomic"

"github.com/moby/moby/pkg/parsers/kernel"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlonZivony do you want me to move environment as its own module? I think we should since it's required by different projects.

@AlonZivony AlonZivony force-pushed the chore/move-parse-to-new-module branch 4 times, most recently from 794c93b to 29b6551 Compare June 9, 2024 13:26
@geyslan
Copy link
Member

geyslan commented Aug 23, 2024

@yanivagman I suppose #4129 removed some blocker of this PR. Anyway, is this still relevant? Converting it to draft to clear filters out.

@geyslan geyslan marked this pull request as draft August 23, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants