diff --git a/README.md b/README.md index c8d6fdb7..19ddf401 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,12 @@ TAG supports **path-style** S3 access only. Virtual-hosted style requests are no When configuring S3 clients, ensure path-style addressing is enabled. See [docs/usage.md](docs/usage.md) for SDK-specific configuration. +On signing-mode PutObject requests, TAG checks each AWS data-frame header against the remaining `X-Amz-Decoded-Content-Length` before yielding its bytes. + +If an upstream response arrives before the request-body writer reaches later frames, TAG waits for that writer to stop and checks any remaining parseable frames before relaying success. The validation read uses the existing upstream request timeout. A timed-out check returns an error, not a successful upload. + +If TAG has received an upstream 2xx before a later frame exceeds the declared length, it withholds that response and does not warm from the partial body. The ordinary proxy cache path invalidates the key again when caching is enabled. Tiered mode removes only the pre-forward metadata version, preserving a newer local write. Earlier valid frames may already have reached upstream. See [AWS Chunked Transfer Encoding](docs/s3-compatibility.md#aws-chunked-transfer-encoding) for details. + ### Response Headers | Header | Description | diff --git a/docs/s3-compatibility.md b/docs/s3-compatibility.md index 4ec6c94d..32e3399d 100644 --- a/docs/s3-compatibility.md +++ b/docs/s3-compatibility.md @@ -63,7 +63,11 @@ TAG supports AWS chunked transfer encoding (streaming SigV4), used by tools like In **transparent proxy mode** (default), chunked uploads are forwarded as-is since the original signatures remain valid (the Host header is not changed). -In **signing mode**, when a client sends a PUT with `X-Amz-Content-Sha256: STREAMING-AWS4-HMAC-SHA256-PAYLOAD`, the body is wrapped in an AWS-proprietary chunked format where each chunk includes a signature chained from the request-level seed signature. Because TAG re-signs requests with the upstream Tigris hostname (changing the `Host` header), these per-chunk signatures become invalid. TAG handles this by decoding the chunked body on-the-fly — stripping the chunk framing and signatures — and forwarding the raw payload as `UNSIGNED-PAYLOAD`. The decoded content length is read from the `X-Amz-Decoded-Content-Length` header. This is a streaming operation with no buffering of the full object. +In **signing mode**, when a client sends a PUT with `X-Amz-Content-Sha256: STREAMING-AWS4-HMAC-SHA256-PAYLOAD`, the body is wrapped in an AWS-proprietary chunked format where each chunk includes a signature chained from the request-level seed signature. Because TAG re-signs requests with the upstream Tigris hostname (changing the `Host` header), these per-chunk signatures become invalid. TAG handles this by decoding the chunked body on-the-fly — stripping the chunk framing and signatures — and forwarding the raw payload as `UNSIGNED-PAYLOAD`. The decoded content length is read from the `X-Amz-Decoded-Content-Length` header. + +The decoder checks each data-frame header against the remaining decoded length before yielding its payload, and rejects a frame that exceeds that extent. If the upstream responds before the request-body writer reaches later frames, TAG waits for that writer to stop and checks any remaining parseable frames before relaying success. The validation read is bounded by the existing upstream request timeout. If it times out, TAG does not relay the upstream success. + +If TAG has received an upstream 2xx before a later frame exceeds the declared length, it withholds that response and does not warm from the partial body. The ordinary proxy cache path invalidates the key again when caching is enabled. Tiered mode removes only the pre-forward metadata version, preserving a newer local write. Earlier valid frames may already have been streamed upstream. Upstream forwarding remains streaming. When warm-on-write and caching select the write-through tee for an eligible small PUT, TAG also retains a bounded copy for cache population. ## Caching Behavior diff --git a/docs/tiered-mode.md b/docs/tiered-mode.md index b2d2304c..16a4de96 100644 --- a/docs/tiered-mode.md +++ b/docs/tiered-mode.md @@ -26,6 +26,11 @@ TAG stores a metadata marker locally. HEAD answers from the marker; GET forwards for the body — the mode's only body traffic. DELETE forwards and drops the marker. +For a signing-forwarded AWS-chunked object PUT with positive decoded length, TAG +requires a readable pre-forward metadata snapshot. If that lookup fails, TAG +returns an error before forwarding so a later frame-validation error can remove +only the displaced version without deleting a newer local write. + **Cross-tier overwrites clean up the displaced version.** A small write over an upstream-tier object deletes the upstream copy asynchronously (best-effort — a failure leaves an orphan for the upstream bucket's own expiry to collect). diff --git a/handlers/aws_chunked_early_response_test.go b/handlers/aws_chunked_early_response_test.go new file mode 100644 index 00000000..d40c534b --- /dev/null +++ b/handlers/aws_chunked_early_response_test.go @@ -0,0 +1,281 @@ +package handlers + +import ( + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + cacheclient "github.com/tigrisdata/ocache/client" + "github.com/tigrisdata/tag/auth" + "github.com/tigrisdata/tag/cache" + "github.com/tigrisdata/tag/config" + "github.com/tigrisdata/tag/proxy" +) + +type routedBodyReadGate struct { + io.ReadCloser + stopAfter int64 + read int64 + blocked chan struct{} + release <-chan struct{} + once sync.Once +} + +func (r *routedBodyReadGate) Read(p []byte) (int, error) { + if len(p) == 0 { + return 0, nil + } + if r.read >= r.stopAfter { + r.once.Do(func() { close(r.blocked) }) + <-r.release + } else if int64(len(p)) > r.stopAfter-r.read { + p = p[:r.stopAfter-r.read] + } + n, err := r.ReadCloser.Read(p) + r.read += int64(n) + return n, err +} + +type routedCommitWriter struct { + http.ResponseWriter + committed chan int + once sync.Once +} + +func (w *routedCommitWriter) WriteHeader(status int) { + w.once.Do(func() { w.committed <- status }) + w.ResponseWriter.WriteHeader(status) +} + +func (w *routedCommitWriter) Write(p []byte) (int, error) { + w.once.Do(func() { w.committed <- http.StatusOK }) + return w.ResponseWriter.Write(p) +} + +func (w *routedCommitWriter) Flush() { + if flusher, ok := w.ResponseWriter.(http.Flusher); ok { + flusher.Flush() + } +} + +func (w *routedCommitWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } + +func TestRoutedAWSChunkedExtentWaitsForEarlyUpstreamResponse(t *testing.T) { + for _, tc := range []struct { + name string + overrun bool + warmOnWrite bool + firstChunkSize int + }{ + {name: "warm-off overrun", overrun: true, firstChunkSize: 1 << 20}, + {name: "warm-off valid control", firstChunkSize: 1 << 20}, + {name: "warm-on tee overrun", overrun: true, warmOnWrite: true, firstChunkSize: 8 * 1024}, + {name: "warm-on tee valid control", warmOnWrite: true, firstChunkSize: 8 * 1024}, + } { + t.Run(tc.name, func(t *testing.T) { + runRoutedEarlyChunkResponse(t, tc.overrun, tc.warmOnWrite, tc.firstChunkSize) + }) + } +} + +func runRoutedEarlyChunkResponse(t *testing.T, overrun, warmOnWrite bool, firstChunkSize int) { + chunkHeader := fmt.Sprintf("%x\r\n", firstChunkSize) + secondChunk := "4\r\nPASS\r\n" + if overrun { + secondChunk = "5\r\nFAIL!\r\n" + } + wireBody := chunkHeader + strings.Repeat("A", firstChunkSize) + "\r\n" + secondChunk + "0\r\n\r\n" + decodedLength := int64(firstChunkSize + 4) + + bodyBlocked := make(chan struct{}) + releaseBody := make(chan struct{}) + var releaseBodyOnce sync.Once + defer releaseBodyOnce.Do(func() { close(releaseBody) }) + + prefixRead := make(chan routedChunkExtentObservation, 1) + upstreamMethods := make(chan string, 8) + allowUpstreamResponse := make(chan struct{}) + var allowResponseOnce sync.Once + defer allowResponseOnce.Do(func() { close(allowUpstreamResponse) }) + upstreamResponded := make(chan struct{}) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamMethods <- r.Method + if r.Method == http.MethodHead { + w.Header().Set("Cache-Control", "no-store") + w.Header().Set("Content-Length", fmt.Sprint(decodedLength)) + w.Header().Set("ETag", `"early-etag"`) + w.WriteHeader(http.StatusOK) + return + } + if r.Method != http.MethodPut { + w.WriteHeader(http.StatusNotFound) + return + } + var prefix [1]byte + n, readErr := io.ReadFull(r.Body, prefix[:]) + prefixRead <- routedChunkExtentObservation{ + body: append([]byte(nil), prefix[:n]...), + bodyReadErr: readErr, + contentLength: r.ContentLength, + } + <-allowUpstreamResponse + w.Header().Set("Connection", "close") + w.Header().Set("ETag", `"early-etag"`) + w.WriteHeader(http.StatusOK) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + _, _ = io.WriteString(w, "stored") + close(upstreamResponded) + })) + t.Cleanup(upstream.Close) + + cfg := config.NewDefault() + cfg.Mode = config.ModeSigning + cfg.Upstream.Endpoint = upstream.URL + cfg.Upstream.Region = routedChunkExtentRegion + cfg.Cache.WarmOnWrite = warmOnWrite + credentials := auth.NewCredentialStore() + credentials.AddCredential(routedChunkExtentAccessKey, routedChunkExtentSecretKey) + forwarder := proxy.NewForwarder(credentials, cfg.Upstream.Endpoint, cfg.Upstream.Region, 1, nil, nil) + gatewayCache := cache.NewDisabledCache() + if warmOnWrite { + gatewayCache = cache.NewCacheWithClient(cacheclient.NewMemoryCache(), &cfg.Cache) + } + service := proxy.NewService(forwarder, gatewayCache, cfg) + router := NewServer(service, "127.0.0.1", 0, false, 0).Router() + committed := make(chan int, 1) + gateway := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.Body = &routedBodyReadGate{ + ReadCloser: r.Body, + stopAfter: int64(len(chunkHeader) + firstChunkSize/2), + blocked: bodyBlocked, + release: releaseBody, + } + router.ServeHTTP(&routedCommitWriter{ResponseWriter: w, committed: committed}, r) + })) + t.Cleanup(gateway.Close) + client := &http.Client{Timeout: 5 * time.Second} + t.Cleanup(client.CloseIdleConnections) + + headers := make(http.Header) + headers.Set("Content-Encoding", "aws-chunked") + headers.Set("X-Amz-Content-Sha256", routedUnsignedChunkHash) + headers.Set("X-Amz-Decoded-Content-Length", fmt.Sprint(decodedLength)) + request, err := auth.NewRequestSigner(gateway.URL, routedChunkExtentRegion).SignRequest( + context.Background(), + http.MethodPut, + "/bucket/object", + strings.NewReader(wireBody), + routedUnsignedChunkHash, + routedChunkExtentAccessKey, + routedChunkExtentSecretKey, + headers, + ) + if err != nil { + t.Fatal(err) + } + if request.ContentLength != int64(len(wireBody)) { + t.Fatalf("incoming Content-Length = %d, want complete wire length %d", request.ContentLength, len(wireBody)) + } + if _, err := auth.NewRequestValidator(credentials).ValidateRequest(request); err != nil { + t.Fatalf("incoming SigV4 request did not validate: %v", err) + } + responseDone := make(chan routedChunkExtentResponse, 1) + go func() { + response, err := client.Do(request) + if err != nil { + responseDone <- routedChunkExtentResponse{err: err} + return + } + defer response.Body.Close() + body, err := io.ReadAll(response.Body) + responseDone <- routedChunkExtentResponse{status: response.StatusCode, body: body, err: err} + }() + + // Do not wait for the client-side WroteRequest callback here. The gateway + // intentionally stops reading the body at this gate, so a large request can + // fill TCP buffers and prevent that callback until the gate is released. + select { + case <-bodyBlocked: + case <-time.After(3 * time.Second): + t.Fatal("gateway did not reach the controlled body-read gate") + } + select { + case observation := <-prefixRead: + if observation.bodyReadErr != nil || string(observation.body) != "A" { + t.Fatalf("upstream prefix observation = body %q, err %v; want one valid-frame byte", observation.body, observation.bodyReadErr) + } + if observation.contentLength != decodedLength { + t.Fatalf("upstream content length = %d, want %d", observation.contentLength, decodedLength) + } + case <-time.After(3 * time.Second): + t.Fatal("upstream did not read a prefix of the valid first frame") + } + + allowResponseOnce.Do(func() { close(allowUpstreamResponse) }) + select { + case <-upstreamResponded: + case <-time.After(3 * time.Second): + t.Fatal("upstream did not send its early 2xx response") + } + + var committedEarly bool + select { + case status := <-committed: + committedEarly = status >= http.StatusOK && status < http.StatusMultipleChoices + case <-time.After(250 * time.Millisecond): + } + releaseBodyOnce.Do(func() { close(releaseBody) }) + + select { + case response := <-responseDone: + if response.err != nil { + t.Fatalf("reading routed response: %v", response.err) + } + if overrun { + if committedEarly || (response.status >= http.StatusOK && response.status < http.StatusMultipleChoices) { + t.Fatalf("early upstream response made the overrun PUT successful: warm-on-write=%t early-commit=%t final-status=%d body=%q", warmOnWrite, committedEarly, response.status, response.body) + } + if warmOnWrite { + methods := make([]string, 0, 2) + for { + select { + case method := <-upstreamMethods: + methods = append(methods, method) + default: + if len(methods) != 1 || methods[0] != http.MethodPut { + t.Fatalf("overrun triggered unexpected upstream/cache requests: %v", methods) + } + return + } + } + } + } else if response.status < http.StatusOK || response.status >= http.StatusMultipleChoices { + t.Fatalf("early upstream response for a valid body returned status %d, body=%q", response.status, response.body) + } else if warmOnWrite { + headObserved := false + timer := time.NewTimer(3 * time.Second) + defer timer.Stop() + for !headObserved { + select { + case method := <-upstreamMethods: + headObserved = method == http.MethodHead + if method != http.MethodPut && method != http.MethodHead { + t.Fatalf("valid early write-through sent unexpected upstream method %q", method) + } + case <-timer.C: + t.Fatal("valid early write-through did not perform its authoritative HEAD") + } + } + } + case <-time.After(5 * time.Second): + t.Fatal("gateway did not finish the early-response upload") + } +} diff --git a/handlers/aws_chunked_extent_test.go b/handlers/aws_chunked_extent_test.go new file mode 100644 index 00000000..303e7722 --- /dev/null +++ b/handlers/aws_chunked_extent_test.go @@ -0,0 +1,285 @@ +package handlers + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/tigrisdata/tag/auth" + "github.com/tigrisdata/tag/cache" + "github.com/tigrisdata/tag/config" + "github.com/tigrisdata/tag/proxy" +) + +const ( + routedChunkExtentAccessKey = "AKIAIOSFODNN7EXAMPLE" + routedChunkExtentSecretKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + routedChunkExtentRegion = "us-east-1" + + routedSignedChunkHash = "STREAMING-AWS4-HMAC-SHA256-PAYLOAD" + routedUnsignedChunkHash = "STREAMING-UNSIGNED-PAYLOAD-TRAILER" +) + +type routedChunkExtentObservation struct { + body []byte + bodyReadErr error + contentLength int64 +} + +type routedChunkExtentFixture struct { + gateway *httptest.Server + client *http.Client + upstreamStarted atomic.Int64 + upstreamResults chan routedChunkExtentObservation +} + +func newRoutedChunkExtentFixture(tb testing.TB) *routedChunkExtentFixture { + tb.Helper() + + fixture := &routedChunkExtentFixture{ + upstreamResults: make(chan routedChunkExtentObservation, 8), + } + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fixture.upstreamStarted.Add(1) + body, err := io.ReadAll(r.Body) + fixture.upstreamResults <- routedChunkExtentObservation{ + body: body, + bodyReadErr: err, + contentLength: r.ContentLength, + } + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, "stored") + })) + tb.Cleanup(upstream.Close) + + cfg := config.NewDefault() + cfg.Mode = config.ModeSigning + cfg.Upstream.Endpoint = upstream.URL + cfg.Upstream.Region = routedChunkExtentRegion + cfg.Cache.WarmOnWrite = false + credentials := auth.NewCredentialStore() + credentials.AddCredential(routedChunkExtentAccessKey, routedChunkExtentSecretKey) + forwarder := proxy.NewForwarder( + credentials, + cfg.Upstream.Endpoint, + cfg.Upstream.Region, + 1, + nil, + nil, + ) + service := proxy.NewService(forwarder, cache.NewDisabledCache(), cfg) + fixture.gateway = httptest.NewServer(NewServer(service, "127.0.0.1", 0, false, 0).Router()) + tb.Cleanup(fixture.gateway.Close) + + fixture.client = &http.Client{Timeout: 5 * time.Second} + tb.Cleanup(fixture.client.CloseIdleConnections) + return fixture +} + +type routedChunkExtentResponse struct { + status int + body []byte + err error +} + +func (f *routedChunkExtentFixture) put(tb testing.TB, hash, decodedLength, wireBody string, includeDecodedLength bool) routedChunkExtentResponse { + tb.Helper() + + headers := make(http.Header) + headers.Set("Content-Encoding", "aws-chunked") + headers.Set("X-Amz-Content-Sha256", hash) + if includeDecodedLength { + headers.Set("X-Amz-Decoded-Content-Length", decodedLength) + } + request, err := auth.NewRequestSigner(f.gateway.URL, routedChunkExtentRegion).SignRequest( + context.Background(), + http.MethodPut, + "/bucket/object", + strings.NewReader(wireBody), + hash, + routedChunkExtentAccessKey, + routedChunkExtentSecretKey, + headers, + ) + if err != nil { + return routedChunkExtentResponse{err: err} + } + + response, err := f.client.Do(request) + if err != nil { + return routedChunkExtentResponse{err: err} + } + defer response.Body.Close() + body, err := io.ReadAll(response.Body) + return routedChunkExtentResponse{status: response.StatusCode, body: body, err: err} +} + +func (f *routedChunkExtentFixture) awaitUpstream(t *testing.T, startedBefore int64, timeout time.Duration) (routedChunkExtentObservation, bool) { + t.Helper() + + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case result := <-f.upstreamResults: + return result, true + case <-timer.C: + if f.upstreamStarted.Load() != startedBefore { + t.Fatalf("upstream request started but did not finish within %s", timeout) + } + return routedChunkExtentObservation{}, false + } +} + +func TestRoutedAWSChunkedDecodedExtent(t *testing.T) { + fixture := newRoutedChunkExtentFixture(t) + + overruns := []struct { + name string + hash string + decodedLength string + decodedLengthValue int64 + wireBody string + allowedForwardedBody []byte + }{ + { + name: "signed single frame exceeds positive extent", + hash: routedSignedChunkHash, + decodedLength: "3", + decodedLengthValue: 3, + wireBody: "8;chunk-signature=sig\r\nabc", + }, + { + name: "signed complete frame exceeds positive extent", + hash: routedSignedChunkHash, + decodedLength: "3", + decodedLengthValue: 3, + wireBody: "8;chunk-signature=sig\r\nabcdefgh\r\n0;chunk-signature=end\r\n\r\n", + allowedForwardedBody: nil, + }, + { + name: "unsigned later frame exceeds remaining extent", + hash: routedUnsignedChunkHash, + decodedLength: "8", + decodedLengthValue: 8, + wireBody: "4\r\nkeep\r\n5\r\nFAIL", + allowedForwardedBody: []byte("keep"), + }, + { + name: "unsigned complete later frame exceeds remaining extent", + hash: routedUnsignedChunkHash, + decodedLength: "8", + decodedLengthValue: 8, + wireBody: "4\r\nkeep\r\n5\r\nFAIL!\r\n0\r\n\r\n", + allowedForwardedBody: []byte("keep"), + }, + { + name: "zero extent rejects data frame", + hash: routedSignedChunkHash, + decodedLength: "0", + decodedLengthValue: 0, + wireBody: "1;chunk-signature=sig\r\nz\r\n0;chunk-signature=end\r\n\r\n", + }, + } + for _, test := range overruns { + t.Run(test.name, func(t *testing.T) { + startedBefore := fixture.upstreamStarted.Load() + response := fixture.put(t, test.hash, test.decodedLength, test.wireBody, true) + successful := response.err == nil && response.status >= http.StatusOK && response.status < http.StatusMultipleChoices + + upstream, observed := fixture.awaitUpstream(t, startedBefore, 250*time.Millisecond) + if successful { + t.Errorf("overlong AWS chunk was reported successful: status=%d body=%q", response.status, response.body) + } + if !observed { + t.Logf("client status=%d client error=%v; upstream observed=false", response.status, response.err) + return + } + complete := upstream.bodyReadErr == nil + t.Logf("client status=%d client error=%v; upstream content-length=%d body=%q complete=%t read-error=%v", response.status, response.err, upstream.contentLength, upstream.body, complete, upstream.bodyReadErr) + if upstream.contentLength != test.decodedLengthValue { + t.Errorf("upstream content length = %d, want declared decoded length %d", upstream.contentLength, test.decodedLengthValue) + } + if !bytes.HasPrefix(test.allowedForwardedBody, upstream.body) { + t.Errorf("upstream received bytes from the offending frame: body=%q allowed-prefix=%q", upstream.body, test.allowedForwardedBody) + } + }) + } + + valid := []struct { + name string + hash string + decodedLength string + wireBody string + wantBody string + }{ + { + name: "signed equal-size frame", + hash: routedSignedChunkHash, + decodedLength: "3", + wireBody: "3;chunk-signature=sig\r\nabc\r\n0;chunk-signature=end\r\n\r\n", + wantBody: "abc", + }, + { + name: "unsigned smaller multi-frame total", + hash: routedUnsignedChunkHash, + decodedLength: "5", + wireBody: "2\r\nab\r\n3\r\ncde\r\n0\r\n\r\n", + wantBody: "abcde", + }, + { + name: "signed empty upload", + hash: routedSignedChunkHash, + decodedLength: "0", + wireBody: "0;chunk-signature=end\r\n\r\n", + }, + { + name: "unsigned empty upload", + hash: routedUnsignedChunkHash, + decodedLength: "0", + wireBody: "0\r\n\r\n", + }, + } + for _, test := range valid { + t.Run(test.name, func(t *testing.T) { + startedBefore := fixture.upstreamStarted.Load() + response := fixture.put(t, test.hash, test.decodedLength, test.wireBody, true) + if response.err != nil { + t.Fatalf("PUT failed: %v", response.err) + } + if response.status != http.StatusOK { + t.Fatalf("PUT status = %d, want %d; body=%q", response.status, http.StatusOK, response.body) + } + + upstream, observed := fixture.awaitUpstream(t, startedBefore, 3*time.Second) + if !observed { + t.Fatal("upstream did not receive the valid PUT") + } + if upstream.bodyReadErr != nil { + t.Fatalf("upstream body read failed: %v", upstream.bodyReadErr) + } + if got := string(upstream.body); got != test.wantBody { + t.Fatalf("upstream body = %q, want %q", got, test.wantBody) + } + }) + } + + t.Run("missing decoded length is rejected before forwarding", func(t *testing.T) { + startedBefore := fixture.upstreamStarted.Load() + response := fixture.put(t, routedSignedChunkHash, "", "3;chunk-signature=sig\r\nabc\r\n0;chunk-signature=end\r\n\r\n", false) + if response.err != nil { + t.Fatalf("PUT failed: %v", response.err) + } + if response.status != http.StatusLengthRequired { + t.Fatalf("PUT status = %d, want %d; body=%q", response.status, http.StatusLengthRequired, response.body) + } + if got := fixture.upstreamStarted.Load(); got != startedBefore { + t.Fatalf("missing decoded length reached upstream: requests=%d before=%d", got, startedBefore) + } + }) +} diff --git a/handlers/response_tracker_test.go b/handlers/response_tracker_test.go new file mode 100644 index 00000000..977d4294 --- /dev/null +++ b/handlers/response_tracker_test.go @@ -0,0 +1,30 @@ +package handlers + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" +) + +type responseTrackerDeadlineWriter struct { + *httptest.ResponseRecorder + readDeadline time.Time +} + +func (w *responseTrackerDeadlineWriter) SetReadDeadline(deadline time.Time) error { + w.readDeadline = deadline + return nil +} + +func TestResponseTrackerUnwrapsReadDeadline(t *testing.T) { + writer := &responseTrackerDeadlineWriter{ResponseRecorder: httptest.NewRecorder()} + deadline := time.Now().Add(time.Second) + controller := http.NewResponseController(&responseTracker{ResponseWriter: writer}) + if err := controller.SetReadDeadline(deadline); err != nil { + t.Fatalf("set request read deadline through responseTracker: %v", err) + } + if !writer.readDeadline.Equal(deadline) { + t.Fatalf("read deadline = %v, want %v", writer.readDeadline, deadline) + } +} diff --git a/handlers/server.go b/handlers/server.go index 8c2e8436..78d6a0ba 100644 --- a/handlers/server.go +++ b/handlers/server.go @@ -288,6 +288,8 @@ type responseTracker struct { committed bool } +func (rt *responseTracker) Unwrap() http.ResponseWriter { return rt.ResponseWriter } + func (rt *responseTracker) WriteHeader(code int) { rt.committed = true rt.ResponseWriter.WriteHeader(code) diff --git a/proxy/chunked_reader.go b/proxy/chunked_reader.go index fc6ddf45..dbe74980 100644 --- a/proxy/chunked_reader.go +++ b/proxy/chunked_reader.go @@ -58,11 +58,31 @@ func IsStreamingPayload(bodyHash string) bool { // Chunk signatures and trailing checksums are not validated (the // request-level signature was already verified by the auth validator). type awsChunkedReader struct { - reader *bufio.Reader - remaining int - done bool + reader *bufio.Reader + remaining int + decodedRemaining int64 // declared decoded bytes not yet assigned to a data chunk; -1 is unknown + done bool } +type awsChunkedReadCloser struct { + reader *awsChunkedReader +} + +func (body *awsChunkedReadCloser) Read(p []byte) (int, error) { + return body.reader.Read(p) +} + +// Close keeps the no-op ownership semantics of the previous io.NopCloser wrapper. +func (*awsChunkedReadCloser) Close() error { return nil } + +// validateRemainingDecodedExtent lets the forwarder inspect unread frames after +// the transport writer stops without passing validation-only bytes through a tee. +func (body *awsChunkedReadCloser) validateRemainingDecodedExtent() error { + return body.reader.validateRemainingDecodedExtent() +} + +var errAWSChunkExceedsDecodedLength = errors.New("AWS chunk exceeds remaining decoded content length") + // awsChunkedReaderBufSize is the decoder's internal bufio buffer. Named so // budget accounting (origin-less PUT admission) can reserve exactly what the // decoder allocates. @@ -70,7 +90,8 @@ const awsChunkedReaderBufSize = 64 * 1024 func newAWSChunkedReader(r io.Reader) *awsChunkedReader { return &awsChunkedReader{ - reader: bufio.NewReaderSize(r, awsChunkedReaderBufSize), + reader: bufio.NewReaderSize(r, awsChunkedReaderBufSize), + decodedRemaining: -1, } } @@ -150,10 +171,46 @@ func (r *awsChunkedReader) readChunkHeader() error { return nil } + if r.decodedRemaining >= 0 { + if size > r.decodedRemaining { + return fmt.Errorf("%w: chunk size %d, remaining decoded content length %d", errAWSChunkExceedsDecodedLength, size, r.decodedRemaining) + } + r.decodedRemaining -= size + } + r.remaining = int(size) return nil } +// validateRemainingDecodedExtent advances through unread AWS framing using the +// reader's existing buffer and returns any framing or read error to the tracker. +// The tracker preserves legacy handling of non-timeout framing errors while +// rejecting extent violations and bounded-read timeouts. +func (r *awsChunkedReader) validateRemainingDecodedExtent() error { + for !r.done { + if r.remaining == 0 { + if err := r.readChunkHeader(); err != nil { + return err + } + if r.done { + return nil + } + } + + n, err := r.reader.Discard(r.remaining) + r.remaining -= n + if err != nil { + return err + } + if r.remaining == 0 { + if err := r.readTrailingCRLF(); err != nil { + return err + } + } + } + return nil +} + // readLine reads a single \n-terminated line from the reader, enforcing // maxChunkHeaderLen to prevent unbounded memory allocation. func (r *awsChunkedReader) readLine() (string, error) { @@ -209,7 +266,10 @@ func decodeChunkedIfNeeded(r *http.Request) (body io.ReadCloser, bodyHash string } decoded := newAWSChunkedReader(r.Body) - return io.NopCloser(decoded), "UNSIGNED-PAYLOAD", contentLength, true + if contentLength >= 0 { + decoded.decodedRemaining = contentLength + } + return &awsChunkedReadCloser{reader: decoded}, "UNSIGNED-PAYLOAD", contentLength, true } // stripAWSChunkedToken removes the aws-chunked token from a Content-Encoding diff --git a/proxy/chunked_reader_test.go b/proxy/chunked_reader_test.go index 7a5fd63f..d4aa223c 100644 --- a/proxy/chunked_reader_test.go +++ b/proxy/chunked_reader_test.go @@ -1,6 +1,7 @@ package proxy import ( + "errors" "io" "net/http" "net/http/httptest" @@ -88,6 +89,123 @@ func TestAWSChunkedReader_SmallReads(t *testing.T) { } } +type oneByteChunkInput struct { + data []byte + offset int + payloadStart int + payloadRead int +} + +func (r *oneByteChunkInput) Read(p []byte) (int, error) { + if len(p) == 0 { + return 0, nil + } + if r.offset == len(r.data) { + return 0, io.EOF + } + p[0] = r.data[r.offset] + if r.offset >= r.payloadStart { + r.payloadRead++ + } + r.offset++ + return 1, nil +} + +func TestAWSChunkedReaderRejectsOversizedHeaderBeforePayload(t *testing.T) { + input := &oneByteChunkInput{ + data: []byte("8\r\nabc"), + payloadStart: len("8\r\n"), + } + reader := newAWSChunkedReader(input) + reader.decodedRemaining = 3 + + buf := make([]byte, 8) + n, err := reader.Read(buf) + if n != 0 { + t.Fatalf("Read yielded %d bytes from an oversized frame, want 0", n) + } + if !errors.Is(err, errAWSChunkExceedsDecodedLength) { + t.Fatalf("Read error = %v, want decoded-extent error", err) + } + if input.payloadRead != 0 { + t.Fatalf("reader consumed %d offending payload bytes before rejecting the header", input.payloadRead) + } +} + +func TestAWSChunkedReaderDecodedExtentAcrossPartialReads(t *testing.T) { + t.Run("valid multi-frame total", func(t *testing.T) { + reader := newAWSChunkedReader(strings.NewReader("2\r\nab\r\n3\r\ncde\r\n0\r\n\r\n")) + reader.decodedRemaining = 5 + + var got strings.Builder + buf := make([]byte, 1) + for { + n, err := reader.Read(buf) + got.Write(buf[:n]) + if err == io.EOF { + break + } + if err != nil { + t.Fatalf("Read failed: %v", err) + } + if n == 0 { + t.Fatal("Read made no progress") + } + } + if got.String() != "abcde" { + t.Fatalf("decoded body = %q, want %q", got.String(), "abcde") + } + }) + + t.Run("later frame exceeds remaining total", func(t *testing.T) { + reader := newAWSChunkedReader(strings.NewReader("2\r\nab\r\n3\r\nFAIL")) + reader.decodedRemaining = 4 + + var got strings.Builder + buf := make([]byte, 1) + for { + n, err := reader.Read(buf) + got.Write(buf[:n]) + if errors.Is(err, errAWSChunkExceedsDecodedLength) { + break + } + if err != nil { + t.Fatalf("Read error = %v, want decoded-extent error", err) + } + if n == 0 { + t.Fatal("Read made no progress") + } + } + if got.String() != "ab" { + t.Fatalf("decoded body = %q, want only prior valid frame %q", got.String(), "ab") + } + }) + + t.Run("frame after exact total", func(t *testing.T) { + reader := newAWSChunkedReader(strings.NewReader("4\r\nkeep\r\n1\r\nX\r\n0\r\n\r\n")) + reader.decodedRemaining = 4 + + var got strings.Builder + buf := make([]byte, 1) + for { + n, err := reader.Read(buf) + got.Write(buf[:n]) + if errors.Is(err, errAWSChunkExceedsDecodedLength) { + break + } + if err != nil { + t.Fatalf("Read error = %v, want decoded-extent error", err) + } + if n == 0 { + t.Fatal("Read made no progress") + } + } + if got.String() != "keep" { + t.Fatalf("decoded body = %q, want only the prior frame %q", got.String(), "keep") + } + }) +} + func TestAWSChunkedReader_LongSignature(t *testing.T) { // Real-world signatures are 64 hex chars sig := strings.Repeat("ab", 32) @@ -239,7 +357,9 @@ func TestPrepareForwardedRequest_Chunked(t *testing.T) { req.Header.Set("Content-Encoding", "aws-chunked") req.ContentLength = 2000 // wire size - prepareForwardedRequest(req, 1024, true) + if err := prepareForwardedRequest(req, 1024, true); err != nil { + t.Fatalf("prepareForwardedRequest: %v", err) + } if req.ContentLength != 1024 { t.Errorf("ContentLength = %d, want 1024", req.ContentLength) @@ -257,7 +377,9 @@ func TestPrepareForwardedRequest_ChunkedZeroByte(t *testing.T) { req.Header.Set("X-Amz-Decoded-Content-Length", "0") req.ContentLength = 100 // wire size with chunk framing - prepareForwardedRequest(req, 0, true) + if err := prepareForwardedRequest(req, 0, true); err != nil { + t.Fatalf("prepareForwardedRequest: %v", err) + } if req.ContentLength != 0 { t.Errorf("ContentLength = %d, want 0", req.ContentLength) @@ -273,11 +395,31 @@ func TestPrepareForwardedRequest_ChunkedZeroByte(t *testing.T) { } } +func TestPrepareForwardedRequest_ZeroExtentOverrun(t *testing.T) { + input := &oneByteChunkInput{ + data: []byte("1\r\nz\r\n"), + payloadStart: len("1\r\n"), + } + reader := newAWSChunkedReader(input) + reader.decodedRemaining = 0 + req := httptest.NewRequest(http.MethodPut, "http://localhost/bucket/key", io.NopCloser(reader)) + + err := prepareForwardedRequest(req, 0, true) + if !errors.Is(err, errAWSChunkExceedsDecodedLength) { + t.Fatalf("prepareForwardedRequest error = %v, want decoded-extent error", err) + } + if input.payloadRead != 0 { + t.Fatalf("prepareForwardedRequest consumed %d overrun payload bytes", input.payloadRead) + } +} + func TestPrepareForwardedRequest_NonChunked(t *testing.T) { req := httptest.NewRequest(http.MethodPut, "http://localhost/bucket/key", nil) req.ContentLength = 0 - prepareForwardedRequest(req, 512, false) + if err := prepareForwardedRequest(req, 512, false); err != nil { + t.Fatalf("prepareForwardedRequest: %v", err) + } if req.ContentLength != 512 { t.Errorf("ContentLength = %d, want 512", req.ContentLength) @@ -288,7 +430,9 @@ func TestPrepareForwardedRequest_NonChunkedNoBody(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "http://localhost/bucket/key", nil) req.ContentLength = 0 - prepareForwardedRequest(req, 0, false) + if err := prepareForwardedRequest(req, 0, false); err != nil { + t.Fatalf("prepareForwardedRequest: %v", err) + } // Should not change ContentLength for non-chunked with 0/negative length if req.ContentLength != 0 { @@ -376,7 +520,9 @@ func TestPrepareForwardedRequest_PreservesNonAWSContentEncoding(t *testing.T) { req := httptest.NewRequest(http.MethodPut, "http://localhost/bucket/key", nil) req.Header.Set("Content-Encoding", "gzip") - prepareForwardedRequest(req, 1024, true) + if err := prepareForwardedRequest(req, 1024, true); err != nil { + t.Fatalf("prepareForwardedRequest: %v", err) + } if req.Header.Get("Content-Encoding") != "gzip" { t.Errorf("Content-Encoding = %q, want %q", req.Header.Get("Content-Encoding"), "gzip") @@ -390,7 +536,9 @@ func TestPrepareForwardedRequest_CombinedContentEncoding(t *testing.T) { req.Header.Set("Content-Encoding", "aws-chunked,gzip") req.ContentLength = 2000 - prepareForwardedRequest(req, 1024, true) + if err := prepareForwardedRequest(req, 1024, true); err != nil { + t.Fatalf("prepareForwardedRequest: %v", err) + } if got := req.Header.Get("Content-Encoding"); got != "gzip" { t.Errorf("Content-Encoding = %q, want %q", got, "gzip") @@ -402,7 +550,9 @@ func TestPrepareForwardedRequest_CombinedContentEncodingWithSpaces(t *testing.T) req := httptest.NewRequest(http.MethodPut, "http://localhost/bucket/key", nil) req.Header.Set("Content-Encoding", "aws-chunked , gzip") - prepareForwardedRequest(req, 1024, true) + if err := prepareForwardedRequest(req, 1024, true); err != nil { + t.Fatalf("prepareForwardedRequest: %v", err) + } if got := req.Header.Get("Content-Encoding"); got != "gzip" { t.Errorf("Content-Encoding = %q, want %q", got, "gzip") diff --git a/proxy/forwarder.go b/proxy/forwarder.go index 076f79fa..f879a729 100644 --- a/proxy/forwarder.go +++ b/proxy/forwarder.go @@ -4,9 +4,13 @@ package proxy import ( "context" "errors" + "fmt" "io" "net/http" + "net/http/httptrace" + "os" "strings" + "sync" "time" "github.com/rs/zerolog/log" @@ -210,6 +214,38 @@ func (b *baseForwarder) executeAndStreamReturningMeta(w http.ResponseWriter, fwd return b.executeAndStreamWithMeta(w, fwdReq, inContentLength, originalReq, true) } +func (b *baseForwarder) validateSuccessfulChunkedRequest(w http.ResponseWriter, fwdReq *http.Request, status int) error { + if status < http.StatusOK || status >= http.StatusMultipleChoices { + return nil + } + tracker, ok := fwdReq.Body.(*requestBodyWriteTracker) + if !ok { + return nil + } + if err := tracker.waitForWrite(fwdReq.Context(), b.httpClient.Timeout); err != nil { + return err + } + if !tracker.validationPending() { + return tracker.validateDecodedExtent() + } + + // The outbound transport can return an upstream response before it has read + // every AWS frame from the client. Bound this second read using the same + // timeout as the upstream request so an idle client cannot hold its 2xx. + timeout := b.httpClient.Timeout + if timeout <= 0 { + timeout = 5 * time.Minute + } + controller := http.NewResponseController(w) + if err := controller.SetReadDeadline(time.Now().Add(timeout)); err != nil { + return fmt.Errorf("setting request-body validation deadline: %w", err) + } + // Leave the deadline in place for the remainder of this request. If validation + // fails with unread body bytes, clearing it here would remove the server's read + // bound while net/http finishes or closes the request. + return tracker.validateDecodedExtent() +} + // executeAndStreamWithMeta streams an upstream response and optionally retains an // owned copy of its headers for a caller that needs metadata after streaming. func (b *baseForwarder) executeAndStreamWithMeta(w http.ResponseWriter, fwdReq *http.Request, inContentLength int64, originalReq *http.Request, captureMeta bool) (int, http.Header, error) { @@ -221,10 +257,20 @@ func (b *baseForwarder) executeAndStreamWithMeta(w http.ResponseWriter, fwdReq * resp, err := b.httpClient.Do(fwdReq) metrics.RecordUpstreamRequest(fwdReq.Method, time.Since(upstreamStart).Seconds(), err) if err != nil { + if resp != nil { + recordUpstreamStatus(w, resp.StatusCode) + if resp.Body != nil { + _ = resp.Body.Close() + } + } log.Error().Err(err).Str("method", fwdReq.Method).Str("path", fwdReq.URL.Path).Msg("Failed to forward request") return 0, nil, err } defer resp.Body.Close() + if err := b.validateSuccessfulChunkedRequest(w, fwdReq, resp.StatusCode); err != nil { + recordUpstreamStatus(w, resp.StatusCode) + return 0, nil, err + } // Run response interceptor before sending headers to client if b.responseInterceptor != nil { @@ -500,10 +546,156 @@ func NewForwarder(credStore *auth.CredentialStore, tigrisEndpoint, region string } } +// requestBodyWriteTracker serializes decoder reads between the HTTP transport and +// a post-response validation pass. The transport can return response headers while +// its concurrent request writer is still reading the body. +type awsChunkExtentValidator interface { + validateRemainingDecodedExtent() error +} + +type requestBodyWriteTracker struct { + body io.ReadCloser + validator awsChunkExtentValidator + mu sync.Mutex + writeResult chan error + readErr error + eof bool + writerDone bool +} + +func trackRequestBodyWrite(req *http.Request, decodedBody io.ReadCloser) *requestBodyWriteTracker { + validator, _ := decodedBody.(awsChunkExtentValidator) + tracker := &requestBodyWriteTracker{ + body: req.Body, + validator: validator, + writeResult: make(chan error, 1), + } + trace := &httptrace.ClientTrace{WroteRequest: func(info httptrace.WroteRequestInfo) { + select { + case tracker.writeResult <- info.Err: + default: + } + }} + *req = *req.WithContext(httptrace.WithClientTrace(req.Context(), trace)) + req.Body = tracker + return tracker +} + +// Read serializes the transport and validation reads of the request body. +func (tracker *requestBodyWriteTracker) Read(p []byte) (int, error) { + tracker.mu.Lock() + defer tracker.mu.Unlock() + if tracker.writerDone { + return 0, io.EOF + } + return tracker.readLocked(p) +} + +func (tracker *requestBodyWriteTracker) readLocked(p []byte) (int, error) { + if tracker.readErr != nil { + return 0, tracker.readErr + } + if tracker.eof { + return 0, io.EOF + } + n, err := tracker.body.Read(p) + if errors.Is(err, io.EOF) { + tracker.eof = true + } else if err != nil { + tracker.readErr = err + } + return n, err +} + +// Close leaves the decoder open until its owner can finish validating the body. +// The signing body wrappers are no-ops on Close, and the inbound server owns r.Body. +func (*requestBodyWriteTracker) Close() error { return nil } + +// waitForWrite waits until net/http's outgoing request-body writer has stopped so +// validation can safely inspect any unread AWS frames. The writer's error is checked +// by validateDecodedExtent when it came from the request body; a network write error +// alone does not change the existing upstream-response behavior. +func (tracker *requestBodyWriteTracker) waitForWrite(ctx context.Context, timeout time.Duration) error { + if timeout <= 0 { + timeout = 5 * time.Minute + } + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case <-tracker.writeResult: + return nil + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return context.DeadlineExceeded + } +} + +func (tracker *requestBodyWriteTracker) validationPending() bool { + tracker.mu.Lock() + defer tracker.mu.Unlock() + return tracker.validator != nil && !tracker.eof && tracker.readErr == nil +} + +// validateDecodedExtent checks unread framing on the same decoder after the +// transport writer stops. The validator bypasses any tee so inspection cannot +// add bytes that the upstream request writer did not read. Extent violations +// and read timeouts reject the upstream success; other framing errors retain +// the existing forwarding behavior. +func (tracker *requestBodyWriteTracker) validateDecodedExtent() error { + tracker.mu.Lock() + defer tracker.mu.Unlock() + tracker.writerDone = true + if errors.Is(tracker.readErr, errAWSChunkExceedsDecodedLength) { + return tracker.readErr + } + if tracker.eof || tracker.readErr != nil || tracker.validator == nil { + return nil + } + if err := tracker.validator.validateRemainingDecodedExtent(); err != nil { + if errors.Is(err, errAWSChunkExceedsDecodedLength) || isRequestBodyReadTimeout(err) { + tracker.readErr = err + return err + } + } + return nil +} + +func isRequestBodyReadTimeout(err error) bool { + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, os.ErrDeadlineExceeded) { + return true + } + var timeoutErr interface{ Timeout() bool } + return errors.As(err, &timeoutErr) && timeoutErr.Timeout() +} + +// recordUpstreamStatus lets mutating handlers distinguish an upstream success +// from a validation failure that prevented forwarding that response to the client. +func recordUpstreamStatus(w http.ResponseWriter, status int) { + if recorder, ok := w.(interface{ recordUpstreamStatus(int) }); ok { + recorder.recordUpstreamStatus(status) + } +} + // prepareForwardedRequest sets Content-Length on the forwarded request and strips // AWS chunked-encoding headers when the request body was decoded from chunked format. -func prepareForwardedRequest(fwdReq *http.Request, contentLength int64, chunked bool) { +func prepareForwardedRequest(fwdReq *http.Request, contentLength int64, chunked bool) error { if chunked { + // A zero-length request is sent upstream with http.NoBody, so the transport + // would never read the decoder. Inspect its first frame header here to reject + // positive data chunks that exceed the zero-byte decoded extent before the + // body is discarded. Other read errors retain the prior empty-body behavior. + if contentLength == 0 && fwdReq.Body != nil && fwdReq.Body != http.NoBody { + var probe [1]byte + n, err := fwdReq.Body.Read(probe[:]) + if n > 0 { + return errAWSChunkExceedsDecodedLength + } + if errors.Is(err, errAWSChunkExceedsDecodedLength) { + return err + } + } + fwdReq.ContentLength = contentLength fwdReq.Header.Del("X-Amz-Decoded-Content-Length") stripAWSChunkedEncoding(fwdReq) @@ -517,6 +709,7 @@ func prepareForwardedRequest(fwdReq *http.Request, contentLength int64, chunked } else if contentLength > 0 { fwdReq.ContentLength = contentLength } + return nil } // stripAWSChunkedEncoding removes "aws-chunked" from the Content-Encoding header. diff --git a/proxy/forwarder_chunked_early_response_test.go b/proxy/forwarder_chunked_early_response_test.go new file mode 100644 index 00000000..02e22424 --- /dev/null +++ b/proxy/forwarder_chunked_early_response_test.go @@ -0,0 +1,273 @@ +package proxy + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "sync" + "testing" + "time" + + "github.com/tigrisdata/tag/auth" +) + +type chunkExtentBodyGate struct { + reader *strings.Reader + stopAfter int64 + read int64 + blocked chan struct{} + release <-chan struct{} + once sync.Once +} + +type testReadDeadlineResponseWriter struct { + *httptest.ResponseRecorder +} + +func (*testReadDeadlineResponseWriter) SetReadDeadline(time.Time) error { return nil } + +type deadlineBlockingReader struct { + mu sync.Mutex + deadline time.Time +} + +func (r *deadlineBlockingReader) SetReadDeadline(deadline time.Time) error { + r.mu.Lock() + r.deadline = deadline + r.mu.Unlock() + return nil +} + +func (r *deadlineBlockingReader) Read([]byte) (int, error) { + r.mu.Lock() + deadline := r.deadline + r.mu.Unlock() + if deadline.IsZero() { + return 0, errors.New("request-body read started without a deadline") + } + timer := time.NewTimer(time.Until(deadline)) + defer timer.Stop() + <-timer.C + return 0, os.ErrDeadlineExceeded +} + +type validationDeadlineResponseWriter struct { + *httptest.ResponseRecorder + reader *deadlineBlockingReader +} + +func (w *validationDeadlineResponseWriter) SetReadDeadline(deadline time.Time) error { + return w.reader.SetReadDeadline(deadline) +} + +func (r *chunkExtentBodyGate) Read(p []byte) (int, error) { + if len(p) == 0 { + return 0, nil + } + if r.read >= r.stopAfter { + r.once.Do(func() { close(r.blocked) }) + <-r.release + } else if int64(len(p)) > r.stopAfter-r.read { + p = p[:r.stopAfter-r.read] + } + n, err := r.reader.Read(p) + r.read += int64(n) + return n, err +} + +func TestRequestBodyWriteTrackerValidatesWithoutTeeingUnreadFrames(t *testing.T) { + for _, tc := range []struct { + name string + wire string + wantExtent bool + }{ + {name: "fitting frame", wire: "4\r\nkeep\r\n4\r\nPASS\r\n0\r\n\r\n"}, + {name: "oversized frame", wire: "4\r\nkeep\r\n5\r\nFAIL!\r\n0\r\n\r\n", wantExtent: true}, + } { + t.Run(tc.name, func(t *testing.T) { + reader := newAWSChunkedReader(strings.NewReader(tc.wire)) + reader.decodedRemaining = 8 + decodedBody := &awsChunkedReadCloser{reader: reader} + var tee strings.Builder + request := &http.Request{Body: io.NopCloser(io.TeeReader(decodedBody, &tee))} + tracker := trackRequestBodyWrite(request, decodedBody) + + prefix := make([]byte, 4) + n, err := request.Body.Read(prefix) + if err != nil || n != 4 || string(prefix) != "keep" { + t.Fatalf("transport read n=%d body=%q err=%v; want keep", n, prefix[:n], err) + } + tracker.writeResult <- nil // deterministic WroteRequest ownership handoff + if err := tracker.waitForWrite(context.Background(), time.Second); err != nil { + t.Fatalf("wait for request writer: %v", err) + } + err = tracker.validateDecodedExtent() + if tc.wantExtent != errors.Is(err, errAWSChunkExceedsDecodedLength) { + t.Fatalf("post-writer validation error = %v, extent violation=%t", err, tc.wantExtent) + } + if got := tee.String(); got != "keep" { + t.Fatalf("validation added unread source bytes to the upstream tee: %q", got) + } + }) + } +} + +func TestSuccessfulChunkedValidationReadUsesDeadline(t *testing.T) { + const timeout = 25 * time.Millisecond + source := &deadlineBlockingReader{} + decoded := &awsChunkedReadCloser{reader: newAWSChunkedReader(source)} + decoded.reader.decodedRemaining = 1 + request := &http.Request{Body: decoded} + tracker := trackRequestBodyWrite(request, decoded) + tracker.writeResult <- nil + writer := &validationDeadlineResponseWriter{ + ResponseRecorder: httptest.NewRecorder(), + reader: source, + } + forwarder := &baseForwarder{httpClient: &http.Client{Timeout: timeout}} + + started := time.Now() + err := forwarder.validateSuccessfulChunkedRequest(writer, request, http.StatusOK) + if !isRequestBodyReadTimeout(err) { + t.Fatalf("validation error = %v, want a request-body timeout", err) + } + if elapsed := time.Since(started); elapsed > time.Second { + t.Fatalf("request-body validation took %s, want it bounded by %s", elapsed, timeout) + } +} + +func TestSigningForwarderChunkedExtentWithEarlyHTTP2Response(t *testing.T) { + const ( + accessKey = "AKIAIOSFODNN7EXAMPLE" + secretKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + region = "us-east-1" + chunkSize = 8 * 1024 + ) + chunkHeader := fmt.Sprintf("%x\r\n", chunkSize) + wireBody := chunkHeader + strings.Repeat("A", chunkSize) + "\r\n5\r\nFAIL!\r\n0\r\n\r\n" + decodedLength := int64(chunkSize + 4) + + protocol := make(chan int, 1) + upstreamPrefix := make(chan []byte, 1) + allowResponse := make(chan struct{}) + var allowOnce sync.Once + defer allowOnce.Do(func() { close(allowResponse) }) + responseSent := make(chan struct{}) + upstream := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + protocol <- r.ProtoMajor + var prefix [1]byte + n, err := io.ReadFull(r.Body, prefix[:]) + if err != nil { + t.Errorf("read first upstream byte: n=%d err=%v", n, err) + } + upstreamPrefix <- append([]byte(nil), prefix[:n]...) + <-allowResponse + w.Header().Set("Content-Length", "6") + w.WriteHeader(http.StatusOK) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + _, _ = io.WriteString(w, "stored") + close(responseSent) + })) + upstream.EnableHTTP2 = true + upstream.StartTLS() + defer upstream.Close() + + credentials := auth.NewCredentialStore() + credentials.AddCredential(accessKey, secretKey) + base := newBaseForwarder(upstream.URL, region, 1) + base.httpClient = upstream.Client() + forwarder := &signingForwarder{ + baseForwarder: base, + credStore: credentials, + validator: auth.NewRequestValidator(credentials), + } + + bodyBlocked := make(chan struct{}) + releaseBody := make(chan struct{}) + var releaseBodyOnce sync.Once + defer releaseBodyOnce.Do(func() { close(releaseBody) }) + requestBody := &chunkExtentBodyGate{ + reader: strings.NewReader(wireBody), + stopAfter: int64(len(chunkHeader) + chunkSize/2), + blocked: bodyBlocked, + release: releaseBody, + } + headers := make(http.Header) + headers.Set("Content-Encoding", "aws-chunked") + headers.Set("X-Amz-Content-Sha256", "STREAMING-UNSIGNED-PAYLOAD-TRAILER") + headers.Set("X-Amz-Decoded-Content-Length", fmt.Sprint(decodedLength)) + request, err := auth.NewRequestSigner("http://client.example.com", region).SignRequest( + context.Background(), + http.MethodPut, + "/bucket/object", + requestBody, + "STREAMING-UNSIGNED-PAYLOAD-TRAILER", + accessKey, + secretKey, + headers, + ) + if err != nil { + t.Fatal(err) + } + request.ContentLength = int64(len(wireBody)) + if _, err := forwarder.validator.ValidateRequest(request); err != nil { + t.Fatalf("incoming SigV4 request did not validate: %v", err) + } + + writer := &testReadDeadlineResponseWriter{ResponseRecorder: httptest.NewRecorder()} + recorder := &statusRecorder{ResponseWriter: writer} + forwardDone := make(chan error, 1) + go func() { + forwardDone <- forwarder.Forward(context.Background(), recorder, request) + }() + select { + case <-bodyBlocked: + case <-time.After(3 * time.Second): + t.Fatal("HTTP/2 forwarder did not reach the controlled body-read gate") + } + select { + case version := <-protocol: + if version != 2 { + t.Fatalf("upstream protocol major = %d, want HTTP/2", version) + } + case <-time.After(3 * time.Second): + t.Fatal("HTTP/2 upstream did not receive the request") + } + select { + case prefix := <-upstreamPrefix: + if string(prefix) != "A" { + t.Fatalf("upstream prefix = %q, want one valid-frame byte", prefix) + } + case <-time.After(3 * time.Second): + t.Fatal("HTTP/2 upstream did not read its prefix") + } + allowOnce.Do(func() { close(allowResponse) }) + select { + case <-responseSent: + case <-time.After(3 * time.Second): + t.Fatal("HTTP/2 upstream did not send its early 2xx response") + } + releaseBodyOnce.Do(func() { close(releaseBody) }) + + select { + case err := <-forwardDone: + if err == nil { + t.Fatalf("signing forwarder returned success after an early HTTP/2 response; response body=%q", writer.Body.String()) + } + if writer.Body.Len() != 0 { + t.Fatalf("signing forwarder exposed upstream success body %q after validation failed", writer.Body.String()) + } + if recorder.status != 0 { + t.Fatalf("client response status was recorded as %d despite validation failure", recorder.status) + } + case <-time.After(5 * time.Second): + t.Fatal("HTTP/2 request body validation did not finish") + } +} diff --git a/proxy/forwarder_signing.go b/proxy/forwarder_signing.go index 9f802d90..f4611f75 100644 --- a/proxy/forwarder_signing.go +++ b/proxy/forwarder_signing.go @@ -52,7 +52,14 @@ func (f *signingForwarder) Forward(ctx context.Context, w http.ResponseWriter, r if err != nil { return err } - prepareForwardedRequest(fwdReq, contentLength, chunked) + if err := prepareForwardedRequest(fwdReq, contentLength, chunked); err != nil { + return err + } + if chunked && contentLength > 0 && fwdReq.Body != nil && fwdReq.Body != http.NoBody { + // The HTTP transport may receive a response before its concurrent body + // writer reaches later AWS chunk headers. Gate a 2xx on that write's result. + trackRequestBodyWrite(fwdReq, body) + } return f.executeAndStream(w, fwdReq, contentLength, nil) } @@ -93,7 +100,14 @@ func (f *signingForwarder) ForwardTeeingBody(ctx context.Context, w http.Respons if err != nil { return 0, nil, "", "", err } - prepareForwardedRequest(fwdReq, contentLength, chunked) + if err := prepareForwardedRequest(fwdReq, contentLength, chunked); err != nil { + return 0, nil, "", "", err + } + if chunked && contentLength > 0 && fwdReq.Body != nil && fwdReq.Body != http.NoBody { + // Validate unread frames from body, not fwdReq.Body's tee, so this check does + // not append bytes that the upstream request writer did not consume. + trackRequestBodyWrite(fwdReq, body) + } // Return the validated credentials so the caller can HEAD/warm without re-validating. status, headers, err := f.executeAndStreamReturningMeta(w, fwdReq, contentLength, nil) @@ -131,7 +145,9 @@ func (f *signingForwarder) ForwardWithCapture(ctx context.Context, w http.Respon if err != nil { return nil, err } - prepareForwardedRequest(fwdReq, contentLength, chunked) + if err := prepareForwardedRequest(fwdReq, contentLength, chunked); err != nil { + return nil, err + } return f.executeAndCapture(w, fwdReq, contentLength, nil) } @@ -169,7 +185,9 @@ func (f *signingForwarder) DoRequestWithCreds(ctx context.Context, r *http.Reque if err != nil { return nil, err } - prepareForwardedRequest(fwdReq, contentLength, chunked) + if err := prepareForwardedRequest(fwdReq, contentLength, chunked); err != nil { + return nil, err + } return f.executeRequest(fwdReq, contentLength, nil) } diff --git a/proxy/reinvalidation_test.go b/proxy/reinvalidation_test.go index 5829f0cc..7cada058 100644 --- a/proxy/reinvalidation_test.go +++ b/proxy/reinvalidation_test.go @@ -2,11 +2,17 @@ package proxy import ( "context" + "errors" + "io" "net/http" "net/http/httptest" + "net/http/httptrace" "strings" + "sync" "testing" + "time" + "github.com/tigrisdata/tag/auth" "github.com/tigrisdata/tag/cache" ) @@ -40,6 +46,167 @@ func TestHandlePutObject_ReinvalidatesAfterForward(t *testing.T) { } } +type earlySuccessRoundTripper struct { + prefixAccepted chan<- []byte + allowResponse <-chan struct{} +} + +func (transport earlySuccessRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + var prefix [1]byte + n, err := io.ReadFull(req.Body, prefix[:]) + if err != nil { + return nil, err + } + transport.prefixAccepted <- append([]byte(nil), prefix[:n]...) + <-transport.allowResponse + if trace := httptrace.ContextClientTrace(req.Context()); trace != nil && trace.WroteRequest != nil { + trace.WroteRequest(httptrace.WroteRequestInfo{}) + } + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{"Content-Length": []string{"6"}}, + Body: io.NopCloser(strings.NewReader("stored")), + ContentLength: 6, + Request: req, + }, nil +} + +type releaseBodyOnDeadlineWriter struct { + *httptest.ResponseRecorder + releaseBody chan struct{} + releaseOnce *sync.Once + deadlineSet chan struct{} + deadlineOnce sync.Once +} + +func (w *releaseBodyOnDeadlineWriter) SetReadDeadline(time.Time) error { + w.releaseOnce.Do(func() { close(w.releaseBody) }) + w.deadlineOnce.Do(func() { close(w.deadlineSet) }) + return nil +} + +func TestHandlePutObject_ReinvalidatesAfterUpstreamSuccessAndValidationError(t *testing.T) { + const ( + accessKey = "AKIAIOSDNN7EXAMPLE" + secretKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + region = "us-east-1" + bucket = "bucket" + key = "chunked-object" + ) + const wireBody = "8\r\nABCDEFGH\r\n5\r\nFAIL!\r\n0\r\n\r\n" + + bodyBlocked := make(chan struct{}) + releaseBody := make(chan struct{}) + var releaseBodyOnce sync.Once + defer releaseBodyOnce.Do(func() { close(releaseBody) }) + allowResponse := make(chan struct{}) + var allowResponseOnce sync.Once + defer allowResponseOnce.Do(func() { close(allowResponse) }) + prefixAccepted := make(chan []byte, 1) + + service, c := newTestService(&mockForwarder{}, true) + credentials := auth.NewCredentialStore() + credentials.AddCredential(accessKey, secretKey) + base := newBaseForwarder("http://upstream.example.com", region, 1) + base.httpClient = &http.Client{ + Transport: earlySuccessRoundTripper{ + prefixAccepted: prefixAccepted, + allowResponse: allowResponse, + }, + Timeout: 5 * time.Second, + } + service.forwarder = &signingForwarder{ + baseForwarder: base, + credStore: credentials, + validator: auth.NewRequestValidator(credentials), + } + service.config.Cache.WarmOnWrite = false + + chunkHeader := "8\r\n" + requestBody := &chunkExtentBodyGate{ + reader: strings.NewReader(wireBody), + stopAfter: int64(len(chunkHeader) + 1), + blocked: bodyBlocked, + release: releaseBody, + } + headers := make(http.Header) + headers.Set("Content-Encoding", "aws-chunked") + headers.Set("X-Amz-Content-Sha256", "STREAMING-UNSIGNED-PAYLOAD-TRAILER") + headers.Set("X-Amz-Decoded-Content-Length", "12") + r, err := auth.NewRequestSigner("http://client.example.com", region).SignRequest( + context.Background(), + http.MethodPut, + "/"+bucket+"/"+key, + requestBody, + "STREAMING-UNSIGNED-PAYLOAD-TRAILER", + accessKey, + secretKey, + headers, + ) + if err != nil { + t.Fatal(err) + } + r.ContentLength = int64(len(wireBody)) + + writer := &releaseBodyOnDeadlineWriter{ + ResponseRecorder: httptest.NewRecorder(), + releaseBody: releaseBody, + releaseOnce: &releaseBodyOnce, + deadlineSet: make(chan struct{}), + } + forwardDone := make(chan error, 1) + go func() { forwardDone <- service.HandlePutObject(writer, r) }() + select { + case prefix := <-prefixAccepted: + if string(prefix) != "A" { + t.Fatalf("upstream prefix = %q, want the first valid-frame byte", prefix) + } + case <-time.After(3 * time.Second): + t.Fatal("upstream did not accept a prefix of the valid frame") + } + + // A GET that began before the PUT completed can repopulate stale bytes after + // the pre-forward invalidation. Model that commit before the upstream 2xx. + staleMeta := &cache.CachedObjectMeta{ + Bucket: bucket, + Key: key, + ETag: `"stale"`, + ContentLength: 5, + StatusCode: http.StatusOK, + } + if err := c.PutWithMeta(context.Background(), bucket, key, staleMeta, []byte("stale"), 60); err != nil { + t.Fatalf("seed racing stale refill: %v", err) + } + allowResponseOnce.Do(func() { close(allowResponse) }) + + select { + case err := <-forwardDone: + if !errors.Is(err, errAWSChunkExceedsDecodedLength) { + t.Fatalf("HandlePutObject error = %v, want the overlong-frame error", err) + } + case <-time.After(5 * time.Second): + t.Fatal("PUT did not finish after the overlong frame was validated") + } + select { + case <-bodyBlocked: + default: + t.Fatal("post-response validation did not reach the unread client body") + } + select { + case <-writer.deadlineSet: + default: + t.Fatal("post-response validation did not set a read deadline") + } + if writer.Body.Len() != 0 { + t.Fatalf("upstream success body reached the client despite validation failure: %q", writer.Body.String()) + } + if _, found, err := c.GetMeta(context.Background(), bucket, key); err != nil { + t.Fatalf("read cache metadata: %v", err) + } else if found { + t.Fatal("racing stale refill survived the upstream-success validation error") + } +} + // A rejected CopyObject leaves the destination unchanged, so the post-forward // re-invalidation must NOT fire — otherwise a racing refill of the still-valid // destination is discarded, causing an unnecessary later miss. diff --git a/proxy/service.go b/proxy/service.go index eb9e9958..ca15e083 100644 --- a/proxy/service.go +++ b/proxy/service.go @@ -658,17 +658,20 @@ func (s *Service) releaseCacheSlot(weight int64) { } } -// statusRecorder wraps http.ResponseWriter to capture the response status code -// written while forwarding an upstream response. Forward() returns nil even when -// upstream responds 4xx/5xx (the response streamed successfully), so mutating -// handlers use this to gate post-forward cache re-invalidation on an actual 2xx — -// otherwise a rejected PUT/DELETE/COPY would still fence the destination and -// discard a valid racing refill, causing later reads to miss unnecessarily. +// statusRecorder wraps http.ResponseWriter to capture the client response status +// and the upstream status observed before a chunked-body validation error. Mutating +// handlers use the client status for ordinary post-forward work and the upstream +// status only to invalidate after an upstream success that could not be relayed. type statusRecorder struct { http.ResponseWriter - status int + status int + upstreamStatus int } +func (rec *statusRecorder) Unwrap() http.ResponseWriter { return rec.ResponseWriter } + +func (rec *statusRecorder) recordUpstreamStatus(status int) { rec.upstreamStatus = status } + func (rec *statusRecorder) WriteHeader(code int) { rec.status = code rec.ResponseWriter.WriteHeader(code) @@ -689,11 +692,15 @@ func (rec *statusRecorder) Flush() { } } -// wroteSuccess reports whether upstream returned a 2xx status. +// wroteSuccess reports whether a 2xx status was written to the client. func (rec *statusRecorder) wroteSuccess() bool { return rec.status >= 200 && rec.status < 300 } +func (rec *statusRecorder) upstreamSucceeded() bool { + return rec.upstreamStatus >= 200 && rec.upstreamStatus < 300 +} + // HandlePutObject handles PUT requests for objects. // Invalidates cache BEFORE forwarding to ensure consistency. func (s *Service) HandlePutObject(w http.ResponseWriter, r *http.Request) error { @@ -721,16 +728,19 @@ func (s *Service) HandlePutObject(w http.ResponseWriter, r *http.Request) error rec := &statusRecorder{ResponseWriter: w} teed, requestRejectsCache, err := s.forwardPutMaybeTee(r.Context(), rec, r, bucket, key) - // Re-invalidate AFTER upstream confirms the write. A GET that raced the - // in-flight PUT may have fetched the pre-PUT object and begun re-caching it; - // this second invalidation bumps the fence past that write's decision-time - // token, so its version-preconditioned commit loses — - // restoring read-after-write semantics. - // Gated on a 2xx: a rejected PUT leaves the object unchanged, so re-invalidating - // would only discard a valid racing refill and cause an unnecessary later miss. - // Routed through invalidateObject (like the pre-forward call) so a failure of this - // read-after-write-critical invalidation is recorded and logged, not discarded. + // Re-invalidate after an upstream success. A GET that raced the in-flight PUT + // may have fetched the pre-PUT object and begun re-caching it; this second + // invalidation bumps the fence past that write's decision-time token. A chunked + // body validation error can prevent the upstream 2xx from reaching the client + // after the origin accepted a prefix, so use that recorded upstream status only + // to invalidate; never warm from the incomplete request. + if err != nil && rec.upstreamSucceeded() && s.cache.IsEnabled() { + s.convergeInvalidation(context.Background(), bucket, key) + } + // Ordinary successful writes retain the existing cache population behavior. if err == nil && rec.wroteSuccess() && s.cache.IsEnabled() { + // Routed through invalidateObject (like the pre-forward call) so a failure of this + // read-after-write-critical invalidation is recorded and logged, not discarded. s.convergeInvalidation(context.Background(), bucket, key) teeHandled := requestRejectsCache if teed != nil { diff --git a/proxy/tiered.go b/proxy/tiered.go index 66d54c16..d3cfcde9 100644 --- a/proxy/tiered.go +++ b/proxy/tiered.go @@ -162,23 +162,28 @@ func (s *Service) handleTieredPut(w http.ResponseWriter, r *http.Request) error } declaredSize, sized := originlessPutSize(r) + var ( + prior *cache.CachedObjectMeta + priorVersion uint64 + priorKnown bool + ) // Local-tier eligibility, before any metadata lookup: a validated caller // (the engine serves from cache with no upstream auth check), a plain - // object PUT, and a declared size within the threshold. Everything else - // forwards and never needs the prior version — a metadata failure must not - // block a PUT that goes upstream anyway (including the unknown-key writes - // that bootstrap credential learning). + // object PUT, and a declared size within the threshold. Other writes normally + // forward without a prior snapshot; signing-forwarded AWS streams are the + // exception because late body validation may need version-guarded cleanup. if result == AuthValidated && originlessPlainObject(r) && sized && declaredSize <= s.config.Cache.SizeThreshold { // The prior version's tier decides what an overwrite must clean up and // where a conditional write is evaluated, so a failed lookup cannot be // read as "no prior". Fail retryably instead. - priorMeta, priorVersion, found, cacheErr := s.cache.GetMetaWithVersion(ctx, bucket, key) + priorMeta, version, found, cacheErr := s.cache.GetMetaWithVersion(ctx, bucket, key) if cacheErr != nil { metrics.RecordRequest("PutObject", "error", metrics.SourceLocal, time.Since(start).Seconds()) return cacheErr } - var prior *cache.CachedObjectMeta + priorVersion = version + priorKnown = true if found { prior = priorMeta } @@ -192,7 +197,7 @@ func (s *Service) handleTieredPut(w http.ResponseWriter, r *http.Request) error // Thread the SAME snapshot into the engine: the tier decision // above and the engine's precondition/token now share one read, // so a marker committing in between cannot make them disagree. - err := s.handleOriginlessPut(rec, r, &putPrior{meta: priorMeta, version: priorVersion, found: found}) + err := s.handleOriginlessPut(rec, r, &putPrior{meta: priorMeta, version: version, found: found}) if err == nil && rec.wroteSuccess() && prior != nil && prior.BodyUpstream { // Small write displaced an upstream-tier version: remove the // upstream copy so it doesn't linger as an orphan. Bound to the @@ -208,22 +213,24 @@ func (s *Service) handleTieredPut(w http.ResponseWriter, r *http.Request) error // that makes this object exist in TAG's authoritative view. // // No pre-forward invalidation, unlike HandlePutObject: for a local-tier - // prior the cache holds the ONLY copy, and a failed forward must leave it - // intact — S3 semantics say a rejected PUT changes nothing. Reads racing - // the in-flight PUT serve the prior version, which is the atomic-replace - // behavior clients expect. The one read-triggered populate in this mode — - // the re-tier — cannot be ordered against this path's writes here (it - // performs none pre-forward); it defends itself with a claim plus its own - // pre-fetch decision token instead (see maybeRetierOnRead). - // No post-success invalidation either: the marker overwrites the prior - // metadata directly (a displaced local body ages out by TTL, the engine's - // own overwrite semantics), which lets the marker commit under the - // PRE-FORWARD decision token — see putUpstreamMarker for why that closes - // the concurrent-DELETE resurrection race. - // Capture the displaced prior before forwarding — tolerated, never blocking: - // it only arms the identity guard of the failure sweep in putUpstreamMarker. - // A failed lookup leaves the prior unknown, and the sweep then refuses to - // delete anything rather than guess. + // prior the cache holds the ONLY copy, and an upstream rejection or unknown + // outcome must leave it intact. Reads racing the in-flight PUT serve the prior + // version, which is the atomic-replace behavior clients expect. The one + // read-triggered populate in this mode — the re-tier — cannot be ordered + // against this path's writes here (it performs none pre-forward); it defends + // itself with a claim plus its own pre-fetch decision token instead (see + // maybeRetierOnRead). + // Ordinary successful forwards need no separate invalidation: the new marker + // replaces prior metadata under the PRE-FORWARD decision token, closing the + // concurrent-DELETE resurrection race (see putUpstreamMarker). When a known + // upstream 2xx cannot be relayed because later body validation fails, no + // marker can be built from the incomplete request; the guarded sweep below + // removes only the displaced prior. + // Capture the displaced prior before forwarding. Signed AWS-chunked PUTs + // require this snapshot because a later frame-validation error can follow an + // upstream 2xx; other marker writes retain best-effort capture. A failed + // required lookup stops before dispatch rather than guessing which version + // a later cleanup may remove. // Only a PLAIN object PUT writes the object and therefore owns the marker. // A sub-resource PUT with no dedicated route (?retention, ?legal-hold, …) // reaches this path too, but it does not create a new object version: it @@ -232,17 +239,34 @@ func (s *Service) handleTieredPut(w http.ResponseWriter, r *http.Request) error // a 2xx response without an ETag would sweep the object's live metadata // into an authoritative miss. markerOwning := originlessPlainObject(r) - - var prior *cache.CachedObjectMeta - var priorVersion uint64 - priorKnown := false - if markerOwning { - prior, priorVersion, priorKnown = s.captureMarkerPrior(ctx, bucket, key) + needsValidationSnapshot := markerOwning && s.cache.IsEnabled() && !s.config.ForwardsTransparently() && + IsStreamingPayload(r.Header.Get("X-Amz-Content-Sha256")) && sized && declaredSize > 0 + if markerOwning && !priorKnown { + if needsValidationSnapshot { + meta, version, found, cacheErr := s.cache.GetMetaWithVersion(ctx, bucket, key) + if cacheErr != nil { + metrics.RecordRequest("PutObject", "error", metrics.SourceLocal, time.Since(start).Seconds()) + return cacheErr + } + priorVersion = version + priorKnown = true + if found { + prior = meta + } + } else { + prior, priorVersion, priorKnown = s.captureMarkerPrior(ctx, bucket, key) + } } rec := &statusRecorder{ResponseWriter: w} err = s.forwarder.Forward(ctx, rec, r) + // A body-validation error can hide an upstream 2xx from the client after + // the origin has changed the object. Remove only the displaced pre-forward + // version; a newer tiered write must keep its authoritative local state. + if err != nil && rec.upstreamSucceeded() && markerOwning && s.cache.IsEnabled() { + s.invalidateDisplacedTieredMeta(bucket, key, prior, priorVersion, priorKnown) + } if err == nil && rec.wroteSuccess() && markerOwning && s.cache.IsEnabled() { s.putUpstreamMarker(r, w.Header().Get("ETag"), bucket, key, prior, priorVersion, priorKnown) } @@ -448,8 +472,9 @@ func (s *Service) headObjectMeta(ctx context.Context, bucket, key, accessKey, se } // invalidateDisplacedTieredMeta converges a key on an authoritative miss -// after a marker could not be established, without either destroying a newer -// racing write or deleting blind: cache.DeleteIfETag removes the entry only +// after a marker could not be established, including a known upstream success +// whose body validation failed, without destroying a newer racing write or +// deleting blind: cache.DeleteMetaIfVersion removes the entry only // while it still IS the displaced prior — the compare and the delete are one // CAS, so the compare-then-delete window the pre-CAS helper documented is // gone. Anything else present is a newer write and keeps the key; this PUT's diff --git a/proxy/tiered_test.go b/proxy/tiered_test.go index c3d5e8ae..265e3171 100644 --- a/proxy/tiered_test.go +++ b/proxy/tiered_test.go @@ -70,6 +70,155 @@ func tieredMock() (*mockForwarder, *atomic.Int64, *atomic.Int64) { return m, forwards, deletes } +var errTieredPriorSnapshotRead = errors.New("injected tiered prior snapshot read failure") + +type failNextTieredVersionRead struct { + cacheclient.CacheClient + failNext atomic.Bool +} + +func (client *failNextTieredVersionRead) GetWithVersion(ctx context.Context, key string) ([]byte, uint64, bool, error) { + if client.failNext.CompareAndSwap(true, false) { + return nil, 0, false, errTieredPriorSnapshotRead + } + return client.CacheClient.GetWithVersion(ctx, key) +} + +func TestTieredSignedChunkedPutRequiresPriorSnapshotBeforeForward(t *testing.T) { + cfg := config.NewDefault() + cfg.Mode = config.ModeTiered + cfg.Upstream.Endpoint = "http://localhost:9000" + cfg.Cache.SetBlockCachingEnabled(false) + cfg.Cache.SizeThreshold = 1 + if err := cfg.Validate(); err != nil { + t.Fatal(err) + } + + flakyClient := &failNextTieredVersionRead{CacheClient: cacheclient.NewMemoryCache()} + c := cache.NewCacheWithClient(flakyClient, &cfg.Cache) + var forwards atomic.Int64 + mock := &mockForwarder{forwardFunc: func(_ context.Context, w http.ResponseWriter, _ *http.Request) error { + forwards.Add(1) + w.WriteHeader(http.StatusOK) + return nil + }} + service := NewService(mock, c, cfg) + prior := &cache.CachedObjectMeta{ + Bucket: "b", + Key: "obj", + ETag: `"prior"`, + ContentLength: 5, + StatusCode: http.StatusOK, + } + if err := c.PutWithMeta(context.Background(), "b", "obj", prior, []byte("prior"), 60); err != nil { + t.Fatalf("seed prior Tiered object: %v", err) + } + flakyClient.failNext.Store(true) + + const wireBody = "8\r\nABCDEFGH\r\n5\r\nFAIL!\r\n0\r\n\r\n" + r := httptest.NewRequest(http.MethodPut, "/b/obj", strings.NewReader(wireBody)) + r.Header.Set("X-Amz-Content-Sha256", StreamingUnsignedTrailerHash) + r.Header.Set("X-Amz-Decoded-Content-Length", "12") + w := httptest.NewRecorder() + if err := service.HandlePutObject(w, r); !errors.Is(err, errTieredPriorSnapshotRead) { + t.Fatalf("HandlePutObject error = %v, want prior-snapshot error", err) + } + if count := forwards.Load(); count != 0 { + t.Fatalf("upstream forwards = %d, want 0 without a prior-version snapshot", count) + } + if w.Body.Len() != 0 { + t.Fatalf("upstream response reached the client before snapshot validation: %q", w.Body.String()) + } + meta, found, err := c.GetMeta(context.Background(), "b", "obj") + if err != nil || !found || meta.ETag != `"prior"` { + t.Fatalf("prior metadata after failed snapshot: found=%t meta=%+v err=%v", found, meta, err) + } + var body bytes.Buffer + if err := c.GetBodyStream(context.Background(), "b", "obj", meta.ETag, &body); err != nil { + t.Fatalf("read prior body: %v", err) + } + if body.String() != "prior" { + t.Fatalf("prior body after failed snapshot = %q, want %q", body.String(), "prior") + } +} + +func TestTieredPutInvalidatesDisplacedPriorOnlyAfterKnownUpstreamSuccess(t *testing.T) { + for _, tc := range []struct { + name string + recordSuccess bool + concurrentWrite bool + rejectUpstream bool + wantFound bool + wantETag string + }{ + {name: "known upstream success", recordSuccess: true}, + {name: "newer local write survives", recordSuccess: true, concurrentWrite: true, wantFound: true, wantETag: `"newer"`}, + {name: "unknown upstream status keeps prior", wantFound: true, wantETag: `"prior"`}, + {name: "upstream rejection keeps prior", rejectUpstream: true, wantFound: true, wantETag: `"prior"`}, + } { + t.Run(tc.name, func(t *testing.T) { + var c *cache.Cache + mock := &mockForwarder{forwardFunc: func(_ context.Context, w http.ResponseWriter, _ *http.Request) error { + if tc.concurrentWrite { + newer := &cache.CachedObjectMeta{ + Bucket: "b", + Key: "obj", + ETag: `"newer"`, + ContentLength: 5, + StatusCode: http.StatusOK, + } + if err := c.PutWithMeta(context.Background(), "b", "obj", newer, []byte("newer"), 60); err != nil { + t.Errorf("seed concurrent local write: %v", err) + } + } + if tc.rejectUpstream { + w.WriteHeader(http.StatusForbidden) + return nil + } + if tc.recordSuccess { + recordUpstreamStatus(w, http.StatusOK) + } + return errAWSChunkExceedsDecodedLength + }} + svc, c := newTieredTestService(mock, 1) + prior := &cache.CachedObjectMeta{ + Bucket: "b", + Key: "obj", + ETag: `"prior"`, + ContentLength: 5, + StatusCode: http.StatusOK, + } + if err := c.PutWithMeta(context.Background(), "b", "obj", prior, []byte("prior"), 60); err != nil { + t.Fatalf("seed prior tiered object: %v", err) + } + + r := httptest.NewRequest(http.MethodPut, "/b/obj", strings.NewReader("aws-chunked-wire")) + r.Header.Set("X-Amz-Content-Sha256", StreamingUnsignedTrailerHash) + r.Header.Set("X-Amz-Decoded-Content-Length", "12") + w := httptest.NewRecorder() + err := svc.HandlePutObject(w, r) + if tc.rejectUpstream { + if err != nil { + t.Fatalf("rejected upstream PUT returned error: %v", err) + } + } else if !errors.Is(err, errAWSChunkExceedsDecodedLength) { + t.Fatalf("HandlePutObject error = %v, want overlong-frame error", err) + } + + meta, found, err := c.GetMeta(context.Background(), "b", "obj") + if err != nil { + t.Fatalf("read tiered metadata: %v", err) + } + if found != tc.wantFound { + t.Fatalf("prior metadata found = %t, want %t", found, tc.wantFound) + } + if found && meta.ETag != tc.wantETag { + t.Fatalf("remaining metadata ETag = %q, want %q", meta.ETag, tc.wantETag) + } + }) + } +} + func tieredDo(t *testing.T, svc *Service, method, path, body string, hdr map[string]string) *httptest.ResponseRecorder { t.Helper() var req *http.Request diff --git a/proxy/write_through_test.go b/proxy/write_through_test.go index 36eca8ec..2b28d1f5 100644 --- a/proxy/write_through_test.go +++ b/proxy/write_through_test.go @@ -201,6 +201,68 @@ func TestHandlePutObject_WriteThroughTee_ETagMismatchFallsBackToWarm(t *testing. } } +// A transport may stop reading the tee before the full request body is copied. If the +// authoritative HEAD advertises a different length, the short tee must fall back to the +// upstream body instead of caching the bytes the transport happened to read. +func TestHandlePutObject_WriteThroughTee_IncompleteBufferFallsBackToWarm(t *testing.T) { + var puts, warmGets atomic.Int32 + const ( + requestBody = "complete-request-body" + teePrefix = "accepted-prefix" + warmBody = "authoritative-origin-body" + ) + mock := &teeMockForwarder{ + mockForwarder: &mockForwarder{ + conditionalResp: headResp(`"tee-etag"`, "text/plain", int64(len(requestBody))), + doFullObjectFunc: warmObjectResponder(&warmGets, warmBody), + }, + teeFunc: func(_ context.Context, w http.ResponseWriter, _ *http.Request, tee io.Writer) (int, http.Header, string, string, error) { + if _, err := io.WriteString(tee, teePrefix); err != nil { + return 0, nil, "", "", err + } + puts.Add(1) + headers := http.Header{"ETag": []string{`"tee-etag"`}} + w.WriteHeader(http.StatusOK) + return http.StatusOK, headers, "access", "secret", nil + }, + } + service, c := newTestService(mock, true) + service.config.Cache.WarmOnWrite = true + service.config.Cache.SizeThreshold = 1 << 20 + service.config.Cache.BlockSize = 1 << 20 + + w := httptest.NewRecorder() + if err := service.HandlePutObject(w, authedPut(wowBucket, wowKey, requestBody)); err != nil { + t.Fatalf("HandlePutObject: %v", err) + } + if w.Code != http.StatusOK { + t.Fatalf("client status = %d, want 200", w.Code) + } + if !metaCached(c, wowBucket, wowKey, 2*time.Second) { + t.Fatal("authoritative body was not cached after the short tee fell back") + } + if got := warmGets.Load(); got != 1 { + t.Fatalf("read-back warms = %d, want 1 after HEAD/tee length mismatch", got) + } + if got := puts.Load(); got != 1 { + t.Fatalf("upstream PUTs = %d, want 1", got) + } + meta, found, err := c.GetMeta(context.Background(), wowBucket, wowKey) + if err != nil || !found { + t.Fatalf("cached metadata found=%t err=%v", found, err) + } + if meta.ETag != `"warm-etag"` { + t.Fatalf("cached ETag = %q, want the authoritative warm body's ETag", meta.ETag) + } + var cachedBody bytes.Buffer + if err := c.GetBodyStream(context.Background(), wowBucket, wowKey, meta.ETag, &cachedBody); err != nil { + t.Fatalf("read cached body: %v", err) + } + if cachedBody.String() != warmBody { + t.Fatalf("cached body = %q, want authoritative body %q", cachedBody.String(), warmBody) + } +} + // If the HEAD itself fails, the tee can't source authoritative metadata, so it falls back to // a read-back warm rather than caching with guessed headers. func TestHandlePutObject_WriteThroughTee_HeadFailureFallsBackToWarm(t *testing.T) {