diff --git a/cmd/object-copy-federation_test.go b/cmd/object-copy-federation_test.go index e4f701ee2..1d1e4462b 100644 --- a/cmd/object-copy-federation_test.go +++ b/cmd/object-copy-federation_test.go @@ -283,7 +283,11 @@ func TestAPIFederatedCopyObjectRejectsInvalidRemoteChecksum(t *testing.T) { objAPITest: func(obj ObjectLayer, instanceType, bucket string, router http.Handler, credentials auth.Credentials, t *testing.T) { data := []byte("remote checksum response fixture") putCopyChecksumSource(t, router, credentials, bucket, "source", data, nil) - for _, value := range []string{"", "invalid-base64", "YQ=="} { + // "" missing, "invalid-base64" unparseable, "YQ==" wrong digest + // length, and "-0" a multipart-marked value that a single + // forwarded PutObject must never yield (it would mislabel the + // destination as composite). + for _, value := range []string{"", "invalid-base64", "YQ==", mustChecksum(t, hash.ChecksumCRC32, data) + "-0"} { t.Run("checksum="+value, func(t *testing.T) { remoteBucket, _, cleanup := setupCopyObjectFederation(t, obj, router, instanceType, bucket, func(header http.Header) { header.Set(xhttp.AmzChecksumCRC32, value) @@ -344,3 +348,92 @@ func testAPIFederatedCopyObjectChecksumIsBoundToWrite(objectAPI ObjectLayer, ins t.Fatalf("%s: response carried checksums beyond the requested CRC32: %s", instanceType, rec.Body.String()) } } + +// TestAPIFederatedCopyObjectEmptySource guards the empty-body regression: a +// checksum-less object gains the S3 default CRC-64NVME, but minio-go streams no +// trailing checksum for a 0-byte body, so the remote returned none, the bind +// found nothing, and every empty-object federated copy 500'd. An empty source +// must now copy with 200 and carry the empty-content checksum, both when a +// checksum is requested explicitly and via the default. +func TestAPIFederatedCopyObjectEmptySource(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIFederatedCopyObjectEmptySource, + endpoints: []string{"CopyObject", "PutObject", "HeadObject", "GetObject"}, + }) +} + +func testAPIFederatedCopyObjectEmptySource(objectAPI ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + srcObject := "federation/empty-source" + putCopyChecksumSource(t, apiRouter, credentials, bucketName, srcObject, nil, nil) + + remoteBucket, _, cleanup := setupCopyObjectFederation(t, objectAPI, apiRouter, instanceType, bucketName) + defer cleanup() + + cases := []struct { + name string + typ hash.ChecksumType + explicit bool + }{ + {name: "explicit-CRC32", typ: hash.ChecksumCRC32, explicit: true}, + {name: "explicit-SHA256", typ: hash.ChecksumSHA256, explicit: true}, + // No requested algorithm: the S3 default CRC-64NVME still applies. + {name: "default-CRC64NVME", typ: hash.ChecksumCRC64NVME}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var headers map[string]string + if tc.explicit { + headers = map[string]string{xhttp.AmzChecksumAlgo: tc.typ.String()} + } + dstObject := "federation/empty-destination-" + tc.name + rec := federatedCopyRequest(t, apiRouter, credentials, bucketName, srcObject, remoteBucket, dstObject, headers) + if rec.Code != http.StatusOK { + t.Fatalf("%s: federated CopyObject of an empty source failed: %d %s", + instanceType, rec.Code, rec.Body.String()) + } + // The empty-content digest must be returned and persisted. + assertCopyChecksumResponse(t, rec, tc.typ, nil) + assertCopyChecksum(t, objectAPI, remoteBucket, dstObject, tc.typ, nil, false, nil) + }) + } +} + +// TestAPIFederatedCopyObjectInheritedChecksum guards that a full-object checksum +// already stored on the source is preserved across a federated copy that +// requests no algorithm. That checksum sets dstOpts.WantChecksum (not +// WantServerSideChecksumType), which the federated branch previously ignored, +// silently dropping the checksum the local path keeps. +func TestAPIFederatedCopyObjectInheritedChecksum(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIFederatedCopyObjectInheritedChecksum, + endpoints: []string{"CopyObject", "PutObject", "HeadObject", "GetObject"}, + }) +} + +func testAPIFederatedCopyObjectInheritedChecksum(objectAPI ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + data := []byte("abc") + want := mustChecksum(t, hash.ChecksumCRC32, data) // "NSRBwg==" + srcObject := "federation/inherited-checksum-source" + putCopyChecksumSource(t, apiRouter, credentials, bucketName, srcObject, data, + map[string]string{xhttp.AmzChecksumCRC32: want}) + + remoteBucket, _, cleanup := setupCopyObjectFederation(t, objectAPI, apiRouter, instanceType, bucketName) + defer cleanup() + + // No algorithm header: the source's stored CRC32 must survive the copy. + dstObject := "federation/inherited-checksum-destination" + rec := federatedCopyRequest(t, apiRouter, credentials, bucketName, srcObject, remoteBucket, dstObject, nil) + if rec.Code != http.StatusOK { + t.Fatalf("%s: federated CopyObject failed: %d %s", instanceType, rec.Code, rec.Body.String()) + } + assertCopyChecksumResponse(t, rec, hash.ChecksumCRC32, data) + assertCopyChecksum(t, objectAPI, remoteBucket, dstObject, hash.ChecksumCRC32, data, false, nil) +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index cbf4b3d14..b434a4593 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -1938,16 +1938,42 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re ServerSideEncryption: dstOpts.ServerSideEncryption, UserTags: tag.ToMap(), } - // When a server-side checksum was requested (explicitly, inherited from - // the source, or the S3 default for a checksum-less object), the local - // path computes, persists and returns it; the federated path must do the - // same. Ask the remote to compute and persist that checksum by streaming - // it as a trailing checksum, so the forwarded write's response carries - // the value back to us. Without this the federated copy silently returns - // an empty checksum (#99). + // The destination must carry the same checksum the local path would + // produce; the federated path has the remote compute, validate, persist + // and return it. Without this the federated copy silently returns an + // empty checksum (#99). wantChecksumType := dstOpts.WantServerSideChecksumType - if wantChecksumType.IsSet() { - opts.Checksum = federatedChecksumType(wantChecksumType) + var checksumHeaderValue string + switch { + case wantChecksumType.IsSet(): + if srcInfo.Size == 0 { + // minio-go streams no trailing checksum for an empty body, so the + // remote would compute none, the bind below would fail, and every + // empty-object federated copy would 500. Forward the empty-content + // digest as an ordinary checksum header instead, so the remote + // validates, persists and returns it (parity with the local path, + // e.g. CRC32 "AAAAAA=="). + if empty := hash.NewChecksumFromData(wantChecksumType, nil); empty != nil { + checksumHeaderValue = empty.Encoded + } + } else { + // Stream a trailing checksum of the requested type so the remote + // computes and persists it over the copied bytes. + opts.Checksum = federatedChecksumType(wantChecksumType) + } + case dstOpts.WantChecksum != nil && dstOpts.WantChecksum.Type.IsSet(): + // A full-object checksum inherited from a checksum-bearing source: the + // local path persists it without recomputation (only multipart + // composite sources are promoted to WantServerSideChecksumType above, + // so this is never composite). Forward the value as an ordinary + // checksum header so the remote validates and persists it, and bind + // the returned value below. WantChecksum.Encoded is always a plain + // digest, never a "-N" multipart form. + wantChecksumType = dstOpts.WantChecksum.Type.Base() + checksumHeaderValue = dstOpts.WantChecksum.Encoded + } + if checksumHeaderValue != "" { + opts.UserMetadata[wantChecksumType.Key()] = checksumHeaderValue } remoteObjInfo, rerr := core.PutObject(ctx, dstBucket, dstObject, srcInfo.Reader, srcInfo.Size, "", "", opts) @@ -1956,12 +1982,19 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re return } objInfo.UserDefined = cloneMSS(opts.UserMetadata) + // A forwarded checksum header is a request detail, not object metadata. + if checksumHeaderValue != "" { + delete(objInfo.UserDefined, wantChecksumType.Key()) + } objInfo.ETag = remoteObjInfo.ETag objInfo.ModTime = remoteObjInfo.LastModified - // Do not acknowledge a requested checksum the remote did not return. + // Bind the checksum the remote computed for this exact write. A single + // forwarded PutObject must yield a full-object digest, so reject a + // missing, malformed, or multipart-marked ("-N") value rather than + // mislabel the destination as composite. if wantChecksumType.IsSet() { cs := hash.NewChecksumWithType(wantChecksumType, federatedChecksumValue(wantChecksumType, remoteObjInfo)) - if cs == nil { + if cs == nil || cs.Type.Is(hash.ChecksumMultipart) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrInternalError), r.URL) return }