From f4c1286c9dfd882384dad74667b639b93f66b418 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 3 Sep 2026 00:45:02 +0800 Subject: [PATCH] fix: apply replicated Object Lock updates only when newer than the stored state A replicated CopyObject uses the REPLACE metadata directive, so the map the handler compared replication timestamps against had already been rebuilt from the request and filtered of Object Lock keys: the stored retention and legal-hold timestamps were never seen, every replica update was applied regardless of order, and the legal-hold timestamp was written under the retention key. A stale replica could turn a newer legal hold off or shorten a newer retention. Capture the stored Object Lock state before the metadata is rebuilt, apply a replica update only when its source timestamp is newer, put the stored state back when the update is stale, and keep each timestamp under its own key. Inherited from upstream; recorded in the advisory ledger. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PvgysXDmhPBBimCReYtA8q Signed-off-by: Feng Ruohang --- cmd/bucket-object-lock.go | 62 +++++++++++++++++++++++++++++++++ cmd/object-handlers.go | 35 +++++++++---------- cmd/replication-trust_test.go | 64 +++++++++++++++++++++++++++++++++++ docs/security/advisories.md | 1 + 4 files changed, 145 insertions(+), 17 deletions(-) diff --git a/cmd/bucket-object-lock.go b/cmd/bucket-object-lock.go index b4ef9292f..80a963932 100644 --- a/cmd/bucket-object-lock.go +++ b/cmd/bucket-object-lock.go @@ -22,9 +22,12 @@ import ( "errors" "math" "net/http" + "strings" + "time" "github.com/minio/minio/internal/auth" objectlock "github.com/minio/minio/internal/bucket/object/lock" + xhttp "github.com/minio/minio/internal/http" "github.com/minio/minio/internal/logger" "github.com/minio/pkg/v3/policy" ) @@ -343,3 +346,62 @@ func checkPutObjectLockAllowed(ctx context.Context, rq *http.Request, bucket, ob func NewBucketObjectLockSys() *BucketObjectLockSys { return &BucketObjectLockSys{} } + +// objectLockState is the Object Lock metadata of a stored object version +// together with the replication timestamps that order updates to it. +type objectLockState struct { + mode, retainUntil, retentionTimestamp string + legalHold, legalHoldTimestamp string +} + +func storedObjectLockState(metadata map[string]string) objectLockState { + return objectLockState{ + mode: metadata[strings.ToLower(xhttp.AmzObjectLockMode)], + retainUntil: metadata[strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)], + retentionTimestamp: metadata[ReservedMetadataPrefixLower+ObjectLockRetentionTimestamp], + legalHold: metadata[strings.ToLower(xhttp.AmzObjectLockLegalHold)], + legalHoldTimestamp: metadata[ReservedMetadataPrefixLower+ObjectLockLegalHoldTimestamp], + } +} + +// olderThan reports whether a stored replication timestamp is missing, +// unreadable, or earlier than the source timestamp, in which case the +// replica update wins. A zero source timestamp never wins. +func olderThan(stored string, src time.Time) bool { + if src.IsZero() { + return false + } + ondisk, err := time.Parse(time.RFC3339Nano, stored) + return err != nil || ondisk.Before(src) +} + +func (s objectLockState) retentionIsOlderThan(src time.Time) bool { + return olderThan(s.retentionTimestamp, src) +} + +func (s objectLockState) legalHoldIsOlderThan(src time.Time) bool { + return olderThan(s.legalHoldTimestamp, src) +} + +// restoreRetention and restoreLegalHold put the stored state back into +// metadata that was rebuilt from a request whose update was not applied. +func (s objectLockState) restoreRetention(metadata map[string]string) { + if s.mode == "" { + return + } + metadata[strings.ToLower(xhttp.AmzObjectLockMode)] = s.mode + metadata[strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)] = s.retainUntil + if s.retentionTimestamp != "" { + metadata[ReservedMetadataPrefixLower+ObjectLockRetentionTimestamp] = s.retentionTimestamp + } +} + +func (s objectLockState) restoreLegalHold(metadata map[string]string) { + if s.legalHold == "" { + return + } + metadata[strings.ToLower(xhttp.AmzObjectLockLegalHold)] = s.legalHold + if s.legalHoldTimestamp != "" { + metadata[ReservedMetadataPrefixLower+ObjectLockLegalHoldTimestamp] = s.legalHoldTimestamp + } +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 78a3347e6..42ef9363e 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -1664,6 +1664,12 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re srcInfo.PutObjReader = pReader + // Object Lock state as stored on disk, captured before the metadata + // directive rebuilds the map. A replica update is applied only when its + // source timestamp is newer than the stored one, and a stale update must + // leave the stored state in place instead of erasing it. + storedLock := storedObjectLockState(srcInfo.UserDefined) + srcInfo.UserDefined, err = getCpObjMetadataFromHeader(ctx, r, srcInfo.UserDefined, allowReplicationMetadata) if err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) @@ -1707,17 +1713,14 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re // apply default bucket configuration/governance headers for dest side. retentionMode, retentionDate, legalHold, s3Err := checkPutObjectLockAllowed(ctx, r, dstBucket, dstObject, getObjectInfo, retPerms, holdPerms, replicaTrusted) if s3Err == ErrNone && retentionMode.Valid() { - lastretentionTimestamp := srcInfo.UserDefined[ReservedMetadataPrefixLower+ObjectLockRetentionTimestamp] if dstOpts.ReplicationRequest { srcTimestamp := dstOpts.ReplicationSourceRetentionTimestamp - if !srcTimestamp.IsZero() { - ondiskTimestamp, err := time.Parse(time.RFC3339Nano, lastretentionTimestamp) - // update retention metadata only if replica timestamp is newer than what's on disk - if err != nil || (err == nil && ondiskTimestamp.Before(srcTimestamp)) { - srcInfo.UserDefined[strings.ToLower(xhttp.AmzObjectLockMode)] = string(retentionMode) - srcInfo.UserDefined[strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)] = amztime.ISO8601Format(retentionDate.UTC()) - srcInfo.UserDefined[ReservedMetadataPrefixLower+ObjectLockRetentionTimestamp] = srcTimestamp.UTC().Format(time.RFC3339Nano) - } + if storedLock.retentionIsOlderThan(srcTimestamp) { + srcInfo.UserDefined[strings.ToLower(xhttp.AmzObjectLockMode)] = string(retentionMode) + srcInfo.UserDefined[strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)] = amztime.ISO8601Format(retentionDate.UTC()) + srcInfo.UserDefined[ReservedMetadataPrefixLower+ObjectLockRetentionTimestamp] = srcTimestamp.UTC().Format(time.RFC3339Nano) + } else { + storedLock.restoreRetention(srcInfo.UserDefined) } } else { srcInfo.UserDefined[strings.ToLower(xhttp.AmzObjectLockMode)] = string(retentionMode) @@ -1727,19 +1730,17 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re } if s3Err == ErrNone && legalHold.Status.Valid() { - lastLegalHoldTimestamp := srcInfo.UserDefined[ReservedMetadataPrefixLower+ObjectLockLegalHoldTimestamp] if dstOpts.ReplicationRequest { srcTimestamp := dstOpts.ReplicationSourceLegalholdTimestamp - if !srcTimestamp.IsZero() { - ondiskTimestamp, err := time.Parse(time.RFC3339Nano, lastLegalHoldTimestamp) - // update legalhold metadata only if replica timestamp is newer than what's on disk - if err != nil || (err == nil && ondiskTimestamp.Before(srcTimestamp)) { - srcInfo.UserDefined[strings.ToLower(xhttp.AmzObjectLockLegalHold)] = string(legalHold.Status) - srcInfo.UserDefined[ReservedMetadataPrefixLower+ObjectLockRetentionTimestamp] = srcTimestamp.Format(time.RFC3339Nano) - } + if storedLock.legalHoldIsOlderThan(srcTimestamp) { + srcInfo.UserDefined[strings.ToLower(xhttp.AmzObjectLockLegalHold)] = string(legalHold.Status) + srcInfo.UserDefined[ReservedMetadataPrefixLower+ObjectLockLegalHoldTimestamp] = srcTimestamp.UTC().Format(time.RFC3339Nano) + } else { + storedLock.restoreLegalHold(srcInfo.UserDefined) } } else { srcInfo.UserDefined[strings.ToLower(xhttp.AmzObjectLockLegalHold)] = string(legalHold.Status) + srcInfo.UserDefined[ReservedMetadataPrefixLower+ObjectLockLegalHoldTimestamp] = UTCNow().Format(time.RFC3339Nano) } } if s3Err != ErrNone { diff --git a/cmd/replication-trust_test.go b/cmd/replication-trust_test.go index 021a17000..b3e1815e1 100644 --- a/cmd/replication-trust_test.go +++ b/cmd/replication-trust_test.go @@ -884,3 +884,67 @@ func testAPIStreamingTrailerWithUntrustedReplicationHeaders(obj ObjectLayer, ins t.Fatalf("%s: uploaded parts %+v, want one part of %d bytes", instanceType, parts.Parts, len(payload)) } } + +// TestAPICopyObjectReplicaLegalHoldTimestamp verifies that a replicated legal +// hold update records its own timestamp under the legal-hold key: a replica +// that arrives later with an older timestamp must not change the hold, the +// retention timestamp must stay untouched, and a newer replica still applies. +func TestAPICopyObjectReplicaLegalHoldTimestamp(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPICopyObjectReplicaLegalHoldTimestamp, + makeBucketOptions: MakeBucketOptions{LockEnabled: true}, + }) +} + +func testAPICopyObjectReplicaLegalHoldTimestamp(obj ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, _ auth.Credentials, t *testing.T, +) { + object := "replication-trust/legal-hold" + if _, err := obj.PutObject(t.Context(), bucketName, object, mustGetPutObjReader(t, bytes.NewReader([]byte("held")), 4, "", ""), ObjectOptions{}); err != nil { + t.Fatal(err) + } + replicator := newObjectAttributesAuthzUser(t, instanceType, bucketName, + `"s3:GetObject","s3:PutObject","s3:ReplicateObject","s3:PutObjectLegalHold","s3:GetObjectLegalHold","s3:GetObjectRetention"`) + apply := func(status, stamp string) { + t.Helper() + headers := map[string]string{ + xhttp.AmzCopySource: url.QueryEscape(SlashSeparator + bucketName + SlashSeparator + object), + xhttp.AmzMetadataDirective: replaceDirective, + xhttp.MinIOSourceReplicationRequest: "true", + xhttp.AmzBucketReplicationStatus: "REPLICA", + xhttp.AmzObjectLockLegalHold: status, + xhttp.MinIOSourceObjectLegalHoldTimestamp: stamp, + } + req, err := newTestSignedRequestV4(http.MethodPut, getCopyObjectURL("", bucketName, object), 0, nil, + replicator.AccessKey, replicator.SecretKey, headers) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + apiRouter.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("%s: replica CopyObject legal hold %s @ %s: status %d: %s", instanceType, status, stamp, rec.Code, rec.Body.String()) + } + } + state := func() (hold, holdStamp string, hasRetentionStamp bool) { + t.Helper() + info, err := obj.GetObjectInfo(t.Context(), bucketName, object, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + _, hasRetentionStamp = info.UserDefined[ReservedMetadataPrefixLower+ObjectLockRetentionTimestamp] + return info.UserDefined[strings.ToLower(xhttp.AmzObjectLockLegalHold)], info.UserDefined[ReservedMetadataPrefixLower+ObjectLockLegalHoldTimestamp], hasRetentionStamp + } + + apply("ON", "2026-09-03T10:00:00Z") + apply("OFF", "2026-09-03T09:00:00Z") // stale replica: must be ignored + if hold, stamp, retention := state(); hold != "ON" || stamp != "2026-09-03T10:00:00Z" || retention { + t.Fatalf("%s: after stale OFF: hold=%q legal-hold timestamp=%q retention timestamp present=%v", instanceType, hold, stamp, retention) + } + apply("OFF", "2026-09-03T11:00:00Z") // newer replica: applies + if hold, stamp, retention := state(); hold != "OFF" || stamp != "2026-09-03T11:00:00Z" || retention { + t.Fatalf("%s: after newer OFF: hold=%q legal-hold timestamp=%q retention timestamp present=%v", instanceType, hold, stamp, retention) + } +} diff --git a/docs/security/advisories.md b/docs/security/advisories.md index 19ea31973..65dd32b2f 100644 --- a/docs/security/advisories.md +++ b/docs/security/advisories.md @@ -49,4 +49,5 @@ The first Silo community release was cut from upstream history that already cont | Change | Fixed by | Summary | | :-- | :-- | :-- | +| Replicated Object Lock updates ignored their timestamps | pre-release cleanup for the release after 20260806 | A replicated `CopyObject` rebuilt the metadata from the request before comparing replication timestamps, so the stored retention and legal-hold timestamps were never seen: any replica update was applied regardless of order, and the legal-hold timestamp was written under the retention key. A stale replica could therefore turn a newer legal hold off or shorten a newer retention. The stored state is now captured first, a replica update is applied only when its timestamp is newer, a stale one leaves the stored state in place, and each timestamp is kept under its own key. Inherited from upstream; every earlier release is affected. | | LDAP TLS regression | `ce1c537eb` | Restores TLS configuration propagation for `ldaps://` `DialURL()` connections so `MINIO_IDENTITY_LDAP_TLS_SKIP_VERIFY` and custom root CAs work again. |