From 3d58e9740b1a7124a252b06400e41eef3cdc4a7c Mon Sep 17 00:00:00 2001 From: Oleg Zaytsev Date: Tue, 29 Oct 2024 14:14:44 +0100 Subject: [PATCH 1/3] http2: improve error when server sends HTTP/1 If for some reason the server sends an HTTP/1.1 response response starting with "HTTP/1.1 " (9 bytes), http2 transport interprets that as a valid frame header for an expected payload of ~4MB, and fails with non-descriptive messages like "unexpected EOF". This could happen, for example, if ALPN is misconfigured on the server's load balancer. This change attempts to improve that feedback by noting in the error messages whenever the frame header was exactly the "HTTP/1.1 bytes". Signed-off-by: Oleg Zaytsev --- http2/frame.go | 16 ++++++++++++++++ http2/frame_test.go | 21 +++++++++++++++++++++ http2/transport_test.go | 42 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+) diff --git a/http2/frame.go b/http2/frame.go index 105c3b279c..65103b4b6e 100644 --- a/http2/frame.go +++ b/http2/frame.go @@ -225,6 +225,16 @@ var fhBytes = sync.Pool{ }, } +var invalidHTTP1LookingFrameHeader = func() FrameHeader { + fh, err := readFrameHeader(make([]byte, frameHeaderLen), strings.NewReader("HTTP/1.1 ")) + if err != nil { + panic(err) + } + return fh +}() + +func (h FrameHeader) looksLikeHTTP1Header() bool { return h == invalidHTTP1LookingFrameHeader } + // ReadFrameHeader reads 9 bytes from r and returns a FrameHeader. // Most users should use Framer.ReadFrame instead. func ReadFrameHeader(r io.Reader) (FrameHeader, error) { @@ -503,10 +513,16 @@ func (fr *Framer) ReadFrame() (Frame, error) { return nil, err } if fh.Length > fr.maxReadSize { + if fh.looksLikeHTTP1Header() { + return nil, fmt.Errorf("http2: failed reading the frame payload: %w, note that the frame header looked like an HTTP/1.1 header", err) + } return nil, ErrFrameTooLarge } payload := fr.getReadBuf(fh.Length) if _, err := io.ReadFull(fr.r, payload); err != nil { + if fh.looksLikeHTTP1Header() { + return nil, fmt.Errorf("http2: failed reading the frame payload: %w, note that the frame header looked like an HTTP/1.1 header", err) + } return nil, err } f, err := typeFrameParser(fh.Type)(fr.frameCache, fh, fr.countError, payload) diff --git a/http2/frame_test.go b/http2/frame_test.go index 86e5d4f80d..0c3ecb72fc 100644 --- a/http2/frame_test.go +++ b/http2/frame_test.go @@ -626,6 +626,27 @@ func TestReadFrameHeader(t *testing.T) { } } +func TestReadFrameHeader_FromHTTP1Header(t *testing.T) { + tests := []struct { + in string + looksLikeHTTP1 bool + }{ + // Ignore high bit: + {in: "\xff\xff\xff" + "\xff" + "\xff" + "\xff\xff\xff\xff", looksLikeHTTP1: false}, + {in: "HTTP/1.1 400 Bad Request\r\n", looksLikeHTTP1: true}, + } + for i, tt := range tests { + got, err := readFrameHeader(make([]byte, 9), strings.NewReader(tt.in)) + if err != nil { + t.Errorf("%d. readFrameHeader(%q) = %v", i, tt.in, err) + continue + } + if got.looksLikeHTTP1Header() != tt.looksLikeHTTP1 { + t.Errorf("%d. readFrameHeader(%q).looksLikeHTTP1Header = %v; want %v", i, tt.in, got.looksLikeHTTP1Header(), tt.looksLikeHTTP1) + } + } +} + func TestReadWriteFrameHeader(t *testing.T) { tests := []struct { len uint32 diff --git a/http2/transport_test.go b/http2/transport_test.go index 757a45a7ad..e4d44bac4d 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -272,6 +272,48 @@ func TestTransport(t *testing.T) { } } +func TestTransportFailureErrorForHTTP1Response(t *testing.T) { + const expectedHTTP1PayloadHint = "frame header looked like an HTTP/1.1 header" + + ts := httptest.NewServer(http.NewServeMux()) + t.Cleanup(ts.Close) + + for _, tc := range []struct { + name string + maxFrameSize uint32 + expectedErrorIs error + }{ + { + name: "with default max frame size", + maxFrameSize: 0, + }, + { + name: "with enough frame size to start reading", + maxFrameSize: invalidHTTP1LookingFrameHeader.Length + 1, + }, + } { + t.Run(tc.name, func(t *testing.T) { + tr := &Transport{ + DialTLSContext: func(ctx context.Context, network, addr string, cfg *tls.Config) (net.Conn, error) { + return net.Dial(network, addr) + }, + MaxReadFrameSize: tc.maxFrameSize, + AllowHTTP: true, + } + + req, err := http.NewRequest("GET", ts.URL, nil) + if err != nil { + t.Fatal(err) + } + + _, err = tr.RoundTrip(req) + if !strings.Contains(err.Error(), expectedHTTP1PayloadHint) { + t.Errorf("expected error to contain %q, got %v", expectedHTTP1PayloadHint, err) + } + }) + } +} + func testTransportReusesConns(t *testing.T, useClient, wantSame bool, modReq func(*http.Request)) { ts := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { io.WriteString(w, r.RemoteAddr) From 4abf41d51d9826e937e52a0d68e0976dc873a0eb Mon Sep 17 00:00:00 2001 From: Oleg Zaytsev Date: Thu, 14 Nov 2024 13:09:01 +0100 Subject: [PATCH 2/3] Create a new FrameHeader for each comparison This saves some initialization time. Signed-off-by: Oleg Zaytsev --- http2/frame.go | 15 +++++---------- http2/frame_test.go | 21 --------------------- 2 files changed, 5 insertions(+), 31 deletions(-) diff --git a/http2/frame.go b/http2/frame.go index 65103b4b6e..bdd7861f0d 100644 --- a/http2/frame.go +++ b/http2/frame.go @@ -225,15 +225,10 @@ var fhBytes = sync.Pool{ }, } -var invalidHTTP1LookingFrameHeader = func() FrameHeader { - fh, err := readFrameHeader(make([]byte, frameHeaderLen), strings.NewReader("HTTP/1.1 ")) - if err != nil { - panic(err) - } +func invalidHTTP1LookingFrameHeader() FrameHeader { + fh, _ := readFrameHeader(make([]byte, frameHeaderLen), strings.NewReader("HTTP/1.1 ")) return fh -}() - -func (h FrameHeader) looksLikeHTTP1Header() bool { return h == invalidHTTP1LookingFrameHeader } +} // ReadFrameHeader reads 9 bytes from r and returns a FrameHeader. // Most users should use Framer.ReadFrame instead. @@ -513,14 +508,14 @@ func (fr *Framer) ReadFrame() (Frame, error) { return nil, err } if fh.Length > fr.maxReadSize { - if fh.looksLikeHTTP1Header() { + if fh == invalidHTTP1LookingFrameHeader() { return nil, fmt.Errorf("http2: failed reading the frame payload: %w, note that the frame header looked like an HTTP/1.1 header", err) } return nil, ErrFrameTooLarge } payload := fr.getReadBuf(fh.Length) if _, err := io.ReadFull(fr.r, payload); err != nil { - if fh.looksLikeHTTP1Header() { + if fh == invalidHTTP1LookingFrameHeader() { return nil, fmt.Errorf("http2: failed reading the frame payload: %w, note that the frame header looked like an HTTP/1.1 header", err) } return nil, err diff --git a/http2/frame_test.go b/http2/frame_test.go index 0c3ecb72fc..86e5d4f80d 100644 --- a/http2/frame_test.go +++ b/http2/frame_test.go @@ -626,27 +626,6 @@ func TestReadFrameHeader(t *testing.T) { } } -func TestReadFrameHeader_FromHTTP1Header(t *testing.T) { - tests := []struct { - in string - looksLikeHTTP1 bool - }{ - // Ignore high bit: - {in: "\xff\xff\xff" + "\xff" + "\xff" + "\xff\xff\xff\xff", looksLikeHTTP1: false}, - {in: "HTTP/1.1 400 Bad Request\r\n", looksLikeHTTP1: true}, - } - for i, tt := range tests { - got, err := readFrameHeader(make([]byte, 9), strings.NewReader(tt.in)) - if err != nil { - t.Errorf("%d. readFrameHeader(%q) = %v", i, tt.in, err) - continue - } - if got.looksLikeHTTP1Header() != tt.looksLikeHTTP1 { - t.Errorf("%d. readFrameHeader(%q).looksLikeHTTP1Header = %v; want %v", i, tt.in, got.looksLikeHTTP1Header(), tt.looksLikeHTTP1) - } - } -} - func TestReadWriteFrameHeader(t *testing.T) { tests := []struct { len uint32 From d6e410daa3a1aa5d1c85ff99c929d27b81cf4783 Mon Sep 17 00:00:00 2001 From: Oleg Zaytsev Date: Mon, 10 Mar 2025 09:20:04 +0000 Subject: [PATCH 3/3] Fix invalidHTTP1LookingFrameHeader call in test `invalidHTTP1LookingFrameHeader` is a function, so it has to be called. Signed-off-by: Oleg Zaytsev --- http2/transport_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http2/transport_test.go b/http2/transport_test.go index e4d44bac4d..e798759e6b 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -289,7 +289,7 @@ func TestTransportFailureErrorForHTTP1Response(t *testing.T) { }, { name: "with enough frame size to start reading", - maxFrameSize: invalidHTTP1LookingFrameHeader.Length + 1, + maxFrameSize: invalidHTTP1LookingFrameHeader().Length + 1, }, } { t.Run(tc.name, func(t *testing.T) {