[REFACTOR] Separate probes and add new required field for probe dependencies #1791

NDStrahilevitz · 2022-06-02T12:44:18Z

Initial Checklist

There is an issue describing the need for this PR.
Git log contains summary of the change.
Git log contains motivation and context of the change.
If part of an EPIC, PR git log contains EPIC number.
If part of an EPIC, PR was added to EPIC description.

Description (git log)

This replaces the fix proposed in #1707

Author: Nadav Strahilevitz <[email protected]>
Date:   Thu Jun 2 12:41:49 2022 +0000

    probes: add required dependency field
    
    This is done through the following steps:
    1. Split the probes into a separate module
    2. Probes are handled implicitly through handles
    3. Events have probe dependencies which includes
    a handle and a required field
    4. Attaching failure is handled according to the required field

Fixes: #1622

Type of change

Code refactor (code improvement and/or code removal)

How Has This Been Tested?

Testing is done through the TRC-9 test in a tracee-tester image running on GKE. Some PR jobs were temporarily removed in this PR to test it faster. They will be added back.

Tests passed

When the tests are passed this checkbox will be filled. This PR shouldn't be merged until then.

Final Checklist:

Pick "Bug Fix" or "Feature", delete the other and mark appropriate checks.

I have made corresponding changes to the documentation.
My code follows the style guidelines (C and Go) of this project.
I have performed a self-review of my own code.
I have commented all functions/methods created explaining what they do.
I have commented my code, particularly in hard-to-understand areas.
My changes generate no new warnings.
I have added tests that prove my fix, or feature, is effective.
New and existing unit tests pass locally with my changes.
Any dependent changes have been merged and published before.

Git Log Checklist:

My commits logs have:

Subject starts with "subsystem|file: description".
Do not end the subject line with a period.
Limit the subject line to 50 characters.
Separate subject from body with a blank line.
Use the imperative mood in the subject line.
Wrap the body at 72 characters.
Use the body to explain what and why instead of how.

rafaeldtinoco · 2022-06-02T15:41:33Z

I'm +1 on this if @yanivagman is (as his team will have a bigger impact on this change, by having the probes declared in another file).

yanivagman

I like this change.
I think the probes package should be under the ebpf package.
Also see some other comments I have.

pkg/ebpf/events_definitions.go

yanivagman · 2022-06-02T15:02:20Z

pkg/ebpf/events_definitions.go

@@ -43,7 +32,7 @@ type EventDefinition struct {
 	Name         string
 	Internal     bool
 	Syscall      bool
-	Probes       []probe
+	Probes       []probeDependency


I wonder if we should now move this to Dependencies, WDYT?

I actually had the same thought initially as well, but when I wrote it out it quickly got too nested for my taste, so I opted to keep it this way.
And since almost all events besides the syscall ones have at least one probe, I think probes can get their own special category.

So why not just keeping it "probe"?

I still thought the name fits more, but it looked worse inside the Dependencies struct. I can rename it back if you find it important that it shouldn't be named xDependency if it's not inside Dependencies.

We can keep it outside of dependencies, but I would keep it probe (no hard feelings about that though)

pkg/ebpf/events_definitions.go

pkg/ebpf/tracee.go

pkg/probes/probes.go

NDStrahilevitz · 2022-06-06T06:34:32Z

I like this change. I think the probes package should be under the ebpf package. Also see some other comments I have.

I moved the package since it made the naming less cumbersome. Otherwise it would be namespaced with the events, and some of them have almost the same name.

yanivagman · 2022-06-07T11:37:54Z

I like this change. I think the probes package should be under the ebpf package. Also see some other comments I have.

I moved the package since it made the naming less cumbersome. Otherwise it would be namespaced with the events, and some of them have almost the same name.

Can you give an example?

NDStrahilevitz · 2022-06-07T13:13:45Z

I like this change. I think the probes package should be under the ebpf package. Also see some other comments I have.

I moved the package since it made the naming less cumbersome. Otherwise it would be namespaced with the events, and some of them have almost the same name.

Can you give an example?

Most of the probes have an event equivalent (I was considering writing a quick probe -> event transformation function but it seemed overkill). So as for example i'll list the exceptions:

Filldir64
SecurityFilePermission

There may be a couple more but these are probes that do not have an equivalent event. The other probes have at least one "equivalent" event, and sometimes other events which can depend on them (For example SchedProccessExecEvent depends on both the SchedProcessExec probe and the LoadElfPhdrs probe(sidenote to that: perhaps this probe should be optional for that event)).

yanivagman · 2022-06-07T13:21:44Z

I like this change. I think the probes package should be under the ebpf package. Also see some other comments I have.

I moved the package since it made the naming less cumbersome. Otherwise it would be namespaced with the events, and some of them have almost the same name.

Can you give an example?

Most of the probes have an event equivalent (I was considering writing a quick probe -> event transformation function but it seemed overkill). So as for example i'll list the exceptions:

Filldir64

SecurityFilePermission

There may be a couple more but these are probes that do not have an equivalent event. The other probes have at least one "equivalent" event, and sometimes other events which can depend on them (For example SchedProccessExecEvent depends on both the SchedProcessExec probe and the LoadElfPhdrs probe(sidenote to that: perhaps this probe should be optional for that event)).

But how will this make naming cumbersome if we put the probes package under the ebpf package?

NDStrahilevitz · 2022-06-07T13:24:13Z

I like this change. I think the probes package should be under the ebpf package. Also see some other comments I have.

I moved the package since it made the naming less cumbersome. Otherwise it would be namespaced with the events, and some of them have almost the same name.

Can you give an example?

Most of the probes have an event equivalent (I was considering writing a quick probe -> event transformation function but it seemed overkill). So as for example i'll list the exceptions:

Filldir64

SecurityFilePermission

There may be a couple more but these are probes that do not have an equivalent event. The other probes have at least one "equivalent" event, and sometimes other events which can depend on them (For example SchedProccessExecEvent depends on both the SchedProcessExec probe and the LoadElfPhdrs probe(sidenote to that: perhaps this probe should be optional for that event)).

But how will this make naming cumbersome if we put the probes package under the ebpf package?

Because then we would have many probes and events named almost the same for exmaple

VfsWriteEventID <-> VfsWriteProbe
CgroupMkdirEventID <-> CgroupMkdirProbe

and so on...

IMO, the events should also be split from the ebpf package and so we could remove the whole EventID from all of them, but this isn't in the scope of this PR.

yanivagman · 2022-06-07T13:30:12Z

Because then we would have many probes and events named almost the same for exmaple

WriteEventID <-> WriteProbe CgroupMkdirEventID <-> CgroupMkdirProbe

and so on...

I don't think it's a problem that we have many probes and events named almost the same.

IMO, the events should also be split from the ebpf package and so we could remove the whole EventID from all of them, but this isn't in the scope of this PR.

Agree.

NDStrahilevitz · 2022-06-07T13:39:30Z

I don't think it's a problem that we have many probes and events named almost the same.>

It's not breaking or anything, but I think it makes the code less orderly and readable and the namespace more cluttered.

I also think there's some functional merit to having the probes in their own package, for example in PR #1804 there is some trigger mechanism introduced for the uprobes, this kind of functionality could (and IMO should) be defined in this package.

If we had the event definitions also inside the events package we could have the clear separation like so:

events.VfsWrite and probes.VfsWrite.

Agree.

I'm willing to take this next refactor PR on myself to justify this decision. I think the ebpf package, while an improvement from before is still very cluttered and it could use the cleanup, keeping the probes there doesn't do it any good in that regards. But - maybe for now it could be a subpackage like so ebpf/probes, WDYT?

yanivagman · 2022-06-07T13:45:40Z

But - maybe for now it could be a subpackage like so ebpf/probes, WDYT?

This is exactly what I suggested :-)

I think the probes package should be under the ebpf package.

Maybe I wasn't clear that I meant a subpackage in that sentence

NDStrahilevitz · 2022-06-07T13:48:50Z

But - maybe for now it could be a subpackage like so ebpf/probes, WDYT?

This is exactly what I suggested :-)

I think the probes package should be under the ebpf package.

Maybe I wasn't clear that I meant a subpackage in that sentence

Haha, no I didn't get that from what you said initially. Alright i'll make that change.

This is done through the following steps: 1. Split the probes into a separate module 2. Probes are handled implicitly through handles 3. Events have probe dependencies which includes a handle and a required field 4. Attaching failure is handled according to the required field

yanivagman

LGTM!

NDStrahilevitz force-pushed the refactor/separate-probes-for-required-dependencies branch from e93b7de to 414085b Compare June 2, 2022 12:45

NDStrahilevitz mentioned this pull request Jun 2, 2022

ebpf: add a new required dependency field. #1707

Closed

19 tasks

NDStrahilevitz requested a review from yanivagman June 2, 2022 12:46

rafaeldtinoco added the needs-review label Jun 2, 2022

rafaeldtinoco added the research label Jun 2, 2022

yanivagman requested changes Jun 2, 2022

View reviewed changes

yanivagman added changes-requested and removed needs-review labels Jun 2, 2022

yanivagman mentioned this pull request Jun 5, 2022

Supported Kubernetes Flavours #1022

Closed

6 tasks

NDStrahilevitz force-pushed the refactor/separate-probes-for-required-dependencies branch from 414085b to 3b7501e Compare June 6, 2022 06:41

NDStrahilevitz requested a review from yanivagman June 6, 2022 06:41

NDStrahilevitz force-pushed the refactor/separate-probes-for-required-dependencies branch 2 times, most recently from 2159fd8 to d745f0f Compare June 7, 2022 10:14

NDStrahilevitz force-pushed the refactor/separate-probes-for-required-dependencies branch from 359922d to c6dac8f Compare June 7, 2022 13:22

NDStrahilevitz force-pushed the refactor/separate-probes-for-required-dependencies branch 2 times, most recently from 5129ed2 to 5abc2be Compare June 7, 2022 14:42

NDStrahilevitz force-pushed the refactor/separate-probes-for-required-dependencies branch from 5abc2be to 5bb22be Compare June 7, 2022 14:58

yanivagman approved these changes Jun 7, 2022

View reviewed changes

yanivagman merged commit 69c7765 into aquasecurity:main Jun 7, 2022

yanivagman mentioned this pull request Jun 8, 2022

[BUG] intermittent errors after PR 1202 (drop privileges) #1803

Closed

5 tasks

NDStrahilevitz deleted the refactor/separate-probes-for-required-dependencies branch June 8, 2022 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTOR] Separate probes and add new required field for probe dependencies #1791

[REFACTOR] Separate probes and add new required field for probe dependencies #1791

NDStrahilevitz commented Jun 2, 2022 •

edited

Loading

rafaeldtinoco commented Jun 2, 2022

yanivagman left a comment

yanivagman Jun 2, 2022

NDStrahilevitz Jun 6, 2022

yanivagman Jun 7, 2022

NDStrahilevitz Jun 7, 2022

yanivagman Jun 7, 2022

NDStrahilevitz commented Jun 6, 2022

yanivagman commented Jun 7, 2022

NDStrahilevitz commented Jun 7, 2022

yanivagman commented Jun 7, 2022

NDStrahilevitz commented Jun 7, 2022 •

edited

Loading

yanivagman commented Jun 7, 2022

NDStrahilevitz commented Jun 7, 2022 •

edited

Loading

yanivagman commented Jun 7, 2022

NDStrahilevitz commented Jun 7, 2022

yanivagman left a comment

[REFACTOR] Separate probes and add new required field for probe dependencies #1791

[REFACTOR] Separate probes and add new required field for probe dependencies #1791

Conversation

NDStrahilevitz commented Jun 2, 2022 • edited Loading

Initial Checklist

Description (git log)

Type of change

How Has This Been Tested?

Final Checklist:

Git Log Checklist:

rafaeldtinoco commented Jun 2, 2022

yanivagman left a comment

Choose a reason for hiding this comment

yanivagman Jun 2, 2022

Choose a reason for hiding this comment

NDStrahilevitz Jun 6, 2022

Choose a reason for hiding this comment

yanivagman Jun 7, 2022

Choose a reason for hiding this comment

NDStrahilevitz Jun 7, 2022

Choose a reason for hiding this comment

yanivagman Jun 7, 2022

Choose a reason for hiding this comment

NDStrahilevitz commented Jun 6, 2022

yanivagman commented Jun 7, 2022

NDStrahilevitz commented Jun 7, 2022

yanivagman commented Jun 7, 2022

NDStrahilevitz commented Jun 7, 2022 • edited Loading

yanivagman commented Jun 7, 2022

NDStrahilevitz commented Jun 7, 2022 • edited Loading

yanivagman commented Jun 7, 2022

NDStrahilevitz commented Jun 7, 2022

yanivagman left a comment

Choose a reason for hiding this comment

NDStrahilevitz commented Jun 2, 2022 •

edited

Loading

NDStrahilevitz commented Jun 7, 2022 •

edited

Loading

NDStrahilevitz commented Jun 7, 2022 •

edited

Loading