diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 156e4d231..d412fa789 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -2135,6 +2135,87 @@ mod test { Ok(()) } + #[tokio::test] + async fn test_kms_bundle_actions_require_an_unscoped_statement() -> Result<()> { + use crate::policy::action::{Action, KmsAction}; + + let scoped = Policy::parse_config( + br#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["kms:*"], + "Resource": ["arn:aws:kms:::key/key-a"] + } + ] +}"#, + )?; + let unscoped = Policy::parse_config( + br#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["kms:Backup", "kms:Restore"] + } + ] +}"#, + )?; + let partially_scoped = Policy::parse_config( + br#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["kms:Backup", "kms:Restore"], + "NotResource": ["arn:aws:kms:::key/protected"] + } + ] +}"#, + )?; + let scoped_deny = Policy::parse_config( + br#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["kms:Backup", "kms:Restore"] + }, + { + "Effect": "Deny", + "Action": ["kms:Backup", "kms:Restore"], + "Resource": ["arn:aws:kms:::key/protected"] + } + ] +}"#, + )?; + let conditions = HashMap::new(); + let claims = HashMap::new(); + + for action in [KmsAction::BackupAction, KmsAction::RestoreAction] { + let args = kms_args(Action::KmsAction(action), "", &conditions, &claims); + assert!( + !scoped.is_allowed(&args).await, + "a single-key grant must not authorize the all-key {action:?} operation" + ); + assert!( + !partially_scoped.is_allowed(&args).await, + "a grant excluding one key must not authorize the all-key {action:?} operation" + ); + assert!( + unscoped.is_allowed(&args).await, + "an action-only grant must continue authorizing {action:?}" + ); + assert!( + !scoped_deny.is_allowed(&args).await, + "a deny for included key material must block the all-key {action:?} operation" + ); + } + + Ok(()) + } + #[tokio::test] async fn test_kms_statement_without_resource_matches_every_key() -> Result<()> { use crate::policy::action::{Action, KmsAction}; diff --git a/crates/policy/src/policy/statement.rs b/crates/policy/src/policy/statement.rs index a45466713..93e44a3d7 100644 --- a/crates/policy/src/policy/statement.rs +++ b/crates/policy/src/policy/statement.rs @@ -14,7 +14,7 @@ use super::{ ActionSet, Args, BucketPolicyArgs, Effect, Error as IamError, Functions, ID, Principal, ResourceSet, Validator, - action::{Action, S3Action}, + action::{Action, KmsAction, S3Action}, function::key_name::{KeyName, S3KeyName}, resource::Resource, variables::{VariableContext, VariableResolver}, @@ -182,6 +182,14 @@ impl Statement { /// left empty. An empty `args.object` means the caller did not scope the request to a /// key, which preserves the legacy match-every-key behaviour. async fn kms_key_scope_matches(&self, args: &Args<'_>, resolver: &VariableResolver) -> bool { + if matches!(args.action, Action::KmsAction(KmsAction::BackupAction | KmsAction::RestoreAction)) + && (!self.resources.is_empty() || !self.not_resources.is_empty()) + { + // Global bundle operations require an unscoped Allow, while a Deny + // covering any key must still block an operation covering every key. + return matches!(self.effect, Effect::Deny); + } + let kms_resources: Vec<&Resource> = self.resources.iter().filter(|resource| resource.is_kms()).collect(); let kms_not_resources: Vec<&Resource> = self.not_resources.iter().filter(|resource| resource.is_kms()).collect();