From e0729f5f4d2a00ffb211896b0633a7aa7b11be84 Mon Sep 17 00:00:00 2001 From: GatewayJ <835269233@qq.com> Date: Sat, 16 May 2026 19:19:04 +0800 Subject: [PATCH] fix(policy): align action-family validation and defaults (#2984) * fix(policy): align action-family validation and defaults * test(e2e): add accountinfo service-account roundtrip * test(policy): add mixed action family cases --- .../src/replication_extension_test.rs | 67 +++ crates/policy/src/policy.rs | 3 + crates/policy/src/policy/policy.rs | 429 +++++++++++++----- crates/policy/src/policy/statement.rs | 71 ++- 4 files changed, 458 insertions(+), 112 deletions(-) diff --git a/crates/e2e_test/src/replication_extension_test.rs b/crates/e2e_test/src/replication_extension_test.rs index a362678eb..33fb7e19a 100644 --- a/crates/e2e_test/src/replication_extension_test.rs +++ b/crates/e2e_test/src/replication_extension_test.rs @@ -651,6 +651,22 @@ async fn list_service_accounts( Ok(response.json().await?) } +async fn get_account_info( + env: &RustFSTestEnvironment, + signer_access_key: &str, + signer_secret_key: &str, +) -> Result> { + let url = format!("{}/rustfs/admin/v3/accountinfo", env.url); + let response = signed_request(http::Method::GET, &url, signer_access_key, signer_secret_key, None, None).await?; + if response.status() != StatusCode::OK { + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + return Err(format!("account info failed: {status} {body}").into()); + } + + Ok(response.json().await?) +} + async fn wait_for_service_accounts( env: &RustFSTestEnvironment, signer_access_key: &str, @@ -2379,6 +2395,57 @@ async fn test_site_replication_replicates_group_policy_backed_access_real_dual_n Ok(()) } +#[tokio::test] +#[serial] +async fn test_service_account_policy_from_accountinfo_round_trips_real_single_node() -> TestResult { + init_logging(); + + let mut env = RustFSTestEnvironment::new().await?; + env.start_rustfs_server(vec![]).await?; + + let account_info = get_account_info(&env, &env.access_key, &env.secret_key).await?; + let policy_str = account_info + .get("policy") + .and_then(|value| value.as_str()) + .ok_or("account info policy should be a JSON string")?; + + let policy: serde_json::Value = serde_json::from_str(policy_str)?; + let statements = policy + .get("Statement") + .and_then(|value| value.as_array()) + .ok_or("account info policy should include Statement array")?; + + assert!(!statements.is_empty(), "account info policy Statement should not be empty: {policy}"); + + let req = AddServiceAccountReq { + policy: Some(policy), + target_user: None, + access_key: "svcacct-info-sample".to_string(), + secret_key: "svcacct-info-sample-secret-key-123456".to_string(), + name: Some("svcacct-info-sample".to_string()), + description: Some("service account created from accountinfo sample policy".to_string()), + expiration: None, + comment: None, + }; + + let created = add_service_account(&env, &env.access_key, &env.secret_key, &req).await?; + assert_eq!(created.0, "svcacct-info-sample"); + + let listed = + wait_for_service_accounts(&env, &env.access_key, &env.secret_key, Some(&env.access_key), &["svcacct-info-sample"]) + .await?; + assert!( + listed + .accounts + .iter() + .any(|account| account.access_key == "svcacct-info-sample"), + "created service account should be listed for parent user: {:?}", + listed.accounts + ); + + Ok(()) +} + #[tokio::test] #[serial] async fn test_site_replication_replicates_multiple_service_accounts_real_dual_node() -> Result<(), Box> { diff --git a/crates/policy/src/policy.rs b/crates/policy/src/policy.rs index 756f44a2e..43ae5b72a 100644 --- a/crates/policy/src/policy.rs +++ b/crates/policy/src/policy.rs @@ -67,6 +67,9 @@ pub enum Error { #[error("invalid action: '{0}'")] InvalidAction(String), + #[error("'Action' contains mixed action families in the same statement")] + MixedActionFamilies, + #[error("invalid resource, type: '{0}', pattern: '{1}'")] InvalidResource(String, String), } diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index ce33efabb..8311a8142 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -364,31 +364,45 @@ pub mod default { use super::Policy; #[allow(clippy::incompatible_msrv)] - pub static DEFAULT_POLICIES: LazyLock<[(&'static str, Policy); 6]> = LazyLock::new(|| { + pub static DEFAULT_POLICIES: LazyLock<[(&'static str, Policy); 5]> = LazyLock::new(|| { [ ( "readwrite", Policy { id: "".into(), version: DEFAULT_VERSION.into(), - statements: vec![Statement { - sid: "".into(), - effect: Effect::Allow, - actions: ActionSet({ - let mut hash_set = HashSet::new(); - hash_set.insert(Action::S3Action(S3Action::AllActions)); - hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction)); - hash_set - }), - not_actions: ActionSet(Default::default()), - resources: ResourceSet({ - let mut hash_set = HashSet::new(); - hash_set.insert(Resource::S3("*".into())); - hash_set - }), - conditions: Functions::default(), - ..Default::default() - }], + statements: vec![ + Statement { + sid: "".into(), + effect: Effect::Allow, + actions: ActionSet({ + let mut hash_set = HashSet::new(); + hash_set.insert(Action::S3Action(S3Action::AllActions)); + hash_set + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet({ + let mut hash_set = HashSet::new(); + hash_set.insert(Resource::S3("*".into())); + hash_set + }), + 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 + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet(Default::default()), + conditions: Functions::default(), + ..Default::default() + }, + ], }, ), ( @@ -396,26 +410,40 @@ pub mod default { Policy { id: "".into(), version: DEFAULT_VERSION.into(), - statements: vec![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.insert(Action::StsAction(StsAction::AssumeRoleAction)); - hash_set - }), - not_actions: ActionSet(Default::default()), - resources: ResourceSet({ - let mut hash_set = HashSet::new(); - hash_set.insert(Resource::S3("*".into())); - hash_set - }), - conditions: Functions::default(), - ..Default::default() - }], + statements: vec![ + 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 + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet({ + let mut hash_set = HashSet::new(); + hash_set.insert(Resource::S3("*".into())); + hash_set + }), + 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 + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet(Default::default()), + conditions: Functions::default(), + ..Default::default() + }, + ], }, ), ( @@ -423,49 +451,38 @@ pub mod default { Policy { id: "".into(), version: DEFAULT_VERSION.into(), - statements: vec![Statement { - sid: "".into(), - effect: Effect::Allow, - actions: ActionSet({ - let mut hash_set = HashSet::new(); - hash_set.insert(Action::S3Action(S3Action::PutObjectAction)); - hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction)); - hash_set - }), - not_actions: ActionSet(Default::default()), - resources: ResourceSet({ - let mut hash_set = HashSet::new(); - hash_set.insert(Resource::S3("*".into())); - hash_set - }), - conditions: Functions::default(), - ..Default::default() - }], - }, - ), - ( - "writeonly", - Policy { - id: "".into(), - version: DEFAULT_VERSION.into(), - statements: vec![Statement { - sid: "".into(), - effect: Effect::Allow, - actions: ActionSet({ - let mut hash_set = HashSet::new(); - hash_set.insert(Action::S3Action(S3Action::PutObjectAction)); - hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction)); - hash_set - }), - not_actions: ActionSet(Default::default()), - resources: ResourceSet({ - let mut hash_set = HashSet::new(); - hash_set.insert(Resource::S3("*".into())); - hash_set - }), - conditions: Functions::default(), - ..Default::default() - }], + statements: vec![ + Statement { + sid: "".into(), + effect: Effect::Allow, + actions: ActionSet({ + let mut hash_set = HashSet::new(); + hash_set.insert(Action::S3Action(S3Action::PutObjectAction)); + hash_set + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet({ + let mut hash_set = HashSet::new(); + hash_set.insert(Resource::S3("*".into())); + hash_set + }), + 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 + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet(Default::default()), + conditions: Functions::default(), + ..Default::default() + }, + ], }, ), ( @@ -473,31 +490,45 @@ pub mod default { Policy { id: "".into(), version: DEFAULT_VERSION.into(), - statements: vec![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.insert(Action::StsAction(StsAction::AssumeRoleAction)); - hash_set - }), - not_actions: ActionSet(Default::default()), - resources: ResourceSet({ - let mut hash_set = HashSet::new(); - hash_set.insert(Resource::S3("*".into())); - hash_set - }), - conditions: Functions::default(), - ..Default::default() - }], + statements: vec![ + 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 + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet({ + let mut hash_set = HashSet::new(); + hash_set.insert(Resource::S3("*".into())); + hash_set + }), + 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 + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet(Default::default()), + conditions: Functions::default(), + ..Default::default() + }, + ], }, ), ( @@ -512,7 +543,6 @@ pub mod default { actions: ActionSet({ let mut hash_set = HashSet::new(); hash_set.insert(Action::AdminAction(AdminAction::AllAdminActions)); - hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction)); hash_set }), not_actions: ActionSet(Default::default()), @@ -526,7 +556,6 @@ pub mod default { actions: ActionSet({ let mut hash_set = HashSet::new(); hash_set.insert(Action::KmsAction(KmsAction::AllActions)); - hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction)); hash_set }), not_actions: ActionSet(Default::default()), @@ -540,7 +569,6 @@ pub mod default { actions: ActionSet({ let mut hash_set = HashSet::new(); hash_set.insert(Action::S3Action(S3Action::AllActions)); - hash_set.insert(Action::StsAction(StsAction::AssumeRoleAction)); hash_set }), not_actions: ActionSet(Default::default()), @@ -552,6 +580,19 @@ pub mod default { 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 + }), + not_actions: ActionSet(Default::default()), + resources: ResourceSet(HashSet::new()), + conditions: Functions::default(), + ..Default::default() + }, ], }, ), @@ -713,6 +754,14 @@ mod test { } } + #[test] + fn test_default_policy_names_are_unique() { + let mut names = HashSet::new(); + for (name, _) in default::DEFAULT_POLICIES.iter() { + assert!(names.insert(*name), "duplicate default policy name: {name}"); + } + } + #[tokio::test] async fn test_deny_only_checks_only_deny_statements() -> Result<()> { let data = r#" @@ -1448,6 +1497,166 @@ mod test { ); } + #[test] + fn test_admin_statement_without_resource_is_valid() { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["admin:ServerInfo"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!( + result.is_ok(), + "Admin-only Action statement without Resource should be valid, got: {:?}", + result.err() + ); + } + + #[test] + fn test_sts_statement_without_resource_is_valid() { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["sts:AssumeRole"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!( + result.is_ok(), + "STS-only Action statement without Resource should be valid, got: {:?}", + result.err() + ); + } + + #[test] + fn test_kms_statement_without_resource_is_valid() { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["kms:*"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!( + result.is_ok(), + "KMS-only Action statement without Resource should be valid, got: {:?}", + result.err() + ); + } + + #[test] + fn test_mixed_action_families_are_invalid_even_with_resource() { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["admin:*", "sts:AssumeRole"], + "Resource": ["arn:aws:s3:::*"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_err(), "Mixed action families should be rejected"); + assert!( + matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::MixedActionFamilies)), + "Error should be MixedActionFamilies, got: {:?}", + result.unwrap_err() + ); + } + + #[test] + fn test_mixed_action_families_are_invalid_even_without_resource() { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["admin:*", "s3:GetObject"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_err(), "Mixed action families should be rejected even when Resource is missing"); + assert!( + matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::MixedActionFamilies)), + "Error should be MixedActionFamilies, got: {:?}", + result.unwrap_err() + ); + } + + #[test] + fn test_mixed_action_families_with_wildcard_variants_are_invalid() { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:*", "admin:*", "sts:AssumeRole"], + "Resource": ["arn:aws:s3:::*"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_err(), "Mixed action families with wildcard variants should be rejected"); + assert!( + matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::MixedActionFamilies)), + "Error should be MixedActionFamilies, got: {:?}", + result.unwrap_err() + ); + } + + #[test] + fn test_notaction_without_resource_remains_invalid() { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "NotAction": ["s3:DeleteObject"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_err(), "NotAction statement without Resource should remain invalid"); + assert!( + matches!(result.as_ref().unwrap_err(), Error::PolicyError(IamError::NonResource)), + "Error should be NonResource, got: {:?}", + result.unwrap_err() + ); + } + #[test] fn test_bucket_policy_serialize_omits_empty_fields() { use crate::policy::action::{Action, ActionSet, S3Action}; diff --git a/crates/policy/src/policy/statement.rs b/crates/policy/src/policy/statement.rs index 6498b55c0..6bea77cc8 100644 --- a/crates/policy/src/policy/statement.rs +++ b/crates/policy/src/policy/statement.rs @@ -78,6 +78,15 @@ fn build_resource(action: &Action, bucket: &str, object: &str, bucket_resource_o resource } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ActionFamily { + S3, + Admin, + Sts, + Kms, + Mixed, +} + impl Statement { fn is_kms(&self) -> bool { for act in self.actions.iter() { @@ -109,6 +118,48 @@ impl Statement { false } + fn action_family(&self) -> Option { + if self.actions.is_empty() { + return None; + } + + let mut saw_s3 = false; + let mut saw_admin = false; + let mut saw_sts = false; + let mut saw_kms = false; + + for action in self.actions.iter() { + match action { + Action::S3Action(_) => saw_s3 = true, + Action::AdminAction(_) => saw_admin = true, + Action::StsAction(_) => saw_sts = true, + Action::KmsAction(_) => saw_kms = true, + Action::None => {} + } + } + + let family_count = saw_s3 as u8 + saw_admin as u8 + saw_sts as u8 + saw_kms as u8; + + if family_count != 1 { + return Some(ActionFamily::Mixed); + } + + if saw_s3 { + return Some(ActionFamily::S3); + } + if saw_admin { + return Some(ActionFamily::Admin); + } + if saw_sts { + return Some(ActionFamily::Sts); + } + if saw_kms { + return Some(ActionFamily::Kms); + } + + Some(ActionFamily::Mixed) + } + /// Returns true when this statement would reach `conditions.evaluate_with_resolver` in /// [`Statement::is_allowed`] (including the KMS shortcut path). Does not evaluate conditions. pub(crate) async fn request_reaches_condition_eval(&self, args: &Args<'_>, resolver: &VariableResolver) -> bool { @@ -186,9 +237,25 @@ impl Validator for Statement { return Err(IamError::BothActionAndNotAction.into()); } - // policy must contain either Resource or NotResource (but not both), and cannot have both empty. + let action_family = if self.not_actions.is_empty() { + match self.action_family() { + Some(ActionFamily::Mixed) => return Err(IamError::MixedActionFamilies.into()), + family => family, + } + } else { + None + }; + + // Policy must contain either Resource or NotResource (but not both), unless + // the statement is Action-mode Admin/STS/KMS. if self.resources.is_empty() && self.not_resources.is_empty() { - return Err(IamError::NonResource.into()); + let allow_empty_resource = matches!( + action_family, + Some(ActionFamily::Admin) | Some(ActionFamily::Sts) | Some(ActionFamily::Kms) + ); + if !allow_empty_resource { + return Err(IamError::NonResource.into()); + } } if !self.resources.is_empty() && !self.not_resources.is_empty() {