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

Jump to bottom

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

Closed
aimuz opened this issue Sep 24, 2022 · 9 comments
Closed

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

aimuz opened this issue Sep 24, 2022 · 9 comments
Assignees
@neild
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@aimuz
Copy link
Contributor

aimuz commented Sep 24, 2022 •
edited

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
Copy link
Contributor Author

aimuz commented Sep 24, 2022

kubernetes/kubernetes#112693

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

@aimuz
Copy link
Contributor Author

aimuz commented Sep 24, 2022

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

@aimuz aimuz mentioned this issue Sep 24, 2022
Closed
@aimuz
Copy link
Contributor Author

aimuz commented Sep 26, 2022

Related to this #32112

@aimuz aimuz changed the title net/http: The server closed the connection by error after sending GOAWAY net/http: No graceful closing of the connection after sending GOAWAY Sep 26, 2022
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 26, 2022
@dmitshur dmitshur added this to the Backlog milestone Sep 26, 2022
@neild
Copy link
Contributor

neild commented Sep 26, 2022

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.

@neild neild self-assigned this Sep 26, 2022
@neild neild added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 26, 2022
@gopherbot
Copy link

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

@aimuz
Copy link
Contributor Author

aimuz commented Sep 27, 2022

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
Copy link
Contributor Author

aimuz commented Sep 27, 2022

does the standard library also need to be repaired

This was referenced Sep 28, 2022
Closed
Merged
@aojea
Copy link
Contributor

aojea commented Sep 28, 2022 •
edited

@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
Copy link
Contributor

aojea commented Sep 30, 2022

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

@liggitt liggitt mentioned this issue Oct 26, 2022
Merged
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @gopherbot
http2: discard more frames after GOAWAY
05c9dea 
After sending a GOAWAY with NO_ERROR, we should discard all frames
for streams with larger identifiers than the last stream identifier
in the GOAWAY frame. We weren't discarding RST_STREAM frames, which
could cause us to incorrectly detect a protocol error when handling
a RST_STREAM for a discarded stream.

Hoist post-GOAWAY frame discarding higher in the loop rather than
handling it on a per-frame-type basis.

We are also supposed to count discarded DATA frames against
connection-level flow control, possibly sending WINDOW_UPDATE
messages to return the flow control. We weren't doing this;
this is now fixed.

Fixes golang/go#55846

Change-Id: I7603a529c00b8637e648eee9cc4608fb5fa5199b
Reviewed-on: https://go-review.googlesource.com/c/net/+/434909
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: LI ZHEN <mr.imuz@gmail.com>
Reviewed-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>
@golang golang locked and limited conversation to collaborators Oct 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@neild neild

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

net/http: the server closed the connection by error after sending GOAWAY aimuz/net
5 participants
@neild @dmitshur @aojea @gopherbot @aimuz