Ignore invalid host header between go1.6 and old docker clients by runcom · Pull Request #22000 · moby/moby

runcom · 2016-04-13T14:23:32Z

Fix: #20865

reworked the patch from @ibuildthecloud just in case Docker still needs this or something like this

ping @cpuguy83

Signed-off-by: Antonio Murdaca runcom@redhat.com

calavera · 2016-04-13T17:19:36Z

we're not using Go 1.6 and I'd rather not scan the connection in each request looking for that header.

runcom · 2016-04-13T17:41:29Z

I'd rather not scan the connection in each request looking for that header.

any other idea on how to do this then? I understand you're not using go1.6 now but then 1.7 will have the same issue and old clients will be broken.

tianon · 2016-04-25T19:54:48Z

I think our downstreams would appreciate having some official solution to this sooner rather than later. 😢 (Since they're going to be increasingly bit by this the further away from Go 1.6's release we get.)

I ran into it on Gentoo and was thankful to see this patch which does fix the issue for me locally (and I don't mind the minor perf hit of the header scanning). IMO since we'll need it eventually, we might as well be working on ways to work around it so we're prepared.

yosifkit · 2016-04-26T17:10:48Z

pkg/listeners/malformed_host_override.go

Wouldn't this be slightly simpler with:

if strings.HasPrefix(parts[1], "Host: /") { # old docker stuff } else { head = parts }

The first if just skips any other clients except the docker client itself. I don't think we want to fix other clients that could send invalid host header - we're just interested in the docker client case. so if you use curl with an invalid host header not at parts[1](like the docker client) you'll be getting the invalid host header error and we shouldn't care, curl|users should.

They both have the same logic. What case am I missing?

parts[1] current code new code

foo head=parts head=parts

Host: bar head=parts head=parts

Host: /baz handle stuff handle stuff

runcom · 2016-05-03T07:54:38Z

ping @icecrime

cpuguy83 · 2016-05-03T14:56:32Z

Wondering if we really think this will be a problem in the 1.12 time-frame.
What's the use-case for a unix-socket client to be running with a different version of the daemon (now 2 versions old).

runcom · 2016-05-03T15:00:53Z

ppl could have the old docker client built in an image and suddenly it stops working.

tiborvass · 2016-05-05T19:01:41Z

Let's add a Docker envvar that allows people to add the hacky workaround. Let's document it properly and warn that we will remove it in 2 releases.

stevvooe · 2016-05-05T20:14:27Z

This seems like an untenable amount of technical debt to take on, especially considering that the Host header is clearly invalid. This fix is a future bug waiting to happen.

What's wrong with requiring old clients to connect over http? Can we not backport a fix to correctly set the host header?

thaJeztah · 2016-05-05T23:04:10Z

@stevvooe concerns were raised that even if we would back port a fix to older versions (which we basically don't do), there would be containers around containing older docker clients that connect using a socket (docker run -v /var/run/docker.sock:/var/run/docker.sock something). Those containers would no longer be able to run in newer versions of docker.

runcom · 2016-05-06T07:05:33Z

I'm modifying this patch to use an env variable daemon side and activate this behavior, as @tibor said, we'll remove it in 2 releases

runcom · 2016-05-06T08:18:39Z

This seems like an untenable amount of technical debt to take on, especially considering that the Host header is clearly invalid. This fix is a future bug waiting to happen.

untenable? Supporting this as an env variable is IMO anything but untenable. People who needs this will provide the variable, people who doesn't won't. We'll remove these specific files in two releases and it'll be straightforward...
Fact is that people are experiencing this issue and we're not using go1.6 because of this. The untenable tech debt we're carrying is PR like this one distribution/distribution#1696 where we're forced not to use go1.6 because there's this specific issue laying around yet which isn't right to just ignore to me.

runcom · 2016-05-06T08:50:45Z

I've moved everything docker-side so when/if this will be removed it will be straightforward to do so and we don't clutter pkg/listeners. I'll be adding documentation about this env variable ping @thaJeztah

runcom · 2016-05-06T08:54:57Z

cmd/dockerd/daemon_unix.go

naming here is greatly welcome 😄

runcom · 2016-05-06T08:57:36Z

ping @tiborvass @tianon @cpuguy83

Starting the daemon w/o and w/ (Environment=DOCKER_HOST_HEADER_COMPAT=1 with systemd) the env variable provided:

$ ./docker-1.9.1 version
Client:
 Version:      1.9.1
 API version:  1.21
 Go version:   go1.4.3
 Git commit:   a34a1d5
 Built:        Fri Nov 20 17:56:04 UTC 2015
 OS/Arch:      linux/amd64
Error response from daemon: 400 Bad Request: malformed Host header

$ ./docker-1.9.1 version
Client:
 Version:      1.9.1
 API version:  1.21
 Go version:   go1.4.3
 Git commit:   a34a1d5
 Built:        Fri Nov 20 17:56:04 UTC 2015
 OS/Arch:      linux/amd64

Server:
 Version:      1.12.0-dev
 API version:  1.24
 Go version:   go1.6.1
 Git commit:   35c2cd1
 Built:        2016-05-06T10:49:33.614538998+02:00
 OS/Arch:      linux/amd64
 Experimental: true

runcom · 2016-05-06T09:04:08Z

cmd/dockerd/daemon_windows.go

does windows need this also? @jhowardmsft

thaJeztah · 2016-05-06T09:33:27Z

how should we name the environment var? DOCKER_NO_HOST_HEADER_CHECK? DOCKER_HOST_HEADER_COMPAT? or more generic DOCKER_COMPAT_MODE?

duglin · 2016-05-06T13:01:58Z

DOCKER_HOST_HEADER_COMPAT sounds better than something with HACK in it - as funny as that is. ;-)

code looks good to me so far - if we can just get the docs and perhaps a testcase?

thaJeztah · 2016-05-26T13:18:54Z

Note that the environment variable is removed in #22888, and this is now the default

runcom · 2016-05-26T13:22:29Z

🎉

…ents upstream reference: moby#22000 BenchmarkWithHack-4 50000 37082 ns/op 44.50 MB/s 1920 B/op 30 allocs/op BenchmarkNoHack-4 50000 30829 ns/op 53.52 MB/s 0 B/op 0 allocs/op Signed-off-by: Brian Goff <cpuguy83@gmail.com> Signed-off-by: Antonio Murdaca <runcom@redhat.com>

…ents Upstream reference: moby#22000 Upstream reference: moby#23046 Upstream reference: moby#22888 Signed-off-by: Antonio Murdaca <runcom@redhat.com>

BenchmarkWithHack-4 50000 37082 ns/op 44.50 MB/s 1920 B/op 30 allocs/op BenchmarkNoHack-4 50000 30829 ns/op 53.52 MB/s 0 B/op 0 allocs/op bump go version to v1.7 will hit issue: moby#20865 backport this patch for fix this. cherry-pick from: moby#22000 Use a 1.9.1 client connect to a 1.11.2 daemon which is build by golang 1.7.1 before this patch: ``` $ ./docker-1.9.1 version WARNING: Error loading config file:Invalid auth configuration file Client: Version: 1.9.1 API version: 1.21 Go version: go1.4.3 Git commit: a34a1d5 Built: Fri Nov 20 17:56:04 UTC 2015 OS/Arch: linux/amd64 Error response from daemon: 400 Bad Request: malformed Host header ``` after this patch ``` $ ./docker-1.9.1 version WARNING: Error loading config file:Invalid auth configuration file Client: Version: 1.9.1 API version: 1.21 Go version: go1.4.3 Git commit: a34a1d5 Built: Fri Nov 20 17:56:04 UTC 2015 OS/Arch: linux/amd64 Server: Version: 1.11.2 API version: 1.23 Go version: go1.7.1 Git commit: 69f8a6b-unsupported Built: 2017-01-10T03:03:45.940523050+00:00 OS/Arch: linux/amd64 ``` Signed-off-by: Brian Goff <cpuguy83@gmail.com> Signed-off-by: Antonio Murdaca <runcom@redhat.com> Signed-off-by: Lei Jitang <leijitang@huawei.com>

Ignore invalid host header between go1.6 and old docker clients to bump go to v1.7.1 ``` BenchmarkWithHack-4 50000 37082 ns/op 44.50 MB/s 1920 B/op 30 allocs/op BenchmarkNoHack-4 50000 30829 ns/op 53.52 MB/s 0 B/op 0 allocs/op ``` bump go version to v1.7 will hit issue: moby#20865 backport this patch for fix this. cherry-pick from: moby#22000 Use a 1.9.1 client connect to a 1.11.2 daemon which is build by golang 1.7.1 before this patch: ``` $ ./docker-1.9.1 version WARNING: Error loading config file:Invalid auth configuration file Client: Version: 1.9.1 API version: 1.21 Go version: go1.4.3 Git commit: a34a1d5 Built: Fri Nov 20 17:56:04 UTC 2015 OS/Arch: linux/amd64 Error response from daemon: 400 Bad Request: malformed Host header ``` after this patch ``` $ ./docker-1.9.1 version WARNING: Error loading config file:Invalid auth configuration file Client: Version: 1.9.1 API version: 1.21 Go version: go1.4.3 Git commit: a34a1d5 Built: Fri Nov 20 17:56:04 UTC 2015 OS/Arch: linux/amd64 Server: Version: 1.11.2 API version: 1.23 Go version: go1.7.1 Git commit: 69f8a6b-unsupported Built: 2017-01-10T03:03:45.940523050+00:00 OS/Arch: linux/amd64 ``` Signed-off-by: Brian Goff <cpuguy83@gmail.com> Signed-off-by: Antonio Murdaca <runcom@redhat.com> Signed-off-by: Lei Jitang <leijitang@huawei.com> See merge request docker/docker!244

This hack was added to fix a compatibility with clients that were built using Go 1.5 and older (added in 3d6f598) This hack causes some problems with current clients; with Go 1.5 and older no longer being supported for some time, and being several years old, it should now be ok to remove this hack altogether. People using tools that are built with those versions of Go wouldn't have updated those for years, and are probably out of date anyway; that's not something we can continue taking into account. This will affect docker clients (the docker cli) for docker 1.12 and older. Those versions have reached EOL a long time ago (and have known unpatched vulnerabilities), so should no longer be used anyway, but We should add a nebtuib in the release notes, just in case someone, somewhere, still has such old tools. For those affected, using a more recent client (and if needed, setting the DOCKER_API_VERSION environment variable to the needed API version) should provide a way out. This reverts the changes originally made in; moby#22000 and moby#22888, which were to address moby#20865. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This hack was added to fix a compatibility with clients that were built using Go 1.5 and older (added in 3d6f598) This hack causes some problems with current clients; with Go 1.5 and older no longer being supported for some time, and being several years old, it should now be ok to remove this hack altogether. People using tools that are built with those versions of Go wouldn't have updated those for years, and are probably out of date anyway; that's not something we can continue taking into account. This will affect docker clients (the docker cli) for docker 1.12 and older. Those versions have reached EOL a long time ago (and have known unpatched vulnerabilities), so should no longer be used anyway, but We should add a nebtuib in the release notes, just in case someone, somewhere, still has such old tools. For those affected, using a more recent client (and if needed, setting the DOCKER_API_VERSION environment variable to the needed API version) should provide a way out. This reverts the changes originally made in; moby#22000 and moby#22888, which were to address moby#20865. Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: zach <Zachary.Joyner@linux.com>

between go1.6 and old docker clients Origin: moby#22000

GordonTheTurtle added the status/0-triage label Apr 13, 2016

runcom force-pushed the malformed-host-header-upstream branch 2 times, most recently from 16f0627 to 1caaf5a Compare April 13, 2016 14:31

runcom added status/1-design-review and removed status/0-triage labels Apr 13, 2016

yosifkit reviewed Apr 26, 2016
View reviewed changes

aaronlehmann mentioned this pull request May 4, 2016

System CA pool excluded when registry CA is used from /etc/docker #12756

Closed

runcom force-pushed the malformed-host-header-upstream branch 4 times, most recently from f71ec21 to 35c2cd1 Compare May 6, 2016 08:49

runcom reviewed May 6, 2016
View reviewed changes

cmd/dockerd/daemon_unix.go Outdated

Copy link
Copy Markdown

Member Author

runcom May 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming here is greatly welcome 😄

runcom reviewed May 6, 2016
View reviewed changes

cmd/dockerd/daemon_windows.go Outdated

Copy link
Copy Markdown

Member Author

runcom May 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does windows need this also? @jhowardmsft

runcom force-pushed the malformed-host-header-upstream branch from 35c2cd1 to fe95987 Compare May 6, 2016 09:13

This was referenced May 23, 2016

Docker moving to Go 1.6 will break Weave weaveworks/weave#2308

Closed

Docker moving to Go 1.6 will break Weave weaveworks/weave#2309

Closed

thaJeztah mentioned this pull request Jun 5, 2016

Upgrade to Go 1.6.2 (again) docker-archive/classicswarm#2332

Closed

thaJeztah mentioned this pull request Jun 17, 2016

v1.12.0: some fixes to CHANGELOG #23594

Merged

tiborvass removed the impact/changelog label Jun 17, 2016

thaJeztah mentioned this pull request Jul 17, 2016

when is excon updated to make docker-api work again? boot2docker/boot2docker#1181

Closed

pbrisbin mentioned this pull request Jul 29, 2016

Error 400 bad request: malformed host header on first analyze qltysh/qlty#475

Closed

thaJeztah mentioned this pull request Apr 15, 2019

Remove hack MalformedHostHeaderOverride #39076

Merged

ailispaw added a commit to bargees/moby that referenced this pull request May 28, 2022

[PATCH] Ignore invalid host header

f69a089

between go1.6 and old docker clients Origin: moby#22000

thaJeztah mentioned this pull request Jul 12, 2023

Golang client fails to attach to streams with "http: invalid Host header" with go1.20.6, go1.19.11 #45935

Closed

neersighted mentioned this pull request Jul 12, 2023

ci(bin-image): fix meta step #45932

Merged

crazy-max mentioned this pull request Jul 12, 2023

hack: allow to set GO_VERSION during tests moby/buildkit#4017

Merged

parts[1]	current code	new code
foo	head=parts	head=parts
Host: bar	head=parts	head=parts
Host: /baz	handle stuff	handle stuff

Conversation

runcom commented Apr 13, 2016

Uh oh!

calavera commented Apr 13, 2016

Uh oh!

runcom commented Apr 13, 2016

Uh oh!

tianon commented Apr 25, 2016

Uh oh!

yosifkit Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

runcom Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

yosifkit Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

runcom commented May 3, 2016

Uh oh!

cpuguy83 commented May 3, 2016

Uh oh!

runcom commented May 3, 2016

Uh oh!

tiborvass commented May 5, 2016

Uh oh!

stevvooe commented May 5, 2016

Uh oh!

thaJeztah commented May 5, 2016

Uh oh!

runcom commented May 6, 2016

Uh oh!

runcom commented May 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented May 6, 2016

Uh oh!

runcom May 6, 2016

Choose a reason for hiding this comment

Uh oh!

runcom commented May 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom May 6, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 6, 2016

Uh oh!

duglin commented May 6, 2016

Uh oh!

thaJeztah commented May 26, 2016

Uh oh!

runcom commented May 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

runcom commented May 6, 2016 •

edited

Loading

runcom commented May 6, 2016 •

edited

Loading