fix(authz): fail closed on policy load errors (#5359)

* fix(authz): fail closed on policy load errors

* test(authz): cover policy failure precedence

* test: align policy failure expectations

* test: align upload part copy fail-closed expectation
This commit is contained in:
cxymds
2026-07-28 17:04:35 +08:00
committed by GitHub
parent 2216f00cfd
commit 4fb9b0dc7f
5 changed files with 163 additions and 36 deletions
+18
View File
@@ -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(),
+102 -11
View File
@@ -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<bool> {
Self::is_allowed_with_policy(args, Self::get(args.bucket).await).await
}
async fn is_allowed_with_policy(args: &BucketPolicyArgs<'_>, policy: Result<BucketPolicy>) -> Result<bool> {
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<BucketPolicy> {
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<Vec<String>>,
conditions: &'a HashMap<String, Vec<String>>,
) -> 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");
}
}