fix(server): honor explicit Deny in role trust policies

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()));
+    }
+}

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 <arn>`) 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.

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 |
+| [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 |