diff --git a/backend/common.go b/backend/common.go index 3fcf26dc..f7bf15aa 100644 --- a/backend/common.go +++ b/backend/common.go @@ -425,8 +425,9 @@ func isValidTagComponent(str string) bool { } // ComputeMultipartETagFromPartETags computes the S3 multipart ETag -// ("-") from the completed-part ETags. -func ComputeMultipartETagFromPartETags(parts []types.CompletedPart) (string, error) { +// ("-") from the completed-part ETags. uploadID is only +// used to report the upload in an InvalidPart error. +func ComputeMultipartETagFromPartETags(uploadID string, parts []types.CompletedPart) (string, error) { var partsEtagBytes []byte for _, part := range parts { if part.ETag == nil { @@ -437,7 +438,7 @@ func ComputeMultipartETagFromPartETags(parts []types.CompletedPart) (string, err } bts, err := getEtagBytes(*part.ETag) if err != nil { - return "", s3err.GetAPIError(s3err.ErrInvalidPart) + return "", s3err.GetInvalidPartErr(uploadID, *part.PartNumber, *part.ETag) } partsEtagBytes = append(partsEtagBytes, bts...) } diff --git a/backend/common_test.go b/backend/common_test.go index af398c56..eb0213d8 100644 --- a/backend/common_test.go +++ b/backend/common_test.go @@ -16,11 +16,15 @@ package backend import ( "bytes" + "crypto/md5" + "encoding/hex" "encoding/json" "errors" + "fmt" "reflect" "testing" + "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/versity/versitygw/s3err" ) @@ -330,3 +334,50 @@ func TestParseCopySource(t *testing.T) { }) } } + +func TestComputeMultipartETagFromPartETags(t *testing.T) { + etag1 := "5d41402abc4b2a76b9719d911017c592" + etag2 := "7d793037a0760186574b0282f2f435e7" + raw1, _ := hex.DecodeString(etag1) + raw2, _ := hex.DecodeString(etag2) + sum := md5.Sum(append(raw1, raw2...)) + wantETag := fmt.Sprintf("\"%s-2\"", hex.EncodeToString(sum[:])) + + part := func(n int32, etag string) types.CompletedPart { + return types.CompletedPart{PartNumber: &n, ETag: &etag} + } + + tests := []struct { + name string + parts []types.CompletedPart + wantETag string + wantErr error + }{ + { + name: "quoted and unquoted part etags", + parts: []types.CompletedPart{part(1, "\""+etag1+"\""), part(2, etag2)}, + wantETag: wantETag, + }, + { + name: "missing part etag", + parts: []types.CompletedPart{{PartNumber: new(int32)}}, + wantErr: s3err.GetAPIError(s3err.ErrMalformedXML), + }, + { + name: "invalid part etag reports the upload, part and etag", + parts: []types.CompletedPart{part(1, etag1), part(2, "\"invalidETag\"")}, + wantErr: s3err.GetInvalidPartErr("upload-id", 2, "\"invalidETag\""), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + etag, err := ComputeMultipartETagFromPartETags("upload-id", tt.parts) + if !reflect.DeepEqual(err, tt.wantErr) { + t.Fatalf("expected error %#v, got %#v", tt.wantErr, err) + } + if etag != tt.wantETag { + t.Fatalf("expected etag %q, got %q", tt.wantETag, etag) + } + }) + } +} diff --git a/backend/posix/posix.go b/backend/posix/posix.go index f4c4dc08..fc815cfd 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -2371,7 +2371,7 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C // Compute the default multipart ETag token used for claim naming. // In standard mode this is the S3-compatible multipart MD5 ETag; in // dataIntegrityEtag mode it may fall back to a deterministic claim token. - multipartClaimToken, err := backend.ComputeMultipartETagFromPartETags(parts) + multipartClaimToken, err := backend.ComputeMultipartETagFromPartETags(uploadID, parts) if err != nil { if !p.dataIntegrityEtag { return res, "", err diff --git a/tests/integration/CompleteMultipartUpload.go b/tests/integration/CompleteMultipartUpload.go index 83052c24..9a577b3d 100644 --- a/tests/integration/CompleteMultipartUpload.go +++ b/tests/integration/CompleteMultipartUpload.go @@ -190,6 +190,47 @@ func CompleteMultipartUpload_invalid_ETag(s *S3Conf) error { return nil }) } + +func CompleteMultipartUpload_invalid_ETag_error_fields(s *S3Conf) error { + testName := "CompleteMultipartUpload_invalid_ETag_error_fields" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + out, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.UploadPart(ctx, &s3.UploadPartInput{ + Bucket: &bucket, + Key: &obj, + UploadId: out.UploadId, + PartNumber: getPtr(int32(1)), + Body: bytes.NewReader([]byte("dummy")), + }) + cancel() + if err != nil { + return err + } + + // the sdk doesn't expose the InvalidPart error fields, so the + // request is signed and sent by hand + body := []byte(`invalidETag1`) + req, err := createSignedReq(http.MethodPost, s.endpoint, + fmt.Sprintf("%v/%v?uploadId=%v", bucket, obj, *out.UploadId), + s.awsID, s.awsSecret, "s3", s.awsRegion, "", body, time.Now(), nil) + if err != nil { + return err + } + + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + + return checkHTTPResponseApiErr(resp, s3err.GetInvalidPartErr(*out.UploadId, 1, "invalidETag")) + }) +} func CompleteMultipartUpload_invalid_checksum_type(s *S3Conf) error { testName := "CompleteMultipartUpload_invalid_checksum_type" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 32528b72..066d9b4a 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -588,6 +588,7 @@ func TestCompleteMultipartUpload(ts *TestState) { ts.Run(CompleteMultipartUpload_default_content_type) ts.Run(CompleteMultipartUpload_overwrite_resets_attributes) ts.Run(CompleteMultipartUpload_invalid_ETag) + ts.Run(CompleteMultipartUpload_invalid_ETag_error_fields) ts.Run(CompleteMultipartUpload_small_upload_size) ts.Run(CompleteMultipartUpload_empty_parts) ts.Run(CompleteMultipartUpload_missing_part_fields) @@ -3356,6 +3357,7 @@ func GetIntTests() IntTests { "CompleteMultipartUpload_default_content_type": CompleteMultipartUpload_default_content_type, "CompleteMultipartUpload_overwrite_resets_attributes": CompleteMultipartUpload_overwrite_resets_attributes, "CompleteMultipartUpload_invalid_ETag": CompleteMultipartUpload_invalid_ETag, + "CompleteMultipartUpload_invalid_ETag_error_fields": CompleteMultipartUpload_invalid_ETag_error_fields, "CompleteMultipartUpload_small_upload_size": CompleteMultipartUpload_small_upload_size, "CompleteMultipartUpload_empty_parts": CompleteMultipartUpload_empty_parts, "CompleteMultipartUpload_missing_part_fields": CompleteMultipartUpload_missing_part_fields, diff --git a/tests/test_rest_multipart.sh b/tests/test_rest_multipart.sh index 27bb5ec1..bcbfccc5 100755 --- a/tests/test_rest_multipart.sh +++ b/tests/test_rest_multipart.sh @@ -65,9 +65,6 @@ source ./tests/drivers/upload_part/upload_part_rest.sh # tags: curl,multipart,CompleteMultipartUpload,invalid-header,ETag @test "REST - complete upload - invalid part" { - if [ "$DIRECT" != "true" ]; then - skip "https://github.com/versity/versitygw/issues/1008" - fi run get_bucket_name "$BUCKET_ONE_NAME" assert_success bucket_name="$output"