template: disable sandboxed rendering on Windows (#23432)

Following #23443, we no longer need to sandbox template rendering on Windows.

.changelog/23432.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+template: Fix template rendering on Windows
+```

client/allocrunner/taskrunner/template/template.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"log"
 	"math/rand"
 	"os"
 	"path/filepath"
@@ -19,17 +18,14 @@ import (
 
 	ctconf "github.com/hashicorp/consul-template/config"
 	"github.com/hashicorp/consul-template/manager"
-	"github.com/hashicorp/consul-template/renderer"
 	"github.com/hashicorp/consul-template/signals"
 	envparse "github.com/hashicorp/go-envparse"
 	"github.com/hashicorp/go-hclog"
 	multierror "github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/nomad/client/allocrunner/taskrunner/interfaces"
-	trenderer "github.com/hashicorp/nomad/client/allocrunner/taskrunner/template/renderer"
 	"github.com/hashicorp/nomad/client/config"
 	"github.com/hashicorp/nomad/client/taskenv"
 	"github.com/hashicorp/nomad/helper/pointer"
-	"github.com/hashicorp/nomad/helper/subproc"
 	"github.com/hashicorp/nomad/nomad/structs"
 	structsc "github.com/hashicorp/nomad/nomad/structs/config"
 )
@@ -194,13 +190,6 @@ func NewTaskTemplateManager(config *TaskTemplateManagerConfig) (*TaskTemplateMan
 		tm.signals[tmpl.ChangeSignal] = sig
 	}
 
-	// the platform sandbox needs to be created before we construct the runner
-	// so that reading the template is sandboxed
-	err := createPlatformSandbox(config)
-	if err != nil {
-		return nil, err
-	}
-
 	// Build the consul-template runner
 	runner, lookup, err := templateRunner(config)
 	if err != nil {
@@ -229,8 +218,6 @@ func (tm *TaskTemplateManager) Stop() {
 	if tm.runner != nil {
 		tm.runner.Stop()
 	}
-
-	destroyPlatformSandbox(tm.config)
 }
 
 // SetDriverHandle sets the executor
@@ -1014,90 +1001,6 @@ type sandboxConfig struct {
 	contents    []byte
 }
 
-func ReaderFn(taskID, taskDir string, sandboxEnabled bool) func(string) ([]byte, error) {
-	if !sandboxEnabled {
-		return nil
-	}
-	thisBin := subproc.Self()
-
-	return func(src string) ([]byte, error) {
-
-		sandboxCfg := &sandboxConfig{
-			thisBin:     thisBin,
-			sandboxPath: taskDir,
-			sourcePath:  src,
-			taskID:      taskID,
-		}
-
-		stdout, stderr, code, err := readTemplateFromSandbox(sandboxCfg)
-		if err != nil && code != 0 {
-			return nil, fmt.Errorf("%v: %s", err, string(stderr))
-		}
-
-		// this will get wrapped in CT log formatter
-		fmt.Fprintf(os.Stderr, "[DEBUG] %s", string(stderr))
-		return stdout, nil
-	}
-}
-
-func RenderFn(taskID, taskDir string, sandboxEnabled bool) func(*renderer.RenderInput) (*renderer.RenderResult, error) {
-	if !sandboxEnabled {
-		return nil
-	}
-	thisBin := subproc.Self()
-
-	return func(i *renderer.RenderInput) (*renderer.RenderResult, error) {
-		wouldRender := false
-		didRender := false
-
-		sandboxCfg := &sandboxConfig{
-			thisBin:     thisBin,
-			sandboxPath: taskDir,
-			destPath:    i.Path,
-			perms:       strconv.FormatUint(uint64(i.Perms), 8),
-			user:        i.User,
-			group:       i.Group,
-			taskID:      taskID,
-			contents:    i.Contents,
-		}
-
-		logs, code, err := renderTemplateInSandbox(sandboxCfg)
-		if err != nil {
-			if len(logs) > 0 {
-				log.Printf("[ERROR] %v: %s", err, logs)
-			} else {
-				log.Printf("[ERROR] %v", err)
-			}
-			return &renderer.RenderResult{
-				DidRender:   false,
-				WouldRender: false,
-				Contents:    []byte{},
-			}, fmt.Errorf("template render subprocess failed: %w", err)
-		}
-		if code == trenderer.ExitWouldRenderButDidnt {
-			didRender = false
-			wouldRender = true
-		} else {
-			didRender = true
-			wouldRender = true
-		}
-
-		// the subprocess emits logs matching the consul-template runner, but we
-		// CT doesn't support hclog, so we just print the whole output here to
-		// stderr the same way CT does so the results look seamless
-		if len(logs) > 0 {
-			log.Printf("[DEBUG] %s", logs)
-		}
-
-		result := &renderer.RenderResult{
-			DidRender:   didRender,
-			WouldRender: wouldRender,
-			Contents:    i.Contents,
-		}
-		return result, nil
-	}
-}
-
 // loadTemplateEnv loads task environment variables from all templates.
 func loadTemplateEnv(tmpls []*structs.Template, taskEnv *taskenv.TaskEnv) (map[string]string, error) {
 	all := make(map[string]string, 50)

client/allocrunner/taskrunner/template/template_default.go

@@ -8,19 +8,19 @@ package template
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"io"
+	"log"
+	"os"
 	"os/exec"
+	"strconv"
 	"time"
 
-	"github.com/hashicorp/nomad/client/allocrunner/taskrunner/template/renderer"
+	"github.com/hashicorp/consul-template/renderer"
+	trenderer "github.com/hashicorp/nomad/client/allocrunner/taskrunner/template/renderer"
+	"github.com/hashicorp/nomad/helper/subproc"
 )
 
-// createPlatformSandbox is a no-op outside of windows
-func createPlatformSandbox(_ *TaskTemplateManagerConfig) error { return nil }
-
-// destroyPlatformSandbox is a no-op outside of windows
-func destroyPlatformSandbox(_ *TaskTemplateManagerConfig) error { return nil }
-
 // renderTemplateInSandbox runs the template-render command in a subprocess that
 // will chroot itself to prevent a task from swapping a directory between the
 // sandbox path and the destination with a symlink pointing to somewhere outside
@@ -65,7 +65,7 @@ func renderTemplateInSandbox(cfg *sandboxConfig) (string, int, error) {
 
 	out, err := cmd.CombinedOutput()
 	code := cmd.ProcessState.ExitCode()
-	if code == renderer.ExitWouldRenderButDidnt {
+	if code == trenderer.ExitWouldRenderButDidnt {
 		err = nil // erase the "exit code 117" error
 	}
 
@@ -104,3 +104,87 @@ func readTemplateFromSandbox(cfg *sandboxConfig) ([]byte, []byte, int, error) {
 	stderr := errb.Bytes()
 	return stdout, stderr, cmd.ProcessState.ExitCode(), err
 }
+
+func RenderFn(taskID, taskDir string, sandboxEnabled bool) func(*renderer.RenderInput) (*renderer.RenderResult, error) {
+	if !sandboxEnabled {
+		return nil
+	}
+	thisBin := subproc.Self()
+
+	return func(i *renderer.RenderInput) (*renderer.RenderResult, error) {
+		wouldRender := false
+		didRender := false
+
+		sandboxCfg := &sandboxConfig{
+			thisBin:     thisBin,
+			sandboxPath: taskDir,
+			destPath:    i.Path,
+			perms:       strconv.FormatUint(uint64(i.Perms), 8),
+			user:        i.User,
+			group:       i.Group,
+			taskID:      taskID,
+			contents:    i.Contents,
+		}
+
+		logs, code, err := renderTemplateInSandbox(sandboxCfg)
+		if err != nil {
+			if len(logs) > 0 {
+				log.Printf("[ERROR] %v: %s", err, logs)
+			} else {
+				log.Printf("[ERROR] %v", err)
+			}
+			return &renderer.RenderResult{
+				DidRender:   false,
+				WouldRender: false,
+				Contents:    []byte{},
+			}, fmt.Errorf("template render subprocess failed: %w", err)
+		}
+		if code == trenderer.ExitWouldRenderButDidnt {
+			didRender = false
+			wouldRender = true
+		} else {
+			didRender = true
+			wouldRender = true
+		}
+
+		// the subprocess emits logs matching the consul-template runner, but we
+		// CT doesn't support hclog, so we just print the whole output here to
+		// stderr the same way CT does so the results look seamless
+		if len(logs) > 0 {
+			log.Printf("[DEBUG] %s", logs)
+		}
+
+		result := &renderer.RenderResult{
+			DidRender:   didRender,
+			WouldRender: wouldRender,
+			Contents:    i.Contents,
+		}
+		return result, nil
+	}
+}
+
+func ReaderFn(taskID, taskDir string, sandboxEnabled bool) func(string) ([]byte, error) {
+	if !sandboxEnabled {
+		return nil
+	}
+	thisBin := subproc.Self()
+
+	return func(src string) ([]byte, error) {
+
+		sandboxCfg := &sandboxConfig{
+			thisBin:     thisBin,
+			sandboxPath: taskDir,
+			sourcePath:  src,
+			taskID:      taskID,
+		}
+
+		stdout, stderr, code, err := readTemplateFromSandbox(sandboxCfg)
+		if err != nil && code != 0 {
+			return nil, fmt.Errorf("%v: %s", err, string(stderr))
+		}
+
+		// this will get wrapped in CT log formatter
+		fmt.Fprintf(os.Stderr, "[DEBUG] %s", string(stderr))
+		return stdout, nil
+	}
+}

client/allocrunner/taskrunner/template/template_windows.go

@@ -6,164 +6,13 @@
 package template
 
 import (
-	"bytes"
-	"context"
-	"errors"
-	"fmt"
-	"io"
-	"path/filepath"
-	"time"
-
-	"github.com/hashicorp/go-hclog"
-	"github.com/hashicorp/nomad/client/allocrunner/taskrunner/template/renderer"
-	"github.com/hashicorp/nomad/helper/subproc"
-	"github.com/hashicorp/nomad/helper/winappcontainer"
-	"github.com/hashicorp/nomad/helper/winexec"
+	"github.com/hashicorp/consul-template/renderer"
 )
 
-const ExitCodeFatal int = 13 // typically this is going to be a bug in Nomad
-
-// createPlatformSandbox creates the AppContainer profile and sets DACLs on the
-// files we want to grant access to.
-func createPlatformSandbox(cfg *TaskTemplateManagerConfig) error {
-
-	if !isSandboxEnabled(cfg) {
-		return nil
-	}
-	thisBin := subproc.Self()
-
-	containerCfg := &winappcontainer.AppContainerConfig{
-		Name: cfg.TaskID,
-		AllowedPaths: []string{
-			thisBin,
-			filepath.Dir(cfg.TaskDir), // give access to the whole alloc working directory
-		},
-	}
-	if cfg.Logger == nil {
-		cfg.Logger = hclog.Default() // prevents panics in tests
-	}
-
-	err := winappcontainer.CreateAppContainer(cfg.Logger, containerCfg)
-	if err != nil {
-		// if Nomad is running as an unprivileged user, we might not be able to
-		// create the sandbox, but in that case we're not vulnerable to the
-		// attacks this is intended to prevent anyways
-		if errors.Is(err, winappcontainer.ErrAccessDeniedToCreateSandbox) {
-			cfg.Logger.Debug("could not create platform sandbox", "error", err)
-			return nil
-		}
-		return fmt.Errorf("could not create platform sandbox: %w", err)
-	}
-
+func RenderFn(taskID, taskDir string, sandboxEnabled bool) func(*renderer.RenderInput) (*renderer.RenderResult, error) {
 	return nil
 }
 
-// destroyPlatformSandbox deletes the AppContainer profile.
-func destroyPlatformSandbox(cfg *TaskTemplateManagerConfig) error {
-
-	if cfg.ClientConfig.TemplateConfig.DisableSandbox {
+func ReaderFn(taskID, taskDir string, sandboxEnabled bool) func(string) ([]byte, error) {
 	return nil
-	}
-
-	if cfg.Logger == nil {
-		cfg.Logger = hclog.Default()
-	}
-
-	err := winappcontainer.DeleteAppContainer(cfg.Logger, cfg.TaskID)
-	if err != nil {
-		cfg.Logger.Warn("could not destroy platform sandbox", "error", err)
-	}
-	return err
-}
-
-// renderTemplateInSandbox runs the template-render command in an AppContainer to
-// prevent a task from swapping a directory between the sandbox path and the
-// destination with a symlink pointing to somewhere outside the sandbox.
-//
-// See renderer/ subdirectory for implementation.
-func renderTemplateInSandbox(cfg *sandboxConfig) (string, int, error) {
-	procThreadAttrs, cleanup, err := winappcontainer.CreateProcThreadAttributes(cfg.taskID)
-	if err != nil {
-		return "", ExitCodeFatal, fmt.Errorf("could not create proc attributes: %v", err)
-	}
-	defer cleanup()
-
-	// Safe to inject user input as command arguments since winexec.Command
-	// does not invoke a shell.
-	args := []string{
-		"template-render",
-		"write",
-		"-sandbox-path", cfg.sandboxPath,
-		"-dest-path", cfg.destPath,
-		"-perms", cfg.perms,
-	}
-	if cfg.user != "" {
-		args = append(args, "-user")
-		args = append(args, cfg.user)
-	}
-	if cfg.group != "" {
-		args = append(args, "-group")
-		args = append(args, cfg.group)
-	}
-
-	ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
-	defer cancel()
-
-	cmd := winexec.CommandContext(ctx, cfg.thisBin, args...)
-	cmd.ProcThreadAttributes = procThreadAttrs
-
-	stdin, err := cmd.StdinPipe()
-	if err != nil {
-		return "", 1, err
-	}
-
-	go func() {
-		defer stdin.Close()
-		io.Copy(stdin, bytes.NewReader(cfg.contents))
-	}()
-
-	out, err := cmd.CombinedOutput()
-	code := cmd.ProcessState.ExitCode()
-	if code == renderer.ExitWouldRenderButDidnt {
-		err = nil // erase the "exit code 117" error
-	}
-
-	return string(out), code, err
-}
-
-// readTemplateFromSandbox runs the template-render command in a subprocess that
-// will chroot itself to prevent a task from swapping a directory between the
-// sandbox path and the source with a symlink pointing to somewhere outside
-// the sandbox.
-func readTemplateFromSandbox(cfg *sandboxConfig) ([]byte, []byte, int, error) {
-	procThreadAttrs, cleanup, err := winappcontainer.CreateProcThreadAttributes(cfg.taskID)
-	if err != nil {
-		return nil, nil, ExitCodeFatal, fmt.Errorf("could not create proc attributes: %v", err)
-	}
-	defer cleanup()
-
-	// Safe to inject user input as command arguments since winexec.Command
-	// does not invoke a shell. Also, the only user-controlled argument here is
-	// the source path which we've already verified is at least a valid path in
-	// the caller.
-	args := []string{
-		"template-render",
-		"read",
-		"-sandbox-path", cfg.sandboxPath,
-		"-source-path", cfg.sourcePath,
-	}
-
-	ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
-	defer cancel()
-
-	cmd := winexec.CommandContext(ctx, cfg.thisBin, args...)
-	cmd.ProcThreadAttributes = procThreadAttrs
-	var outb, errb bytes.Buffer
-	cmd.Stdout = &outb
-	cmd.Stderr = &errb
-
-	err = cmd.Run()
-	stdout := outb.Bytes()
-	stderr := errb.Bytes()
-	return stdout, stderr, cmd.ProcessState.ExitCode(), err
 }

client/allocrunner/taskrunner/template/template_windows_test.go

@@ -1,103 +0,0 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: BUSL-1.1
-
-//go:build windows
-
-package template
-
-import (
-	"fmt"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/hashicorp/nomad/ci"
-	"github.com/hashicorp/nomad/client/taskenv"
-	clienttestutil "github.com/hashicorp/nomad/client/testutil"
-	"github.com/hashicorp/nomad/nomad/mock"
-	"github.com/hashicorp/nomad/nomad/structs"
-	"github.com/shoenig/test/must"
-)
-
-// TestTaskTemplateManager_SymlinkEscapeSource verifies that a malicious or
-// compromised task cannot use a symlink parent directory to cause reads to
-// escape the sandbox
-func TestTaskTemplateManager_SymlinkEscapeSource(t *testing.T) {
-	ci.Parallel(t)
-	clienttestutil.RequireAdministrator(t) // making symlinks is privileged on Windows
-
-	// Create a set of "sensitive" files outside the task dir that the task
-	// should not be able to read or write to, despite filesystem permissions
-	sensitiveDir := t.TempDir()
-	sensitiveFile := filepath.Join(sensitiveDir, "sensitive.txt")
-	os.WriteFile(sensitiveFile, []byte("very-secret-stuff"), 0755)
-
-	a := mock.Alloc()
-	task := a.Job.TaskGroups[0].Tasks[0]
-	task.Name = TestTaskName
-	template := &structs.Template{ChangeMode: structs.TemplateChangeModeNoop}
-
-	// Build a new task environment with a valid DestPath
-	harness := newTestHarness(t, []*structs.Template{template}, false, false)
-	harness.envBuilder = taskenv.NewBuilder(harness.node, a, task, "global")
-	harness.envBuilder.SetClientTaskRoot(harness.taskDir)
-	os.MkdirAll(filepath.Join(harness.taskDir, "local"), 0755)
-	harness.templates[0].DestPath = filepath.Join("local", "dest.tmpl")
-
-	// "Attack" the SourcePath by creating a symlink from the sensitive file to
-	// the task dir; this simulates what happens when the client restarts and
-	// the task attacks while the client is down, which is the easiest case to
-	// reproduce
-	must.NoError(t, os.Symlink(sensitiveDir, filepath.Join(harness.taskDir, "local", "pwned")))
-	harness.templates[0].SourcePath = filepath.Join("local", "pwned", "sensitive.txt")
-	fullSrcPath := filepath.Join(harness.taskDir, harness.templates[0].SourcePath)
-
-	err := harness.startWithErr()
-	t.Cleanup(harness.stop)
-
-	must.EqError(t, err, fmt.Sprintf(
-		"failed to read template: failed to open source file %q: open %s: Access is denied.\n", fullSrcPath, fullSrcPath))
-}
-
-// TestTaskTemplateManager_SymlinkEscapeDest verifies that a malicious or
-// compromised task cannot use a symlink parent directory to cause writes to
-// escape the sandbox
-func TestTaskTemplateManager_SymlinkEscapeDest(t *testing.T) {
-	ci.Parallel(t)
-	clienttestutil.RequireAdministrator(t) // making symlinks is privileged on Windows
-
-	// Create a set of "sensitive" files outside the task dir that the task
-	// should not be able to read or write to, despite filesystem permissions
-	sensitiveDir := t.TempDir()
-	sensitiveFile := filepath.Join(sensitiveDir, "sensitive.txt")
-	os.WriteFile(sensitiveFile, []byte("very-secret-stuff"), 0755)
-
-	a := mock.Alloc()
-	task := a.Job.TaskGroups[0].Tasks[0]
-	task.Name = TestTaskName
-	template := &structs.Template{ChangeMode: structs.TemplateChangeModeNoop}
-
-	// Build a task environment with a valid SourcePath
-	harness := newTestHarness(t, []*structs.Template{template}, false, false)
-	harness.envBuilder = taskenv.NewBuilder(harness.node, a, task, "global")
-	harness.envBuilder.SetClientTaskRoot(harness.taskDir)
-	os.MkdirAll(filepath.Join(harness.taskDir, "local"), 0755)
-
-	harness.templates[0].SourcePath = filepath.Join("local", "source.tmpl")
-	must.NoError(t, os.WriteFile(
-		filepath.Join(harness.taskDir, harness.templates[0].SourcePath),
-		[]byte("hacked!"), 0755))
-
-	// "Attack" the DestPath by creating a symlink from the sensitive file to
-	// the task dir
-	must.NoError(t, os.Symlink(sensitiveDir, filepath.Join(harness.taskDir, "local", "pwned")))
-	harness.templates[0].DestPath = filepath.Join("local", "pwned", "sensitive.txt")
-
-	err := harness.startWithErr()
-	t.Cleanup(harness.stop)
-
-	// Ensure we haven't written despite the error
-	b, err := os.ReadFile(sensitiveFile)
-	must.NoError(t, err)
-	must.Eq(t, "very-secret-stuff", string(b))
-}