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

AlonZivony · 2024-06-02T13:36:44Z

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

yanivagman · 2024-06-02T18:41:47Z

go.mod

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

-go 1.21
-
-toolchain go1.21.5


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

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

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

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.

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.

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

So go mod tidy reinserted the toolchain, right?

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

go version go1.21.6 linux/amd64

yanivagman · 2024-06-02T18:42:19Z

go.mod

@@ -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


This is only temporary, right?

Yea, will need follow-up PR to remove it

yanivagman · 2024-06-02T18:44:53Z

pkg/ebpf/events_pipeline.go

+	return nil
+}
+
+func parseArgsFDs(event *trace.Event, origTimestamp uint64, fdArgPathMap *bpf.BPFMap) error {


Shouldn't it be part of the parsers package?

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.

Can you please rename it to getPathFromFD then?

yanivagman · 2024-06-02T18:46:48Z

pkg/ebpf/net_capture.go

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

 		// sanity checks

-		payloadArg := events.GetArg(event, "payload")
+		payloadArg := parsers.GetArg(event, "payload")


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

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.

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

yanivagman · 2024-06-02T18:48:41Z

pkg/ebpf/processor_funcs.go

@@ -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)


yanivagman · 2024-06-02T18:49:34Z

pkg/events/parsers/data_parsers.go

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

+	"github.com/moby/moby/pkg/parsers/kernel"


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

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.

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

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.

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)

yanivagman · 2024-06-02T18:58:00Z

pkg/events/parsers/parse_args.go

-	switch ID(event.EventID) {
-	case MemProtAlert:
+	switch event.EventName {
+	case "mem_prot_alert":


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.

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.

yanivagman · 2024-06-02T18:58:25Z

pkg/events/parsers/parse_args.go

+		// 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"
+	// 				}
+	// 			}
+	// 		}
+	// 	}


Why did you remove this?

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.

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)

pkg/events/parsers/go.mod

geyslan · 2024-06-06T19:44:19Z

pkg/events/parsers/data_parsers.go

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

+	"github.com/moby/moby/pkg/parsers/kernel"


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

geyslan · 2024-08-23T13:08:12Z

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

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

29b6551

AlonZivony requested a review from yanivagman June 2, 2024 13:36

github-actions bot added area/ebpf area/testing area/events area/build labels Jun 2, 2024

github-actions bot assigned AlonZivony Jun 2, 2024

AlonZivony force-pushed the chore/move-parse-to-new-module branch from d47532f to 0ac73f4 Compare June 2, 2024 13:38

yanivagman reviewed Jun 2, 2024

View reviewed changes

AlonZivony force-pushed the chore/move-parse-to-new-module branch from 0ac73f4 to 5643a7d Compare June 4, 2024 13:46

geyslan reviewed Jun 6, 2024

View reviewed changes

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 mentioned this pull request Jun 10, 2024

Remove dependencies of pkg/utils/environment #4111

Closed

geyslan marked this pull request as draft August 23, 2024 13:08

geyslan added the milestone/v0.23.0 label Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AlonZivony commented Jun 2, 2024

yanivagman Jun 2, 2024

AlonZivony Jun 3, 2024

AlonZivony Jun 3, 2024

geyslan Jun 3, 2024

AlonZivony Jun 4, 2024

AlonZivony Jun 4, 2024

geyslan Jun 6, 2024

AlonZivony Jun 9, 2024

yanivagman Jun 2, 2024

AlonZivony Jun 3, 2024

yanivagman Jun 2, 2024

AlonZivony Jun 3, 2024

yanivagman Jun 9, 2024

yanivagman Jun 2, 2024

AlonZivony Jun 3, 2024

AlonZivony Jun 3, 2024

yanivagman Jun 2, 2024

yanivagman Jun 2, 2024

AlonZivony Jun 3, 2024

geyslan Jun 6, 2024

AlonZivony Jun 9, 2024

yanivagman Jun 9, 2024

yanivagman Jun 2, 2024

AlonZivony Jun 3, 2024

yanivagman Jun 2, 2024

AlonZivony Jun 3, 2024

yanivagman Jun 9, 2024

geyslan Jun 6, 2024

geyslan commented Aug 23, 2024 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

AlonZivony commented Jun 2, 2024

1. Explain what the PR does

2. Explain how to test it

3. Other comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geyslan commented Aug 23, 2024 • edited Loading

geyslan commented Aug 23, 2024 •

edited

Loading