From 37f053ff896c7d14a9445362e230227a6cb1ce6f Mon Sep 17 00:00:00 2001
From: Michael Schurter <mschurter@hashicorp.com>
Date: Fri, 15 Oct 2021 16:56:14 -0700
Subject: [PATCH 1/3] client: never embed alloc_dir in chroot

Fixes #2522

Skip embedding client.alloc_dir when building chroot. If a user
configures a Nomad client agent so that the chroot_env will embed the
client.alloc_dir, Nomad will happily infinitely recurse while building
the chroot until something horrible happens. The best case scenario is
the filesystem's path length limit is hit. The worst case scenario is
disk space is exhausted.

A bad agent configuration will look something like this:

```hcl
data_dir = "/tmp/nomad-badagent"

client {
  enabled = true

  chroot_env {
    # Note that the source matches the data_dir
    "/tmp/nomad-badagent" = "/ohno"
    # ...
  }
}
```

Note that `/ohno/client` (the state_dir) will still be created but not
`/ohno/alloc` (the alloc_dir).
While I cannot think of a good reason why someone would want to embed
Nomad's client (and possibly server) directories in chroots, there
should be no cause for harm. chroots are only built when Nomad runs as
root, and Nomad disables running exec jobs as root by default. Therefore
even if client state is copied into chroots, it will be inaccessible to
tasks.

Skipping the `data_dir` and `{client,server}.state_dir` is possible, but
this PR attempts to implement the minimum viable solution to reduce risk
of unintended side effects or bugs.

When running tests as root in a vm without the fix, the following error
occurs:

```
=== RUN   TestAllocDir_SkipAllocDir
    alloc_dir_test.go:520:
                Error Trace:    alloc_dir_test.go:520
                Error:          Received unexpected error:
                                Couldn't create destination file /tmp/TestAllocDir_SkipAllocDir1457747331/001/nomad/test/testtask/nomad/test/testtask/.../nomad/test/testtask/secrets/.nomad-mount: open /tmp/TestAllocDir_SkipAllocDir1457747331/001/nomad/test/.../testtask/secrets/.nomad-mount: file name too long
                Test:           TestAllocDir_SkipAllocDir
--- FAIL: TestAllocDir_SkipAllocDir (22.76s)
```

Also removed unused Copy methods on AllocDir and TaskDir structs.

Thanks to @eveld for not letting me forget about this!
---
 client/allocdir/alloc_dir.go                  | 40 +++------
 client/allocdir/alloc_dir_test.go             | 86 +++++++++++++------
 client/allocdir/task_dir.go                   | 25 +++---
 client/allocdir/task_dir_test.go              |  8 +-
 client/allocdir/testing.go                    |  4 +-
 client/allocrunner/alloc_runner.go            |  3 +-
 .../allocrunner/consul_grpc_sock_hook_test.go |  8 +-
 .../allocrunner/consul_http_sock_hook_test.go |  4 +-
 .../taskrunner/connect_native_hook_test.go    | 11 ++-
 .../taskrunner/envoy_bootstrap_hook_test.go   | 15 ++--
 .../taskrunner/envoy_version_hook_test.go     |  8 +-
 .../taskrunner/template/template_test.go      |  2 +-
 client/allocwatcher/alloc_watcher.go          |  2 +-
 client/allocwatcher/alloc_watcher_test.go     |  5 +-
 client/fs_endpoint_test.go                    |  2 +-
 command/agent/consul/int_test.go              |  3 +-
 .../shared/executor/executor_linux_test.go    |  2 +-
 drivers/shared/executor/executor_test.go      |  2 +-
 plugins/drivers/testutils/testing.go          |  2 +-
 website/content/docs/configuration/client.mdx |  3 +
 website/content/docs/configuration/index.mdx  |  4 -
 21 files changed, 128 insertions(+), 111 deletions(-)

diff --git a/client/allocdir/alloc_dir.go b/client/allocdir/alloc_dir.go
index 537e79d18ae..0dc52029cad 100644
--- a/client/allocdir/alloc_dir.go
+++ b/client/allocdir/alloc_dir.go
@@ -84,6 +84,10 @@ type AllocDir struct {
 	// TaskDirs is a mapping of task names to their non-shared directory.
 	TaskDirs map[string]*TaskDir
 
+	// clientAllocDir is the client agent's root alloc directory. It must
+	// be excluded from chroots and is configured via client.alloc_dir.
+	clientAllocDir string
+
 	// built is true if Build has successfully run
 	built bool
 
@@ -104,36 +108,16 @@ type AllocDirFS interface {
 
 // NewAllocDir initializes the AllocDir struct with allocDir as base path for
 // the allocation directory.
-func NewAllocDir(logger hclog.Logger, allocDir string) *AllocDir {
+func NewAllocDir(logger hclog.Logger, clientAllocDir, allocID string) *AllocDir {
 	logger = logger.Named("alloc_dir")
+	allocDir := filepath.Join(clientAllocDir, allocID)
 	return &AllocDir{
-		AllocDir:  allocDir,
-		SharedDir: filepath.Join(allocDir, SharedAllocName),
-		TaskDirs:  make(map[string]*TaskDir),
-		logger:    logger,
-	}
-}
-
-// Copy an AllocDir and all of its TaskDirs. Returns nil if AllocDir is
-// nil.
-func (d *AllocDir) Copy() *AllocDir {
-	if d == nil {
-		return nil
-	}
-
-	d.mu.RLock()
-	defer d.mu.RUnlock()
-
-	dcopy := &AllocDir{
-		AllocDir:  d.AllocDir,
-		SharedDir: d.SharedDir,
-		TaskDirs:  make(map[string]*TaskDir, len(d.TaskDirs)),
-		logger:    d.logger,
-	}
-	for k, v := range d.TaskDirs {
-		dcopy.TaskDirs[k] = v.Copy()
+		clientAllocDir: clientAllocDir,
+		AllocDir:       allocDir,
+		SharedDir:      filepath.Join(allocDir, SharedAllocName),
+		TaskDirs:       make(map[string]*TaskDir),
+		logger:         logger,
 	}
-	return dcopy
 }
 
 // NewTaskDir creates a new TaskDir and adds it to the AllocDirs TaskDirs map.
@@ -141,7 +125,7 @@ func (d *AllocDir) NewTaskDir(name string) *TaskDir {
 	d.mu.Lock()
 	defer d.mu.Unlock()
 
-	td := newTaskDir(d.logger, d.AllocDir, name)
+	td := newTaskDir(d.logger, d.clientAllocDir, d.AllocDir, name)
 	d.TaskDirs[name] = td
 	return td
 }
diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go
index 8dc61b5cd62..86cd8d917c3 100644
--- a/client/allocdir/alloc_dir_test.go
+++ b/client/allocdir/alloc_dir_test.go
@@ -5,6 +5,7 @@ import (
 	"bytes"
 	"context"
 	"io"
+	"io/fs"
 	"io/ioutil"
 	"log"
 	"os"
@@ -53,7 +54,7 @@ func TestAllocDir_BuildAlloc(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	defer d.Destroy()
 	d.NewTaskDir(t1.Name)
 	d.NewTaskDir(t2.Name)
@@ -103,7 +104,7 @@ func TestAllocDir_MountSharedAlloc(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	defer d.Destroy()
 	if err := d.Build(); err != nil {
 		t.Fatalf("Build() failed: %v", err)
@@ -148,7 +149,7 @@ func TestAllocDir_Snapshot(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	defer d.Destroy()
 	if err := d.Build(); err != nil {
 		t.Fatalf("Build() failed: %v", err)
@@ -235,13 +236,13 @@ func TestAllocDir_Move(t *testing.T) {
 	defer os.RemoveAll(tmp2)
 
 	// Create two alloc dirs
-	d1 := NewAllocDir(testlog.HCLogger(t), tmp1)
+	d1 := NewAllocDir(testlog.HCLogger(t), tmp1, "test")
 	if err := d1.Build(); err != nil {
 		t.Fatalf("Build() failed: %v", err)
 	}
 	defer d1.Destroy()
 
-	d2 := NewAllocDir(testlog.HCLogger(t), tmp2)
+	d2 := NewAllocDir(testlog.HCLogger(t), tmp2, "test")
 	if err := d2.Build(); err != nil {
 		t.Fatalf("Build() failed: %v", err)
 	}
@@ -296,7 +297,7 @@ func TestAllocDir_EscapeChecking(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	if err := d.Build(); err != nil {
 		t.Fatalf("Build() failed: %v", err)
 	}
@@ -337,7 +338,7 @@ func TestAllocDir_ReadAt_SecretDir(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	if err := d.Build(); err != nil {
 		t.Fatalf("Build() failed: %v", err)
 	}
@@ -419,25 +420,6 @@ func TestAllocDir_CreateDir(t *testing.T) {
 	}
 }
 
-// TestAllocDir_Copy asserts that AllocDir.Copy does a deep copy of itself and
-// all TaskDirs.
-func TestAllocDir_Copy(t *testing.T) {
-	a := NewAllocDir(testlog.HCLogger(t), "foo")
-	a.NewTaskDir("bar")
-	a.NewTaskDir("baz")
-
-	b := a.Copy()
-
-	// Clear the logger
-	require.Equal(t, a, b)
-
-	// Make sure TaskDirs map is copied
-	a.NewTaskDir("new")
-	if b.TaskDirs["new"] != nil {
-		t.Errorf("TaskDirs map shared between copied")
-	}
-}
-
 func TestPathFuncs(t *testing.T) {
 	dir, err := ioutil.TempDir("", "nomadtest-pathfuncs")
 	if err != nil {
@@ -502,3 +484,55 @@ func TestAllocDir_DetectContentType(t *testing.T) {
 		require.Equal(expectedEncodings[file], res, "unexpected output for %v", file)
 	}
 }
+
+// TestAllocDir_SkipAllocDir asserts that building a chroot which contains
+// itself will *not* infinitely recurse. AllocDirs should always skip embedding
+// themselves into chroots.
+//
+// Warning: If this test fails it may fill your disk before failing, so be
+// careful and/or confident.
+func TestAllocDir_SkipAllocDir(t *testing.T) {
+	MountCompatible(t)
+
+	// Create root, alloc, and other dirs
+	rootDir := t.TempDir()
+
+	clientAllocDir := filepath.Join(rootDir, "nomad")
+	require.NoError(t, os.Mkdir(clientAllocDir, fs.ModeDir|0o777))
+
+	otherDir := filepath.Join(rootDir, "etc")
+	require.NoError(t, os.Mkdir(otherDir, fs.ModeDir|0o777))
+
+	// chroot contains client.alloc_dir! This could cause infinite
+	// recursion.
+	chroot := map[string]string{
+		rootDir: "/",
+	}
+
+	allocDir := NewAllocDir(testlog.HCLogger(t), clientAllocDir, "test")
+	taskDir := allocDir.NewTaskDir("testtask")
+
+	require.NoError(t, allocDir.Build())
+	defer allocDir.Destroy()
+
+	// Build chroot
+	err := taskDir.Build(true, chroot)
+	require.NoError(t, err)
+
+	// Assert other directory *was* embedded
+	embeddedOtherDir := filepath.Join(clientAllocDir, "test", "testtask", "etc")
+	if _, err := os.Stat(embeddedOtherDir); os.IsNotExist(err) {
+		t.Fatalf("expected other directory to exist at: %q", embeddedOtherDir)
+	}
+
+	// Assert client.alloc_dir was *not* embedded
+	embeddedChroot := filepath.Join(clientAllocDir, "test", "testtask", "nomad")
+	s, err := os.Stat(embeddedChroot)
+	if s != nil {
+		t.Logf("somehow you managed to embed the chroot without causing infinite recursion!")
+		t.Fatalf("expected chroot to not exist at: %q", embeddedChroot)
+	}
+	if !os.IsNotExist(err) {
+		t.Fatalf("expected chroot to not exist but error is: %v", err)
+	}
+}
diff --git a/client/allocdir/task_dir.go b/client/allocdir/task_dir.go
index 9d99f971754..d516c313cf1 100644
--- a/client/allocdir/task_dir.go
+++ b/client/allocdir/task_dir.go
@@ -39,6 +39,10 @@ type TaskDir struct {
 	// <task_dir>/secrets/
 	SecretsDir string
 
+	// skip embedding these paths in chroots. Used for avoiding embedding
+	// client.alloc_dir recursively.
+	skip map[string]struct{}
+
 	logger hclog.Logger
 }
 
@@ -46,11 +50,14 @@ type TaskDir struct {
 // create paths on disk.
 //
 // Call AllocDir.NewTaskDir to create new TaskDirs
-func newTaskDir(logger hclog.Logger, allocDir, taskName string) *TaskDir {
+func newTaskDir(logger hclog.Logger, clientAllocDir, allocDir, taskName string) *TaskDir {
 	taskDir := filepath.Join(allocDir, taskName)
 
 	logger = logger.Named("task_dir").With("task_name", taskName)
 
+	// skip embedding client.alloc_dir in chroots
+	skip := map[string]struct{}{clientAllocDir: {}}
+
 	return &TaskDir{
 		AllocDir:       allocDir,
 		Dir:            taskDir,
@@ -59,21 +66,14 @@ func newTaskDir(logger hclog.Logger, allocDir, taskName string) *TaskDir {
 		SharedTaskDir:  filepath.Join(taskDir, SharedAllocName),
 		LocalDir:       filepath.Join(taskDir, TaskLocal),
 		SecretsDir:     filepath.Join(taskDir, TaskSecrets),
+		skip:           skip,
 		logger:         logger,
 	}
 }
 
-// Copy a TaskDir. Panics if TaskDir is nil as TaskDirs should never be nil.
-func (t *TaskDir) Copy() *TaskDir {
-	// No nested structures other than the logger which is safe to share,
-	// so just copy the struct
-	tcopy := *t
-	return &tcopy
-}
-
 // Build default directories and permissions in a task directory. chrootCreated
 // allows skipping chroot creation if the caller knows it has already been
-// done.
+// done. client.alloc_dir will be skipped.
 func (t *TaskDir) Build(createChroot bool, chroot map[string]string) error {
 	if err := os.MkdirAll(t.Dir, 0777); err != nil {
 		return err
@@ -149,6 +149,11 @@ func (t *TaskDir) buildChroot(entries map[string]string) error {
 func (t *TaskDir) embedDirs(entries map[string]string) error {
 	subdirs := make(map[string]string)
 	for source, dest := range entries {
+		if _, ok := t.skip[source]; ok {
+			// source in skip list
+			continue
+		}
+
 		// Check to see if directory exists on host.
 		s, err := os.Stat(source)
 		if os.IsNotExist(err) {
diff --git a/client/allocdir/task_dir_test.go b/client/allocdir/task_dir_test.go
index b39c275f2ba..61aa3b3027c 100644
--- a/client/allocdir/task_dir_test.go
+++ b/client/allocdir/task_dir_test.go
@@ -17,7 +17,7 @@ func TestTaskDir_EmbedNonexistent(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	defer d.Destroy()
 	td := d.NewTaskDir(t1.Name)
 	if err := d.Build(); err != nil {
@@ -39,7 +39,7 @@ func TestTaskDir_EmbedDirs(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	defer d.Destroy()
 	td := d.NewTaskDir(t1.Name)
 	if err := d.Build(); err != nil {
@@ -96,7 +96,7 @@ func TestTaskDir_NonRoot_Image(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	defer d.Destroy()
 	td := d.NewTaskDir(t1.Name)
 	if err := d.Build(); err != nil {
@@ -119,7 +119,7 @@ func TestTaskDir_NonRoot(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(testlog.HCLogger(t), tmp)
+	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
 	defer d.Destroy()
 	td := d.NewTaskDir(t1.Name)
 	if err := d.Build(); err != nil {
diff --git a/client/allocdir/testing.go b/client/allocdir/testing.go
index 6e870fb4231..6ea7d7bb990 100644
--- a/client/allocdir/testing.go
+++ b/client/allocdir/testing.go
@@ -10,13 +10,13 @@ import (
 
 // TestAllocDir returns a built alloc dir in a temporary directory and cleanup
 // func.
-func TestAllocDir(t testing.T, l hclog.Logger, prefix string) (*AllocDir, func()) {
+func TestAllocDir(t testing.T, l hclog.Logger, prefix, id string) (*AllocDir, func()) {
 	dir, err := ioutil.TempDir("", prefix)
 	if err != nil {
 		t.Fatalf("Couldn't create temp dir: %v", err)
 	}
 
-	allocDir := NewAllocDir(l, dir)
+	allocDir := NewAllocDir(l, dir, id)
 
 	cleanup := func() {
 		if err := os.RemoveAll(dir); err != nil {
diff --git a/client/allocrunner/alloc_runner.go b/client/allocrunner/alloc_runner.go
index 31a6956c2f6..7dcf072a948 100644
--- a/client/allocrunner/alloc_runner.go
+++ b/client/allocrunner/alloc_runner.go
@@ -3,7 +3,6 @@ package allocrunner
 import (
 	"context"
 	"fmt"
-	"path/filepath"
 	"sync"
 	"time"
 
@@ -227,7 +226,7 @@ func NewAllocRunner(config *Config) (*allocRunner, error) {
 	ar.allocBroadcaster = cstructs.NewAllocBroadcaster(ar.logger)
 
 	// Create alloc dir
-	ar.allocDir = allocdir.NewAllocDir(ar.logger, filepath.Join(config.ClientConfig.AllocDir, alloc.ID))
+	ar.allocDir = allocdir.NewAllocDir(ar.logger, config.ClientConfig.AllocDir, alloc.ID)
 
 	ar.taskHookCoordinator = newTaskHookCoordinator(ar.logger, tg.Tasks)
 
diff --git a/client/allocrunner/consul_grpc_sock_hook_test.go b/client/allocrunner/consul_grpc_sock_hook_test.go
index f700c8ce4ec..2730ac62703 100644
--- a/client/allocrunner/consul_grpc_sock_hook_test.go
+++ b/client/allocrunner/consul_grpc_sock_hook_test.go
@@ -39,7 +39,7 @@ func TestConsulGRPCSocketHook_PrerunPostrun_Ok(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap")
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID)
 	defer cleanup()
 
 	// Start the unix socket proxy
@@ -105,15 +105,15 @@ func TestConsulGRPCSocketHook_Prerun_Error(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap")
-	defer cleanup()
-
 	// A config without an Addr or GRPCAddr is invalid.
 	consulConfig := &config.ConsulConfig{}
 
 	alloc := mock.Alloc()
 	connectAlloc := mock.ConnectAlloc()
 
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID)
+	defer cleanup()
+
 	{
 		// An alloc without a Connect proxy sidecar should not return
 		// an error.
diff --git a/client/allocrunner/consul_http_sock_hook_test.go b/client/allocrunner/consul_http_sock_hook_test.go
index 7fdfe04c985..05bc31908a7 100644
--- a/client/allocrunner/consul_http_sock_hook_test.go
+++ b/client/allocrunner/consul_http_sock_hook_test.go
@@ -28,7 +28,7 @@ func TestConsulSocketHook_PrerunPostrun_Ok(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask")
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask", alloc.ID)
 	defer cleanupDir()
 
 	// start unix socket proxy
@@ -93,7 +93,7 @@ func TestConsulHTTPSocketHook_Prerun_Error(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask")
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask", alloc.ID)
 	defer cleanupDir()
 
 	consulConfig := new(config.ConsulConfig)
diff --git a/client/allocrunner/taskrunner/connect_native_hook_test.go b/client/allocrunner/taskrunner/connect_native_hook_test.go
index b9a447fe3cc..2ce016d9b49 100644
--- a/client/allocrunner/taskrunner/connect_native_hook_test.go
+++ b/client/allocrunner/taskrunner/connect_native_hook_test.go
@@ -272,11 +272,10 @@ func TestTaskRunner_ConnectNativeHook_Noop(t *testing.T) {
 	t.Parallel()
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative")
-	defer cleanup()
-
 	alloc := mock.Alloc()
 	task := alloc.Job.LookupTaskGroup(alloc.TaskGroup).Tasks[0]
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID)
+	defer cleanup()
 
 	// run the connect native hook. use invalid consul address as it should not get hit
 	h := newConnectNativeHook(newConnectNativeHookConfig(alloc, &config.ConsulConfig{
@@ -328,7 +327,7 @@ func TestTaskRunner_ConnectNativeHook_Ok(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative")
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID)
 	defer cleanup()
 
 	// register group services
@@ -393,7 +392,7 @@ func TestTaskRunner_ConnectNativeHook_with_SI_token(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative")
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID)
 	defer cleanup()
 
 	// register group services
@@ -590,7 +589,7 @@ func TestTaskRunner_ConnectNativeHook_shareTLS_override(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative")
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID)
 	defer cleanup()
 
 	// register group services
diff --git a/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go b/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go
index 5d473613fb3..33e0420f196 100644
--- a/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go
+++ b/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go
@@ -329,7 +329,7 @@ func TestEnvoyBootstrapHook_with_SI_token(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap")
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID)
 	defer cleanup()
 
 	// Register Group Services
@@ -430,7 +430,7 @@ func TestTaskRunner_EnvoyBootstrapHook_sidecar_ok(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap")
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID)
 	defer cleanup()
 
 	// Register Group Services
@@ -495,7 +495,7 @@ func TestTaskRunner_EnvoyBootstrapHook_gateway_ok(t *testing.T) {
 
 	// Setup an Allocation
 	alloc := mock.ConnectIngressGatewayAlloc("bridge")
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyBootstrapIngressGateway")
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyBootstrapIngressGateway", alloc.ID)
 	defer cleanupDir()
 
 	// Get a Consul client
@@ -573,11 +573,10 @@ func TestTaskRunner_EnvoyBootstrapHook_Noop(t *testing.T) {
 	t.Parallel()
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap")
-	defer cleanup()
-
 	alloc := mock.Alloc()
 	task := alloc.Job.LookupTaskGroup(alloc.TaskGroup).Tasks[0]
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID)
+	defer cleanup()
 
 	// Run Envoy bootstrap Hook. Use invalid Consul address as it should
 	// not get hit.
@@ -646,7 +645,7 @@ func TestTaskRunner_EnvoyBootstrapHook_RecoverableError(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap")
+	allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID)
 	defer cleanup()
 
 	// Unlike the successful test above, do NOT register the group services
@@ -724,7 +723,7 @@ func TestTaskRunner_EnvoyBootstrapHook_retryTimeout(t *testing.T) {
 		Kind: structs.NewTaskKind(structs.ConnectProxyPrefix, "foo"),
 	}
 	tg.Tasks = append(tg.Tasks, sidecarTask)
-	allocDir, cleanupAlloc := allocdir.TestAllocDir(t, logger, "EnvoyBootstrapRetryTimeout")
+	allocDir, cleanupAlloc := allocdir.TestAllocDir(t, logger, "EnvoyBootstrapRetryTimeout", alloc.ID)
 	defer cleanupAlloc()
 
 	// Get a Consul client
diff --git a/client/allocrunner/taskrunner/envoy_version_hook_test.go b/client/allocrunner/taskrunner/envoy_version_hook_test.go
index 26d739bd7c7..299a9961f5d 100644
--- a/client/allocrunner/taskrunner/envoy_version_hook_test.go
+++ b/client/allocrunner/taskrunner/envoy_version_hook_test.go
@@ -272,7 +272,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_custom(t *testing.T) {
 	alloc := mock.ConnectAlloc()
 	alloc.Job.TaskGroups[0].Tasks[0] = mock.ConnectSidecarTask()
 	alloc.Job.TaskGroups[0].Tasks[0].Config["image"] = "custom-${NOMAD_envoy_version}:latest"
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook")
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID)
 	defer cleanupDir()
 
 	// Setup a mock for Consul API
@@ -319,7 +319,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_skip(t *testing.T) {
 	alloc.Job.TaskGroups[0].Tasks[0].Config = map[string]interface{}{
 		"command": "/sidecar",
 	}
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook")
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID)
 	defer cleanupDir()
 
 	// Setup a mock for Consul API
@@ -362,7 +362,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_fallback(t *testing.T) {
 	// Setup an Allocation
 	alloc := mock.ConnectAlloc()
 	alloc.Job.TaskGroups[0].Tasks[0] = mock.ConnectSidecarTask()
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook")
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID)
 	defer cleanupDir()
 
 	// Setup a mock for Consul API
@@ -403,7 +403,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_error(t *testing.T) {
 	// Setup an Allocation
 	alloc := mock.ConnectAlloc()
 	alloc.Job.TaskGroups[0].Tasks[0] = mock.ConnectSidecarTask()
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook")
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID)
 	defer cleanupDir()
 
 	// Setup a mock for Consul API
diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go
index 84fca0a9d25..32fa862a6cc 100644
--- a/client/allocrunner/taskrunner/template/template_test.go
+++ b/client/allocrunner/taskrunner/template/template_test.go
@@ -1581,7 +1581,7 @@ func TestTaskTemplateManager_Escapes(t *testing.T) {
 	alloc := mock.Alloc()
 	task := alloc.Job.TaskGroups[0].Tasks[0]
 	logger := testlog.HCLogger(t)
-	allocDir := allocdir.NewAllocDir(logger, filepath.Join(clientConf.AllocDir, alloc.ID))
+	allocDir := allocdir.NewAllocDir(logger, clientConf.AllocDir, alloc.ID)
 	taskDir := allocDir.NewTaskDir(task.Name)
 
 	containerEnv := func() *taskenv.Builder {
diff --git a/client/allocwatcher/alloc_watcher.go b/client/allocwatcher/alloc_watcher.go
index 35d3954022a..4e6bbb2cf78 100644
--- a/client/allocwatcher/alloc_watcher.go
+++ b/client/allocwatcher/alloc_watcher.go
@@ -509,7 +509,7 @@ func (p *remotePrevAlloc) getNodeAddr(ctx context.Context, nodeID string) (strin
 // Destroy on the returned allocdir if no error occurs.
 func (p *remotePrevAlloc) migrateAllocDir(ctx context.Context, nodeAddr string) (*allocdir.AllocDir, error) {
 	// Create the previous alloc dir
-	prevAllocDir := allocdir.NewAllocDir(p.logger, filepath.Join(p.config.AllocDir, p.prevAllocID))
+	prevAllocDir := allocdir.NewAllocDir(p.logger, p.config.AllocDir, p.prevAllocID)
 	if err := prevAllocDir.Build(); err != nil {
 		return nil, fmt.Errorf("error building alloc dir for previous alloc %q: %v", p.prevAllocID, err)
 	}
diff --git a/client/allocwatcher/alloc_watcher_test.go b/client/allocwatcher/alloc_watcher_test.go
index ea92525a48e..4c4b6370240 100644
--- a/client/allocwatcher/alloc_watcher_test.go
+++ b/client/allocwatcher/alloc_watcher_test.go
@@ -36,12 +36,11 @@ func newFakeAllocRunner(t *testing.T, logger hclog.Logger) *fakeAllocRunner {
 	alloc.Job.TaskGroups[0].EphemeralDisk.Sticky = true
 	alloc.Job.TaskGroups[0].EphemeralDisk.Migrate = true
 
-	path, err := ioutil.TempDir("", "nomad_test_watcher")
-	require.NoError(t, err)
+	path := t.TempDir()
 
 	return &fakeAllocRunner{
 		alloc:       alloc,
-		AllocDir:    allocdir.NewAllocDir(logger, path),
+		AllocDir:    allocdir.NewAllocDir(logger, path, alloc.ID),
 		Broadcaster: cstructs.NewAllocBroadcaster(logger),
 	}
 }
diff --git a/client/fs_endpoint_test.go b/client/fs_endpoint_test.go
index 60bb0deefff..a8c71265c35 100644
--- a/client/fs_endpoint_test.go
+++ b/client/fs_endpoint_test.go
@@ -43,7 +43,7 @@ func tempAllocDir(t testing.TB) *allocdir.AllocDir {
 		t.Fatalf("failed to chmod dir: %v", err)
 	}
 
-	return allocdir.NewAllocDir(testlog.HCLogger(t), dir)
+	return allocdir.NewAllocDir(testlog.HCLogger(t), dir, "test")
 }
 
 type nopWriteCloser struct {
diff --git a/command/agent/consul/int_test.go b/command/agent/consul/int_test.go
index f725afd638b..12aa80f8f35 100644
--- a/command/agent/consul/int_test.go
+++ b/command/agent/consul/int_test.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"io/ioutil"
 	"os"
-	"path/filepath"
 	"testing"
 	"time"
 
@@ -126,7 +125,7 @@ func TestConsul_Integration(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 	logUpdate := &mockUpdater{logger}
-	allocDir := allocdir.NewAllocDir(logger, filepath.Join(conf.AllocDir, alloc.ID))
+	allocDir := allocdir.NewAllocDir(logger, conf.AllocDir, alloc.ID)
 	if err := allocDir.Build(); err != nil {
 		t.Fatalf("error building alloc dir: %v", err)
 	}
diff --git a/drivers/shared/executor/executor_linux_test.go b/drivers/shared/executor/executor_linux_test.go
index a8a71f88448..16d5fdc3f71 100644
--- a/drivers/shared/executor/executor_linux_test.go
+++ b/drivers/shared/executor/executor_linux_test.go
@@ -64,7 +64,7 @@ func testExecutorCommandWithChroot(t *testing.T) *testExecCmd {
 	task := alloc.Job.TaskGroups[0].Tasks[0]
 	taskEnv := taskenv.NewBuilder(mock.Node(), alloc, task, "global").Build()
 
-	allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), filepath.Join(os.TempDir(), alloc.ID))
+	allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), t.TempDir(), alloc.ID)
 	if err := allocDir.Build(); err != nil {
 		t.Fatalf("AllocDir.Build() failed: %v", err)
 	}
diff --git a/drivers/shared/executor/executor_test.go b/drivers/shared/executor/executor_test.go
index cd15321b65b..6a396ea6a8f 100644
--- a/drivers/shared/executor/executor_test.go
+++ b/drivers/shared/executor/executor_test.go
@@ -61,7 +61,7 @@ func testExecutorCommand(t *testing.T) *testExecCmd {
 	task := alloc.Job.TaskGroups[0].Tasks[0]
 	taskEnv := taskenv.NewBuilder(mock.Node(), alloc, task, "global").Build()
 
-	allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), filepath.Join(os.TempDir(), alloc.ID))
+	allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), t.TempDir(), alloc.ID)
 	if err := allocDir.Build(); err != nil {
 		t.Fatalf("AllocDir.Build() failed: %v", err)
 	}
diff --git a/plugins/drivers/testutils/testing.go b/plugins/drivers/testutils/testing.go
index 8088381db6c..b26b7e1f32b 100644
--- a/plugins/drivers/testutils/testing.go
+++ b/plugins/drivers/testutils/testing.go
@@ -85,7 +85,7 @@ func (h *DriverHarness) MkAllocDir(t *drivers.TaskConfig, enableLogs bool) func(
 	require.NoError(h.t, err)
 	t.AllocDir = dir
 
-	allocDir := allocdir.NewAllocDir(h.logger, dir)
+	allocDir := allocdir.NewAllocDir(h.logger, dir, t.AllocID)
 	require.NoError(h.t, allocDir.Build())
 	taskDir := allocDir.NewTaskDir(t.Name)
 
diff --git a/website/content/docs/configuration/client.mdx b/website/content/docs/configuration/client.mdx
index 368de73d14d..f1a9ecc43cf 100644
--- a/website/content/docs/configuration/client.mdx
+++ b/website/content/docs/configuration/client.mdx
@@ -189,6 +189,9 @@ environment with the most commonly used parts of the operating system. Please
 see the [Nomad `exec` driver documentation](/docs/drivers/exec#chroot) for
 the full list.
 
+As of Nomad 1.2, Nomad will never attempt to embed the `alloc_dir` in the
+chroot as doing so would cause infinite recursion.
+
 ### `options` Parameters
 
 ~> Note: In Nomad 0.9 client configuration options for drivers were deprecated.
diff --git a/website/content/docs/configuration/index.mdx b/website/content/docs/configuration/index.mdx
index 1d3d2bf37fa..7013a3ef546 100644
--- a/website/content/docs/configuration/index.mdx
+++ b/website/content/docs/configuration/index.mdx
@@ -151,10 +151,6 @@ testing.
   directory to store cluster state, including the replicated log and snapshot
   data. This must be specified as an absolute path.
 
-  ~> **WARNING**: This directory **must not** be set to a directory that is
-  [included in the chroot](/docs/drivers/exec#chroot) if you use the
-  [`exec`](/docs/drivers/exec) driver.
-
 - `disable_anonymous_signature` `(bool: false)` - Specifies if Nomad should
   provide an anonymous signature for de-duplication with the update check.
 

From f41b625532d1e452af9d3c391e67274183007d99 Mon Sep 17 00:00:00 2001
From: Michael Schurter <mschurter@hashicorp.com>
Date: Fri, 15 Oct 2021 17:13:12 -0700
Subject: [PATCH 2/3] docs: add #11334 to changelog

---
 .changelog/11334.txt | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 .changelog/11334.txt

diff --git a/.changelog/11334.txt b/.changelog/11334.txt
new file mode 100644
index 00000000000..df88d6e7eed
--- /dev/null
+++ b/.changelog/11334.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+client: Never embed client.alloc_dir in chroots to prevent infinite recursion from misconfiguration.
+```

From eeb1da8a2ef6bc6379d1766bf2d2723bbea30e44 Mon Sep 17 00:00:00 2001
From: Michael Schurter <mschurter@hashicorp.com>
Date: Mon, 18 Oct 2021 10:32:41 -0700
Subject: [PATCH 3/3] test: update tests to properly use AllocDir

Also use t.TempDir when possible.
---
 .../allocrunner/consul_http_sock_hook_test.go |  6 +--
 .../taskrunner/connect_native_hook_test.go    |  2 +-
 .../taskrunner/dispatch_hook_test.go          | 15 ++++---
 .../taskrunner/envoy_version_hook_test.go     |  2 +-
 .../taskrunner/task_runner_test.go            |  3 +-
 client/fs_endpoint_test.go                    | 43 +++++++++----------
 drivers/docker/driver_unix_test.go            |  6 +--
 .../shared/executor/executor_linux_test.go    |  2 +-
 plugins/drivers/testutils/testing.go          |  4 +-
 testutil/wait.go                              |  4 +-
 10 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/client/allocrunner/consul_http_sock_hook_test.go b/client/allocrunner/consul_http_sock_hook_test.go
index 05bc31908a7..3d5a97ec55e 100644
--- a/client/allocrunner/consul_http_sock_hook_test.go
+++ b/client/allocrunner/consul_http_sock_hook_test.go
@@ -93,14 +93,14 @@ func TestConsulHTTPSocketHook_Prerun_Error(t *testing.T) {
 
 	logger := testlog.HCLogger(t)
 
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask", alloc.ID)
-	defer cleanupDir()
-
 	consulConfig := new(config.ConsulConfig)
 
 	alloc := mock.Alloc()
 	connectNativeAlloc := mock.ConnectNativeAlloc("bridge")
 
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask", alloc.ID)
+	defer cleanupDir()
+
 	{
 		// an alloc without a connect native task should not return an error
 		h := newConsulHTTPSocketHook(logger, alloc, allocDir, consulConfig)
diff --git a/client/allocrunner/taskrunner/connect_native_hook_test.go b/client/allocrunner/taskrunner/connect_native_hook_test.go
index 2ce016d9b49..a9c43d21016 100644
--- a/client/allocrunner/taskrunner/connect_native_hook_test.go
+++ b/client/allocrunner/taskrunner/connect_native_hook_test.go
@@ -469,7 +469,7 @@ func TestTaskRunner_ConnectNativeHook_shareTLS(t *testing.T) {
 
 		logger := testlog.HCLogger(t)
 
-		allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative")
+		allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID)
 		defer cleanup()
 
 		// register group services
diff --git a/client/allocrunner/taskrunner/dispatch_hook_test.go b/client/allocrunner/taskrunner/dispatch_hook_test.go
index 9ac683a146e..9f56fe0fd1d 100644
--- a/client/allocrunner/taskrunner/dispatch_hook_test.go
+++ b/client/allocrunner/taskrunner/dispatch_hook_test.go
@@ -26,12 +26,13 @@ func TestTaskRunner_DispatchHook_NoPayload(t *testing.T) {
 	require := require.New(t)
 	ctx := context.Background()
 	logger := testlog.HCLogger(t)
-	allocDir := allocdir.NewAllocDir(logger, "nomadtest_nopayload")
-	defer allocDir.Destroy()
 
 	// Default mock alloc/job is not a dispatch job
 	alloc := mock.BatchAlloc()
 	task := alloc.Job.TaskGroups[0].Tasks[0]
+
+	allocDir := allocdir.NewAllocDir(logger, "nomadtest_nopayload", alloc.ID)
+	defer allocDir.Destroy()
 	taskDir := allocDir.NewTaskDir(task.Name)
 	require.NoError(taskDir.Build(false, nil))
 
@@ -61,8 +62,6 @@ func TestTaskRunner_DispatchHook_Ok(t *testing.T) {
 	require := require.New(t)
 	ctx := context.Background()
 	logger := testlog.HCLogger(t)
-	allocDir := allocdir.NewAllocDir(logger, "nomadtest_dispatchok")
-	defer allocDir.Destroy()
 
 	// Default mock alloc/job is not a dispatch job; update it
 	alloc := mock.BatchAlloc()
@@ -77,6 +76,9 @@ func TestTaskRunner_DispatchHook_Ok(t *testing.T) {
 	task.DispatchPayload = &structs.DispatchPayloadConfig{
 		File: "out",
 	}
+
+	allocDir := allocdir.NewAllocDir(logger, "nomadtest_dispatchok", alloc.ID)
+	defer allocDir.Destroy()
 	taskDir := allocDir.NewTaskDir(task.Name)
 	require.NoError(taskDir.Build(false, nil))
 
@@ -104,8 +106,6 @@ func TestTaskRunner_DispatchHook_Error(t *testing.T) {
 	require := require.New(t)
 	ctx := context.Background()
 	logger := testlog.HCLogger(t)
-	allocDir := allocdir.NewAllocDir(logger, "nomadtest_dispatcherr")
-	defer allocDir.Destroy()
 
 	// Default mock alloc/job is not a dispatch job; update it
 	alloc := mock.BatchAlloc()
@@ -121,6 +121,9 @@ func TestTaskRunner_DispatchHook_Error(t *testing.T) {
 	task.DispatchPayload = &structs.DispatchPayloadConfig{
 		File: "out",
 	}
+
+	allocDir := allocdir.NewAllocDir(logger, "nomadtest_dispatcherr", alloc.ID)
+	defer allocDir.Destroy()
 	taskDir := allocDir.NewTaskDir(task.Name)
 	require.NoError(taskDir.Build(false, nil))
 
diff --git a/client/allocrunner/taskrunner/envoy_version_hook_test.go b/client/allocrunner/taskrunner/envoy_version_hook_test.go
index 299a9961f5d..225c8d6e683 100644
--- a/client/allocrunner/taskrunner/envoy_version_hook_test.go
+++ b/client/allocrunner/taskrunner/envoy_version_hook_test.go
@@ -228,7 +228,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_standard(t *testing.T) {
 	// Setup an Allocation
 	alloc := mock.ConnectAlloc()
 	alloc.Job.TaskGroups[0].Tasks[0] = mock.ConnectSidecarTask()
-	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook")
+	allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID)
 	defer cleanupDir()
 
 	// Setup a mock for Consul API
diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go
index 9e81abd316f..f3ef206cfd6 100644
--- a/client/allocrunner/taskrunner/task_runner_test.go
+++ b/client/allocrunner/taskrunner/task_runner_test.go
@@ -80,8 +80,7 @@ func testTaskRunnerConfig(t *testing.T, alloc *structs.Allocation, taskName stri
 	}
 
 	// Create the alloc dir + task dir
-	allocPath := filepath.Join(clientConf.AllocDir, alloc.ID)
-	allocDir := allocdir.NewAllocDir(logger, allocPath)
+	allocDir := allocdir.NewAllocDir(logger, clientConf.AllocDir, alloc.ID)
 	if err := allocDir.Build(); err != nil {
 		cleanup()
 		t.Fatalf("error building alloc dir: %v", err)
diff --git a/client/fs_endpoint_test.go b/client/fs_endpoint_test.go
index a8c71265c35..fa650df2767 100644
--- a/client/fs_endpoint_test.go
+++ b/client/fs_endpoint_test.go
@@ -31,19 +31,14 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// tempAllocDir returns a new alloc dir that is rooted in a temp dir. The caller
-// should destroy the temp dir.
+// tempAllocDir returns a new alloc dir that is rooted in a temp dir. Caller
+// should cleanup with AllocDir.Destroy()
 func tempAllocDir(t testing.TB) *allocdir.AllocDir {
-	dir, err := ioutil.TempDir("", "nomadtest")
-	if err != nil {
-		t.Fatalf("TempDir() failed: %v", err)
-	}
+	dir := t.TempDir()
 
-	if err := os.Chmod(dir, 0777); err != nil {
-		t.Fatalf("failed to chmod dir: %v", err)
-	}
+	require.NoError(t, os.Chmod(dir, 0o777))
 
-	return allocdir.NewAllocDir(testlog.HCLogger(t), dir, "test")
+	return allocdir.NewAllocDir(testlog.HCLogger(t), dir, "test_allocid")
 }
 
 type nopWriteCloser struct {
@@ -1561,12 +1556,11 @@ func TestFS_findClosest(t *testing.T) {
 
 func TestFS_streamFile_NoFile(t *testing.T) {
 	t.Parallel()
-	require := require.New(t)
 	c, cleanup := TestClient(t, nil)
 	defer cleanup()
 
 	ad := tempAllocDir(t)
-	defer os.RemoveAll(ad.AllocDir)
+	defer ad.Destroy()
 
 	frames := make(chan *sframer.StreamFrame, 32)
 	framer := sframer.NewStreamFramer(frames, streamHeartbeatRate, streamBatchWindow, streamFrameSize)
@@ -1575,11 +1569,11 @@ func TestFS_streamFile_NoFile(t *testing.T) {
 
 	err := c.endpoints.FileSystem.streamFile(
 		context.Background(), 0, "foo", 0, ad, framer, nil)
-	require.NotNil(err)
+	require.Error(t, err)
 	if runtime.GOOS == "windows" {
-		require.Contains(err.Error(), "cannot find the file")
+		require.Contains(t, err.Error(), "cannot find the file")
 	} else {
-		require.Contains(err.Error(), "no such file")
+		require.Contains(t, err.Error(), "no such file")
 	}
 }
 
@@ -1591,7 +1585,8 @@ func TestFS_streamFile_Modify(t *testing.T) {
 
 	// Get a temp alloc dir
 	ad := tempAllocDir(t)
-	defer os.RemoveAll(ad.AllocDir)
+	require.NoError(t, ad.Build())
+	defer ad.Destroy()
 
 	// Create a file in the temp dir
 	streamFile := "stream_file"
@@ -1660,16 +1655,15 @@ func TestFS_streamFile_Truncate(t *testing.T) {
 
 	// Get a temp alloc dir
 	ad := tempAllocDir(t)
-	defer os.RemoveAll(ad.AllocDir)
+	require.NoError(t, ad.Build())
+	defer ad.Destroy()
 
 	// Create a file in the temp dir
 	data := []byte("helloworld")
 	streamFile := "stream_file"
 	streamFilePath := filepath.Join(ad.AllocDir, streamFile)
 	f, err := os.Create(streamFilePath)
-	if err != nil {
-		t.Fatalf("Failed to create file: %v", err)
-	}
+	require.NoError(t, err)
 	defer f.Close()
 
 	// Start the reader
@@ -1768,7 +1762,8 @@ func TestFS_streamImpl_Delete(t *testing.T) {
 
 	// Get a temp alloc dir
 	ad := tempAllocDir(t)
-	defer os.RemoveAll(ad.AllocDir)
+	require.NoError(t, ad.Build())
+	defer ad.Destroy()
 
 	// Create a file in the temp dir
 	data := []byte("helloworld")
@@ -1840,7 +1835,8 @@ func TestFS_logsImpl_NoFollow(t *testing.T) {
 
 	// Get a temp alloc dir and create the log dir
 	ad := tempAllocDir(t)
-	defer os.RemoveAll(ad.AllocDir)
+	require.NoError(t, ad.Build())
+	defer ad.Destroy()
 
 	logDir := filepath.Join(ad.SharedDir, allocdir.LogDirName)
 	if err := os.MkdirAll(logDir, 0777); err != nil {
@@ -1908,7 +1904,8 @@ func TestFS_logsImpl_Follow(t *testing.T) {
 
 	// Get a temp alloc dir and create the log dir
 	ad := tempAllocDir(t)
-	defer os.RemoveAll(ad.AllocDir)
+	require.NoError(t, ad.Build())
+	defer ad.Destroy()
 
 	logDir := filepath.Join(ad.SharedDir, allocdir.LogDirName)
 	if err := os.MkdirAll(logDir, 0777); err != nil {
diff --git a/drivers/docker/driver_unix_test.go b/drivers/docker/driver_unix_test.go
index 019b42764b1..701ed898571 100644
--- a/drivers/docker/driver_unix_test.go
+++ b/drivers/docker/driver_unix_test.go
@@ -765,15 +765,15 @@ func copyImage(t *testing.T, taskDir *allocdir.TaskDir, image string) {
 
 // copyFile moves an existing file to the destination
 func copyFile(src, dst string, t *testing.T) {
+	t.Helper()
 	in, err := os.Open(src)
 	if err != nil {
 		t.Fatalf("copying %v -> %v failed: %v", src, dst, err)
 	}
 	defer in.Close()
 	out, err := os.Create(dst)
-	if err != nil {
-		t.Fatalf("copying %v -> %v failed: %v", src, dst, err)
-	}
+	require.NoError(t, err, "copying %v -> %v failed: %v", src, dst, err)
+
 	defer func() {
 		if err := out.Close(); err != nil {
 			t.Fatalf("copying %v -> %v failed: %v", src, dst, err)
diff --git a/drivers/shared/executor/executor_linux_test.go b/drivers/shared/executor/executor_linux_test.go
index 16d5fdc3f71..687c646350e 100644
--- a/drivers/shared/executor/executor_linux_test.go
+++ b/drivers/shared/executor/executor_linux_test.go
@@ -64,7 +64,7 @@ func testExecutorCommandWithChroot(t *testing.T) *testExecCmd {
 	task := alloc.Job.TaskGroups[0].Tasks[0]
 	taskEnv := taskenv.NewBuilder(mock.Node(), alloc, task, "global").Build()
 
-	allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), t.TempDir(), alloc.ID)
+	allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), os.TempDir(), alloc.ID)
 	if err := allocDir.Build(); err != nil {
 		t.Fatalf("AllocDir.Build() failed: %v", err)
 	}
diff --git a/plugins/drivers/testutils/testing.go b/plugins/drivers/testutils/testing.go
index b26b7e1f32b..32cc2447731 100644
--- a/plugins/drivers/testutils/testing.go
+++ b/plugins/drivers/testutils/testing.go
@@ -83,10 +83,12 @@ func (h *DriverHarness) Kill() {
 func (h *DriverHarness) MkAllocDir(t *drivers.TaskConfig, enableLogs bool) func() {
 	dir, err := ioutil.TempDir("", "nomad_driver_harness-")
 	require.NoError(h.t, err)
-	t.AllocDir = dir
 
 	allocDir := allocdir.NewAllocDir(h.logger, dir, t.AllocID)
 	require.NoError(h.t, allocDir.Build())
+
+	t.AllocDir = allocDir.AllocDir
+
 	taskDir := allocDir.NewTaskDir(t.Name)
 
 	caps, err := h.Capabilities()
diff --git a/testutil/wait.go b/testutil/wait.go
index 4f9965a6a82..0f4dfffed63 100644
--- a/testutil/wait.go
+++ b/testutil/wait.go
@@ -201,7 +201,9 @@ func WaitForRunningWithToken(t testing.TB, rpc rpcFn, job *structs.Job, token st
 
 	var resp structs.JobAllocationsResponse
 
-	WaitForResult(func() (bool, error) {
+	// This can be quite slow if the job has expensive setup such as
+	// downloading large artifacts or creating a chroot.
+	WaitForResultRetries(2000*TestMultiplier(), func() (bool, error) {
 		args := &structs.JobSpecificRequest{}
 		args.JobID = job.ID
 		args.QueryOptions.Region = job.Region