diff --git a/crates/ecstore/src/bucket/object_lock/objectlock_sys.rs b/crates/ecstore/src/bucket/object_lock/objectlock_sys.rs index 1ecea8253..d5c699068 100644 --- a/crates/ecstore/src/bucket/object_lock/objectlock_sys.rs +++ b/crates/ecstore/src/bucket/object_lock/objectlock_sys.rs @@ -52,6 +52,7 @@ pub fn is_retention_active(mode: &str, retain_until_date: Option<&s3s::dto::Date /// Check if retention modification is blocked for the given object. pub fn check_retention_for_modification( user_defined: &std::collections::HashMap, + new_mode: Option<&str>, new_retain_until: Option, bypass_governance: bool, ) -> Option { @@ -67,6 +68,7 @@ pub fn check_retention_for_modification( } let existing_retain_until = retention.retain_until_date.as_ref().map(|d| OffsetDateTime::from(d.clone())); + let mode_changed = new_mode != Some(mode_str); // Check if new retention period is shorter than existing let is_shortening = match (&existing_retain_until, &new_retain_until) { @@ -78,7 +80,7 @@ pub fn check_retention_for_modification( // COMPLIANCE mode: cannot shorten retention at all (even with bypass) // Can only extend the retention period if mode_str == ObjectLockRetentionMode::COMPLIANCE { - if is_shortening { + if mode_changed || is_shortening { return Some(ObjectLockBlockReason::Retention { mode: mode_str.to_string(), retain_until: existing_retain_until, @@ -93,7 +95,7 @@ pub fn check_retention_for_modification( // - Extending retention: allowed without bypass permission // - Shortening/removing retention: requires bypass permission if mode_str == ObjectLockRetentionMode::GOVERNANCE { - if is_shortening && !bypass_governance { + if (mode_changed || is_shortening) && !bypass_governance { return Some(ObjectLockBlockReason::Retention { mode: mode_str.to_string(), retain_until: existing_retain_until, @@ -380,7 +382,7 @@ mod tests { // No existing retention - modification should be allowed let user_defined = std::collections::HashMap::new(); let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(30)); - assert!(check_retention_for_modification(&user_defined, new_retain, false).is_none()); + assert!(check_retention_for_modification(&user_defined, None, new_retain, false).is_none()); } #[test] @@ -398,7 +400,10 @@ mod tests { // Extending by another 30 days should be allowed let new_retain = Some(existing_retain + time::Duration::days(30)); - assert!(check_retention_for_modification(&user_defined, new_retain, false).is_none()); + assert!( + check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::COMPLIANCE), new_retain, false) + .is_none() + ); } #[test] @@ -416,7 +421,8 @@ mod tests { // Shortening to 30 days should be blocked let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(30)); - let result = check_retention_for_modification(&user_defined, new_retain, false); + let result = + check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::COMPLIANCE), new_retain, false); assert!(result.is_some()); assert!(matches!(result, Some(ObjectLockBlockReason::Retention { .. }))); } @@ -435,7 +441,7 @@ mod tests { ); // Clearing (None) should be blocked - let result = check_retention_for_modification(&user_defined, None, false); + let result = check_retention_for_modification(&user_defined, None, None, false); assert!(result.is_some()); } @@ -454,7 +460,8 @@ mod tests { // Shortening from 30 days to 15 days without bypass should be blocked let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(15)); - let result = check_retention_for_modification(&user_defined, new_retain, false); + let result = + check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::GOVERNANCE), new_retain, false); assert!(result.is_some()); } @@ -474,7 +481,10 @@ mod tests { // Extending from 30 days to 60 days without bypass should be allowed let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(60)); - assert!(check_retention_for_modification(&user_defined, new_retain, false).is_none()); + assert!( + check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::GOVERNANCE), new_retain, false) + .is_none() + ); } #[test] @@ -492,7 +502,75 @@ mod tests { // Shortening from 30 days to 15 days with bypass should be allowed let new_retain = Some(OffsetDateTime::now_utc() + time::Duration::days(15)); - assert!(check_retention_for_modification(&user_defined, new_retain, true).is_none()); + assert!( + check_retention_for_modification(&user_defined, Some(ObjectLockRetentionMode::GOVERNANCE), new_retain, true) + .is_none() + ); + } + + #[test] + fn test_check_retention_for_modification_governance_mode_change_without_bypass() { + let mut user_defined = std::collections::HashMap::new(); + let existing_retain = OffsetDateTime::now_utc() + time::Duration::days(30); + user_defined.insert("x-amz-object-lock-mode".to_string(), "GOVERNANCE".to_string()); + user_defined.insert( + "x-amz-object-lock-retain-until-date".to_string(), + existing_retain + .format(&time::format_description::well_known::Rfc3339) + .unwrap(), + ); + + let result = check_retention_for_modification( + &user_defined, + Some(ObjectLockRetentionMode::COMPLIANCE), + Some(existing_retain), + false, + ); + assert!(result.is_some()); + } + + #[test] + fn test_check_retention_for_modification_governance_mode_change_with_bypass() { + let mut user_defined = std::collections::HashMap::new(); + let existing_retain = OffsetDateTime::now_utc() + time::Duration::days(30); + user_defined.insert("x-amz-object-lock-mode".to_string(), "GOVERNANCE".to_string()); + user_defined.insert( + "x-amz-object-lock-retain-until-date".to_string(), + existing_retain + .format(&time::format_description::well_known::Rfc3339) + .unwrap(), + ); + + assert!( + check_retention_for_modification( + &user_defined, + Some(ObjectLockRetentionMode::COMPLIANCE), + Some(existing_retain), + true, + ) + .is_none() + ); + } + + #[test] + fn test_check_retention_for_modification_compliance_mode_change() { + let mut user_defined = std::collections::HashMap::new(); + let existing_retain = OffsetDateTime::now_utc() + time::Duration::days(30); + user_defined.insert("x-amz-object-lock-mode".to_string(), "COMPLIANCE".to_string()); + user_defined.insert( + "x-amz-object-lock-retain-until-date".to_string(), + existing_retain + .format(&time::format_description::well_known::Rfc3339) + .unwrap(), + ); + + let result = check_retention_for_modification( + &user_defined, + Some(ObjectLockRetentionMode::GOVERNANCE), + Some(existing_retain), + true, + ); + assert!(result.is_some()); } #[test] diff --git a/rustfs/src/app/bucket_usecase.rs b/rustfs/src/app/bucket_usecase.rs index 71fbc01ce..b72d2ecac 100644 --- a/rustfs/src/app/bucket_usecase.rs +++ b/rustfs/src/app/bucket_usecase.rs @@ -35,8 +35,10 @@ use rustfs_ecstore::bucket::{ BUCKET_VERSIONING_CONFIG, }, metadata_sys, + object_lock::ObjectLockApi, policy_sys::PolicySys, utils::serialize, + versioning::VersioningApi, versioning_sys::BucketVersioningSys, }; use rustfs_ecstore::client::object_api_utils::to_s3s_etag; @@ -69,6 +71,32 @@ fn to_internal_error(err: impl Display) -> S3Error { S3Error::with_message(S3ErrorCode::InternalError, format!("{err}")) } +fn versioning_configuration_has_object_lock_incompatible_settings(config: &VersioningConfiguration) -> bool { + config.suspended() + || config.exclude_folders.unwrap_or(false) + || config + .excluded_prefixes + .as_ref() + .is_some_and(|excluded_prefixes| !excluded_prefixes.is_empty()) +} + +async fn validate_bucket_versioning_update(bucket: &str, config: &VersioningConfiguration) -> S3Result<()> { + match metadata_sys::get_object_lock_config(bucket).await { + Ok((object_lock_config, _)) => { + if object_lock_config.enabled() && versioning_configuration_has_object_lock_incompatible_settings(config) { + return Err(S3Error::with_message( + S3ErrorCode::InvalidBucketState, + "An Object Lock configuration is present on this bucket, versioning cannot be suspended.".to_string(), + )); + } + } + Err(StorageError::ConfigNotFound) => {} + Err(err) => return Err(ApiError::from(err).into()), + } + + Ok(()) +} + fn create_bucket_exists_response(is_owner: bool) -> S3Result> { if is_owner { return Ok(S3Response::new(CreateBucketOutput::default())); @@ -1357,6 +1385,8 @@ impl DefaultBucketUsecase { .. } = req.input; + validate_bucket_versioning_update(&bucket, &versioning_configuration).await?; + let data = serialize_config(&versioning_configuration)?; metadata_sys::update(&bucket, BUCKET_VERSIONING_CONFIG, data) @@ -1629,6 +1659,16 @@ mod tests { req } + #[test] + fn versioning_configuration_has_object_lock_incompatible_settings_rejects_suspended() { + let config = VersioningConfiguration { + status: Some(BucketVersioningStatus::from_static(BucketVersioningStatus::SUSPENDED)), + ..Default::default() + }; + + assert!(versioning_configuration_has_object_lock_incompatible_settings(&config)); + } + #[test] fn resolve_notification_region_prefers_global_region() { let binding = resolve_notification_region(Some("us-east-1".parse().unwrap()), Some("ap-southeast-1".parse().unwrap())); diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index f72b6c897..79f86a921 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -554,6 +554,83 @@ pub(crate) async fn build_put_like_object_lock_metadata( Ok(Some(eval_metadata)) } +const MAXIMUM_RETENTION_DAYS: i32 = 36_500; +const MAXIMUM_RETENTION_YEARS: i32 = 100; + +fn invalid_object_lock_configuration(message: impl Into) -> S3Error { + S3Error::with_message(S3ErrorCode::MalformedXML, message.into()) +} + +fn invalid_retention_period(message: impl Into) -> S3Error { + let mut err = S3Error::with_message(S3ErrorCode::Custom("InvalidRetentionPeriod".into()), message.into()); + err.set_status_code(StatusCode::BAD_REQUEST); + err +} + +fn validate_default_retention_configuration(default_retention: &DefaultRetention) -> S3Result<()> { + let Some(mode) = default_retention.mode.as_ref() else { + return Err(invalid_object_lock_configuration("retention mode must be specified")); + }; + + match mode.as_str() { + ObjectLockRetentionMode::COMPLIANCE | ObjectLockRetentionMode::GOVERNANCE => {} + _ => { + return Err(invalid_object_lock_configuration(format!("unknown retention mode {}", mode.as_str()))); + } + } + + match (default_retention.days, default_retention.years) { + (Some(days), None) => { + if days <= 0 { + return Err(invalid_retention_period( + "Default retention period must be a positive integer value for 'Days'", + )); + } + if days > MAXIMUM_RETENTION_DAYS { + return Err(invalid_retention_period(format!("Default retention period too large for 'Days' {days}",))); + } + } + (None, Some(years)) => { + if years <= 0 { + return Err(invalid_retention_period( + "Default retention period must be a positive integer value for 'Years'", + )); + } + if years > MAXIMUM_RETENTION_YEARS { + return Err(invalid_retention_period(format!( + "Default retention period too large for 'Years' {years}", + ))); + } + } + (Some(_), Some(_)) => { + return Err(invalid_object_lock_configuration("either Days or Years must be specified, not both")); + } + (None, None) => { + return Err(invalid_object_lock_configuration("either Days or Years must be specified")); + } + } + + Ok(()) +} + +fn validate_object_lock_configuration_input(input_cfg: &ObjectLockConfiguration) -> S3Result<()> { + let enabled = input_cfg.object_lock_enabled.as_ref().map(ObjectLockEnabled::as_str); + if enabled != Some(ObjectLockEnabled::ENABLED) { + return Err(invalid_object_lock_configuration( + "only 'Enabled' value is allowed to ObjectLockEnabled element", + )); + } + + if let Some(rule) = input_cfg.rule.as_ref() { + let Some(default_retention) = rule.default_retention.as_ref() else { + return Err(invalid_object_lock_configuration("Rule must include DefaultRetention")); + }; + validate_default_retention_configuration(default_retention)?; + } + + Ok(()) +} + pub(crate) fn validate_existing_object_lock_for_write(existing_obj_info: &ObjectInfo) -> S3Result<()> { let legal_hold = get_object_legalhold_meta(&existing_obj_info.user_defined); if legal_hold @@ -1164,6 +1241,8 @@ impl DefaultObjectUsecase { .await .map_err(ApiError::from)?; + validate_object_lock_configuration_input(&input_cfg)?; + match metadata_sys::get_object_lock_config(&bucket).await { Ok(_) => {} Err(err) => { @@ -1237,6 +1316,7 @@ impl DefaultObjectUsecase { .as_ref() .and_then(|r| r.retain_until_date.as_ref()) .map(|d| OffsetDateTime::from(d.clone())); + let new_mode = retention.as_ref().and_then(|r| r.mode.as_ref()).map(|mode| mode.as_str()); // TODO(security): Known TOCTOU race condition (fix in future PR). // @@ -1270,7 +1350,7 @@ impl DefaultObjectUsecase { if let Ok(existing_obj_info) = store.get_object_info(&bucket, &key, &check_opts).await { let bypass_governance = has_bypass_governance_header(&req.headers); if let Some(block_reason) = - check_retention_for_modification(&existing_obj_info.user_defined, new_retain_until, bypass_governance) + check_retention_for_modification(&existing_obj_info.user_defined, new_mode, new_retain_until, bypass_governance) { return Err(S3Error::with_message(S3ErrorCode::AccessDenied, block_reason.error_message())); } @@ -5010,6 +5090,51 @@ mod tests { assert_eq!(err.code(), &S3ErrorCode::InternalError); } + #[test] + fn validate_object_lock_configuration_rejects_disabled_status() { + let cfg = ObjectLockConfiguration { + object_lock_enabled: Some(ObjectLockEnabled::from("Disabled".to_string())), + rule: None, + }; + + let err = validate_object_lock_configuration_input(&cfg).unwrap_err(); + assert_eq!(err.code(), &S3ErrorCode::MalformedXML); + } + + #[test] + fn validate_object_lock_configuration_rejects_invalid_default_retention_mode() { + let cfg = ObjectLockConfiguration { + object_lock_enabled: Some(ObjectLockEnabled::from_static(ObjectLockEnabled::ENABLED)), + rule: Some(ObjectLockRule { + default_retention: Some(DefaultRetention { + mode: Some(ObjectLockRetentionMode::from("abc".to_string())), + days: Some(1), + years: None, + }), + }), + }; + + let err = validate_object_lock_configuration_input(&cfg).unwrap_err(); + assert_eq!(err.code(), &S3ErrorCode::MalformedXML); + } + + #[test] + fn validate_object_lock_configuration_rejects_days_and_years_together() { + let cfg = ObjectLockConfiguration { + object_lock_enabled: Some(ObjectLockEnabled::from_static(ObjectLockEnabled::ENABLED)), + rule: Some(ObjectLockRule { + default_retention: Some(DefaultRetention { + mode: Some(ObjectLockRetentionMode::from_static(ObjectLockRetentionMode::GOVERNANCE)), + days: Some(1), + years: Some(1), + }), + }), + }; + + let err = validate_object_lock_configuration_input(&cfg).unwrap_err(); + assert_eq!(err.code(), &S3ErrorCode::MalformedXML); + } + #[tokio::test] async fn execute_put_object_retention_returns_internal_error_when_store_uninitialized() { let input = PutObjectRetentionInput::builder() diff --git a/scripts/s3-tests/excluded_tests.txt b/scripts/s3-tests/excluded_tests.txt index 295147e3c..eb4bbbd39 100644 --- a/scripts/s3-tests/excluded_tests.txt +++ b/scripts/s3-tests/excluded_tests.txt @@ -237,26 +237,6 @@ test_non_multipart_get_part test_non_multipart_sse_c_get_part test_object_copy_canned_acl test_object_header_acl_grants -test_object_lock_changing_mode_from_compliance -test_object_lock_changing_mode_from_governance_with_bypass -test_object_lock_changing_mode_from_governance_without_bypass -test_object_lock_get_obj_lock -test_object_lock_get_obj_metadata -test_object_lock_put_obj_lock -test_object_lock_put_obj_lock_invalid_days -test_object_lock_put_obj_lock_invalid_mode -test_object_lock_put_obj_lock_invalid_status -test_object_lock_put_obj_lock_invalid_years -test_object_lock_put_obj_lock_with_days_and_years -test_object_lock_put_obj_retention -test_object_lock_put_obj_retention_increase_period -test_object_lock_put_obj_retention_invalid_mode -test_object_lock_put_obj_retention_override_default_retention -test_object_lock_put_obj_retention_shorten_period -test_object_lock_put_obj_retention_shorten_period_bypass -test_object_lock_put_obj_retention_versionid -test_object_lock_suspend_versioning -test_object_lock_uploading_obj test_object_raw_get_x_amz_expires_not_expired test_object_raw_get_x_amz_expires_not_expired_tenant test_object_raw_get_x_amz_expires_out_max_range diff --git a/scripts/s3-tests/implemented_tests.txt b/scripts/s3-tests/implemented_tests.txt index e24a2691c..ec6f19e67 100644 --- a/scripts/s3-tests/implemented_tests.txt +++ b/scripts/s3-tests/implemented_tests.txt @@ -403,6 +403,26 @@ test_object_lock_delete_object_with_retention_and_marker test_object_lock_delete_multipart_object_with_legal_hold_on test_object_lock_delete_multipart_object_with_retention test_object_lock_multi_delete_object_with_retention +test_object_lock_changing_mode_from_compliance +test_object_lock_changing_mode_from_governance_with_bypass +test_object_lock_changing_mode_from_governance_without_bypass +test_object_lock_get_obj_lock +test_object_lock_get_obj_metadata +test_object_lock_put_obj_lock +test_object_lock_put_obj_lock_invalid_days +test_object_lock_put_obj_lock_invalid_mode +test_object_lock_put_obj_lock_invalid_status +test_object_lock_put_obj_lock_invalid_years +test_object_lock_put_obj_lock_with_days_and_years +test_object_lock_put_obj_retention +test_object_lock_put_obj_retention_increase_period +test_object_lock_put_obj_retention_invalid_mode +test_object_lock_put_obj_retention_override_default_retention +test_object_lock_put_obj_retention_shorten_period +test_object_lock_put_obj_retention_shorten_period_bypass +test_object_lock_put_obj_retention_versionid +test_object_lock_suspend_versioning +test_object_lock_uploading_obj # Checksum validation tests test_object_checksum_sha256