From 803b45126c7c587fb331212ac69176d5755d9cd8 Mon Sep 17 00:00:00 2001 From: Eric Conklin Date: Thu, 4 Jun 2026 14:38:02 -0500 Subject: [PATCH] Fix SCP binder source account scope --- iamscope/resolver/scp_binder.py | 17 +++-- tests/test_scp_binder.py | 112 +++++++++++++++++++++++++++++++- 2 files changed, 121 insertions(+), 8 deletions(-) diff --git a/iamscope/resolver/scp_binder.py b/iamscope/resolver/scp_binder.py index 890b576..75ad80b 100644 --- a/iamscope/resolver/scp_binder.py +++ b/iamscope/resolver/scp_binder.py @@ -134,7 +134,9 @@ def bind_all_scps( For each (edge, constraint) pair, checks if the SCP's action scope matches the edge action. OU inheritance is handled by checking if - the edge's destination account is within the constraint's scope. + the edge's source/principal account is within the constraint's scope. + SCPs govern principals, not target resources; resource pattern + matching still uses the edge destination inside bind_scp_to_edge. Args: edges: All edges in the graph. @@ -150,13 +152,18 @@ def bind_all_scps( bindings: list[EdgeConstraint] = [] for edge in edges: - dst_account = _extract_account_from_ref(edge.dst) + src_account = _extract_account_from_ref(edge.src) for constraint in constraints: - # Check scope: does this SCP apply to the edge's destination account? - if ou_account_map is not None and dst_account: + # Check scope: does this SCP apply to the edge's source/principal + # account? If the source account is unavailable, do not fall back + # to the target/resource account; that would incorrectly apply + # target-account SCPs to external callers. + if ou_account_map is not None: + if not src_account: + continue scope_accounts = ou_account_map.get(constraint.scope_id, set()) - if dst_account not in scope_accounts: + if src_account not in scope_accounts: continue binding = bind_scp_to_edge(edge, constraint) diff --git a/tests/test_scp_binder.py b/tests/test_scp_binder.py index c300171..8ae3aee 100644 --- a/tests/test_scp_binder.py +++ b/tests/test_scp_binder.py @@ -383,9 +383,9 @@ def test_global_binding_no_scope_map(self) -> None: assert len(bindings) == 1 def test_ou_scope_limits_binding(self) -> None: - """With ou_account_map, SCP only binds to edges in scope.""" - edge_in_scope = _make_edge(dst_id="arn:aws:iam::111111\u003111111:role/InScope") - edge_out_scope = _make_edge(dst_id="arn:aws:iam::999999\u003999999:role/OutOfScope") + """With ou_account_map, SCP only binds to source principals in scope.""" + edge_in_scope = _make_edge(src_id="arn:aws:iam::111111\u003111111:root") + edge_out_scope = _make_edge(src_id="arn:aws:iam::999999\u003999999:root") scp = _make_scp_constraint(deny_actions=["sts:AssumeRole"], scope_id="ou-prod") @@ -420,6 +420,112 @@ def test_output_sorted_by_composite_key(self) -> None: assert keys == sorted(keys) +class TestBindAllScpsSourceAccountScope: + """Regression coverage: SCP scope applies to the principal account, not target account.""" + + def _cross_account_edge(self) -> Edge: + return _make_edge( + src_id="arn:aws:iam::111111\u003111111:role/Caller", + src_type=NODE_TYPE_IAM_ROLE, + dst_id="arn:aws:iam::222222\u003222222:role/TargetRole", + ) + + def test_cross_account_source_scoped_deny_binds(self) -> None: + edge = self._cross_account_edge() + scp = _make_scp_constraint(deny_actions=["sts:AssumeRole"], scope_id="ou-source") + ou_map = {"ou-source": {"111111\u003111111"}} + + bindings = bind_all_scps([edge], [scp], ou_account_map=ou_map) + + assert len(bindings) == 1 + assert bindings[0].edge_id == edge.edge_id + assert bindings[0].constraint_id == scp.constraint_id + + def test_cross_account_target_scoped_deny_does_not_bind(self) -> None: + edge = self._cross_account_edge() + scp = _make_scp_constraint(deny_actions=["sts:AssumeRole"], scope_id="ou-target") + ou_map = {"ou-target": {"222222\u003222222"}} + + bindings = bind_all_scps([edge], [scp], ou_account_map=ou_map) + + assert bindings == [] + + def test_resource_matching_remains_target_based(self) -> None: + edge = self._cross_account_edge() + scp = _make_scp_constraint( + deny_actions=["sts:AssumeRole"], + resource_patterns=["arn:aws:iam::222222\u003222222:role/OtherRole"], + scope_id="ou-source", + ) + ou_map = {"ou-source": {"111111\u003111111"}} + + bindings = bind_all_scps([edge], [scp], ou_account_map=ou_map) + + assert bindings == [] + + def test_principal_applicability_still_uses_source_principal(self) -> None: + edge = self._cross_account_edge() + scp = _make_scp_constraint( + deny_actions=["sts:AssumeRole"], + applicable_principal_patterns=["arn:aws:iam::111111\u003111111:role/OtherCaller"], + scope_id="ou-source", + ) + ou_map = {"ou-source": {"111111\u003111111"}} + + bindings = bind_all_scps([edge], [scp], ou_account_map=ou_map) + + assert bindings == [] + + def test_same_account_behavior_still_binds(self) -> None: + edge = _make_edge( + src_id="arn:aws:iam::111111\u003111111:role/Caller", + src_type=NODE_TYPE_IAM_ROLE, + dst_id="arn:aws:iam::111111\u003111111:role/TargetRole", + ) + scp = _make_scp_constraint(deny_actions=["sts:AssumeRole"], scope_id="ou-same") + ou_map = {"ou-same": {"111111\u003111111"}} + + bindings = bind_all_scps([edge], [scp], ou_account_map=ou_map) + + assert len(bindings) == 1 + + def test_ou_map_uses_source_account_not_target_account(self) -> None: + edge = self._cross_account_edge() + source_scoped_scp = _make_scp_constraint( + deny_actions=["sts:AssumeRole"], + scope_id="ou-source", + policy_id="p-source", + statement_id="source", + ) + target_scoped_scp = _make_scp_constraint( + deny_actions=["sts:AssumeRole"], + scope_id="ou-target", + policy_id="p-target", + statement_id="target", + ) + ou_map = { + "ou-source": {"111111\u003111111"}, + "ou-target": {"222222\u003222222"}, + } + + bindings = bind_all_scps([edge], [target_scoped_scp, source_scoped_scp], ou_account_map=ou_map) + + assert len(bindings) == 1 + assert bindings[0].constraint_id == source_scoped_scp.constraint_id + + def test_unavailable_source_account_does_not_fall_back_to_target_account(self) -> None: + edge = _make_edge( + src_id="__synthetic__:external-principal-set", + dst_id="arn:aws:iam::222222\u003222222:role/TargetRole", + ) + scp = _make_scp_constraint(deny_actions=["sts:AssumeRole"], scope_id="ou-target") + ou_map = {"ou-target": {"222222\u003222222"}} + + bindings = bind_all_scps([edge], [scp], ou_account_map=ou_map) + + assert bindings == [] + + class TestBindingReason: """Tests for human-readable binding reason."""