Merge pull request #25309 from cpuguy83/25253_fix_accidental_bind_create

Validate mount paths on task create

daemon/cluster/executor/container/container.go

@@ -53,6 +53,10 @@ func (c *containerConfig) setTask(t *api.Task) error {
 		return ErrImageRequired
 	}
 
+	if err := validateMounts(container.Mounts); err != nil {
+		return err
+	}
+
 	// index the networks by name
 	c.networksAttachments = make(map[string]*api.NetworkAttachment, len(t.Networks))
 	for _, attachment := range t.Networks {

daemon/cluster/executor/container/validate.go

@@ -0,0 +1,39 @@
+package container
+
+import (
+	"fmt"
+	"path/filepath"
+
+	"github.com/docker/swarmkit/api"
+)
+
+func validateMounts(mounts []api.Mount) error {
+	for _, mount := range mounts {
+		// Target must always be absolute
+		if !filepath.IsAbs(mount.Target) {
+			return fmt.Errorf("invalid mount target, must be an absolute path: %s", mount.Target)
+		}
+
+		switch mount.Type {
+		// The checks on abs paths are required due to the container API confusing
+		// volume mounts as bind mounts when the source is absolute (and vice-versa)
+		// See #25253
+		// TODO: This is probably not neccessary once #22373 is merged
+		case api.MountTypeBind:
+			if !filepath.IsAbs(mount.Source) {
+				return fmt.Errorf("invalid bind mount source, must be an absolute path: %s", mount.Source)
+			}
+		case api.MountTypeVolume:
+			if filepath.IsAbs(mount.Source) {
+				return fmt.Errorf("invalid volume mount source, must not be an absolute path: %s", mount.Source)
+			}
+		case api.MountTypeTmpfs:
+			if mount.Source != "" {
+				return fmt.Errorf("invalid tmpfs source, source must be empty")
+			}
+		default:
+			return fmt.Errorf("invalid mount type: %s", mount.Type)
+		}
+	}
+	return nil
+}

daemon/cluster/executor/container/validate_test.go

@@ -0,0 +1,118 @@
+package container
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/docker/docker/daemon"
+	"github.com/docker/docker/pkg/stringid"
+	"github.com/docker/swarmkit/api"
+)
+
+func newTestControllerWithMount(m api.Mount) (*controller, error) {
+	return newController(&daemon.Daemon{}, &api.Task{
+		ID:        stringid.GenerateRandomID(),
+		ServiceID: stringid.GenerateRandomID(),
+		Spec: api.TaskSpec{
+			Runtime: &api.TaskSpec_Container{
+				Container: &api.ContainerSpec{
+					Image: "image_name",
+					Labels: map[string]string{
+						"com.docker.swarm.task.id": "id",
+					},
+					Mounts: []api.Mount{m},
+				},
+			},
+		},
+	})
+}
+
+func TestControllerValidateMountBind(t *testing.T) {
+	// with improper source
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.MountTypeBind,
+		Source: "foo",
+		Target: testAbsPath,
+	}); err == nil || !strings.Contains(err.Error(), "invalid bind mount source") {
+		t.Fatalf("expected  error, got: %v", err)
+	}
+
+	// with proper source
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.MountTypeBind,
+		Source: testAbsPath,
+		Target: testAbsPath,
+	}); err != nil {
+		t.Fatalf("expected  error, got: %v", err)
+	}
+}
+
+func TestControllerValidateMountVolume(t *testing.T) {
+	// with improper source
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.MountTypeVolume,
+		Source: testAbsPath,
+		Target: testAbsPath,
+	}); err == nil || !strings.Contains(err.Error(), "invalid volume mount source") {
+		t.Fatalf("expected error, got: %v", err)
+	}
+
+	// with proper source
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.MountTypeVolume,
+		Source: "foo",
+		Target: testAbsPath,
+	}); err != nil {
+		t.Fatalf("expected error, got: %v", err)
+	}
+}
+
+func TestControllerValidateMountTarget(t *testing.T) {
+	// with improper target
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.MountTypeBind,
+		Source: testAbsPath,
+		Target: "foo",
+	}); err == nil || !strings.Contains(err.Error(), "invalid mount target") {
+		t.Fatalf("expected error, got: %v", err)
+	}
+
+	// with proper target
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.MountTypeBind,
+		Source: testAbsPath,
+		Target: testAbsPath,
+	}); err != nil {
+		t.Fatalf("expected no error, got: %v", err)
+	}
+}
+
+func TestControllerValidateMountTmpfs(t *testing.T) {
+	// with improper target
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.MountTypeTmpfs,
+		Source: "foo",
+		Target: testAbsPath,
+	}); err == nil || !strings.Contains(err.Error(), "invalid tmpfs source") {
+		t.Fatalf("expected error, got: %v", err)
+	}
+
+	// with proper target
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.MountTypeTmpfs,
+		Target: testAbsPath,
+	}); err != nil {
+		t.Fatalf("expected no error, got: %v", err)
+	}
+}
+
+func TestControllerValidateMountInvalidType(t *testing.T) {
+	// with improper target
+	if _, err := newTestControllerWithMount(api.Mount{
+		Type:   api.Mount_MountType(9999),
+		Source: "foo",
+		Target: testAbsPath,
+	}); err == nil || !strings.Contains(err.Error(), "invalid mount type") {
+		t.Fatalf("expected error, got: %v", err)
+	}
+}

daemon/cluster/executor/container/validate_unix_test.go

@@ -0,0 +1,7 @@
+// +build !windows
+
+package container
+
+const (
+	testAbsPath = "/foo"
+)

daemon/cluster/executor/container/validate_windows_test.go

@@ -0,0 +1,5 @@
+package container
+
+const (
+	testAbsPath = `c:\foo`
+)