From 3f3ae22bd31404c3303ccf1b7756c63646bd0035 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=94=90=E5=B0=8F=E9=B8=AD?= Date: Sun, 23 Aug 2026 01:57:47 +0800 Subject: [PATCH] fix(object-lock): require source timestamps before replication passes WORM gate Adversarial review of the replication WORM bypass found a silent unlock: an authorized replication write that carries no source timestamp for a category (the source never held the object, only its tags changed) is not judged by receiver-side LWW, so the metadata replace would drop the destination's legal hold or retention unjudged. Before the bypass that write was merely rejected. Gate the bypass on `replication_write_may_pass_worm_gate`: the write passes only when it carries the source timestamp of every category that currently locks the version, so LWW decides each one; otherwise it stays WORM-rejected. The set layer evaluates the lock gate first so malformed persisted lock metadata still fails closed, and the app-layer pre-check applies the same rule. Refs rustfs/backlog#1953 --- crates/ecstore/src/api/mod.rs | 2 +- .../src/bucket/object_lock/objectlock_sys.rs | 86 ++++++++++++++++++- crates/ecstore/src/set_disk/mod.rs | 1 + crates/ecstore/src/set_disk/ops/object.rs | 82 +++++++++++++++--- rustfs/src/app/object_usecase.rs | 40 ++++++--- rustfs/src/app/storage_api.rs | 10 +++ 6 files changed, 198 insertions(+), 23 deletions(-) diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index 17fffac3d..577e4df52 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -161,7 +161,7 @@ pub mod bucket { pub mod objectlock_sys { pub use crate::bucket::object_lock::objectlock_sys::{ BucketObjectLockSys, ObjectLockBlockReason, add_years, check_object_lock_for_deletion, - check_retention_for_modification, is_retention_active, + check_retention_for_modification, is_retention_active, replication_write_may_pass_worm_gate, }; } } diff --git a/crates/ecstore/src/bucket/object_lock/objectlock_sys.rs b/crates/ecstore/src/bucket/object_lock/objectlock_sys.rs index 71d09484d..69f366553 100644 --- a/crates/ecstore/src/bucket/object_lock/objectlock_sys.rs +++ b/crates/ecstore/src/bucket/object_lock/objectlock_sys.rs @@ -15,7 +15,7 @@ use crate::bucket::metadata_sys::{ObjectLockConfigState, get_object_lock_config, get_object_lock_config_state}; use crate::bucket::object_lock::objectlock; use crate::error::{Error, Result, StorageError}; -use crate::object_api::ObjectInfo; +use crate::object_api::{ObjectInfo, ObjectOptions}; use s3s::dto::{Date, DefaultRetention, ObjectLockConfiguration, ObjectLockLegalHoldStatus, ObjectLockRetentionMode}; use s3s::header::{X_AMZ_OBJECT_LOCK_LEGAL_HOLD, X_AMZ_OBJECT_LOCK_MODE, X_AMZ_OBJECT_LOCK_RETAIN_UNTIL_DATE}; use std::sync::Arc; @@ -136,12 +136,41 @@ pub fn add_years(dt: OffsetDateTime, years: i32) -> OffsetDateTime { /// Check if an object has legal hold enabled. /// Returns true if legal hold is ON. -#[allow(dead_code, reason = "asserted by this file's tests (backlog#1823)")] fn has_legal_hold(user_defined: &std::collections::HashMap) -> bool { let lhold = objectlock::get_object_legalhold_meta(user_defined); matches!(lhold.status, Some(ref st) if st.as_str() == ObjectLockLegalHoldStatus::ON) } +/// Whether an authorized replication write (`ObjectOptions::replication_request`) +/// may overwrite a locked destination version. +/// +/// The source's lock state governs a replica (MinIO `checkPutObjectLockAllowed` +/// skips the existing-version check for replicas), and a source-side hold +/// release or retention change reaches this site only through this write. The +/// overwrite is allowed only when the write carries the source timestamp of +/// every category that currently locks the version, so receiver-side LWW +/// (`merge_replication_metadata_lww`) judges each of them: a category locked +/// more recently here is kept, otherwise the source's newer state wins. A write +/// without that timestamp carries no source decision for the category — the +/// metadata replace would lift the lock unjudged — so it stays WORM-rejected. +pub fn replication_write_may_pass_worm_gate( + user_defined: &std::collections::HashMap, + opts: &ObjectOptions, +) -> bool { + if !opts.replication_request { + return false; + } + if has_legal_hold(user_defined) && opts.replication_legalhold_timestamp.is_none() { + return false; + } + let ret = objectlock::get_object_retention_meta(user_defined); + let retention_locked = ret + .mode + .as_ref() + .is_some_and(|mode| is_retention_active(mode.as_str(), ret.retain_until_date.as_ref())); + !(retention_locked && opts.replication_retention_timestamp.is_none()) +} + /// Check if an object is locked based on its metadata. /// This is a common function used by both lifecycle evaluation and deletion checks. /// @@ -491,6 +520,59 @@ mod tests { } } + /// A replication write passes the WORM gate only when it carries the + /// source timestamp of every category that currently locks the version. + #[test] + fn replication_write_passes_worm_gate_only_with_every_locking_category_timestamp() { + use rustfs_utils::http::headers::{ + AMZ_OBJECT_LOCK_LEGAL_HOLD_LOWER, AMZ_OBJECT_LOCK_MODE_LOWER, AMZ_OBJECT_LOCK_RETAIN_UNTIL_DATE_LOWER, + }; + + let hold = [(AMZ_OBJECT_LOCK_LEGAL_HOLD_LOWER, "ON")]; + let retention = [ + (AMZ_OBJECT_LOCK_MODE_LOWER, "GOVERNANCE"), + (AMZ_OBJECT_LOCK_RETAIN_UNTIL_DATE_LOWER, "2099-01-01T00:00:00Z"), + ]; + let expired = [ + (AMZ_OBJECT_LOCK_MODE_LOWER, "COMPLIANCE"), + (AMZ_OBJECT_LOCK_RETAIN_UNTIL_DATE_LOWER, "2000-01-01T00:00:00Z"), + ]; + let metadata = |entries: &[&[(&str, &str)]]| -> std::collections::HashMap { + entries + .iter() + .flat_map(|entries| entries.iter()) + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect() + }; + let opts = |hold_ts: bool, retention_ts: bool| ObjectOptions { + replication_request: true, + replication_legalhold_timestamp: hold_ts.then_some(OffsetDateTime::UNIX_EPOCH), + replication_retention_timestamp: retention_ts.then_some(OffsetDateTime::UNIX_EPOCH), + ..Default::default() + }; + + let locked_by_both = metadata(&[&hold, &retention]); + assert!(replication_write_may_pass_worm_gate(&locked_by_both, &opts(true, true))); + assert!(!replication_write_may_pass_worm_gate(&locked_by_both, &opts(true, false))); + assert!(!replication_write_may_pass_worm_gate(&locked_by_both, &opts(false, true))); + + assert!(replication_write_may_pass_worm_gate(&metadata(&[&hold]), &opts(true, false))); + assert!(!replication_write_may_pass_worm_gate(&metadata(&[&hold]), &opts(false, true))); + assert!(replication_write_may_pass_worm_gate(&metadata(&[&retention]), &opts(false, true))); + assert!(!replication_write_may_pass_worm_gate(&metadata(&[&retention]), &opts(true, false))); + + // Expired retention and a released hold no longer lock anything. + let unlocked = metadata(&[&expired, &[(AMZ_OBJECT_LOCK_LEGAL_HOLD_LOWER, "OFF")]]); + assert!(replication_write_may_pass_worm_gate(&unlocked, &opts(false, false))); + + // Never for a non-replication write, whatever it carries. + let local = ObjectOptions { + replication_request: false, + ..opts(true, true) + }; + assert!(!replication_write_may_pass_worm_gate(&metadata(&[&hold]), &local)); + } + /// A local PutObjectRetention / PutObjectLegalHold "clear" persists the /// lock keys as empty strings (the MinIO on-disk shape, see /// `parse_object_lock_retention`); that is "no lock", not corruption, and diff --git a/crates/ecstore/src/set_disk/mod.rs b/crates/ecstore/src/set_disk/mod.rs index 5625d7583..564987b8e 100644 --- a/crates/ecstore/src/set_disk/mod.rs +++ b/crates/ecstore/src/set_disk/mod.rs @@ -47,6 +47,7 @@ use crate::bucket::metadata_sys; use crate::bucket::metadata_sys::ObjectLockConfigState; use crate::bucket::object_lock::objectlock_sys::{ check_object_lock_for_deletion_with_config, check_object_lock_for_deletion_with_state, check_retention_for_modification, + replication_write_may_pass_worm_gate, }; use crate::bucket::replication::{ ReplicateDecision, ReplicationObjectBridge, ReplicationState, ReplicationStatusType, VersionPurgeStatusType, diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index a84ccb077..5c52f5872 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -2662,16 +2662,13 @@ impl SetDisks { Error::other("explicit-version PUT is missing its Object Lock configuration snapshot") })?; // The WORM gate protects the locked version from local - // overwrites only. For an authorized replication write - // (ReplicateObjectAction, `replication_request`) the - // source's lock state governs the replica, as in MinIO's - // `checkPutObjectLockAllowed` (`!replica` guard): a - // legal-hold release or retention change reaches this - // site only through this write, and rejecting it loops - // through MRF forever. Receiver-side LWW below still - // keeps a category locked more recently here. - if !opts.replication_request - && check_object_lock_for_deletion_with_state(object_lock_config.state(), &existing, false)?.is_some() + // overwrites; an authorized replication write passes it + // only when the LWW merge below will judge every + // locking category (see + // `replication_write_may_pass_worm_gate`). Gate first so + // malformed lock metadata still fails closed. + if check_object_lock_for_deletion_with_state(object_lock_config.state(), &existing, false)?.is_some() + && !replication_write_may_pass_worm_gate(&existing.user_defined, opts) { return Err(StorageError::PrefixAccessDenied(bucket.to_string(), object.to_string())); } @@ -8379,15 +8376,20 @@ mod replication_lww_tests { put_version(set_disks, bucket, object, version_id, &versioned_opts(version_id, local)).await; } + /// Inbound legal-hold release from a source that also carries the (same) + /// COMPLIANCE retention; the sender stamps a source timestamp for every + /// category the source version has. fn inbound_legal_hold_release_opts(version_id: &str, timestamp: &str) -> ObjectOptions { let mut inbound = HashMap::new(); inbound.insert(AMZ_OBJECT_LOCK_LEGAL_HOLD_LOWER.to_string(), "OFF".to_string()); insert_str(&mut inbound, SUFFIX_OBJECTLOCK_LEGALHOLD_TIMESTAMP, timestamp.to_string()); inbound.insert(AMZ_OBJECT_LOCK_MODE_LOWER.to_string(), "COMPLIANCE".to_string()); inbound.insert(AMZ_OBJECT_LOCK_RETAIN_UNTIL_DATE_LOWER.to_string(), "2099-01-01T00:00:00Z".to_string()); + insert_str(&mut inbound, SUFFIX_OBJECTLOCK_RETENTION_TIMESTAMP, T_OLD.to_string()); ObjectOptions { replication_request: true, replication_legalhold_timestamp: Some(parse_ts(timestamp)), + replication_retention_timestamp: Some(parse_ts(T_OLD)), ..versioned_opts(version_id, inbound) } } @@ -8460,6 +8462,66 @@ mod replication_lww_tests { ); } + /// A replication write that carries no source decision for a locking + /// category (here: tags changed at a source that never held the object) + /// must not lift the destination's hold by replacing the metadata + /// unjudged; it stays WORM-rejected like a local overwrite. + #[tokio::test] + async fn inbound_without_legal_hold_timestamp_stays_rejected_on_held_version() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; + let bucket = "lww-locked-unjudged-category"; + let object = "object"; + let version_id = Uuid::new_v4().to_string(); + make_bucket(&disk_stores, bucket).await; + seed_locked_version(&set_disks, bucket, object, &version_id, T_OLD).await; + + let mut inbound = HashMap::new(); + inbound.insert(AMZ_OBJECT_TAGGING.to_string(), "k=v".to_string()); + insert_str(&mut inbound, SUFFIX_TAGGING_TIMESTAMP, T_NEW.to_string()); + inbound.insert(AMZ_OBJECT_LOCK_MODE_LOWER.to_string(), "COMPLIANCE".to_string()); + inbound.insert(AMZ_OBJECT_LOCK_RETAIN_UNTIL_DATE_LOWER.to_string(), "2099-01-01T00:00:00Z".to_string()); + let opts = ObjectOptions { + replication_request: true, + replication_tagging_timestamp: Some(parse_ts(T_NEW)), + replication_retention_timestamp: Some(parse_ts(T_NEW)), + replication_legalhold_timestamp: None, + ..versioned_opts(&version_id, inbound) + }; + let mut reader = PutObjReader::from_vec(b"lww-body".to_vec()); + let err = set_disks + .put_object(bucket, object, &mut reader, &opts) + .await + .expect_err("a replication write without the legal-hold source timestamp must stay rejected"); + assert!(matches!(err, StorageError::PrefixAccessDenied(_, _)), "unexpected error: {err}"); + + let info = version_info(&set_disks, bucket, object, &version_id).await; + assert_eq!(info.user_defined.get(AMZ_OBJECT_LOCK_LEGAL_HOLD_LOWER).map(String::as_str), Some("ON")); + } + + /// The gate runs before the replication bypass, so malformed persisted + /// lock metadata still fails closed for an authorized replication write. + #[tokio::test] + async fn replication_write_on_malformed_lock_metadata_still_fails_closed() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; + let bucket = "lww-locked-malformed"; + let object = "object"; + let version_id = Uuid::new_v4().to_string(); + make_bucket(&disk_stores, bucket).await; + let mut local = HashMap::new(); + local.insert(AMZ_OBJECT_LOCK_LEGAL_HOLD_LOWER.to_string(), "MAYBE".to_string()); + put_version(&set_disks, bucket, object, &version_id, &versioned_opts(&version_id, local)).await; + + let mut reader = PutObjReader::from_vec(b"lww-body".to_vec()); + let err = set_disks + .put_object(bucket, object, &mut reader, &inbound_legal_hold_release_opts(&version_id, T_NEW)) + .await + .expect_err("malformed persisted lock metadata must fail the replication write closed"); + assert!(!matches!(err, StorageError::PrefixAccessDenied(_, _)), "unexpected error: {err}"); + + let info = version_info(&set_disks, bucket, object, &version_id).await; + assert_eq!(info.user_defined.get(AMZ_OBJECT_LOCK_LEGAL_HOLD_LOWER).map(String::as_str), Some("MAYBE")); + } + /// The bypass is scoped to authorized replication writes: the same /// explicit-version PUT without `replication_request` stays WORM-rejected. #[tokio::test] diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index cdb3a02a7..8ced0f7a2 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -39,7 +39,7 @@ use super::storage_api::object_usecase::bucket::{ metadata_sys, object_lock::{ objectlock::{get_object_legalhold_meta, get_object_retention_meta}, - objectlock_sys::{check_object_lock_for_deletion, is_retention_active}, + objectlock_sys::{check_object_lock_for_deletion, is_retention_active, replication_write_may_pass_worm_gate}, }, predict_lifecycle_expiration, quota::{QuotaCheckResult, QuotaError, QuotaOperation}, @@ -3938,13 +3938,10 @@ pub(crate) fn validate_existing_object_lock_for_write(existing_obj_info: &Object if put_like_write_creates_new_version(opts) { return Ok(()); } - // An authorized replication write (ReplicateObjectAction set - // `replication_request`) carries the source's lock state for the replica, - // so the destination's current hold/retention must not reject it (MinIO - // `checkPutObjectLockAllowed` skips the existing-version check for - // replicas). The set layer's commit-lock LWW still keeps a category that - // was locked more recently on this site. - if opts.replication_request { + // An authorized replication write may replace the locked version when the + // set layer's commit-lock LWW will judge every locking category; the set + // layer re-checks the same rule under the lock. + if replication_write_may_pass_worm_gate(&existing_obj_info.user_defined, opts) { return Ok(()); } @@ -11047,14 +11044,17 @@ mod tests { } /// The source's lock state governs the replica (rustfs/backlog#1953): - /// an authorized replication write may overwrite a locked version; the - /// set layer's LWW still decides per category. + /// an authorized replication write carrying the locking category's source + /// timestamp may overwrite a locked version; the set layer's LWW then + /// decides per category. #[test] fn validate_existing_object_lock_allows_authorized_replication_overwrite() { let opts = ObjectOptions { versioned: true, version_id: Some(Uuid::new_v4().to_string()), replication_request: true, + replication_retention_timestamp: Some(OffsetDateTime::UNIX_EPOCH), + replication_legalhold_timestamp: Some(OffsetDateTime::UNIX_EPOCH), ..Default::default() }; @@ -11064,6 +11064,26 @@ mod tests { .expect("replication write must bypass the destination legal hold"); } + /// Without the locking category's source timestamp the LWW merge cannot + /// judge it, so the write stays rejected instead of lifting the lock. + #[test] + fn validate_existing_object_lock_rejects_replication_overwrite_without_lock_timestamp() { + let opts = ObjectOptions { + versioned: true, + version_id: Some(Uuid::new_v4().to_string()), + replication_request: true, + replication_tagging_timestamp: Some(OffsetDateTime::UNIX_EPOCH), + ..Default::default() + }; + + let err = validate_existing_object_lock_for_write(&compliance_retained_object_info(), &opts) + .expect_err("COMPLIANCE lock must hold without a retention source timestamp"); + assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + let err = validate_existing_object_lock_for_write(&legal_hold_object_info(), &opts) + .expect_err("legal hold must hold without a legal-hold source timestamp"); + assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + } + #[test] fn is_put_object_extract_requested_accepts_meta_header() { let mut headers = HeaderMap::new(); diff --git a/rustfs/src/app/storage_api.rs b/rustfs/src/app/storage_api.rs index d90cfb6a8..953ebc8e2 100644 --- a/rustfs/src/app/storage_api.rs +++ b/rustfs/src/app/storage_api.rs @@ -587,6 +587,16 @@ pub(crate) mod bucket { retain_until_date, ) } + + pub(crate) fn replication_write_may_pass_worm_gate( + user_defined: &std::collections::HashMap, + opts: &crate::storage::storage_api::StorageObjectOptions, + ) -> bool { + crate::storage::storage_api::ecstore_bucket::object_lock::objectlock_sys::replication_write_may_pass_worm_gate( + user_defined, + opts, + ) + } } }