container users and groups from process root #677

deepskyblue86 · 2022-10-20T14:35:41Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area libsinsp

/area tests

Does this PR require a change in the driver versions?

What this PR does / why we need it:
Introduce add_container_user and add_container_group.
Those are called by sinsp_threadinfo set_user / set_group and load the information from /proc//root/etc.
For that to work, the process running sinsp has to be privileged and in the host pid namespace.
For more information see man 5 proc.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

deepskyblue86 · 2022-10-20T14:58:56Z

userspace/libsinsp/user.cpp

-		auto &userlist = m_userlist[container_id];
-		userlist[uid].uid = uid;
-		userlist[uid].gid = gid;
-		strlcpy(userlist[uid].name, name, MAX_CREDENTIALS_STR_LEN);
-		strlcpy(userlist[uid].homedir, home, SCAP_MAX_PATH_SIZE);
-		strlcpy(userlist[uid].shell, shell, SCAP_MAX_PATH_SIZE);


Moved into userinfo_map_insert

deepskyblue86 · 2022-10-20T14:59:27Z

userspace/libsinsp/user.cpp

-		auto &grplist = m_grouplist[container_id];
-		grplist[gid].gid = gid;
-		strlcpy(grplist[gid].name, name, MAX_CREDENTIALS_STR_LEN);


Moved into groupinfo_map_insert

deepskyblue86 · 2022-10-20T15:02:51Z

userspace/libsinsp/threadinfo.cpp

@@ -505,8 +505,15 @@ void sinsp_threadinfo::set_user(uint32_t uid)
 	scap_userinfo *user = m_inspector->m_usergroup_manager.get_user(m_container_id, uid);


Mind that sinsp_threadinfo::set_user gets called by parse_chroot_exit, parse_clone_exit, parse_execve_exit

deepskyblue86 · 2022-10-20T15:04:21Z

userspace/libsinsp/user.cpp

-		if (name == NULL)
-		{
-			name = "<NA>";
-		}
-		if (home == NULL)
-		{
-			home = "<NA>";
-		}
-		if (shell == NULL)
-		{
-			shell = "<NA>";
-		}


Avoid putting <NA> in cache on containers

deepskyblue86 · 2022-10-20T15:07:38Z

userspace/libsinsp/user.cpp

@@ -613,45 +691,3 @@ void sinsp_usergroup_manager::notify_group_changed(const scap_groupinfo *group,
 	m_inspector->m_pending_state_evts.push(cevt);
 #endif
 }
-
-void sinsp_usergroup_manager::load_from_container(const std::string &container_id, const std::string &overlayfs_root)


This worked well with docker, but wasn't called by containerd.

I tested with a Job on k0s:

apiVersion: batch/v1 kind: Job metadata: name: custom-user spec: template: spec: securityContext: runAsUser: 37 containers: - name: custom-user image: busybox:latest command: ["sleep", "3600" ] restartPolicy: Never backoffLimit: 4

deepskyblue86 · 2022-10-20T15:09:19Z

userspace/libsinsp/user.cpp

+	scap_userinfo *retval{nullptr};
+
+#if defined HAVE_PWD_H && defined HAVE_FGET__ENT
+	std::string proc_root_etc = s_host_root + "/proc/" + std::to_string(pid) + "/root/etc/";


From https://linux.die.net/man/5/proc:

/proc/[pid]/root
UNIX and Linux support the idea of a per-process root of the file system, set by the chroot system call. This file is a symbolic link that points to the process's root directory

This is a freakingly neat idea! Thanks for this!
@terylt @araujof

Credits to @gnosek for teaching me that. Credits to myself for a few days of experiments 🤣

Really nice!

FedeDP

I love these changes!
I am going to give this a bit of testing asap and come back to you!

FedeDP · 2022-10-20T15:55:51Z

Also, the CI did explode :D
btw @loresuso you might be interested in this since you developed the old solution with me!

Finally, i think we can also drop the container engines part to load the lowerdir .

deepskyblue86 · 2022-10-20T16:07:30Z

Yeah, the UT (which I actually wrote myself 😅) failed, I was eager to get some feedback and I didn't run them.
I'll update this PR soon!

loresuso · 2022-10-20T16:19:06Z

Cool! I am taking a closer look but it SGTM :)

deepskyblue86 · 2022-10-20T16:20:15Z

Also, the CI did explode :D btw @loresuso you might be interested in this since you developed the old solution with me!

Finally, i think we can also drop the container engines part to load the lowerdir .

@FedeDP do you want me to drop that in this PR?

FedeDP · 2022-10-20T17:10:09Z

Yep, thanks!

deepskyblue86 · 2022-10-20T18:18:06Z

I found something weird: if I checkout 0.9.0 sinsp-example behaves as expected, while on master I have {"error": ""} and no container events (I'm doing -j -f 'evt.type in (execve, execveat) and evt.dir=< and container.id != host').

Molter73 · 2022-10-21T07:49:20Z

I rebased the PR for adding e2e tests to CI on Tuesday, so whatever broke them is a change after that. I'll try to look into them ASAP, but if you guys find what is wrong, you can open a PR.

FedeDP · 2022-10-21T07:50:45Z

Hi Mauro! I think the fix is #678 ! Thanks to Angelo <3

FedeDP · 2022-10-21T08:37:22Z

We can rebase on top of master :)

FedeDP

/approve
I really like this :)
We can cleanup the overalyfs things later, in a subsequent PR!

poiana · 2022-10-21T09:04:09Z

LGTM label has been added.

Git tree hash: 7afc1b7e23321ddd423db20bd77bfe3181c566e3

FedeDP

Mmh on a better look, it seems like the container start is triggering an useradded event for host users (tested with sinsp-example:

sudo ./libsinsp/examples/sinsp-example --bpf driver/bpf/probe.o -f evt.type=useradded -o "%evt.arg[2] %evt.arg[5]"

evt.arg[2] is user name
evt.arg[5] is container id

It prints:

{"evt.arg[2]":"root","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"bin","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"daemon","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"mail","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"ftp","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"http","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"nobody","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"dbus","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"systemd-journal-remote","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"systemd-network","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"systemd-oom","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"systemd-resolve","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"systemd-timesync","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"systemd-coredump","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"uuidd","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"avahi","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"polkitd","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"rtkit","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"sddm","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"usbmux","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"federico","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"tss","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"git","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"geoclue","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"nm-openvpn","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"openvpn","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"brltty","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"sophosav","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"redis","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"dnsmasq","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"libvirt-qemu","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"mysql","evt.arg[5]":"2d4a2dd77f70"}
{"evt.arg[2]":"qemu","evt.arg[5]":"2d4a2dd77f70"}

Note the "federico" in there; these are the host users.

FedeDP · 2022-10-21T13:25:01Z

/hold

Have HAVE_FGET__ENT representing _DEFAULT_SOURCE || _SVID_SOURCE, as a feature test for fgetpwent() / fgetgrent() Signed-off-by: Angelo Puglisi <[email protected]>

deepskyblue86 · 2022-11-09T18:45:44Z

@Molter73 I'd like you to check the e2e part, if you have some time. Thanks in advance.

Molter73

Sorry for the delay @deepskyblue86! The changes on the e2e tests look solid, great to see someone actively using them! 😄

I left a few comments on the rest of the code, but overall the changes LGTM!

userspace/libsinsp/procfs_utils.cpp

userspace/libsinsp/user.cpp

`readlink` does not append a null terminator to `buf`, so we might want to ensure at least one byte is left untouched in order to prevent possible buffer overruns Signed-off-by: Angelo Puglisi <[email protected]> Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>

Signed-off-by: Angelo Puglisi <[email protected]> Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>

Molter73

/lgtm

poiana · 2022-11-21T10:55:22Z

LGTM label has been added.

Git tree hash: 60da174f5558ca010ad752432af9a5eb3ecf1ca1

gnosek

Just one question, LGTM otherwise :)

gnosek · 2022-11-21T13:36:59Z

userspace/libsinsp/threadinfo.cpp

+		else if(uid != 0)
+		{
+			//
+			// When a container is running with a specific user and this
+			// get called with 0, it's too early to make an attempt.
+			// As a downside we won't load users for containers running as
+			// root, but we will load them if e.g.docker exec -u <specific-user>.
+			//
+			user = m_inspector->m_usergroup_manager.add_container_user(m_container_id, m_pid, uid, m_inspector->is_live());
+		}


Why is it "too early"? Because we caught a thread switching into the container? What if the container is started as root?

What is uid here? If we're calling this from all sorts of parsers, it's not a special uid, like the docker -u user, right? So how does the uid != 0 check work?

Maybe hardcoding uid=0 as "root" would be good enough btw?

@deepskyblue86 :)

Why is it "too early"?
By trial and error, I found that the thread had the container id but wasn't really inside the container (i.e. runc:[1:CHILD] in my tests).

What if the container is started as root?
We'll have <NA> 🤷 but if / when it switches to a different user we'll load the users.

What is uid here?
a) sinsp_threadinfo::set_user is being called by parse_chroot_exit, parse_clone_exit, parse_execve_exit, parse_setuid_exit, parse_setresuid_exit.
b) It's actually the user of docker -u or k8s runAsUser.
c) you can see that it's being tested (test/e2e/tests/test_process/test_container.py) by starting a hashicorp/http-echo:alpine container with 11:110

Maybe hardcoding uid=0 as "root" would be good enough btw?
I was a bit uncertain about doing so, but if it's already two of us I'd go with that 😀

Maybe hardcoding uid=0 as "root" would be good enough btw?

Makes sense.

FedeDP

/approve

FedeDP · 2022-11-22T08:37:44Z

/hold

FedeDP · 2022-11-22T08:38:45Z

Sorry @gnosek i might have left behind your comment and approved :(
Hopefully i was able to hold this.

FedeDP · 2022-11-22T08:52:26Z

Uh i already put this on hold 20d ago ahah @deepskyblue86 care to answer to grzeg question?

poiana · 2022-11-23T17:55:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deepskyblue86, FedeDP, gnosek, Molter73

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~~ [FedeDP,Molter73,gnosek]

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

FedeDP · 2022-11-23T17:56:15Z

/unhold

poiana added kind/feature New feature or request release-note-none dco-signoff: yes area/libsinsp labels Oct 20, 2022

poiana requested review from hbrueckner and Molter73 October 20, 2022 14:35

poiana added the size/XL label Oct 20, 2022

deepskyblue86 commented Oct 20, 2022

View reviewed changes

FedeDP reviewed Oct 20, 2022

View reviewed changes

deepskyblue86 force-pushed the usergroup_manager-from-pid-root branch from 7b0099f to 900dab6 Compare October 20, 2022 16:15

deepskyblue86 force-pushed the usergroup_manager-from-pid-root branch from 900dab6 to 8ced729 Compare October 21, 2022 08:43

FedeDP previously approved these changes Oct 21, 2022

View reviewed changes

poiana assigned FedeDP Oct 21, 2022

poiana added the lgtm label Oct 21, 2022

poiana added the approved label Oct 21, 2022

Andreagit97 added this to the 0.10.0 milestone Oct 21, 2022

FedeDP reviewed Oct 21, 2022

View reviewed changes

poiana added the do-not-merge/hold label Oct 21, 2022

refactor(userspace/libsinsp): introduce HAVE_FGET__ENT

52e623b

Have HAVE_FGET__ENT representing _DEFAULT_SOURCE || _SVID_SOURCE, as a feature test for fgetpwent() / fgetgrent() Signed-off-by: Angelo Puglisi <[email protected]>

poiana removed the lgtm label Nov 8, 2022

poiana requested a review from FedeDP November 8, 2022 19:20

Molter73 reviewed Nov 15, 2022

View reviewed changes

userspace/libsinsp/procfs_utils.cpp Outdated Show resolved Hide resolved

userspace/libsinsp/procfs_utils.cpp Outdated Show resolved Hide resolved

userspace/libsinsp/procfs_utils.cpp Outdated Show resolved Hide resolved

userspace/libsinsp/user.cpp Show resolved Hide resolved

deepskyblue86 and others added 2 commits November 17, 2022 14:50

update(userspace/libsinsp): more descriptive warning message

c371d3d

Signed-off-by: Angelo Puglisi <[email protected]> Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>

deepskyblue86 requested review from Molter73 and FedeDP and removed request for FedeDP, hbrueckner and Molter73 November 21, 2022 09:49

Molter73 approved these changes Nov 21, 2022

View reviewed changes

poiana assigned Molter73 Nov 21, 2022

poiana added the lgtm label Nov 21, 2022

gnosek reviewed Nov 21, 2022

View reviewed changes

FedeDP approved these changes Nov 22, 2022

View reviewed changes

gnosek approved these changes Nov 23, 2022

View reviewed changes

poiana assigned gnosek Nov 23, 2022

poiana removed the do-not-merge/hold label Nov 23, 2022

poiana merged commit 6f9bec4 into falcosecurity:master Nov 23, 2022

deepskyblue86 deleted the usergroup_manager-from-pid-root branch November 23, 2022 18:03

poiana mentioned this pull request Dec 7, 2022

DO NOT MERGE: Add hotpot timings #777

Closed

poiana mentioned this pull request Feb 10, 2023

Oss merge libs 0.10.1 #874

Closed

poiana mentioned this pull request Apr 17, 2023

Ssprod 23814 add addl event init method #1052

Closed

		@@ -505,8 +505,15 @@ void sinsp_threadinfo::set_user(uint32_t uid)
		scap_userinfo *user = m_inspector->m_usergroup_manager.get_user(m_container_id, uid);

container users and groups from process root #677

container users and groups from process root #677

Conversation

deepskyblue86 commented Oct 20, 2022 • edited Loading

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

FedeDP left a comment

Choose a reason for hiding this comment

FedeDP commented Oct 20, 2022

deepskyblue86 commented Oct 20, 2022 • edited Loading

loresuso commented Oct 20, 2022

deepskyblue86 commented Oct 20, 2022

FedeDP commented Oct 20, 2022

deepskyblue86 commented Oct 20, 2022

Molter73 commented Oct 21, 2022

FedeDP commented Oct 21, 2022

FedeDP commented Oct 21, 2022

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Oct 21, 2022

FedeDP left a comment

Choose a reason for hiding this comment

FedeDP commented Oct 21, 2022

deepskyblue86 commented Nov 9, 2022

Molter73 left a comment

Choose a reason for hiding this comment

Molter73 left a comment

Choose a reason for hiding this comment

poiana commented Nov 21, 2022

gnosek left a 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

FedeDP left a comment

Choose a reason for hiding this comment

FedeDP commented Nov 22, 2022

FedeDP commented Nov 22, 2022

FedeDP commented Nov 22, 2022

poiana commented Nov 23, 2022

FedeDP commented Nov 23, 2022

deepskyblue86 commented Oct 20, 2022 •

edited

Loading

deepskyblue86 commented Oct 20, 2022 •

edited

Loading