From cd89a97529dbd53f1cb3622c8988939caa8b2919 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Thu, 24 Mar 2016 10:55:14 -0700 Subject: [PATCH 1/2] Operator specifiable blacklist for task's using certain users --- api/tasks.go | 20 +++++---- client/config/config.go | 48 ++++++++++++++++++---- client/task_runner.go | 52 ++++++++++++++++++------ client/task_runner_test.go | 26 ++++++++++++ command/alloc_status.go | 6 +++ nomad/structs/structs.go | 33 +++++++++++---- website/source/docs/agent/config.html.md | 15 +++++++ 7 files changed, 163 insertions(+), 37 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index 3c18269ea..f357c78db 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -158,6 +158,7 @@ type TaskState struct { const ( TaskDriverFailure = "Driver Failure" TaskReceived = "Received" + TaskFailedValidation = "Failed Validation" TaskStarted = "Started" TaskTerminated = "Terminated" TaskKilled = "Killed" @@ -170,13 +171,14 @@ const ( // TaskEvent is an event that effects the state of a task and contains meta-data // appropriate to the events type. type TaskEvent struct { - Type string - Time int64 - DriverError string - ExitCode int - Signal int - Message string - KillError string - StartDelay int64 - DownloadError string + Type string + Time int64 + DriverError string + ExitCode int + Signal int + Message string + KillError string + StartDelay int64 + DownloadError string + ValidationError string } diff --git a/client/config/config.go b/client/config/config.go index 702d9a5e2..b4100d7b8 100644 --- a/client/config/config.go +++ b/client/config/config.go @@ -20,6 +20,22 @@ var ( "AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_SESSION_TOKEN", "GOOGLE_APPLICATION_CREDENTIALS", }, ",") + + // DefaulUserBlacklist is the default set of users that tasks are not + // allowed to run as when using a driver in "user.checked_drivers" + DefaultUserBlacklist = strings.Join([]string{ + "root", + "Administrator", + }, ",") + + // DefaultUserCheckedDrivers is the set of drivers we apply the user + // blacklist onto. For virtualized drivers it often doesn't make sense to + // make this stipulation so by default they are ignored. + DefaultUserCheckedDrivers = strings.Join([]string{ + "exec", + "raw_exec", + "java", + }, ",") ) // RPCHandler can be provided to the Client if there is a local server @@ -105,21 +121,17 @@ func (c *Config) Copy() *Config { // Read returns the specified configuration value or "". func (c *Config) Read(id string) string { - val, ok := c.Options[id] - if !ok { - return "" - } - return val + return c.Options[id] } // ReadDefault returns the specified configuration value, or the specified // default value if none is set. func (c *Config) ReadDefault(id string, defaultValue string) string { - val := c.Read(id) - if val != "" { - return val + val, ok := c.Options[id] + if !ok { + return defaultValue } - return defaultValue + return val } // ReadBool parses the specified option as a boolean. @@ -158,3 +170,21 @@ func (c *Config) ReadStringListToMap(key string) map[string]struct{} { } return list } + +// ReadStringListToMap tries to parse the specified option as a comma seperated list. +// If there is an error in parsing, an empty list is returned. +func (c *Config) ReadStringListToMapDefault(key, defaultValue string) map[string]struct{} { + val, ok := c.Options[key] + if !ok { + val = defaultValue + } + + list := make(map[string]struct{}) + if val != "" { + for _, e := range strings.Split(val, ",") { + trimmed := strings.TrimSpace(e) + list[trimmed] = struct{}{} + } + } + return list +} diff --git a/client/task_runner.go b/client/task_runner.go index a2d25ad22..6689c7903 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -214,10 +214,49 @@ func (r *TaskRunner) Run() { r.logger.Printf("[DEBUG] client: starting task context for '%s' (alloc '%s')", r.task.Name, r.alloc.ID) + if err := r.validateTask(); err != nil { + r.setState( + structs.TaskStateDead, + structs.NewTaskEvent(structs.TaskFailedValidation).SetValidationError(err)) + return + } + r.run() return } +// validateTask validates the fields of the task and returns an error if the +// task is invalid. +func (r *TaskRunner) validateTask() error { + var mErr multierror.Error + + // Validate the user. + unallowedUsers := r.config.ReadStringListToMapDefault("user.blacklist", config.DefaultUserBlacklist) + checkDrivers := r.config.ReadStringListToMapDefault("user.checked_drivers", config.DefaultUserCheckedDrivers) + if _, driverMatch := checkDrivers[r.task.Driver]; driverMatch { + if _, unallowed := unallowedUsers[r.task.User]; unallowed { + mErr.Errors = append(mErr.Errors, fmt.Errorf("running as user %q is disallowed", r.task.User)) + } + } + + // Validate the 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.logger.Printf("[ERR] client: allocation %q, task %v, artifact %#v (%v) fails validation: %v", + r.alloc.ID, r.task.Name, artifact, i, err) + mErr.Errors = append(mErr.Errors, fmt.Errorf("artifact (%d) failed validation: %v", i, err)) + } + } + + if len(mErr.Errors) == 1 { + return mErr.Errors[0] + } + return mErr.ErrorOrNil() +} + func (r *TaskRunner) run() { // Predeclare things so we an jump to the RESTART var handleEmpty bool @@ -236,18 +275,7 @@ func (r *TaskRunner) run() { return } - 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: %v", - r.alloc.ID, r.task.Name, artifact, i, err) - return - } - + for _, artifact := range r.task.Artifacts { if err := getter.GetArtifact(artifact, taskDir, r.logger); err != nil { r.setState(structs.TaskStateDead, structs.NewTaskEvent(structs.TaskArtifactDownloadFailed).SetDownloadError(err)) diff --git a/client/task_runner_test.go b/client/task_runner_test.go index 309d77daf..908834fcc 100644 --- a/client/task_runner_test.go +++ b/client/task_runner_test.go @@ -363,3 +363,29 @@ func TestTaskRunner_Download_Retries(t *testing.T) { t.Fatalf("Seventh Event was %v; want %v", upd.events[6].Type, structs.TaskNotRestarting) } } + +func TestTaskRunner_Validate_UserEnforcement(t *testing.T) { + _, tr := testTaskRunner(false) + + // Try to run as root with exec. + tr.task.Driver = "exec" + tr.task.User = "root" + if err := tr.validateTask(); err == nil { + t.Fatalf("expected error running as root with exec") + } + + // Try to run a non-blacklisted user with exec. + tr.task.Driver = "exec" + tr.task.User = "foobar" + if err := tr.validateTask(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Try to run as root with docker. + tr.task.Driver = "docker" + tr.task.User = "root" + if err := tr.validateTask(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + +} diff --git a/command/alloc_status.go b/command/alloc_status.go index c41eb3c05..ead7ab454 100644 --- a/command/alloc_status.go +++ b/command/alloc_status.go @@ -201,6 +201,12 @@ func (c *AllocStatusCommand) taskStatus(alloc *api.Allocation) { desc = "Task started by client" case api.TaskReceived: desc = "Task received by client" + case api.TaskFailedValidation: + if event.ValidationError != "" { + desc = event.ValidationError + } else { + desc = "Validation of task failed" + } case api.TaskDriverFailure: if event.DriverError != "" { desc = event.DriverError diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 065b07d1f..68e4aec5b 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1803,26 +1803,35 @@ func (ts *TaskState) Failed() bool { return false } - return ts.Events[l-1].Type == TaskNotRestarting + switch ts.Events[l-1].Type { + case TaskNotRestarting, TaskArtifactDownloadFailed, TaskFailedValidation: + return true + default: + return false + } } const ( - // A Driver failure indicates that the task could not be started due to a + // TaskDriveFailure indicates that the task could not be started due to a // failure in the driver. TaskDriverFailure = "Driver Failure" - // Task Received signals that the task has been pulled by the client at the + // TaskReceived signals that the task has been pulled by the client at the // given timestamp. TaskReceived = "Received" - // Task Started signals that the task was started and its timestamp can be + // TaskFailedValidation indicates the task was invalid and as such was not + // run. + TaskFailedValidation = "Failed Validation" + + // TaskStarted signals that the task was started and its timestamp can be // used to determine the running length of the task. TaskStarted = "Started" - // Task terminated indicates that the task was started and exited. + // TaskTerminated indicates that the task was started and exited. TaskTerminated = "Terminated" - // Task Killed indicates a user has killed the task. + // TaskKilled indicates a user has killed the task. TaskKilled = "Killed" // TaskRestarting indicates that task terminated and is being restarted. @@ -1832,7 +1841,7 @@ const ( // restarted because it has exceeded its restart policy. TaskNotRestarting = "Restarts Exceeded" - // Task Downloading Artifacts means the task is downloading the artifacts + // TaskDownloadingArtifacts means the task is downloading the artifacts // specified in the task. TaskDownloadingArtifacts = "Downloading Artifacts" @@ -1863,6 +1872,9 @@ type TaskEvent struct { // Artifact Download fields DownloadError string // Error downloading artifacts + + // Validation fields + ValidationError string // Validation error } func (te *TaskEvent) GoString() string { @@ -1928,6 +1940,13 @@ func (e *TaskEvent) SetDownloadError(err error) *TaskEvent { return e } +func (e *TaskEvent) SetValidationError(err error) *TaskEvent { + if err != nil { + e.ValidationError = err.Error() + } + return e +} + // TaskArtifact is an artifact to download before running the task. type TaskArtifact struct { // GetterSource is the source to download an artifact using go-getter diff --git a/website/source/docs/agent/config.html.md b/website/source/docs/agent/config.html.md index 9ceed03fe..ae9930c27 100644 --- a/website/source/docs/agent/config.html.md +++ b/website/source/docs/agent/config.html.md @@ -381,6 +381,21 @@ documentation [here](/docs/drivers/index.html) * `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, `AWS_SESSION_TOKEN` * `GOOGLE_APPLICATION_CREDENTIALS` +* `user.blacklist`: An operator specifiable blacklist of users which a task is + not allowed to run as when using a driver in `user.checked_drivers`. + Defaults to: + + * `root` + * `Administrator` + +* `user.checked_drivers`: An operator specifiable list of drivers to enforce + the the `user.blacklist`. With virtualized drivers, this enforcement often + doesn't make sense and as such the default is set to: + + * `exec` + * `raw_exec` + * `java` + * `fingerprint.whitelist`: A comma separated list of whitelisted fingerprinters. If specified, fingerprinters not in the whitelist will be disabled. If the whitelist is empty, all fingerprinters are used. From a1ff98a69a0f221b75b785b3208a415f226fd671 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Fri, 25 Mar 2016 12:43:50 -0700 Subject: [PATCH 2/2] swap raw_exec and qemu in the blacklist --- client/config/config.go | 2 +- website/source/docs/agent/config.html.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/config/config.go b/client/config/config.go index b4100d7b8..f5a0a6b0b 100644 --- a/client/config/config.go +++ b/client/config/config.go @@ -33,7 +33,7 @@ var ( // make this stipulation so by default they are ignored. DefaultUserCheckedDrivers = strings.Join([]string{ "exec", - "raw_exec", + "qemu", "java", }, ",") ) diff --git a/website/source/docs/agent/config.html.md b/website/source/docs/agent/config.html.md index ae9930c27..041bf31c8 100644 --- a/website/source/docs/agent/config.html.md +++ b/website/source/docs/agent/config.html.md @@ -389,11 +389,11 @@ documentation [here](/docs/drivers/index.html) * `Administrator` * `user.checked_drivers`: An operator specifiable list of drivers to enforce - the the `user.blacklist`. With virtualized drivers, this enforcement often + the the `user.blacklist`. For drivers using containers, this enforcement often doesn't make sense and as such the default is set to: * `exec` - * `raw_exec` + * `qemu` * `java` * `fingerprint.whitelist`: A comma separated list of whitelisted fingerprinters.