fix: forward the legal hold as an Object Lock header on federated CopyObject

A cross-deployment CopyObject that requests `x-amz-object-lock-legal-hold: ON` answered 200 while the destination carried no hold. The resolved value reached the remote as ordinary user metadata, `X-Amz-Meta-X-Amz-Object-Lock-Legal-Hold`, so nothing applied it. Retention requested on the same copy survived, which is what made the loss easy to miss. The federation branch passes the resolved metadata map straight to `Core.PutObject` as `PutObjectOptions.UserMetadata`. minio-go's `Header()` writes the typed lock fields first, then prefixes every UserMetadata key it does not recognise with `x-amz-meta-`; `supportedHeaders` covers `x-amz-object-lock-mode` and `x-amz-object-lock-retain-until-date` but not `x-amz-object-lock-legal-hold`, and `isAmzHeader` does not match it either. Retention therefore arrives as real headers and the hold does not. The high-level `validate()` that would have rejected the key never runs, because `Core.PutObject` goes straight to the low-level PUT. Carry the hold on the typed `LegalHold` option and forward a cloned map with the raw key removed. The clone matters twice: typed fields are written before the UserMetadata loop, so a leftover raw key would add a bogus `x-amz-meta-` entry beside the correct header, and the proxy's own response and event metadata are rebuilt from the resolved values rather than the forwarding map, which no longer carries the hold. Retention stays in the map deliberately. It already passes through as a standard header, and moving it to the typed `RetainUntilDate` field would format with `time.RFC3339` and truncate a retain-until date to whole seconds. The new test asserts the wire: the remote must receive `X-Amz-Object-Lock-Legal-Hold` and never the `x-amz-meta-` spelling, and the destination version must actually store the hold. It fails without the change with "legal hold forwarded as user metadata [ON]". Fixes #166 Signed-off-by: Ayush Sharma <72848455+Aeirx@users.noreply.github.com>
2026-09-11 21:14:04 +03:00 · 2026-09-09 18:42:07 +05:30
parent 2f61325d4a
commit e7e87402ed
2 changed files with 169 additions and 2 deletions
@@ -0,0 +1,139 @@
+// Copyright (c) 2015-2026 MinIO, Inc.
+//
+// This file is part of MinIO Object Storage stack
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package cmd
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/minio/minio/internal/auth"
+	objectlock "github.com/minio/minio/internal/bucket/object/lock"
+	xhttp "github.com/minio/minio/internal/http"
+)
+
+// enableBucketObjectLock puts a lock-enabled configuration on an existing
+// bucket so checkPutObjectLockAllowed accepts a legal-hold header instead of
+// rejecting the request with ErrInvalidBucketObjectLockConfiguration.
+func enableBucketObjectLock(t *testing.T, bucket string) {
+	t.Helper()
+	meta, err := globalBucketMetadataSys.Get(bucket)
+	if err != nil {
+		t.Fatalf("unable to read bucket metadata for %s: %v", bucket, err)
+	}
+	updated := meta
+	updated.ObjectLockConfigXML = enabledBucketObjectLockConfig
+	updated.VersioningConfigXML = enabledBucketVersioningConfig
+	// The XML alone is not enough: BucketMetadata keeps a parsed copy that the
+	// lookups actually read, and it is only populated by parseAllConfigs.
+	if err := updated.parseAllConfigs(t.Context(), newObjectLayerFn()); err != nil {
+		t.Fatalf("unable to parse bucket metadata for %s: %v", bucket, err)
+	}
+	globalBucketMetadataSys.Set(bucket, updated)
+}
+
+// legalHoldHeaders returns the forwarded legal-hold headers the remote saw,
+// separated into the real Object Lock header and the user-metadata spelling
+// minio-go produces for an unrecognised UserMetadata key.
+func (c *federationRemoteCapture) legalHoldHeaders() (typed, asMetadata []string) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	metaKey := "X-Amz-Meta-" + xhttp.AmzObjectLockLegalHold
+	for _, h := range c.headers {
+		for k, v := range h {
+			switch {
+			case strings.EqualFold(k, xhttp.AmzObjectLockLegalHold):
+				typed = append(typed, strings.Join(v, ","))
+			case strings.EqualFold(k, metaKey):
+				asMetadata = append(asMetadata, strings.Join(v, ","))
+			}
+		}
+	}
+	return typed, asMetadata
+}
+
+// TestAPIFederatedCopyObjectLegalHold drives the legacy etcd federation branch
+// of CopyObjectHandler with an explicit legal hold on the copy.
+//
+// Before the fix the resolved hold was forwarded inside
+// PutObjectOptions.UserMetadata. minio-go's Header() prefixes every
+// UserMetadata key it does not recognise with "x-amz-meta-", and
+// x-amz-object-lock-legal-hold is in neither supportedHeaders nor isAmzHeader,
+// so the hold reached the remote as X-Amz-Meta-X-Amz-Object-Lock-Legal-Hold.
+// The destination stored no hold and the copy still answered 200 -- a silent
+// loss of a WORM control (#166). Retention requested on the same copy survived,
+// which is what made it easy to miss.
+func TestAPIFederatedCopyObjectLegalHold(t *testing.T) {
+	defer DetectTestLeak(t)()
+	ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{
+		t:          t,
+		objAPITest: testAPIFederatedCopyObjectLegalHold,
+		endpoints:  []string{"CopyObject", "PutObject", "HeadObject", "GetObject"},
+	})
+}
+
+func testAPIFederatedCopyObjectLegalHold(objectAPI ObjectLayer, instanceType, bucketName string,
+	apiRouter http.Handler, credentials auth.Credentials, t *testing.T,
+) {
+	data := []byte("federated copy with a legal hold")
+	srcObject := "federation/legal-hold-source"
+	putCopyChecksumSource(t, apiRouter, credentials, bucketName, srcObject, data, nil)
+
+	remoteBucket, capture, cleanup := setupCopyObjectFederation(t, objectAPI, apiRouter, instanceType, bucketName)
+	defer cleanup()
+
+	// Both roles read bucket metadata from the shared backend in this fixture,
+	// so one configuration covers the proxy's own check and the remote's.
+	enableBucketObjectLock(t, bucketName)
+	enableBucketObjectLock(t, remoteBucket)
+
+	dstObject := "federation/legal-hold-destination"
+	rec := federatedCopyRequest(t, apiRouter, credentials, bucketName, srcObject, remoteBucket, dstObject,
+		map[string]string{xhttp.AmzObjectLockLegalHold: string(objectlock.LegalHoldOn)})
+	if rec.Code != http.StatusOK {
+		t.Fatalf("%s: federated CopyObject with a legal hold failed: %d %s",
+			instanceType, rec.Code, rec.Body.String())
+	}
+
+	// The wire is the point: the hold must arrive as the Object Lock header,
+	// never as user metadata. A 200 with the metadata spelling is exactly the
+	// silent loss this test exists for.
+	typed, asMetadata := capture.legalHoldHeaders()
+	if len(asMetadata) != 0 {
+		t.Fatalf("%s: legal hold forwarded as user metadata %v; the destination stores no hold",
+			instanceType, asMetadata)
+	}
+	if len(typed) == 0 {
+		t.Fatalf("%s: no %s header reached the remote deployment", instanceType, xhttp.AmzObjectLockLegalHold)
+	}
+	for _, got := range typed {
+		if !strings.EqualFold(got, string(objectlock.LegalHoldOn)) {
+			t.Fatalf("%s: forwarded legal hold = %q, want %q", instanceType, got, objectlock.LegalHoldOn)
+		}
+	}
+
+	// And it must actually be stored on the destination version.
+	oi, err := objectAPI.GetObjectInfo(t.Context(), remoteBucket, dstObject, ObjectOptions{})
+	if err != nil {
+		t.Fatalf("%s: unable to stat the federated copy destination: %v", instanceType, err)
+	}
+	if hold := objectlock.GetObjectLegalHoldMeta(oi.UserDefined); hold.Status != objectlock.LegalHoldOn {
+		t.Fatalf("%s: destination legal hold = %q, want %q (metadata: %v)",
+			instanceType, hold.Status, objectlock.LegalHoldOn, oi.UserDefined)
+	}
+}
@@ -1945,11 +1945,36 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re
 				delete(srcInfo.UserDefined, k)
 			}
 		}
+		// Legal hold does not survive the metadata map. minio-go's Header()
+		// writes the typed lock fields first, then prefixes every UserMetadata
+		// key it does not recognise with "x-amz-meta-": supportedHeaders lists
+		// x-amz-object-lock-mode and x-amz-object-lock-retain-until-date but not
+		// x-amz-object-lock-legal-hold, and isAmzHeader does not match it
+		// either. Forwarded in the map the hold arrives as
+		// X-Amz-Meta-X-Amz-Object-Lock-Legal-Hold, the destination stores no
+		// hold, and the copy still answers 200 (#166).
+		//
+		// Carry only the hold on the typed field, and forward a clone without
+		// the raw key: typed fields are written before the UserMetadata loop, so
+		// a leftover raw key would add a bogus x-amz-meta- entry beside the
+		// correct header. Retention stays in the map -- it already passes
+		// through as a standard header, and moving it to the typed
+		// RetainUntilDate field would truncate the date to whole seconds.
+		legalHoldKey := strings.ToLower(xhttp.AmzObjectLockLegalHold)
+		forwardedLegalHold := srcInfo.UserDefined[legalHoldKey]
+		forwardedMeta := srcInfo.UserDefined
+		if forwardedLegalHold != "" {
+			forwardedMeta = cloneMSS(srcInfo.UserDefined)
+			delete(forwardedMeta, legalHoldKey)
+		}
 		opts := miniogo.PutObjectOptions{
-			UserMetadata:         srcInfo.UserDefined,
+			UserMetadata:         forwardedMeta,
 			ServerSideEncryption: dstOpts.ServerSideEncryption,
 			UserTags:             tag.ToMap(),
 		}
+		if forwardedLegalHold != "" {
+			opts.LegalHold = miniogo.LegalHoldStatus(forwardedLegalHold)
+		}
 		// 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
@@ -1996,7 +2021,10 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re
 			writeErrorResponse(ctx, w, toAPIError(ctx, rerr), r.URL)
 			return
 		}
-		objInfo.UserDefined = cloneMSS(opts.UserMetadata)
+		// Built from the resolved values rather than the forwarding map: the
+		// legal hold was moved onto the typed option above, so opts.UserMetadata
+		// no longer carries it and the response and event would under-report.
+		objInfo.UserDefined = cloneMSS(srcInfo.UserDefined)
 		// A forwarded checksum header is a request detail, not object metadata.
 		if checksumHeaderValue != "" {
 			delete(objInfo.UserDefined, wantChecksumType.Key())