fix: container.privileged in containerd and crio #79

holyspectral · 2021-09-08T14:59:55Z

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area libsinsp

What this PR does / why we need it:

Currently container.privileged flag does not always work when container runtime is containerd or crio. This is because userspace/libsinsp/cri.cpp gets container.privileged flag from an out-of-date location.

Since CRI doesn't define where the flag should be stored, containerd and crio both implement their own version.

containerd: info/config/linux/security_context/privileged
https://github.com/containerd/containerd/blob/main/pkg/cri/server/container_status.go#L153
crio: info/privileged
https://github.com/cri-o/cri-o/blob/master/server/container_status.go#L125

This PR creates a fallback logic. When the existing logic fails to get the flag, it will try to find the flag from containerd and cri-o specific location.

Which issue(s) this PR fixes:

Fixes falcosecurity/falco#1630

Special notes for your reviewer:

Verified on ROSA 4.8.4 (cri-o://1.21.2-8.rhaos4.8.git8d4264e.el8) and AKS (containerd://1.4.8+azure) based on 17f5df5 and this patch.

Does this PR introduce a user-facing change?:

NONE

poiana · 2021-09-08T15:00:04Z

Welcome @holyspectral! It looks like this is your first PR to falcosecurity/libs 🎉

poiana · 2021-09-08T15:00:04Z

Hi @holyspectral. Thanks for your PR.

I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gnosek

LGTM, but I'm curious about the need for another containerd-specific block. Did something change or was I on drugs when I wrote the first one?

gnosek · 2021-09-08T15:03:06Z

userspace/libsinsp/cri.cpp

+		priv_found = true;
+	}
+
+	// containerd


The chunk above is also expected to work for containerd. Did something change between versions?

I think it's possible that it was changed at some point...in cri.proto the Info field is defined as

Info map[string]string `protobuf:"bytes,2,rep,name=info,proto3" json:"info,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"`

Since it doesn't say where those flags should be located, container runtime can create backward-incompatible change accidentally.

I wonder which version of containerd was tested previously? I can take a look to see when this was changed.

Hmm, hard to say but it's been a while ago:

commit 3e24f84696631b7fec23ce929795e328722f4946 Author: Grzegorz Nosek <grzegorz.nosek@sysdig.com> Date: Mon Dec 17 19:53:23 2018 +0100 Initial CRI support

The logic hasn't changed since, even though it's been moved around quite a bit.

I want to say 1.1.something but, well, it's been a few years now :)

Hmm I couldn't find any significant changes in containerd and runtime-spec since 2018. The runtimeSpec->Linux doesn't have security_context anymore too as far as I can tell.

@gnosek What's your opinion about this? I'm ok with either keeping or removing the existing containerd logic.

I'd say leave it, with an // old containerd? comment to match your newer ones

gnosek · 2021-09-08T15:03:23Z

userspace/libsinsp/cri.h

 	 *
 	 * Note: only containerd exposes this data
 	 */


I believe this comment is outdated now :)

gnosek

LGTM, thanks!

poiana · 2021-09-08T17:17:26Z

LGTM label has been added.

Git tree hash: f519342fe8be28003e9b8921335f28a81fbe7ee2

Retrieve privileged from the correct locations of ContainerStatusResponse. - containerd: info/config/linux/security_context/privileged - crio: info/privileged Signed-off-by: Shang-Wen Wang (Sam Wang) <sam_s_wang@trendmicro.com>

gnosek

Still LGTM :)

poiana · 2021-09-08T17:22:09Z

LGTM label has been added.

Git tree hash: a955f363c37bb3be471e5e22a66e1c2bad43e49b

holyspectral · 2021-09-10T14:08:13Z

Hi @gnosek @leogr Is there anything that I can do to get this merged?

gnosek · 2021-09-10T14:10:01Z

Let's see what happens ;)

/approve

poiana · 2021-09-10T14:10:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnosek, holyspectral

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gnosek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

leogr

👍

poiana added kind/bug Something isn't working release-note-none area/libsinsp dco-signoff: yes needs-ok-to-test labels Sep 8, 2021

poiana requested review from gnosek and leogr September 8, 2021 15:00

poiana added the size/S label Sep 8, 2021

gnosek reviewed Sep 8, 2021

View reviewed changes

holyspectral force-pushed the container-privileged-flag branch from 5e2cf63 to dcb84d9 Compare September 8, 2021 17:15

gnosek previously approved these changes Sep 8, 2021

View reviewed changes

poiana assigned gnosek Sep 8, 2021

poiana added the lgtm label Sep 8, 2021

fix: container.privileged in containerd and crio

282286a

Retrieve privileged from the correct locations of ContainerStatusResponse. - containerd: info/config/linux/security_context/privileged - crio: info/privileged Signed-off-by: Shang-Wen Wang (Sam Wang) <sam_s_wang@trendmicro.com>

holyspectral dismissed gnosek’s stale review via 282286a September 8, 2021 17:20

holyspectral force-pushed the container-privileged-flag branch from dcb84d9 to 282286a Compare September 8, 2021 17:20

poiana removed the lgtm label Sep 8, 2021

holyspectral requested a review from gnosek September 8, 2021 17:21

gnosek approved these changes Sep 8, 2021

View reviewed changes

poiana added the lgtm label Sep 8, 2021

poiana added the approved label Sep 10, 2021

leogr approved these changes Sep 15, 2021

View reviewed changes

poiana assigned leogr Sep 15, 2021

poiana merged commit 89e1263 into falcosecurity:master Sep 15, 2021

leogr mentioned this pull request Sep 15, 2021

Unable to detect events with containerd and kubernetes falcosecurity/falco#1630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: container.privileged in containerd and crio #79

fix: container.privileged in containerd and crio #79

holyspectral commented Sep 8, 2021

poiana commented Sep 8, 2021

poiana commented Sep 8, 2021

gnosek left a comment

gnosek Sep 8, 2021

holyspectral Sep 8, 2021

gnosek Sep 8, 2021

holyspectral Sep 8, 2021

gnosek Sep 8, 2021

gnosek Sep 8, 2021

gnosek left a comment

poiana commented Sep 8, 2021

gnosek left a comment

poiana commented Sep 8, 2021

holyspectral commented Sep 10, 2021

gnosek commented Sep 10, 2021

poiana commented Sep 10, 2021

leogr left a comment

fix: container.privileged in containerd and crio #79

fix: container.privileged in containerd and crio #79

Conversation

holyspectral commented Sep 8, 2021

poiana commented Sep 8, 2021

poiana commented Sep 8, 2021

gnosek left a comment

Choose a reason for hiding this comment

gnosek Sep 8, 2021

Choose a reason for hiding this comment

holyspectral Sep 8, 2021

Choose a reason for hiding this comment

gnosek Sep 8, 2021

Choose a reason for hiding this comment

holyspectral Sep 8, 2021

Choose a reason for hiding this comment

gnosek Sep 8, 2021

Choose a reason for hiding this comment

gnosek Sep 8, 2021

Choose a reason for hiding this comment

gnosek left a comment

Choose a reason for hiding this comment

poiana commented Sep 8, 2021

gnosek left a comment

Choose a reason for hiding this comment

poiana commented Sep 8, 2021

holyspectral commented Sep 10, 2021

gnosek commented Sep 10, 2021

poiana commented Sep 10, 2021

leogr left a comment

Choose a reason for hiding this comment