exec: allow running commands from host volume #14851

tgross · 2022-10-07T19:48:57Z

The exec driver and other drivers derived from the shared executor check the path of the command before handing off to libcontainer to ensure that the command doesn't escape the sandbox. But we don't check any host volume mounts, which should be safe to use as a source for executables if we're letting the user mount them to the container in the first place.

Check the mount config to verify the executable lives in the mount's host path, but then return an absolute path within the mount's task path so that we can hand that off to libcontainer to run.

Includes a good bit of refactoring here because the anchoring of the final task path has different code paths for inside the task dir vs inside a mount. But I've fleshed out the test coverage of this a good bit to ensure we haven't created any regressions in the process.

Longer-term with some sort of exec v2 I'd like to switch all of this to an overlay FS, but we'll need a new FSIsolation mode so as not to break any 3rd-party drivers relying on the existing alloc dir behavior.

Example jobspec that this enables, assuming a binary named hello at the host volume named shared_data:

job "example" {
  datacenters = ["dc1"]

  group "hello" {

    volume "host_data" {
      type      = "host"
      read_only = false
      source    = "shared_data"
    }

    task "hello" {

      driver = "exec"

      config {
        command = "/host_data/hello"
      }

      volume_mount {
        volume      = "host_data"
        destination = "/host_data"
        read_only   = true
      }

    }
  }
}

mikenomitch · 2022-10-07T20:22:56Z

Would close #11781

schmichael · 2022-10-07T21:22:09Z

The exec driver and other drivers derived from the shared executor check the path of the command before handing off to libcontainer to ensure that the command doesn't escape the sandbox

Is the only reason for this to "speed up" errors due to the executable not being reachable to pre-chroot instead of post? Seems like just letting the command fail to execute in the chroot would fix this surprising behavior and simplify code at the cost of "slower" task failures?

If we used an overlayfs we could remove that slowdown for the failure case as well as the happy path.

tgross · 2022-11-09T21:00:07Z

Is the only reason for this to "speed up" errors due to the executable not being reachable to pre-chroot instead of post?

It turns out we also set the executable bit on the command binary for the user. For the exec driver (or any driver with FSIsolationChroot instead of FSIsolationImage) the task dir includes copies of the client.chroot_env. So it's safe to set the executable bit... so long as we make sure that we're doing so to one of those files copied into the task dir sandbox. Or now, in the mounted host volume.

The exec driver and other drivers derived from the shared executor check the path of the command before handing off to libcontainer to ensure that the command doesn't escape the sandbox. But we don't check any host volume mounts, which should be safe to use as a source for executables if we're letting the user mount them to the container in the first place. Check the mount config to verify the executable lives in the mount's host path, but then return an absolute path within the mount's task path so that we can hand that off to libcontainer to run. Includes a good bit of refactoring here because the anchoring of the final task path has different code paths for inside the task dir vs inside a mount. But I've fleshed out the test coverage of this a good bit to ensure we haven't created any regressions in the process.

tgross · 2022-11-10T20:20:57Z

drivers/shared/executor/executor_linux.go

-	path := absPath
-
-	// Ensure that the path is contained in the chroot, and find it relative to the container
-	rel, err := filepath.Rel(command.TaskDir, path)
-	if err != nil {
-		return nil, fmt.Errorf("failed to determine relative path base=%q target=%q: %v", command.TaskDir, path, err)
-	}
-
-	// Turn relative-to-chroot path into absolute path to avoid
-	// libcontainer trying to resolve the binary using $PATH.
-	// Do *not* use filepath.Join as it will translate ".."s returned by
-	// filepath.Rel. Prepending "/" will cause the path to be rooted in the
-	// chroot which is the desired behavior.
-	path = "/" + rel


Note for reviewers: this has been folded in to lookupTaskBin's inner functions. The alternative was to have lookupTaskBin pass back flags to the caller about where it got the paths, and that was all pretty gross.

tgross · 2022-11-10T20:24:20Z

drivers/shared/executor/executor_linux.go

-// lookPathIn looks for a file with PATH inside the directory root. Like exec.LookPath
-func lookPathIn(path string, root string, bin string) (string, error) {
-	// exec.LookPath(file string)
-	for _, dir := range filepath.SplitList(path) {
-		if dir == "" {
-			// match unix shell behavior, empty path element == .
-			dir = "."
-		}
-		path := filepath.Join(root, dir, bin)
-		f, err := os.Stat(path)
-		if err != nil {
-			continue
-		}
-		if m := f.Mode(); !m.IsDir() {


Note for reviewers: this was copied almost verbatim out of the stdlib but passed a mock PATH. But since we control the mock path there's no need to do any PATH parsing and we can just iterate over a known set of directories instead. That lets us use getPathInTaskDir for searching the task dir, the local dir, and these paths rather than having special cases.

shoenig

LGTM; see thoughts

drivers/shared/executor/executor_linux.go

shoenig · 2022-11-10T23:10:11Z

drivers/shared/executor/executor_linux.go

+// identical for binaries that don't come from mounts).
+//
+// See also executor.lookupBin for a version used by non-isolated drivers.
+func lookupTaskBin(command *ExecCommand) (string, string, error) {
 	taskDir := command.TaskDir
 	bin := command.Cmd


does it make sense to search task/local/ and task/ if the bin is given as absolute? don't think i've ever tried that, but it sounds confusing if it works

It does sound confusing, and I spent much of yesterday puzzled over that! But it's the existing behavior (ref executor_linux.go#L814-L819) so changing it now will break backwards compatibility. Let's save changing it for exec v2.

github-actions · 2023-03-12T02:17:19Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

tgross added theme/driver/exec theme/driver/java labels Oct 7, 2022

vercel bot deployed to Preview – nomad-storybook-and-ui October 7, 2022 19:49 View deployment

tgross requested review from mikenomitch and schmichael October 7, 2022 19:49

tgross added the type/enhancement label Oct 7, 2022

tgross force-pushed the exec-from-mount branch from 2908d76 to a46c91d Compare October 7, 2022 19:53

tgross self-assigned this Oct 7, 2022

vercel bot deployed to Preview – nomad-storybook-and-ui October 7, 2022 19:56 View deployment

tgross added this to the 1.4.x milestone Oct 7, 2022

tgross force-pushed the exec-from-mount branch from a46c91d to 54046a1 Compare November 9, 2022 22:01

vercel bot deployed to Preview – nomad-storybook-and-ui November 9, 2022 22:08 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui November 10, 2022 17:05 View deployment

tgross force-pushed the exec-from-mount branch from 2e3735c to 52926d9 Compare November 10, 2022 20:01

vercel bot deployed to Preview – nomad-storybook-and-ui November 10, 2022 20:08 View deployment

tgross force-pushed the exec-from-mount branch from 52926d9 to e4fc11e Compare November 10, 2022 20:17

vercel bot deployed to Preview – nomad-storybook-and-ui November 10, 2022 20:23 View deployment

tgross commented Nov 10, 2022

View reviewed changes

tgross marked this pull request as ready for review November 10, 2022 20:24

tgross requested a review from shoenig November 10, 2022 20:24

tgross modified the milestones: 1.4.x, 1.4.3 Nov 10, 2022

vercel bot deployed to Preview – nomad November 10, 2022 20:25 View deployment

tgross mentioned this pull request Nov 10, 2022

Ability to run the command directly from a mounted hosted volume #11781

Closed

shoenig approved these changes Nov 10, 2022

View reviewed changes

refactor to check for regular files

0fd8108

add search order to docs

e565ffe

vercel bot deployed to Preview – nomad-storybook-and-ui November 11, 2022 14:07 View deployment

vercel bot deployed to Preview – nomad November 11, 2022 14:07 View deployment

tgross merged commit 11a5f79 into main Nov 11, 2022

tgross deleted the exec-from-mount branch November 11, 2022 14:51

tgross added the backport/1.4.x backport to 1.4.x release line label Nov 11, 2022

hc-github-team-nomad-core mentioned this pull request Nov 11, 2022

Backport of exec: allow running commands from host volume into release/1.4.x #15216

Merged

tgross mentioned this pull request Nov 18, 2022

Support running executables from mounted paths in exec driver (like docker driver) #8410

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exec: allow running commands from host volume #14851

exec: allow running commands from host volume #14851

tgross commented Oct 7, 2022 •

edited

Loading

mikenomitch commented Oct 7, 2022

schmichael commented Oct 7, 2022 •

edited

Loading

tgross commented Nov 9, 2022

tgross Nov 10, 2022

tgross Nov 10, 2022

shoenig left a comment

shoenig Nov 10, 2022

tgross Nov 11, 2022

github-actions bot commented Mar 12, 2023

exec: allow running commands from host volume #14851

exec: allow running commands from host volume #14851

Conversation

tgross commented Oct 7, 2022 • edited Loading

mikenomitch commented Oct 7, 2022

schmichael commented Oct 7, 2022 • edited Loading

tgross commented Nov 9, 2022

tgross Nov 10, 2022

Choose a reason for hiding this comment

tgross Nov 10, 2022

Choose a reason for hiding this comment

shoenig left a comment

Choose a reason for hiding this comment

shoenig Nov 10, 2022

Choose a reason for hiding this comment

tgross Nov 11, 2022

Choose a reason for hiding this comment

github-actions bot commented Mar 12, 2023

tgross commented Oct 7, 2022 •

edited

Loading

schmichael commented Oct 7, 2022 •

edited

Loading