Commit ffd1e9a0 authored by Mahmood Ali's avatar Mahmood Ali
Browse files

drivers/docker: enforce volumes.enabled

When volumes.enable flag is off in Docker driver, disable all mounts of
paths outside alloc dir.
parent 5306b1e9
No related merge requests found
Showing with 206 additions and 32 deletions
+206 -32
......@@ -541,7 +541,9 @@ func (d *Driver) containerBinds(task *drivers.TaskConfig, driverConfig *TaskConf
secretDirBind := fmt.Sprintf("%s:%s", task.TaskDir().SecretsDir, task.Env[taskenv.SecretsDir])
binds := []string{allocDirBind, taskLocalBind, secretDirBind}
if !d.config.Volumes.Enabled && driverConfig.VolumeDriver != "" {
localBindVolume := driverConfig.VolumeDriver == "" || driverConfig.VolumeDriver == "local"
if !d.config.Volumes.Enabled && !localBindVolume {
return nil, fmt.Errorf("volumes are not enabled; cannot use volume driver %q", driverConfig.VolumeDriver)
}
......@@ -551,25 +553,15 @@ func (d *Driver) containerBinds(task *drivers.TaskConfig, driverConfig *TaskConf
return nil, fmt.Errorf("invalid docker volume: %q", userbind)
}
// Resolve dotted path segments
parts[0] = filepath.Clean(parts[0])
// Paths inside task dir are always allowed, Relative paths are always allowed as they mount within a container
// When a VolumeDriver is set, we assume we receive a binding in the format volume-name:container-dest
// Otherwise, we assume we receive a relative path binding in the format relative/to/task:/also/in/container
if localBindVolume {
parts[0] = expandPath(task.TaskDir().Dir, parts[0])
// Absolute paths aren't always supported
if filepath.IsAbs(parts[0]) {
if !d.config.Volumes.Enabled {
// Disallow mounting arbitrary absolute paths
if !d.config.Volumes.Enabled && !isParentPath(task.AllocDir, parts[0]) {
return nil, fmt.Errorf("volumes are not enabled; cannot mount host paths: %+q", userbind)
}
binds = append(binds, userbind)
continue
}
// Relative paths are always allowed as they mount within a container
// When a VolumeDriver is set, we assume we receive a binding in the format volume-name:container-dest
// Otherwise, we assume we receive a relative path binding in the format relative/to/task:/also/in/container
if driverConfig.VolumeDriver == "" {
// Expand path relative to alloc dir
parts[0] = filepath.Join(task.TaskDir().Dir, parts[0])
}
binds = append(binds, strings.Join(parts, ":"))
......@@ -742,13 +734,11 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T
}
if hm.Type == "bind" {
if filepath.IsAbs(filepath.Clean(hm.Source)) {
if !d.config.Volumes.Enabled {
return c, fmt.Errorf("volumes are not enabled; cannot mount host path: %q", hm.Source)
}
} else {
// Relative paths are always allowed as they mount within a container, and treated as relative to task dir
hm.Source = filepath.Join(task.TaskDir().Dir, hm.Source)
hm.Source = expandPath(task.TaskDir().Dir, hm.Source)
// paths inside alloc dir are always allowed as they mount within a container, and treated as relative to task dir
if !d.config.Volumes.Enabled && !isParentPath(task.AllocDir, hm.Source) {
return c, fmt.Errorf("volumes are not enabled; cannot mount host path: %q %q", hm.Source, task.AllocDir)
}
}
......
......@@ -1149,7 +1149,6 @@ func TestDockerDriver_CreateContainerConfig(t *testing.T) {
require.Equal(t, "org/repo:0.1", c.Config.Image)
require.EqualValues(t, opt, c.HostConfig.StorageOpt)
}
func TestDockerDriver_Capabilities(t *testing.T) {
if !tu.IsTravis() {
t.Parallel()
......@@ -1638,6 +1637,143 @@ func TestDockerDriver_VolumesDisabled(t *testing.T) {
}
func TestDockerDriver_BindMountsHonorVolumesEnabledFlag(t *testing.T) {
t.Parallel()
allocDir := "/tmp/nomad/alloc-dir"
cases := []struct {
name string
requiresVolumes bool
volumeDriver string
volumes []string
expectedVolumes []string
}{
{
name: "basic plugin",
requiresVolumes: true,
volumeDriver: "nfs",
volumes: []string{"test-path:/tmp/taskpath"},
expectedVolumes: []string{"test-path:/tmp/taskpath"},
},
{
name: "absolute default driver",
requiresVolumes: true,
volumeDriver: "",
volumes: []string{"/abs/test-path:/tmp/taskpath"},
expectedVolumes: []string{"/abs/test-path:/tmp/taskpath"},
},
{
name: "absolute local driver",
requiresVolumes: true,
volumeDriver: "local",
volumes: []string{"/abs/test-path:/tmp/taskpath"},
expectedVolumes: []string{"/abs/test-path:/tmp/taskpath"},
},
{
name: "relative default driver",
requiresVolumes: false,
volumeDriver: "",
volumes: []string{"test-path:/tmp/taskpath"},
expectedVolumes: []string{"/tmp/nomad/alloc-dir/demo/test-path:/tmp/taskpath"},
},
{
name: "relative local driver",
requiresVolumes: false,
volumeDriver: "local",
volumes: []string{"test-path:/tmp/taskpath"},
expectedVolumes: []string{"/tmp/nomad/alloc-dir/demo/test-path:/tmp/taskpath"},
},
{
name: "relative outside task-dir default driver",
requiresVolumes: false,
volumeDriver: "",
volumes: []string{"../test-path:/tmp/taskpath"},
expectedVolumes: []string{"/tmp/nomad/alloc-dir/test-path:/tmp/taskpath"},
},
{
name: "relative outside task-dir local driver",
requiresVolumes: false,
volumeDriver: "local",
volumes: []string{"../test-path:/tmp/taskpath"},
expectedVolumes: []string{"/tmp/nomad/alloc-dir/test-path:/tmp/taskpath"},
},
{
name: "relative outside alloc-dir default driver",
requiresVolumes: true,
volumeDriver: "",
volumes: []string{"../../test-path:/tmp/taskpath"},
expectedVolumes: []string{"/tmp/nomad/test-path:/tmp/taskpath"},
},
{
name: "relative outside task-dir local driver",
requiresVolumes: true,
volumeDriver: "local",
volumes: []string{"../../test-path:/tmp/taskpath"},
expectedVolumes: []string{"/tmp/nomad/test-path:/tmp/taskpath"},
},
}
t.Run("with volumes enabled", func(t *testing.T) {
dh := dockerDriverHarness(t, nil)
driver := dh.Impl().(*Driver)
driver.config.Volumes.Enabled = true
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
task, cfg, _ := dockerTask(t)
cfg.VolumeDriver = c.volumeDriver
cfg.Volumes = c.volumes
task.AllocDir = allocDir
task.Name = "demo"
require.NoError(t, task.EncodeConcreteDriverConfig(cfg))
cc, err := driver.createContainerConfig(task, cfg, "org/repo:0.1")
require.NoError(t, err)
for _, v := range c.expectedVolumes {
require.Contains(t, cc.HostConfig.Binds, v)
}
})
}
})
t.Run("with volumes disabled", func(t *testing.T) {
dh := dockerDriverHarness(t, nil)
driver := dh.Impl().(*Driver)
driver.config.Volumes.Enabled = false
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
task, cfg, _ := dockerTask(t)
cfg.VolumeDriver = c.volumeDriver
cfg.Volumes = c.volumes
task.AllocDir = allocDir
task.Name = "demo"
require.NoError(t, task.EncodeConcreteDriverConfig(cfg))
cc, err := driver.createContainerConfig(task, cfg, "org/repo:0.1")
if c.requiresVolumes {
require.Error(t, err, "volumes are not enabled")
} else {
require.NoError(t, err)
for _, v := range c.expectedVolumes {
require.Contains(t, cc.HostConfig.Binds, v)
}
}
})
}
})
}
func TestDockerDriver_VolumesEnabled(t *testing.T) {
if !tu.IsTravis() {
t.Parallel()
......@@ -1810,13 +1946,8 @@ func TestDockerDriver_MountsSerialization(t *testing.T) {
},
},
{
// FIXME: This needs to be true but we have a bug with security implications.
// The relative paths check should restrict access to alloc-dir subtree
// documenting existing behavior in test here and need to follow up in another commit
requiresVolumes: false,
name: "bind relative outside",
name: "bind relative outside",
requiresVolumes: true,
passedMounts: []DockerMount{
{
Type: "bind",
......
......@@ -5,6 +5,7 @@ import (
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"github.com/docker/cli/cli/config/configfile"
......@@ -200,3 +201,20 @@ func validateCgroupPermission(s string) bool {
return true
}
// expandPath returns the absolute path of dir, relative to base if dir is relative path.
// base is expected to be an absolute path
func expandPath(base, dir string) string {
if filepath.IsAbs(dir) {
return filepath.Clean(dir)
}
return filepath.Clean(filepath.Join(base, dir))
}
// isParentPath returns true if path is a child or decendant of parent path.
// Both inputes need to be absolute paths.
func isParentPath(parent, path string) bool {
rel, err := filepath.Rel(parent, path)
return err == nil && !strings.HasPrefix(rel, "..")
}
......@@ -35,3 +35,38 @@ func TestValidateCgroupPermission(t *testing.T) {
}
}
func TestExpandPath(t *testing.T) {
cases := []struct {
base string
target string
expected string
}{
{"/tmp/alloc/task", "/home/user", "/home/user"},
{"/tmp/alloc/task", "/home/user/..", "/home"},
{"/tmp/alloc/task", ".", "/tmp/alloc/task"},
{"/tmp/alloc/task", "..", "/tmp/alloc"},
{"/tmp/alloc/task", "d1/d2", "/tmp/alloc/task/d1/d2"},
{"/tmp/alloc/task", "../d1/d2", "/tmp/alloc/d1/d2"},
{"/tmp/alloc/task", "../../d1/d2", "/tmp/d1/d2"},
}
for _, c := range cases {
t.Run(c.expected, func(t *testing.T) {
require.Equal(t, c.expected, expandPath(c.base, c.target))
})
}
}
func TestIsParentPath(t *testing.T) {
require.True(t, isParentPath("/a/b/c", "/a/b/c"))
require.True(t, isParentPath("/a/b/c", "/a/b/c/d"))
require.True(t, isParentPath("/a/b/c", "/a/b/c/d/e"))
require.False(t, isParentPath("/a/b/c", "/a/b/d"))
require.False(t, isParentPath("/a/b/c", "/a/b/cd"))
require.False(t, isParentPath("/a/b/c", "/a/d/c"))
require.False(t, isParentPath("/a/b/c", "/d/e/c"))
}
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment