From ee4bd806ba6ccac0013eb812e7f30bfcfe12d22a Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Tue, 29 Mar 2016 16:37:43 +0000 Subject: [PATCH] API/CLI discrepancy on hostname validation (#21595). This fix tries to fix the discrepancy between API and CLI on hostname validation. Previously, the hostname validation was handled at the CLI interface in runconfig/opts/parse.go and return an error if the hostname is invalid. However, if an end user use the remote API to pass the hostname, the error will not be returned immediately. Instead the error will only be thrown out when the container creation fails. This creates behavior discrepancy between API and CLI. In this fix, the hostname validation was moved to verifyContainerSettings so the behavior will be the same for API and CLI. After the change, since CLI does not handle the hostname validation any more, the previous unit tests about hostname validation on CLI in runconfig/opts/parse_test.go has to be updated as well because there is no validation at this stage. All those unit tests are moved to integration test TestRunTooLongHostname so that the hostname validation is still properly covered as before. Note: Since the hostname validation moved to API, the error message changes from `invalid hostname format for --hostname:` to `invalid hostname format:` as well because `--hostname` is passed to CLI only. This fix fixes #21595. Signed-off-by: Yong Tang --- daemon/daemon.go | 13 ++++++++++ integration-cli/docker_cli_run_test.go | 36 +++++++++++++++++++------- runconfig/opts/parse.go | 10 ------- runconfig/opts/parse_test.go | 15 +---------- 4 files changed, 41 insertions(+), 33 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index a8c5fb9ace..71e7a81e4a 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -13,6 +13,7 @@ import ( "os" "path" "path/filepath" + "regexp" "runtime" "strings" "sync" @@ -1338,6 +1339,18 @@ func (daemon *Daemon) verifyContainerSettings(hostConfig *containertypes.HostCon return nil, err } } + + // Validate if the given hostname is RFC 1123 (https://tools.ietf.org/html/rfc1123) compliant. + if len(config.Hostname) > 0 { + // RFC1123 specifies that 63 bytes is the maximium length + // Windows has the limitation of 63 bytes in length + // Linux hostname is limited to HOST_NAME_MAX=64, not not including the terminating null byte. + // We limit the length to 63 bytes here to match RFC1035 and RFC1123. + matched, _ := regexp.MatchString("^(([[:alnum:]]|[[:alnum:]][[:alnum:]\\-]*[[:alnum:]])\\.)*([[:alnum:]]|[[:alnum:]][[:alnum:]\\-]*[[:alnum:]])$", config.Hostname) + if len(config.Hostname) > 63 || !matched { + return nil, fmt.Errorf("invalid hostname format: %s", config.Hostname) + } + } } if hostConfig == nil { diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 89cec57f72..8c4e5f4267 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -4295,15 +4295,33 @@ func (s *DockerSuite) TestRunTooLongHostname(c *check.C) { hostname1 := "this-is-a-way-too-long-hostname-but-it-should-give-a-nice-error.local" out, _, err := dockerCmdWithError("run", "--hostname", hostname1, "busybox", "echo", "test") c.Assert(err, checker.NotNil, check.Commentf("Expected docker run to fail!")) - c.Assert(out, checker.Contains, "invalid hostname format for --hostname:", check.Commentf("Expected to have 'invalid hostname format for --hostname:' in the output, get: %s!", out)) + c.Assert(out, checker.Contains, "invalid hostname format:", check.Commentf("Expected to have 'invalid hostname format:' in the output, get: %s!", out)) - // HOST_NAME_MAX=64 so 65 bytes will fail - hostname2 := "this-is-a-hostname-with-65-bytes-so-it-should-give-an-error.local" - out, _, err = dockerCmdWithError("run", "--hostname", hostname2, "busybox", "echo", "test") - c.Assert(err, checker.NotNil, check.Commentf("Expected docker run to fail!")) - c.Assert(out, checker.Contains, "invalid hostname format for --hostname:", check.Commentf("Expected to have 'invalid hostname format for --hostname:' in the output, get: %s!", out)) + // Addtional test cases + validHostnames := map[string]string{ + "hostname": "hostname", + "host-name": "host-name", + "hostname123": "hostname123", + "123hostname": "123hostname", + "hostname-of-63-bytes-long-should-be-valid-and-without-any-error": "hostname-of-63-bytes-long-should-be-valid-and-without-any-error", + } + for hostname := range validHostnames { + dockerCmd(c, "run", "--hostname", hostname, "busybox", "echo", "test") + } - // 64 bytes will be OK - hostname3 := "this-is-a-hostname-with-64-bytes-so-will-not-give-an-error.local" - dockerCmd(c, "run", "--hostname", hostname3, "busybox", "echo", "test") + invalidHostnames := map[string]string{ + "^hostname": "invalid hostname format: ^hostname", + "hostname%": "invalid hostname format: hostname%", + "host&name": "invalid hostname format: host&name", + "-hostname": "invalid hostname format: -hostname", + "host_name": "invalid hostname format: host_name", + "hostname-of-64-bytes-long-should-be-invalid-and-be-with-an-error": "invalid hostname format: hostname-of-64-bytes-long-should-be-invalid-and-be-with-an-error", + } + + for hostname, expectedError := range invalidHostnames { + out, _, err = dockerCmdWithError("run", "--hostname", hostname, "busybox", "echo", "test") + c.Assert(err, checker.NotNil, check.Commentf("Expected docker run to fail!")) + c.Assert(out, checker.Contains, expectedError, check.Commentf("Expected to have '%s' in the output, get: %s!", expectedError, out)) + + } } diff --git a/runconfig/opts/parse.go b/runconfig/opts/parse.go index 44a896d77c..c3b593dd4f 100644 --- a/runconfig/opts/parse.go +++ b/runconfig/opts/parse.go @@ -6,7 +6,6 @@ import ( "fmt" "io/ioutil" "path" - "regexp" "strconv" "strings" @@ -243,15 +242,6 @@ func Parse(cmd *flag.FlagSet, args []string) (*container.Config, *container.Host if *flEntrypoint != "" { entrypoint = strslice.StrSlice{*flEntrypoint} } - // Validate if the given hostname is RFC 1123 (https://tools.ietf.org/html/rfc1123) compliant. - hostname := *flHostname - if hostname != "" { - // Linux hostname is limited to HOST_NAME_MAX=64, not including the terminating null byte. - matched, _ := regexp.MatchString("^(([[:alnum:]]|[[:alnum:]][[:alnum:]\\-]*[[:alnum:]])\\.)*([[:alnum:]]|[[:alnum:]][[:alnum:]\\-]*[[:alnum:]])$", hostname) - if len(hostname) > 64 || !matched { - return nil, nil, nil, cmd, fmt.Errorf("invalid hostname format for --hostname: %s", hostname) - } - } ports, portBindings, err := nat.ParsePortSpecs(flPublish.GetAll()) if err != nil { diff --git a/runconfig/opts/parse_test.go b/runconfig/opts/parse_test.go index 1885544317..30f755c792 100644 --- a/runconfig/opts/parse_test.go +++ b/runconfig/opts/parse_test.go @@ -390,15 +390,7 @@ func TestParseHostname(t *testing.T) { "host-name": "host-name", "hostname123": "hostname123", "123hostname": "123hostname", - "hostname-of-64-bytes-long-should-be-valid-and-without-any-errors": "hostname-of-64-bytes-long-should-be-valid-and-without-any-errors", - } - invalidHostnames := map[string]string{ - "^hostname": "invalid hostname format for --hostname: ^hostname", - "hostname%": "invalid hostname format for --hostname: hostname%", - "host&name": "invalid hostname format for --hostname: host&name", - "-hostname": "invalid hostname format for --hostname: -hostname", - "host_name": "invalid hostname format for --hostname: host_name", - "hostname-of-65-bytes-long-should-be-invalid-and-be-given-an-error": "invalid hostname format for --hostname: hostname-of-65-bytes-long-should-be-invalid-and-be-given-an-error", + "hostname-of-63-bytes-long-should-be-valid-and-without-any-error": "hostname-of-63-bytes-long-should-be-valid-and-without-any-error", } hostnameWithDomain := "--hostname=hostname.domainname" hostnameWithDomainTld := "--hostname=hostname.domainname.tld" @@ -407,11 +399,6 @@ func TestParseHostname(t *testing.T) { t.Fatalf("Expected the config to have 'hostname' as hostname, got '%v'", config.Hostname) } } - for hostname, expectedError := range invalidHostnames { - if _, _, err := parse(t, fmt.Sprintf("--hostname=%s", hostname)); err == nil || err.Error() != expectedError { - t.Fatalf("Expected error '%v' with '--hostname=%s', got '%s'", expectedError, hostname, err) - } - } if config, _ := mustParse(t, hostnameWithDomain); config.Hostname != "hostname.domainname" && config.Domainname != "" { t.Fatalf("Expected the config to have 'hostname' as hostname.domainname, got '%v'", config.Hostname) }