diff --git a/crates/ecstore/src/bucket/metadata_sys.rs b/crates/ecstore/src/bucket/metadata_sys.rs index f937e0eb2..97dbf9147 100644 --- a/crates/ecstore/src/bucket/metadata_sys.rs +++ b/crates/ecstore/src/bucket/metadata_sys.rs @@ -741,6 +741,8 @@ impl BucketMetadataSys { if let Some(config) = &bm.policy_config { Ok((config.clone(), bm.policy_config_updated_at)) + } else if !bm.policy_config_json.is_empty() { + Ok((serde_json::from_slice(&bm.policy_config_json)?, bm.policy_config_updated_at)) } else { Err(Error::ConfigNotFound) } @@ -1051,6 +1053,22 @@ mod tests { ); } + #[tokio::test] + async fn get_bucket_policy_rejects_malformed_cached_policy() { + let (_dirs, ecstore) = isolated_store_over_temp_disks().await; + let sys = BucketMetadataSys::new(ecstore); + let mut metadata = BucketMetadata::new("malformed-policy"); + metadata.policy_config_json = b"{".to_vec(); + sys.set("malformed-policy".to_string(), Arc::new(metadata)).await; + + let err = sys + .get_bucket_policy("malformed-policy") + .await + .expect_err("malformed persisted policy must not be treated as missing"); + + assert!(matches!(err, Error::Io(_)), "malformed persisted policy must surface its parse failure"); + } + fn target(bucket: &str, id: &str) -> BucketTarget { BucketTarget { source_bucket: bucket.to_string(), diff --git a/crates/ecstore/src/bucket/policy_sys.rs b/crates/ecstore/src/bucket/policy_sys.rs index 14e54252e..75a9a4473 100644 --- a/crates/ecstore/src/bucket/policy_sys.rs +++ b/crates/ecstore/src/bucket/policy_sys.rs @@ -15,23 +15,26 @@ use super::metadata_sys::get_bucket_metadata_sys; use crate::error::{Result, StorageError}; use rustfs_policy::policy::{BucketPolicy, BucketPolicyArgs}; -use tracing::info; pub struct PolicySys {} impl PolicySys { pub async fn is_allowed(args: &BucketPolicyArgs<'_>) -> bool { - match Self::get(args.bucket).await { - Ok(cfg) => return cfg.is_allowed(args).await, - Err(err) => { - if err != StorageError::ConfigNotFound { - info!("config get err {:?}", err); - } - } - } - - args.is_owner + matches!(Self::try_is_allowed(args).await, Ok(true)) } + + pub async fn try_is_allowed(args: &BucketPolicyArgs<'_>) -> Result { + Self::is_allowed_with_policy(args, Self::get(args.bucket).await).await + } + + async fn is_allowed_with_policy(args: &BucketPolicyArgs<'_>, policy: Result) -> Result { + match policy { + Ok(policy) => Ok(policy.is_allowed(args).await), + Err(StorageError::ConfigNotFound) => Ok(args.is_owner), + Err(err) => Err(err), + } + } + pub async fn get(bucket: &str) -> Result { let bucket_meta_sys_lock = get_bucket_metadata_sys()?; let bucket_meta_sys = bucket_meta_sys_lock.read().await; @@ -41,3 +44,91 @@ impl PolicySys { Ok(cfg) } } + +#[cfg(test)] +mod tests { + use super::{PolicySys, StorageError}; + use rustfs_policy::policy::action::{Action, S3Action}; + use rustfs_policy::policy::{BucketPolicy, BucketPolicyArgs}; + use std::collections::HashMap; + + fn args<'a>( + is_owner: bool, + groups: &'a Option>, + conditions: &'a HashMap>, + ) -> BucketPolicyArgs<'a> { + BucketPolicyArgs { + bucket: "bucket", + action: Action::S3Action(S3Action::GetObjectAction), + is_owner, + account: "account", + groups, + conditions, + object: "object", + } + } + + #[tokio::test] + async fn missing_policy_preserves_owner_and_iam_fallback_semantics() { + let groups = None; + let conditions = HashMap::new(); + assert!( + PolicySys::is_allowed_with_policy(&args(true, &groups, &conditions), Err(StorageError::ConfigNotFound),) + .await + .expect("missing policy should preserve owner access") + ); + assert!( + !PolicySys::is_allowed_with_policy(&args(false, &groups, &conditions), Err(StorageError::ConfigNotFound),) + .await + .expect("missing policy should defer non-owner access to IAM") + ); + } + + #[tokio::test] + async fn policy_load_failures_propagate() { + let groups = None; + let conditions = HashMap::new(); + for (failure, expected_message) in [ + (StorageError::Io(std::io::Error::other("policy read failed")), "policy read failed"), + ( + StorageError::other("bucket metadata sys not initialized for this instance"), + "bucket metadata sys not initialized for this instance", + ), + ] { + let result = PolicySys::is_allowed_with_policy(&args(true, &groups, &conditions), Err(failure)).await; + + assert!( + matches!(result, Err(StorageError::Io(ref err)) if err.to_string().contains(expected_message)), + "policy I/O and uninitialized metadata failures must propagate instead of granting owner access" + ); + } + } + + #[tokio::test] + async fn explicit_bucket_deny_precedes_iam_allow() { + let groups = None; + let conditions = HashMap::new(); + let policy: BucketPolicy = serde_json::from_str( + r#"{ + "Version":"2012-10-17", + "Statement":[{ + "Effect":"Deny", + "Principal":{"AWS":"*"}, + "Action":["s3:GetObject"], + "Resource":["arn:aws:s3:::bucket/*"] + }] +}"#, + ) + .expect("deny policy should parse"); + + let bucket_allowed = PolicySys::is_allowed_with_policy(&args(true, &groups, &conditions), Ok(policy)) + .await + .expect("loaded bucket policy should evaluate"); + let iam_allowed = true; + let request_allowed = bucket_allowed && iam_allowed; + + assert!(iam_allowed, "test precondition: IAM grants the action"); + assert!(!bucket_allowed, "test precondition: bucket policy explicitly denies the action"); + assert!(!request_allowed, "explicit bucket Deny must reject before IAM Allow fallback"); + } +} diff --git a/rustfs/src/app/bucket_usecase.rs b/rustfs/src/app/bucket_usecase.rs index dd94e0a15..0fe1474a1 100644 --- a/rustfs/src/app/bucket_usecase.rs +++ b/rustfs/src/app/bucket_usecase.rs @@ -1810,7 +1810,7 @@ impl DefaultBucketUsecase { client_info, ); - let read_allowed = PolicySys::is_allowed(&BucketPolicyArgs { + let read_allowed = PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: &bucket, action: Action::S3Action(S3Action::ListBucketAction), is_owner: false, @@ -1819,9 +1819,10 @@ impl DefaultBucketUsecase { conditions: &conditions, object: "", }) - .await; + .await + .map_err(ApiError::from)?; - let write_allowed = PolicySys::is_allowed(&BucketPolicyArgs { + let write_allowed = PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: &bucket, action: Action::S3Action(S3Action::PutObjectAction), is_owner: false, @@ -1830,7 +1831,8 @@ impl DefaultBucketUsecase { conditions: &conditions, object: "", }) - .await; + .await + .map_err(ApiError::from)?; let mut is_public = read_allowed || write_allowed; let ignore_public_acls = match metadata_sys::get_public_access_block_config(&bucket).await { diff --git a/rustfs/src/error.rs b/rustfs/src/error.rs index f4cf54dd1..63e6f3bf8 100644 --- a/rustfs/src/error.rs +++ b/rustfs/src/error.rs @@ -494,6 +494,14 @@ mod tests { assert!(downcast_storage_error.is_some()); } + #[test] + fn policy_metadata_read_failure_maps_to_internal_error() { + let api_error = ApiError::from(StorageError::Io(IoError::other("policy read failed"))); + + assert_eq!(api_error.code, S3ErrorCode::InternalError); + assert!(api_error.source.is_some()); + } + #[test] fn test_kms_service_unavailable_maps_to_retryable_error() { let api_error = ApiError::from(StorageError::other(KmsUnavailableError)); diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index 35360763f..a5df755ee 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -505,7 +505,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R let owner_can_bypass_deny = owner_can_bypass_policy_deny(is_owner, &action); if !bucket_name.is_empty() && !owner_can_bypass_deny - && !PolicySys::is_allowed(&BucketPolicyArgs { + && !PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: bucket_name, action, // Early explicit-deny gate for bucket policy: use owner short-circuit path so @@ -517,6 +517,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R object: object.as_str(), }) .await + .map_err(ApiError::from)? { return Err(s3_error!(AccessDenied, "Access Denied")); } @@ -535,7 +536,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R }; let delete_version_allowed = iam_store.eval_prepared(&prepared, &delete_version_args).await; if !delete_version_allowed - && !PolicySys::is_allowed(&BucketPolicyArgs { + && !PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: bucket.as_str(), action: Action::S3Action(S3Action::DeleteObjectVersionAction), is_owner, @@ -545,6 +546,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R object: object.as_str(), }) .await + .map_err(ApiError::from)? { return Err(s3_error!(AccessDenied, "Access Denied")); } @@ -571,7 +573,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R return Ok(()); } - let policy_allowed_fallback = PolicySys::is_allowed(&BucketPolicyArgs { + let policy_allowed_fallback = PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: bucket.as_str(), action, is_owner, @@ -580,7 +582,8 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R conditions: &conditions, object: object.as_str(), }) - .await; + .await + .map_err(ApiError::from)?; if policy_allowed_fallback { authorize_table_data_plane_if_needed(action, bucket.as_str(), object.as_str(), cred, is_owner, &conditions, claims) @@ -605,7 +608,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R return Ok(()); } - if PolicySys::is_allowed(&BucketPolicyArgs { + if PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: bucket.as_str(), action: Action::S3Action(S3Action::ListBucketAction), is_owner, @@ -615,6 +618,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R object: object.as_str(), }) .await + .map_err(ApiError::from)? { return Ok(()); } @@ -688,7 +692,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R let bucket_name = bucket.as_str(); if !bucket_name.is_empty() - && !PolicySys::is_allowed(&BucketPolicyArgs { + && !PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: bucket_name, action, // Early explicit-deny gate for bucket policy in anonymous path. @@ -699,13 +703,14 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R object: object.as_str(), }) .await + .map_err(ApiError::from)? { return Err(s3_error!(AccessDenied, "Access Denied")); } if action != Action::S3Action(S3Action::ListAllMyBucketsAction) { if action == Action::S3Action(S3Action::DeleteObjectAction) && version_id.is_some() { - let delete_version_allowed = PolicySys::is_allowed(&BucketPolicyArgs { + let delete_version_allowed = PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: bucket.as_str(), action: Action::S3Action(S3Action::DeleteObjectVersionAction), is_owner: false, @@ -714,13 +719,14 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R conditions: &conditions, object: object.as_str(), }) - .await; + .await + .map_err(ApiError::from)?; if !delete_version_allowed { return Err(s3_error!(AccessDenied, "Access Denied")); } } - let policy_allowed = PolicySys::is_allowed(&BucketPolicyArgs { + let policy_allowed = PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: bucket.as_str(), action, is_owner: false, @@ -729,7 +735,8 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R conditions: &conditions, object: object.as_str(), }) - .await; + .await + .map_err(ApiError::from)?; // A bucket policy granting s3:ListBucket also covers listing versions. This // fallback has to feed the same post-authorization gates as the direct grant @@ -737,7 +744,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R // ListObjectVersions after RestrictPublicBuckets is turned on. let policy_allowed = policy_allowed || (action == Action::S3Action(S3Action::ListBucketVersionsAction) - && PolicySys::is_allowed(&BucketPolicyArgs { + && PolicySys::try_is_allowed(&BucketPolicyArgs { bucket: bucket.as_str(), action: Action::S3Action(S3Action::ListBucketAction), is_owner: false, @@ -746,7 +753,8 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R conditions: &conditions, object: "", }) - .await); + .await + .map_err(ApiError::from)?); if policy_allowed { deny_anonymous_table_data_plane_if_needed(action, bucket.as_str(), object.as_str()).await?; @@ -2763,7 +2771,7 @@ mod tests { } #[tokio::test] - async fn abort_multipart_upload_rejects_unauthorized_request() { + async fn abort_multipart_upload_fails_closed_when_policy_metadata_is_unavailable() { let fs = FS::new(); let mut req = build_request( AbortMultipartUploadInput::builder() @@ -2779,12 +2787,12 @@ mod tests { let err = fs .abort_multipart_upload(&mut req) .await - .expect_err("missing credentials should reject access"); - assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + .expect_err("unavailable policy metadata must fail closed"); + assert_eq!(err.code(), &S3ErrorCode::InternalError); } #[tokio::test] - async fn complete_multipart_upload_rejects_unauthorized_request() { + async fn complete_multipart_upload_fails_closed_when_policy_metadata_is_unavailable() { let fs = FS::new(); let mut req = build_request( CompleteMultipartUploadInput::builder() @@ -2801,12 +2809,12 @@ mod tests { let err = fs .complete_multipart_upload(&mut req) .await - .expect_err("missing credentials should reject access"); - assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + .expect_err("unavailable policy metadata must fail closed"); + assert_eq!(err.code(), &S3ErrorCode::InternalError); } #[tokio::test] - async fn upload_part_copy_rejects_unauthorized_request() { + async fn upload_part_copy_fails_closed_when_policy_metadata_is_unavailable() { let fs = FS::new(); let mut req = build_request( UploadPartCopyInput::builder() @@ -2828,7 +2836,7 @@ mod tests { let err = fs .upload_part_copy(&mut req) .await - .expect_err("missing credentials should reject access"); - assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + .expect_err("unavailable policy metadata must fail closed"); + assert_eq!(err.code(), &S3ErrorCode::InternalError); } }