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

Various cgroup and mounting fixes and optimizations #3829

Merged
merged 4 commits into from
Feb 7, 2024

Conversation

NDStrahilevitz
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz commented Jan 30, 2024

1. What the PR does

ae406df optimize(enrich): skip non hid mkdir events
9d97d23 chore: improve logging in container related code
96f0c90 fix: existing_container bad output
0433158 feat(mount): use mountinfo instead of /proc/mounts
9d2206c feat(cgroup): always switch to root cgroupns

2. Explain how to test it

  1. Deploy an EKS environment (v1.27+)
  2. Deploy tracee with a rule for tracking container_create
  3. Deploy a new pod
  4. The two container_create events should be enriched (barring some race conditions)

3. Other comments

I've found that we have some race conditions right now where enrichment will finish but a container event will be sent before. This is another bug which should be tackled separately (either by fixing the condition itself, or changing how a container_create event is triggered).

Resolve #3826
Fix #3689
Fix #3824

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM overall. I put some questions.

pkg/ebpf/events_enrich.go Outdated Show resolved Hide resolved
pkg/ebpf/events_enrich.go Show resolved Hide resolved
pkg/ebpf/events_enrich.go Show resolved Hide resolved
pkg/ebpf/events_enrich.go Outdated Show resolved Hide resolved
pkg/ebpf/tracee.go Show resolved Hide resolved
pkg/events/usermode.go Show resolved Hide resolved
pkg/mount/mount.go Show resolved Hide resolved
pkg/cgroup/cgroup.go Outdated Show resolved Hide resolved
pkg/cgroup/cgroup.go Outdated Show resolved Hide resolved
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

Approving for when ready.

@NDStrahilevitz
Copy link
Collaborator Author

@yanivagman Do you want to review this before I fix and merge?

Copy link
Collaborator

@yanivagman yanivagman left a comment

Choose a reason for hiding this comment

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

LGTM after the suggested changes

@NDStrahilevitz NDStrahilevitz force-pushed the switch_cgroup_ns branch 2 times, most recently from 43a7498 to a877430 Compare February 7, 2024 15:01
pkg/mount/mount.go Outdated Show resolved Hide resolved
/proc/mounts file does not tell where the mount originates from. This
could cause us to use an unmanaged mount which is actually pointing to
a purposfully mounted internal container cgroup fs. Since we always want
to rely on host info, use the /proc/self/mountinfo file, which lets us
know which filesystem on the root was actually mounted.

For the reasons above, the MountOnce struct was refactored to the
MountHostOnce struct, which does the same job but explicitly searches
for mounts originating in the host filesystem.
existing_container event had one wrong argument metadata.
In addition, iteration on the event set to send was done in a range.
As such the event reference would switch mid send, causing duplicate
existing_container events.
Enrich should only be done on relevant cgroup events. Those not in the
relevant HID can be sent out.
@NDStrahilevitz NDStrahilevitz merged commit f97d875 into aquasecurity:main Feb 7, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants