From 38f8b0eb10725c40fb3c7e0719accd240cd39e22 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 1 Aug 2016 17:00:13 -0400 Subject: [PATCH] Validate mount paths on task create This is intended as a minor fix for 1.12.1 so that task creation doesn't do unexpected things when the user supplies erroneous paths. In particular, because we're currently using hostConfig.Binds to setup mounts, if a user uses an absolute path for a volume mount source, or a non-absolute path for a bind mount source, the engine will do the opposite of what the user requested since all absolute paths are treated as binds and all non-absolute paths are treated as named volumes. Fixes #25253 Signed-off-by: Brian Goff --- .../cluster/executor/container/container.go | 4 + daemon/cluster/executor/container/validate.go | 39 ++++++ .../executor/container/validate_test.go | 118 ++++++++++++++++++ .../executor/container/validate_unix_test.go | 7 ++ .../container/validate_windows_test.go | 5 + 5 files changed, 173 insertions(+) create mode 100644 daemon/cluster/executor/container/validate.go create mode 100644 daemon/cluster/executor/container/validate_test.go create mode 100644 daemon/cluster/executor/container/validate_unix_test.go create mode 100644 daemon/cluster/executor/container/validate_windows_test.go diff --git a/daemon/cluster/executor/container/container.go b/daemon/cluster/executor/container/container.go index 31782be6d4..19e23558ec 100644 --- a/daemon/cluster/executor/container/container.go +++ b/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 { diff --git a/daemon/cluster/executor/container/validate.go b/daemon/cluster/executor/container/validate.go new file mode 100644 index 0000000000..db348fe953 --- /dev/null +++ b/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 +} diff --git a/daemon/cluster/executor/container/validate_test.go b/daemon/cluster/executor/container/validate_test.go new file mode 100644 index 0000000000..acbe558f2e --- /dev/null +++ b/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) + } +} diff --git a/daemon/cluster/executor/container/validate_unix_test.go b/daemon/cluster/executor/container/validate_unix_test.go new file mode 100644 index 0000000000..5e122de918 --- /dev/null +++ b/daemon/cluster/executor/container/validate_unix_test.go @@ -0,0 +1,7 @@ +// +build !windows + +package container + +const ( + testAbsPath = "/foo" +) diff --git a/daemon/cluster/executor/container/validate_windows_test.go b/daemon/cluster/executor/container/validate_windows_test.go new file mode 100644 index 0000000000..8eb3e9987a --- /dev/null +++ b/daemon/cluster/executor/container/validate_windows_test.go @@ -0,0 +1,5 @@ +package container + +const ( + testAbsPath = `c:\foo` +)