From 6c2259e4a5bd207a0a3defb85bf057eb0d175eb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Matczuk?= Date: Thu, 21 Nov 2024 10:51:05 +0100 Subject: [PATCH 1/8] martian/log: add helper function to easily display logs in test --- internal/martian/log/std.go | 42 +++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 internal/martian/log/std.go diff --git a/internal/martian/log/std.go b/internal/martian/log/std.go new file mode 100644 index 00000000..e5e60835 --- /dev/null +++ b/internal/martian/log/std.go @@ -0,0 +1,42 @@ +// Copyright 2022-2024 Sauce Labs Inc., all rights reserved. +// +// Copyright 2015 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package log + +import ( + "context" + stdlog "log" +) + +type testLogger struct{} + +var _ Logger = testLogger{} + +func (testLogger) Infof(_ context.Context, format string, args ...any) { + stdlog.Printf("INFO: "+format, args...) +} + +func (testLogger) Debugf(_ context.Context, format string, args ...any) { + stdlog.Printf("DEBUG: "+format, args...) +} + +func (testLogger) Errorf(_ context.Context, format string, args ...any) { + stdlog.Printf("ERROR: "+format, args...) +} + +func SetTestLogger() { + SetLogger(testLogger{}) +} From d68d04786008c818ef39aad533652199fed13c04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Matczuk?= Date: Wed, 20 Nov 2024 14:00:09 +0100 Subject: [PATCH 2/8] martian: extract isHeaderOnlySpec() from shouldChunk() --- internal/martian/flush.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/internal/martian/flush.go b/internal/martian/flush.go index c4c9429a..c8b2d71f 100644 --- a/internal/martian/flush.go +++ b/internal/martian/flush.go @@ -36,21 +36,23 @@ func shouldChunk(res *http.Response) bool { return false } - // Please read 3.3.2 and 3.3.3 of RFC 7230 for more details https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2. - if res.Request.Method == http.MethodHead { - return false - } - // The 204/304 response MUST NOT contain a - // message-body, and thus is always terminated by the first empty line - // after the header fields. - if res.StatusCode == http.StatusNoContent || res.StatusCode == http.StatusNotModified { - return false - } - if res.StatusCode < 200 { - return false - } + return !isHeaderOnlySpec(res) +} - return true +// isHeaderOnlySpec returns true iff the response should have only headers according to the HTTP spec (RFC 7230). +// +// Any response to a HEAD request and any response with a 1xx +// (Informational), 204 (No Content), or 304 (Not Modified) status +// code is always terminated by the first empty line after the +// header fields, regardless of the header fields present in the +// message, and thus cannot contain a message body. +// +// https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.3 +func isHeaderOnlySpec(res *http.Response) bool { + return res.Request.Method == http.MethodHead || + res.StatusCode/100 == 1 || + res.StatusCode == http.StatusNoContent || + res.StatusCode == http.StatusNotModified } func isTextEventStream(res *http.Response) bool { From 65f10c0583c6c1eda39163f632dddb3538a2b9dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Matczuk?= Date: Wed, 20 Nov 2024 14:01:25 +0100 Subject: [PATCH 3/8] martian: use writeHeadResponse() for all header-only response types Note that res.Body == http.NoBody is not always the case ex. when it's wrapped, or comes from http/2. --- internal/martian/proxy_conn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/martian/proxy_conn.go b/internal/martian/proxy_conn.go index 891c8f94..fa65f0d3 100644 --- a/internal/martian/proxy_conn.go +++ b/internal/martian/proxy_conn.go @@ -427,7 +427,7 @@ func (p *proxyConn) writeResponse(res *http.Response) error { switch { case req.Method == http.MethodConnect && res.StatusCode/100 == 2: err = writeConnectOKResponse(p.brw.Writer) - case req.Method == http.MethodHead && res.Body == http.NoBody: + case isHeaderOnlySpec(res): // The http package is misbehaving when writing a HEAD response. // See https://github.com/golang/go/issues/62015 for details. // This works around the issue by writing the response manually. From 0b984bdc7db5f56e5b20064952c38da3c76cf8a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Matczuk?= Date: Wed, 20 Nov 2024 14:05:08 +0100 Subject: [PATCH 4/8] martian(proxy): harden roundTrip() on header-only responses --- internal/martian/proxy.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/internal/martian/proxy.go b/internal/martian/proxy.go index 5682380a..a35b7d5f 100644 --- a/internal/martian/proxy.go +++ b/internal/martian/proxy.go @@ -353,7 +353,18 @@ func (p *Proxy) roundTrip(req *http.Request) (*http.Response, error) { return proxyutil.NewResponse(200, http.NoBody, req), nil } - return p.rt.RoundTrip(req) + res, err := p.rt.RoundTrip(req) + if err != nil { + return nil, err + } + + if isHeaderOnlySpec(res) && res.StatusCode != http.StatusSwitchingProtocols && res.Body != http.NoBody { + log.Infof(req.Context(), "unexpected body in header-only response: %d, closing body", res.StatusCode) + res.Body.Close() + res.Body = http.NoBody + } + + return res, err } func (p *Proxy) errorResponse(req *http.Request, err error) *http.Response { From 8ff58079a9775e3df9548095c184c511c4378db5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Matczuk?= Date: Wed, 20 Nov 2024 15:33:46 +0100 Subject: [PATCH 5/8] martian: add http 304 test Test ensures that after responding with status code 304 the connection is left intact. --- internal/martian/proxy_test.go | 71 ++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/internal/martian/proxy_test.go b/internal/martian/proxy_test.go index ba3bc7da..ad31a9fc 100644 --- a/internal/martian/proxy_test.go +++ b/internal/martian/proxy_test.go @@ -485,6 +485,77 @@ func TestIntegrationHTTP101SwitchingProtocols(t *testing.T) { } } +func TestIntegrationHTTP304NotModified(t *testing.T) { + t.Parallel() + + tm := martiantest.NewModifier() + h := testHelper{ + Proxy: func(p *Proxy) { + p.ReadTimeout = 200 * time.Millisecond + p.WriteTimeout = 200 * time.Millisecond + p.RequestModifier = tm + p.ResponseModifier = tm + p.AllowHTTP = true + }, + } + + sl, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("net.Listen(): got %v, want no error", err) + } + + go func() { + conn, err := sl.Accept() + if err != nil { + log.Errorf(context.TODO(), "proxy_test: failed to accept connection: %v", err) + return + } + defer conn.Close() + + log.Infof(context.TODO(), "proxy_test: accepted connection: %s", conn.RemoteAddr()) + + req, err := http.ReadRequest(bufio.NewReader(conn)) + if err != nil { + log.Errorf(context.TODO(), "proxy_test: failed to read request: %v", err) + return + } + + res := proxyutil.NewResponse(304, http.NoBody, req) + res.Header.Set("Content-Length", "13") + res.Write(conn) + log.Infof(context.TODO(), "proxy_test: sent 304 response") + + log.Infof(context.TODO(), "proxy_test: closed connection") + }() + + conn, cancel := h.proxyConn(t) + defer cancel() + defer conn.Close() + + host := sl.Addr().String() + + req, err := http.NewRequest(http.MethodGet, "http://"+host, http.NoBody) + if err != nil { + t.Fatalf("http.NewRequest(): got %v, want no error", err) + } + if err := req.WriteProxy(conn); err != nil { + t.Fatalf("req.WriteProxy(): got %v, want no error", err) + } + + res, err := http.ReadResponse(bufio.NewReader(conn), nil) + if err != nil { + t.Fatalf("http.ReadResponse(): got %v, want no error", err) + } + defer res.Body.Close() + + if got, want := res.StatusCode, 304; got != want { + t.Fatalf("res.StatusCode: got %d, want %d", got, want) + } + if _, err := conn.Read(nil); err != nil { + t.Fatalf("conn.Read(): got %v, want no error", err) + } +} + func TestIntegrationUnexpectedUpstreamFailure(t *testing.T) { t.Parallel() From 9fa911318da9e7312b6efb24dc6ca083c9bf7598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Matczuk?= Date: Thu, 21 Nov 2024 10:59:54 +0100 Subject: [PATCH 6/8] chore(martian): rename writeHeadResponse() to writeHeaderOnlyResponse() --- internal/martian/head.go | 4 ++-- internal/martian/proxy_conn.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/martian/head.go b/internal/martian/head.go index 474fe65d..ec146ff4 100644 --- a/internal/martian/head.go +++ b/internal/martian/head.go @@ -26,8 +26,8 @@ import ( "golang.org/x/exp/maps" ) -// writeHeadResponse writes the status line and header of r to w. -func writeHeadResponse(w io.Writer, res *http.Response) error { +// writeHeaderOnlyResponse writes the status line and header of r to w. +func writeHeaderOnlyResponse(w io.Writer, res *http.Response) error { // Status line text := res.Status if text == "" { diff --git a/internal/martian/proxy_conn.go b/internal/martian/proxy_conn.go index fa65f0d3..e6fad05f 100644 --- a/internal/martian/proxy_conn.go +++ b/internal/martian/proxy_conn.go @@ -431,7 +431,7 @@ func (p *proxyConn) writeResponse(res *http.Response) error { // The http package is misbehaving when writing a HEAD response. // See https://github.com/golang/go/issues/62015 for details. // This works around the issue by writing the response manually. - err = writeHeadResponse(p.brw.Writer, res) + err = writeHeaderOnlyResponse(p.brw.Writer, res) default: // Add support for Server Sent Events - relay HTTP chunks and flush after each chunk. // This is safe for events that are smaller than the buffer io.Copy uses (32KB). From 67063b42915d10cdfcd7e53e75fd6e72f858d789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Matczuk?= Date: Thu, 21 Nov 2024 11:03:06 +0100 Subject: [PATCH 7/8] chore(martian): move writeHeaderOnlyResponse() to proxy_conn.go --- internal/martian/head.go | 77 ---------------------------------- internal/martian/proxy_conn.go | 53 +++++++++++++++++++++++ 2 files changed, 53 insertions(+), 77 deletions(-) delete mode 100644 internal/martian/head.go diff --git a/internal/martian/head.go b/internal/martian/head.go deleted file mode 100644 index ec146ff4..00000000 --- a/internal/martian/head.go +++ /dev/null @@ -1,77 +0,0 @@ -// Copyright 2022-2024 Sauce Labs Inc., all rights reserved. -// -// Copyright 2015 Google Inc. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package martian - -import ( - "fmt" - "io" - "net/http" - "strconv" - "strings" - - "golang.org/x/exp/maps" -) - -// writeHeaderOnlyResponse writes the status line and header of r to w. -func writeHeaderOnlyResponse(w io.Writer, res *http.Response) error { - // Status line - text := res.Status - if text == "" { - text = http.StatusText(res.StatusCode) - if text == "" { - text = "status code " + strconv.Itoa(res.StatusCode) - } - } else { - // Just to reduce stutter, if user set res.Status to "200 OK" and StatusCode to 200. - // Not important. - text = strings.TrimPrefix(text, strconv.Itoa(res.StatusCode)+" ") - } - - if _, err := fmt.Fprintf(w, "HTTP/%d.%d %03d %s\r\n", res.ProtoMajor, res.ProtoMinor, res.StatusCode, text); err != nil { - return err - } - - // Header - if err := res.Header.Write(w); err != nil { - return err - } - - // Add Trailer header if needed - if len(res.Trailer) > 0 { - if _, err := io.WriteString(w, "Trailer: "); err != nil { - return err - } - - for i, k := range maps.Keys(res.Trailer) { - if i > 0 { - if _, err := io.WriteString(w, ", "); err != nil { - return err - } - } - if _, err := io.WriteString(w, k); err != nil { - return err - } - } - } - - // End-of-header - if _, err := io.WriteString(w, "\r\n"); err != nil { - return err - } - - return nil -} diff --git a/internal/martian/proxy_conn.go b/internal/martian/proxy_conn.go index e6fad05f..5a198f40 100644 --- a/internal/martian/proxy_conn.go +++ b/internal/martian/proxy_conn.go @@ -26,10 +26,13 @@ import ( "io" "net" "net/http" + "strconv" + "strings" "time" "github.com/saucelabs/forwarder/internal/martian/log" "github.com/saucelabs/forwarder/internal/martian/proxyutil" + "golang.org/x/exp/maps" ) type proxyConn struct { @@ -471,3 +474,53 @@ func (p *proxyConn) writeResponse(res *http.Response) error { return nil } + +// writeHeaderOnlyResponse writes the status line and header of r to w. +func writeHeaderOnlyResponse(w io.Writer, res *http.Response) error { + // Status line + text := res.Status + if text == "" { + text = http.StatusText(res.StatusCode) + if text == "" { + text = "status code " + strconv.Itoa(res.StatusCode) + } + } else { + // Just to reduce stutter, if user set res.Status to "200 OK" and StatusCode to 200. + // Not important. + text = strings.TrimPrefix(text, strconv.Itoa(res.StatusCode)+" ") + } + + if _, err := fmt.Fprintf(w, "HTTP/%d.%d %03d %s\r\n", res.ProtoMajor, res.ProtoMinor, res.StatusCode, text); err != nil { + return err + } + + // Header + if err := res.Header.Write(w); err != nil { + return err + } + + // Add Trailer header if needed + if len(res.Trailer) > 0 { + if _, err := io.WriteString(w, "Trailer: "); err != nil { + return err + } + + for i, k := range maps.Keys(res.Trailer) { + if i > 0 { + if _, err := io.WriteString(w, ", "); err != nil { + return err + } + } + if _, err := io.WriteString(w, k); err != nil { + return err + } + } + } + + // End-of-header + if _, err := io.WriteString(w, "\r\n"); err != nil { + return err + } + + return nil +} From e662d1310832fcfc9350bb77b3c233e8affed650 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Matczuk?= Date: Thu, 21 Nov 2024 12:16:40 +0100 Subject: [PATCH 8/8] martian: when handling 101 response panic on response read after uconn extraction The std http package returns the underlying connection in body of 101 response. The response body needs to be removed form the response. Make reads from the replaced body panic to ensure that it's always handled correctly. --- internal/martian/proxy.go | 9 +++++++++ internal/martian/proxy_conn.go | 3 +-- internal/martian/proxy_handler.go | 3 +-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/internal/martian/proxy.go b/internal/martian/proxy.go index a35b7d5f..efa66b18 100644 --- a/internal/martian/proxy.go +++ b/internal/martian/proxy.go @@ -18,6 +18,7 @@ import ( "context" "crypto/tls" "errors" + "io" "net" "net/http" "net/url" @@ -388,3 +389,11 @@ func upgradeType(h http.Header) string { } return h.Get("Upgrade") } + +type panicReader struct{} + +func (panicReader) Read(p []byte) (int, error) { + panic("unexpected read") +} + +var panicBody = io.NopCloser(panicReader{}) diff --git a/internal/martian/proxy_conn.go b/internal/martian/proxy_conn.go index 5a198f40..87be8c3d 100644 --- a/internal/martian/proxy_conn.go +++ b/internal/martian/proxy_conn.go @@ -259,8 +259,7 @@ func (p *proxyConn) handleUpgradeResponse(res *http.Response) error { log.Errorf(res.Request.Context(), "internal error: switching protocols response with non-writable body") return errClose } - - res.Body = nil + res.Body = panicBody if err := p.tunnel(resUpType, res, uconn); err != nil { log.Errorf(res.Request.Context(), "%s tunnel: %v", resUpType, err) diff --git a/internal/martian/proxy_handler.go b/internal/martian/proxy_handler.go index 67d3d89b..30525e5a 100644 --- a/internal/martian/proxy_handler.go +++ b/internal/martian/proxy_handler.go @@ -149,8 +149,7 @@ func (p proxyHandler) handleUpgradeResponse(rw http.ResponseWriter, req *http.Re log.Errorf(ctx, "%s tunnel: internal error: switching protocols response with non-ReadWriteCloser body", resUpType) panic(http.ErrAbortHandler) } - - res.Body = nil + res.Body = panicBody if err := p.tunnel(resUpType, rw, req, res, uconn); err != nil { log.Errorf(ctx, "%s tunnel: %v", resUpType, err)