Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

template destination backwards compatibility #9148

Closed
tgross opened this issue Oct 22, 2020 · 4 comments · Fixed by #9149
Closed

template destination backwards compatibility #9148

tgross opened this issue Oct 22, 2020 · 4 comments · Fixed by #9149
Assignees
Labels
theme/docs Documentation issues and enhancements theme/template type/bug

Comments

@tgross
Copy link
Member

tgross commented Oct 22, 2020

Prior to Nomad 0.12.5, you could use ${NOMAD_SECRETS_DIR}/mysecret.txt as the template.destination. With the recent security patch (#9139) that will get interpolated as the absolute path /secrets/mysecret.txt and get treated as though it were a sandbox escape. Note the leading /!

The relative path secrets/mysecret.txt works just fine as expected. And documentation is pretty clear that the destination path is supposed to be relative to the working directory, but we do in fact allow absolute paths so long as they don't escape the file sandbox (so you could do /var/nomad/allocs/:alloc_id/:task_name/secrets/mysecret.txt if you were so inclined). But this is pretty lousy UX and a nasty surprise for folks.

At the very least, this change should be documented.

(Noticed while working on #9144 #9146)

@tgross
Copy link
Member Author

tgross commented Oct 22, 2020

I'm realizing where the issue is: the tricky bit with this is that fixing it without somehow special casing NOMAD_SECRETS_DIR, etc. would make the behavior of the disable_file_sandbox ambiguous. Maybe it's worth special-casing for those task directory env vars.

@tgross
Copy link
Member Author

tgross commented Oct 22, 2020

After an internal discussion w/ @schmichael and @krishicks we've realized that this breaks any case where someone was passing an absolute directory path in the destination field. Although the docs say "relative-to", it wasn't enforced at all so of course we've baked in behaviors that depend on it.

The fix will be to revert the behavior around the destination field to unconditionally prepend the task working directory, and then do the escape check.

@tgross
Copy link
Member Author

tgross commented Oct 22, 2020

Will ship in 1.0. I'm in the process of backporting to 0.12.7, 0.11.6, and 0.10.7.

@tgross tgross added this to the 1.0 milestone Oct 22, 2020
tgross added a commit that referenced this issue Oct 23, 2020
* docs: add regression warning for GH-9148 to upgrade guide
* changelog entry
@tgross tgross removed this from the 1.0 milestone Oct 27, 2020
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
theme/docs Documentation issues and enhancements theme/template type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant