fix(auth): log structured denial reasons for generic AccessDenied responses (#5761)

This commit is contained in:
Zhengchao An
2026-08-06 10:45:54 +08:00
committed by GitHub
parent dbf51117a1
commit efd5481b35
6 changed files with 123 additions and 11 deletions
@@ -209,6 +209,7 @@ impl From<ObjectZipDownloadReqInfoSnapshot> 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,
}
}
+8
View File
@@ -800,6 +800,8 @@ async fn is_list_objects_metadata_action_allowed<T>(
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::<ReqInfo>() {
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);
+12
View File
@@ -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()),
+1
View File
@@ -171,6 +171,7 @@ impl ProtocolStorageClient {
replication_request_authorized: false,
region: None,
request_context: Some(RequestContext::fallback()),
suppress_denial_log: false,
});
let req = S3Request {
+98 -9
View File
@@ -61,6 +61,11 @@ pub(crate) struct ReqInfo {
#[allow(dead_code)]
pub region: Option<s3s::region::Region>,
pub request_context: Option<RequestContext>,
/// 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<T>(req: &S3Request<T>) -> 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("<anonymous>"),
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("<anonymous>"),
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<T>(req: &mut S3Request<T>, action: Action) -> S3Result<()> {
let remote_addr = req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0));
@@ -675,6 +735,15 @@ pub async fn authorize_request<T>(req: &mut S3Request<T>, 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::<std::sync::Arc<ServerContextSlot>>() {
@@ -804,7 +873,7 @@ pub async fn authorize_request<T>(req: &mut S3Request<T>, 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<T>(req: &mut S3Request<T>, 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<T>(req: &mut S3Request<T>, 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<T>(req: &mut S3Request<T>, 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<T>(req: &mut S3Request<T>, 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<T>(req: &mut S3Request<T>, 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<T>(req: &mut S3Request<T>, 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!(
+2 -2
View File
@@ -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