diff --git a/CHANGELOG.md b/CHANGELOG.md index 99d5ae501770..05c98c9161e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ IMPROVEMENTS: * jobspec: Lowered minimum CPU allowed from 10 to 1. [[GH-8996](https://github.com/hashicorp/nomad/issues/8996)] __BACKWARDS INCOMPATIBILITIES:__ + * core: null characters are prohibited in region, datacenter, job name/ID, task group name, and task name [[GH-9020](https://github.com/hashicorp/nomad/issues/9020)] * driver/docker: Tasks are now issued SIGTERM instead of SIGINT when stopping [[GH-8932](https://github.com/hashicorp/nomad/issues/8932)] BUG FIXES: diff --git a/api/jobs_test.go b/api/jobs_test.go index b72dc0760209..bc366949f464 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -1291,7 +1291,7 @@ func TestJobs_Info(t *testing.T) { // Trying to retrieve a job by ID before it exists // returns an error - id := "job-id/with\\troublesome:characters\n?&字\000" + id := "job-id/with\\troublesome:characters\n?&字" _, _, err := jobs.Info(id, nil) if err == nil || !strings.Contains(err.Error(), "not found") { t.Fatalf("expected not found error, got: %#v", err) @@ -1993,7 +1993,7 @@ func TestJobs_ScaleAction(t *testing.T) { defer s.Stop() jobs := c.Jobs() - id := "job-id/with\\troublesome:characters\n?&字\000" + id := "job-id/with\\troublesome:characters\n?&字" job := testJobWithScalingPolicy() job.ID = &id groupName := *job.TaskGroups[0].Name @@ -2054,7 +2054,7 @@ func TestJobs_ScaleAction_Error(t *testing.T) { defer s.Stop() jobs := c.Jobs() - id := "job-id/with\\troublesome:characters\n?&字\000" + id := "job-id/with\\troublesome:characters\n?&字" job := testJobWithScalingPolicy() job.ID = &id groupName := *job.TaskGroups[0].Name @@ -2106,7 +2106,7 @@ func TestJobs_ScaleAction_Noop(t *testing.T) { defer s.Stop() jobs := c.Jobs() - id := "job-id/with\\troublesome:characters\n?&字\000" + id := "job-id/with\\troublesome:characters\n?&字" job := testJobWithScalingPolicy() job.ID = &id groupName := *job.TaskGroups[0].Name @@ -2161,7 +2161,7 @@ func TestJobs_ScaleStatus(t *testing.T) { jobs := c.Jobs() // Trying to retrieve a status before it exists returns an error - id := "job-id/with\\troublesome:characters\n?&字\000" + id := "job-id/with\\troublesome:characters\n?&字" _, _, err := jobs.ScaleStatus(id, nil) require.Error(err) require.Contains(err.Error(), "not found") diff --git a/command/agent/command.go b/command/agent/command.go index b394ca99faf6..d94671e4e061 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -302,6 +302,18 @@ func (c *Command) isValidConfig(config, cmdConfig *Config) bool { return false } + // Check that the region does not contain invalid characters + if strings.ContainsAny(config.Region, "\000") { + c.Ui.Error("Region contains invalid characters") + return false + } + + // Check that the datacenter name does not contain invalid characters + if strings.ContainsAny(config.Datacenter, "\000") { + c.Ui.Error("Datacenter contains invalid characters") + return false + } + // Set up the TLS configuration properly if we have one. // XXX chelseakomlo: set up a TLSConfig New method which would wrap // constructor-type actions like this. diff --git a/command/agent/command_test.go b/command/agent/command_test.go index eed6bba82ed2..a2ddcc1962e8 100644 --- a/command/agent/command_test.go +++ b/command/agent/command_test.go @@ -7,8 +7,9 @@ import ( "strings" "testing" - "github.com/hashicorp/nomad/version" "github.com/mitchellh/cli" + + "github.com/hashicorp/nomad/version" ) func TestCommand_Implements(t *testing.T) { @@ -142,3 +143,101 @@ func TestCommand_MetaConfigValidation(t *testing.T) { } } } + +func TestCommand_NullCharInDatacenter(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "nomad") + if err != nil { + t.Fatalf("err: %s", err) + } + defer os.RemoveAll(tmpDir) + + tcases := []string{ + "char-\\000-in-the-middle", + "ends-with-\\000", + "\\000-at-the-beginning", + } + for _, tc := range tcases { + configFile := filepath.Join(tmpDir, "conf1.hcl") + err = ioutil.WriteFile(configFile, []byte(` + datacenter = "`+tc+`" + client{ + enabled = true + }`), 0600) + if err != nil { + t.Fatalf("err: %s", err) + } + + // Make a new command. We preemptively close the shutdownCh + // so that the command exits immediately instead of blocking. + ui := cli.NewMockUi() + shutdownCh := make(chan struct{}) + close(shutdownCh) + cmd := &Command{ + Version: version.GetVersion(), + Ui: ui, + ShutdownCh: shutdownCh, + } + + // To prevent test failures on hosts whose hostname resolves to + // a loopback address, we must append a bind address + args := []string{"-client", "-data-dir=" + tmpDir, "-config=" + configFile, "-bind=169.254.0.1"} + if code := cmd.Run(args); code != 1 { + t.Fatalf("args: %v\nexit: %d\n", args, code) + } + + out := ui.ErrorWriter.String() + exp := "Datacenter contains invalid characters" + if !strings.Contains(out, exp) { + t.Fatalf("expect to find %q\n\n%s", exp, out) + } + } +} + +func TestCommand_NullCharInRegion(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "nomad") + if err != nil { + t.Fatalf("err: %s", err) + } + defer os.RemoveAll(tmpDir) + + tcases := []string{ + "char-\\000-in-the-middle", + "ends-with-\\000", + "\\000-at-the-beginning", + } + for _, tc := range tcases { + configFile := filepath.Join(tmpDir, "conf1.hcl") + err = ioutil.WriteFile(configFile, []byte(` + region = "`+tc+`" + client{ + enabled = true + }`), 0600) + if err != nil { + t.Fatalf("err: %s", err) + } + + // Make a new command. We preemptively close the shutdownCh + // so that the command exits immediately instead of blocking. + ui := cli.NewMockUi() + shutdownCh := make(chan struct{}) + close(shutdownCh) + cmd := &Command{ + Version: version.GetVersion(), + Ui: ui, + ShutdownCh: shutdownCh, + } + + // To prevent test failures on hosts whose hostname resolves to + // a loopback address, we must append a bind address + args := []string{"-client", "-data-dir=" + tmpDir, "-config=" + configFile, "-bind=169.254.0.1"} + if code := cmd.Run(args); code != 1 { + t.Fatalf("args: %v\nexit: %d\n", args, code) + } + + out := ui.ErrorWriter.String() + exp := "Region contains invalid characters" + if !strings.Contains(out, exp) { + t.Fatalf("expect to find %q\n\n%s", exp, out) + } + } +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index bce817474fd6..66150b2dd566 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -3943,9 +3943,13 @@ func (j *Job) Validate() error { mErr.Errors = append(mErr.Errors, errors.New("Missing job ID")) } else if strings.Contains(j.ID, " ") { mErr.Errors = append(mErr.Errors, errors.New("Job ID contains a space")) + } else if strings.Contains(j.ID, "\000") { + mErr.Errors = append(mErr.Errors, errors.New("Job ID contains a null chararacter")) } if j.Name == "" { mErr.Errors = append(mErr.Errors, errors.New("Missing job name")) + } else if strings.Contains(j.Name, "\000") { + mErr.Errors = append(mErr.Errors, errors.New("Job Name contains a null chararacter")) } if j.Namespace == "" { mErr.Errors = append(mErr.Errors, errors.New("Job must be in a namespace")) @@ -5740,6 +5744,8 @@ func (tg *TaskGroup) Validate(j *Job) error { var mErr multierror.Error if tg.Name == "" { mErr.Errors = append(mErr.Errors, errors.New("Missing task group name")) + } else if strings.Contains(tg.Name, "\000") { + mErr.Errors = append(mErr.Errors, errors.New("Task group name contains null character")) } if tg.Count < 0 { mErr.Errors = append(mErr.Errors, errors.New("Task group count can't be negative")) @@ -6462,6 +6468,8 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices // We enforce this so that when creating the directory on disk it will // not have any slashes. mErr.Errors = append(mErr.Errors, errors.New("Task name cannot include slashes")) + } else if strings.Contains(t.Name, "\000") { + mErr.Errors = append(mErr.Errors, errors.New("Task name cannot include null characters")) } if t.Driver == "" { mErr.Errors = append(mErr.Errors, errors.New("Missing task driver")) diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 942710db769f..0f11942633cb 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -170,6 +170,30 @@ func TestJob_ValidateScaling(t *testing.T) { require.Contains(mErr.Errors[0].Error(), "task group count must not be less than minimum count in scaling policy") } +func TestJob_ValidateNullChar(t *testing.T) { + assert := assert.New(t) + + // job id should not allow null characters + job := testJob() + job.ID = "id_with\000null_character" + assert.Error(job.Validate(), "null character in job ID should not validate") + + // job name should not allow null characters + job.ID = "happy_little_job_id" + job.Name = "my job name with \000 characters" + assert.Error(job.Validate(), "null character in job name should not validate") + + // task group name should not allow null characters + job.Name = "my job" + job.TaskGroups[0].Name = "oh_no_another_\000_char" + assert.Error(job.Validate(), "null character in task group name should not validate") + + // task name should not allow null characters + job.TaskGroups[0].Name = "so_much_better" + job.TaskGroups[0].Tasks[0].Name = "ive_had_it_with_these_\000_chars_in_these_names" + assert.Error(job.Validate(), "null character in task name should not validate") +} + func TestJob_Warnings(t *testing.T) { cases := []struct { Name string diff --git a/website/pages/docs/upgrade/upgrade-specific.mdx b/website/pages/docs/upgrade/upgrade-specific.mdx index 5301c84b5377..a8d095a846e0 100644 --- a/website/pages/docs/upgrade/upgrade-specific.mdx +++ b/website/pages/docs/upgrade/upgrade-specific.mdx @@ -24,6 +24,14 @@ When stopping tasks running with the Docker task driver, Nomad documents that a versions of Nomad would issue `SIGINT` instead. Starting again with Nomad v0.13.0 `SIGTERM` will be sent by default when stopping Docker tasks. +### Null characters in region, datacenter, job name/ID, task group name, and task names + +Starting with Nomad v0.13.0, jobs will fail validation if any of the following +contain null character: the job ID or name, the task group name, or the task name. Any +jobs meeting this requirement should be modified before an update to v0.13.0. Similarly, +client and server config validation will prohibit either the region or the datacenter +from containing null characters. + ## Nomad 0.12.0 ### `mbits` and Task Network Resource deprecation