From 975003d60abd46e7d5aa3c0cf7fbb56971879fde Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Mon, 3 Aug 2026 16:11:52 +0800 Subject: [PATCH] fix(s3): stop authorizing DeleteBucketWebsite with a read action (#5665) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete_bucket_website` authorized through `s3:GetBucketPolicy` while `put_bucket_website` used `s3:PutBucketPolicy`. The handler is a real mutation — `rustfs/src/storage/ecfs.rs` calls `delete_bucket_metadata_config(bucket, BUCKET_WEBSITE_CONFIG)`, permanently removing the persisted website configuration. So a principal holding only {"Effect":"Allow","Action":["s3:GetBucketPolicy"], "Resource":"arn:aws:s3:::victim"} — an ordinary read-only "may read my bucket policy" grant — could send `DELETE /victim?website` and destroy the configuration. On a bucket whose policy grants that to `Principal: "*"`, it is reachable anonymously. AWS treats this as its own permission: "This DELETE action requires the S3:DeleteBucketWebsite permission." RustFS has no dedicated `s3:PutBucketWebsite` / `s3:DeleteBucketWebsite` action, so this keeps the existing bucket-config convention (`s3:PutBucketPolicy`, the same one `put_bucket_request_payment` and `put_bucket_accelerate_configuration` use) rather than adding actions, which would silently invalidate deployed policies that already grant website writes. Rather than correcting one constant, both handlers now route through a single `bucket_website_config_authorize_action()`, so the read/write pair cannot drift apart again. Swept the rest of the surface while here: `delete_bucket_website` was the only mutation handler authorizing through a Get*/List* action. `delete_bucket_ownership_controls`, `put_bucket_ownership_controls` and `put_bucket_metrics_configuration` return `Ok(())` with no authorization, but none of them is implemented outside the access hook, so there is no operation to authorize — left alone. Adding a dedicated `s3:DeleteBucketWebsite` for full AWS parity is a separate change with a policy-compatibility impact; noted, not done here. Verification: cargo fmt --all --check, git diff --check, cargo check -p rustfs --all-targets, cargo clippy -p rustfs --all-targets (clean), and the new regression test. Mutation-checked: restoring `GetBucketPolicyAction` turns `bucket_website_config_never_authorizes_through_a_read_action` red. --- rustfs/src/storage/access.rs | 37 ++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index 570846091..7b30012ab 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -867,6 +867,15 @@ fn put_bucket_policy_authorize_action() -> Action { Action::S3Action(S3Action::PutBucketPolicyAction) } +/// Both website-config handlers authorize through this one function so the +/// write side and the delete side cannot drift apart. RustFS has no dedicated +/// `s3:PutBucketWebsite` / `s3:DeleteBucketWebsite` action, so the bucket-config +/// mutation convention applies (same as `put_bucket_request_payment` and +/// `put_bucket_accelerate_configuration`). +fn bucket_website_config_authorize_action() -> Action { + Action::S3Action(S3Action::PutBucketPolicyAction) +} + fn post_object_authorize_action() -> Action { Action::S3Action(S3Action::PutObjectAction) } @@ -1336,7 +1345,7 @@ impl S3Access for FS { let req_info = ext_req_info_mut(&mut req.extensions)?; req_info.bucket = Some(req.input.bucket.clone()); - authorize_request(req, Action::S3Action(S3Action::GetBucketPolicyAction)).await + authorize_request(req, bucket_website_config_authorize_action()).await } /// Checks whether the DeleteObject request has accesses to the resources. @@ -2035,7 +2044,7 @@ impl S3Access for FS { let req_info = ext_req_info_mut(&mut req.extensions)?; req_info.bucket = Some(req.input.bucket.clone()); - authorize_request(req, Action::S3Action(S3Action::PutBucketPolicyAction)).await + authorize_request(req, bucket_website_config_authorize_action()).await } /// Checks whether the PutObject request has accesses to the resources. @@ -2235,12 +2244,13 @@ mod tests { use super::{ AMZ_WRITE_OFFSET_BYTES_HEADER, BucketPolicyArgs, BucketPolicyExistingObjectTagHint, BucketPolicyRawLoadErrorKind, FS, ObjectTagConditions, PostObjectRequestMarker, ReqInfo, S3Access, StorageError, - bucket_policy_needs_existing_object_tag_from_hint, classify_bucket_policy_raw_load_error, - complete_multipart_upload_authorize_action, get_bucket_policy_authorize_action, has_write_offset_bytes_header, - legal_hold_write_requested, list_parts_authorize_action, load_bucket_policy_existing_object_tag_hint, - merge_list_bucket_query_conditions, merge_request_object_tag_conditions, owner_can_bypass_policy_deny, - post_object_authorize_action, put_bucket_policy_authorize_action, request_context_from_req, retention_write_requested, - secondary_tag_hint_action, table_data_plane_admin_action, validate_post_object_success_controls, versioned_read_action, + bucket_policy_needs_existing_object_tag_from_hint, bucket_website_config_authorize_action, + classify_bucket_policy_raw_load_error, complete_multipart_upload_authorize_action, get_bucket_policy_authorize_action, + has_write_offset_bytes_header, legal_hold_write_requested, list_parts_authorize_action, + load_bucket_policy_existing_object_tag_hint, merge_list_bucket_query_conditions, merge_request_object_tag_conditions, + owner_can_bypass_policy_deny, post_object_authorize_action, put_bucket_policy_authorize_action, request_context_from_req, + retention_write_requested, secondary_tag_hint_action, table_data_plane_admin_action, + validate_post_object_success_controls, versioned_read_action, }; use crate::error::ApiError; use http::{Extensions, HeaderMap, HeaderValue, Method, Uri}; @@ -2301,6 +2311,17 @@ mod tests { assert_eq!(put_bucket_policy_authorize_action(), Action::S3Action(S3Action::PutBucketPolicyAction)); } + #[test] + fn bucket_website_config_never_authorizes_through_a_read_action() { + let action = bucket_website_config_authorize_action(); + + // The regression: DeleteBucketWebsite permanently removes the persisted + // website configuration but authorized through s3:GetBucketPolicy, so a + // read-only "may read my bucket policy" grant was enough to destroy it. + assert_ne!(action, Action::S3Action(S3Action::GetBucketPolicyAction)); + assert_eq!(action, Action::S3Action(S3Action::PutBucketPolicyAction)); + } + #[test] fn post_object_uses_put_object_action() { assert_eq!(post_object_authorize_action(), Action::S3Action(S3Action::PutObjectAction));