Support idmap mounts for volumes #3717

rata · 2023-02-01T11:35:56Z

This PR implements support for this runtime-spec change that added idmap mounts support: opencontainers/runtime-spec#1143

We open the idmap source paths and call mount_setattr() in runc PARENT,
as we need privileges in the init userns for that, and then sends the
fds to the child process. For this fd passing we use the same mechanism
used in other parts of thecode, the _LIBCONTAINER_ env vars.

The mount is finished (unix.MoveMount) from go code, inside the userns,
so we reuse all the prepareBindMount() security checks and the remount
logic for some flags too.

This PR only supports idmap mounts when userns are used AND the mappings
are the same specified for the userns mapping. This limitation is to
simplify the initial implementation, as all our users so far only need
this, and we can avoid sending over netlink the mappings, creating a
userns with this custom mapping, etc. Future PRs will remove this
limitation.

As the idmap case is quite similar to the existing mount sources case we
open with O_PATH, some simple refactors are done to share more code and
to group the slices of fds in go code. To that end, we created the
mountFds struct, and add all the slices of fds there.

This replaces PR #3429, as that PR tries to mount already when we are inside the user namespace. AFAIK, that will never work and therefore this PR tries a completely different way to do the idmap mounts.

This PR is co-authored-by: Francis Laniel <flaniel@linux.microsoft.com>

cc @eiffel-fl

Question

One open question I have is I added most validations in libcontainer/configs/validate/validator.go but I couldn't help to notice that other parts of the code do some other validations. All examples that I tried hit the validations, but am I missing adding the validations in some other part, that don't use this maybe when runc is called differently? Or adding them there should be enough?

TODO

Pass idmap mount sources using bootstrap data (now hardcoded an example path)
Add integration tests
Modify the mountToRootfs() function to not move_mount and finish the idmap mount
Update runtime spec to have the new fields for UID/GID mappings
Address all TODOs I added in this PR

Changelog entry

Support idmap mounts as specified in the OCI runtime-spec. Currently the mount mappings need to be identical to the mappings used in the user namespace section.
Enforce absolute paths for mounts

Closes: #3429
Closes: #3020

rata · 2023-02-01T11:39:28Z

@AkihiroSuda can we add this to the 1.2 milestone? 🙏

rata · 2023-02-03T16:27:40Z

We have something working now. Will polish and push next week. Still tests and that missing.

rata · 2023-02-09T13:54:08Z

Test failures seem unrelated, for example:

make[1]: Entering directory '/go/src/github.com/opencontainers/runc'
fatal: detected dubious ownership in repository at '/go/src/github.com/opencontainers/runc'
To add an exception for this directory, call:

	git config --global --add safe.directory /go/src/github.com/opencontainers/runc
go build -trimpath -buildmode=pie -a -tags "seccomp urfave_cli_no_docs netgo osusergo" -ldflags "-X main.gitCommit= -X main.version=1.1.0+dev -linkmode external -extldflags --static-pie -w -s -buildid=" -o runc .
error obtaining VCS status: exit status 128
	Use -buildvcs=false to disable VCS stamping.
make[1]: *** [Makefile:69: static] Error 1

or go 1.19 with -race failing due to criu:

Run # criu repo
Warning: apt-key output should not be parsed (stdout is not a terminal)
curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
gpg: no valid OpenPGP data found.
Error: Process completed with exit code 2.

libcontainer/configs/mount_linux.go

libcontainer/configs/validate/validator.go

libcontainer/container_linux.go

AkihiroSuda · 2023-02-09T17:53:13Z

Please rebase, then the CI should be green

thaJeztah · 2023-07-11T14:52:31Z

We've had some issues with packages in the containerd repository (which was related to mirrors configured on the workers that only have a subset of architectures).

I gave CI a kick (hoping it was just a fluke) 🤞

thaJeztah

LGTM, thanks so much!

⚠️ ⚠️ I did not review the C code extensively; I'm not a C-coder, and don't want to pretend being one, so other than "a proper glance", haven't reviewed those commits. Reviews from others on those files therefore would be appreciated 👍

lifubang

Two small suggestions. we aleady have 3 LGTMs, I think we can move forward now.

libcontainer/nsenter/nsexec.c

This commit adds support for idmap mounts as specified in the runtime-spec. We open the idmap source paths and call mount_setattr() in runc PARENT, as we need privileges in the init userns for that, and then sends the fds to the child process. For this fd passing we use the same mechanism used in other parts of thecode, the _LIBCONTAINER_ env vars. The mount is finished (unix.MoveMount) from go code, inside the userns, so we reuse all the prepareBindMount() security checks and the remount logic for some flags too. This commit only supports idmap mounts when userns are used AND the mappings are the same specified for the userns mapping. This limitation is to simplify the initial implementation, as all our users so far only need this, and we can avoid sending over netlink the mappings, creating a userns with this custom mapping, etc. Future PRs will remove this limitation. Co-authored-by: Francis Laniel <flaniel@linux.microsoft.com> Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

Co-authored-by: Francis Laniel <flaniel@linux.microsoft.com> Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

rata · 2023-07-17T11:43:23Z

@lifubang thanks, fixed that too. Feel free to merge now! 🎉

lifubang · 2023-07-17T13:55:32Z

Thanks everyone working on this.

cyphar

Ah, this was merged while I was reviewing it. I guess I'll send a fix-up PR then... (I had 6-7 other comments but GitHub won't let me post them because the "diff has changed"...)

cyphar · 2023-07-17T08:58:24Z

libcontainer/rootfs_linux.go

+			if m.idmapFD == -1 {
+				return fmt.Errorf("error creating mount %+v: idmapFD is invalid, should point to a valid fd", m)
+			}
+			if err := unix.MoveMount(m.idmapFD, "", -1, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil {


Suggested change

if err := unix.MoveMount(m.idmapFD, "", -1, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil {

if err := unix.MoveMount(m.idmapFD, "", unix.AT_FDCWD, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil {

cyphar · 2023-07-17T09:02:02Z

libcontainer/nsenter/idmap.h

+static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flags, struct mount_attr *attr, size_t size)
+{
+	return syscall(__NR_mount_setattr, dfd, path, flags, attr, size);
+}
+
+static inline int sys_open_tree(int dfd, const char *filename, unsigned int flags)
+{
+	return syscall(__NR_open_tree, dfd, filename, flags);
+}


nit: We don't usually call these wrappers sys_foo, we just call them foo, but I guess it doesn't really matter.

cyphar · 2023-07-17T09:03:18Z

libcontainer/nsenter/nsexec.c

+			continue;
+		}
+
+		int fd_tree = sys_open_tree(-EBADF, idmap_src,


Suggested change

int fd_tree = sys_open_tree(-EBADF, idmap_src,

int fd_tree = sys_open_tree(AT_FDCWD, idmap_src,

We are enforcing that the path is a abs dir. If we use AT_FDCWD it will work (if the validation is skipped for some reason), while with this IIRC it will fail if the path is a rel path.

Why is this suggestion better?

rata · 2023-07-17T16:39:44Z

@cyphar Sure, feel free to open a PR and tag me if that works for you. Otherwise, let me know what suggested changes you had in mind

lifubang · 2023-07-18T11:00:44Z

Ah, this was merged while I was reviewing it.

Sorry to interrupt your code review. Maybe you can set this PR's label to status/2-codereview if the label is status/4-merge(This is learned from @thaJeztah ^_^) when you are doing an important code review. Thanks.

By the way, I think all the maintainers should have a msg group in a instant messaging tool If convenient. Then we can announce some important things in this msg group.

AkihiroSuda · 2023-07-18T11:05:08Z

msg

Do you have an account in opencontainers.slack.com ?
If you don't have, you can create one in https://communityinviter.com/apps/opencontainers/join-the-oci-community

lifubang · 2023-07-18T11:24:36Z

msg

Do you have an account in opencontainers.slack.com ? If you don't have, you can create one in https://communityinviter.com/apps/opencontainers/join-the-oci-community

Has created one account: Lifubang. Can you bring me in runc user group if there is? Thanks.

AkihiroSuda · 2023-07-18T11:34:40Z

Done, but not much active currently

This was a warning already and it was requested to make this an error while we will add validation of idmap mounts: opencontainers/runc#3717 (comment) I've also tested a k8s cluster and the config.json generated by containerd didn't use any relative paths. I tested one pod, so it was definitely not an extensive test. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

rata force-pushed the rata/idmap branch 2 times, most recently from df4f448 to 95170b5 Compare February 1, 2023 11:38

rata force-pushed the rata/idmap branch from 95170b5 to e1d3317 Compare February 1, 2023 11:39

This was referenced Feb 1, 2023

Support Id mapped mounts for shared volumes #3429

Closed

go:mod: runtime-spec v1.1.0-rc.1; add docs/spec-conformance.md #3716

Merged

AkihiroSuda added this to the 1.2.0 milestone Feb 1, 2023

rata force-pushed the rata/idmap branch 2 times, most recently from ee112d5 to 409c31e Compare February 9, 2023 12:23

rata changed the title ~~WIP: Support idmap mounts for volumes~~ Support idmap mounts for volumes Feb 9, 2023

rata marked this pull request as ready for review February 9, 2023 12:23

rata force-pushed the rata/idmap branch 6 times, most recently from 39913c4 to 854940e Compare February 9, 2023 13:35

rata force-pushed the rata/idmap branch 5 times, most recently from ed0739a to d92ec12 Compare February 9, 2023 15:34

This was referenced Feb 9, 2023

RFC: Initial support of idmapped mount points containerd/containerd#5890

Merged

Support idmapped mounts (kernel 5.12) containerd/containerd#5075

Closed

AkihiroSuda reviewed Feb 9, 2023

View reviewed changes

libcontainer/configs/mount_linux.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Feb 9, 2023

View reviewed changes

libcontainer/configs/validate/validator.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Feb 9, 2023

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

This was referenced Jul 11, 2023

Define documented const for magic -1 file-descriptor #3934

Closed

consider unifying mountEntry.srcFD and mountEntry.idmapFD approach #3935

Open

thaJeztah approved these changes Jul 11, 2023

View reviewed changes

rata mentioned this pull request Jul 12, 2023

Consider renaming Config.UidMappings and GidMappings to UIDMappings and GIDMappings #3936

Closed

lifubang added the impact/changelog label Jul 15, 2023

lifubang reviewed Jul 16, 2023

View reviewed changes

libcontainer/nsenter/nsexec.c Outdated Show resolved Hide resolved

libcontainer/nsenter/nsexec.c Outdated Show resolved Hide resolved

rata and others added 2 commits July 17, 2023 13:30

tests/integration: Add tests for idmap mounts

b460dc3

Co-authored-by: Francis Laniel <flaniel@linux.microsoft.com> Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

rata force-pushed the rata/idmap branch from 77e942b to b460dc3 Compare July 17, 2023 11:30

lifubang merged commit f73b05d into opencontainers:main Jul 17, 2023

rata deleted the rata/idmap branch July 17, 2023 14:14

rata mentioned this pull request Jul 17, 2023

Support idmapped mount (merged in kernel 5.12) #2821

Closed

cyphar reviewed Jul 17, 2023

View reviewed changes

eiffel-fl mentioned this pull request Jul 18, 2023

Renaming *Mappings fields and use int* for mountEntry.fd #3939

Merged

eiffel-fl mentioned this pull request Jul 21, 2023

Support for ID map mounts without userns #3943

Closed

cpuguy83 mentioned this pull request Jul 27, 2023

Handle relative paths in VOLUME directives moby/moby#46093

Open

yawqi mentioned this pull request Oct 13, 2023

support user namespace kata-containers/kata-containers#8170

Open

cyphar mentioned this pull request Mar 14, 2024

release 1.2.0-rc.1 #4221

Merged

devnoname120 mentioned this pull request May 7, 2024

Mounting a volume with rootless with always assign ownership to root moby/moby#45919

Closed

polarathene mentioned this pull request Jul 12, 2024

docker uid/gid mapping when userns is configured in docker daemon. moby/moby#48130

Closed

AkihiroSuda mentioned this pull request Nov 1, 2024

content: update UserNS document for runc v1.2 and containerd v2.0 kubernetes/website#48613

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support idmap mounts for volumes #3717

Support idmap mounts for volumes #3717

rata commented Feb 1, 2023 •

edited

Loading

rata commented Feb 1, 2023 •

edited

Loading

rata commented Feb 3, 2023

rata commented Feb 9, 2023

AkihiroSuda commented Feb 9, 2023

thaJeztah commented Jul 11, 2023

thaJeztah left a comment

lifubang left a comment

rata commented Jul 17, 2023

lifubang commented Jul 17, 2023

cyphar left a comment •

edited

Loading

cyphar Jul 17, 2023

cyphar Jul 17, 2023

cyphar Jul 17, 2023

rata Jul 17, 2023

rata commented Jul 17, 2023

lifubang commented Jul 18, 2023 •

edited

Loading

AkihiroSuda commented Jul 18, 2023

lifubang commented Jul 18, 2023

AkihiroSuda commented Jul 18, 2023

	if err := unix.MoveMount(m.idmapFD, "", -1, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil {
	if err := unix.MoveMount(m.idmapFD, "", unix.AT_FDCWD, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil {

	int fd_tree = sys_open_tree(-EBADF, idmap_src,
	int fd_tree = sys_open_tree(AT_FDCWD, idmap_src,

Support idmap mounts for volumes #3717

Support idmap mounts for volumes #3717

Conversation

rata commented Feb 1, 2023 • edited Loading

Question

TODO

Changelog entry

rata commented Feb 1, 2023 • edited Loading

rata commented Feb 3, 2023

rata commented Feb 9, 2023

AkihiroSuda commented Feb 9, 2023

thaJeztah commented Jul 11, 2023

thaJeztah left a comment

Choose a reason for hiding this comment

lifubang left a comment

Choose a reason for hiding this comment

rata commented Jul 17, 2023

lifubang commented Jul 17, 2023

cyphar left a comment • edited Loading

Choose a reason for hiding this comment

cyphar Jul 17, 2023

Choose a reason for hiding this comment

cyphar Jul 17, 2023

Choose a reason for hiding this comment

cyphar Jul 17, 2023

Choose a reason for hiding this comment

rata Jul 17, 2023

Choose a reason for hiding this comment

rata commented Jul 17, 2023

lifubang commented Jul 18, 2023 • edited Loading

AkihiroSuda commented Jul 18, 2023

lifubang commented Jul 18, 2023

AkihiroSuda commented Jul 18, 2023

rata commented Feb 1, 2023 •

edited

Loading

rata commented Feb 1, 2023 •

edited

Loading

cyphar left a comment •

edited

Loading

lifubang commented Jul 18, 2023 •

edited

Loading