diff --git a/api/tasks.go b/api/tasks.go index d09339f34e27..84765c71abd9 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -99,6 +99,7 @@ type Task struct { type TaskArtifact struct { GetterSource string GetterOptions map[string]string + RelativeDest string } // NewTask creates and initializes a new Task. diff --git a/client/driver/exec.go b/client/driver/exec.go index f097d007950f..672f0e96cb01 100644 --- a/client/driver/exec.go +++ b/client/driver/exec.go @@ -113,7 +113,7 @@ func (d *ExecDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, }, executorCtx) if err != nil { pluginClient.Kill() - return nil, fmt.Errorf("error starting process via the plugin: %v", err) + return nil, err } d.logger.Printf("[DEBUG] driver.exec: started process via plugin with pid: %v", ps.Pid) diff --git a/client/driver/executor/executor.go b/client/driver/executor/executor.go index ceba6b0ead01..904bd3448b36 100644 --- a/client/driver/executor/executor.go +++ b/client/driver/executor/executor.go @@ -7,6 +7,7 @@ import ( "net" "os" "os/exec" + "path/filepath" "runtime" "strings" "sync" @@ -159,20 +160,36 @@ func (e *UniversalExecutor) LaunchCmd(command *ExecCommand, ctx *ExecutorContext e.cmd.Stdout = e.lro e.cmd.Stderr = e.lre - // setting the env, path and args for the command e.ctx.TaskEnv.Build() - e.cmd.Env = ctx.TaskEnv.EnvList() - e.cmd.Path = ctx.TaskEnv.ReplaceEnv(command.Cmd) - e.cmd.Args = append([]string{e.cmd.Path}, ctx.TaskEnv.ParseAndReplace(command.Args)...) - // Ensure that the binary being started is executable. - if err := e.makeExecutable(e.cmd.Path); err != nil { + // Look up the binary path and make it executable + absPath, err := e.lookupBin(ctx.TaskEnv.ReplaceEnv(command.Cmd)) + if err != nil { + return nil, err + } + + if err := e.makeExecutable(absPath); err != nil { return nil, err } - // starting the process + // Determine the path to run as it may have to be relative to the chroot. + path := absPath + if e.command.FSIsolation { + rel, err := filepath.Rel(e.taskDir, absPath) + if err != nil { + return nil, err + } + path = rel + } + + // Set the commands arguments + e.cmd.Path = path + e.cmd.Args = append([]string{path}, ctx.TaskEnv.ParseAndReplace(command.Args)...) + e.cmd.Env = ctx.TaskEnv.EnvList() + + // Start the process if err := e.cmd.Start(); err != nil { - return nil, fmt.Errorf("error starting command: %v", err) + return nil, err } go e.wait() ic := &cstructs.IsolationConfig{Cgroup: e.groups} @@ -328,8 +345,36 @@ func (e *UniversalExecutor) configureTaskDir() error { return nil } -// makeExecutablePosix makes the given file executable for root,group,others. -func (e *UniversalExecutor) makeExecutablePosix(binPath string) error { +// lookupBin looks for path to the binary to run by looking for the binary in +// the following locations, in-order: task/local/, task/, based on host $PATH. +// The return path is absolute. +func (e *UniversalExecutor) lookupBin(bin string) (string, error) { + // Check in the local directory + local := filepath.Join(e.taskDir, allocdir.TaskLocal, bin) + if _, err := os.Stat(local); err == nil { + return local, nil + } + + // Check at the root of the task's directory + root := filepath.Join(e.taskDir, bin) + if _, err := os.Stat(root); err == nil { + return root, nil + } + + // Check the $PATH + if host, err := exec.LookPath(bin); err == nil { + return host, nil + } + + return "", fmt.Errorf("binary %q could not be found", bin) +} + +// makeExecutable makes the given file executable for root,group,others. +func (e *UniversalExecutor) makeExecutable(binPath string) error { + if runtime.GOOS == "windows" { + return nil + } + fi, err := os.Stat(binPath) if err != nil { if os.IsNotExist(err) { diff --git a/client/driver/executor/executor_basic.go b/client/driver/executor/executor_basic.go index 00480e9dcbdf..7d59ee3666ec 100644 --- a/client/driver/executor/executor_basic.go +++ b/client/driver/executor/executor_basic.go @@ -2,25 +2,7 @@ package executor -import ( - "path/filepath" - "runtime" - - cgroupConfig "github.com/opencontainers/runc/libcontainer/configs" -) - -func (e *UniversalExecutor) makeExecutable(binPath string) error { - if runtime.GOOS == "windows" { - return nil - } - - path := binPath - if !filepath.IsAbs(binPath) { - // The path must be relative the allocations directory. - path = filepath.Join(e.taskDir, binPath) - } - return e.makeExecutablePosix(path) -} +import cgroupConfig "github.com/opencontainers/runc/libcontainer/configs" func (e *UniversalExecutor) configureChroot() error { return nil diff --git a/client/driver/executor/executor_linux.go b/client/driver/executor/executor_linux.go index d6c4cf38bf73..48ec5f729eca 100644 --- a/client/driver/executor/executor_linux.go +++ b/client/driver/executor/executor_linux.go @@ -36,18 +36,6 @@ var ( } ) -func (e *UniversalExecutor) makeExecutable(binPath string) error { - path := binPath - if e.command.FSIsolation { - // The path must be relative the chroot - path = filepath.Join(e.taskDir, binPath) - } else if !filepath.IsAbs(binPath) { - // The path must be relative the allocations directory. - path = filepath.Join(e.taskDir, binPath) - } - return e.makeExecutablePosix(path) -} - // configureIsolation configures chroot and creates cgroups func (e *UniversalExecutor) configureIsolation() error { if e.command.FSIsolation { diff --git a/client/driver/executor/executor_test.go b/client/driver/executor/executor_test.go index 0a43e3494940..ab5d7099d010 100644 --- a/client/driver/executor/executor_test.go +++ b/client/driver/executor/executor_test.go @@ -232,3 +232,38 @@ func TestExecutor_Start_Kill(t *testing.T) { t.Fatalf("Command output incorrectly: want %v; got %v", expected, act) } } + +func TestExecutor_MakeExecutable(t *testing.T) { + // Create a temp file + f, err := ioutil.TempFile("", "") + if err != nil { + t.Fatal(err) + } + defer f.Close() + defer os.Remove(f.Name()) + + // Set its permissions to be non-executable + f.Chmod(os.FileMode(0610)) + + // Make a fake exececutor + ctx := testExecutorContext(t) + defer ctx.AllocDir.Destroy() + executor := NewExecutor(log.New(os.Stdout, "", log.LstdFlags)) + + err = executor.(*UniversalExecutor).makeExecutable(f.Name()) + if err != nil { + t.Fatalf("makeExecutable() failed: %v", err) + } + + // Check the permissions + stat, err := f.Stat() + if err != nil { + t.Fatalf("Stat() failed: %v", err) + } + + act := stat.Mode().Perm() + exp := os.FileMode(0755) + if act != exp { + t.Fatalf("expected permissions %v; got %v", err) + } +} diff --git a/client/driver/java.go b/client/driver/java.go index 19b66ff841ab..d4863df57c9c 100644 --- a/client/driver/java.go +++ b/client/driver/java.go @@ -173,7 +173,7 @@ func (d *JavaDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, }, executorCtx) if err != nil { pluginClient.Kill() - return nil, fmt.Errorf("error starting process via the plugin: %v", err) + return nil, err } d.logger.Printf("[DEBUG] driver.java: started process with pid: %v", ps.Pid) diff --git a/client/driver/qemu.go b/client/driver/qemu.go index 47d0ed5c578d..60c3354d3a96 100644 --- a/client/driver/qemu.go +++ b/client/driver/qemu.go @@ -198,7 +198,7 @@ func (d *QemuDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, ps, err := exec.LaunchCmd(&executor.ExecCommand{Cmd: args[0], Args: args[1:]}, executorCtx) if err != nil { pluginClient.Kill() - return nil, fmt.Errorf("error starting process via the plugin: %v", err) + return nil, err } d.logger.Printf("[INFO] Started new QemuVM: %s", vmID) diff --git a/client/driver/raw_exec.go b/client/driver/raw_exec.go index 564d8061aa9a..2dc0aceebbd0 100644 --- a/client/driver/raw_exec.go +++ b/client/driver/raw_exec.go @@ -103,7 +103,7 @@ func (d *RawExecDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandl ps, err := exec.LaunchCmd(&executor.ExecCommand{Cmd: command, Args: driverConfig.Args}, executorCtx) if err != nil { pluginClient.Kill() - return nil, fmt.Errorf("error starting process via the plugin: %v", err) + return nil, err } d.logger.Printf("[DEBUG] driver.raw_exec: started process with pid: %v", ps.Pid) diff --git a/client/driver/rkt.go b/client/driver/rkt.go index 1c73400ac12c..76a1fa64ea27 100644 --- a/client/driver/rkt.go +++ b/client/driver/rkt.go @@ -246,7 +246,7 @@ func (d *RktDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, e ps, err := execIntf.LaunchCmd(&executor.ExecCommand{Cmd: absPath, Args: cmdArgs}, executorCtx) if err != nil { pluginClient.Kill() - return nil, fmt.Errorf("error starting process via the plugin: %v", err) + return nil, err } d.logger.Printf("[DEBUG] driver.rkt: started ACI %q with: %v", img, cmdArgs) diff --git a/client/getter/getter.go b/client/getter/getter.go index 92d295f473f5..22a3f369e5e3 100644 --- a/client/getter/getter.go +++ b/client/getter/getter.go @@ -4,6 +4,7 @@ import ( "fmt" "log" "net/url" + "path/filepath" "sync" gg "github.com/hashicorp/go-getter" @@ -59,16 +60,17 @@ func getGetterUrl(artifact *structs.TaskArtifact) (string, error) { return u.String(), nil } -// GetArtifact downloads an artifact into the specified destination directory. -func GetArtifact(artifact *structs.TaskArtifact, destDir string, logger *log.Logger) error { +// GetArtifact downloads an artifact into the specified task directory. +func GetArtifact(artifact *structs.TaskArtifact, taskDir string, logger *log.Logger) error { url, err := getGetterUrl(artifact) if err != nil { return err } // Download the artifact - if err := getClient(url, destDir).Get(); err != nil { - return err + dest := filepath.Join(taskDir, artifact.RelativeDest) + if err := getClient(url, dest).Get(); err != nil { + return fmt.Errorf("GET error: %v", err) } return nil diff --git a/client/getter/getter_test.go b/client/getter/getter_test.go index 7282000df01b..208f9efe534d 100644 --- a/client/getter/getter_test.go +++ b/client/getter/getter_test.go @@ -21,11 +21,11 @@ func TestGetArtifact_FileAndChecksum(t *testing.T) { defer ts.Close() // Create a temp directory to download into - destDir, err := ioutil.TempDir("", "nomad-test") + taskDir, err := ioutil.TempDir("", "nomad-test") if err != nil { t.Fatalf("failed to make temp directory: %v", err) } - defer os.RemoveAll(destDir) + defer os.RemoveAll(taskDir) // Create the artifact file := "test.sh" @@ -38,13 +38,48 @@ func TestGetArtifact_FileAndChecksum(t *testing.T) { // Download the artifact logger := log.New(os.Stderr, "", log.LstdFlags) - if err := GetArtifact(artifact, destDir, logger); err != nil { + if err := GetArtifact(artifact, taskDir, logger); err != nil { t.Fatalf("GetArtifact failed: %v", err) } // Verify artifact exists - if _, err := os.Stat(filepath.Join(destDir, file)); err != nil { - t.Fatalf("source path error: %s", err) + if _, err := os.Stat(filepath.Join(taskDir, file)); err != nil { + t.Fatalf("file not found: %s", err) + } +} + +func TestGetArtifact_File_RelativeDest(t *testing.T) { + // Create the test server hosting the file to download + ts := httptest.NewServer(http.FileServer(http.Dir(filepath.Dir("./test-fixtures/")))) + defer ts.Close() + + // Create a temp directory to download into + taskDir, err := ioutil.TempDir("", "nomad-test") + if err != nil { + t.Fatalf("failed to make temp directory: %v", err) + } + defer os.RemoveAll(taskDir) + + // Create the artifact + file := "test.sh" + relative := "foo/" + artifact := &structs.TaskArtifact{ + GetterSource: fmt.Sprintf("%s/%s", ts.URL, file), + GetterOptions: map[string]string{ + "checksum": "md5:bce963762aa2dbfed13caf492a45fb72", + }, + RelativeDest: relative, + } + + // Download the artifact + logger := log.New(os.Stderr, "", log.LstdFlags) + if err := GetArtifact(artifact, taskDir, logger); err != nil { + t.Fatalf("GetArtifact failed: %v", err) + } + + // Verify artifact was downloaded to the correct path + if _, err := os.Stat(filepath.Join(taskDir, relative, file)); err != nil { + t.Fatalf("file not found: %s", err) } } @@ -54,11 +89,11 @@ func TestGetArtifact_InvalidChecksum(t *testing.T) { defer ts.Close() // Create a temp directory to download into - destDir, err := ioutil.TempDir("", "nomad-test") + taskDir, err := ioutil.TempDir("", "nomad-test") if err != nil { t.Fatalf("failed to make temp directory: %v", err) } - defer os.RemoveAll(destDir) + defer os.RemoveAll(taskDir) // Create the artifact with an incorrect checksum file := "test.sh" @@ -71,7 +106,7 @@ func TestGetArtifact_InvalidChecksum(t *testing.T) { // Download the artifact and expect an error logger := log.New(os.Stderr, "", log.LstdFlags) - if err := GetArtifact(artifact, destDir, logger); err == nil { + if err := GetArtifact(artifact, taskDir, logger); err == nil { t.Fatalf("GetArtifact should have failed") } } @@ -116,17 +151,17 @@ func TestGetArtifact_Archive(t *testing.T) { // Create a temp directory to download into and create some of the same // files that exist in the artifact to ensure they are overriden - destDir, err := ioutil.TempDir("", "nomad-test") + taskDir, err := ioutil.TempDir("", "nomad-test") if err != nil { t.Fatalf("failed to make temp directory: %v", err) } - defer os.RemoveAll(destDir) + defer os.RemoveAll(taskDir) create := map[string]string{ "exist/my.config": "to be replaced", "untouched": "existing top-level", } - createContents(destDir, create, t) + createContents(taskDir, create, t) file := "archive.tar.gz" artifact := &structs.TaskArtifact{ @@ -137,7 +172,7 @@ func TestGetArtifact_Archive(t *testing.T) { } logger := log.New(os.Stderr, "", log.LstdFlags) - if err := GetArtifact(artifact, destDir, logger); err != nil { + if err := GetArtifact(artifact, taskDir, logger); err != nil { t.Fatalf("GetArtifact failed: %v", err) } @@ -148,5 +183,5 @@ func TestGetArtifact_Archive(t *testing.T) { "new/my.config": "hello world\n", "test.sh": "sleep 1\n", } - checkContents(destDir, expected, t) + checkContents(taskDir, expected, t) } diff --git a/client/task_runner.go b/client/task_runner.go index 32571acb7bd0..afe481ad62cd 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -240,7 +240,18 @@ func (r *TaskRunner) run() { return } - for _, artifact := range r.task.Artifacts { + for i, artifact := range r.task.Artifacts { + // Verify the artifact doesn't escape the task directory. + if err := artifact.Validate(); err != nil { + // If this error occurs there is potentially a server bug or + // mallicious, server spoofing. + r.setState(structs.TaskStateDead, + structs.NewTaskEvent(structs.TaskArtifactDownloadFailed).SetDownloadError(err)) + r.logger.Printf("[ERR] client: allocation %q, task %v, artifact %v (%v) fails validation", + r.alloc.ID, r.task.Name, artifact, i) + return + } + if err := getter.GetArtifact(artifact, taskDir, r.logger); err != nil { r.setState(structs.TaskStateDead, structs.NewTaskEvent(structs.TaskArtifactDownloadFailed).SetDownloadError(err)) diff --git a/jobspec/parse.go b/jobspec/parse.go index 853640f75098..1baf29cf6866 100644 --- a/jobspec/parse.go +++ b/jobspec/parse.go @@ -617,6 +617,7 @@ func parseArtifacts(result *[]*structs.TaskArtifact, list *ast.ObjectList) error valid := []string{ "source", "options", + "destination", } if err := checkHCLKeys(o.Val, valid); err != nil { return err @@ -629,6 +630,11 @@ func parseArtifacts(result *[]*structs.TaskArtifact, list *ast.ObjectList) error delete(m, "options") + // Default to downloading to the local directory. + if _, ok := m["destination"]; !ok { + m["destination"] = "local/" + } + var ta structs.TaskArtifact if err := mapstructure.WeakDecode(m, &ta); err != nil { return err diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 28cc6afe86a4..4616db771d3b 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -131,12 +131,14 @@ func TestParse(t *testing.T) { Artifacts: []*structs.TaskArtifact{ { GetterSource: "http://foo.com/artifact", + RelativeDest: "local/", GetterOptions: map[string]string{ "checksum": "md5:b8a4f3f72ecab0510a6a31e997461c5f", }, }, { GetterSource: "http://bar.com/artifact", + RelativeDest: "local/", GetterOptions: map[string]string{ "checksum": "md5:ff1cc0d3432dad54d607c1505fb7245c", }, @@ -320,6 +322,58 @@ func TestParse(t *testing.T) { nil, true, }, + + { + "artifacts.hcl", + &structs.Job{ + ID: "binstore-storagelocker", + Name: "binstore-storagelocker", + Type: "service", + Priority: 50, + Region: "global", + + TaskGroups: []*structs.TaskGroup{ + &structs.TaskGroup{ + Name: "binsl", + Count: 1, + Tasks: []*structs.Task{ + &structs.Task{ + Name: "binstore", + Driver: "docker", + Resources: &structs.Resources{ + CPU: 100, + MemoryMB: 10, + DiskMB: 300, + IOPS: 0, + }, + LogConfig: &structs.LogConfig{ + MaxFiles: 10, + MaxFileSizeMB: 10, + }, + Artifacts: []*structs.TaskArtifact{ + { + GetterSource: "http://foo.com/bar", + GetterOptions: map[string]string{}, + RelativeDest: "", + }, + { + GetterSource: "http://foo.com/baz", + GetterOptions: map[string]string{}, + RelativeDest: "local/", + }, + { + GetterSource: "http://foo.com/bam", + GetterOptions: map[string]string{}, + RelativeDest: "var/foo", + }, + }, + }, + }, + }, + }, + }, + false, + }, } for _, tc := range cases { diff --git a/jobspec/test-fixtures/artifacts.hcl b/jobspec/test-fixtures/artifacts.hcl new file mode 100644 index 000000000000..361e8dc61b89 --- /dev/null +++ b/jobspec/test-fixtures/artifacts.hcl @@ -0,0 +1,21 @@ +job "binstore-storagelocker" { + group "binsl" { + task "binstore" { + driver = "docker" + + artifact { + source = "http://foo.com/bar" + destination = "" + } + + artifact { + source = "http://foo.com/baz" + } + artifact { + source = "http://foo.com/bam" + destination = "var/foo" + } + resources {} + } + } +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 21153f9f63b5..a3fad3ee59ed 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "net/url" + "path/filepath" "reflect" "regexp" "strconv" @@ -1913,6 +1914,10 @@ type TaskArtifact struct { // GetterOptions are options to use when downloading the artifact using // go-getter. GetterOptions map[string]string `mapstructure:"options"` + + // RelativeDest is the download destination given relative to the task's + // directory. + RelativeDest string `mapstructure:"destination"` } func (ta *TaskArtifact) Copy() *TaskArtifact { @@ -1925,16 +1930,36 @@ func (ta *TaskArtifact) Copy() *TaskArtifact { return nta } +func (ta *TaskArtifact) GoString() string { + return fmt.Sprintf("%+v", ta) +} + func (ta *TaskArtifact) Validate() error { // Verify the source var mErr multierror.Error if ta.GetterSource == "" { mErr.Errors = append(mErr.Errors, fmt.Errorf("source must be specified")) + } else { + _, err := url.Parse(ta.GetterSource) + if err != nil { + mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid source URL %q: %v", ta.GetterSource, err)) + } } - _, err := url.Parse(ta.GetterSource) + // Verify the destination doesn't escape the tasks directory + alloc := "/foo/bar/" + abs, err := filepath.Abs(filepath.Join(alloc, ta.RelativeDest)) if err != nil { - mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid source URL %q: %v", ta.GetterSource, err)) + mErr.Errors = append(mErr.Errors, err) + return mErr.ErrorOrNil() + } + rel, err := filepath.Rel(alloc, abs) + if err != nil { + mErr.Errors = append(mErr.Errors, err) + return mErr.ErrorOrNil() + } + if strings.HasPrefix(rel, "..") { + mErr.Errors = append(mErr.Errors, fmt.Errorf("destination escapes task's directory")) } // Verify the checksum diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 1b2ce95173a5..04f96635d515 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -777,6 +777,28 @@ func TestTaskArtifact_Validate_Source(t *testing.T) { } } +func TestTaskArtifact_Validate_Dest(t *testing.T) { + valid := &TaskArtifact{GetterSource: "google.com"} + if err := valid.Validate(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + valid.RelativeDest = "local/" + if err := valid.Validate(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + valid.RelativeDest = "local/.." + if err := valid.Validate(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + valid.RelativeDest = "local/../.." + if err := valid.Validate(); err == nil { + t.Fatalf("expected error: %v", err) + } +} + func TestTaskArtifact_Validate_Checksum(t *testing.T) { cases := []struct { Input *TaskArtifact diff --git a/website/source/docs/jobspec/index.html.md b/website/source/docs/jobspec/index.html.md index 0a68cb5f013b..7d277bbc292e 100644 --- a/website/source/docs/jobspec/index.html.md +++ b/website/source/docs/jobspec/index.html.md @@ -430,6 +430,10 @@ The `artifact` object maps supports the following keys: * `source` - The path to the artifact to download. +* `destination` - An optional path to download the artifact into relative to the + root of the task's directory. If the `destination` key is omitted, it will + default to `local/`. + * `options` - The `options` block allows setting parameters for `go-getter`. An example is given below: diff --git a/website/source/docs/jobspec/json.html.md b/website/source/docs/jobspec/json.html.md index d8ea022451e1..c62a8d1ec216 100644 --- a/website/source/docs/jobspec/json.html.md +++ b/website/source/docs/jobspec/json.html.md @@ -414,13 +414,16 @@ is started. The `Artifact` object maps supports the following keys: -* `Source` - The path to the artifact to download. +* `GetterSource` - The path to the artifact to download. -* `Options` - The `options` block allows setting parameters for `go-getter`. An - example is given below: +* `RelativeDest` - The destination to download the artifact relative the task's + directory. + +* `GetterOptions` - A `map[string]string` block of options for `go-getter`. An + example is given below: ``` -"Options": { +"GetterOptions": { "checksum": "md5:c4aa853ad2215426eb7d70a21922e794", "aws_access_key_id": "",