From 189b99aa2b1cd8ee48efdeba6592902c5e28604e Mon Sep 17 00:00:00 2001 From: James Lamanna Date: Fri, 28 Jan 2022 22:53:28 -0800 Subject: [PATCH 1/2] http2/h2c: handle request bodies during h2c connection upgrading If a request that triggered an upgrade from HTTP/1.1 -> HTTP/2 contained a body, it would not be replayed by the server as a HTTP/2 data frame. This would result in hangs as the client would get no data back, as the request body was never actually handled. This code corrects this, and sends HTTP/2 DATA frames with the request body. As an example: Client: ``` $ curl -v --http2 -d 'POST BODY' http://localhost:5555 * Trying 127.0.0.1... * TCP_NODELAY set * Connected to localhost (127.0.0.1) port 5555 (#0) > POST / HTTP/1.1 > Host: localhost:5555 > User-Agent: curl/7.64.1 > Accept: */* > Connection: Upgrade, HTTP2-Settings > Upgrade: h2c > HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA > Content-Length: 9 > Content-Type: application/x-www-form-urlencoded > * upload completely sent off: 9 out of 9 bytes < HTTP/1.1 101 Switching Protocols < Connection: Upgrade < Upgrade: h2c * Received 101 * Using HTTP2, server supports multi-use * Connection state changed (HTTP/2 confirmed) * Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0 * Connection state changed (MAX_CONCURRENT_STREAMS == 250)! < HTTP/2 200 < content-length: 0 < date: Sat, 29 Jan 2022 06:51:05 GMT < * Connection #0 to host localhost left intact * Closing connection 0 ``` Echo server: ``` $ ./bin/h2test Listening [0.0.0.0:5555]... Request: {Method:POST URL:/ Proto:HTTP/2.0 ProtoMajor:2 ProtoMinor:0 Header:map[Accept:[*/*] Content-Length:[9] Content-Type:[application/x-www-form-urlencoded] User-Agent:[curl/7.64.1]] Body:0xc000098120 GetBody: ContentLength:9 TransferEncoding:[] Close:false Host:localhost:5555 Form:map[] PostForm:map[] MultipartForm: Trailer:map[] RemoteAddr:127.0.0.1:54540 RequestURI:/ TLS: Cancel: Response: ctx:0xc0000a0000} Received body: POST BODY ``` Fixes #38064 --- http2/h2c/h2c.go | 32 ++++++++++++++++++ http2/h2c/h2c_test.go | 76 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/http2/h2c/h2c.go b/http2/h2c/h2c.go index c0970d8467..3dba9175d1 100644 --- a/http2/h2c/h2c.go +++ b/http2/h2c/h2c.go @@ -249,6 +249,38 @@ func convertH1ReqToH2(r *http.Request) (*bytes.Buffer, []http2.Setting, error) { } } + // Any request body create as DATA frames + if r.Body != nil && r.Body != http.NoBody { + body, err := io.ReadAll(r.Body) + if err != nil { + return nil, nil, fmt.Errorf("Could not read request body: %v", err) + } + + needOneDataFrame := len(body) < maxFrameSize + err = framer.WriteData(1, + needOneDataFrame, // end stream? + body) + if err != nil { + return nil, nil, err + } + + for i := maxFrameSize; i < len(body); i += maxFrameSize { + if len(body)-i > maxFrameSize { + if err := framer.WriteData(1, + false, // end stream? + body[i:maxFrameSize]); err != nil { + return nil, nil, err + } + } else { + if err := framer.WriteData(1, + true, // end stream? + body[i:]); err != nil { + return nil, nil, err + } + } + } + } + return h2Bytes, settings, nil } diff --git a/http2/h2c/h2c_test.go b/http2/h2c/h2c_test.go index bd9461ec6e..201046e991 100644 --- a/http2/h2c/h2c_test.go +++ b/http2/h2c/h2c_test.go @@ -104,3 +104,79 @@ func TestContext(t *testing.T) { t.Fatal(err) } } + +func Test_convertH1ReqToH2_with_POST(t *testing.T) { + postBody := "Some POST Body" + + r, err := http.NewRequest("POST", "http://localhost:80", bytes.NewBufferString(postBody)) + if err != nil { + t.Fatal(err) + } + + r.Header.Set("Upgrade", "h2c") + r.Header.Set("Connection", "Upgrade, HTTP2-Settings") + r.Header.Set("HTTP2-Settings", "AAEAAEAAAAIAAAABAAMAAABkAAQBAAAAAAUAAEAA") // Some Default Settings + h2Bytes, _, err := convertH1ReqToH2(r) + + if err != nil { + t.Fatal(err) + } + + // Read off the preface + preface := []byte(http2.ClientPreface) + if h2Bytes.Len() < len(preface) { + t.Fatal("Could not read HTTP/2 ClientPreface") + } + readPreface := h2Bytes.Next(len(preface)) + if string(readPreface) != http2.ClientPreface { + t.Fatalf("Expected Preface %s but got: %s", http2.ClientPreface, string(readPreface)) + } + + framer := http2.NewFramer(nil, h2Bytes) + + // Should get a SETTINGS, HEADERS, and then DATA + expectedFrameTypes := []http2.FrameType{http2.FrameSettings, http2.FrameHeaders, http2.FrameData} + for frameNumber := 0; h2Bytes.Len() > 0; { + frame, err := framer.ReadFrame() + if err != nil { + t.Fatal(err) + } + + if frameNumber >= len(expectedFrameTypes) { + t.Errorf("Got more than %d frames, wanted only %d", len(expectedFrameTypes), len(expectedFrameTypes)) + } + + if frame.Header().Type != expectedFrameTypes[frameNumber] { + t.Errorf("Got FrameType %v, wanted %v", frame.Header().Type, expectedFrameTypes[frameNumber]) + } + + frameNumber += 1 + + switch f := frame.(type) { + case *http2.SettingsFrame: + if frameNumber != 1 { + t.Errorf("Got SETTINGS frame as frame #%d, wanted it as frame #1", frameNumber) + } + case *http2.HeadersFrame: + if frameNumber != 2 { + t.Errorf("Got HEADERS frame as frame #%d, wanted it as frame #2", frameNumber) + } + if f.FrameHeader.StreamID != 1 { + t.Fatalf("Expected StreamId 1, got %v", f.FrameHeader.StreamID) + } + case *http2.DataFrame: + if frameNumber != 3 { + t.Errorf("Got DATA frame as frame #%d, wanted it as frame #3", frameNumber) + } + if f.FrameHeader.StreamID != 1 { + t.Errorf("Got StreamID %v, wanted 1", f.FrameHeader.StreamID) + } + + body := string(f.Data()) + + if body != postBody { + t.Errorf("Got DATA body %s, wanted %s", body, postBody) + } + } + } +} From ba79222830de46c3accddb442b867c4ca37a7635 Mon Sep 17 00:00:00 2001 From: James Lamanna Date: Fri, 4 Feb 2022 18:54:47 -0800 Subject: [PATCH 2/2] Fix body reading to read by frame size Read body by chunks instead of using ReadAll() Add test case for exact multiple of frame size --- http2/h2c/h2c.go | 39 +++++++--------- http2/h2c/h2c_test.go | 103 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 117 insertions(+), 25 deletions(-) diff --git a/http2/h2c/h2c.go b/http2/h2c/h2c.go index 3dba9175d1..d2a4c12613 100644 --- a/http2/h2c/h2c.go +++ b/http2/h2c/h2c.go @@ -251,32 +251,27 @@ func convertH1ReqToH2(r *http.Request) (*bytes.Buffer, []http2.Setting, error) { // Any request body create as DATA frames if r.Body != nil && r.Body != http.NoBody { - body, err := io.ReadAll(r.Body) - if err != nil { - return nil, nil, fmt.Errorf("Could not read request body: %v", err) - } - - needOneDataFrame := len(body) < maxFrameSize - err = framer.WriteData(1, - needOneDataFrame, // end stream? - body) - if err != nil { - return nil, nil, err - } + buf := make([]byte, maxFrameSize) + for { + n, err := r.Body.Read(buf) + if err != nil && err != io.EOF { + return nil, nil, fmt.Errorf("Could not read request body: %v", err) + } - for i := maxFrameSize; i < len(body); i += maxFrameSize { - if len(body)-i > maxFrameSize { - if err := framer.WriteData(1, - false, // end stream? - body[i:maxFrameSize]); err != nil { - return nil, nil, err - } - } else { - if err := framer.WriteData(1, + if n < maxFrameSize || err == io.EOF { + err = framer.WriteData(1, true, // end stream? - body[i:]); err != nil { + buf[:n]) + if err != nil { return nil, nil, err } + break + } + + if err = framer.WriteData(1, + false, // end stream? + buf); err != nil { + return nil, nil, err } } } diff --git a/http2/h2c/h2c_test.go b/http2/h2c/h2c_test.go index 201046e991..ee8b73a8bc 100644 --- a/http2/h2c/h2c_test.go +++ b/http2/h2c/h2c_test.go @@ -9,6 +9,7 @@ import ( "bytes" "context" "crypto/tls" + "encoding/base64" "fmt" "io/ioutil" "log" @@ -105,7 +106,7 @@ func TestContext(t *testing.T) { } } -func Test_convertH1ReqToH2_with_POST(t *testing.T) { +func TestConvertH1ReqToH2WithPOST(t *testing.T) { postBody := "Some POST Body" r, err := http.NewRequest("POST", "http://localhost:80", bytes.NewBufferString(postBody)) @@ -162,14 +163,14 @@ func Test_convertH1ReqToH2_with_POST(t *testing.T) { t.Errorf("Got HEADERS frame as frame #%d, wanted it as frame #2", frameNumber) } if f.FrameHeader.StreamID != 1 { - t.Fatalf("Expected StreamId 1, got %v", f.FrameHeader.StreamID) + t.Fatalf("Got StreamID %v, wanted StreamID 1", f.FrameHeader.StreamID) } case *http2.DataFrame: if frameNumber != 3 { t.Errorf("Got DATA frame as frame #%d, wanted it as frame #3", frameNumber) } if f.FrameHeader.StreamID != 1 { - t.Errorf("Got StreamID %v, wanted 1", f.FrameHeader.StreamID) + t.Fatalf("Got StreamID %v, wanted StreamID 1", f.FrameHeader.StreamID) } body := string(f.Data()) @@ -180,3 +181,99 @@ func Test_convertH1ReqToH2_with_POST(t *testing.T) { } } } + +func TestConvertH1ReqToH2WithPOSTBodyMultipleOfFrameSize(t *testing.T) { + frameSize := 1024 + fillByte := byte(0x45) + postBody := bytes.Repeat([]byte{fillByte}, frameSize*2) + + r, err := http.NewRequest("POST", "http://localhost:80", bytes.NewBuffer(postBody)) + if err != nil { + t.Fatal(err) + } + + var settingsBuffer bytes.Buffer + settingsFramer := http2.NewFramer(&settingsBuffer, nil) + settingsFramer.WriteSettings(http2.Setting{http2.SettingMaxFrameSize, uint32(frameSize)}) + settingsEncoded := base64.URLEncoding.EncodeToString(settingsBuffer.Bytes()) + + r.Header.Set("Upgrade", "h2c") + r.Header.Set("Connection", "Upgrade, HTTP2-Settings") + r.Header.Set("HTTP2-Settings", settingsEncoded) + h2Bytes, _, err := convertH1ReqToH2(r) + + if err != nil { + t.Fatal(err) + } + + // Read off the preface + preface := []byte(http2.ClientPreface) + if h2Bytes.Len() < len(preface) { + t.Fatal("Could not read HTTP/2 ClientPreface") + } + readPreface := h2Bytes.Next(len(preface)) + if string(readPreface) != http2.ClientPreface { + t.Fatalf("Expected Preface %s but got: %s", http2.ClientPreface, string(readPreface)) + } + + framer := http2.NewFramer(nil, h2Bytes) + + // Should get a SETTINGS, HEADERS, and then DATA + expectedFrameTypes := []http2.FrameType{http2.FrameSettings, http2.FrameHeaders, http2.FrameData, http2.FrameData, http2.FrameData} + for frameNumber := 0; h2Bytes.Len() > 0; { + frame, err := framer.ReadFrame() + if err != nil { + t.Fatal(err) + } + + if frameNumber >= len(expectedFrameTypes) { + t.Errorf("Got more than %d frames, wanted only %d", len(expectedFrameTypes), len(expectedFrameTypes)) + } + + if frame.Header().Type != expectedFrameTypes[frameNumber] { + t.Errorf("Got FrameType %v, wanted %v", frame.Header().Type, expectedFrameTypes[frameNumber]) + } + + frameNumber += 1 + + switch f := frame.(type) { + case *http2.SettingsFrame: + if frameNumber != 1 { + t.Errorf("Got SETTINGS frame as frame #%d, wanted it as frame #1", frameNumber) + } + case *http2.HeadersFrame: + if frameNumber != 2 { + t.Errorf("Got HEADERS frame as frame #%d, wanted it as frame #2", frameNumber) + } + if f.FrameHeader.StreamID != 1 { + t.Fatalf("Got StreamID %v, wanted StreamID 1", f.FrameHeader.StreamID) + } + case *http2.DataFrame: + if frameNumber < 3 { + t.Errorf("Got DATA frame as frame #%d, wanted it as frame #3 or later", frameNumber) + } + if f.FrameHeader.StreamID != 1 { + t.Fatalf("Got StreamID %v, wanted StreamID 1", f.FrameHeader.StreamID) + } + + if frameNumber < len(expectedFrameTypes) && len(f.Data()) < frameSize { + t.Errorf("Expected data frame with length %d, got %d", frameSize, len(f.Data())) + } + + if frameNumber == len(expectedFrameTypes) && len(f.Data()) != 0 { + t.Errorf("Got non-empty DATA frame as last frame, expected it to be empty") + } + + if frameNumber == len(expectedFrameTypes) && (f.FrameHeader.Flags&http2.FlagHeadersEndStream) == 0 { + t.Errorf("Got last DATA frame with end stream not set, expected end stream set") + } + + for _, b := range f.Data() { + if b != fillByte { + t.Errorf("Got data byte %d, wanted %d", b, fillByte) + break + } + } + } + } +}