fix(server): honor explicit Deny in role trust policies by navareamol · Pull Request #402 · ExtendDB/extenddb

navareamol · 2026-10-16T03:43:07Z

Summary

evaluate_trust_policy() in crates/server/src/management/assume_role.rs
skipped every statement whose Effect was not "Allow"
(if effect != "Allow" { continue; }), 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 that an accompanying Deny was meant to block.

This reworks trust-policy evaluation to follow IAM order — an explicit Deny
wins over Allow — matching the identity-policy evaluator already in the tree
(crates/auth/src/policy/evaluator.rs, phase 1 = explicit deny).

Changes

evaluate_trust_policy now scans for a matching explicit Deny first (blocks
assumption), then looks for a matching Allow (grants). No match → implicit deny.
Factored the decision into a store-free trust_decision helper plus
statement_matches / effect_of, so the Deny-over-Allow ordering is
unit-testable without a CatalogStore.
Added unit tests: explicit-deny override in both statement orders,
principal-scoped Deny (a Deny aimed at another principal must not block),
conditional Deny via aws:PrincipalTag, plus the existing Allow and
implicit-deny paths and principal_matches / effect_of coverage.

A statement whose Condition block fails to parse is treated as non-matching in
both phases (it can neither grant nor block), matching the pre-existing
Allow-path handling; this is documented on the function.

Why

Fixes: https://t.corp.amazon.com/P472369996/communication

Testing

Run under the project MSRV (Rust 1.88):

cargo +1.88 fmt --all -- --check
cargo +1.88 clippy -p extenddb-server --all-targets -- -D warnings
cargo +1.88 clippy -p extenddb-server --all-targets -- -W clippy::pedantic -W clippy::unwrap_used -W clippy::expect_used
cargo +1.88 test -p extenddb-server --lib

fmt: clean
clippy (-D warnings): clean
clippy (pedantic set from CONTRIBUTING.md): no new findings in the changed file
tests: 58 passed, 0 failed (including 12 management::assume_role::tests)
I have read CONTRIBUTING.md
All tests pass (cargo test --workspace)
Code is formatted (cargo fmt --check)
Clippy is clean (cargo clippy -- -W clippy::pedantic)
I have added or updated tests for new functionality
I have updated documentation if behavior changed — ADR-0022 records the decision and the behavior change; the evaluate_trust_policy doc comment points to it
Breaking changes are noted below — not a breaking API change; the behavior change (a matching Deny now blocks assumption) is documented in the PR's Notes section and ADR-0022. Leave unchecked, or check it and rely on that note — your call
ADR captures the decision — ADR-0022 (docs/adr/0022-trust-policy-deny-over-allow.md), no RFC needed (not an auth-model change)

Notes

This corrects an authorization defect: Deny statements in role trust policies
were not enforced, so they behaved as no-ops. The fix makes Deny behave as
documented and brings trust-policy evaluation in line with the identity-policy
evaluator. It is a behavioral bug fix, not a change to the auth model.

The decision is recorded in
ADR-0022.

The AssumeRole endpoint is admin-only (Basic Auth) and the admin supplies the
caller ARN, which bounds the practical impact, but the evaluation logic was
incorrect and is now fixed.

Behavior change to note: a trust policy that already contained a matching Deny
alongside a broad Allow previously granted assumption and will now block it.
Trust policies containing only Allow statements are unaffected.

Touches a CODEOWNERS-protected auth path, so it requires owner review.

Summaryevaluate_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.

What

Why

Closes #

Testing done

Checklist

I have read CONTRIBUTING.md
All tests pass (cargo test --workspace)
Code is formatted (cargo fmt --check)
Clippy is clean (cargo clippy -- -W clippy::pedantic)
I have added or updated tests for new functionality
I have updated documentation if behavior changed
Breaking changes are noted below (if any)
If this changes the wire protocol, Storage trait, auth model, on-disk
format, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).

ADR / RFC:

Breaking changes

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.

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.

LeeroyHannigan

The reorder is right and the unit tests catch its removal (2 of 12 fail with the Deny phase disabled). I ran 31 trust-policy documents against live IAM and STS and against this branch on SQLite and PostgreSQL, with main as control. Deny-over-Allow in both orders, Deny on another principal, {"AWS": "*"}, Null on sts:ExternalId and BoolIfExists all match IAM. fmt, clippy and the pedantic set are clean on 1.88.

What does not match. Each of these is a Deny that IAM enforces and this branch ignores, unless marked over-deny.

Blocking. A negated operator with an absent key never matches, so Deny with StringNotEquals sts:ExternalId allows when the caller sends no ExternalId. IAM denies.
Blocking. SQLite never resolves caller tags (split(":user/") against a user/<name> input), so a tag-conditioned Deny allows and a tag-conditioned Allow returns 403. PostgreSQL is correct.
Blocking. Trust policies are not validated at write. IAM rejected 8 of 14 malformed documents; create_role stored all 13 that had a Version. A Deny with an unknown operator is stored and dropped at evaluation. The doc comment claims the opposite.
Effect is compared case-sensitively. IAM accepts and enforces "deny".
Account root and account-id principals never match. On IAM a Deny on either covers every principal in the account.
Action is ignored. A Deny on sts:TagSession only over-denies a plain AssumeRole; an Allow on sts:TagSession only grants it; session tags without sts:TagSession are accepted. IAM does the reverse on all three.
No test goes through evaluate_trust_policy or the endpoint on any backend.
docs/design/05-component-auth.md and 05-component-auth-v3.md still describe Allow-only evaluation.

Minor: denials write no audit line; bare "Principal": "*" is not an IAM trust-policy shape; a single-object Statement denies everything (IAM accepts it); the PR body has the template pasted twice and links an internal ticket.

Measurements are on the inline comments.

LeeroyHannigan · 2026-10-16T09:16:58Z

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


Not true of the code. create_role (iam_role.rs:69) checks only that Version and Statement exist; nothing parses the statements. IAM rejected 8 of 14 malformed trust documents I sent to UpdateAssumeRolePolicy; this branch stored all 13 that had a Version. So an unparseable Deny is stored, then dropped at evaluation, so it grants. Validate at write, and fail closed on a Deny whose condition does not parse.

LeeroyHannigan · 2026-10-16T09:16:58Z

+    // 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) {


statement_matches ignores Action, so any matching Deny blocks whatever action it names. On IAM, Deny alice with Action: sts:TagSession only still allows a plain AssumeRole; this branch denies it. Same for sts:SetSourceIdentity. Match Action and NotAction against sts:AssumeRole (plus sts:TagSession when session_tags is set) the way the identity evaluator does.

LeeroyHannigan · 2026-10-16T09:16:58Z


+/// 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("")


Case-sensitive. IAM accepted "Effect": "deny" and enforced it; here the statement is skipped and the caller is allowed. parse_statement in document.rs:196 uses eq_ignore_ascii_case. Use the same.

LeeroyHannigan · 2026-10-16T09:16:58Z

+        return false;
+    }
+    match parse_conditions(&stmt["Condition"]) {
+        Ok(conds) => conds.iter().all(|c| evaluate_condition(c, context)),


evaluate_condition returns false for a negated operator when the key is absent; IAM returns true. Measured: Allow alice plus Deny alice with StringNotEquals sts:ExternalId = secret, called with no ExternalId. IAM denies, this branch allows on SQLite and PostgreSQL. That is the confused-deputy Deny and it does nothing here. The fix is in condition.rs (other => None => false); is_negative_operator already exists for the ForAllValues arm.

LeeroyHannigan · 2026-10-16T09:16:58Z

+    }
+    match parse_conditions(&stmt["Condition"]) {
+        Ok(conds) => conds.iter().all(|c| evaluate_condition(c, context)),
+        Err(_) => false,


With no validation at write this is where a mistyped Deny disappears. Deny alice with StringEqualz, and with NotIpAddress (not in parse_base_operator), both allow here. IAM rejects the first document and enforces the second. A Deny that fails to parse should match.

LeeroyHannigan · 2026-10-16T09:16:58Z

 }
+
+#[cfg(test)]
+mod tests {


Nothing here goes through evaluate_trust_policy, fetch_caller_tags or the endpoint. tests/test_auth_integration.py has create_role and assume_role and runs on all four backends in CI. Add Deny-over-Allow, the tag Deny and the ExternalId Deny there. The tag case fails on SQLite today, see below.

LeeroyHannigan · 2026-10-16T09:16:58Z

+    // ---- principal_matches ------------------------------------------------
+
+    #[test]
+    fn principal_wildcard_string_matches_any() {


IAM rejects a bare "*" principal in a trust policy (invalid principal: "STAR":"*") and accepts {"AWS": "*"}. This pins the bare form as supported. Document it as an extension or drop it.

LeeroyHannigan · 2026-10-16T09:16:58Z

+    }
+
+    #[test]
+    fn conditional_deny_only_blocks_when_condition_matches() {


This passes because the context is built in memory; on SQLite the wire does the opposite. storage-sqlite/src/management_store/mod.rs:595 splits on ":user/", but fetch_caller_tags above passes user/<name>, so the tag map is always empty. Measured with alice tagged team=contractors: this Deny allows on SQLite (IAM and PostgreSQL deny), and an Allow on the same tag returns 403. The other three backends use strip_prefix("user/"). Fix the SQLite store and add a wire test.

LeeroyHannigan · 2026-10-16T09:16:58Z

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


The documented model was Allow-only: docs/design/05-component-auth.md step 6 and 05-component-auth-v3.md step 2. Update both, and add to the admin guide what a trust-policy Deny matches and what it does not (account principals, Action, NotPrincipal).

LeeroyHannigan · 2026-10-16T09:16:58Z

+- **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`


This is the fail-open choice, and "surfaced at role creation" is not true of create_role. Deny alice with StringEqualz is stored and the caller is allowed; IAM refuses the document. Validate at write and fail closed, or say plainly that a malformed Deny grants.

LeeroyHannigan · 2026-10-21T11:06:28Z

Thanks @navareamol. Your fix for the ignored trust-policy Deny is the first commit of #420, with your authorship kept and the commit message unchanged apart from one sentence.

#420 builds on it: trust policies are validated at write the way IAM validates them, the condition keys STS populates for AssumeRole are resolved so a Deny gated on them applies, and identity-policy conditions on DynamoDB requests get the same treatment. Each expected result was measured against IAM, STS and DynamoDB.

I suggest closing this PR in favour of #420. If you would rather land your commit here first, say so and I will rebase #420 on it.

navareamol requested review from LeeroyHannigan, amrith, c33howard, jcshepherd, pdf-amzn, robinnsc and yesyayen as code owners October 16, 2026 03:43

LeeroyHannigan requested changes Oct 16, 2026

View reviewed changes

LeeroyHannigan mentioned this pull request Oct 21, 2026

fix(auth): enforce trust-policy Deny and match IAM condition evaluation #420

Merged

8 tasks done

Navigation Menu

Repository navigation

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

fix(server): honor explicit Deny in role trust policies#402
navareamol wants to merge 1 commit into
mainfrom
fix/trust-policy-deny-ignored

navareamol commented Oct 16, 2026 •

edited by LeeroyHannigan

Loading

Uh oh!

LeeroyHannigan left a comment

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan Oct 16, 2026

Uh oh!

LeeroyHannigan commented Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

navareamol commented Oct 16, 2026 • edited by LeeroyHannigan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Why

Testing

Notes

What

Why

Testing done

Checklist

Breaking changes

Uh oh!

LeeroyHannigan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeeroyHannigan commented Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

navareamol commented Oct 16, 2026 •

edited by LeeroyHannigan

Loading