Merge pull request #34224 from estesp/no-chown-nwfiles-outside-metadata

Only chown network files within container metadata

daemon/volumes_unix.go

@@ -84,8 +84,13 @@ func (daemon *Daemon) setupMounts(c *container.Container) ([]container.Mount, er
 	// remapped root (user namespaces)
 	rootIDs := daemon.idMappings.RootPair()
 	for _, mount := range netMounts {
-		if err := os.Chown(mount.Source, rootIDs.UID, rootIDs.GID); err != nil {
-			return nil, err
+		// we should only modify ownership of network files within our own container
+		// metadata repository. If the user specifies a mount path external, it is
+		// up to the user to make sure the file has proper ownership for userns
+		if strings.Index(mount.Source, daemon.repository) == 0 {
+			if err := os.Chown(mount.Source, rootIDs.UID, rootIDs.GID); err != nil {
+				return nil, err
+			}
 		}
 	}
 	return append(mounts, netMounts...), nil

integration-cli/docker_api_containers_unix_test.go

@@ -0,0 +1,77 @@
+// +build !windows
+
+package main
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	"github.com/docker/docker/api/types"
+	containertypes "github.com/docker/docker/api/types/container"
+	mounttypes "github.com/docker/docker/api/types/mount"
+	networktypes "github.com/docker/docker/api/types/network"
+	"github.com/docker/docker/client"
+	"github.com/docker/docker/integration-cli/checker"
+	"github.com/docker/docker/pkg/ioutils"
+	"github.com/docker/docker/pkg/system"
+	"github.com/go-check/check"
+	"github.com/stretchr/testify/assert"
+	"golang.org/x/net/context"
+)
+
+func (s *DockerSuite) TestContainersAPINetworkMountsNoChown(c *check.C) {
+	// chown only applies to Linux bind mounted volumes; must be same host to verify
+	testRequires(c, DaemonIsLinux, SameHostDaemon)
+
+	tmpDir, err := ioutils.TempDir("", "test-network-mounts")
+	c.Assert(err, checker.IsNil)
+	defer os.RemoveAll(tmpDir)
+
+	// make tmp dir readable by anyone to allow userns process to mount from
+	err = os.Chmod(tmpDir, 0755)
+	c.Assert(err, checker.IsNil)
+	// create temp files to use as network mounts
+	tmpNWFileMount := filepath.Join(tmpDir, "nwfile")
+
+	err = ioutil.WriteFile(tmpNWFileMount, []byte("network file bind mount"), 0644)
+	c.Assert(err, checker.IsNil)
+
+	config := containertypes.Config{
+		Image: "busybox",
+	}
+	hostConfig := containertypes.HostConfig{
+		Mounts: []mounttypes.Mount{
+			{
+				Type:   "bind",
+				Source: tmpNWFileMount,
+				Target: "/etc/resolv.conf",
+			},
+			{
+				Type:   "bind",
+				Source: tmpNWFileMount,
+				Target: "/etc/hostname",
+			},
+			{
+				Type:   "bind",
+				Source: tmpNWFileMount,
+				Target: "/etc/hosts",
+			},
+		},
+	}
+
+	cli, err := client.NewEnvClient()
+	c.Assert(err, checker.IsNil)
+	defer cli.Close()
+
+	ctrCreate, err := cli.ContainerCreate(context.Background(), &config, &hostConfig, &networktypes.NetworkingConfig{}, "")
+	c.Assert(err, checker.IsNil)
+	// container will exit immediately because of no tty, but we only need the start sequence to test the condition
+	err = cli.ContainerStart(context.Background(), ctrCreate.ID, types.ContainerStartOptions{})
+	c.Assert(err, checker.IsNil)
+
+	// check that host-located bind mount network file did not change ownership when the container was started
+	statT, err := system.Stat(tmpNWFileMount)
+	c.Assert(err, checker.IsNil)
+	assert.Equal(c, uint32(0), statT.UID(), "bind mounted network file should not change ownership from root")
+}

integration-cli/docker_cli_run_test.go

@@ -3115,6 +3115,11 @@ func (s *DockerSuite) TestRunNetworkFilesBindMount(c *check.C) {
 	filename := createTmpFile(c, expected)
 	defer os.Remove(filename)
 
+	// for user namespaced test runs, the temp file must be accessible to unprivileged root
+	if err := os.Chmod(filename, 0646); err != nil {
+		c.Fatalf("error modifying permissions of %s: %v", filename, err)
+	}
+
 	nwfiles := []string{"/etc/resolv.conf", "/etc/hosts", "/etc/hostname"}
 
 	for i := range nwfiles {
@@ -3132,6 +3137,11 @@ func (s *DockerSuite) TestRunNetworkFilesBindMountRO(c *check.C) {
 	filename := createTmpFile(c, "test123")
 	defer os.Remove(filename)
 
+	// for user namespaced test runs, the temp file must be accessible to unprivileged root
+	if err := os.Chmod(filename, 0646); err != nil {
+		c.Fatalf("error modifying permissions of %s: %v", filename, err)
+	}
+
 	nwfiles := []string{"/etc/resolv.conf", "/etc/hosts", "/etc/hostname"}
 
 	for i := range nwfiles {
@@ -3149,6 +3159,11 @@ func (s *DockerSuite) TestRunNetworkFilesBindMountROFilesystem(c *check.C) {
 	filename := createTmpFile(c, "test123")
 	defer os.Remove(filename)
 
+	// for user namespaced test runs, the temp file must be accessible to unprivileged root
+	if err := os.Chmod(filename, 0646); err != nil {
+		c.Fatalf("error modifying permissions of %s: %v", filename, err)
+	}
+
 	nwfiles := []string{"/etc/resolv.conf", "/etc/hosts", "/etc/hostname"}
 
 	for i := range nwfiles {