-
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
Update go-getter vendored library #5446
Conversation
Thanks for the PR. We are stabilizing code for 0.9.0 release, and concentrating on urgent fixes. I'll target merging this PR after we cut 0.9.0. Also, when symlink issue came up before, some colleagues had security concerns about symlinks - we need to ensure that symlinks in artifacts don't provide an escape hatch to host when running with exec/docker drivers. |
Unlesss I am missing something about the way chroot works (but really we're using runC here right in both cases)? This isn't possible unless you're using hard links -- which my patch didn't attempt to solve (we don't need it). See this q&a for example We would prefer this get in for the 0.9 release as we're still building a patched version of Nomad in our infra which isn't great. Any chance we can put some priority on this? |
Unfortunately artifacts are downloaded and templates are rendered outside the chroot. We test to ensure their path's don't escape the chroot/alloc directory, but allowing artifacts to write symlinks complicates this. An artifact could create a symlink outside the alloc dir, and then another artifact or a template could overwrite arbitrary files on the filesystem by following that symlink. 0.9.0 is way overdue and feature frozen, so I'm afraid this will have to wait until post-release. If you're able to add tests and any necessary code to prevent alloc dir escaping by symlinks from artifacts, we'd be happy to merge it into the 0.9.1 branch where we're already staging features. Otherwise this is definitely something we need to support, but I'm afraid I can't commit to a roadmap. It's possible our existing checks prevent symlink-from-artifact abuse, but it will take some testing and thinking-like-an-attacker to ensure that's the case. |
I might have some bandwidth next week. Do you have any requirements for these tests? |
I think we'll need a test on both sides (go-getter and nomad) as it's subtle enough logic it seems easy for one side to break without the other noticing. go-getterI don't think the symlink support in go-getter won't support Nomad's use case as-is, but I haven't tested. It needs to support a working directory like tar's This should make go-getter's tar behave like gnu tar which I believe has the chroot-esque security properties we need (my kingdom to just unpack artifacts while chroot'd, but I think that's even more complicated). nomadThen this test might be a good one to copy, paste, and change. I would create a tarball with a few symlinks: one valid (relative path), one invalid with absolute path ( Call The symlink with dots in the path should be dropped (logging like Does all of that make sense? I think it should provide the desired feature while preventing security issues, but I could be missing something. |
@schmichael and @notnoop it looks like go-getter was updated after this PR in #6215, but it also looks like there's some ongoing discussion here that maybe we should capture in a standalone issue? Or do we think that's all resolved at this point? |
Thank you for the contribution. We've updated go-getter few times since this PR is made. Though, the symlink support got reverted, and the user visible issue is tracked in hashicorp/go-getter#60 and #2292 . Closing for now. |
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. |
What I did:
And a lot of mucking around with broken dependencies until
make dev
produces a correct binaryThis fixes Nomad's lack of "symlinks" support in Tarball(s) (Artefacts).