c8d: Handle userns properly #46375

rumpl · 2023-08-30T14:29:31Z

- What I did

Remapped the GID/UID when a snapshot is created if the daemon is started with --userns-remap. This implementation differs from the graph driver one slightly. With graph drivers the files/directories are chown'ed when the image is downloaded and the contents extracted. Basically, with graph drivers we remap on pull and with containerd we remap on run.

Note: with this change builds still fail, my guess is because buildkit still mounts the roots in normal mode, I'll continue looking but I think we can merge this one, I am pretty sure some changes in buildkit are needed:

$ docker build -t userns . 
[+] Building 1.7s (6/6) FINISHED                                                                                                                               docker:local
 => [internal] load build definition from dd                                                                                                                           0.0s
 => => transferring dockerfile: 71B                                                                                                                                    0.0s
 => [internal] load .dockerignore                                                                                                                                      0.0s
 => => transferring context: 2B                                                                                                                                        0.0s
 => [internal] load metadata for docker.io/library/debian:bullseye-slim                                                                                                1.3s
 => [auth] library/debian:pull token for registry-1.docker.io                                                                                                          0.0s
 => CACHED [1/2] FROM docker.io/library/debian:bullseye-slim@sha256:61386e11b5256efa33823cbfafd668dd651dbce810b24a8fb7b2e32fa7f65a85                                   0.0s
 => => resolve docker.io/library/debian:bullseye-slim@sha256:61386e11b5256efa33823cbfafd668dd651dbce810b24a8fb7b2e32fa7f65a85                                          0.0s
 => ERROR [2/2] RUN touch testa                                                                                                                                        0.3s
------
 > [2/2] RUN touch testa:
0.287 runc run failed: unable to start container process: error during container init: error mounting "/tmp/root/165536.165536/buildkit/executor/hosts.iv21cdui6qvdwqel9cdo93spo" to rootfs at "/etc/hosts": open /tmp/root/165536.165536/buildkit/executor/7yecjv2i25qdu44khgx41b450/rootfs/etc/hosts: permission denied
------
dd:2
--------------------
   1 |     FROM debian:bullseye-slim
   2 | >>> RUN touch testa
   3 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c touch testa" did not complete successfully: exit code: 1

- How I did it

- How to verify it

TestDaemonUserNamespaceRootSetting should not fail any more

$ make DOCKER_GRAPHDRIVER=overlayfs TEST_FILTER=TestDaemonUserNamespaceRootSetting TEST_INTEGRATION_USE_SNAPSHOTTER=1 test-integration

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

vvoland

Does this break diff/commit (all files are chowned and treated as modified)?

EDIT: Ah, it shouldn't because it creates 2 snapshots on top of the base image.
Wondering if we should also adjust calculateSnapshotParentUsage to account for that?

rumpl · 2023-08-30T16:12:44Z

Wondering if we should also adjust calculateSnapshotParentUsage to account for that?

Hm... With overlayfs this would mean the reported size is 2x if metacopy is not enabled, which is true since chown would copy the files.

thaJeztah · 2023-08-30T16:24:36Z

Looking forward to idmapped mounts, which probably would make these kind of things a lot easier.

dmcgowan · 2023-09-01T14:06:02Z

daemon/containerd/image_snapshot_unix.go

+		return err
+	}
+
+	if err := remapRootFS(ctx, mounts, rootPair); err != nil {


Why does this only use the rootPair and not the full mapping?

RootPair is the remapped root uid/gid

Comparing to containerd's implementation which increments over the full userns range https://github.com/containerd/containerd/blob/v1.7.5/container_opts_unix.go#L104

Fixed thanks

dmcgowan · 2023-09-01T14:51:12Z

daemon/containerd/image_snapshot_unix.go

+				return fmt.Errorf("cannot get underlying data for %s", path)
+			}
+
+			return os.Lchown(path, int(stat.Uid)+idpair.UID, int(stat.Gid)+idpair.GID)


Might be clearer to use i.idMapping.ToContainer

Ok switched to using ToHost, ToContainer is the other way around and was giving me errors. With ToHost the uid/gid is the right one inside the container. Might be a good idea to add a test for this

Added a test for this

vvoland

Overall SGTM; left some nits though

vvoland · 2023-09-05T10:41:39Z

daemon/containerd/image_snapshot_unix.go

+	}
+
+	if err := i.remapRootFS(ctx, mounts); err != nil {
+		snapshotter.Remove(ctx, usernsID) //nolint: errcheck


Could we log this error instead of ignoring it?

daemon/containerd/image_snapshot_windows.go

daemon/containerd/image_snapshot_unix.go

thaJeztah

LGTM, but spotted some minor issues; let me know if you think it's worth fixing those before merging

thaJeztah · 2023-09-08T21:38:22Z

daemon/snapshotter/mount.go

+	dirID := idtools.Identity{
+		UID: idtools.CurrentIdentity().UID,
+		GID: root.GID,
+	}
+	if err := idtools.MkdirAllAndChown(path.Dir(target), 0o710, dirID); err != nil {
+		return "", err
+	}


Can you change this to use filepath.Dir() ?

I also thought for a long time that path is for / (and filepath only if you need cross-platform); https://github.com/golang/go/blob/2e7e1d2e8d96ddf2aff00feb645cfa94bc9e4786/src/path/path.go#L5-L6

Package path implements utility routines for manipulating slash-separated paths.

But a second paragraph was added at some point! golang/go@7465bfb

To manipulate operating system paths, use the path/filepath package.

While at it, perhaps also inline that dirID var (I see the other implementations also have the above issues, but we can look at those separately)

Suggested change

dirID := idtools.Identity{

UID: idtools.CurrentIdentity().UID,

GID: root.GID,

}

if err := idtools.MkdirAllAndChown(path.Dir(target), 0o710, dirID); err != nil {

return "", err

}

if err := idtools.MkdirAllAndChown(filepath.Dir(target), 0o710, idtools.Identity{

UID: idtools.CurrentIdentity().UID,

GID: root.GID,

}); err != nil {

return "", err

}

thaJeztah · 2023-09-08T21:42:44Z

daemon/containerd/image_snapshot_unix.go

+			fid := idtools.Identity{UID: int(stat.Uid), GID: int(stat.Gid)}
+			ids, err := i.idMapping.ToHost(fid)


Perhaps inline this one as well;

Suggested change

fid := idtools.Identity{UID: int(stat.Uid), GID: int(stat.Gid)}

ids, err := i.idMapping.ToHost(fid)

ids, err := i.idMapping.ToHost(idtools.Identity{UID: int(stat.Uid), GID: int(stat.Gid)}

thaJeztah · 2023-09-08T21:43:14Z

daemon/snapshotter/mount.go

@@ -67,7 +67,6 @@ type refCountMounter struct {

 func (m *refCountMounter) Mount(mounts []mount.Mount, containerID string) (target string, retErr error) {
 	target = filepath.Join(m.home, mountsDir, m.snapshotter, containerID)
-


unrelated change? 🙈

Thought the code looked better that way

thaJeztah · 2023-09-11T10:50:46Z

daemon/snapshotter/mount.go

+	if err := idtools.MkdirAllAndChown(filepath.Dir(target), 0o710, idtools.Identity{
 		UID: idtools.CurrentIdentity().UID,
 		GID: root.GID,
-	}
-	if err := idtools.MkdirAllAndChown(path.Dir(target), 0o710, dirID); err != nil {
+	}); err != nil {


🙈 looks like your touch-up ended in the second commit instead of the first 🙈

🤦 forgot I had two commits in this PR

Fixed, thanks

Graph drivers create the parent directory with rootPair().GID:CurrentIdentity().UID owner. This change brings these in line Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

thaJeztah · 2023-09-11T12:59:05Z

daemon/containerd/image_snapshot.go

+	if i.idMapping.Empty() {
+		_, err = snapshotter.Prepare(ctx, id, parentSnapshot)
+		return err
+	}
+
+	return i.remapSnapshot(ctx, snapshotter, id, parentSnapshot, lease)


Ohman, changes look good, but I'm just looking at this part again, and this could be a potential foot-gun if either a linter, or some "helpful" contribution fixes "unhandled error" here;

Suggested change

if i.idMapping.Empty() {

_, err = snapshotter.Prepare(ctx, id, parentSnapshot)

return err

}

return i.remapSnapshot(ctx, snapshotter, id, parentSnapshot, lease)

if i.idMapping.Empty() {

if _, err := snapshotter.Prepare(ctx, id, parentSnapshot); err != nil {

return err

}

}

return i.remapSnapshot(ctx, snapshotter, id, parentSnapshot, lease)

The above would break this code (which would hopefully be caught in CI.

Do you think it's worth to be defensive here, and to reverse the order?

Suggested change

if i.idMapping.Empty() {

_, err = snapshotter.Prepare(ctx, id, parentSnapshot)

return err

}

return i.remapSnapshot(ctx, snapshotter, id, parentSnapshot, lease)

if !i.idMapping.Empty() {

return i.remapSnapshot(ctx, snapshotter, id, parentSnapshot, lease)

}

_, err = snapshotter.Prepare(ctx, id, parentSnapshot)

return err

Sure I can reverse the order 👍

If the daemon is run with --userns-remap we need to chown the prepared snapshot Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

thaJeztah

LGTM

rumpl added area/runtime area/security/userns kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration process/cherry-pick/24.0 labels Aug 30, 2023

vvoland reviewed Aug 30, 2023

View reviewed changes

rumpl requested a review from vvoland September 1, 2023 08:40

dmcgowan reviewed Sep 1, 2023

View reviewed changes

rumpl force-pushed the c8d-userns-remap branch from 874e755 to f00f041 Compare September 1, 2023 14:36

dmcgowan reviewed Sep 1, 2023

View reviewed changes

rumpl force-pushed the c8d-userns-remap branch 3 times, most recently from 17fc2bb to 69cdb1c Compare September 4, 2023 09:50

vvoland approved these changes Sep 5, 2023

View reviewed changes

rumpl force-pushed the c8d-userns-remap branch from 69cdb1c to 072b120 Compare September 5, 2023 11:52

vvoland reviewed Sep 5, 2023

View reviewed changes

daemon/containerd/image_snapshot_unix.go Outdated Show resolved Hide resolved

rumpl force-pushed the c8d-userns-remap branch from 072b120 to 6c7d022 Compare September 5, 2023 15:20

rumpl added the status/2-code-review label Sep 5, 2023

thaJeztah requested a review from dmcgowan September 6, 2023 15:52

dmcgowan approved these changes Sep 6, 2023

View reviewed changes

thaJeztah approved these changes Sep 8, 2023

View reviewed changes

rumpl force-pushed the c8d-userns-remap branch 2 times, most recently from 29b6218 to 2a793cf Compare September 11, 2023 08:35

thaJeztah reviewed Sep 11, 2023

View reviewed changes

c8d/mount: Create same directory owners as graphdrivers

833b514

Graph drivers create the parent directory with rootPair().GID:CurrentIdentity().UID owner. This change brings these in line Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

rumpl force-pushed the c8d-userns-remap branch from 2a793cf to 87a8f00 Compare September 11, 2023 11:26

thaJeztah reviewed Sep 11, 2023

View reviewed changes

c8d: Handle userns properly

0313544

If the daemon is run with --userns-remap we need to chown the prepared snapshot Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

rumpl force-pushed the c8d-userns-remap branch from 87a8f00 to 0313544 Compare September 11, 2023 14:40

thaJeztah approved these changes Sep 11, 2023

View reviewed changes

rumpl merged commit d0d3ddd into moby:master Sep 11, 2023
103 checks passed

thaJeztah added this to the 25.0.0 milestone Oct 26, 2023

vvoland mentioned this pull request Jan 19, 2024

engine: add release notes for v25.0.0 docker/docs#18253

Merged

thaJeztah removed the process/cherry-pick/24.0 label Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c8d: Handle userns properly #46375

c8d: Handle userns properly #46375

rumpl commented Aug 30, 2023 •

edited

vvoland left a comment •

edited

rumpl commented Aug 30, 2023

thaJeztah commented Aug 30, 2023

dmcgowan Sep 1, 2023

rumpl Sep 1, 2023

dmcgowan Sep 1, 2023

rumpl Sep 1, 2023

rumpl Sep 1, 2023

dmcgowan Sep 1, 2023

rumpl Sep 4, 2023

rumpl Sep 4, 2023

vvoland left a comment

vvoland Sep 5, 2023

thaJeztah left a comment

thaJeztah Sep 8, 2023

thaJeztah Sep 8, 2023

thaJeztah Sep 8, 2023

rumpl Sep 11, 2023

thaJeztah Sep 11, 2023

rumpl Sep 11, 2023

rumpl Sep 11, 2023

thaJeztah Sep 11, 2023

rumpl Sep 11, 2023

rumpl Sep 11, 2023

thaJeztah left a comment

		fid := idtools.Identity{UID: int(stat.Uid), GID: int(stat.Gid)}
		ids, err := i.idMapping.ToHost(fid)

	fid := idtools.Identity{UID: int(stat.Uid), GID: int(stat.Gid)}
	ids, err := i.idMapping.ToHost(fid)
	ids, err := i.idMapping.ToHost(idtools.Identity{UID: int(stat.Uid), GID: int(stat.Gid)}

		@@ -67,7 +67,6 @@ type refCountMounter struct {

		func (m *refCountMounter) Mount(mounts []mount.Mount, containerID string) (target string, retErr error) {
		target = filepath.Join(m.home, mountsDir, m.snapshotter, containerID)

c8d: Handle userns properly #46375

c8d: Handle userns properly #46375

Conversation

rumpl commented Aug 30, 2023 • edited

vvoland left a comment • edited

Choose a reason for hiding this comment

rumpl commented Aug 30, 2023

thaJeztah commented Aug 30, 2023

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

vvoland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah 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

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

thaJeztah left a comment

Choose a reason for hiding this comment

rumpl commented Aug 30, 2023 •

edited

vvoland left a comment •

edited