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.

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

x/net/http2: pseudo header :protocol can not appear after regular header fields #70728

Closed
aojea opened this issue Dec 8, 2024 · 8 comments
Closed
Assignees
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@aojea
Copy link
Contributor

aojea commented Dec 8, 2024

Found during the implementation of the websockets protocol over http2 on the websocket server https://go-review.googlesource.com/c/net/+/632755

2024/12/08 14:50:29 http2: Transport encoding header ":protocol" = "websocket"
2024/12/08 14:50:29 http2: Framer 0xc0001be460: read SETTINGS flags=ACK len=0
2024/12/08 14:50:29 http2: Transport encoding header "accept-encoding" = "gzip"
2024/12/08 14:50:29 http2: Framer 0xc0001600e0: read SETTINGS flags=ACK len=0
2024/12/08 14:50:29 http2: Transport encoding header "user-agent" = "Go-http-client/2.0"
2024/12/08 14:50:29 http2: Transport received SETTINGS flags=ACK len=0
2024/12/08 14:50:29 http2: Framer 0xc0001600e0: wrote HEADERS flags=END_HEADERS stream=1 len=89
2024/12/08 14:50:29 http2: server read frame SETTINGS flags=ACK len=0
2024/12/08 14:50:29 http2: Framer 0xc0001be460: read HEADERS flags=END_HEADERS stream=1 len=89
2024/12/08 14:50:29 http2: decoded hpack field header field ":authority" = "127.0.0.1:44877"
2024/12/08 14:50:29 http2: decoded hpack field header field ":method" = "CONNECT"
2024/12/08 14:50:29 http2: decoded hpack field header field ":path" = "/echo"
2024/12/08 14:50:29 http2: decoded hpack field header field ":scheme" = "https"
2024/12/08 14:50:29 http2: decoded hpack field header field "sec-websocket-version" = "13"
2024/12/08 14:50:29 http2: decoded hpack field header field ":protocol" = "websocket"
2024/12/08 14:50:29 http2: invalid header: pseudo header field after regular
2024/12/08 14:50:29 http2: Framer 0xc0001be460: wrote RST_STREAM stream=1 len=4 ErrCode=PROTOCOL_ERROR
2024/12/08 14:50:29 http2: Framer 0xc0001600e0: read RST_STREAM stream=1 len=4 ErrCode=PROTOCOL_ERROR
2024/12/08 14:50:29 http2: Transport received RST_STREAM stream=1 len=4 ErrCode=PROTOCOL_ERROR
2024/12/08 14:50:29 http2: Transport closing idle conn 0xc0001621c0 (forSingleUse=false, maxStream=1)
2024/12/08 14:50:29 http2: Transport readFrame error on conn 0xc0001621c0: (*net.OpError) read tcp 127.0.0.1:51842->127.0.0.1:44877: use of closed network connection

/cc @neild

@cagedmantis
Copy link
Contributor

@aojea Would you be able to provide a snippet of code that could easily reproduce this condition?

@cagedmantis cagedmantis added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 9, 2024
@aojea
Copy link
Contributor Author

aojea commented Dec 9, 2024

@cagedmantis I found that using the test in https://go-review.googlesource.com/c/net/+/632755

You can check that out

go test ./websocket -v -count 1 -c
GODEBUG=http2debug=2 stress ./websocket.test -test.run TestEchoHTTP2

after a few seconds you'll find a trace in any of the logged files

grep -i pseudo /tmp/go-stress-20241208T150059-*
/tmp/go-stress-20241208T150059-1000814886:2024/12/08 15:01:07 http2: invalid header: pseudo header field after regular
/tmp/go-stress-20241208T150059-101774849:2024/12/08 15:01:14 http2: invalid header: pseudo header field after regular
/tmp/go-stress-20241208T150059-1025520657:2024/12/08 15:01:06 http2: invalid header: pseudo header field after regular
/tmp/go-stress-20241208T150059-1038053119:2024/12/08 15:01:02 http2: invalid header: pseudo header field after regular
/tmp/go-stress-20241208T150059-1041408016:2024/12/08 15:01:16 http2: invalid header: pseudo header field after regular
/tmp/go-stress-20241208T150059-1106862457:2024/12/08 15:01:21 http2: invalid header: pseudo header field after regular
/tmp/go-stress-20241208T150059-1120817889:2024/12/08 15:01:17 http2: invalid header: pseudo header field after regular

If you see here https://github.com/golang/net/blob/552d8ac903a11a9fde71a88732f5b58b6b394178/http2/server.go#L2268-L2277 the :protocol pseudo header is added as a normal header and it is implemented over a map, so listing will not guarantee the order IIUIC

rp.header = make(http.Header)
	for _, hf := range f.RegularFields() {
		rp.header.Add(sc.canonicalHeader(hf.Name), hf.Value)
	}
	if rp.authority == "" {
		rp.authority = rp.header.Get("Host")
	}
	if rp.protocol != "" {
		rp.header.Set(":protocol", rp.protocol)
	}

@cagedmantis cagedmantis added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Dec 9, 2024
@cagedmantis cagedmantis added this to the Unreleased milestone Dec 9, 2024
@cagedmantis
Copy link
Contributor

cc @tombergan

@neild neild self-assigned this Dec 9, 2024
@prattmic prattmic modified the milestones: Unreleased, Go1.24 Jan 6, 2025
@prattmic
Copy link
Member

prattmic commented Jan 6, 2025

I'm tentatively marking this as a release blocker, as #71128 points out that https://go.dev/cl/610977 is vendored into 1.24 and this sounds like a bug/regression in that CL (though I may be misunderstanding).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/641476 mentions this issue: http2: encode :protocol pseudo-header before regular headers

gopherbot pushed a commit to golang/net that referenced this issue Jan 9, 2025
HTTP/2 requires that pseudo-headers (which start with : and are
used to pass information other than the regular request headers)
be encoded before all regular headers.

The x/net/http2 Transport's extended CONNECT support is enabled by
the user setting a ":protocol" header in the Request. This header
matches the pseudo-header that will be sent on the wire.

Ensure that the :protocol pseudo-header is sent before any regular
headers.

For golang/go#70728

Change-Id: I70de7ad524ab9457d6dfb61cb3fabe3d53c6b39b
Reviewed-on: https://go-review.googlesource.com/c/net/+/641476
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Reviewed-by: Antonio Ojea <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/643256 mentions this issue: [internal-branch.go1.24-vendor] http2: encode :protocol pseudo-header before regular headers

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 18, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/642398 mentions this issue: net/http: update bundled golang.org/x/net/http2 [generated]

gopherbot pushed a commit to golang/net that referenced this issue Jan 21, 2025
… before regular headers

HTTP/2 requires that pseudo-headers (which start with : and are
used to pass information other than the regular request headers)
be encoded before all regular headers.

The x/net/http2 Transport's extended CONNECT support is enabled by
the user setting a ":protocol" header in the Request. This header
matches the pseudo-header that will be sent on the wire.

Ensure that the :protocol pseudo-header is sent before any regular
headers.

For golang/go#70728

Change-Id: I70de7ad524ab9457d6dfb61cb3fabe3d53c6b39b
Reviewed-on: https://go-review.googlesource.com/c/net/+/641476
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Reviewed-by: Antonio Ojea <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
(cherry picked from commit 445eead)
Reviewed-on: https://go-review.googlesource.com/c/net/+/643256
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants