From 9952f5ff2382a1619a2e69c526c4c21a862168a5 Mon Sep 17 00:00:00 2001 From: Amol Navare Date: Thu, 15 Oct 2026 19:58:27 -0700 Subject: [PATCH] fix(server): honor explicit Deny in role trust policies evaluate_trust_policy() skipped every statement whose Effect was not "Allow", so Deny statements in a role's trust policy were silently ignored. An administrator relying on a Deny to revoke a principal's ability to assume a role got no protection: a broad `Allow *` still granted assumption to a principal an accompanying Deny was meant to block. Rework evaluation to follow IAM order, matching the identity-policy evaluator (policy/evaluator.rs, phase 1): scan for a matching explicit Deny first and block assumption if one is found, then look for a matching Allow. Factor the decision into a store-free `trust_decision` helper plus `statement_matches`/`effect_of` so the Deny-over-Allow ordering is unit-testable, and add tests covering explicit-deny override (both statement orders), principal-scoped Deny, conditional Deny via aws:PrincipalTag, and the existing Allow/implicit-deny paths. Record the decision in ADR-0022. Reported via AWS Bug Bounty (P472369996). The AssumeRole endpoint is admin-only (Basic Auth) with an admin-supplied caller ARN, which bounds real-world exposure, but the authorization logic was incorrect. --- crates/server/src/management/assume_role.rs | 228 ++++++++++++++++-- docs/adr/0022-trust-policy-deny-over-allow.md | 107 ++++++++ docs/adr/README.md | 3 +- 3 files changed, 320 insertions(+), 18 deletions(-) create mode 100644 docs/adr/0022-trust-policy-deny-over-allow.md diff --git a/crates/server/src/management/assume_role.rs b/crates/server/src/management/assume_role.rs index af4a41397..f06d50b49 100755 --- a/crates/server/src/management/assume_role.rs +++ b/crates/server/src/management/assume_role.rs @@ -272,14 +272,25 @@ async fn generate_and_store_session( /// Evaluate a trust policy to determine if `caller_arn` is allowed to assume /// the role. /// -/// Checks each Statement for `Effect: Allow` with a matching Principal and -/// passing conditions. Principal matching supports: +/// Follows IAM evaluation order: an explicit `Deny` that matches the caller +/// wins over any `Allow`, exactly as the identity-policy evaluator does +/// (`policy/evaluator.rs`, phase 1). Only if no matching `Deny` is found does +/// a matching `Allow` grant assumption. See ADR-0022. +/// +/// A statement "matches" when its Principal matches the caller and all of its +/// conditions pass. Principal matching supports: /// - `"*"` (any caller) /// - `{"AWS": "arn:..."}` (single ARN) /// - `{"AWS": ["arn:...", ...]}` (list of ARNs) /// /// Conditions are evaluated using the policy engine's `AssumeRoleContext`, -/// which supports `aws:PrincipalTag/*` and `sts:ExternalId` keys. +/// which supports `aws:PrincipalTag/*` and `sts:ExternalId` keys. A statement +/// whose `Condition` block fails to parse is treated as non-matching: it can +/// neither grant via `Allow` nor block via `Deny`. Treating an unparseable +/// `Deny` as "does not match" is the fail-open direction for that one +/// statement, but a malformed trust policy is an administrator error surfaced +/// at role creation, and matching the parse-failure handling of the `Allow` +/// path keeps the two consistent. async fn evaluate_trust_policy( trust_policy: &Value, caller_arn: &str, @@ -296,30 +307,52 @@ async fn evaluate_trust_policy( external_id: external_id.map(String::from), }; - for stmt in statements { - let effect = stmt.get("Effect").and_then(|e| e.as_str()).unwrap_or(""); - if effect != "Allow" { - continue; - } + trust_decision(statements, caller_arn, &context) +} - let principal = &stmt["Principal"]; - if !principal_matches(principal, caller_arn) { - continue; +/// Decide assumption from already-resolved statements and context. +/// +/// Split out from `evaluate_trust_policy` so the Deny-over-Allow ordering is +/// unit-testable without a `CatalogStore` (the store is only needed to build +/// the context's principal tags). +fn trust_decision(statements: &[Value], caller_arn: &str, context: &AssumeRoleContext) -> bool { + // Phase 1: explicit Deny. Any matching Deny statement blocks assumption + // outright, regardless of any Allow present in the same policy. + for stmt in statements { + if effect_of(stmt) == "Deny" && statement_matches(stmt, caller_arn, context) { + return false; } + } - let conditions_pass = match parse_conditions(&stmt["Condition"]) { - Ok(conds) => conds.iter().all(|c| evaluate_condition(c, &context)), - Err(_) => false, - }; - - if conditions_pass { + // Phase 2: Allow. With no matching Deny, a matching Allow grants assumption. + for stmt in statements { + if effect_of(stmt) == "Allow" && statement_matches(stmt, caller_arn, context) { return true; } } + // Implicit deny: no statement granted assumption. false } +/// The `Effect` string of a statement, or `""` when absent/non-string. +fn effect_of(stmt: &Value) -> &str { + stmt.get("Effect").and_then(|e| e.as_str()).unwrap_or("") +} + +/// Whether a trust-policy statement matches the caller: its Principal matches +/// `caller_arn` and every condition passes. Independent of the statement's +/// `Effect`, so both the Deny and Allow phases share one matching definition. +fn statement_matches(stmt: &Value, caller_arn: &str, context: &AssumeRoleContext) -> bool { + if !principal_matches(&stmt["Principal"], caller_arn) { + return false; + } + match parse_conditions(&stmt["Condition"]) { + Ok(conds) => conds.iter().all(|c| evaluate_condition(c, context)), + Err(_) => false, + } +} + /// Check if a Principal field matches the given caller ARN. fn principal_matches(principal: &Value, caller_arn: &str) -> bool { match principal { @@ -371,3 +404,164 @@ async fn fetch_caller_tags( } } } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + use std::collections::HashMap; + + fn ctx() -> AssumeRoleContext { + AssumeRoleContext { + principal_tags: HashMap::new(), + external_id: None, + } + } + + fn ctx_with_tags(tags: &[(&str, &str)]) -> AssumeRoleContext { + AssumeRoleContext { + principal_tags: tags + .iter() + .map(|(k, v)| ((*k).to_owned(), (*v).to_owned())) + .collect(), + external_id: None, + } + } + + const CALLER: &str = "arn:aws:iam::123456789012:user/alice"; + + // ---- principal_matches ------------------------------------------------ + + #[test] + fn principal_wildcard_string_matches_any() { + assert!(principal_matches(&json!("*"), CALLER)); + } + + #[test] + fn principal_aws_single_arn_matches_exact() { + assert!(principal_matches(&json!({"AWS": CALLER}), CALLER)); + assert!(!principal_matches( + &json!({"AWS": "arn:aws:iam::123456789012:user/bob"}), + CALLER + )); + } + + #[test] + fn principal_aws_wildcard_matches_any() { + assert!(principal_matches(&json!({"AWS": "*"}), CALLER)); + } + + #[test] + fn principal_aws_list_matches_any_member() { + let p = json!({"AWS": ["arn:aws:iam::123456789012:user/bob", CALLER]}); + assert!(principal_matches(&p, CALLER)); + let p2 = json!({"AWS": ["arn:aws:iam::123456789012:user/bob"]}); + assert!(!principal_matches(&p2, CALLER)); + } + + #[test] + fn principal_missing_aws_does_not_match() { + assert!(!principal_matches( + &json!({"Service": "ec2.amazonaws.com"}), + CALLER + )); + assert!(!principal_matches(&json!(null), CALLER)); + } + + // ---- effect_of -------------------------------------------------------- + + #[test] + fn effect_of_reads_string_or_defaults_empty() { + assert_eq!(effect_of(&json!({"Effect": "Allow"})), "Allow"); + assert_eq!(effect_of(&json!({"Effect": "Deny"})), "Deny"); + assert_eq!(effect_of(&json!({})), ""); + assert_eq!(effect_of(&json!({"Effect": 5})), ""); + } + + // ---- trust_decision: core regression --------------------------------- + + #[test] + fn allow_grants_when_principal_matches() { + let stmts = vec![json!({"Effect": "Allow", "Principal": {"AWS": CALLER}})]; + assert!(trust_decision(&stmts, CALLER, &ctx())); + } + + #[test] + fn no_matching_allow_is_implicit_deny() { + let stmts = vec![json!({ + "Effect": "Allow", + "Principal": {"AWS": "arn:aws:iam::123456789012:user/bob"} + })]; + assert!(!trust_decision(&stmts, CALLER, &ctx())); + } + + /// The ticket regression: an explicit Deny must override a broad Allow. + /// Before the fix, Deny statements were skipped entirely and the caller + /// was granted assumption via the `Allow *`. + #[test] + fn explicit_deny_overrides_allow_regardless_of_order() { + // Deny listed after Allow. + let allow_then_deny = vec![ + json!({"Effect": "Allow", "Principal": "*"}), + json!({"Effect": "Deny", "Principal": {"AWS": CALLER}}), + ]; + assert!( + !trust_decision(&allow_then_deny, CALLER, &ctx()), + "explicit Deny after Allow must block assumption" + ); + + // Deny listed before Allow. + let deny_then_allow = vec![ + json!({"Effect": "Deny", "Principal": {"AWS": CALLER}}), + json!({"Effect": "Allow", "Principal": "*"}), + ]; + assert!( + !trust_decision(&deny_then_allow, CALLER, &ctx()), + "explicit Deny before Allow must block assumption" + ); + } + + #[test] + fn deny_targeting_other_principal_does_not_block() { + let stmts = vec![ + json!({"Effect": "Allow", "Principal": "*"}), + json!({"Effect": "Deny", "Principal": {"AWS": "arn:aws:iam::123456789012:user/bob"}}), + ]; + assert!( + trust_decision(&stmts, CALLER, &ctx()), + "a Deny aimed at a different principal must not block this caller" + ); + } + + #[test] + fn conditional_deny_only_blocks_when_condition_matches() { + // Deny alice only when her PrincipalTag/team == "contractors". + let stmts = vec![ + json!({"Effect": "Allow", "Principal": "*"}), + json!({ + "Effect": "Deny", + "Principal": {"AWS": CALLER}, + "Condition": {"StringEquals": {"aws:PrincipalTag/team": "contractors"}} + }), + ]; + + // Tag matches → Deny applies → blocked. + assert!(!trust_decision( + &stmts, + CALLER, + &ctx_with_tags(&[("team", "contractors")]) + )); + + // Tag does not match → Deny inert → Allow grants. + assert!(trust_decision( + &stmts, + CALLER, + &ctx_with_tags(&[("team", "employees")]) + )); + } + + #[test] + fn empty_statement_list_denies() { + assert!(!trust_decision(&[], CALLER, &ctx())); + } +} diff --git a/docs/adr/0022-trust-policy-deny-over-allow.md b/docs/adr/0022-trust-policy-deny-over-allow.md new file mode 100644 index 000000000..e7c02c1e1 --- /dev/null +++ b/docs/adr/0022-trust-policy-deny-over-allow.md @@ -0,0 +1,107 @@ +# ADR-0022: Role trust-policy evaluation follows IAM Deny-over-Allow order + +- Status: Accepted +- Date: 2026-10-15 +- Deciders: @navamol + +## Context + +`AssumeRole` (`crates/server/src/management/assume_role.rs`) decides whether a +caller may assume a role by evaluating the role's trust policy against the +caller ARN. The original `evaluate_trust_policy()` iterated the statements and +skipped any whose `Effect` was not `"Allow"`: + +```rust +if effect != "Allow" { + continue; +} +``` + +A matching `Allow` returned `true`; otherwise the function returned `false` +(implicit deny). The consequence is that `Deny` statements in a trust policy +were never evaluated — they behaved as no-ops. An administrator who added a +`Deny` for a specific principal alongside a broad `Allow` (for example +`Allow Principal "*"` plus `Deny Principal `) got no protection: the denied +principal was still granted assumption through the `Allow`. + +This diverges from IAM semantics and, more importantly, from ExtendDB's own +identity-policy evaluator (`crates/auth/src/policy/evaluator.rs`), which already +implements the standard order — phase 1 is an explicit-deny scan across all +policies before any `Allow` is considered. Trust-policy evaluation and +identity-policy evaluation disagreeing on how `Deny` works is a correctness and +least-privilege hazard: the behavior a trust policy's author reasonably expects +(a `Deny` denies) is the behavior the identity evaluator already provides. + +Reported via AWS Bug Bounty. The endpoint is admin-only (Basic Auth) and the +admin supplies the caller ARN, which bounds the practical blast radius, but the +authorization logic itself was incorrect. + +## Options Considered + +1. **Leave Allow-only, document the limitation.** Keep the existing behavior and + state in the docs that trust policies support only `Allow`. Cheapest, but it + leaves a silently-ignored `Deny` as a trap: policy authors would still write + `Deny` statements expecting them to work, and the two policy evaluators would + remain inconsistent. + +2. **Reject trust policies that contain `Deny` at role-creation time.** Fail + fast so no one can author a `Deny` that does nothing. Removes the trap but + also removes a legitimate, expected capability, and still leaves trust-policy + evaluation less expressive than identity-policy evaluation. + +3. **Evaluate `Deny` before `Allow` (IAM order).** Scan for a matching explicit + `Deny` first and block assumption if one is found; only then look for a + matching `Allow`. Matches IAM and the existing identity-policy evaluator. + +## Decision + +Trust-policy evaluation follows IAM order: a matching explicit `Deny` blocks +role assumption and is evaluated before any `Allow` (Option 3). With no matching +`Deny`, a matching `Allow` grants assumption; with neither, assumption is +implicitly denied. + +## Rationale + +- **Consistency with the identity-policy evaluator.** `policy/evaluator.rs` + already does explicit-deny-first across identity policies, permissions + boundaries, and session policies. Trust policies evaluating `Deny` differently + was an inconsistency a reader would have to discover by accident. +- **Principle of least astonishment / least privilege.** A trust policy that + says `Deny` must deny. Silently ignoring it fails safe only by luck (when + there is no broad `Allow`) and fails open in exactly the case an author writes + a `Deny` to guard against. +- **Matches the semantics callers already know.** ExtendDB speaks the DynamoDB / + IAM wire contract; explicit-deny-wins is the IAM evaluation rule operators + expect. +- **Not an auth-model change.** This corrects behavior to the documented and + already-implemented-elsewhere model. It does not introduce a new policy + language, condition key, or evaluation phase, so it is a bug fix rather than an + RFC-level change to the auth model. + +## Consequences + +- **Easier:** trust-policy authors can rely on `Deny`, and the two policy + evaluators now share one mental model (explicit deny wins). +- **Behavioral change to be aware of:** a trust policy that already contained a + matching `Deny` alongside a broad `Allow` previously granted assumption and + will now block it. This is the intended fix, but it is a behavior change for + any deployment that (perhaps unknowingly) relied on `Deny` being ignored. There + is no change for trust policies that contain only `Allow` statements. +- **Follow-up:** the decision logic was factored into a store-free + `trust_decision` helper (plus `statement_matches` / `effect_of`) so the + Deny-over-Allow ordering is unit-tested directly. A statement whose + `Condition` block fails to parse is treated as non-matching in both the `Deny` + and `Allow` phases, matching the pre-existing `Allow`-path handling. +- **Commitment:** trust-policy and identity-policy evaluation should stay aligned + on evaluation order; a future change to one should consider the other. + +--- + +## License + +Copyright 2026 ExtendDB contributors. Licensed under the Apache License, Version 2.0. +See [LICENSE](../../LICENSE) for the full text. + +This software is provided "as is" without warranty of any kind. ExtendDB is not +affiliated with, endorsed by, or sponsored by Amazon Web Services. "DynamoDB" is +a trademark of Amazon.com, Inc. diff --git a/docs/adr/README.md b/docs/adr/README.md index 3b06a97b1..2deec1259 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -47,4 +47,5 @@ decision, write a new ADR. | [0018](0018-cassandra-stream-implementation.md) | Cassandra: DynamoDB Streams implementation | Accepted | | [0019](0019-cassandra-logical-backup-restore.md) | Cassandra: logical backup and restore | Accepted | | [0020](0020-cassandra-occ-update-item.md) | Cassandra: optimistic concurrency control for UpdateItem and PutItem | Accepted | -| [0021](0021-cassandra-lwt-delete-atomicity-gap.md) | Cassandra: LWT delete atomicity gap and required worker/saga pattern | Accepted | \ No newline at end of file +| [0021](0021-cassandra-lwt-delete-atomicity-gap.md) | Cassandra: LWT delete atomicity gap and required worker/saga pattern | Accepted | +| [0022](0022-trust-policy-deny-over-allow.md) | Role trust-policy evaluation follows IAM Deny-over-Allow order | Accepted | \ No newline at end of file