From cd7226d398d5404346e9f0cec37a30f76bef97ee Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 17 Nov 2020 21:59:47 -0800 Subject: [PATCH 1/3] client: fix interpolation in template source While Nomad v0.12.8 fixed `NOMAD_{ALLOC,TASK,SECRETS}_DIR` use in `template.destination`, interpolating these variables in `template.source` caused a path escape error. **Why not apply the destination fix to source?** The destination fix forces destination to always be relative to the task directory. This makes sense for the destination as a destination outside the task directory would be unreachable by the task. There's no reason to ever render a template outside the task directory. (Using `..` does allow destinations to escape the task directory if `template.disable_file_sandbox = true`. That's just awkward and unsafe enough I hope no one uses it.) There is a reason to source a template outside a task directory. At least if there weren't then I can't think of why we implemented `template.disable_file_sandbox`. So v0.12.8 left the behavior of `template.source` the more straightforward "Interpolate and validate." However, since outside of `raw_exec` every other driver uses absolute paths for `NOMAD_*_DIR` interpolation, this means those variables are unusable unless `disable_file_sandbox` is set. **The Fix** The variables are now interpolated as relative paths *only for the purpose of rendering templates.* This is an unfortunate special case, but reflects the fact that the templates view of the filesystem is completely different (unconstrainted) vs the task's view (chrooted). Arguably the values of these variables *should be context-specific.* I think it's more reasonable to think of the "hack" as templating running uncontainerized than that giving templates different paths is a hack. **TODO** - [ ] E2E tests - [ ] Job validation may still be broken and prevent my fix from working? **raw_exec** `raw_exec` is actually broken _a different way_ as exercised by tests in this commit. I think we should probably remove these tests and fix that in a followup PR/release, but I wanted to leave them in for the initial review and discussion. Since non-containerized source paths are broken anyway, perhaps there's another solution to this entire problem I'm overlooking? --- .../taskrunner/template/template.go | 23 +- .../taskrunner/template/template_test.go | 246 ++++++++++++++++++ 2 files changed, 267 insertions(+), 2 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 2bf11400ca69..bbc9fea489bf 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/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 } } diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index ba2340c84c2d..be1385b34919 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/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,250 @@ 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 + + // 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. + { + 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, + }, + //TODO: Fix this test. I *think* it should fail instead of + // forcing it relative. The double joining of the task dir + // onto the destination seems like a bug. + { + Name: "RawExecDstEscapesErr", + Config: func() *TaskTemplateManagerConfig { + return &TaskTemplateManagerConfig{ + ClientConfig: clientConf, + TaskDir: taskDir.Dir, + EnvBuilder: rawExecEnv(), + Templates: []*structs.Template{ + { + SourcePath: "${NOMAD_TASK_DIR}/src", + DestPath: "/etc/dst_escapes", + }, + }, + } + }, + Err: destEscapesErr, + }, + } + + for i := range cases { + tc := cases[i] + t.Run(tc.Name, func(t *testing.T) { + 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 From 5b83ca0b5ddd58cd3191dd53ae760248182a25a4 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 18 Nov 2020 10:01:02 -0800 Subject: [PATCH 2/3] client: skip broken test and fix assertion --- .../taskrunner/template/template_test.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index be1385b34919..5bb989900ff5 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1496,6 +1496,9 @@ func TestTaskTemplateManager_Escapes(t *testing.T) { 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 @@ -1615,8 +1618,9 @@ func TestTaskTemplateManager_Escapes(t *testing.T) { }, //TODO: Fix this test. I *think* it should pass. The double // joining of the task dir onto the destination seems like - // a bug. + // a bug. https://github.com/hashicorp/nomad/issues/9389 { + Skip: true, Name: "RawExecOk", Config: func() *TaskTemplateManagerConfig { return &TaskTemplateManagerConfig{ @@ -1651,11 +1655,8 @@ func TestTaskTemplateManager_Escapes(t *testing.T) { }, Err: sourceEscapesErr, }, - //TODO: Fix this test. I *think* it should fail instead of - // forcing it relative. The double joining of the task dir - // onto the destination seems like a bug. { - Name: "RawExecDstEscapesErr", + Name: "RawExecDstAbsoluteOk", Config: func() *TaskTemplateManagerConfig { return &TaskTemplateManagerConfig{ ClientConfig: clientConf, @@ -1664,18 +1665,22 @@ func TestTaskTemplateManager_Escapes(t *testing.T) { Templates: []*structs.Template{ { SourcePath: "${NOMAD_TASK_DIR}/src", - DestPath: "/etc/dst_escapes", + DestPath: "/etc/absolutely_relative", }, }, } }, - Err: destEscapesErr, + 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 { From 252ac1e1ab5053651e696bd69dadb06d1840e690 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 18 Nov 2020 10:43:48 -0800 Subject: [PATCH 3/3] e2e: test template path interpolation --- e2e/consultemplate/consultemplate.go | 64 +++++++++++++++++++ .../input/bad_template_paths.nomad | 37 +++++++++++ e2e/consultemplate/input/template_paths.nomad | 37 +++++++++++ 3 files changed, 138 insertions(+) create mode 100644 e2e/consultemplate/input/bad_template_paths.nomad create mode 100644 e2e/consultemplate/input/template_paths.nomad diff --git a/e2e/consultemplate/consultemplate.go b/e2e/consultemplate/consultemplate.go index aa33f7be99d4..7bc158282b65 100644 --- a/e2e/consultemplate/consultemplate.go +++ b/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 { diff --git a/e2e/consultemplate/input/bad_template_paths.nomad b/e2e/consultemplate/input/bad_template_paths.nomad new file mode 100644 index 000000000000..af9adde549f3 --- /dev/null +++ b/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 + } + } + } +} diff --git a/e2e/consultemplate/input/template_paths.nomad b/e2e/consultemplate/input/template_paths.nomad new file mode 100644 index 000000000000..d52e26282974 --- /dev/null +++ b/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 + } + } + } +}