From 42d9d5247d234bbf5aaf2565c35e21450a8c01fc Mon Sep 17 00:00:00 2001 From: GatewayJ <835269233@qq.com> Date: Sat, 13 Jun 2026 18:47:08 +0800 Subject: [PATCH] fix(policy): normalize IAM policy readback (#3402) --- crates/policy/src/policy/action.rs | 10 ++-- crates/policy/src/policy/policy.rs | 76 ++++++++++++++++++++++++- crates/policy/src/policy/resource.rs | 32 +++++++++-- crates/policy/src/policy/statement.rs | 6 +- rustfs/src/admin/access_key_identity.rs | 7 +-- 5 files changed, 114 insertions(+), 17 deletions(-) diff --git a/crates/policy/src/policy/action.rs b/crates/policy/src/policy/action.rs index ce0dc099f..4eed3ab46 100644 --- a/crates/policy/src/policy/action.rs +++ b/crates/policy/src/policy/action.rs @@ -36,10 +36,12 @@ impl Serialize for ActionSet { // Always serialize as array, even for single action, to match S3 specification // and ensure compatibility with AWS SDK clients that expect array format - let mut seq = serializer.serialize_seq(Some(self.0.len()))?; - for action in &self.0 { - let action_str: &str = action.into(); - seq.serialize_element(action_str)?; + let mut actions: Vec<&str> = self.0.iter().map(Into::into).collect(); + actions.sort_unstable(); + + let mut seq = serializer.serialize_seq(Some(actions.len()))?; + for action in actions { + seq.serialize_element(action)?; } seq.end() } diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index c842c84db..d90fdf420 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -57,7 +57,7 @@ impl Args<'_> { #[derive(Serialize, Deserialize, Clone, Default, Debug)] #[serde(deny_unknown_fields)] pub struct Policy { - #[serde(default, rename = "ID")] + #[serde(default, rename = "ID", skip_serializing_if = "ID::is_empty")] pub id: ID, #[serde(default, rename = "Version")] pub version: String, @@ -1918,6 +1918,80 @@ mod test { ); } + #[test] + fn test_iam_policy_serialize_omits_empty_fields() { + let policy = Policy::parse_config( + br#"{ + "Version":"2012-10-17", + "Statement":[{ + "Effect":"Allow", + "Action":["s3:GetObject","s3:PutObject"], + "Resource":["arn:aws:s3:::example-bucket/*"] + }] +}"#, + ) + .expect("policy without optional fields should parse"); + + let value = serde_json::to_value(&policy).expect("policy should serialize"); + let policy_object = value.as_object().expect("policy should serialize as an object"); + assert!(!policy_object.contains_key("ID"), "empty ID should be omitted"); + + let statements = value["Statement"].as_array().expect("Statement should serialize as an array"); + let statement = statements + .first() + .expect("serialized policy should include the statement") + .as_object() + .expect("statement should serialize as an object"); + assert!(!statement.contains_key("Sid"), "empty Sid should be omitted"); + assert!(!statement.contains_key("Condition"), "empty Condition should be omitted"); + } + + #[test] + fn test_policy_action_and_resource_serialization_is_stable() { + let policy = Policy::parse_config( + br#"{ + "Version":"2012-10-17", + "Statement":[{ + "Effect":"Allow", + "Action":["s3:PutObject","s3:DeleteObject","s3:ListBucket","s3:GetObject"], + "Resource":["arn:aws:s3:::example-bucket/*","arn:aws:s3:::example-bucket"] + }] +}"#, + ) + .expect("policy with multiple actions and resources should parse"); + + let first = serde_json::to_value(&policy).expect("policy should serialize"); + let second = serde_json::to_value(&policy).expect("policy should serialize consistently"); + assert_eq!(first, second, "policy serialization should be deterministic"); + + let statement = first["Statement"][0] + .as_object() + .expect("statement should serialize as an object"); + let actions: Vec<_> = statement["Action"] + .as_array() + .expect("Action should serialize as an array") + .iter() + .map(|action| action.as_str().expect("Action entries should be strings")) + .collect(); + assert_eq!( + actions, + vec!["s3:DeleteObject", "s3:GetObject", "s3:ListBucket", "s3:PutObject"], + "Action serialization should use a stable order" + ); + + let resources: Vec<_> = statement["Resource"] + .as_array() + .expect("Resource should serialize as an array") + .iter() + .map(|resource| resource.as_str().expect("Resource entries should be strings")) + .collect(); + assert_eq!( + resources, + vec!["arn:aws:s3:::example-bucket", "arn:aws:s3:::example-bucket/*"], + "Resource serialization should use a stable order" + ); + } + #[test] fn test_bucket_policy_serialize_omits_empty_fields() { use crate::policy::action::{Action, ActionSet, S3Action}; diff --git a/crates/policy/src/policy/resource.rs b/crates/policy/src/policy/resource.rs index 69fd61ccb..223c62499 100644 --- a/crates/policy/src/policy/resource.rs +++ b/crates/policy/src/policy/resource.rs @@ -31,9 +31,34 @@ use super::{ variables::PolicyVariableResolver, }; -#[derive(Serialize, Clone, Default, Debug)] +#[derive(Clone, Default, Debug)] pub struct ResourceSet(pub HashSet); +impl Serialize for ResourceSet { + fn serialize(&self, serializer: S) -> std::result::Result + where + S: serde::Serializer, + { + use serde::ser::SerializeSeq; + + let mut resources: Vec = self + .0 + .iter() + .map(|resource| match resource { + Resource::S3(value) => format!("{}{}", Resource::S3_PREFIX, value), + Resource::Kms(value) => value.clone(), + }) + .collect(); + resources.sort_unstable(); + + let mut seq = serializer.serialize_seq(Some(resources.len()))?; + for resource in resources { + seq.serialize_element(&resource)?; + } + seq.end() + } +} + impl ResourceSet { /// Returns true if the resource set is empty. pub fn is_empty(&self) -> bool { @@ -208,11 +233,10 @@ impl Resource { impl TryFrom<&str> for Resource { type Error = Error; fn try_from(value: &str) -> std::result::Result { - let resource = if value.starts_with(Self::S3_PREFIX) { - Resource::S3(value.strip_prefix(Self::S3_PREFIX).unwrap().into()) - } else { + let Some(value) = value.strip_prefix(Self::S3_PREFIX) else { return Err(IamError::InvalidResource("unknown".into(), value.into()).into()); }; + let resource = Resource::S3(value.into()); resource.is_valid()?; Ok(resource) diff --git a/crates/policy/src/policy/statement.rs b/crates/policy/src/policy/statement.rs index c3f4220a1..94bd9d32c 100644 --- a/crates/policy/src/policy/statement.rs +++ b/crates/policy/src/policy/statement.rs @@ -24,7 +24,7 @@ use serde::{Deserialize, Serialize}; #[derive(Serialize, Deserialize, Clone, Default, Debug)] #[serde(deny_unknown_fields)] pub struct Statement { - #[serde(rename = "Sid", default)] + #[serde(rename = "Sid", default, skip_serializing_if = "ID::is_empty")] pub sid: ID, #[serde(rename = "Effect")] pub effect: Effect, @@ -36,7 +36,7 @@ pub struct Statement { pub resources: ResourceSet, #[serde(rename = "NotResource", default, skip_serializing_if = "ResourceSet::is_empty")] pub not_resources: ResourceSet, - #[serde(rename = "Condition", default)] + #[serde(rename = "Condition", default, skip_serializing_if = "Functions::is_empty")] pub conditions: Functions, } @@ -151,7 +151,7 @@ impl Statement { } } - let family_count = saw_s3 as u8 + saw_admin as u8 + saw_sts as u8 + saw_kms as u8; + let family_count = u8::from(saw_s3) + u8::from(saw_admin) + u8::from(saw_sts) + u8::from(saw_kms); if family_count != 1 { return Some(ActionFamily::Mixed); diff --git a/rustfs/src/admin/access_key_identity.rs b/rustfs/src/admin/access_key_identity.rs index affcb2eed..bbf6ada78 100644 --- a/rustfs/src/admin/access_key_identity.rs +++ b/rustfs/src/admin/access_key_identity.rs @@ -891,10 +891,7 @@ mod tests { assert_eq!(resp.open_id_specific_info.display_name.as_deref(), Some("RustFS User")); assert_eq!(resp.open_id_specific_info.display_name_claim.as_deref(), Some("name")); assert_eq!(resp.open_id_specific_info.config_name, None); - assert_eq!( - resp.info.policy, - Some("{\n \"ID\": \"\",\n \"Version\": \"\",\n \"Statement\": []\n}".to_string()) - ); + assert_eq!(resp.info.policy, Some("{\n \"Version\": \"\",\n \"Statement\": []\n}".to_string())); let body = serde_json::to_value(&resp).expect("serialize openid sts response"); assert_eq!(body.get("userType").and_then(|v| v.as_str()), Some("STS")); @@ -932,7 +929,7 @@ mod tests { assert_eq!(body.get("description").and_then(|v| v.as_str()), Some("openid derived temporary account")); assert_eq!( body.get("policy").and_then(|v| v.as_str()), - Some("{\n \"ID\": \"\",\n \"Version\": \"\",\n \"Statement\": []\n}") + Some("{\n \"Version\": \"\",\n \"Statement\": []\n}") ); } }