diff --git a/container_test.go b/container_test.go index 3bd901956..7085bf3d1 100644 --- a/container_test.go +++ b/container_test.go @@ -704,7 +704,12 @@ func TestContainerExecNoBinaryExists(t *testing.T) { func TestUserNamespaces(t *testing.T) { t.Parallel() + t.Run("WritableRootFS", func(t *testing.T) { testUserNamespaces(t, false) }) + // see #1373 and runc#1572 + t.Run("ReadonlyRootFS", func(t *testing.T) { testUserNamespaces(t, true) }) +} +func testUserNamespaces(t *testing.T, readonlyRootFS bool) { client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -714,7 +719,7 @@ func TestUserNamespaces(t *testing.T) { var ( image Image ctx, cancel = testContext() - id = t.Name() + id = strings.Replace(t.Name(), "/", "-", -1) ) defer cancel() @@ -726,13 +731,17 @@ func TestUserNamespaces(t *testing.T) { } } - container, err := client.NewContainer(ctx, id, - WithNewSpec(withImageConfig(image), - withExitStatus(7), - withUserNamespace(0, 1000, 10000), - ), - withRemappedSnapshot(id, image, 1000, 1000), - ) + opts := []NewContainerOpts{WithNewSpec(withImageConfig(image), + withExitStatus(7), + withUserNamespace(0, 1000, 10000), + )} + if readonlyRootFS { + opts = append(opts, withRemappedSnapshotView(id, image, 1000, 1000)) + } else { + opts = append(opts, withRemappedSnapshot(id, image, 1000, 1000)) + } + + container, err := client.NewContainer(ctx, id, opts...) if err != nil { t.Error(err) return diff --git a/helpers_unix_test.go b/helpers_unix_test.go index 7b2aa3385..8d950a9b7 100644 --- a/helpers_unix_test.go +++ b/helpers_unix_test.go @@ -40,8 +40,9 @@ func withExecArgs(s *specs.Process, args ...string) { } var ( - withUserNamespace = WithUserNamespace - withRemappedSnapshot = WithRemappedSnapshot - withNewSnapshot = WithNewSnapshot - withImageConfig = WithImageConfig + withUserNamespace = WithUserNamespace + withRemappedSnapshot = WithRemappedSnapshot + withRemappedSnapshotView = WithRemappedSnapshotView + withNewSnapshot = WithNewSnapshot + withImageConfig = WithImageConfig ) diff --git a/helpers_windows_test.go b/helpers_windows_test.go index e5f1ee4e7..7661f7d59 100644 --- a/helpers_windows_test.go +++ b/helpers_windows_test.go @@ -63,6 +63,12 @@ func withRemappedSnapshot(id string, i Image, u, g uint32) NewContainerOpts { } } +func withRemappedSnapshotView(id string, i Image, u, g uint32) NewContainerOpts { + return func(ctx context.Context, client *Client, c *containers.Container) error { + return nil + } +} + func withNoop(_ context.Context, _ *Client, _ *containers.Container, _ *specs.Spec) error { return nil } diff --git a/mount/mount_linux.go b/mount/mount_linux.go index 5995051b8..906703c15 100644 --- a/mount/mount_linux.go +++ b/mount/mount_linux.go @@ -8,7 +8,36 @@ import ( func (m *Mount) Mount(target string) error { flags, data := parseMountOptions(m.Options) - return unix.Mount(m.Source, target, m.Type, uintptr(flags), data) + + // propagation types. + const ptypes = unix.MS_SHARED | unix.MS_PRIVATE | unix.MS_SLAVE | unix.MS_UNBINDABLE + + // Ensure propagation type change flags aren't included in other calls. + oflags := flags &^ ptypes + + // In the case of remounting with changed data (data != ""), need to call mount (moby/moby#34077). + if flags&unix.MS_REMOUNT == 0 || data != "" { + // Initial call applying all non-propagation flags for mount + // or remount with changed data + if err := unix.Mount(m.Source, target, m.Type, uintptr(oflags), data); err != nil { + return err + } + } + + if flags&ptypes != 0 { + // Change the propagation type. + const pflags = ptypes | unix.MS_REC | unix.MS_SILENT + if err := unix.Mount("", target, "", uintptr(flags&pflags), ""); err != nil { + return err + } + } + + const broflags = unix.MS_BIND | unix.MS_RDONLY + if oflags&broflags == broflags { + // Remount the bind to apply read only. + return unix.Mount("", target, "", uintptr(oflags|unix.MS_REMOUNT), "") + } + return nil } func Unmount(mount string, flags int) error { diff --git a/snapshot/testsuite/testsuite.go b/snapshot/testsuite/testsuite.go index e588ee374..b7bae12aa 100644 --- a/snapshot/testsuite/testsuite.go +++ b/snapshot/testsuite/testsuite.go @@ -38,6 +38,8 @@ func SnapshotterSuite(t *testing.T, name string, snapshotterFn func(ctx context. // Rename test still fails on some kernels with overlay //t.Run("Rename", makeTest(name, snapshotterFn, checkRename)) + + t.Run("ViewReadonly", makeTest(name, snapshotterFn, checkSnapshotterViewReadonly)) } func makeTest(name string, snapshotterFn func(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error), fn func(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string)) func(t *testing.T) { @@ -651,3 +653,40 @@ func checkRemove(ctx context.Context, t *testing.T, snapshotter snapshot.Snapsho t.Fatal(err) } } + +// checkSnapshotterViewReadonly ensures a KindView snapshot to be mounted as a read-only filesystem. +// This function is called only when WithTestViewReadonly is true. +func checkSnapshotterViewReadonly(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) { + preparing := filepath.Join(work, "preparing") + if _, err := snapshotter.Prepare(ctx, preparing, ""); err != nil { + t.Fatal(err) + } + committed := filepath.Join(work, "commited") + if err := snapshotter.Commit(ctx, committed, preparing); err != nil { + t.Fatal(err) + } + view := filepath.Join(work, "view") + m, err := snapshotter.View(ctx, view, committed) + if err != nil { + t.Fatal(err) + } + viewMountPoint := filepath.Join(work, "view-mount") + if err := os.MkdirAll(viewMountPoint, 0777); err != nil { + t.Fatal(err) + } + + // Just checking the option string of m is not enough, we need to test real mount. (#1368) + if err := mount.MountAll(m, viewMountPoint); err != nil { + t.Fatal(err) + } + + testfile := filepath.Join(viewMountPoint, "testfile") + if err := ioutil.WriteFile(testfile, []byte("testcontent"), 0777); err != nil { + t.Logf("write to %q failed with %v (EROFS is expected but can be other error code)", testfile, err) + } else { + t.Fatalf("write to %q should fail (EROFS) but did not fail", testfile) + } + testutil.Unmount(t, viewMountPoint) + assert.NoError(t, snapshotter.Remove(ctx, view)) + assert.NoError(t, snapshotter.Remove(ctx, committed)) +} diff --git a/spec_opts_unix.go b/spec_opts_unix.go index db9735593..532d38edc 100644 --- a/spec_opts_unix.go +++ b/spec_opts_unix.go @@ -225,6 +225,15 @@ func WithUserNamespace(container, host, size uint32) SpecOpts { // WithRemappedSnapshot creates a new snapshot and remaps the uid/gid for the // filesystem to be used by a container with user namespaces func WithRemappedSnapshot(id string, i Image, uid, gid uint32) NewContainerOpts { + return withRemappedSnapshotBase(id, i, uid, gid, false) +} + +// WithRemappedSnapshotView is similar to WithRemappedSnapshot but rootfs is mounted as read-only. +func WithRemappedSnapshotView(id string, i Image, uid, gid uint32) NewContainerOpts { + return withRemappedSnapshotBase(id, i, uid, gid, true) +} + +func withRemappedSnapshotBase(id string, i Image, uid, gid uint32, readonly bool) NewContainerOpts { return func(ctx context.Context, client *Client, c *containers.Container) error { diffIDs, err := i.(*image).i.RootFS(ctx, client.ContentStore()) if err != nil { @@ -257,7 +266,12 @@ func WithRemappedSnapshot(id string, i Image, uid, gid uint32) NewContainerOpts if err := snapshotter.Commit(ctx, usernsID, usernsID+"-remap"); err != nil { return err } - if _, err := snapshotter.Prepare(ctx, id, usernsID); err != nil { + if readonly { + _, err = snapshotter.View(ctx, id, usernsID) + } else { + _, err = snapshotter.Prepare(ctx, id, usernsID) + } + if err != nil { return err } c.RootFS = id