From ef8f90be9118a58ffd238abde026d77990467346 Mon Sep 17 00:00:00 2001 From: overtrue Date: Fri, 4 Sep 2026 16:28:57 +0800 Subject: [PATCH] fix(ilm): reject invalid retention counts and validate lifecycle filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `NewerNoncurrentVersions` had no lower bound at PUT, and evaluation read a negative count through `usize::try_from(...).unwrap_or(usize::MAX)`. An HTTP-accepted rule therefore retained (almost) everything and silently stopped expiring versions — the one outcome a retention rule must never produce by accident. Reject a negative count during validation, and stop reading one as "retain everything" anywhere it can still arrive from older persistence or an import: evaluation takes no action for such a rule and says so in a diagnostic, the batch limit path yields no event, and `Evaluator::eval` reports a typed corruption error to callers that can surface one. A count-only noncurrent expiration is a MinIO extension, not an AWS form. It used to be rejected as an actionless rule and was never executed. It is now accepted and honoured with the semantics MinIO gives it: the newest N noncurrent versions are kept and every older one is due as soon as it became noncurrent. Zero keeps the meaning the batch limit path has always given it — no count constraint — so a zero-count rule with no age condition still has no action. `LifecycleRuleFilter` is an all-`Option` DTO, so the schema constraints were not checked anywhere: validate at most one top-level predicate, an `And` that combines at least two, no repeated tag key, tag key/value limits, non-negative sizes, and `ObjectSizeGreaterThan < ObjectSizeLessThan`. An empty filter stays valid — AWS documents it as "every object in the bucket". Schema-shape violations are reported with a distinct `ErrorKind` so the S3 boundary answers them with `MalformedXML`; rejected values keep the `InvalidArgument` this path has always returned. backlog#2201 --- crates/ecstore/src/api/mod.rs | 5 +- crates/ecstore/src/bucket/lifecycle/core.rs | 6 +- crates/lifecycle/src/core.rs | 711 +++++++++++++++++++- crates/lifecycle/src/evaluator.rs | 16 +- rustfs/src/app/bucket_usecase.rs | 85 ++- rustfs/src/app/storage_api.rs | 6 + 6 files changed, 808 insertions(+), 21 deletions(-) diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index 6ad57fb21..94d23fb1e 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -89,8 +89,9 @@ pub mod bucket { #[allow(clippy::module_inception)] pub mod lifecycle { pub use crate::bucket::lifecycle::lifecycle::{ - Event, ExpirationOptions, IlmAction, Lifecycle, LifecycleCalculate, ObjectOpts, RuleValidate, - TRANSITION_COMPLETE, TRANSITION_PENDING, TransitionOptions, expected_expiry_time, object_opts_from_object_info, + Event, ExpirationOptions, IlmAction, LIFECYCLE_MALFORMED_XML_ERROR_KIND, Lifecycle, LifecycleCalculate, + ObjectOpts, RuleValidate, TRANSITION_COMPLETE, TRANSITION_PENDING, TransitionOptions, expected_expiry_time, + object_opts_from_object_info, }; } diff --git a/crates/ecstore/src/bucket/lifecycle/core.rs b/crates/ecstore/src/bucket/lifecycle/core.rs index 897874a25..7a4ee71e2 100644 --- a/crates/ecstore/src/bucket/lifecycle/core.rs +++ b/crates/ecstore/src/bucket/lifecycle/core.rs @@ -15,9 +15,9 @@ use crate::object_api::ObjectInfo; pub use rustfs_lifecycle::{ - Event, ExpirationOptions, IlmAction, Lifecycle, LifecycleCalculate, ObjectOpts, RuleValidate, TRANSITION_COMPLETE, - TRANSITION_PENDING, TransitionOptions, abort_incomplete_multipart_upload_due, expected_expiry_time, - expiration_action_has_valid_target, + Event, ExpirationOptions, IlmAction, LIFECYCLE_MALFORMED_XML_ERROR_KIND, Lifecycle, LifecycleCalculate, ObjectOpts, + RuleValidate, TRANSITION_COMPLETE, TRANSITION_PENDING, TransitionOptions, abort_incomplete_multipart_upload_due, + expected_expiry_time, expiration_action_has_valid_target, }; pub fn object_opts_from_object_info(oi: &ObjectInfo) -> ObjectOpts { diff --git a/crates/lifecycle/src/core.rs b/crates/lifecycle/src/core.rs index fc959b92a..b93dcb88d 100644 --- a/crates/lifecycle/src/core.rs +++ b/crates/lifecycle/src/core.rs @@ -18,7 +18,7 @@ use s3s::dto::{ BucketLifecycleConfiguration, ExpirationStatus, LifecycleExpiration, LifecycleRule, LifecycleRuleFilter, NoncurrentVersionTransition, ObjectLockConfiguration, ObjectLockEnabled, RestoreRequest, Transition, }; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use time::macros::offset; use time::{self, Duration, OffsetDateTime}; @@ -61,6 +61,69 @@ const ERR_LIFECYCLE_EXPIRED_OBJECT_DELETE_MARKER_WITH_TAGS: &str = "Rule with ExpiredObjectDeleteMarker cannot have tags based filtering"; const ERR_LIFECYCLE_RULE_MUST_HAVE_ACTION: &str = "Rule must have at least one of Expiration, Transition, NoncurrentVersionExpiration, NoncurrentVersionTransition, or DelMarkerExpiration"; const ERR_LIFECYCLE_PREFIX_FILTER_CONFLICT: &str = "Legacy Prefix and Filter cannot both be present in a lifecycle rule. Use Filter.Prefix instead of the top-level Prefix element."; +const ERR_LIFECYCLE_INVALID_NEWER_NONCURRENT_VERSIONS: &str = "'NewerNoncurrentVersions' must be a non-negative integer"; +const ERR_LIFECYCLE_FILTER_TOO_MANY_PREDICATES: &str = + "Filter must have at most one of Prefix, Tag, ObjectSizeGreaterThan, ObjectSizeLessThan or And; combine predicates with And"; +const ERR_LIFECYCLE_FILTER_AND_TOO_FEW_PREDICATES: &str = "Filter And must contain at least two predicates"; +const ERR_LIFECYCLE_FILTER_DUPLICATE_TAG_KEY: &str = "Filter must not repeat a tag key"; +const ERR_LIFECYCLE_FILTER_INVALID_TAG: &str = "Tag key must be 1-128 characters and tag value must be at most 256 characters"; +const ERR_LIFECYCLE_FILTER_NEGATIVE_SIZE: &str = "ObjectSizeGreaterThan and ObjectSizeLessThan must not be negative"; +const ERR_LIFECYCLE_FILTER_SIZE_RANGE: &str = "ObjectSizeGreaterThan must be smaller than ObjectSizeLessThan"; +const ERR_LIFECYCLE_CORRUPT_NEWER_NONCURRENT_VERSIONS: &str = + "persisted lifecycle rule carries a negative 'NewerNoncurrentVersions'"; + +/// Longest tag key S3 accepts. +const MAX_TAG_KEY_LEN: usize = 128; +/// Longest tag value S3 accepts. +const MAX_TAG_VALUE_LEN: usize = 256; + +/// A validation failure that the S3 boundary must answer with `MalformedXML` +/// rather than `InvalidArgument`: the document does not match the published +/// schema shape (wrong number of `Filter` predicates, a one-member `And`). +/// +/// Everything else stays [`std::io::ErrorKind::Other`], which the boundary +/// already maps to `InvalidArgument`. +pub const LIFECYCLE_MALFORMED_XML_ERROR_KIND: std::io::ErrorKind = std::io::ErrorKind::InvalidData; + +/// A persisted rule that could never have passed validation. Callers that can +/// report an error surface it; evaluation itself stays fail-closed and takes +/// no action for the rule. +pub const LIFECYCLE_CORRUPT_RULE_ERROR_KIND: std::io::ErrorKind = std::io::ErrorKind::InvalidData; + +fn malformed_xml_error(message: &'static str) -> std::io::Error { + std::io::Error::new(LIFECYCLE_MALFORMED_XML_ERROR_KIND, message) +} + +/// The retention count a rule keeps, or `None` when the persisted value is +/// negative — a shape PUT validation rejects, so reaching it means the rule +/// came from older persistence or an import. +/// +/// A negative count must never be read as "retain everything": that is how an +/// invalid configuration silently stopped deleting versions (backlog#2201). +pub fn retained_noncurrent_versions(count: i32) -> Option { + usize::try_from(count).ok() +} + +/// Does any rule carry a retention count that validation would have rejected? +pub fn lifecycle_has_corrupt_retention_count(lc: &BucketLifecycleConfiguration) -> bool { + lc.rules.iter().any(rule_has_corrupt_retention_count) +} + +fn rule_has_corrupt_retention_count(rule: &LifecycleRule) -> bool { + let expiration_count = rule + .noncurrent_version_expiration + .as_ref() + .and_then(|expiration| expiration.newer_noncurrent_versions); + let transition_counts = rule + .noncurrent_version_transitions + .iter() + .flatten() + .filter_map(|transition| transition.newer_noncurrent_versions); + expiration_count + .into_iter() + .chain(transition_counts) + .any(|count| retained_noncurrent_versions(count).is_none()) +} pub use rustfs_scanner_metrics::metrics::IlmAction; @@ -137,6 +200,17 @@ impl RuleValidate for LifecycleRule { return Err(std::io::Error::other(ERR_LIFECYCLE_PREFIX_FILTER_CONFLICT)); } + if let Some(filter) = self.filter.as_ref() { + validate_lifecycle_filter(filter)?; + } + + // A negative retention count was accepted and then read as "retain + // (almost) everything" during evaluation, so an HTTP-accepted rule + // silently stopped deleting versions (backlog#2201). + if rule_has_corrupt_retention_count(self) { + return Err(std::io::Error::other(ERR_LIFECYCLE_INVALID_NEWER_NONCURRENT_VERSIONS)); + } + // Rule with DelMarkerExpiration cannot have tags based filtering let has_tag_filter = self .filter @@ -169,11 +243,14 @@ impl RuleValidate for LifecycleRule { // Rule must have at least one action let has_expiration = self.expiration.is_some(); let has_transition = self.transitions.as_ref().is_some_and(|t| !t.is_empty()); - let has_noncurrent_expiration = self - .noncurrent_version_expiration - .as_ref() - .and_then(|e| e.noncurrent_days) - .is_some(); + // `NewerNoncurrentVersions` on its own is a MinIO extension, not an AWS + // form: it keeps the newest N noncurrent versions and expires the rest + // with no age condition. RustFS accepts it for MinIO compatibility, so + // it has to count as an action here — otherwise a count-only rule was + // rejected as actionless (backlog#2201). + let has_noncurrent_expiration = self.noncurrent_version_expiration.as_ref().is_some_and(|expiration| { + expiration.noncurrent_days.is_some() || expiration.newer_noncurrent_versions.is_some_and(|count| count > 0) + }); let has_noncurrent_transition = self .noncurrent_version_transitions .as_ref() @@ -199,6 +276,79 @@ impl RuleValidate for LifecycleRule { } } +/// Structural validation for `LifecycleRuleFilter`. +/// +/// The generated DTO is all-`Option`, so the S3 schema constraints have to be +/// checked here: at most one top-level predicate, an `And` that actually +/// combines at least two, no repeated tag key, tag key/value limits, and a +/// coherent non-negative size range (backlog#2201). +/// +/// A filter with no predicate at all stays valid: AWS documents an empty +/// `Filter` as "applies to every object in the bucket", and rejecting it would +/// break the most common way to write an unconditional rule. +fn validate_lifecycle_filter(filter: &LifecycleRuleFilter) -> Result<(), std::io::Error> { + let top_level_predicates = usize::from(filter.prefix.is_some()) + + usize::from(filter.tag.is_some()) + + usize::from(filter.object_size_greater_than.is_some()) + + usize::from(filter.object_size_less_than.is_some()) + + usize::from(filter.and.is_some()); + if top_level_predicates > 1 { + return Err(malformed_xml_error(ERR_LIFECYCLE_FILTER_TOO_MANY_PREDICATES)); + } + + if let Some(tag) = filter.tag.as_ref() { + validate_lifecycle_tag(tag)?; + } + + if let Some(and) = filter.and.as_ref() { + let tags = and.tags.as_deref().unwrap_or(&[]); + let and_predicates = usize::from(and.prefix.is_some()) + + tags.len() + + usize::from(and.object_size_greater_than.is_some()) + + usize::from(and.object_size_less_than.is_some()); + if and_predicates < 2 { + return Err(malformed_xml_error(ERR_LIFECYCLE_FILTER_AND_TOO_FEW_PREDICATES)); + } + let mut seen_keys = HashSet::with_capacity(tags.len()); + for tag in tags { + validate_lifecycle_tag(tag)?; + let key = tag.key.as_deref().unwrap_or_default(); + if !seen_keys.insert(key) { + return Err(std::io::Error::other(ERR_LIFECYCLE_FILTER_DUPLICATE_TAG_KEY)); + } + } + validate_lifecycle_size_bounds(and.object_size_greater_than, and.object_size_less_than)?; + } + + validate_lifecycle_size_bounds(filter.object_size_greater_than, filter.object_size_less_than)?; + + Ok(()) +} + +/// S3 requires a tag to carry a key; both key and value are length-bounded. +/// The DTO makes both optional, so a keyless tag has to be rejected here +/// rather than silently matching nothing. +fn validate_lifecycle_tag(tag: &s3s::dto::Tag) -> Result<(), std::io::Error> { + let key = tag.key.as_deref().unwrap_or_default(); + let value = tag.value.as_deref().unwrap_or_default(); + if key.is_empty() || key.chars().count() > MAX_TAG_KEY_LEN || value.chars().count() > MAX_TAG_VALUE_LEN { + return Err(std::io::Error::other(ERR_LIFECYCLE_FILTER_INVALID_TAG)); + } + Ok(()) +} + +fn validate_lifecycle_size_bounds(greater_than: Option, less_than: Option) -> Result<(), std::io::Error> { + if greater_than.is_some_and(|size| size < 0) || less_than.is_some_and(|size| size < 0) { + return Err(std::io::Error::other(ERR_LIFECYCLE_FILTER_NEGATIVE_SIZE)); + } + if let (Some(greater_than), Some(less_than)) = (greater_than, less_than) + && greater_than >= less_than + { + return Err(std::io::Error::other(ERR_LIFECYCLE_FILTER_SIZE_RANGE)); + } + Ok(()) +} + fn lifecycle_rule_prefix(rule: &LifecycleRule) -> Option<&str> { // Prefer a non-empty legacy prefix; treat an empty legacy prefix as if it were not set if let Some(p) = rule.prefix.as_deref() @@ -289,6 +439,10 @@ impl Lifecycle for BucketLifecycleConfiguration { { return true; } + // A positive count is an action on its own (the MinIO count-only + // form). Zero means "no count constraint" here, exactly as the + // batch limit path reads it, and a negative count is corrupt — + // neither makes the rule active (backlog#2201). if let Some(newer_noncurrent_versions) = rule_noncurrent_version_expiration.newer_noncurrent_versions && newer_noncurrent_versions > 0 { @@ -611,18 +765,44 @@ impl Lifecycle for BucketLifecycleConfiguration { continue; } + // A retention count that PUT validation would have rejected can + // only come from older persistence or an import. Take no action + // for the rule instead of reading the negative value as "retain + // (almost) everything", which is how such a rule silently + // stopped deleting versions (backlog#2201). + if !obj.is_latest && rule_has_corrupt_retention_count(rule) { + debug!( + event = EVENT_LIFECYCLE_NONCURRENT_EXPIRY_SKIPPED, + component = LOG_COMPONENT_ECSTORE, + subsystem = LOG_SUBSYSTEM_LIFECYCLE, + object = %obj.name, + rule_id = %rule.id.clone().unwrap_or_default(), + reason = "corrupt_newer_noncurrent_versions", + "Skipped noncurrent expiration for a rule with an invalid retention count" + ); + continue; + } + if !obj.is_latest && let Some(ref noncurrent_version_expiration) = rule.noncurrent_version_expiration && let Some(retain_newer_noncurrent_versions) = noncurrent_version_expiration.newer_noncurrent_versions - && newer_noncurrent_versions < usize::try_from(retain_newer_noncurrent_versions).unwrap_or(usize::MAX) + && let Some(retained) = retained_noncurrent_versions(retain_newer_noncurrent_versions) + && newer_noncurrent_versions < retained { continue; } if !obj.is_latest && let Some(ref noncurrent_version_expiration) = rule.noncurrent_version_expiration - && let Some(noncurrent_days) = noncurrent_version_expiration.noncurrent_days + && (noncurrent_version_expiration.noncurrent_days.is_some() + || noncurrent_version_expiration + .newer_noncurrent_versions + .is_some_and(|count| count > 0)) { + // A count-only rule (MinIO extension) has no age condition: + // every version past the retained count is due as soon as it + // became noncurrent, i.e. zero days after the successor. + let noncurrent_days = noncurrent_version_expiration.noncurrent_days.unwrap_or(0); if let Some(successor_mod_time) = obj.successor_mod_time { let expected_expiry = expected_expiry_time(successor_mod_time, noncurrent_days); if now.unix_timestamp() >= expected_expiry.unix_timestamp() { @@ -785,15 +965,18 @@ impl Lifecycle for BucketLifecycleConfiguration { for rule in filter_rules.iter() { if let Some(ref noncurrent_version_expiration) = rule.noncurrent_version_expiration { return if let Some(newer_noncurrent_versions) = noncurrent_version_expiration.newer_noncurrent_versions { - if newer_noncurrent_versions == 0 { + // Zero means "no count constraint"; a negative count is + // corrupt and must not be read as "retain everything" + // (backlog#2201). Neither yields a limit event. + let Some(retained) = retained_noncurrent_versions(newer_noncurrent_versions).filter(|c| *c > 0) else { continue; - } + }; Event { action: IlmAction::DeleteVersionAction, rule_id: rule.id.clone().unwrap_or_default(), noncurrent_days: u32::try_from(noncurrent_version_expiration.noncurrent_days.unwrap_or(0)) .unwrap_or(u32::MAX), - newer_noncurrent_versions: usize::try_from(newer_noncurrent_versions).unwrap_or(usize::MAX), + newer_noncurrent_versions: retained, due: Some(OffsetDateTime::UNIX_EPOCH), storage_class: "".into(), } @@ -1138,7 +1321,11 @@ mod tests { use super::*; use metrics_util::MetricKind; use metrics_util::debugging::{DebugValue, DebuggingRecorder}; - use s3s::dto::{LifecycleRuleFilter, TransitionStorageClass}; + use s3s::dto::{ + LifecycleRuleAndOperator, LifecycleRuleFilter, NoncurrentVersionExpiration, NoncurrentVersionTransition, + TransitionStorageClass, + }; + use s3s::xml::{Deserialize as XmlDeserialize, SerializeContent as XmlSerializeContent}; use serial_test::serial; use std::sync::Arc; use time::macros::datetime; @@ -4177,6 +4364,506 @@ mod tests { /// /// Case counts are tuned so the whole module runs in seconds inside the /// default CI test job. + // ---- backlog#2201: retention-count and Filter invariants ----------------- + + fn rule_with_noncurrent_expiration(expiration: NoncurrentVersionExpiration) -> LifecycleRule { + LifecycleRule { + status: ExpirationStatus::from_static(ExpirationStatus::ENABLED), + expiration: None, + abort_incomplete_multipart_upload: None, + del_marker_expiration: None, + filter: None, + id: Some("noncurrent".to_string()), + noncurrent_version_expiration: Some(expiration), + noncurrent_version_transitions: None, + prefix: None, + transitions: None, + } + } + + fn rule_with_filter(filter: LifecycleRuleFilter) -> LifecycleRule { + LifecycleRule { + status: ExpirationStatus::from_static(ExpirationStatus::ENABLED), + expiration: Some(LifecycleExpiration { + days: Some(1), + ..Default::default() + }), + abort_incomplete_multipart_upload: None, + del_marker_expiration: None, + filter: Some(filter), + id: Some("filtered".to_string()), + noncurrent_version_expiration: None, + noncurrent_version_transitions: None, + prefix: None, + transitions: None, + } + } + + fn config_with_rules(rules: Vec) -> BucketLifecycleConfiguration { + BucketLifecycleConfiguration { + expiry_updated_at: None, + rules, + } + } + + fn tag(key: &str, value: &str) -> s3s::dto::Tag { + s3s::dto::Tag { + key: Some(key.to_string()), + value: Some(value.to_string()), + } + } + + #[tokio::test] + async fn validate_rejects_negative_newer_noncurrent_versions() { + // A negative retention count used to be accepted and then read as + // usize::MAX during evaluation, so the rule silently stopped deleting + // versions (backlog#2201). + let lc = config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(30), + newer_noncurrent_versions: Some(-1), + })]); + + let err = lc + .validate(&ObjectLockConfiguration::default()) + .await + .expect_err("a negative retention count must be rejected"); + + assert_eq!(err.to_string(), ERR_LIFECYCLE_INVALID_NEWER_NONCURRENT_VERSIONS); + assert_ne!(err.kind(), LIFECYCLE_MALFORMED_XML_ERROR_KIND, "value errors stay InvalidArgument"); + } + + #[tokio::test] + async fn validate_rejects_negative_newer_noncurrent_versions_on_transition() { + let mut rule = rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(30), + newer_noncurrent_versions: None, + }); + rule.noncurrent_version_transitions = Some(vec![NoncurrentVersionTransition { + newer_noncurrent_versions: Some(-3), + noncurrent_days: Some(1), + storage_class: Some(TransitionStorageClass::from_static(TransitionStorageClass::GLACIER)), + }]); + + // The transition validator already refuses a negative count, and it runs + // first, so this pins the rejection rather than the message. The gap + // this PR closes is the expiration side, which had no such check. + config_with_rules(vec![rule]) + .validate(&ObjectLockConfiguration::default()) + .await + .expect_err("a negative retention count on a transition must be rejected"); + } + + #[tokio::test] + async fn zero_newer_noncurrent_versions_means_no_count_constraint() { + // Zero carries no constraint, matching how the batch limit path has + // always read it. Alongside an age condition the rule is valid; on its + // own it says nothing, so the rule has no action. + config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(30), + newer_noncurrent_versions: Some(0), + })]) + .validate(&ObjectLockConfiguration::default()) + .await + .expect("zero count alongside NoncurrentDays is valid"); + + let err = config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: None, + newer_noncurrent_versions: Some(0), + })]) + .validate(&ObjectLockConfiguration::default()) + .await + .expect_err("a zero count on its own is not an action"); + assert_eq!(err.to_string(), ERR_LIFECYCLE_RULE_MUST_HAVE_ACTION); + } + + #[tokio::test] + async fn validate_accepts_count_only_noncurrent_expiration() { + // MinIO extension: NewerNoncurrentVersions with no NoncurrentDays. It + // used to be rejected as an actionless rule (backlog#2201). + let lc = config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: None, + newer_noncurrent_versions: Some(2), + })]); + + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("a count-only noncurrent expiration rule is accepted"); + } + + #[tokio::test] + async fn eval_inner_expires_versions_beyond_count_only_retention() { + // Count-only rules have no age condition: everything past the retained + // count is due as soon as it became noncurrent. + let lc = config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: None, + newer_noncurrent_versions: Some(2), + })]); + let opts = ObjectOpts { + name: "obj".to_string(), + mod_time: Some(datetime!(2025-01-15 10:30:45 UTC)), + successor_mod_time: Some(datetime!(2025-01-15 10:30:45 UTC)), + is_latest: false, + num_versions: 5, + ..Default::default() + }; + + // Rank 2 is the third-newest noncurrent version: past a retention of 2. + let expired = lc.eval_inner(&opts, datetime!(2025-01-15 10:30:46 UTC), 2).await; + assert_eq!(expired.action, IlmAction::DeleteVersionAction); + assert_eq!(expired.rule_id, "noncurrent"); + + // Rank 1 is still within the retained count. + let retained = lc.eval_inner(&opts, datetime!(2025-01-15 10:30:46 UTC), 1).await; + assert_eq!(retained.action, IlmAction::NoneAction); + } + + #[tokio::test] + #[serial] + async fn eval_inner_keeps_age_condition_when_count_and_days_are_set() { + // With both set, the count gates which versions are candidates and the + // age condition still decides when they are due. + with_default_ilm_process_time(|| {}); + let lc = config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(10), + newer_noncurrent_versions: Some(1), + })]); + let opts = ObjectOpts { + name: "obj".to_string(), + mod_time: Some(datetime!(2025-01-01 00:00:00 UTC)), + successor_mod_time: Some(datetime!(2025-01-01 00:00:00 UTC)), + is_latest: false, + num_versions: 3, + ..Default::default() + }; + + let too_young = lc.eval_inner(&opts, datetime!(2025-01-05 00:00:00 UTC), 2).await; + assert_eq!(too_young.action, IlmAction::NoneAction, "the age condition still applies"); + + let due = lc.eval_inner(&opts, datetime!(2025-01-20 00:00:00 UTC), 2).await; + assert_eq!(due.action, IlmAction::DeleteVersionAction); + } + + #[tokio::test] + async fn eval_inner_takes_no_action_for_a_corrupt_retention_count() { + // Reachable only from older persistence or an import; it must not be + // read as "retain everything", and it must not delete either. + let lc = config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(1), + newer_noncurrent_versions: Some(-1), + })]); + let opts = ObjectOpts { + name: "obj".to_string(), + mod_time: Some(datetime!(2025-01-01 00:00:00 UTC)), + successor_mod_time: Some(datetime!(2025-01-01 00:00:00 UTC)), + is_latest: false, + num_versions: 3, + ..Default::default() + }; + + let event = lc.eval_inner(&opts, datetime!(2025-06-01 00:00:00 UTC), 2).await; + + assert_eq!(event.action, IlmAction::NoneAction); + } + + #[test] + fn corrupt_retention_count_is_detected_on_either_action() { + let mut transition_rule = rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(1), + newer_noncurrent_versions: Some(0), + }); + transition_rule.noncurrent_version_transitions = Some(vec![NoncurrentVersionTransition { + newer_noncurrent_versions: Some(-1), + noncurrent_days: Some(1), + storage_class: Some(TransitionStorageClass::from_static(TransitionStorageClass::GLACIER)), + }]); + + assert!(lifecycle_has_corrupt_retention_count(&config_with_rules(vec![ + rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(1), + newer_noncurrent_versions: Some(-1), + }) + ]))); + assert!(lifecycle_has_corrupt_retention_count(&config_with_rules(vec![transition_rule]))); + assert!(!lifecycle_has_corrupt_retention_count(&config_with_rules(vec![ + rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(1), + newer_noncurrent_versions: Some(3), + }) + ]))); + } + + #[test] + fn count_only_rules_are_active_only_for_a_positive_count() { + let positive = config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: None, + newer_noncurrent_versions: Some(2), + })]); + assert!(positive.has_active_rules("")); + + let corrupt = config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: None, + newer_noncurrent_versions: Some(-1), + })]); + assert!(!corrupt.has_active_rules(""), "a corrupt retention count must not make a rule active"); + } + + #[tokio::test] + async fn noncurrent_versions_expiration_limit_ignores_a_corrupt_count() { + // The batch path must not read a negative count as "retain everything". + let lc = Arc::new(config_with_rules(vec![rule_with_noncurrent_expiration(NoncurrentVersionExpiration { + noncurrent_days: Some(1), + newer_noncurrent_versions: Some(-1), + })])); + let opts = ObjectOpts { + name: "obj".to_string(), + mod_time: Some(datetime!(2025-01-01 00:00:00 UTC)), + is_latest: false, + ..Default::default() + }; + + let event = lc.noncurrent_versions_expiration_limit(&opts).await; + + assert_eq!(event.action, IlmAction::NoneAction); + assert_eq!(event.newer_noncurrent_versions, 0); + } + + #[tokio::test] + async fn validate_covers_filter_invariants() { + struct Case { + name: &'static str, + filter: LifecycleRuleFilter, + expected: Option<(&'static str, std::io::ErrorKind)>, + } + + let cases = vec![ + Case { + // AWS documents an empty Filter as "every object in the bucket". + name: "empty filter applies to all objects", + filter: LifecycleRuleFilter::default(), + expected: None, + }, + Case { + name: "single prefix predicate", + filter: LifecycleRuleFilter { + prefix: Some("logs/".to_string()), + ..Default::default() + }, + expected: None, + }, + Case { + name: "two top-level predicates", + filter: LifecycleRuleFilter { + prefix: Some("logs/".to_string()), + tag: Some(tag("env", "prod")), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_TOO_MANY_PREDICATES, LIFECYCLE_MALFORMED_XML_ERROR_KIND)), + }, + Case { + name: "prefix alongside And", + filter: LifecycleRuleFilter { + prefix: Some("logs/".to_string()), + and: Some(LifecycleRuleAndOperator { + prefix: Some("logs/".to_string()), + tags: Some(vec![tag("env", "prod")]), + ..Default::default() + }), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_TOO_MANY_PREDICATES, LIFECYCLE_MALFORMED_XML_ERROR_KIND)), + }, + Case { + name: "And with a single member", + filter: LifecycleRuleFilter { + and: Some(LifecycleRuleAndOperator { + prefix: Some("logs/".to_string()), + ..Default::default() + }), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_AND_TOO_FEW_PREDICATES, LIFECYCLE_MALFORMED_XML_ERROR_KIND)), + }, + Case { + name: "And with two members", + filter: LifecycleRuleFilter { + and: Some(LifecycleRuleAndOperator { + prefix: Some("logs/".to_string()), + tags: Some(vec![tag("env", "prod")]), + ..Default::default() + }), + ..Default::default() + }, + expected: None, + }, + Case { + name: "And with two tags", + filter: LifecycleRuleFilter { + and: Some(LifecycleRuleAndOperator { + tags: Some(vec![tag("env", "prod"), tag("team", "storage")]), + ..Default::default() + }), + ..Default::default() + }, + expected: None, + }, + Case { + name: "And repeating a tag key", + filter: LifecycleRuleFilter { + and: Some(LifecycleRuleAndOperator { + tags: Some(vec![tag("env", "prod"), tag("env", "dev")]), + ..Default::default() + }), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_DUPLICATE_TAG_KEY, std::io::ErrorKind::Other)), + }, + Case { + name: "empty tag key", + filter: LifecycleRuleFilter { + tag: Some(tag("", "prod")), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_INVALID_TAG, std::io::ErrorKind::Other)), + }, + Case { + name: "missing tag key", + filter: LifecycleRuleFilter { + tag: Some(s3s::dto::Tag { + key: None, + value: Some("prod".to_string()), + }), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_INVALID_TAG, std::io::ErrorKind::Other)), + }, + Case { + name: "tag key at the limit", + filter: LifecycleRuleFilter { + tag: Some(tag(&"k".repeat(MAX_TAG_KEY_LEN), "prod")), + ..Default::default() + }, + expected: None, + }, + Case { + name: "tag key past the limit", + filter: LifecycleRuleFilter { + tag: Some(tag(&"k".repeat(MAX_TAG_KEY_LEN + 1), "prod")), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_INVALID_TAG, std::io::ErrorKind::Other)), + }, + Case { + name: "tag value past the limit", + filter: LifecycleRuleFilter { + tag: Some(tag("env", &"v".repeat(MAX_TAG_VALUE_LEN + 1))), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_INVALID_TAG, std::io::ErrorKind::Other)), + }, + Case { + name: "negative ObjectSizeGreaterThan", + filter: LifecycleRuleFilter { + object_size_greater_than: Some(-1), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_NEGATIVE_SIZE, std::io::ErrorKind::Other)), + }, + Case { + name: "negative ObjectSizeLessThan", + filter: LifecycleRuleFilter { + object_size_less_than: Some(-5), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_NEGATIVE_SIZE, std::io::ErrorKind::Other)), + }, + Case { + name: "inverted size range inside And", + filter: LifecycleRuleFilter { + and: Some(LifecycleRuleAndOperator { + object_size_greater_than: Some(100), + object_size_less_than: Some(100), + ..Default::default() + }), + ..Default::default() + }, + expected: Some((ERR_LIFECYCLE_FILTER_SIZE_RANGE, std::io::ErrorKind::Other)), + }, + Case { + name: "valid size range inside And", + filter: LifecycleRuleFilter { + and: Some(LifecycleRuleAndOperator { + object_size_greater_than: Some(1), + object_size_less_than: Some(2), + ..Default::default() + }), + ..Default::default() + }, + expected: None, + }, + ]; + + for case in cases { + let result = config_with_rules(vec![rule_with_filter(case.filter)]) + .validate(&ObjectLockConfiguration::default()) + .await; + match (case.expected, result) { + (None, Ok(())) => {} + (None, Err(err)) => panic!("{}: expected acceptance, got {err}", case.name), + (Some((message, _)), Ok(())) => panic!("{}: expected rejection with {message}", case.name), + (Some((message, kind)), Err(err)) => { + assert_eq!(err.to_string(), message, "{}", case.name); + assert_eq!(err.kind(), kind, "{}: wrong S3 error category", case.name); + } + } + } + } + + #[tokio::test] + async fn validate_keeps_legacy_prefix_and_filter_mutually_exclusive() { + let mut rule = rule_with_filter(LifecycleRuleFilter { + prefix: Some("logs/".to_string()), + ..Default::default() + }); + rule.prefix = Some("legacy/".to_string()); + + let err = config_with_rules(vec![rule]) + .validate(&ObjectLockConfiguration::default()) + .await + .expect_err("legacy Prefix and Filter cannot both be present"); + + assert_eq!(err.to_string(), ERR_LIFECYCLE_PREFIX_FILTER_CONFLICT); + } + + #[test] + fn count_only_rule_round_trips_through_xml() { + // The MinIO count-only form has to survive the wire codec, or the rule + // this PR now accepts could not be persisted and read back. + let xml = br#"count-onlyEnabled2"#; + let mut deserializer = s3s::xml::Deserializer::new(xml); + let parsed = + ::deserialize(&mut deserializer).expect("count-only XML parses"); + + let expiration = parsed.rules[0] + .noncurrent_version_expiration + .as_ref() + .expect("noncurrent expiration is present"); + assert_eq!(expiration.newer_noncurrent_versions, Some(2)); + assert_eq!(expiration.noncurrent_days, None); + + let mut buf = Vec::new(); + let mut serializer = s3s::xml::Serializer::new(&mut buf); + XmlSerializeContent::serialize_content(&parsed, &mut serializer).expect("count-only config serializes"); + let serialized = String::from_utf8(buf).expect("serialized XML is UTF-8"); + assert!( + serialized.contains("2"), + "retention count survives the round trip: {serialized}" + ); + assert!( + !serialized.contains(""), + "a count-only rule must not gain an age condition: {serialized}" + ); + } + mod proptests { use super::*; use proptest::prelude::*; diff --git a/crates/lifecycle/src/evaluator.rs b/crates/lifecycle/src/evaluator.rs index 80bd61de6..a11b0f550 100644 --- a/crates/lifecycle/src/evaluator.rs +++ b/crates/lifecycle/src/evaluator.rs @@ -22,7 +22,10 @@ use rustfs_replication::ReplicationStatusType; use rustfs_scanner_metrics::metrics::IlmAction; use crate::object_lock; -use crate::{Event, Lifecycle, ObjectOpts, expiration_action_has_valid_target}; +use crate::{ + Event, LIFECYCLE_CORRUPT_RULE_ERROR_KIND, Lifecycle, ObjectOpts, expiration_action_has_valid_target, + lifecycle_has_corrupt_retention_count, +}; const LOG_COMPONENT_ECSTORE: &str = "ecstore"; const LOG_SUBSYSTEM_LIFECYCLE: &str = "lifecycle"; @@ -155,6 +158,17 @@ impl Evaluator { format!("number of versions mismatch, expected {}, got {}", objs[0].num_versions, objs.len()), )); } + // PUT validation rejects a negative retention count, so a rule that + // carries one came from older persistence or an import. Report it + // instead of evaluating a configuration that cannot be honoured; + // `eval_inner` independently takes no action for such a rule + // (backlog#2201). + if lifecycle_has_corrupt_retention_count(&self.policy) { + return Err(std::io::Error::new( + LIFECYCLE_CORRUPT_RULE_ERROR_KIND, + "lifecycle configuration carries a negative 'NewerNoncurrentVersions'", + )); + } Ok(self.eval_inner(objs, OffsetDateTime::now_utc()).await) } } diff --git a/rustfs/src/app/bucket_usecase.rs b/rustfs/src/app/bucket_usecase.rs index 7dada3492..efd40cc6d 100644 --- a/rustfs/src/app/bucket_usecase.rs +++ b/rustfs/src/app/bucket_usecase.rs @@ -27,8 +27,8 @@ use super::storage_api::bucket_usecase::bucket::target::BucketTarget; use super::storage_api::bucket_usecase::bucket::{ ObjectLockConfigExt as _, VersioningConfigExt as _, lifecycle::bucket_lifecycle_ops::{ - enqueue_expiry_for_existing_objects, enqueue_transition_for_existing_objects, run_stale_multipart_upload_cleanup_once, - validate_lifecycle_config, validate_transition_tier, + LIFECYCLE_MALFORMED_XML_ERROR_KIND, enqueue_expiry_for_existing_objects, enqueue_transition_for_existing_objects, + run_stale_multipart_upload_cleanup_once, validate_lifecycle_config, validate_transition_tier, }, metadata::{ BUCKET_CORS_CONFIG, BUCKET_LIFECYCLE_CONFIG, BUCKET_NOTIFICATION_CONFIG, BUCKET_POLICY_CONFIG, @@ -1187,6 +1187,21 @@ fn validate_lifecycle_rule_status(rules: &[LifecycleRule]) -> std::result::Resul Ok(()) } +/// Map a lifecycle validation failure onto the S3 error the client should see. +/// +/// The validator reports a schema-shape violation (a `Filter` with more than +/// one predicate, a one-member `And`) with +/// [`LIFECYCLE_MALFORMED_XML_ERROR_KIND`]; AWS answers those with +/// `MalformedXML`. Everything else is a value the schema allows but S3 refuses, +/// which stays `InvalidArgument` — the code this path has always returned +/// (backlog#2201). +fn lifecycle_validation_error(err: &std::io::Error) -> S3Error { + if err.kind() == LIFECYCLE_MALFORMED_XML_ERROR_KIND { + return S3Error::with_message(S3ErrorCode::MalformedXML, format!("Malformed XML: {err}")); + } + s3_error!(InvalidArgument, "{err}") +} + fn lifecycle_has_transition_rules(config: &BucketLifecycleConfiguration) -> bool { config.rules.iter().any(|rule| { rule.status == ExpirationStatus::from_static(ExpirationStatus::ENABLED) @@ -2270,7 +2285,7 @@ impl DefaultBucketUsecase { }; if let Err(err) = validate_lifecycle_config(&input_cfg, &rcfg).await { - return Err(s3_error!(InvalidArgument, "{err}")); + return Err(lifecycle_validation_error(&err)); } if let Err(err) = validate_transition_tier(&input_cfg).await { @@ -3985,6 +4000,70 @@ mod tests { assert_eq!(rules[2].id.as_deref(), Some("rule-2")); } + #[tokio::test] + async fn put_bucket_lifecycle_validation_errors_keep_their_s3_code() { + // The PUT path answers a schema-shape violation with MalformedXML and a + // rejected value with InvalidArgument. Both categories are produced by + // the real validator here, so the mapping cannot drift from it + // (backlog#2201). + let malformed = validate_lifecycle_config( + &BucketLifecycleConfiguration { + expiry_updated_at: None, + rules: vec![LifecycleRule { + status: ExpirationStatus::from_static(ExpirationStatus::ENABLED), + expiration: Some(LifecycleExpiration { + days: Some(1), + ..Default::default() + }), + abort_incomplete_multipart_upload: None, + del_marker_expiration: None, + filter: Some(s3s::dto::LifecycleRuleFilter { + prefix: Some("logs/".to_string()), + tag: Some(s3s::dto::Tag { + key: Some("env".to_string()), + value: Some("prod".to_string()), + }), + ..Default::default() + }), + id: Some("two-predicates".to_string()), + noncurrent_version_expiration: None, + noncurrent_version_transitions: None, + prefix: None, + transitions: None, + }], + }, + &ObjectLockConfiguration::default(), + ) + .await + .expect_err("a Filter with two predicates is a schema violation"); + assert_eq!(*lifecycle_validation_error(&malformed).code(), S3ErrorCode::MalformedXML); + + let invalid_value = validate_lifecycle_config( + &BucketLifecycleConfiguration { + expiry_updated_at: None, + rules: vec![LifecycleRule { + status: ExpirationStatus::from_static(ExpirationStatus::ENABLED), + expiration: None, + abort_incomplete_multipart_upload: None, + del_marker_expiration: None, + filter: None, + id: Some("negative-count".to_string()), + noncurrent_version_expiration: Some(s3s::dto::NoncurrentVersionExpiration { + noncurrent_days: Some(30), + newer_noncurrent_versions: Some(-1), + }), + noncurrent_version_transitions: None, + prefix: None, + transitions: None, + }], + }, + &ObjectLockConfiguration::default(), + ) + .await + .expect_err("a negative retention count is rejected"); + assert_eq!(*lifecycle_validation_error(&invalid_value).code(), S3ErrorCode::InvalidArgument); + } + #[test] fn validate_lifecycle_rule_status_rejects_invalid_status() { let rules = vec![LifecycleRule { diff --git a/rustfs/src/app/storage_api.rs b/rustfs/src/app/storage_api.rs index e90f9d9ae..b488a0412 100644 --- a/rustfs/src/app/storage_api.rs +++ b/rustfs/src/app/storage_api.rs @@ -389,6 +389,12 @@ pub(crate) mod bucket { lc.validate(lock_config).await } + + /// The `std::io::ErrorKind` [`validate_lifecycle_config`] uses for a + /// lifecycle document that violates the published schema shape, which + /// the S3 boundary answers with `MalformedXML` (backlog#2201). + pub(crate) const LIFECYCLE_MALFORMED_XML_ERROR_KIND: std::io::ErrorKind = + crate::storage::storage_api::ecstore_bucket::lifecycle::lifecycle::LIFECYCLE_MALFORMED_XML_ERROR_KIND; } pub(crate) mod lifecycle_contract {