fix: correct federated CopyObject checksum edge cases (#99 follow-ups)

Three residual checksum defects in the legacy etcd federation branch of
CopyObjectHandler, found by post-merge review of #157.

1. Empty-source 500 regression. A checksum-less object gains the S3 default
   CRC-64NVME (WantServerSideChecksumType is set), but minio-go streams no
   trailing checksum for a 0-byte body (contentLength == 0), so the remote
   computed none, federatedChecksumValue was empty, hash.NewChecksumWithType
   returned nil, and the handler returned 500 -- so every empty-object
   federated copy failed. For a 0-byte source, forward the empty-content digest
   as an ordinary checksum request header instead, so the remote validates,
   persists and returns it, matching the local path (e.g. CRC32 "AAAAAA==").

2. Inherited full-object checksum dropped. When the source already carries a
   full-object checksum, the local path sets dstOpts.WantChecksum, not
   WantServerSideChecksumType (only multipart-composite sources are promoted).
   The federated branch inspected only WantServerSideChecksumType, so a
   checksum-bearing source's checksum was silently discarded on a federated
   copy that requested no algorithm. Forward WantChecksum.Encoded (always a
   plain digest) as a checksum header so the remote validates and persists it,
   and bind the returned value, matching local persistence.

3. Multipart-suffixed remote value accepted. The bind accepted a value like
   "NSRBwg==-0": NewChecksumWithType parses the "-N" as ChecksumMultipart with
   WantParts 0 and the length-only validator passes, so the destination was
   returned as COMPOSITE. A single forwarded PutObject must yield a full-object
   digest, so reject a multipart-marked parsed value in addition to the
   existing nil (missing/malformed) rejection.

A forwarded checksum request header is stripped from objInfo.UserDefined so it
is not mistaken for object metadata.

Out of scope: the SSE federated-copy corruption (srcInfo.Reader/Size mismatch
for encrypted sources) predates this work and is filed separately.

New federated regressions cover empty source with requested and default
checksum (200 + correct value + persisted), an inherited full-object checksum
preserved without a requested algorithm, and a multipart-suffixed remote value
rejected. Red/green verified for each against the merged code.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>

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 "<valid>-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)
+}

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 destination must carry the same checksum the local path would
-		// the source, or the S3 default for a checksum-less object), the local
+		// produce; the federated path has the remote compute, validate, persist
-		// path computes, persists and returns it; the federated path must do the
+		// and return it. Without this the federated copy silently returns an
-		// same. Ask the remote to compute and persist that checksum by streaming
+		// empty checksum (#99).
-		// 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).
 		wantChecksumType := dstOpts.WantServerSideChecksumType
-		if wantChecksumType.IsSet() {
+		var checksumHeaderValue string
-			opts.Checksum = federatedChecksumType(wantChecksumType)
+		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
 			}