net/http: No graceful closing of the connection after sending GOAWAY

[aimuz]

What version of Go are you using (go version)?

$ go version
go version go1.19.1 darwin/amd64

Does this issue reproduce with the latest release?

yes

https://pkg.go.dev/vuln/GO-2022-0969

https://go.dev/cl/428735

What operating system and processor architecture are you using (go env)?

go env Output

$ go env

What did you do?

go get -u golang.org/x/net

What did you expect to see?

Existing streams are processed normally after sending GOAWAY and no new streams are accepted

What did you see instead?

Existing stream is incorrectly terminated

--- FAIL: TestGOAWAYConcurrency (6.06s)
    goaway_test.go:486: failed to request "/get-with-goaway", err: failed to read response body, error: http2: server sent GOAWAY and closed the connection; LastStreamID=9, ErrCode=NO_ERROR, debug=""
    goaway_test.go:486: failed to request "/get-with-goaway", err: failed to read response body, error: http2: server sent GOAWAY and closed the connection; LastStreamID=9, ErrCode=NO_ERROR, debug=""
    goaway_test.go:486: failed to request "/get-with-goaway", err: failed request test server, err: Get "https://127.0.0.1:50125/get-with-goaway": http2: server sent GOAWAY and closed the connection; LastStreamID=9, ErrCode=NO_ERROR, debug=""
    goaway_test.go:486: failed to request "/get-with-goaway", err: failed to read response body, error: http2: server sent GOAWAY and closed the connection; LastStreamID=5, ErrCode=NO_ERROR, debug=""
    goaway_test.go:486: failed to request "/get-with-goaway", err: failed to read response body, error: http2: server sent GOAWAY and closed the connection; LastStreamID=5, ErrCode=NO_ERROR, debug=""
    goaway_test.go:486: failed to request "/get-with-goaway", err: failed request test server, err: Get "https://127.0.0.1:50125/get-with-goaway": http2: server sent GOAWAY and closed the connection; LastStreamID=5, ErrCode=NO_ERROR, debug=""
    goaway_test.go:516: in-flight watch was broken by GOAWAY frame, expect response body: hello, got:
FAIL
exit status 1
FAIL    k8s.io/apiserver/pkg/server/filters     6.899s

Related to this #54658

@neild Can you help me look at it?

This will alleviate

diff --git a/http2/server.go b/http2/server.go
index a894e69..364c25c 100644
--- a/http2/server.go
+++ b/http2/server.go
@@ -1369,7 +1369,7 @@ func (sc *serverConn) startGracefulShutdownInternal() {
 func (sc *serverConn) goAway(code ErrCode) {
        sc.serveG.check()
        if sc.inGoAway {
-               if sc.goAwayCode == ErrCodeNo {
+               if sc.goAwayCode == ErrCodeNo && code != ErrCodeProtocol {
                        sc.goAwayCode = code
                }
                return

Question was introduced by https://go.dev/cl/428735

[aimuz]

kubernetes/kubernetes#112693

Upgrading golang.org/x/net in k8s raises issues

[aimuz]

Not sure this fix will cause CVE-2022-27664 to open, can you give me more details?

[aimuz]

Related to this #32112

[neild]

This is a bug in the HTTP/2 server's handling of RST_STREAM frames: After sending a GOAWAY frame, the server should ignore frames for streams greater than the last stream identifier sent in the GOAWAY frame. We do this for HEADERS and DATA frames, but not for RST_STREAM. The HTTP/2 client is sending a RST_STREAM for the aborted streams (which it doesn't need to do, but is allowed to do), and the server is treating this as a protocol violation.

This wasn't apparent previous to https://go.dev/cl/428735, because the PROTOCOL_ERROR state was silently discarded.

I should have a fix shortly.

[gopherbot]

Change https://go.dev/cl/434909 mentions this issue: http2: discard more frames after GOAWAY

[aimuz]

I tested it with no more problems.
There are more logs like this in the logs

2022/09/27 09:44:37 http2: Transport readFrame error on conn 0xc000f4fe00: (*net.OpError) read tcp 127.0.0.1:60933->127.0.0.1:60365: use of closed network connection

[aimuz]

does the standard library also need to be repaired

[aojea]

@neild if you move the logic to skip frames based on the stream id to the processFrame() method maybe this commit golang/net@39120d0 gets obsolete and/or redundant

@@ -1459,6 +1460,14 @@ func (sc *serverConn) processFrame(f Frame) error {
                sc.sawFirstSettings = true
        }
 
+       if sc.inGoAway && f.Header().StreamID > sc.maxClientStreamID {
+               sc.vlogf("http2: server ignoring frame: %v", f.Header())
+               // Section 6.8: After sending a GOAWAY frame, the sender
+               // can discard frames for streams initiated by the
+               // receiver with identifiers higher than the identified
+               // last stream.
+               return nil
+       }
        switch f := f.(type) 

[aojea]

nevermind, I didn't see you already had a patch with the fix https://go.dev/cl/434909