client: fix interpolation in template source #9383
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
There was a problem hiding this comment. The regression patch in 0.12.7 (the CVE fix was 0.12.6) made it so that we always append the destination onto the task directory (ref). So I checked out this PR and removed the patch code, and it looks like this test fails in the same way. This either an existing bug or a regression that 0.12.7 introduced. 😦 There was a problem hiding this comment. Ok, I ported this test over to 0.12.5 (pre-CVE patch) and it still fails in the same way, so this is an existing bug. I'd propose we open a new issue for that and then comment out this test with a link to that issue, so we remember to resolve it later and not hold this up. There was a problem hiding this comment. Thanks for testing. Issue filed: #9389 Going to skip these for now and reference that issue. |
||
| { | ||
| 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. | ||
schmichael marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| 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 | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an existing test at https://github.com/hashicorp/nomad/blob/v0.12.8/client/allocrunner/taskrunner/template/template_test.go#L362-L452 that covers using interpolation to escape. This test setup is nicer so maybe we could move those test cases into this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to leave that test be for now. It's definitely uglier, but it's a bit more comprehensive than this minimal unit test. There's so many state permutations to test in templates I'm pretty nervous reworking that test might miss some subtle state (eg actually executing the templates and meta var interpolation are unique to that test).