Merge pull request #34224 from estesp/no-chown-nwfiles-outside-metadata
Only chown network files within container metadata
diff --git a/daemon/volumes_unix.go b/daemon/volumes_unix.go
index 0ab0c6d..bee1fb1 100644
--- a/daemon/volumes_unix.go
+++ b/daemon/volumes_unix.go
@@ -84,8 +84,13 @@
// 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
diff --git a/integration-cli/docker_api_containers_unix_test.go b/integration-cli/docker_api_containers_unix_test.go
new file mode 100644
index 0000000..4964f52
--- /dev/null
+++ b/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")
+}
diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go
index 6ac10e7..6722523 100644
--- a/integration-cli/docker_cli_run_test.go
+++ b/integration-cli/docker_cli_run_test.go
@@ -3115,6 +3115,11 @@
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 @@
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 @@
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 {
