From d4ef12e42ebfde8575caa86e8b649cf0d6fcb2e5 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Sat, 3 Oct 2020 21:46:58 +0000 Subject: [PATCH 1/4] documenting tests around null characters in job id, task group name, and task name --- nomad/structs/structs_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 942710db769..0c721cd319c 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -170,6 +170,25 @@ 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") + + // task group name should not allow null characters + job.ID = "happy_little_job_id" + 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 = "what_is_with_these_\000_chars" + assert.Error(job.Validate(), "null character in task name should not validate") +} + func TestJob_Warnings(t *testing.T) { cases := []struct { Name string From 39c73f1b3256b1f780d329de381a3bf2d1a21d9b Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Sat, 3 Oct 2020 22:07:18 +0000 Subject: [PATCH 2/4] updated job validate to refute job/group/task IDs containing null characters updated CHANGELOG and upgrade guide --- CHANGELOG.md | 1 + nomad/structs/structs.go | 6 ++++++ nomad/structs/structs_test.go | 2 +- website/pages/docs/upgrade/upgrade-specific.mdx | 6 ++++++ 4 files changed, 14 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99d5ae50177..dcc8f3f51d0 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: job IDs, task group names, and task names are not allowed to contain null characters [[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/nomad/structs/structs.go b/nomad/structs/structs.go index bce817474fd..41f8adc4ae8 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -3943,6 +3943,8 @@ 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")) @@ -5740,6 +5742,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 +6466,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 0c721cd319c..9de7bece998 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -185,7 +185,7 @@ func TestJob_ValidateNullChar(t *testing.T) { // task name should not allow null characters job.TaskGroups[0].Name = "so_much_better" - job.TaskGroups[0].Tasks[0].Name = "what_is_with_these_\000_chars" + 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") } diff --git a/website/pages/docs/upgrade/upgrade-specific.mdx b/website/pages/docs/upgrade/upgrade-specific.mdx index 5301c84b537..37ff32c5253 100644 --- a/website/pages/docs/upgrade/upgrade-specific.mdx +++ b/website/pages/docs/upgrade/upgrade-specific.mdx @@ -24,6 +24,12 @@ 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 job, task group, and task IDs + +Starting with Nomad v0.13.0, job will fail validation if any of the following +contain null character: the job ID, the task group name, or the task name. Any +jobs meeting this requirement should be modified before an update to v0.13.0. + ## Nomad 0.12.0 ### `mbits` and Task Network Resource deprecation From 5062e74b3e644887ffc39b41e07f341c1ba9af47 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Sat, 3 Oct 2020 22:34:15 +0000 Subject: [PATCH 3/4] updated api tests wrt backwards compat on null chars in IDs --- api/jobs_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index b72dc076020..bc366949f46 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") From b0c2e5176a8e0dff88c152460f3d59f9310df62a Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Mon, 5 Oct 2020 14:52:07 +0000 Subject: [PATCH 4/4] updated docs and validation to further prohibit null chars in region, datacenter, and job name --- CHANGELOG.md | 2 +- command/agent/command.go | 12 +++ command/agent/command_test.go | 101 +++++++++++++++++- nomad/structs/structs.go | 2 + nomad/structs/structs_test.go | 7 +- .../pages/docs/upgrade/upgrade-specific.mdx | 10 +- 6 files changed, 127 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcc8f3f51d0..05c98c9161e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ IMPROVEMENTS: * jobspec: Lowered minimum CPU allowed from 10 to 1. [[GH-8996](https://github.com/hashicorp/nomad/issues/8996)] __BACKWARDS INCOMPATIBILITIES:__ - * core: job IDs, task group names, and task names are not allowed to contain null characters [[GH-9020](https://github.com/hashicorp/nomad/issues/9020)] + * 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/command/agent/command.go b/command/agent/command.go index b394ca99faf..d94671e4e06 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 eed6bba82ed..a2ddcc1962e 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 41f8adc4ae8..66150b2dd56 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -3948,6 +3948,8 @@ func (j *Job) Validate() error { } 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")) diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 9de7bece998..0f11942633c 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -178,8 +178,13 @@ func TestJob_ValidateNullChar(t *testing.T) { job.ID = "id_with\000null_character" assert.Error(job.Validate(), "null character in job ID should not validate") - // task group name should not allow null characters + // 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") diff --git a/website/pages/docs/upgrade/upgrade-specific.mdx b/website/pages/docs/upgrade/upgrade-specific.mdx index 37ff32c5253..a8d095a846e 100644 --- a/website/pages/docs/upgrade/upgrade-specific.mdx +++ b/website/pages/docs/upgrade/upgrade-specific.mdx @@ -24,11 +24,13 @@ 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 job, task group, and task IDs +### Null characters in region, datacenter, job name/ID, task group name, and task names -Starting with Nomad v0.13.0, job will fail validation if any of the following -contain null character: the job ID, the task group name, or the task name. Any -jobs meeting this requirement should be modified before an update to v0.13.0. +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