fix(policy): preserve IAM policy readback shape (#3431)

This commit is contained in:
GatewayJ
2026-06-14 16:36:28 +08:00
committed by GitHub
parent fc17e75fb2
commit 3928117c8f
4 changed files with 173 additions and 164 deletions
+26 -27
View File
@@ -17,7 +17,7 @@ use serde::{
Deserialize, Deserializer, Serialize,
de::{self, Error as DeError, Visitor},
};
use std::{collections::HashSet, fmt, ops::Deref};
use std::{fmt, ops::Deref};
use strum::{EnumString, IntoStaticStr};
use super::{Error as IamError, Validator, utils::wildcard};
@@ -25,7 +25,7 @@ use super::{Error as IamError, Validator, utils::wildcard};
/// A set of policy actions that always serializes as an array of strings,
/// conforming to the S3 policy specification for consistency and compatibility.
#[derive(Clone, Default, Debug)]
pub struct ActionSet(pub HashSet<Action>);
pub struct ActionSet(pub Vec<Action>);
impl Serialize for ActionSet {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
@@ -36,12 +36,10 @@ 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 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)?;
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)?;
}
seq.end()
}
@@ -53,6 +51,12 @@ impl ActionSet {
self.0.is_empty()
}
fn push_unique(&mut self, action: Action) {
if !self.0.contains(&action) {
self.0.push(action);
}
}
pub fn is_match(&self, action: &Action) -> bool {
for act in self.0.iter() {
if act.is_match(action) {
@@ -71,7 +75,7 @@ impl ActionSet {
}
impl Deref for ActionSet {
type Target = HashSet<Action>;
type Target = [Action];
fn deref(&self) -> &Self::Target {
&self.0
@@ -87,7 +91,7 @@ impl Validator for ActionSet {
impl PartialEq for ActionSet {
fn eq(&self, other: &Self) -> bool {
self.len() == other.len() && self.0.iter().all(|x| other.0.contains(x))
self.0.iter().all(|x| other.0.contains(x)) && other.0.iter().all(|x| self.0.contains(x))
}
}
@@ -110,9 +114,9 @@ impl<'de> Deserialize<'de> for ActionSet {
E: de::Error,
{
let action = Action::try_from(value).map_err(|e| E::custom(format!("invalid action: {}", e)))?;
let mut set = HashSet::new();
set.insert(action);
Ok(ActionSet(set))
let mut actions = ActionSet::default();
actions.push_unique(action);
Ok(actions)
}
fn visit_seq<A>(self, mut seq: A) -> std::result::Result<Self::Value, A::Error>
@@ -120,18 +124,18 @@ impl<'de> Deserialize<'de> for ActionSet {
A: de::SeqAccess<'de>,
A::Error: DeError,
{
let mut set = HashSet::with_capacity(seq.size_hint().unwrap_or(0));
let mut actions = ActionSet(Vec::with_capacity(seq.size_hint().unwrap_or(0)));
while let Some(value) = seq.next_element::<String>()? {
match Action::try_from(value.as_str()) {
Ok(action) => {
set.insert(action);
actions.push_unique(action);
}
Err(e) => {
return Err(A::Error::custom(format!("invalid action: {}", e)));
}
}
}
Ok(ActionSet(set))
Ok(actions)
}
}
@@ -725,7 +729,6 @@ pub enum KmsAction {
#[cfg(test)]
mod tests {
use super::*;
use std::collections::HashSet;
#[test]
fn test_action_wildcard_parsing() {
@@ -774,9 +777,7 @@ mod tests {
#[test]
fn test_actionset_serialize_single_element() {
// Single element should serialize as array for S3 specification compliance
let mut set = HashSet::new();
set.insert(Action::S3Action(S3Action::GetObjectAction));
let actionset = ActionSet(set);
let actionset = ActionSet(vec![Action::S3Action(S3Action::GetObjectAction)]);
let json = serde_json::to_string(&actionset).expect("Should serialize");
let parsed: serde_json::Value = serde_json::from_str(&json).expect("Should parse");
@@ -789,10 +790,10 @@ mod tests {
#[test]
fn test_actionset_serialize_multiple_elements() {
// Multiple elements should serialize as array
let mut set = HashSet::new();
set.insert(Action::S3Action(S3Action::GetObjectAction));
set.insert(Action::S3Action(S3Action::PutObjectAction));
let actionset = ActionSet(set);
let actionset = ActionSet(vec![
Action::S3Action(S3Action::GetObjectAction),
Action::S3Action(S3Action::PutObjectAction),
]);
let json = serde_json::to_string(&actionset).expect("Should serialize");
let parsed: serde_json::Value = serde_json::from_str(&json).expect("Should parse");
@@ -804,9 +805,7 @@ mod tests {
#[test]
fn test_actionset_wildcard_serialization() {
// Wildcard action should serialize as array for S3 specification compliance
let mut set = HashSet::new();
set.insert(Action::try_from("*").expect("Should parse wildcard"));
let actionset = ActionSet(set);
let actionset = ActionSet(vec![Action::try_from("*").expect("Should parse wildcard")]);
let json = serde_json::to_string(&actionset).expect("Should serialize");
let parsed: serde_json::Value = serde_json::from_str(&json).expect("Should parse");
+116 -107
View File
@@ -355,7 +355,7 @@ pub async fn bucket_policy_needs_existing_object_tag_for_args(policy: &BucketPol
}
pub mod default {
use std::{collections::HashSet, sync::LazyLock};
use std::sync::LazyLock;
use crate::policy::{
ActionSet, DEFAULT_VERSION, Effect, Functions, ResourceSet, Statement,
@@ -377,28 +377,16 @@ pub mod default {
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::S3Action(S3Action::AllActions));
hash_set
}),
actions: ActionSet(vec![Action::S3Action(S3Action::AllActions)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet({
let mut hash_set = HashSet::new();
hash_set.insert(Resource::S3("*".into()));
hash_set
}),
resources: ResourceSet(vec![Resource::S3("*".into())]),
conditions: Functions::default(),
..Default::default()
},
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
hash_set
}),
actions: ActionSet(vec![Action::StsAction(StsAction::AssumeRoleAction)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet(Default::default()),
conditions: Functions::default(),
@@ -416,30 +404,20 @@ pub mod default {
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::S3Action(S3Action::GetBucketLocationAction));
hash_set.insert(Action::S3Action(S3Action::GetObjectAction));
hash_set.insert(Action::S3Action(S3Action::GetBucketQuotaAction));
hash_set
}),
actions: ActionSet(vec![
Action::S3Action(S3Action::GetBucketLocationAction),
Action::S3Action(S3Action::GetObjectAction),
Action::S3Action(S3Action::GetBucketQuotaAction),
]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet({
let mut hash_set = HashSet::new();
hash_set.insert(Resource::S3("*".into()));
hash_set
}),
resources: ResourceSet(vec![Resource::S3("*".into())]),
conditions: Functions::default(),
..Default::default()
},
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
hash_set
}),
actions: ActionSet(vec![Action::StsAction(StsAction::AssumeRoleAction)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet(Default::default()),
conditions: Functions::default(),
@@ -457,28 +435,16 @@ pub mod default {
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::S3Action(S3Action::PutObjectAction));
hash_set
}),
actions: ActionSet(vec![Action::S3Action(S3Action::PutObjectAction)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet({
let mut hash_set = HashSet::new();
hash_set.insert(Resource::S3("*".into()));
hash_set
}),
resources: ResourceSet(vec![Resource::S3("*".into())]),
conditions: Functions::default(),
..Default::default()
},
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
hash_set
}),
actions: ActionSet(vec![Action::StsAction(StsAction::AssumeRoleAction)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet(Default::default()),
conditions: Functions::default(),
@@ -496,35 +462,25 @@ pub mod default {
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::AdminAction(AdminAction::ProfilingAdminAction));
hash_set.insert(Action::AdminAction(AdminAction::TraceAdminAction));
hash_set.insert(Action::AdminAction(AdminAction::ConsoleLogAdminAction));
hash_set.insert(Action::AdminAction(AdminAction::ServerInfoAdminAction));
hash_set.insert(Action::AdminAction(AdminAction::TopLocksAdminAction));
hash_set.insert(Action::AdminAction(AdminAction::HealthInfoAdminAction));
hash_set.insert(Action::AdminAction(AdminAction::PrometheusAdminAction));
hash_set.insert(Action::AdminAction(AdminAction::BandwidthMonitorAction));
hash_set
}),
actions: ActionSet(vec![
Action::AdminAction(AdminAction::ProfilingAdminAction),
Action::AdminAction(AdminAction::TraceAdminAction),
Action::AdminAction(AdminAction::ConsoleLogAdminAction),
Action::AdminAction(AdminAction::ServerInfoAdminAction),
Action::AdminAction(AdminAction::TopLocksAdminAction),
Action::AdminAction(AdminAction::HealthInfoAdminAction),
Action::AdminAction(AdminAction::PrometheusAdminAction),
Action::AdminAction(AdminAction::BandwidthMonitorAction),
]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet({
let mut hash_set = HashSet::new();
hash_set.insert(Resource::S3("*".into()));
hash_set
}),
resources: ResourceSet(vec![Resource::S3("*".into())]),
conditions: Functions::default(),
..Default::default()
},
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
hash_set
}),
actions: ActionSet(vec![Action::StsAction(StsAction::AssumeRoleAction)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet(Default::default()),
conditions: Functions::default(),
@@ -542,56 +498,36 @@ pub mod default {
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::AdminAction(AdminAction::AllAdminActions));
hash_set
}),
actions: ActionSet(vec![Action::AdminAction(AdminAction::AllAdminActions)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet(HashSet::new()),
resources: ResourceSet(Vec::new()),
conditions: Functions::default(),
..Default::default()
},
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::KmsAction(KmsAction::AllActions));
hash_set
}),
actions: ActionSet(vec![Action::KmsAction(KmsAction::AllActions)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet(HashSet::new()),
resources: ResourceSet(Vec::new()),
conditions: Functions::default(),
..Default::default()
},
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::S3Action(S3Action::AllActions));
hash_set
}),
actions: ActionSet(vec![Action::S3Action(S3Action::AllActions)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet({
let mut hash_set = HashSet::new();
hash_set.insert(Resource::S3("*".into()));
hash_set
}),
resources: ResourceSet(vec![Resource::S3("*".into())]),
conditions: Functions::default(),
..Default::default()
},
Statement {
sid: "".into(),
effect: Effect::Allow,
actions: ActionSet({
let mut hash_set = HashSet::new();
hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction));
hash_set
}),
actions: ActionSet(vec![Action::StsAction(StsAction::AssumeRoleAction)]),
not_actions: ActionSet(Default::default()),
resources: ResourceSet(HashSet::new()),
resources: ResourceSet(Vec::new()),
conditions: Functions::default(),
..Default::default()
},
@@ -1947,7 +1883,7 @@ mod test {
}
#[test]
fn test_policy_action_and_resource_serialization_is_stable() {
fn test_policy_action_and_resource_serialization_preserves_input_order() {
let policy = Policy::parse_config(
br#"{
"Version":"2012-10-17",
@@ -1975,8 +1911,8 @@ mod test {
.collect();
assert_eq!(
actions,
vec!["s3:DeleteObject", "s3:GetObject", "s3:ListBucket", "s3:PutObject"],
"Action serialization should use a stable order"
vec!["s3:PutObject", "s3:DeleteObject", "s3:ListBucket", "s3:GetObject"],
"Action serialization should preserve input order"
);
let resources: Vec<_> = statement["Resource"]
@@ -1987,11 +1923,84 @@ mod test {
.collect();
assert_eq!(
resources,
vec!["arn:aws:s3:::example-bucket", "arn:aws:s3:::example-bucket/*"],
"Resource serialization should use a stable order"
vec!["arn:aws:s3:::example-bucket/*", "arn:aws:s3:::example-bucket"],
"Resource serialization should preserve input order"
);
}
#[test]
fn test_iam_policy_serialize_preserves_non_empty_optional_fields() {
let policy = Policy::parse_config(
br#"{
"Version":"2012-10-17",
"Statement":[{
"Sid":"keep-me",
"Effect":"Allow",
"NotAction":["s3:DeleteObject","s3:PutObject"],
"NotResource":["arn:aws:s3:::example-bucket/private/*","arn:aws:s3:::example-bucket/tmp/*"],
"Condition":{"StringEquals":{"s3:prefix":"home/"}}
}]
}"#,
)
.expect("policy with non-empty optional fields should parse");
let value = serde_json::to_value(&policy).expect("policy should serialize");
let statement = value["Statement"][0]
.as_object()
.expect("statement should serialize as an object");
assert_eq!(statement.get("Sid").and_then(|sid| sid.as_str()), Some("keep-me"));
assert!(statement.contains_key("Condition"), "non-empty Condition should be preserved");
assert!(!statement.contains_key("Action"), "empty Action should be omitted for NotAction policy");
assert!(
!statement.contains_key("Resource"),
"empty Resource should be omitted for NotResource policy"
);
let not_actions: Vec<_> = statement["NotAction"]
.as_array()
.expect("NotAction should serialize as an array")
.iter()
.map(|action| action.as_str().expect("NotAction entries should be strings"))
.collect();
assert_eq!(not_actions, vec!["s3:DeleteObject", "s3:PutObject"]);
let not_resources: Vec<_> = statement["NotResource"]
.as_array()
.expect("NotResource should serialize as an array")
.iter()
.map(|resource| resource.as_str().expect("NotResource entries should be strings"))
.collect();
assert_eq!(
not_resources,
vec!["arn:aws:s3:::example-bucket/private/*", "arn:aws:s3:::example-bucket/tmp/*"]
);
}
#[test]
fn test_iam_policy_serialize_keeps_empty_resource_omitted_for_action_families() {
let policy = Policy::parse_config(
br#"{
"Version":"2012-10-17",
"Statement":[
{"Effect":"Allow","Action":["sts:AssumeRole"]},
{"Effect":"Allow","Action":["admin:*"]},
{"Effect":"Allow","Action":["kms:*"]}
]
}"#,
)
.expect("STS/Admin/KMS statements without resources should parse");
let value = serde_json::to_value(&policy).expect("policy should serialize");
let statements = value["Statement"].as_array().expect("Statement should serialize as an array");
assert_eq!(statements.len(), 3);
for statement in statements {
let statement = statement.as_object().expect("statement should serialize as an object");
assert!(!statement.contains_key("Resource"), "empty Resource should be omitted");
assert!(!statement.contains_key("NotResource"), "empty NotResource should be omitted");
}
}
#[test]
fn test_bucket_policy_serialize_omits_empty_fields() {
use crate::policy::action::{Action, ActionSet, S3Action};
@@ -2021,11 +2030,11 @@ mod test {
policy.statements[0]
.actions
.0
.insert(Action::S3Action(S3Action::ListBucketAction));
.push(Action::S3Action(S3Action::ListBucketAction));
policy.statements[0]
.resources
.0
.insert(Resource::try_from("arn:aws:s3:::test/*").unwrap());
.push(Resource::try_from("arn:aws:s3:::test/*").expect("test resource should parse"));
let json = serde_json::to_string(&policy).expect("Should serialize");
let parsed: serde_json::Value = serde_json::from_str(&json).expect("Should parse");
@@ -2134,11 +2143,11 @@ mod test {
policy.statements[0]
.actions
.0
.insert(Action::S3Action(S3Action::ListBucketAction));
.push(Action::S3Action(S3Action::ListBucketAction));
policy.statements[0]
.resources
.0
.insert(Resource::try_from("arn:aws:s3:::test/*").unwrap());
.push(Resource::try_from("arn:aws:s3:::test/*").expect("test resource should parse"));
let json = serde_json::to_string(&policy).expect("Should serialize");
let parsed: serde_json::Value = serde_json::from_str(&json).expect("Should parse");
+21 -26
View File
@@ -17,12 +17,7 @@ use serde::{
Deserialize, Deserializer, Serialize,
de::{self, Error as DeError, Visitor},
};
use std::{
collections::{HashMap, HashSet},
fmt,
hash::Hash,
ops::Deref,
};
use std::{collections::HashMap, fmt, ops::Deref};
use super::{
Error as IamError, Validator,
@@ -32,7 +27,7 @@ use super::{
};
#[derive(Clone, Default, Debug)]
pub struct ResourceSet(pub HashSet<Resource>);
pub struct ResourceSet(pub Vec<Resource>);
impl Serialize for ResourceSet {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
@@ -41,19 +36,13 @@ impl Serialize for ResourceSet {
{
use serde::ser::SerializeSeq;
let mut resources: Vec<String> = self
.0
.iter()
.map(|resource| match resource {
let mut seq = serializer.serialize_seq(Some(self.0.len()))?;
for resource in &self.0 {
let resource_str = 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.serialize_element(&resource_str)?;
}
seq.end()
}
@@ -65,6 +54,12 @@ impl ResourceSet {
self.0.is_empty()
}
fn push_unique(&mut self, resource: Resource) {
if !self.0.contains(&resource) {
self.0.push(resource);
}
}
pub async fn is_match(&self, resource: &str, conditions: &HashMap<String, Vec<String>>) -> bool {
self.is_match_with_resolver(resource, conditions, None).await
}
@@ -96,7 +91,7 @@ impl ResourceSet {
}
impl Deref for ResourceSet {
type Target = HashSet<Resource>;
type Target = [Resource];
fn deref(&self) -> &Self::Target {
&self.0
@@ -116,7 +111,7 @@ impl Validator for ResourceSet {
impl PartialEq for ResourceSet {
fn eq(&self, other: &Self) -> bool {
self.len() == other.len() && self.0.iter().all(|x| other.0.contains(x))
self.0.iter().all(|x| other.0.contains(x)) && other.0.iter().all(|x| self.0.contains(x))
}
}
@@ -139,9 +134,9 @@ impl<'de> Deserialize<'de> for ResourceSet {
E: de::Error,
{
let resource = Resource::try_from(value).map_err(|e| E::custom(format!("invalid resource: {}", e)))?;
let mut set = HashSet::new();
set.insert(resource);
Ok(ResourceSet(set))
let mut resources = ResourceSet::default();
resources.push_unique(resource);
Ok(resources)
}
fn visit_seq<A>(self, mut seq: A) -> std::result::Result<Self::Value, A::Error>
@@ -149,18 +144,18 @@ impl<'de> Deserialize<'de> for ResourceSet {
A: de::SeqAccess<'de>,
A::Error: DeError,
{
let mut set = HashSet::with_capacity(seq.size_hint().unwrap_or(0));
let mut resources = ResourceSet(Vec::with_capacity(seq.size_hint().unwrap_or(0)));
while let Some(value) = seq.next_element::<String>()? {
match Resource::try_from(value.as_str()) {
Ok(resource) => {
set.insert(resource);
resources.push_unique(resource);
}
Err(e) => {
return Err(A::Error::custom(format!("invalid resource: {}", e)));
}
}
}
Ok(ResourceSet(set))
Ok(resources)
}
}
+10 -4
View File
@@ -21,7 +21,7 @@ use rustfs_iam::store::Store as IamStore;
use rustfs_madmin::{
InfoAccessKeyResp, InfoServiceAccountResp, LDAPSpecificAccessKeyInfo, OpenIDSpecificAccessKeyInfo, ServiceAccountInfo,
};
use rustfs_policy::policy::Policy;
use rustfs_policy::policy::{DEFAULT_VERSION, Policy};
use s3s::{S3Result, s3_error};
use std::collections::HashMap;
use time::OffsetDateTime;
@@ -236,7 +236,10 @@ pub(crate) async fn build_info_service_account_resp<T: IamStore>(
};
let policy = effective_policy
.map(|policy| {
.map(|mut policy| {
if policy.version.is_empty() {
policy.version = DEFAULT_VERSION.to_owned();
}
serde_json::to_string_pretty(&policy).map_err(|e| {
debug!("marshal policy failed, e: {:?}", e);
s3_error!(InternalError, "marshal policy failed")
@@ -891,7 +894,10 @@ 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 \"Version\": \"\",\n \"Statement\": []\n}".to_string()));
assert_eq!(
resp.info.policy,
Some("{\n \"Version\": \"2012-10-17\",\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"));
@@ -929,7 +935,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 \"Version\": \"\",\n \"Statement\": []\n}")
Some("{\n \"Version\": \"2012-10-17\",\n \"Statement\": []\n}")
);
}
}