From 798e620088427524a962c913877e6648cb67d371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sun, 1 Mar 2026 19:00:04 +0800 Subject: [PATCH] fix(s3): add x-amz-grant-* headers to policy condition values (#2031) --- rustfs/src/auth.rs | 38 ++++++++++++++++++++++++ rustfs/src/storage/access.rs | 28 ++++++++++++----- scripts/s3-tests/implemented_tests.txt | 5 ++-- scripts/s3-tests/unimplemented_tests.txt | 1 - 4 files changed, 62 insertions(+), 10 deletions(-) diff --git a/rustfs/src/auth.rs b/rustfs/src/auth.rs index 7c1776562..18c1d25aa 100644 --- a/rustfs/src/auth.rs +++ b/rustfs/src/auth.rs @@ -508,6 +508,25 @@ pub fn get_condition_values( clone_header.remove(*obj_lock); } + // S3 policy condition keys use "x-amz-grant-*" (policy key s3:x-amz-grant-* -> name() returns x-amz-grant-*) + for grant_header in &[ + "x-amz-grant-full-control", + "x-amz-grant-read", + "x-amz-grant-write", + "x-amz-grant-read-acp", + "x-amz-grant-write-acp", + ] { + let values = clone_header + .get_all(*grant_header) + .iter() + .map(|v| v.to_str().unwrap_or("").to_string()) + .collect::>(); + if !values.is_empty() { + args.insert((*grant_header).to_string(), values); + } + clone_header.remove(*grant_header); + } + for (key, _values) in clone_header.iter() { if key.as_str().eq_ignore_ascii_case("x-amz-tagging") { continue; @@ -1038,6 +1057,25 @@ mod tests { ); } + #[test] + fn test_get_condition_values_with_grant_headers() { + let cred = create_test_credentials(); + let mut headers = HeaderMap::new(); + headers.insert("x-amz-grant-full-control", HeaderValue::from_static("id=owner-123")); + headers.insert( + "x-amz-grant-read", + HeaderValue::from_static("uri=http://acs.amazonaws.com/groups/global/AllUsers"), + ); + + let conditions = get_condition_values(&headers, &cred, None, None, None); + + assert_eq!(conditions.get("x-amz-grant-full-control"), Some(&vec!["id=owner-123".to_string()])); + assert_eq!( + conditions.get("x-amz-grant-read"), + Some(&vec!["uri=http://acs.amazonaws.com/groups/global/AllUsers".to_string()]) + ); + } + #[test] fn test_get_condition_values_with_signature_age() { let cred = create_test_credentials(); diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index 92889513e..4e7a5501c 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -166,14 +166,20 @@ async fn load_object_acl(bucket: &str, object: &str, version_id: Option<&str>, h } async fn check_acl_access(req: &S3Request, req_info: &ReqInfo, action: &Action, policy_allowed: bool) -> S3Result { - if req_info.is_owner || policy_allowed { - return Ok(true); - } - let Some((target, permission)) = acl_permission_for_action(action) else { return Ok(true); }; + // For object-level operations, do not bypass on account-level is_owner. + // The bucket/account owner must have explicit ACL grant to access objects owned by others. + let bypass_owner = match target { + AclTarget::Bucket => req_info.is_owner, + AclTarget::Object => false, + }; + if bypass_owner || policy_allowed { + return Ok(true); + } + let bucket = req_info.bucket.as_deref().unwrap_or(""); if bucket.is_empty() { return Ok(true); @@ -364,7 +370,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R return Err(s3_error!(AccessDenied, "Access Denied")); } - if PolicySys::is_allowed(&BucketPolicyArgs { + let policy_allowed_fallback = PolicySys::is_allowed(&BucketPolicyArgs { bucket: req_info.bucket.as_deref().unwrap_or(""), action, is_owner: req_info.is_owner, @@ -373,8 +379,16 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R conditions: &conditions, object: req_info.object.as_deref().unwrap_or(""), }) - .await - { + .await; + + if policy_allowed_fallback { + // For object-level operations, bucket owner (is_owner) must still pass ACL check. + // Policy may have allowed due to is_owner, but object owner is authoritative. + if let Some((AclTarget::Object, _)) = acl_permission_for_action(&action) + && !check_acl_access(req, req_info, &action, false).await? + { + return Err(s3_error!(AccessDenied, "Access Denied")); + } return Ok(()); } diff --git a/scripts/s3-tests/implemented_tests.txt b/scripts/s3-tests/implemented_tests.txt index 1e521eda9..add370a72 100644 --- a/scripts/s3-tests/implemented_tests.txt +++ b/scripts/s3-tests/implemented_tests.txt @@ -21,7 +21,7 @@ # - Object ownership: Bucket ownership controls # # - SSE-KMS: KMS-related edge cases -# - Bucket Policy: Multipart upload authorization, SSE condition keys +# - Bucket Policy: Multipart upload authorization, SSE condition keys, grant header conditions # - Versioning: Concurrent multi-object delete # - ACL: Bucket and Object ACL # - Object Copy: Same/cross bucket, metadata, versioning @@ -36,7 +36,7 @@ # - DeleteObject: Proper NoSuchBucket for deleted buckets # - Multipart Copy: InvalidRange when CopySourceRange exceeds source size # -# Total: 394 tests +# Total: 395 tests test_basic_key_count test_bucket_create_naming_bad_short_one @@ -227,6 +227,7 @@ test_bucket_policy_another_bucket test_bucket_policy_allow_notprincipal test_bucket_policy_multipart test_bucket_policy_put_obj_acl +test_bucket_policy_put_obj_grant test_bucket_policy_put_obj_kms_s3 test_bucket_policy_put_obj_s3_kms test_object_presigned_put_object_with_acl diff --git a/scripts/s3-tests/unimplemented_tests.txt b/scripts/s3-tests/unimplemented_tests.txt index 83588a905..680d2cb52 100644 --- a/scripts/s3-tests/unimplemented_tests.txt +++ b/scripts/s3-tests/unimplemented_tests.txt @@ -28,5 +28,4 @@ test_lifecycle_transition_encrypted # Tests with known issues (need further investigation) test_bucket_policy_different_tenant -test_bucket_policy_put_obj_grant test_bucket_policy_tenanted_bucket