From efd5481b35d656587d3943cf8acfb70bd6eaf018 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Thu, 6 Aug 2026 10:45:54 +0800 Subject: [PATCH] fix(auth): log structured denial reasons for generic AccessDenied responses (#5761) --- .../src/admin/handlers/object_zip_download.rs | 2 + rustfs/src/app/bucket_usecase.rs | 8 ++ rustfs/src/app/object_usecase.rs | 12 ++ rustfs/src/protocols/client.rs | 1 + rustfs/src/storage/access.rs | 107 ++++++++++++++++-- scripts/check_s3s_footprint.sh | 4 +- 6 files changed, 123 insertions(+), 11 deletions(-) diff --git a/rustfs/src/admin/handlers/object_zip_download.rs b/rustfs/src/admin/handlers/object_zip_download.rs index b3597790e..43ee7495c 100644 --- a/rustfs/src/admin/handlers/object_zip_download.rs +++ b/rustfs/src/admin/handlers/object_zip_download.rs @@ -209,6 +209,7 @@ impl From for ReqInfo { replication_request_authorized: false, region: current_region(), request_context: None, + suppress_denial_log: false, } } } @@ -1428,6 +1429,7 @@ mod tests { replication_request_authorized: false, region: current_region(), request_context: None, + suppress_denial_log: false, } } diff --git a/rustfs/src/app/bucket_usecase.rs b/rustfs/src/app/bucket_usecase.rs index 544a6ac1f..80d0b364a 100644 --- a/rustfs/src/app/bucket_usecase.rs +++ b/rustfs/src/app/bucket_usecase.rs @@ -800,6 +800,8 @@ async fn is_list_objects_metadata_action_allowed( req_info.bucket = Some(bucket.to_string()); req_info.object = Some(object.to_string()); req_info.version_id = None; + // Denial here is an expected filter outcome, not an error (issue #5740). + req_info.suppress_denial_log = true; auth_req.extensions.insert(req_info); match authorize_request(&mut auth_req, Action::S3Action(action)).await { @@ -1392,6 +1394,12 @@ impl DefaultBucketUsecase { return Err(S3Error::with_message(S3ErrorCode::AccessDenied, "Access Denied")); } + // The ListAllMyBuckets probe and the per-bucket probes cloned from this + // request treat denial as an expected filter outcome (issue #5740). + if let Some(req_info) = req.extensions.get_mut::() { + req_info.suppress_denial_log = true; + } + let bucket_infos = if let Err(e) = authorize_request(&mut req, Action::S3Action(S3Action::ListAllMyBucketsAction)).await { if e.code() != &S3ErrorCode::AccessDenied { return Err(e); diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index a9e9126b3..51d50db2b 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -6940,6 +6940,10 @@ impl DefaultObjectUsecase { } let mut authorized_deletes = Vec::with_capacity(delete.objects.len()); + // Issue #5740: keep the first per-key denial of this bulk request at + // warn and demote the rest to debug, so a denied 1000-key DeleteObjects + // cannot flood the log. + let mut bulk_denial_logged = false; for (idx, obj_id) in delete.objects.iter().enumerate() { let raw_version_id = obj_id.version_id.clone(); let (version_id, version_uuid) = match normalize_delete_objects_version_id(raw_version_id.clone()) { @@ -6964,6 +6968,10 @@ impl DefaultObjectUsecase { let auth_res = authorize_request(&mut req, Action::S3Action(S3Action::DeleteObjectAction)).await; if auth_res.is_err() { + if !bulk_denial_logged { + bulk_denial_logged = true; + req_info_mut(&mut req)?.suppress_denial_log = true; + } delete_results[idx].error = Some(s3s::dto::Error { code: Some("AccessDenied".to_string()), key: Some(obj_id.key.clone()), @@ -6976,6 +6984,10 @@ impl DefaultObjectUsecase { if bypass_governance { let auth_res = authorize_request(&mut req, Action::S3Action(S3Action::BypassGovernanceRetentionAction)).await; if auth_res.is_err() { + if !bulk_denial_logged { + bulk_denial_logged = true; + req_info_mut(&mut req)?.suppress_denial_log = true; + } delete_results[idx].error = Some(s3s::dto::Error { code: Some("AccessDenied".to_string()), key: Some(obj_id.key.clone()), diff --git a/rustfs/src/protocols/client.rs b/rustfs/src/protocols/client.rs index 6961172db..192b6f615 100644 --- a/rustfs/src/protocols/client.rs +++ b/rustfs/src/protocols/client.rs @@ -171,6 +171,7 @@ impl ProtocolStorageClient { replication_request_authorized: false, region: None, request_context: Some(RequestContext::fallback()), + suppress_denial_log: false, }); let req = S3Request { diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index b86c208e2..7b6df3620 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -61,6 +61,11 @@ pub(crate) struct ReqInfo { #[allow(dead_code)] pub region: Option, pub request_context: Option, + /// Set by probe-style callers that treat AccessDenied as an expected filter + /// outcome (ListBuckets per-bucket fallback, ListObjects metadata permission + /// collection) so `authorize_request` logs those routine denials at `debug` + /// instead of `warn` (issue #5740). + pub suppress_denial_log: bool, } pub(crate) fn replication_request_authorized(req: &S3Request) -> bool { @@ -666,6 +671,61 @@ pub(crate) fn owner_can_bypass_policy_deny(is_owner: bool, action: &Action) -> b ) } +/// Context shared by every denial exit of [`authorize_request`] (issue #5740). +/// +/// The wire response intentionally stays the bare "Access Denied" so no policy +/// detail leaks to clients; the structured server-side event emitted here is +/// the only place the denial reason is recorded. Probe-style callers that use +/// denial as an expected filter outcome (per-bucket ListBuckets fallback, +/// per-object metadata permission collection) set +/// [`ReqInfo::suppress_denial_log`] so their routine denials log at `debug` +/// instead of `warn`. +struct DenialContext<'a> { + quiet: bool, + bucket: &'a str, + object: &'a str, + version_id: Option<&'a str>, + account: Option<&'a str>, + is_owner: bool, +} + +impl DenialContext<'_> { + fn log(&self, reason: &'static str, action: Action) { + if self.quiet { + tracing::debug!( + event = "s3_authorization_denied", + reason, + action = ?action, + bucket = %self.bucket, + object = %self.object, + version_id = ?self.version_id, + account = self.account.unwrap_or(""), + is_owner = self.is_owner, + "authorization probe denied" + ); + return; + } + tracing::warn!( + event = "s3_authorization_denied", + reason, + action = ?action, + bucket = %self.bucket, + object = %self.object, + version_id = ?self.version_id, + account = self.account.unwrap_or(""), + is_owner = self.is_owner, + "request denied by authorization layer" + ); + } + + /// Build the generic AccessDenied response after recording the structured + /// denial event. + fn deny(&self, reason: &'static str, action: Action) -> S3Error { + self.log(reason, action); + s3_error!(AccessDenied, "Access Denied") + } +} + /// Authorizes the request based on the action and credentials. pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3Result<()> { let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); @@ -675,6 +735,15 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R let bucket = req_info.bucket.clone().unwrap_or_default(); let object = req_info.object.clone().unwrap_or_default(); let version_id = req_info.version_id.clone(); + let quiet_denial = req_info.suppress_denial_log; + let denial = DenialContext { + quiet: quiet_denial, + bucket: bucket.as_str(), + object: object.as_str(), + version_id: version_id.as_deref(), + account: cred.as_ref().map(|c| c.access_key.as_str()), + is_owner, + }; if let Some(cred) = &cred { let iam_store = match req.extensions.get::>() { @@ -804,7 +873,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R .await .map_err(ApiError::from)? { - return Err(s3_error!(AccessDenied, "Access Denied")); + return Err(denial.deny("bucket_policy_explicit_deny", action)); } if action == Action::S3Action(S3Action::DeleteObjectAction) && version_id.is_some() { @@ -833,7 +902,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R .await .map_err(ApiError::from)? { - return Err(s3_error!(AccessDenied, "Access Denied")); + return Err(denial.deny("delete_object_version_denied", Action::S3Action(S3Action::DeleteObjectVersionAction))); } } @@ -859,6 +928,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R } if action == Action::S3Action(S3Action::ListAllMyBucketsAction) { + denial.log("iam_implicit_deny", action); return Err(ApiError::access_denied().into()); } @@ -994,7 +1064,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R .await .map_err(ApiError::from)? { - return Err(s3_error!(AccessDenied, "Access Denied")); + return Err(denial.deny("bucket_policy_explicit_deny", action)); } if action != Action::S3Action(S3Action::ListAllMyBucketsAction) { @@ -1011,7 +1081,9 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R .await .map_err(ApiError::from)?; if !delete_version_allowed { - return Err(s3_error!(AccessDenied, "Access Denied")); + return Err( + denial.deny("delete_object_version_denied", Action::S3Action(S3Action::DeleteObjectVersionAction)) + ); } } @@ -1051,12 +1123,12 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R match get_public_access_block_config(bucket_name).await { Ok((config, _)) => { if config.restrict_public_buckets.unwrap_or(false) { - return Err(s3_error!(AccessDenied, "Access Denied")); + return Err(denial.deny("restrict_public_buckets", action)); } } Err(StorageError::ConfigNotFound) => {} Err(_) => { - return Err(s3_error!(AccessDenied, "Access Denied")); + return Err(denial.deny("public_access_block_unavailable", action)); } } return Ok(()); @@ -1064,7 +1136,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R } } - Err(s3_error!(AccessDenied, "Access Denied")) + Err(denial.deny("no_policy_allows_action", action)) } /// Check if the request has the x-amz-bypass-governance-retention header set to true @@ -2521,8 +2593,8 @@ impl S3Access for FS { mod tests { use super::{ AMZ_WRITE_OFFSET_BYTES_HEADER, BucketGenerationGuard, BucketPolicyArgs, BucketPolicyExistingObjectTagHint, - BucketPolicyRawLoadErrorKind, FS, ObjectTagConditions, PostObjectRequestMarker, ReqInfo, S3Access, StorageError, - apply_bucket_generation_guard, apply_copy_source_bucket_generation_guard, + BucketPolicyRawLoadErrorKind, DenialContext, FS, ObjectTagConditions, PostObjectRequestMarker, ReqInfo, S3Access, + StorageError, apply_bucket_generation_guard, apply_copy_source_bucket_generation_guard, 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, install_restore_authorization_test_hook, legal_hold_write_requested, @@ -2960,6 +3032,23 @@ mod tests { )); } + /// Issue #5740: the denial helper must keep the client-facing error identical + /// to the historical bare response — the added diagnostics are log-only. + #[test] + fn test_access_denied_helper_keeps_generic_wire_response() { + let denial = DenialContext { + quiet: false, + bucket: "bucket", + object: "object", + version_id: Some("version"), + account: Some("account"), + is_owner: false, + }; + let err = denial.deny("bucket_policy_explicit_deny", Action::S3Action(S3Action::GetObjectAction)); + assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + assert_eq!(err.message(), Some("Access Denied")); + } + #[test] fn test_secondary_tag_hint_action_for_delete_object_version() { assert_eq!( diff --git a/scripts/check_s3s_footprint.sh b/scripts/check_s3s_footprint.sh index a70cdeb11..5310aeeb5 100755 --- a/scripts/check_s3s_footprint.sh +++ b/scripts/check_s3s_footprint.sh @@ -22,9 +22,9 @@ set -euo pipefail cd "$(dirname "$0")/.." -# Baselines verified on 2026-08-05. Lower-only; see header. +# Baselines verified on 2026-08-06. Lower-only; see header. S3S_IMPORT_FILES_BASELINE=236 -S3_ERROR_LINES_BASELINE=1686 +S3_ERROR_LINES_BASELINE=1680 TMP_DIR="$(mktemp -d)" trap 'rm -rf "$TMP_DIR"' EXIT