Merge pull request #9383 from hashicorp/b-template-escape

client: fix interpolation in template source

client/allocrunner/taskrunner/template/template.go

@@ -2,6 +2,7 @@ package template
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math/rand"
 	"os"
@@ -17,6 +18,7 @@ import (
 	"github.com/hashicorp/consul-template/signals"
 	envparse "github.com/hashicorp/go-envparse"
 	multierror "github.com/hashicorp/go-multierror"
+	"github.com/hashicorp/nomad/client/allocdir"
 	"github.com/hashicorp/nomad/client/allocrunner/taskrunner/interfaces"
 	"github.com/hashicorp/nomad/client/config"
 	"github.com/hashicorp/nomad/client/taskenv"
@@ -38,6 +40,11 @@ const (
 	DefaultMaxTemplateEventRate = 3 * time.Second
 )
 
+var (
+	sourceEscapesErr = errors.New("template source path escapes alloc directory")
+	destEscapesErr   = errors.New("template destination path escapes alloc directory")
+)
+
 // TaskTemplateManager is used to run a set of templates for a given task
 type TaskTemplateManager struct {
 	// config holds the template managers configuration
@@ -548,6 +555,18 @@ func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.Templa
 	sandboxEnabled := !config.ClientConfig.TemplateConfig.DisableSandbox
 	taskEnv := config.EnvBuilder.Build()
 
+	// Make NOMAD_{ALLOC,TASK,SECRETS}_DIR relative paths to avoid treating
+	// them as sandbox escapes when using containers.
+	if taskEnv.EnvMap[taskenv.AllocDir] == allocdir.SharedAllocContainerPath {
+		taskEnv.EnvMap[taskenv.AllocDir] = allocdir.SharedAllocName
+	}
+	if taskEnv.EnvMap[taskenv.TaskLocalDir] == allocdir.TaskLocalContainerPath {
+		taskEnv.EnvMap[taskenv.TaskLocalDir] = allocdir.TaskLocal
+	}
+	if taskEnv.EnvMap[taskenv.SecretsDir] == allocdir.TaskSecretsContainerPath {
+		taskEnv.EnvMap[taskenv.SecretsDir] = allocdir.TaskSecrets
+	}
+
 	ctmpls := make(map[*ctconf.TemplateConfig]*structs.Template, len(config.Templates))
 	for _, tmpl := range config.Templates {
 		var src, dest string
@@ -560,7 +579,7 @@ func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.Templa
 			}
 			escapes := helper.PathEscapesSandbox(config.TaskDir, src)
 			if escapes && sandboxEnabled {
-				return nil, fmt.Errorf("template source path escapes alloc directory")
+				return nil, sourceEscapesErr
 			}
 		}
 
@@ -572,7 +591,7 @@ func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.Templa
 			dest = filepath.Join(config.TaskDir, dest)
 			escapes := helper.PathEscapesSandbox(config.TaskDir, dest)
 			if escapes && sandboxEnabled {
-				return nil, fmt.Errorf("template destination path escapes alloc directory")
+				return nil, destEscapesErr
 			}
 		}
 

client/allocrunner/taskrunner/template/template_test.go

@@ -17,9 +17,11 @@ import (
 	"time"
 
 	ctestutil "github.com/hashicorp/consul/sdk/testutil"
+	"github.com/hashicorp/nomad/client/allocdir"
 	"github.com/hashicorp/nomad/client/config"
 	"github.com/hashicorp/nomad/client/taskenv"
 	"github.com/hashicorp/nomad/helper"
+	"github.com/hashicorp/nomad/helper/testlog"
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
@@ -1452,6 +1454,255 @@ func TestTaskTemplateManager_Config_VaultNamespace_TaskOverride(t *testing.T) {
 	assert.Equal(overriddenNS, *ctconf.Vault.Namespace, "Vault Namespace Value")
 }
 
+// TestTaskTemplateManager_Escapes asserts that when sandboxing is enabled
+// interpolated paths are not incorrectly treated as escaping the alloc dir.
+func TestTaskTemplateManager_Escapes(t *testing.T) {
+	t.Parallel()
+
+	clientConf := config.DefaultConfig()
+	require.False(t, clientConf.TemplateConfig.DisableSandbox, "expected sandbox to be disabled")
+
+	// Set a fake alloc dir to make test output more realistic
+	clientConf.AllocDir = "/fake/allocdir"
+
+	clientConf.Node = mock.Node()
+	alloc := mock.Alloc()
+	task := alloc.Job.TaskGroups[0].Tasks[0]
+	logger := testlog.HCLogger(t)
+	allocDir := allocdir.NewAllocDir(logger, filepath.Join(clientConf.AllocDir, alloc.ID))
+	taskDir := allocDir.NewTaskDir(task.Name)
+
+	containerEnv := func() *taskenv.Builder {
+		// To emulate a Docker or exec tasks we must copy the
+		// Set{Alloc,Task,Secrets}Dir logic in taskrunner/task_dir_hook.go
+		b := taskenv.NewBuilder(clientConf.Node, alloc, task, clientConf.Region)
+		b.SetAllocDir(allocdir.SharedAllocContainerPath)
+		b.SetTaskLocalDir(allocdir.TaskLocalContainerPath)
+		b.SetSecretsDir(allocdir.TaskSecretsContainerPath)
+		return b
+	}
+
+	rawExecEnv := func() *taskenv.Builder {
+		// To emulate a unisolated tasks we must copy the
+		// Set{Alloc,Task,Secrets}Dir logic in taskrunner/task_dir_hook.go
+		b := taskenv.NewBuilder(clientConf.Node, alloc, task, clientConf.Region)
+		b.SetAllocDir(taskDir.SharedAllocDir)
+		b.SetTaskLocalDir(taskDir.LocalDir)
+		b.SetSecretsDir(taskDir.SecretsDir)
+		return b
+	}
+
+	cases := []struct {
+		Name   string
+		Config func() *TaskTemplateManagerConfig
+
+		// Set to skip a test; remove once bugs are fixed
+		Skip bool
+
+		// Expected paths to be returned if Err is nil
+		SourcePath string
+		DestPath   string
+
+		// Err is the expected error to be returned or nil
+		Err error
+	}{
+		{
+			Name: "ContainerOk",
+			Config: func() *TaskTemplateManagerConfig {
+				return &TaskTemplateManagerConfig{
+					ClientConfig: clientConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   containerEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "${NOMAD_TASK_DIR}/src",
+							DestPath:   "${NOMAD_SECRETS_DIR}/dst",
+						},
+					},
+				}
+			},
+			SourcePath: filepath.Join(taskDir.Dir, "local/src"),
+			DestPath:   filepath.Join(taskDir.Dir, "secrets/dst"),
+		},
+		{
+			Name: "ContainerSrcEscapesErr",
+			Config: func() *TaskTemplateManagerConfig {
+				return &TaskTemplateManagerConfig{
+					ClientConfig: clientConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   containerEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "/etc/src_escapes",
+							DestPath:   "${NOMAD_SECRETS_DIR}/dst",
+						},
+					},
+				}
+			},
+			Err: sourceEscapesErr,
+		},
+		{
+			Name: "ContainerSrcEscapesOk",
+			Config: func() *TaskTemplateManagerConfig {
+				unsafeConf := clientConf.Copy()
+				unsafeConf.TemplateConfig.DisableSandbox = true
+				return &TaskTemplateManagerConfig{
+					ClientConfig: unsafeConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   containerEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "/etc/src_escapes_ok",
+							DestPath:   "${NOMAD_SECRETS_DIR}/dst",
+						},
+					},
+				}
+			},
+			SourcePath: "/etc/src_escapes_ok",
+			DestPath:   filepath.Join(taskDir.Dir, "secrets/dst"),
+		},
+		{
+			Name: "ContainerDstAbsoluteOk",
+			Config: func() *TaskTemplateManagerConfig {
+				return &TaskTemplateManagerConfig{
+					ClientConfig: clientConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   containerEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "${NOMAD_TASK_DIR}/src",
+							DestPath:   "/etc/absolutely_relative",
+						},
+					},
+				}
+			},
+			SourcePath: filepath.Join(taskDir.Dir, "local/src"),
+			DestPath:   filepath.Join(taskDir.Dir, "etc/absolutely_relative"),
+		},
+		{
+			Name: "ContainerDstAbsoluteEscapesErr",
+			Config: func() *TaskTemplateManagerConfig {
+				return &TaskTemplateManagerConfig{
+					ClientConfig: clientConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   containerEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "${NOMAD_TASK_DIR}/src",
+							DestPath:   "../escapes",
+						},
+					},
+				}
+			},
+			Err: destEscapesErr,
+		},
+		{
+			Name: "ContainerDstAbsoluteEscapesOk",
+			Config: func() *TaskTemplateManagerConfig {
+				unsafeConf := clientConf.Copy()
+				unsafeConf.TemplateConfig.DisableSandbox = true
+				return &TaskTemplateManagerConfig{
+					ClientConfig: unsafeConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   containerEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "${NOMAD_TASK_DIR}/src",
+							DestPath:   "../escapes",
+						},
+					},
+				}
+			},
+			SourcePath: filepath.Join(taskDir.Dir, "local/src"),
+			DestPath:   filepath.Join(taskDir.Dir, "..", "escapes"),
+		},
+		//TODO: Fix this test. I *think* it should pass. The double
+		//      joining of the task dir onto the destination seems like
+		//      a bug. https://github.com/hashicorp/nomad/issues/9389
+		{
+			Skip: true,
+			Name: "RawExecOk",
+			Config: func() *TaskTemplateManagerConfig {
+				return &TaskTemplateManagerConfig{
+					ClientConfig: clientConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   rawExecEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "${NOMAD_TASK_DIR}/src",
+							DestPath:   "${NOMAD_SECRETS_DIR}/dst",
+						},
+					},
+				}
+			},
+			SourcePath: filepath.Join(taskDir.Dir, "local/src"),
+			DestPath:   filepath.Join(taskDir.Dir, "secrets/dst"),
+		},
+		{
+			Name: "RawExecSrcEscapesErr",
+			Config: func() *TaskTemplateManagerConfig {
+				return &TaskTemplateManagerConfig{
+					ClientConfig: clientConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   rawExecEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "/etc/src_escapes",
+							DestPath:   "${NOMAD_SECRETS_DIR}/dst",
+						},
+					},
+				}
+			},
+			Err: sourceEscapesErr,
+		},
+		{
+			Name: "RawExecDstAbsoluteOk",
+			Config: func() *TaskTemplateManagerConfig {
+				return &TaskTemplateManagerConfig{
+					ClientConfig: clientConf,
+					TaskDir:      taskDir.Dir,
+					EnvBuilder:   rawExecEnv(),
+					Templates: []*structs.Template{
+						{
+							SourcePath: "${NOMAD_TASK_DIR}/src",
+							DestPath:   "/etc/absolutely_relative",
+						},
+					},
+				}
+			},
+			SourcePath: filepath.Join(taskDir.Dir, "local/src"),
+			DestPath:   filepath.Join(taskDir.Dir, "etc/absolutely_relative"),
+		},
+	}
+
+	for i := range cases {
+		tc := cases[i]
+		t.Run(tc.Name, func(t *testing.T) {
+			if tc.Skip {
+				t.Skip("FIXME: Skipping broken test")
+			}
+			config := tc.Config()
+			mapping, err := parseTemplateConfigs(config)
+			if tc.Err == nil {
+				// Ok path
+				require.NoError(t, err)
+				require.NotNil(t, mapping)
+				require.Len(t, mapping, 1)
+				for k := range mapping {
+					require.Equal(t, tc.SourcePath, *k.Source)
+					require.Equal(t, tc.DestPath, *k.Destination)
+					t.Logf("Rendering %s => %s", *k.Source, *k.Destination)
+				}
+			} else {
+				// Err path
+				assert.EqualError(t, err, tc.Err.Error())
+				require.Nil(t, mapping)
+			}
+
+		})
+	}
+}
+
 func TestTaskTemplateManager_BlockedEvents(t *testing.T) {
 	// The tests sets a template that need keys 0, 1, 2, 3, 4,
 	// then subsequently sets 0, 1, 2 keys

e2e/consultemplate/consultemplate.go

@@ -7,10 +7,13 @@ import (
 	"time"
 
 	capi "github.com/hashicorp/consul/api"
+	"github.com/hashicorp/nomad/api"
+	"github.com/hashicorp/nomad/e2e/e2eutil"
 	e2e "github.com/hashicorp/nomad/e2e/e2eutil"
 	"github.com/hashicorp/nomad/e2e/framework"
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/jobspec"
+	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/testutil"
 )
 
@@ -223,6 +226,67 @@ job: {{ env "NOMAD_JOB_NAME" }}
 	}
 }
 
+// TestTemplatePathInterpolation_Ok asserts that NOMAD_*_DIR variables are
+// properly interpolated into template source and destination paths without
+// being treated as escaping.
+func (tc *ConsulTemplateTest) TestTemplatePathInterpolation_Ok(f *framework.F) {
+	jobID := "template-paths-" + uuid.Generate()[:8]
+	tc.jobIDs = append(tc.jobIDs, jobID)
+
+	allocStubs := e2eutil.RegisterAndWaitForAllocs(
+		f.T(), tc.Nomad(), "consultemplate/input/template_paths.nomad", jobID, "")
+	f.Len(allocStubs, 1)
+	allocID := allocStubs[0].ID
+
+	e2eutil.WaitForAllocRunning(f.T(), tc.Nomad(), allocID)
+
+	f.NoError(waitForTemplateRender(allocID, "task/secrets/foo/dst",
+		func(out string) bool {
+			return len(out) > 0
+		}, nil), "expected file to have contents")
+}
+
+// TestTemplatePathInterpolation_Bad asserts that template.source paths are not
+// allowed to escape the sandbox directory tree by default.
+func (tc *ConsulTemplateTest) TestTemplatePathInterpolation_Bad(f *framework.F) {
+	wc := &e2e.WaitConfig{}
+	interval, retries := wc.OrDefault()
+
+	jobID := "bad-template-paths-" + uuid.Generate()[:8]
+	tc.jobIDs = append(tc.jobIDs, jobID)
+
+	allocStubs := e2eutil.RegisterAndWaitForAllocs(
+		f.T(), tc.Nomad(), "consultemplate/input/bad_template_paths.nomad", jobID, "")
+	f.Len(allocStubs, 1)
+	allocID := allocStubs[0].ID
+
+	// Wait for alloc to fail
+	var err error
+	var alloc *api.Allocation
+	testutil.WaitForResultRetries(retries, func() (bool, error) {
+		time.Sleep(interval)
+		alloc, _, err = tc.Nomad().Allocations().Info(allocID, nil)
+		if err != nil {
+			return false, err
+		}
+
+		return alloc.ClientStatus == structs.AllocClientStatusFailed, fmt.Errorf("expected status failed, but was: %s", alloc.ClientStatus)
+	}, func(err error) {
+		f.T().Fatalf("failed to wait on alloc: %v", err)
+	})
+
+	// Assert the "source escapes" error occurred to prevent false
+	// positives.
+	found := false
+	for _, event := range alloc.TaskStates["task"].Events {
+		if strings.Contains(event.DisplayMessage, "template source path escapes alloc directory") {
+			found = true
+			break
+		}
+	}
+	f.True(found, "alloc failed but NOT due to expected source path escape error")
+}
+
 // waitForTemplateRender is a helper that grabs a file via alloc fs
 // and tests it for
 func waitForTemplateRender(allocID, path string, test func(string) bool, wc *e2e.WaitConfig) error {

e2e/consultemplate/input/bad_template_paths.nomad

@@ -0,0 +1,37 @@
+job "bad-template-paths" {
+  datacenters = ["dc1", "dc2"]
+
+  constraint {
+    attribute = "${attr.kernel.name}"
+    value     = "linux"
+  }
+
+  group "template-paths" {
+
+    restart {
+      attempts = 0
+      mode     = "fail"
+    }
+
+    task "task" {
+
+      driver = "docker"
+
+      config {
+        image   = "busybox:1"
+        command = "/bin/sh"
+        args    = ["-c", "sleep 300"]
+      }
+
+      template {
+        source      = "/etc/passwd"
+        destination = "${NOMAD_SECRETS_DIR}/foo/dst"
+      }
+
+      resources {
+        cpu    = 128
+        memory = 64
+      }
+    }
+  }
+}

e2e/consultemplate/input/template_paths.nomad

@@ -0,0 +1,37 @@
+job "template-paths" {
+  datacenters = ["dc1", "dc2"]
+
+  constraint {
+    attribute = "${attr.kernel.name}"
+    value     = "linux"
+  }
+
+  group "template-paths" {
+
+    task "task" {
+
+      driver = "docker"
+
+      config {
+        image   = "busybox:1"
+        command = "/bin/sh"
+        args    = ["-c", "sleep 300"]
+      }
+
+      artifact {
+        source      = "https://google.com"
+        destination = "local/foo/src"
+      }
+
+      template {
+        source      = "${NOMAD_TASK_DIR}/foo/src"
+        destination = "${NOMAD_SECRETS_DIR}/foo/dst"
+      }
+
+      resources {
+        cpu    = 128
+        memory = 64
+      }
+    }
+  }
+}