diff --git a/s3api/controllers/object-put.go b/s3api/controllers/object-put.go index 97174b05..2b036a40 100644 --- a/s3api/controllers/object-put.go +++ b/s3api/controllers/object-put.go @@ -27,6 +27,7 @@ import ( "github.com/gofiber/fiber/v3" "github.com/versity/versitygw/auth" "github.com/versity/versitygw/debuglogger" + "github.com/versity/versitygw/s3api/middlewares" "github.com/versity/versitygw/s3api/utils" "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3event" @@ -307,6 +308,21 @@ func (c S3ApiController) UploadPart(ctx fiber.Ctx) (*Response, error) { } else { body = bytes.NewReader([]byte{}) } + // aws-chunked bodies are framed and length-checked by the chunk readers + // (the ones implementing middlewares.ChecksumReader). A plain body has + // nothing but Content-Length to tell a finished upload from an aborted one. + // + // Use the raw Content-Length header, not contentLength: that variable may + // have been replaced by X-Amz-Decoded-Content-Length above, which describes + // the DECODED size. That header only applies to aws-chunked payloads, and + // those skip this wrapper anyway. AWS S3 ignores it on a plain body and + // stores Content-Length bytes, so checking against the decoded value would + // reject a complete upload. + if _, chunked := body.(middlewares.ChecksumReader); !chunked { + if raw, cerr := strconv.ParseInt(ctx.Get("Content-Length"), 10, 64); cerr == nil && raw > 0 { + body = utils.NewContentLengthReader(body, raw) + } + } res, err := c.be.UploadPart(ctx.RequestCtx(), &s3.UploadPartInput{ @@ -807,6 +823,21 @@ func (c S3ApiController) PutObject(ctx fiber.Ctx) (*Response, error) { } else { body = bytes.NewReader([]byte{}) } + // aws-chunked bodies are framed and length-checked by the chunk readers + // (the ones implementing middlewares.ChecksumReader). A plain body has + // nothing but Content-Length to tell a finished upload from an aborted one. + // + // Use the raw Content-Length header, not contentLength: that variable may + // have been replaced by X-Amz-Decoded-Content-Length above, which describes + // the DECODED size. That header only applies to aws-chunked payloads, and + // those skip this wrapper anyway. AWS S3 ignores it on a plain body and + // stores Content-Length bytes, so checking against the decoded value would + // reject a complete upload. + if _, chunked := body.(middlewares.ChecksumReader); !chunked { + if raw, cerr := strconv.ParseInt(ctx.Get("Content-Length"), 10, 64); cerr == nil && raw > 0 { + body = utils.NewContentLengthReader(body, raw) + } + } ifMatch, ifNoneMatch := utils.ParsePreconditionMatchHeaders(ctx) diff --git a/s3api/utils/content-length-reader.go b/s3api/utils/content-length-reader.go new file mode 100644 index 00000000..db8b45d2 --- /dev/null +++ b/s3api/utils/content-length-reader.go @@ -0,0 +1,53 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package utils + +import ( + "errors" + "io" + + "github.com/versity/versitygw/debuglogger" + "github.com/versity/versitygw/s3err" +) + +// ContentLengthReader turns an EOF that arrives before Content-Length bytes +// were read into an IncompleteBody error. +// +// fasthttp's request stream reports a client that closed the connection in +// the middle of a fixed-length body as a plain io.EOF (only the chunked +// transfer-encoding path is converted to io.ErrUnexpectedEOF), and io.Copy +// treats io.EOF as a normal end of stream. Without this check an aborted +// upload is committed as a complete, shorter object. aws-chunked bodies do +// not need this: their chunk readers already validate the framing. +type ContentLengthReader struct { + r io.Reader + remaining int64 +} + +func NewContentLengthReader(r io.Reader, contentLength int64) *ContentLengthReader { + return &ContentLengthReader{r: r, remaining: contentLength} +} + +func (cr *ContentLengthReader) Read(p []byte) (int, error) { + n, err := cr.r.Read(p) + cr.remaining -= int64(n) + if cr.remaining > 0 && (errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF)) { + debuglogger.Logf("request body ended %v bytes short of Content-Length", cr.remaining) + return n, s3err.GetAPIError(s3err.ErrIncompleteBody) + } + return n, err +} + +var _ io.Reader = &ContentLengthReader{} diff --git a/s3api/utils/content-length-reader_test.go b/s3api/utils/content-length-reader_test.go new file mode 100644 index 00000000..89bb7445 --- /dev/null +++ b/s3api/utils/content-length-reader_test.go @@ -0,0 +1,170 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package utils + +import ( + "errors" + "io" + "strings" + "testing" + + "github.com/versity/versitygw/s3err" +) + +// eofWithDataReader returns its payload and reports io.EOF together with the +// final bytes, the way a connection that was closed mid-body surfaces through +// fasthttp's request stream. +type eofWithDataReader struct { + data []byte + pos int +} + +func (r *eofWithDataReader) Read(p []byte) (int, error) { + n := copy(p, r.data[r.pos:]) + r.pos += n + if r.pos >= len(r.data) { + return n, io.EOF + } + return n, nil +} + +// oneByteReader hands out a single byte per call so the shortfall is only +// known on the very last Read. +type oneByteReader struct { + data []byte + pos int +} + +func (r *oneByteReader) Read(p []byte) (int, error) { + if r.pos >= len(r.data) { + return 0, io.EOF + } + p[0] = r.data[r.pos] + r.pos++ + return 1, nil +} + +func TestContentLengthReader(t *testing.T) { + incomplete := s3err.GetAPIError(s3err.ErrIncompleteBody) + + for _, tt := range []struct { + name string + body string + contentLength int64 + newReader func(string) io.Reader + wantErr error + }{ + { + name: "complete body", + body: "hello world", + contentLength: 11, + wantErr: nil, + }, + { + name: "empty body", + body: "", + contentLength: 0, + wantErr: nil, + }, + { + name: "truncated body", + body: "hel", + contentLength: 11, + wantErr: incomplete, + }, + { + name: "truncated body, EOF with final bytes", + body: "hel", + contentLength: 11, + newReader: func(s string) io.Reader { return &eofWithDataReader{data: []byte(s)} }, + wantErr: incomplete, + }, + { + name: "truncated body, one byte per read", + body: "hel", + contentLength: 11, + newReader: func(s string) io.Reader { return &oneByteReader{data: []byte(s)} }, + wantErr: incomplete, + }, + { + name: "empty body but length announced", + body: "", + contentLength: 5, + wantErr: incomplete, + }, + { + // Should not be reachable through fasthttp, but the reader must + // not invent an error when more arrives than was announced. + name: "body longer than Content-Length", + body: "hello world", + contentLength: 5, + wantErr: nil, + }, + } { + t.Run(tt.name, func(t *testing.T) { + newReader := tt.newReader + if newReader == nil { + newReader = func(s string) io.Reader { return strings.NewReader(s) } + } + + got, err := io.ReadAll(NewContentLengthReader(newReader(tt.body), tt.contentLength)) + + if string(got) != tt.body { + t.Errorf("data: got %q, want %q", string(got), tt.body) + } + if tt.wantErr == nil { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + return + } + + var apiErr s3err.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("got %v, want %v", err, tt.wantErr) + } + if apiErr.Code != "IncompleteBody" { + t.Fatalf("got code %q, want IncompleteBody", apiErr.Code) + } + }) + } +} + +// A non-EOF error from the wrapped reader must reach the caller unchanged: +// a signed payload whose body ends early already fails with +// ContentSHA256Mismatch from the checksum reader, and that error is the one +// the client should see. +func TestContentLengthReaderPassesThroughOtherErrors(t *testing.T) { + want := s3err.GetAPIError(s3err.ErrContentSHA256Mismatch) + r := NewContentLengthReader(&failingReader{err: want}, 100) + + _, err := io.ReadAll(r) + + var apiErr s3err.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("got %v, want %v", err, want) + } + if apiErr.Code != want.Code { + t.Fatalf("got code %q, want %q", apiErr.Code, want.Code) + } +} + +type failingReader struct { + err error +} + +func (r *failingReader) Read(p []byte) (int, error) { + return 0, r.err +} diff --git a/tests/integration/PutObject.go b/tests/integration/PutObject.go index d2622d3b..50f837e0 100644 --- a/tests/integration/PutObject.go +++ b/tests/integration/PutObject.go @@ -1330,3 +1330,141 @@ func PutObject_false_negative_object_names(s *S3Conf) error { return nil }) } + +// abortedBodyReader yields `sent` bytes and then fails, so the transport tears +// the connection down mid-body. That is what a client that dies or cancels +// looks like on the wire. +type abortedBodyReader struct { + remaining int +} + +func (r *abortedBodyReader) Read(p []byte) (int, error) { + if r.remaining <= 0 { + return 0, fmt.Errorf("connection aborted by test") + } + n := len(p) + if n > r.remaining { + n = r.remaining + } + for i := 0; i < n; i++ { + p[i] = 'a' + } + r.remaining -= n + return n, nil +} + +// putObjectAborted announces `declared` bytes, sends `sent`, then drops the +// connection. It returns no error: the request is expected to fail. +func putObjectAborted(s *S3Conf, bucket, obj string, declared, sent int, extra map[string]string) { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodPut, + fmt.Sprintf("%s/%s/%s", s.endpoint, bucket, obj), &abortedBodyReader{remaining: sent}) + if err != nil { + return + } + req.ContentLength = int64(declared) + req.Header.Set("x-amz-content-sha256", "UNSIGNED-PAYLOAD") + for k, v := range extra { + req.Header.Set(k, v) + } + + signer := v4.NewSigner() + if err := signer.SignHTTP(req.Context(), + aws.Credentials{AccessKeyID: s.awsID, SecretAccessKey: s.awsSecret}, + req, "UNSIGNED-PAYLOAD", "s3", s.awsRegion, time.Now()); err != nil { + return + } + + resp, err := s.httpClient.Do(req) + if err == nil && resp != nil { + resp.Body.Close() + } +} + +// PutObject_aborted_plain_body checks that a plain (non-aws-chunked) PUT whose +// body ends before Content-Length does not become a readable object. +// +// Real S3 rejects the request and the key stays absent. The posix backend used +// to truncate the object to the bytes received and link it into place, so the +// aborted upload showed up as a complete - but shorter - object. +func PutObject_aborted_plain_body(s *S3Conf) error { + testName := "PutObject_aborted_plain_body" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "aborted-plain" + putObjectAborted(s, bucket, obj, 65536, 20000, nil) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err == nil { + return fmt.Errorf("expected the aborted upload to leave no object, but %v exists", obj) + } + return nil + }) +} + +// PutObject_plain_body_with_decoded_length checks that a COMPLETE plain upload +// still succeeds when it happens to carry X-Amz-Decoded-Content-Length. +// +// AWS S3 ignores that header on a plain body and stores Content-Length bytes. +// The length check must therefore use Content-Length, not the decoded value - +// otherwise a complete upload is rejected with IncompleteBody. +func PutObject_plain_body_with_decoded_length(s *S3Conf) error { + testName := "PutObject_plain_body_with_decoded_length" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "plain-with-decoded-length" + data := "hello world" + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + req, err := http.NewRequestWithContext(ctx, http.MethodPut, + fmt.Sprintf("%s/%s/%s", s.endpoint, bucket, obj), strings.NewReader(data)) + if err != nil { + cancel() + return err + } + req.Header.Set("x-amz-content-sha256", "UNSIGNED-PAYLOAD") + // Larger than Content-Length on purpose: this is the shape that used to + // be rejected once the body was checked against the decoded value. + req.Header.Set("X-Amz-Decoded-Content-Length", "99999") + + signer := v4.NewSigner() + if err := signer.SignHTTP(req.Context(), + aws.Credentials{AccessKeyID: s.awsID, SecretAccessKey: s.awsSecret}, + req, "UNSIGNED-PAYLOAD", "s3", s.awsRegion, time.Now()); err != nil { + cancel() + return fmt.Errorf("failed to sign the request: %w", err) + } + + resp, err := s.httpClient.Do(req) + cancel() + if err != nil { + return fmt.Errorf("send request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("expected the response status code to be %v, instead got %v", + http.StatusOK, resp.StatusCode) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + if out.ContentLength == nil || *out.ContentLength != int64(len(data)) { + return fmt.Errorf("expected the stored object to be %v bytes, instead got %v", + len(data), out.ContentLength) + } + return nil + }) +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 8c932b7d..ffdd1da7 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -168,6 +168,8 @@ func TestGetBucketLocation(ts *TestState) { func TestPutObject(ts *TestState) { ts.Run(PutObject_non_existing_bucket) ts.Run(PutObject_special_chars) + ts.Run(PutObject_aborted_plain_body) + ts.Run(PutObject_plain_body_with_decoded_length) ts.Run(PutObject_tagging) ts.Run(PutObject_missing_object_lock_retention_config) ts.Run(PutObject_with_object_lock) @@ -2122,6 +2124,7 @@ func TestUnsignedStreaminPayloadTrailer(ts *TestState) { ts.Run(UnsignedStreamingPayloadTrailer_multiple_checksum_headers) ts.Run(UnsignedStreamingPayloadTrailer_sdk_algo_and_trailer_mismatch) ts.Run(UnsignedStreamingPayloadTrailer_incomplete_body) + ts.Run(UnsignedStreamingPayloadTrailer_aborted_connection) ts.Run(UnsignedStreamingPayloadTrailer_invalid_chunk_size) ts.Run(UnsignedStreamingPayloadTrailer_content_length_payload_size_mismatch) ts.Run(UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme) @@ -2868,6 +2871,8 @@ func GetIntTests() IntTests { "GetBucketLocation_no_access": GetBucketLocation_no_access, "PutObject_non_existing_bucket": PutObject_non_existing_bucket, "PutObject_special_chars": PutObject_special_chars, + "PutObject_aborted_plain_body": PutObject_aborted_plain_body, + "PutObject_plain_body_with_decoded_length": PutObject_plain_body_with_decoded_length, "PutObject_tagging": PutObject_tagging, "PutObject_success": PutObject_success, "PutObject_default_content_type": PutObject_default_content_type, @@ -3559,6 +3564,7 @@ func GetIntTests() IntTests { "RouterCopySourceNotAllowed": RouterCopySourceNotAllowed, "RouterListVersionsWithKey": RouterListVersionsWithKey, "UnsignedStreaminPayloadTrailer_malformed_trailer": UnsignedStreaminPayloadTrailer_malformed_trailer, + "UnsignedStreamingPayloadTrailer_aborted_connection": UnsignedStreamingPayloadTrailer_aborted_connection, "UnsignedStreamingPayloadTrailer_missing_invalid_dec_content_length": UnsignedStreamingPayloadTrailer_missing_invalid_dec_content_length, "UnsignedStreamingPayloadTrailer_invalid_trailing_checksum": UnsignedStreamingPayloadTrailer_invalid_trailing_checksum, "UnsignedStreamingPayloadTrailer_incorrect_trailing_checksum": UnsignedStreamingPayloadTrailer_incorrect_trailing_checksum, diff --git a/tests/integration/unsigned_streaming_payload_trailer.go b/tests/integration/unsigned_streaming_payload_trailer.go index 505c30f2..ca93953e 100644 --- a/tests/integration/unsigned_streaming_payload_trailer.go +++ b/tests/integration/unsigned_streaming_payload_trailer.go @@ -6,7 +6,10 @@ import ( "fmt" "net/http" "strings" + "time" + "github.com/aws/aws-sdk-go-v2/aws" + v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/versity/versitygw/s3err" @@ -611,3 +614,99 @@ func UnsignedStreamingPayloadTrailer_not_allowed(s *S3Conf) error { return nil }) } + +// abortedChunkReader serves the first n bytes of a chunk-framed payload and +// then fails, so the transport tears the connection down mid-body. +type abortedChunkReader struct { + data []byte + pos int +} + +func (r *abortedChunkReader) Read(p []byte) (int, error) { + if r.pos >= len(r.data) { + return 0, fmt.Errorf("connection aborted by test") + } + n := copy(p, r.data[r.pos:]) + r.pos += n + return n, nil +} + +// UnsignedStreamingPayloadTrailer_aborted_connection checks that an aws-chunked +// upload whose connection dies in the middle of a chunk leaves no object. +// +// UnsignedStreamingPayloadTrailer_incomplete_body already covers malformed and +// truncated framing, but every one of those is a COMPLETE request: the body is +// short, Content-Length agrees with it, and the chunk reader rejects what it +// parses. This one is the other shape - the framing is valid and the bytes +// simply stop arriving, which is what a client that dies or cancels looks like +// on the wire. That is the shape the plain path got wrong. +// +// The chunk readers already handled it, so this is a regression guard: the +// Content-Length check added for plain bodies must not change this path. +// +// NOTE: the request must really take the chunked path. The gateway decides +// that from x-amz-content-sha256 alone, so an UNSIGNED-PAYLOAD request with +// Content-Encoding: aws-chunked stays on the plain path - a test written that +// way silently duplicates the plain one instead of covering this. +func UnsignedStreamingPayloadTrailer_aborted_connection(s *S3Conf) error { + testName := "UnsignedStreamingPayloadTrailer_aborted_connection" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + object := "aborted-streaming" + + decoded, payload, err := constructUnsignedPaylod(65536) + if err != nil { + return fmt.Errorf("failed to construct the payload: %w", err) + } + full := append(payload, []byte("0\r\nx-amz-checksum-crc64nvme:dPVWc2vU1+Q=\r\n\r\n")...) + + // Stop well inside the first chunk: the header has been parsed, the + // data has not finished arriving. + cut := len(full) / 4 + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + req, err := http.NewRequestWithContext(ctx, http.MethodPut, + fmt.Sprintf("%s/%s/%s", s.endpoint, bucket, object), + &abortedChunkReader{data: full[:cut]}) + if err != nil { + cancel() + return fmt.Errorf("failed to create a request: %w", err) + } + // Announce the whole framed payload, send a quarter of it. + req.ContentLength = int64(len(full)) + req.Header.Set("x-amz-content-sha256", "STREAMING-UNSIGNED-PAYLOAD-TRAILER") + req.Header.Set("x-amz-trailer", "x-amz-checksum-crc64nvme") + req.Header.Set("x-amz-decoded-content-length", fmt.Sprintf("%v", decoded)) + + signer := v4.NewSigner() + if err := signer.SignHTTP(req.Context(), + aws.Credentials{AccessKeyID: s.awsID, SecretAccessKey: s.awsSecret}, + req, "STREAMING-UNSIGNED-PAYLOAD-TRAILER", "s3", s.awsRegion, time.Now()); err != nil { + cancel() + return fmt.Errorf("failed to sign the request: %w", err) + } + + // The request is expected to fail: either the gateway answers with an + // error or the connection is gone. Both are fine - what matters is the + // object below. + resp, doErr := s.httpClient.Do(req) + cancel() + if doErr == nil && resp != nil { + resp.Body.Close() + if resp.StatusCode < 300 { + return fmt.Errorf("expected the aborted upload to fail, got %v", resp.StatusCode) + } + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &object, + }) + cancel() + if err == nil { + return fmt.Errorf("expected the aborted upload to leave no object, but %v exists", object) + } + + return nil + }) +}