update template and artifact interpolation to use client-relative paths

resolves #9839
resolves #6929
resolves #6910

e2e: template env interpolation path testing

client/allocrunner/taskrunner/artifact_hook.go

@@ -52,7 +52,8 @@ func (h *artifactHook) Prestart(ctx context.Context, req *interfaces.TaskPrestar
 
 		h.logger.Debug("downloading artifact", "artifact", artifact.GetterSource)
 		//XXX add ctx to GetArtifact to allow cancelling long downloads
-		if err := getter.GetArtifact(req.TaskEnv, artifact, req.TaskDir.Dir); err != nil {
+		if err := getter.GetArtifact(req.TaskEnv, artifact); err != nil {
+
 			wrapped := structs.NewRecoverableError(
 				fmt.Errorf("failed to download artifact %q: %v", artifact.GetterSource, err),
 				true,

client/allocrunner/taskrunner/artifact_hook_test.go

@@ -94,7 +94,7 @@ func TestTaskRunner_ArtifactHook_PartialDone(t *testing.T) {
 	}()
 
 	req := &interfaces.TaskPrestartRequest{
-		TaskEnv: taskenv.NewEmptyTaskEnv(),
+		TaskEnv: taskenv.NewTaskEnv(nil, nil, nil, nil, destdir, ""),
 		TaskDir: &allocdir.TaskDir{Dir: destdir},
 		Task: &structs.Task{
 			Artifacts: []*structs.TaskArtifact{

client/allocrunner/taskrunner/envoy_version_hook_test.go

@@ -17,10 +17,10 @@ import (
 )
 
 var (
-	taskEnvDefault = taskenv.NewTaskEnv(nil, nil, map[string]string{
+	taskEnvDefault = taskenv.NewTaskEnv(nil, nil, nil, map[string]string{
 		"meta.connect.sidecar_image": envoy.ImageFormat,
 		"meta.connect.gateway_image": envoy.ImageFormat,
-	})
+	}, "", "")
 )
 
 func TestEnvoyVersionHook_semver(t *testing.T) {
@@ -140,7 +140,9 @@ func TestEnvoyVersionHook_interpolateImage(t *testing.T) {
 		}
 		hook.interpolateImage(task, taskenv.NewTaskEnv(map[string]string{
 			"MY_ENVOY": "my/envoy",
-		}, nil, nil))
+		}, map[string]string{
+			"MY_ENVOY": "my/envoy",
+		}, nil, nil, "", ""))
 		require.Equal(t, "my/envoy", task.Config["image"])
 	})
 

client/allocrunner/taskrunner/getter/getter.go

@@ -5,12 +5,11 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
-	"path/filepath"
 	"strings"
 	"sync"
 
 	gg "github.com/hashicorp/go-getter"
-	"github.com/hashicorp/nomad/helper"
+
 	"github.com/hashicorp/nomad/nomad/structs"
 )
 
@@ -33,6 +32,7 @@ const (
 // is usually satisfied by taskenv.TaskEnv.
 type EnvReplacer interface {
 	ReplaceEnv(string) string
+	ClientPath(string, bool) (string, bool)
 }
 
 func makeGetters(headers http.Header) map[string]gg.Getter {
@@ -130,21 +130,18 @@ func getHeaders(env EnvReplacer, m map[string]string) http.Header {
 }
 
 // GetArtifact downloads an artifact into the specified task directory.
-func GetArtifact(taskEnv EnvReplacer, artifact *structs.TaskArtifact, taskDir string) error {
+func GetArtifact(taskEnv EnvReplacer, artifact *structs.TaskArtifact) error {
 	ggURL, err := getGetterUrl(taskEnv, artifact)
 	if err != nil {
 		return newGetError(artifact.GetterSource, err, false)
 	}
 
+	dest, escapes := taskEnv.ClientPath(artifact.RelativeDest, true)
 	// Verify the destination is still in the task sandbox after interpolation
-	// Note: we *always* join here even if we get passed an absolute path so
-	// that $NOMAD_SECRETS_DIR and friends can be used and always fall inside
-	// the task working directory
-	dest := filepath.Join(taskDir, artifact.RelativeDest)
-	escapes := helper.PathEscapesSandbox(taskDir, dest)
 	if escapes {
 		return newGetError(artifact.RelativeDest,
-			errors.New("artifact destination path escapes the alloc directory"), false)
+			errors.New("artifact destination path escapes the alloc directory"),
+			false)
 	}
 
 	// Convert from string getter mode to go-getter const

client/allocrunner/taskrunner/getter/getter_test.go

@@ -15,39 +15,69 @@ import (
 	"testing"
 
 	"github.com/hashicorp/nomad/client/taskenv"
+	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/stretchr/testify/require"
 )
 
 // noopReplacer is a noop version of taskenv.TaskEnv.ReplaceEnv.
-type noopReplacer struct{}
+type noopReplacer struct {
+	taskDir string
+}
+
+func clientPath(taskDir, path string, join bool) (string, bool) {
+	if !filepath.IsAbs(path) || (helper.PathEscapesSandbox(taskDir, path) && join) {
+		path = filepath.Join(taskDir, path)
+	}
+	path = filepath.Clean(path)
+	if taskDir != "" && !helper.PathEscapesSandbox(taskDir, path) {
+		return path, false
+	}
+	return path, true
+}
 
 func (noopReplacer) ReplaceEnv(s string) string {
 	return s
 }
 
-var noopTaskEnv = noopReplacer{}
+func (r noopReplacer) ClientPath(p string, join bool) (string, bool) {
+	path, escapes := clientPath(r.taskDir, r.ReplaceEnv(p), join)
+	return path, escapes
+}
+
+func noopTaskEnv(taskDir string) EnvReplacer {
+	return noopReplacer{
+		taskDir: taskDir,
+	}
+}
 
 // upperReplacer is a version of taskenv.TaskEnv.ReplaceEnv that upper-cases
 // the given input.
-type upperReplacer struct{}
+type upperReplacer struct {
+	taskDir string
+}
 
 func (upperReplacer) ReplaceEnv(s string) string {
 	return strings.ToUpper(s)
 }
 
+func (u upperReplacer) ClientPath(p string, join bool) (string, bool) {
+	path, escapes := clientPath(u.taskDir, u.ReplaceEnv(p), join)
+	return path, escapes
+}
+
 func removeAllT(t *testing.T, path string) {
 	require.NoError(t, os.RemoveAll(path))
 }
 
 func TestGetArtifact_getHeaders(t *testing.T) {
 	t.Run("nil", func(t *testing.T) {
-		require.Nil(t, getHeaders(noopTaskEnv, nil))
+		require.Nil(t, getHeaders(noopTaskEnv(""), nil))
 	})
 
 	t.Run("empty", func(t *testing.T) {
-		require.Nil(t, getHeaders(noopTaskEnv, make(map[string]string)))
+		require.Nil(t, getHeaders(noopTaskEnv(""), make(map[string]string)))
 	})
 
 	t.Run("set", func(t *testing.T) {
@@ -94,12 +124,14 @@ func TestGetArtifact_Headers(t *testing.T) {
 	}
 
 	// Download the artifact.
-	taskEnv := new(upperReplacer)
-	err = GetArtifact(taskEnv, artifact, taskDir)
+	taskEnv := upperReplacer{
+		taskDir: taskDir,
+	}
+	err = GetArtifact(taskEnv, artifact)
 	require.NoError(t, err)
 
 	// Verify artifact exists.
-	b, err := ioutil.ReadFile(filepath.Join(taskDir, file))
+	b, err := ioutil.ReadFile(filepath.Join(taskDir, taskEnv.ReplaceEnv(file)))
 	require.NoError(t, err)
 
 	// Verify we wrote the interpolated header value into the file that is our
@@ -129,7 +161,7 @@ func TestGetArtifact_FileAndChecksum(t *testing.T) {
 	}
 
 	// Download the artifact
-	if err := GetArtifact(noopTaskEnv, artifact, taskDir); err != nil {
+	if err := GetArtifact(noopTaskEnv(taskDir), artifact); err != nil {
 		t.Fatalf("GetArtifact failed: %v", err)
 	}
 
@@ -163,7 +195,7 @@ func TestGetArtifact_File_RelativeDest(t *testing.T) {
 	}
 
 	// Download the artifact
-	if err := GetArtifact(noopTaskEnv, artifact, taskDir); err != nil {
+	if err := GetArtifact(noopTaskEnv(taskDir), artifact); err != nil {
 		t.Fatalf("GetArtifact failed: %v", err)
 	}
 
@@ -197,7 +229,7 @@ func TestGetArtifact_File_EscapeDest(t *testing.T) {
 	}
 
 	// attempt to download the artifact
-	err = GetArtifact(noopTaskEnv, artifact, taskDir)
+	err = GetArtifact(noopTaskEnv(taskDir), artifact)
 	if err == nil || !strings.Contains(err.Error(), "escapes") {
 		t.Fatalf("expected GetArtifact to disallow sandbox escape: %v", err)
 	}
@@ -247,7 +279,7 @@ func TestGetArtifact_InvalidChecksum(t *testing.T) {
 	}
 
 	// Download the artifact and expect an error
-	if err := GetArtifact(noopTaskEnv, artifact, taskDir); err == nil {
+	if err := GetArtifact(noopTaskEnv(taskDir), artifact); err == nil {
 		t.Fatalf("GetArtifact should have failed")
 	}
 }
@@ -312,7 +344,7 @@ func TestGetArtifact_Archive(t *testing.T) {
 		},
 	}
 
-	if err := GetArtifact(noopTaskEnv, artifact, taskDir); err != nil {
+	if err := GetArtifact(noopTaskEnv(taskDir), artifact); err != nil {
 		t.Fatalf("GetArtifact failed: %v", err)
 	}
 
@@ -345,7 +377,7 @@ func TestGetArtifact_Setuid(t *testing.T) {
 		},
 	}
 
-	require.NoError(t, GetArtifact(noopTaskEnv, artifact, taskDir))
+	require.NoError(t, GetArtifact(noopTaskEnv(taskDir), artifact))
 
 	var expected map[string]int
 
@@ -470,7 +502,7 @@ func TestGetGetterUrl_Queries(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			act, err := getGetterUrl(noopTaskEnv, c.artifact)
+			act, err := getGetterUrl(noopTaskEnv(""), c.artifact)
 			if err != nil {
 				t.Fatalf("want %q; got err %v", c.output, err)
 			} else if act != c.output {

client/allocrunner/taskrunner/task_dir_hook.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	log "github.com/hashicorp/go-hclog"
+
 	"github.com/hashicorp/nomad/client/allocdir"
 	"github.com/hashicorp/nomad/client/allocrunner/interfaces"
 	cconfig "github.com/hashicorp/nomad/client/config"
@@ -76,6 +77,12 @@ func (h *taskDirHook) Prestart(ctx context.Context, req *interfaces.TaskPrestart
 
 // setEnvvars sets path and host env vars depending on the FS isolation used.
 func setEnvvars(envBuilder *taskenv.Builder, fsi drivers.FSIsolation, taskDir *allocdir.TaskDir, conf *cconfig.Config) {
+
+	envBuilder.SetClientTaskRoot(taskDir.Dir)
+	envBuilder.SetClientSharedAllocDir(taskDir.SharedAllocDir)
+	envBuilder.SetClientTaskLocalDir(taskDir.LocalDir)
+	envBuilder.SetClientTaskSecretsDir(taskDir.SecretsDir)
+
 	// Set driver-specific environment variables
 	switch fsi {
 	case drivers.FSIsolationNone:
@@ -93,11 +100,10 @@ func setEnvvars(envBuilder *taskenv.Builder, fsi drivers.FSIsolation, taskDir *a
 	// Set the host environment variables for non-image based drivers
 	if fsi != drivers.FSIsolationImage {
 		// COMPAT(1.0) using inclusive language, blacklist is kept for backward compatibility.
-		denylist := conf.ReadAlternativeDefault(
+		filter := strings.Split(conf.ReadAlternativeDefault(
 			[]string{"env.denylist", "env.blacklist"},
 			cconfig.DefaultEnvDenylist,
-		)
-		filter := strings.Split(denylist, ",")
+		), ",")
 		envBuilder.SetHostEnvvars(filter)
 	}
 }

client/allocrunner/taskrunner/template/template.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"math/rand"
 	"os"
-	"path/filepath"
 	"sort"
 	"strconv"
 	"strings"
@@ -18,7 +17,6 @@ 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"
@@ -211,7 +209,7 @@ func (tm *TaskTemplateManager) run() {
 	}
 
 	// Read environment variables from env templates before we unblock
-	envMap, err := loadTemplateEnv(tm.config.Templates, tm.config.TaskDir, tm.config.EnvBuilder.Build())
+	envMap, err := loadTemplateEnv(tm.config.Templates, tm.config.EnvBuilder.Build())
 	if err != nil {
 		tm.config.Lifecycle.Kill(context.Background(),
 			structs.NewTaskEvent(structs.TaskKilling).
@@ -416,7 +414,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time
 		}
 
 		// Read environment variables from templates
-		envMap, err := loadTemplateEnv(tm.config.Templates, tm.config.TaskDir, tm.config.EnvBuilder.Build())
+		envMap, err := loadTemplateEnv(tm.config.Templates, tm.config.EnvBuilder.Build())
 		if err != nil {
 			tm.config.Lifecycle.Kill(context.Background(),
 				structs.NewTaskEvent(structs.TaskKilling).
@@ -571,41 +569,20 @@ 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
 		if tmpl.SourcePath != "" {
-			src = taskEnv.ReplaceEnv(tmpl.SourcePath)
-			if !filepath.IsAbs(src) {
-				src = filepath.Join(config.TaskDir, src)
-			} else {
-				src = filepath.Clean(src)
-			}
-			escapes := helper.PathEscapesSandbox(config.TaskDir, src)
+			var escapes bool
+			src, escapes = taskEnv.ClientPath(tmpl.SourcePath, false)
 			if escapes && sandboxEnabled {
 				return nil, sourceEscapesErr
 			}
 		}
 
 		if tmpl.DestPath != "" {
-			dest = taskEnv.ReplaceEnv(tmpl.DestPath)
-			// Note: we *always* join here even if we get passed an absolute
-			// path so that $NOMAD_SECRETS_DIR and friends can be used and
-			// always fall inside the task working directory
-			dest = filepath.Join(config.TaskDir, dest)
-			escapes := helper.PathEscapesSandbox(config.TaskDir, dest)
+			var escapes bool
+			dest, escapes = taskEnv.ClientPath(tmpl.DestPath, true)
 			if escapes && sandboxEnabled {
 				return nil, destEscapesErr
 			}
@@ -740,14 +717,15 @@ func newRunnerConfig(config *TaskTemplateManagerConfig,
 }
 
 // loadTemplateEnv loads task environment variables from all templates.
-func loadTemplateEnv(tmpls []*structs.Template, taskDir string, taskEnv *taskenv.TaskEnv) (map[string]string, error) {
+func loadTemplateEnv(tmpls []*structs.Template, taskEnv *taskenv.TaskEnv) (map[string]string, error) {
 	all := make(map[string]string, 50)
 	for _, t := range tmpls {
 		if !t.Envvars {
 			continue
 		}
 
-		dest := filepath.Join(taskDir, taskEnv.ReplaceEnv(t.DestPath))
+		// we checked escape before we rendered the file
+		dest, _ := taskEnv.ClientPath(t.DestPath, true)
 		f, err := os.Open(dest)
 		if err != nil {
 			return nil, fmt.Errorf("error opening env template: %v", err)