-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client: fix interpolation in template source #9383
Conversation
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?
24def15
to
cd7226d
Compare
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.
The patch itself LGTM. IIRC we considered this approach for 0.12.7 but I don't remember why we decided not to go that way. Definitely some open questions about how raw_exec
is supposed to be behaving.
@@ -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) { |
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).
}, | ||
//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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😦
=== RUN TestTaskTemplateManager_Escapes/RawExecOk
template_test.go:1688:
Error Trace: template_test.go:1688
Error: Not equal:
expected: "/fake/allocdir/8b3aaa2a-beca-ae55-2ece-304012490a0d/web/secrets/dst"
actual : "/fake/allocdir/8b3aaa2a-beca-ae55-2ece-304012490a0d/web/fake/allocdir/8b3aaa2a-beca-ae55-2ece-304012490a0d/web/secrets/dst"
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-/fake/allocdir/8b3aaa2a-beca-ae55-2ece-304012490a0d/web/secrets/dst
+/fake/allocdir/8b3aaa2a-beca-ae55-2ece-304012490a0d/web/fake/allocdir/8b3aaa2a-beca-ae55-2ece-304012490a0d/web/secrets/dst
Test: TestTaskTemplateManager_Escapes/RawExecOk
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.
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.
=== RUN TestTaskTemplateManager_Escapes/RawExecOk
template_test.go:1749:
Error Trace: template_test.go:1749
Error: Not equal:
expected: "/fake/allocdir/254b7cac-88fa-b795-da67-8e28cc507ec7/web/local/src"
actual : "/fake/allocdir/254b7cac-88fa-b795-da67-8e28cc507ec7/web/fake/allocdir/254b7cac-88fa-b795-da67-8e28cc507ec7/web/local/src"
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-/fake/allocdir/254b7cac-88fa-b795-da67-8e28cc507ec7/web/local/src
+/fake/allocdir/254b7cac-88fa-b795-da67-8e28cc507ec7/web/fake/allocdir/254b7cac-88fa-b795-da67-8e28cc507ec7/web/local/src
Test: TestTaskTemplateManager_Escapes/RawExecOk
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.
Thanks for testing. Issue filed: #9389
Going to skip these for now and reference that issue.
The E2E test consultemplate doesn't exercise the source field at all, unfortunately. Otherwise we might have caught this one. (It also doesn't exercise |
fe91adc
to
252ac1e
Compare
E2E test LGTM. Ran these tests on a full E2E cluster built from this sha:
|
Backport of client: fix interpolation in template source #9383
Backport to 0.11 of client: fix interpolation in template source #9383
Backport to 0.10 of client: fix interpolation in template source #9383
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Followup to #9149
While Nomad v0.12.8 fixed
NOMAD_{ALLOC,TASK,SECRETS}_DIR
use intemplate.destination
, interpolating these variables intemplate.source
caused a path escape error.repro
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
..
doesallow destinations to escape the task directory if
template.disable_file_sandbox = true
. That's just awkward and unsafeenough 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 thebehavior of
template.source
the more straightforward "Interpolate andvalidate."
However, since outside of
raw_exec
every other driver uses absolutepaths for
NOMAD_*_DIR
interpolation, this means those variables areunusable 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.
raw_exec
Filed: #9389
raw_exec
is actually broken a different way as exercised by tests inthis 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?