Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[23.0 backport] client: define a "dummy" hostname to use for local connections #45971

Merged
merged 4 commits into from
Jul 14, 2023
Merged

[23.0 backport] client: define a "dummy" hostname to use for local connections #45971

merged 4 commits into from
Jul 14, 2023

Conversation

thaJeztah
Copy link
Member

For local communications (npipe://, unix://), the hostname is not used, but we need valid and meaningful hostname.

The current code used the client's addr as hostname in some cases, which could contain the path for the unix-socket (/var/run/docker.sock), which gets rejected by go1.20.6 and go1.19.11 because of a security fix for CVE-2023-29406 , which was implemented in https://go.dev/issue/60374.

Prior versions go Go would clean the host header, and strip slashes in the process, but go1.20.6 and go1.19.11 no longer do, and reject the host header.

This patch introduces a DummyHost const, and uses this dummy host for cases where we don't need an actual hostname.

Before this patch (using go1.20.6):

make GO_VERSION=1.20.6 TEST_FILTER=TestAttach test-integration
=== RUN   TestAttachWithTTY
    attach_test.go:46: assertion failed: error is not nil: http: invalid Host header
--- FAIL: TestAttachWithTTY (0.11s)
=== RUN   TestAttachWithoutTTy
    attach_test.go:46: assertion failed: error is not nil: http: invalid Host header
--- FAIL: TestAttachWithoutTTy (0.02s)
FAIL

With this patch applied:

make GO_VERSION=1.20.6 TEST_FILTER=TestAttach test-integration
INFO: Testing against a local daemon
=== RUN   TestAttachWithTTY
--- PASS: TestAttachWithTTY (0.12s)
=== RUN   TestAttachWithoutTTy
--- PASS: TestAttachWithoutTTy (0.02s)
PASS

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
client: TestSetHostHeader: don't use un-keyed literals
65468a2 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 2a59188)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
client: define a "dummy" hostname to use for local connections
5532d51 
For local communications (npipe://, unix://), the hostname is not used,
but we need valid and meaningful hostname.

The current code used the client's `addr` as hostname in some cases, which
could contain the path for the unix-socket (`/var/run/docker.sock`), which
gets rejected by go1.20.6 and go1.19.11 because of a security fix for
[CVE-2023-29406 ][1], which was implemented in  https://go.dev/issue/60374.

Prior versions go Go would clean the host header, and strip slashes in the
process, but go1.20.6 and go1.19.11 no longer do, and reject the host
header.

This patch introduces a `DummyHost` const, and uses this dummy host for
cases where we don't need an actual hostname.

Before this patch (using go1.20.6):

    make GO_VERSION=1.20.6 TEST_FILTER=TestAttach test-integration
    === RUN   TestAttachWithTTY
        attach_test.go:46: assertion failed: error is not nil: http: invalid Host header
    --- FAIL: TestAttachWithTTY (0.11s)
    === RUN   TestAttachWithoutTTy
        attach_test.go:46: assertion failed: error is not nil: http: invalid Host header
    --- FAIL: TestAttachWithoutTTy (0.02s)
    FAIL

With this patch applied:

    make GO_VERSION=1.20.6 TEST_FILTER=TestAttach test-integration
    INFO: Testing against a local daemon
    === RUN   TestAttachWithTTY
    --- PASS: TestAttachWithTTY (0.12s)
    === RUN   TestAttachWithoutTTy
    --- PASS: TestAttachWithoutTTy (0.02s)
    PASS

[1]: GHSA-f8f7-69v5-w4vx

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 92975f0)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
pkg/plugins: use a dummy hostname for local connections
5f68c54 
For local communications (npipe://, unix://), the hostname is not used,
but we need valid and meaningful hostname.

The current code used the socket path as hostname, which gets rejected by
go1.20.6 and go1.19.11 because of a security fix for [CVE-2023-29406 ][1],
which was implemented in  https://go.dev/issue/60374.

Prior versions go Go would clean the host header, and strip slashes in the
process, but go1.20.6 and go1.19.11 no longer do, and reject the host
header.

Before this patch, tests would fail on go1.20.6:

    === FAIL: pkg/authorization TestAuthZRequestPlugin (15.01s)
    time="2023-07-12T12:53:45Z" level=warning msg="Unable to connect to plugin: //tmp/authz2422457390/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 1s"
    time="2023-07-12T12:53:46Z" level=warning msg="Unable to connect to plugin: //tmp/authz2422457390/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 2s"
    time="2023-07-12T12:53:48Z" level=warning msg="Unable to connect to plugin: //tmp/authz2422457390/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 4s"
    time="2023-07-12T12:53:52Z" level=warning msg="Unable to connect to plugin: //tmp/authz2422457390/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 8s"
        authz_unix_test.go:82: Failed to authorize request Post "http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq": http: invalid Host header

[1]: GHSA-f8f7-69v5-w4vx

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 6b7705d)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
testutil: use dummyhost for non-tcp connections
03c5a81 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit e1db9e9)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah added this to the 23.0.7 milestone Jul 14, 2023
@thaJeztah
Copy link
Member Author

This one's green at least 🎉

I'll bring this one in, and do a vendor update in BuildKit 0.11 branch

@thaJeztah thaJeztah merged commit f00e7af into moby:23.0 Jul 14, 2023
86 checks passed
@thaJeztah thaJeztah deleted the 23.0_backport_fix_host_header branch July 14, 2023 21:58
@DmitriyLewen DmitriyLewen mentioned this pull request Jul 21, 2023
Merged
6 tasks
zregvart added a commit to zregvart/ec-cli that referenced this pull request Jul 21, 2023
@zregvart
Fix acceptance tests container launching issue
1c2c185 
A change[1] was backported to golang 1.19 that introduces additional
checks on the `Host` HTTP header. The docker client connecting via
Unix domain socket to the docker daemon does not specify the `Host`
header and that triggers the error `http: invalid Host header`.

The fix[2] was introduced in an unreleased version[3] of docker client
which we use via commit id here to resolve the issue.

[1] golang/go#61075
[2] moby/moby#45935
[3] moby/moby#45971
@zregvart zregvart mentioned this pull request Jul 21, 2023
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corhere corhere corhere approved these changes

Assignees
No one assigned
Labels
impact/changelog kind/bugfix PR's that fix bugs status/2-code-review
Projects
None yet
Milestone
23.0.7
Development

Successfully merging this pull request may close these issues.
2 participants
@thaJeztah @corhere