diff --git a/s3api/controllers/object-put.go b/s3api/controllers/object-put.go index 2b036a40..c07dd072 100644 --- a/s3api/controllers/object-put.go +++ b/s3api/controllers/object-put.go @@ -246,12 +246,7 @@ func (c S3ApiController) UploadPart(ctx fiber.Ctx) (*Response, error) { if contentLengthStr == "" { contentLengthStr = "0" } - // Use decoded content length if available because the - // middleware will decode the chunked transfer encoding decodedLength := ctx.Get("X-Amz-Decoded-Content-Length") - if decodedLength != "" { - contentLengthStr = decodedLength - } err := c.verifyAccess(ctx, auth.AccessOptions{ @@ -280,7 +275,6 @@ func (c S3ApiController) UploadPart(ctx fiber.Ctx) (*Response, error) { }, }, s3err.GetInvalidArgumentErr(s3err.InvalidArgPartNumber, ctx.Query("partNumber")) } - contentLength, err := strconv.ParseInt(contentLengthStr, 10, 64) if err != nil { debuglogger.Logf("error parsing content length %q: %v", contentLengthStr, err) @@ -308,20 +302,23 @@ func (c S3ApiController) UploadPart(ctx fiber.Ctx) (*Response, error) { } else { body = bytes.NewReader([]byte{}) } + _, chunked := body.(middlewares.ChecksumReader) + if chunked && decodedLength != "" { + contentLength, err = strconv.ParseInt(decodedLength, 10, 64) + if err != nil { + debuglogger.Logf("error parsing content length %q: %v", decodedLength, err) + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, s3err.GetAPIError(s3err.ErrInvalidRequest) + } + } // 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) - } + if !chunked && contentLength > 0 { + body = utils.NewContentLengthReader(body, contentLength) } res, err := c.be.UploadPart(ctx.RequestCtx(), @@ -723,12 +720,7 @@ func (c S3ApiController) PutObject(ctx fiber.Ctx) (*Response, error) { if contentLengthStr == "" { contentLengthStr = "0" } - // Use decoded content length if available because the - // middleware will decode the chunked transfer encoding decodedLength := ctx.Get("X-Amz-Decoded-Content-Length") - if decodedLength != "" { - contentLengthStr = decodedLength - } actions := []auth.Action{auth.PutObjectAction} if tagging != "" { @@ -787,7 +779,6 @@ func (c S3ApiController) PutObject(ctx fiber.Ctx) (*Response, error) { }, }, err } - contentLength, err := strconv.ParseInt(contentLengthStr, 10, 64) if err != nil { debuglogger.Logf("error parsing content length %q: %v", contentLengthStr, err) @@ -823,20 +814,23 @@ func (c S3ApiController) PutObject(ctx fiber.Ctx) (*Response, error) { } else { body = bytes.NewReader([]byte{}) } + _, chunked := body.(middlewares.ChecksumReader) + if chunked && decodedLength != "" { + contentLength, err = strconv.ParseInt(decodedLength, 10, 64) + if err != nil { + debuglogger.Logf("error parsing content length %q: %v", decodedLength, err) + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, s3err.GetAPIError(s3err.ErrInvalidRequest) + } + } // 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) - } + if !chunked && contentLength > 0 { + body = utils.NewContentLengthReader(body, contentLength) } ifMatch, ifNoneMatch := utils.ParsePreconditionMatchHeaders(ctx) diff --git a/s3api/controllers/object-put_test.go b/s3api/controllers/object-put_test.go index d93e7e23..14611a2d 100644 --- a/s3api/controllers/object-put_test.go +++ b/s3api/controllers/object-put_test.go @@ -17,6 +17,7 @@ package controllers import ( "context" "encoding/xml" + "io" "strings" "testing" "time" @@ -32,6 +33,13 @@ import ( "github.com/versity/versitygw/s3response" ) +type checksumBodyReader struct { + io.Reader +} + +func (checksumBodyReader) Algorithm() string { return "" } +func (checksumBodyReader) Checksum() string { return "" } + func TestS3ApiController_PutObjectTagging(t *testing.T) { validTaggingBody, err := xml.Marshal( s3response.Tagging{ @@ -496,7 +504,12 @@ func TestS3ApiController_UploadPart(t *testing.T) { { name: "invalid content length", input: testInput{ - locals: defaultLocals, + locals: map[utils.ContextKey]any{ + utils.ContextKeyIsRoot: true, + utils.ContextKeyParsedAcl: auth.ACL{Owner: "root"}, + utils.ContextKeyAccount: auth.Account{Access: "root", Role: auth.RoleAdmin}, + utils.ContextKeyBodyReader: checksumBodyReader{strings.NewReader("")}, + }, headers: map[string]string{ "X-Amz-Decoded-Content-Length": "invalid_cLength", }, @@ -631,6 +644,52 @@ func TestS3ApiController_UploadPart(t *testing.T) { } } +func TestS3ApiController_UploadPartPlainBodyUsesContentLength(t *testing.T) { + be := &BackendMock{ + UploadPartFunc: func(_ context.Context, input *s3.UploadPartInput) (*s3.UploadPartOutput, error) { + assert.Equal(t, int64(5), *input.ContentLength) + body, err := io.ReadAll(input.Body) + assert.NoError(t, err) + assert.Equal(t, "hello", string(body)) + return &s3.UploadPartOutput{}, nil + }, + GetBucketPolicyFunc: func(_ context.Context, _ string) ([]byte, error) { + return nil, s3err.GetAPIError(s3err.ErrAccessDenied) + }, + } + ctrl := S3ApiController{be: be, mpMaxParts: 10000} + + testController(t, ctrl.UploadPart, &Response{ + Headers: map[string]*string{ + "ETag": nil, + "x-amz-checksum-crc32": nil, + "x-amz-checksum-crc32c": nil, + "x-amz-checksum-crc64nvme": nil, + "x-amz-checksum-sha1": nil, + "x-amz-checksum-sha256": nil, + "x-amz-checksum-sha512": nil, + "x-amz-checksum-md5": nil, + "x-amz-checksum-xxhash64": nil, + "x-amz-checksum-xxhash3": nil, + "x-amz-checksum-xxhash128": nil, + }, + MetaOpts: &MetaOptions{BucketOwner: "root", ContentLength: 5}, + }, nil, ctxInputs{ + body: []byte("hello"), + locals: map[utils.ContextKey]any{ + utils.ContextKeyIsRoot: true, + utils.ContextKeyParsedAcl: auth.ACL{Owner: "root"}, + utils.ContextKeyAccount: auth.Account{Access: "root", Role: auth.RoleAdmin}, + utils.ContextKeyBodyReader: strings.NewReader("hello"), + }, + headers: map[string]string{ + "Content-Length": "5", + "X-Amz-Decoded-Content-Length": "99999", + }, + queries: map[string]string{"partNumber": "1", "uploadId": "upload"}, + }) +} + func TestS3ApiController_UploadPartCopy(t *testing.T) { tests := []struct { name string @@ -1309,7 +1368,12 @@ func TestS3ApiController_PutObject(t *testing.T) { { name: "invalid content length", input: testInput{ - locals: defaultLocals, + locals: map[utils.ContextKey]any{ + utils.ContextKeyIsRoot: true, + utils.ContextKeyParsedAcl: auth.ACL{Owner: "root"}, + utils.ContextKeyAccount: auth.Account{Access: "root", Role: auth.RoleAdmin}, + utils.ContextKeyBodyReader: checksumBodyReader{strings.NewReader("")}, + }, extraMockErr: s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotFound), headers: map[string]string{ "X-Amz-Decoded-Content-Length": "invalid_length", diff --git a/tests/integration/UploadPart.go b/tests/integration/UploadPart.go index 573d3baa..68977315 100644 --- a/tests/integration/UploadPart.go +++ b/tests/integration/UploadPart.go @@ -24,9 +24,12 @@ import ( "hash/crc32" "hash/crc64" "math/bits" + "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" @@ -502,6 +505,77 @@ func UploadPart_success(s *S3Conf) error { }) } +// UploadPart_plain_body_with_decoded_length checks that a complete plain part +// uses Content-Length rather than X-Amz-Decoded-Content-Length. The latter is +// only meaningful for aws-chunked uploads and must not preallocate a zero tail. +func UploadPart_plain_body_with_decoded_length(s *S3Conf) error { + testName := "UploadPart_plain_body_with_decoded_length" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "plain-part-with-decoded-length" + upload, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + + data := "hello" + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + req, err := http.NewRequestWithContext(ctx, http.MethodPut, + fmt.Sprintf("%s/%s/%s?uploadId=%s&partNumber=1", s.endpoint, bucket, obj, *upload.UploadId), + strings.NewReader(data)) + if err != nil { + cancel() + return fmt.Errorf("create upload part request: %w", err) + } + req.Header.Set("x-amz-content-sha256", "UNSIGNED-PAYLOAD") + 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("sign upload part request: %w", err) + } + + resp, err := s.httpClient.Do(req) + cancel() + if err != nil { + return fmt.Errorf("send upload part request: %w", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("expected upload part status %v, got %v", http.StatusOK, resp.StatusCode) + } + + partNumber := int32(1) + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: upload.UploadId, + MultipartUpload: &types.CompletedMultipartUpload{Parts: []types.CompletedPart{{ + ETag: aws.String(resp.Header.Get("ETag")), + PartNumber: &partNumber, + }}}, + }) + cancel() + if err != nil { + return fmt.Errorf("complete multipart upload: %w", err) + } + + 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 completed object to be %v bytes, got %v", len(data), out.ContentLength) + } + return nil + }) +} + func UploadPart_data_integrity_etag(s *S3Conf) error { testName := "UploadPart_data_integrity_etag" partNumber := int32(1) diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index ffdd1da7..92ea4ebe 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -474,6 +474,7 @@ func TestUploadPart(ts *TestState) { ts.Run(UploadPart_with_checksums_success) } ts.Run(UploadPart_success) + ts.Run(UploadPart_plain_body_with_decoded_length) ts.Run(UploadPart_etag_quoting_consistency) } @@ -2873,6 +2874,7 @@ func GetIntTests() IntTests { "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, + "UploadPart_plain_body_with_decoded_length": UploadPart_plain_body_with_decoded_length, "PutObject_tagging": PutObject_tagging, "PutObject_success": PutObject_success, "PutObject_default_content_type": PutObject_default_content_type,