diff --git a/internal/httpctx/context_keys.go b/internal/httpctx/context_keys.go index 615eb15e..13845ae7 100644 --- a/internal/httpctx/context_keys.go +++ b/internal/httpctx/context_keys.go @@ -32,6 +32,7 @@ const ( ContextKeyParsedAcl ContextKey = "parsed-acl" ContextKeySkipResBodyLog ContextKey = "skip-res-body-log" ContextKeyBodyReader ContextKey = "body-reader" + ContextKeyBodyStream ContextKey = "body-stream" ContextKeySkip ContextKey = "__skip" ContextKeyStack ContextKey = "stack" ContextKeyBucketOwner ContextKey = "bucket-owner" diff --git a/s3api/middlewares/body-reader.go b/s3api/middlewares/body-reader.go index 0e52cc11..e4f363bd 100644 --- a/s3api/middlewares/body-reader.go +++ b/s3api/middlewares/body-reader.go @@ -88,7 +88,7 @@ var _ ChecksumReader = &MockChecksumReader{} func wrapBodyReader(ctx fiber.Ctx, wr func(io.Reader) io.Reader) { rdr, ok := utils.ContextKeyBodyReader.Get(ctx).(io.Reader) if !ok { - rdr = ctx.Request().BodyStream() + rdr = requestBodyStream(ctx) // Override the body reader with an empty reader to prevent panics // in case of unexpected or malformed HTTP requests. if rdr == nil { diff --git a/s3api/middlewares/drain-request-body.go b/s3api/middlewares/drain-request-body.go index 25c6b02e..0c4ed240 100644 --- a/s3api/middlewares/drain-request-body.go +++ b/s3api/middlewares/drain-request-body.go @@ -22,6 +22,7 @@ import ( "github.com/gofiber/fiber/v3" "github.com/versity/versitygw/debuglogger" + "github.com/versity/versitygw/s3api/utils" ) const ( @@ -57,35 +58,41 @@ var ( // // Draining first lets the client finish its write and read the real error. It // also keeps keep-alive connections in sync: leftover body bytes would otherwise -// be parsed as the start of the next request. +// be parsed as the start of the next request, which on a connection shared by an +// upstream proxy mixes requests across tenants. +// +// It wraps the body in a bodyStreamTracker on the way in, which is what tells a +// body the handler finished from one it abandoned. Every reader in the chain +// must therefore take the body from requestBodyStream. // // Register it before every route so it wraps all of them. It runs before the // fiber ErrorHandler, which fiber invokes after the handler chain returns, so // that handler must not reset the response header the drain may have written to. func DrainRequestBody() fiber.Handler { return func(ctx fiber.Ctx) error { + var body *bodyStreamTracker + if stream := ctx.Request().BodyStream(); stream != nil { + body = &bodyStreamTracker{reader: stream} + utils.ContextKeyBodyStream.Set(ctx, body) + } + // deferred so a panic unwinding through the chain still drains - defer drainRequestBody(ctx) + defer drainRequestBody(ctx, body) + return ctx.Next() } } -func drainRequestBody(ctx fiber.Ctx) { - stream := ctx.Request().BodyStream() - if stream == nil { - // The body was either absent or already buffered in full by fasthttp. +func drainRequestBody(ctx fiber.Ctx, body *bodyStreamTracker) { + if body == nil || ctx.Request().BodyStream() == nil { + // The body was either absent, or buffered in full and released by + // fasthttp behind the chain's back, the way ctx.Body() does. return } - // fasthttp's requestStream reports EOF idempotently for a Content-Length - // body, but not for a chunked one: past the terminating chunk it goes back - // to the socket for another chunk header that will never come. Reading a - // chunked body the handler already finished would block until the deadline - // and hold the response back with it, so only Content-Length framing is - // drained. Nothing is lost for the aws-chunked uploads this exists for -- - // STREAMING-* payloads carry a Content-Length. - cLength := ctx.Request().Header.ContentLength() - if cLength <= 0 { + if errors.Is(body.end, io.EOF) { + // The handler read the body to its end: the socket holds nothing more + // of it and the connection is already in sync for the next request. return } @@ -93,8 +100,26 @@ func drainRequestBody(ctx fiber.Ctx) { if conn != nil { defer conn.SetReadDeadline(time.Time{}) } + + // The leftovers are read back through the tracker, so a drain that reaches + // the end of the body records it the same way the handler's reads would. + src := io.Reader(body) + if body.end != nil { + // The framing broke before the body ended, so what is still queued + // cannot be told apart from the start of the next request and the + // connection cannot carry one. Draining is still worth attempting: the + // connection is going away either way, and absorbing the client's + // in-flight write is what lets it read the S3 error instead of an RST. + // Only the raw socket can absorb it once the framing is gone. + ctx.Response().Header.SetConnectionClose() + if conn == nil { + return + } + src = conn + } + reader := &drainReader{ - reader: stream, + reader: src, conn: conn, deadline: time.Now().Add(drainTotalTimeout), } @@ -124,6 +149,51 @@ func drainRequestBody(ctx fiber.Ctx) { ctx.Response().Header.SetConnectionClose() } +// requestBodyStream returns the reader the request body must be read through. +// Reading ctx.Request().BodyStream() directly instead hides those reads from +// DrainRequestBody, which then cannot tell a finished body from an abandoned +// one and falls back to closing the connection. +// +// It yields the raw stream where DrainRequestBody is not registered, and nil +// when the request carries no streamed body. +func requestBodyStream(ctx fiber.Ctx) io.Reader { + if body, ok := utils.ContextKeyBodyStream.Get(ctx).(*bodyStreamTracker); ok { + return body + } + + return ctx.Request().BodyStream() +} + +// bodyStreamTracker remembers how the request body ended, so the drain can tell +// a body the handler read to its end from one it stopped partway through. +// +// fasthttp cannot be asked a second time. Its requestStream reports EOF +// idempotently for a Content-Length body, but not for a chunked one: past the +// terminating chunk it goes back to the socket for another chunk header that +// will never come, so a read of a body the handler already finished blocks +// until the deadline and holds the response back with it. The first terminal +// result is recorded here and replayed instead. +type bodyStreamTracker struct { + reader io.Reader + // end is the first error the stream ended on: nil while it still has more + // to give, io.EOF once it was read out in full, and the framing error if it + // broke before that. + end error +} + +func (t *bodyStreamTracker) Read(p []byte) (int, error) { + if t.end != nil { + return 0, t.end + } + + n, err := t.reader.Read(p) + if err != nil { + t.end = err + } + + return n, err +} + // drainReader refreshes the connection's read deadline before every read, so a // client that keeps sending is never cut off mid-drain while one that goes quiet // is dropped after drainIdleTimeout. deadline caps the whole drain regardless. diff --git a/s3api/middlewares/drain-request-body_test.go b/s3api/middlewares/drain-request-body_test.go index 293217e1..49d315a9 100644 --- a/s3api/middlewares/drain-request-body_test.go +++ b/s3api/middlewares/drain-request-body_test.go @@ -86,15 +86,7 @@ func slowClientFinishesWriting(t *testing.T, disableKeepalive bool) { } // a fully drained body leaves the connection in sync for the next request - if _, err := conn.Write(putHeaders(addr, 0)); err != nil { - t.Fatalf("write second request: %v", err) - } - resp2, err := http.ReadResponse(br, nil) - if err != nil { - t.Fatalf("read second response: %v", err) - } - defer resp2.Body.Close() - if resp2.StatusCode != http.StatusBadRequest { + if resp2 := reuseConnection(t, conn, br, addr, resp); resp2.StatusCode != http.StatusBadRequest { t.Fatalf("expected status %v on the reused connection, got %v", http.StatusBadRequest, resp2.StatusCode) } } @@ -133,9 +125,7 @@ func TestDrainRequestBody_closesConnectionWhenUnreadBodyExceedsTheLimit(t *testi } func TestDrainRequestBody_givesUpOnAClientThatStopsSending(t *testing.T) { - idle, total := drainIdleTimeout, drainTotalTimeout - drainIdleTimeout, drainTotalTimeout = 100*time.Millisecond, 250*time.Millisecond - t.Cleanup(func() { drainIdleTimeout, drainTotalTimeout = idle, total }) + shortenDrainTimeouts(t) addr := startEarlyResponder(t, true) @@ -173,7 +163,8 @@ func TestDrainRequestBody_givesUpOnAClientThatStopsSending(t *testing.T) { // A chunked body the handler read to its end must not be read again: fasthttp's // requestStream goes back to the socket for another chunk header past the -// terminating chunk, which would hold the response back until the deadline. +// terminating chunk, which would hold the response back until the deadline. It +// also has nothing left to desync the connection, so it keeps keep-alive. func TestDrainRequestBody_doesNotStallAChunkedBodyTheHandlerFinished(t *testing.T) { addr := startFullReader(t) @@ -186,13 +177,13 @@ func TestDrainRequestBody_doesNotStallAChunkedBodyTheHandlerFinished(t *testing. t.Fatalf("set deadline: %v", err) } - body := "PUT /object HTTP/1.1\r\nHost: " + addr + "\r\nTransfer-Encoding: chunked\r\n\r\n5\r\nhello\r\n0\r\n\r\n" - if _, err := conn.Write([]byte(body)); err != nil { + if _, err := conn.Write(chunkedRequest(addr, []byte("hello"))); err != nil { t.Fatalf("write request: %v", err) } start := time.Now() - resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + br := bufio.NewReader(conn) + resp, err := http.ReadResponse(br, nil) if err != nil { t.Fatalf("read response: %v", err) } @@ -204,6 +195,131 @@ func TestDrainRequestBody_doesNotStallAChunkedBodyTheHandlerFinished(t *testing. if elapsed := time.Since(start); elapsed > drainIdleTimeout { t.Fatalf("the response was held back for %v: the drain re-read a finished chunked body", elapsed) } + if resp.Close { + t.Fatal("expected the connection to stay usable after a chunked body the handler finished") + } + + if resp2 := reuseConnection(t, conn, br, addr, resp); resp2.StatusCode != http.StatusOK { + t.Fatalf("expected status %v on the reused connection, got %v", http.StatusOK, resp2.StatusCode) + } +} + +// A chunked body the handler abandoned is drained like any other. Only the read +// past its end is unsafe, and the drain never gets there on a body it finished. +func TestDrainRequestBody_drainsAnUnreadChunkedBody(t *testing.T) { + addr := startEarlyResponder(t, false) + + conn, err := net.Dial("tcp", addr) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.SetDeadline(time.Now().Add(30 * time.Second)); err != nil { + t.Fatalf("set deadline: %v", err) + } + + // leave one decoded byte unread after the handler's initial read + chunk := bytes.Repeat([]byte("a"), handlerReadBytes+1) + if _, err := conn.Write(chunkedRequest(addr, chunk)); err != nil { + t.Fatalf("write request: %v", err) + } + + br := bufio.NewReader(conn) + resp, err := http.ReadResponse(br, nil) + if err != nil { + t.Fatalf("read response: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("expected status %v, got %v", http.StatusBadRequest, resp.StatusCode) + } + if resp.Close { + t.Fatal("expected the connection to stay usable after the chunked body was drained") + } + + if resp2 := reuseConnection(t, conn, br, addr, resp); resp2.StatusCode != http.StatusBadRequest { + t.Fatalf("expected status %v on the reused connection, got %v", http.StatusBadRequest, resp2.StatusCode) + } +} + +func TestDrainRequestBody_closesConnectionWhenUnreadChunkedBodyExceedsTheLimit(t *testing.T) { + addr := startEarlyResponder(t, false) + chunk := bytes.Repeat([]byte("a"), int(maxDrainBytes)*4) + + conn, err := net.Dial("tcp", addr) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.SetDeadline(time.Now().Add(30 * time.Second)); err != nil { + t.Fatalf("set deadline: %v", err) + } + + // the write is expected to fail once the server gives up draining + go conn.Write(chunkedRequest(addr, chunk)) + + resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + if err != nil { + t.Fatalf("read response: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("expected status %v, got %v", http.StatusBadRequest, resp.StatusCode) + } + if !resp.Close { + t.Fatal("expected 'Connection: close', the undrained chunk bytes would desync the next request") + } +} + +// Broken chunk framing leaves nothing that can be decoded as body bytes, so the +// connection cannot be reused. The drain still absorbs what the client is +// writing, off the socket itself, so it reaches the S3 error instead of an RST. +func TestDrainRequestBody_drainsAndClosesOnBrokenChunkedFraming(t *testing.T) { + shortenDrainTimeouts(t) + + addr := startEarlyResponder(t, false) + // small enough to be absorbed in full, big enough that it cannot sit in the + // socket buffers while the server decides to close + garbage := bytes.Repeat([]byte("a"), 128<<10) + + conn, err := net.Dial("tcp", addr) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.SetDeadline(time.Now().Add(30 * time.Second)); err != nil { + t.Fatalf("set deadline: %v", err) + } + + // a chunk size that is not a hex number: the handler's first read fails + head := fmt.Appendf(nil, "PUT /object HTTP/1.1\r\nHost: %s\r\nTransfer-Encoding: chunked\r\n\r\nzz\r\n", addr) + if _, err := conn.Write(head); err != nil { + t.Fatalf("write request: %v", err) + } + + // dribble the rest out, the way a client keeps uploading after the gateway + // has already given up on the request + for off := 0; off < len(garbage); off += 4 << 10 { + if _, err := conn.Write(garbage[off:min(off+(4<<10), len(garbage))]); err != nil { + t.Fatalf("the server dropped the connection with %v of %v bytes sent: %v", off, len(garbage), err) + } + time.Sleep(time.Millisecond) + } + + resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + if err != nil { + t.Fatalf("read response: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("expected status %v, got %v", http.StatusBadRequest, resp.StatusCode) + } + if !resp.Close { + t.Fatal("expected 'Connection: close', broken chunk framing cannot be resynchronized") + } } // The same, for a Content-Length body: fasthttp reports EOF idempotently there, @@ -255,7 +371,7 @@ func startEarlyResponder(t *testing.T, disableKeepalive bool) string { }) app.Use("*", DrainRequestBody()) app.Put("/object", func(ctx fiber.Ctx) error { - if body := ctx.Request().BodyStream(); body != nil { + if body := requestBodyStream(ctx); body != nil { // consume a little of it, the way the chunk reader parses a chunk // header before rejecting the upload io.CopyN(io.Discard, body, handlerReadBytes) //nolint:errcheck @@ -288,10 +404,10 @@ func putHeaders(addr string, contentLength int) []byte { func startFullReader(t *testing.T) string { t.Helper() - app := fiber.New(fiber.Config{StreamRequestBody: true, DisableKeepalive: true}) + app := fiber.New(fiber.Config{StreamRequestBody: true}) app.Use("*", DrainRequestBody()) app.Put("/object", func(ctx fiber.Ctx) error { - if body := ctx.Request().BodyStream(); body != nil { + if body := requestBodyStream(ctx); body != nil { if _, err := io.Copy(io.Discard, body); err != nil { return err } @@ -301,3 +417,43 @@ func startFullReader(t *testing.T) string { return listen(t, app) } + +// chunkedRequest builds a PUT that carries body as a single chunk, the framing +// a client uses when it cannot announce a Content-Length up front. +func chunkedRequest(addr string, body []byte) []byte { + req := fmt.Appendf(nil, "PUT /object HTTP/1.1\r\nHost: %s\r\nTransfer-Encoding: chunked\r\n\r\n%x\r\n", addr, len(body)) + req = append(req, body...) + + return append(req, "\r\n0\r\n\r\n"...) +} + +// reuseConnection sends a second, bodiless request on the same connection once +// prev is off the wire. It is answered only while the connection is still in +// sync with the client. +func reuseConnection(t *testing.T, conn net.Conn, br *bufio.Reader, addr string, prev *http.Response) *http.Response { + t.Helper() + + if _, err := io.Copy(io.Discard, prev.Body); err != nil { + t.Fatalf("read response body: %v", err) + } + if _, err := conn.Write(putHeaders(addr, 0)); err != nil { + t.Fatalf("write second request: %v", err) + } + resp, err := http.ReadResponse(br, nil) + if err != nil { + t.Fatalf("read second response: %v", err) + } + t.Cleanup(func() { resp.Body.Close() }) //nolint:errcheck + + return resp +} + +// shortenDrainTimeouts keeps a test that waits the drain out from taking the +// production timeouts to finish. +func shortenDrainTimeouts(t *testing.T) { + t.Helper() + + idle, total := drainIdleTimeout, drainTotalTimeout + drainIdleTimeout, drainTotalTimeout = 100*time.Millisecond, 250*time.Millisecond + t.Cleanup(func() { drainIdleTimeout, drainTotalTimeout = idle, total }) +} diff --git a/s3api/middlewares/object-post-auth.go b/s3api/middlewares/object-post-auth.go index aabc20ef..d3b03cb1 100644 --- a/s3api/middlewares/object-post-auth.go +++ b/s3api/middlewares/object-post-auth.go @@ -72,7 +72,7 @@ func AuthorizePostObject(root RootUserConfig, iam auth.IAMService, region string return s3err.GetAPIError(s3err.ErrMalformedPOSTRequest) } - bodyRdr := ctx.Request().BodyStream() + bodyRdr := requestBodyStream(ctx) if bodyRdr == nil { bodyRdr = bytes.NewReader(ctx.BodyRaw()) } diff --git a/s3api/middlewares/public-bucket.go b/s3api/middlewares/public-bucket.go index ac39bc37..6e90bc6a 100644 --- a/s3api/middlewares/public-bucket.go +++ b/s3api/middlewares/public-bucket.go @@ -98,7 +98,7 @@ func AuthorizePublicBucketAccess(be backend.Backend, s3action string, policyPerm return err } else if utils.IsUnsignedPaylod(payloadHash) { // for UNSIGNED-PAYLOD simply store the body reader in context locals - utils.ContextKeyBodyReader.Set(ctx, ctx.Request().BodyStream()) + utils.ContextKeyBodyReader.Set(ctx, requestBodyStream(ctx)) return nil } else { // stack a hash reader to calculated the payload sha256 hash diff --git a/s3api/utils/context-keys.go b/s3api/utils/context-keys.go index 374287c6..54612b77 100644 --- a/s3api/utils/context-keys.go +++ b/s3api/utils/context-keys.go @@ -32,6 +32,7 @@ const ( ContextKeyParsedAcl = httpctx.ContextKeyParsedAcl ContextKeySkipResBodyLog = httpctx.ContextKeySkipResBodyLog ContextKeyBodyReader = httpctx.ContextKeyBodyReader + ContextKeyBodyStream = httpctx.ContextKeyBodyStream ContextKeySkip = httpctx.ContextKeySkip ContextKeyStack = httpctx.ContextKeyStack ContextKeyBucketOwner = httpctx.ContextKeyBucketOwner