Skip to content

[do not merge] update to go1.20.6 and add client host fix#45943

Closed
thaJeztah wants to merge 5 commits intomoby:masterfrom
thaJeztah:go1.20.6_with_client_fix
Closed

[do not merge] update to go1.20.6 and add client host fix#45943
thaJeztah wants to merge 5 commits intomoby:masterfrom
thaJeztah:go1.20.6_with_client_fix

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Jul 12, 2023

This was referenced Jul 12, 2023
Merged
Merged
@thaJeztah thaJeztah force-pushed the go1.20.6_with_client_fix branch from d50803b to 8a179fa Compare July 12, 2023 12:49
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jul 12, 2023

Looks like we need more code updated;

[2023-07-12T12:54:46.838Z] === Failed
[2023-07-12T12:54:46.838Z] === FAIL: pkg/authorization TestAuthZRequestPluginError (15.01s)
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:53:30Z" level=warning msg="Unable to connect to plugin: //tmp/authz4264769183/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz4264769183%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 1s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:53:31Z" level=warning msg="Unable to connect to plugin: //tmp/authz4264769183/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz4264769183%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 2s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:53:33Z" level=warning msg="Unable to connect to plugin: //tmp/authz4264769183/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz4264769183%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 4s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:53:37Z" level=warning msg="Unable to connect to plugin: //tmp/authz4264769183/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz4264769183%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 8s"
[2023-07-12T12:54:46.838Z]     authz_unix_test.go:50: Failed to authorize request Post "http://%2F%2Ftmp%2Fauthz4264769183%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq": http: invalid Host header
[2023-07-12T12:54:46.838Z] 
[2023-07-12T12:54:46.838Z] === FAIL: pkg/authorization TestAuthZRequestPlugin (15.01s)
[2023-07-12T12:54:46.838Z] 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"
[2023-07-12T12:54:46.838Z] 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"
[2023-07-12T12:54:46.838Z] 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"
[2023-07-12T12:54:46.838Z] 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"
[2023-07-12T12:54:46.838Z]     authz_unix_test.go:82: Failed to authorize request Post "http://%2F%2Ftmp%2Fauthz2422457390%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq": http: invalid Host header
[2023-07-12T12:54:46.838Z] 
[2023-07-12T12:54:46.838Z] === FAIL: pkg/authorization TestAuthZResponsePlugin (15.01s)
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:00Z" level=warning msg="Unable to connect to plugin: //tmp/authz3990016376/authz-test-plugin.sock/AuthZPlugin.AuthZRes: Post \"http://%2F%2Ftmp%2Fauthz3990016376%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZRes\": http: invalid Host header, retrying in 1s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:01Z" level=warning msg="Unable to connect to plugin: //tmp/authz3990016376/authz-test-plugin.sock/AuthZPlugin.AuthZRes: Post \"http://%2F%2Ftmp%2Fauthz3990016376%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZRes\": http: invalid Host header, retrying in 2s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:03Z" level=warning msg="Unable to connect to plugin: //tmp/authz3990016376/authz-test-plugin.sock/AuthZPlugin.AuthZRes: Post \"http://%2F%2Ftmp%2Fauthz3990016376%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZRes\": http: invalid Host header, retrying in 4s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:07Z" level=warning msg="Unable to connect to plugin: //tmp/authz3990016376/authz-test-plugin.sock/AuthZPlugin.AuthZRes: Post \"http://%2F%2Ftmp%2Fauthz3990016376%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZRes\": http: invalid Host header, retrying in 8s"
[2023-07-12T12:54:46.838Z]     authz_unix_test.go:112: Failed to authorize request Post "http://%2F%2Ftmp%2Fauthz3990016376%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZRes": http: invalid Host header
[2023-07-12T12:54:46.838Z] 
[2023-07-12T12:54:46.838Z] === FAIL: pkg/authorization TestMiddlewareWrapHandler/Positive_Test_Case_: (15.01s)
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:30Z" level=warning msg="Unable to connect to plugin: //tmp/authz1534243050/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz1534243050%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 1s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:31Z" level=warning msg="Unable to connect to plugin: //tmp/authz1534243050/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz1534243050%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 2s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:33Z" level=warning msg="Unable to connect to plugin: //tmp/authz1534243050/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz1534243050%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 4s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:37Z" level=warning msg="Unable to connect to plugin: //tmp/authz1534243050/authz-test-plugin.sock/AuthZPlugin.AuthZReq: Post \"http://%2F%2Ftmp%2Fauthz1534243050%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header, retrying in 8s"
[2023-07-12T12:54:46.838Z] time="2023-07-12T12:54:45Z" level=error msg="AuthZRequest for GET www.example.com/auth returned error: plugin plugin failed with error: Post \"http://%2F%2Ftmp%2Fauthz1534243050%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq\": http: invalid Host header"
[2023-07-12T12:54:46.838Z]     middleware_unix_test.go:60: assertion failed: error is not nil: plugin plugin failed with error: Post "http://%2F%2Ftmp%2Fauthz1534243050%2Fauthz-test-plugin.sock/AuthZPlugin.AuthZReq": http: invalid Host header
[2023-07-12T12:54:46.838Z] 
[2023-07-12T12:54:46.838Z] === FAIL: pkg/authorization TestMiddlewareWrapHandler (30.02s)

@thaJeztah thaJeztah force-pushed the go1.20.6_with_client_fix branch 4 times, most recently from 3e995ed to fe29f6f Compare July 12, 2023 16:06
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jul 12, 2023 •
edited
Loading

BuildKit tests still failing on this one, but probably because BuildKit v0.11 uses docker engine v23.0.0-rc.1; https://github.com/moby/buildkit/blob/0a0807ebcc8573d6da9be70728c2f75408a6dad2/go.mod#L30C18-L30C52

github.com/docker/docker v23.0.0-rc.1+incompatible

So this fix would have to be backported to v23, and buildkit would have to update the dependency.. fun fun

I had a couple of PRs to update dependencies, but they were not (yet) accepted, so perhaps time to revive those

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jul 12, 2023

Alright; CI finished, and looks like everything is green except for BuildKit CI

Screenshot 2023-07-12 at 19 01 50

Once #45947 is merged, I guess I could temporarily pin BuildKit CI to v1.20.5, but it looks like that PR still has some failures (not sure what)

@thaJeztah thaJeztah force-pushed the go1.20.6_with_client_fix branch 3 times, most recently from 2434e26 to 5023412 Compare July 13, 2023 09:47
@crazy-max
Copy link
Copy Markdown
Member

crazy-max commented Jul 13, 2023

Can you also bump go version in buildkit workflow?

moby/.github/workflows/buildkit.yml

Line 16 in d93d3e2

GO_VERSION: "1.20.5"

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jul 13, 2023

That will probably make it fail again until BuildKit is fixed with this fix?

@thaJeztah thaJeztah force-pushed the go1.20.6_with_client_fix branch 3 times, most recently from a4f2cbd to 9174b85 Compare July 13, 2023 14:39
thaJeztah added 5 commits July 14, 2023 18:56
@thaJeztah
client: TestSetHostHeader: don't use un-keyed literals
2a59188 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
client: define a "dummy" hostname to use for local connections
92975f0 
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>
@thaJeztah
pkg/plugins: use a dummy hostname for local connections
6b7705d 
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>
@thaJeztah
testutil: use dummyhost for non-tcp connections
e1db9e9 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
update go to go1.20.6
a43b951 
go1.20.6 (released 2023-07-11) includes a security fix to the net/http package,
as well as bug fixes to the compiler, cgo, the cover tool, the go command,
the runtime, and the crypto/ecdsa, go/build, go/printer, net/mail, and text/template
packages. See the Go 1.20.6 milestone on our issue tracker for details.

https://github.com/golang/go/issues?q=milestone%3AGo1.20.6+label%3ACherryPickApproved

Full diff: golang/go@go1.20.5...go1.20.6

These minor releases include 1 security fixes following the security policy:

net/http: insufficient sanitization of Host header

The HTTP/1 client did not fully validate the contents of the Host header.
A maliciously crafted Host header could inject additional headers or entire
requests. The HTTP/1 client now refuses to send requests containing an
invalid Request.Host or Request.URL.Host value.

Thanks to Bartek Nowotarski for reporting this issue.

Includes security fixes for [CVE-2023-29406 ][1] and Go issue https://go.dev/issue/60374

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

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the go1.20.6_with_client_fix branch from 9174b85 to a43b951 Compare July 14, 2023 18:38
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jul 14, 2023

This one was just for testing; closing

@thaJeztah thaJeztah closed this Jul 14, 2023
@thaJeztah thaJeztah deleted the go1.20.6_with_client_fix branch July 14, 2023 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tianon tianon Awaiting requested review from tianon tianon will be requested when the pull request is marked ready for review tianon is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@thaJeztah @crazy-max