Skip to content

sec(iac): scope Azure purchase role to onboarded subscription - #1658

Open
cristim wants to merge 6 commits into
mainfrom
sec/1545-arm-role-scope
Open

sec(iac): scope Azure purchase role to onboarded subscription#1658
cristim wants to merge 6 commits into
mainfrom
sec/1545-arm-role-scope

Conversation

@cristim

@cristim cristim commented Jul 28, 2026

Copy link
Copy Markdown
Member

Problem

arm/CUDly-CrossSubscription/template.json is deployed by customers into their own Azure tenants. Alongside the intended subscription-scope assignment it declared two grants that reached past the subscription being onboarded:

  1. A second roleAssignment with "scope": "/providers/Microsoft.Capacity" — as an absolute path that denotes tenant-wide reservation orders.
  2. /providers/Microsoft.Capacity in the custom role definition's assignableScopes — declaring the role eligible for assignment at tenant scope by anyone who can create role assignments there.

Verified live on current origin/main (887d51fd6), at template.json:64-67 and :86-100.

What the grant actually did

Worth recording, because it is narrower than the template reads. Running az deployment sub validate on the pre-fix template resolves the tenant assignment to:

/subscriptions/<subId>/providers/providers/Microsoft.Capacity/providers/Microsoft.Authorization/roleAssignments/<guid>

Note the doubled providers/providers. In a subscription-scoped deployment ARM appends the scope value beneath the subscription rather than treating it as absolute, so the likely apply-time outcome is a malformed target rather than a live tenant-wide grant. That has not been confirmed either way without applying to a real tenant, so this treats the grant as possibly live and tells operators to verify and revoke. Item (2) is a real widening regardless of how (1) resolved.

Before / after

Before After
assignableScopes /subscriptions/{id} + /providers/Microsoft.Capacity /subscriptions/{id} only
role assignments 3 subscription-scoped + 1 at /providers/Microsoft.Capacity 3 subscription-scoped

In words: the service principal was declared able to act on reservation orders across the whole Azure AD tenant, including subscriptions the customer never onboarded; it can now act only on the single subscription that az deployment sub create targeted.

Is a tenant-level grant genuinely required? No.

Evidence, all in-repo:

  • iac/federation/azure-target/terraform/main.tf:77-83 — the production customer onboarding path assigns only at /subscriptions/${local.subscription_id} and works.
  • terraform/modules/iam/azure/cudly-reservation-role/main.tf:58-71 — documents the tenant-root scope as deliberately removed, with include_capacity_provider_scope defaulting to false.
  • known-issues.md records the origin: the tenant assignment was a workaround for the built-in Reservation Reader role not existing in every tenant. The custom role added by Azure ARM template grants role missing purchase/calculatePrice actions -- all reservation purchases 403 #731 removed that need; the assignment was left behind.
  • Azure authorises a reservation purchase against the subscription named in the request body's billingScopeId, not against the tenant-level Microsoft.Capacity path.

So this was drift, not a requirement. Rather than adding an opt-in flag to the template, a tenant-wide grant is documented in known-issues.md as a separate, manual, explicitly consented az role assignment create step — never a default.

Fail-closed

There is no scope parameter at all. Every scope derives from subscription().subscriptionId, the deployment target itself. There is no caller-supplied scope string to omit, widen, or inject, so there is no validation to bypass, which is strictly stronger than validating a scope parameter. The guard now enforces exactly this, rather than merely checking the string looks subscription-shaped (see Regression guard).

Regression guard

scripts/check-azure-role-parity.sh compared only the actions lists, which is exactly why this drift stayed green in CI: both files agreed on actions while disagreeing on scope. It now also asserts scope.

Review of the first version of that guard found eight reproducible ways to reintroduce the tenant-wide grant while keeping it green. All are fixed in the second commit. This is a CI drift guard, not a security boundary (anyone who can edit the template can edit the script), so it is tuned against accidental reintroduction:

# Bypass Fix
1 /providers/microsoft.capacity lowercased — Azure namespaces are case-insensitive, bash =~ is not match under shopt -s nocasematch
2 assignment hidden in a nested Microsoft.Resources/deployments, or nested in a parent resource, or type in different casing recursive .. walk, ascii_downcase on type, and refuse nested deployments outright
3 variables.tf absent — the default-false assertion silently skipped referenced flag whose default cannot be read is now an error
4 [concat('/subscriptions/', parameters('otherSubscriptionId'))] — passes a substring anchor, grants in another subscription exact-match allowlist
5 $schema repointed at the management-group template, widening every scope-less assignment without touching a scope string $schema pinned to subscriptionDeploymentTemplate

Correcting a claim in the first version of this PR. It said the guard was "allowlist-anchored (accepted only if it visibly contains /subscriptions/)". That is what the code did, and it is much weaker than it reads: case 4 above satisfies it, in a template named CrossSubscription. The allowlist is now genuinely exact-match — only the canonical expression bound to the deployment target, or a bare literal /subscriptions/<guid> for fixtures — and role assignments must carry no explicit scope at all. The token denylist is retained as defence in depth, and the in-code comment no longer overstates what is enforced.

Self-tests go from 2 cases to 9, one per bypass. Each was checked to fail for its own reason rather than incidentally:

PASS: matching lists exit 0                     PASS: other-subscription ARM exits 1
PASS: drifted ARM exits 1                       PASS: nested-deployment ARM exits 1
PASS: tenant-scope ARM exits 1                  PASS: management-group schema ARM exits 1
PASS: obfuscated tenant-scope ARM exits 1       PASS: TF flag without variables.tf exits 1
PASS: lowercase tenant-scope ARM exits 1
Results: 9 passed, 0 failed.

No workflow file was touched: the existing azure-role-parity CI job already runs both scripts.

Regression guard, round 2

A second, independent adversarial review defeated the guard above three more ways, despite its 9-case suite passing and the guard passing on the real files:

# Bypass Fix
F1 A bare literal /subscriptions/<guid> was accepted by GUID shape alone (never by value), so a template hard-coding a foreign subscription passed — including in the deployable shape (canonical expression retained, foreign literal appended to the same array). nocasematch was also live across that regex, so an uppercase GUID passed too. Literals are no longer accepted at all, of any case. Only the canonical ARM expression, or its [subscription().id] equivalent, is accepted.
F3 The actions extractor matched only the top-level .resources[] array with a case-sensitive ==, while the scope walk was recursive and case-insensitive. A second role definition typed microsoft.authorization/roleDefinitions (lowercase) granting actions: ["*"] was invisible to the actions axis — the guard printed OK: ARM and TF actions lists match and correctly counted the extra role on the scope axis, proving the two axes disagreed about how many role definitions existed. Actions extraction now uses the same recursive, case-insensitive selector as the scope walk, unioned across every matched role definition.
F4 Only permissions[0].actions was compared. ARM unions every permissions[] entry, so a second entry appended after the canonical one (e.g. {"actions": ["*"]}) passed. notActions, dataActions, and notDataActions were never compared at all — a first entry with dataActions: ["*"] also passed. permissions[].actions[] is now flattened across the whole array, and notActions/dataActions/notDataActions are compared the same way actions is.
F2 The scope comparison was byte-exact, so whitespace, quote style (" vs '), a redundant empty-string concat argument, and the [subscription().id] equivalent all failed despite being semantically identical to the canonical expression. A pure reformat could red CI. Both sides are normalized (whitespace stripped, quote style unified, redundant empty-string arg dropped) before comparing; the accepted spellings are printed in the error message.
F5 Microsoft.Resources/deploymentScripts and Microsoft.Resources/deploymentStacks were not refused, unlike plain deployments — despite a deployment script's runtime az/az-cli commands being able to issue role assignments this check never sees as JSON. Both resource types are now refused alongside deployments.
F6 A role assignment's roleDefinitionId was unconstrained as long as it carried no explicit scope: a built-in Owner grant correctly inheriting the (subscription-scoped) deployment passed, since only the scope axis was checked. roleDefinitionId is now allowlisted to the three roles this template ever assigns (the custom purchaser, Reader, Cost Management Reader).

Correcting a claim in the previous round of this PR. The code comment above LITERAL_SUBSCRIPTION_RE (and this PR's own round-1 description, item 4's fix column) said a bare literal /subscriptions/<guid> was accepted "for the test fixtures". That justification was false and was disproved directly: the fixtures were rewritten to use the canonical expression instead, the literal-acceptance branch was deleted, and the full self-test suite still passes. The literal was never structurally required by anything; accepting it just weakened the guard to GUID shape, not GUID value — the exact hole F1 exploits.

Self-tests go from 9 cases to 18, one per bypass across both rounds (nine round-2 cases: two for F1, one for F2, one for F3, two for F4, two for F5, one for F6). Each was reproduced against the pre-fix revision (fc1f5b782) first — confirmed exit 0 (or, for F2, a false-positive exit 1) — and confirmed to close (correct exit code) after the fix:

Results: 18 passed, 0 failed.

shellcheck on both scripts: exit 0 (no findings). bash -n on both scripts: exit 0. Verified against the real arm/CUDly-CrossSubscription/template.json and terraform/modules/iam/azure/cudly-reservation-role/main.tf: still exit 0.

One item deliberately left alone: several malformed-input self-test variants exit 5 (a jq error surfacing through set -e) rather than 1. Still fail-closed, not a defect, and out of scope for this round — normalizing exit codes for that path was explicitly not attempted here.

Regression guard, self-review before round 3

Before handing round 2 to another reviewer, self-checked the two highest-risk things it added — a normalizer (which exists to make different strings equal) and a newly-recursive JSON walk (which exists to see more of the template than before). Found one real issue in each, both fixed here with their own self-test case:

  • normalize_scope_expr() blanket-stripped all whitespace, including inside the /subscriptions/ string literal itself, so [concat('/sub scriptions/', subscription().subscriptionId)] (a typo, not an attack) normalized to the same text as the real canonical scope and was wrongly accepted. Not exploitable for escalation — the corrupted literal fails to deploy rather than pointing anywhere else — but a normalizer that can't tell "reformatted" from "corrupted" is the wrong shape of tool. Whitespace is now collapsed only where it is immediately adjacent to structural punctuation ([ ] ( ) ,), which by construction cannot reach inside a string literal's content. Verified against every legitimate spelling from round 2 (still accepted) plus a battery of adversarial inputs — an extra trailing parameters('evil') argument after the empty-string arg, a managementGroup().id suffix, a foreign literal — none of which normalize to canonical.
  • The actions extractor, the nested-deployment refusal, and the scope walk all used an unrooted .. over the whole document. A decorative object under variables or outputs that merely happened to carry a roleDefinitions-shaped "type" field — never actually deployed — was treated as a real grant, reproduced directly: a variables.decorativeDocOnly object granting actions: ["*"] at /providers/Microsoft.Capacity caused an otherwise-clean, fully-matching template to fail CI. That can only make the guard fail closed on a correct template, never accept a bad one, but a guard that reds valid input is exactly the pressure that gets a guard deleted or bypassed — which is how the original tenant-wide grant went unnoticed for as long as it did. All three walks are now rooted at .resources, which still finds a resource nested inside another resource's own resources array without ever leaving the tree of things ARM actually deploys.

Also confirmed on request: ALLOWED_ROLE_DEFINITION_IDS's Reader and Cost Management Reader entries are not fixture-driven — the real template.json genuinely assigns both (lines 91 and 103, roleDefinitionId: [variables('roles').reader] / [variables('roles').costManagementReader]) alongside the custom purchaser role, matching exactly the "three roles CUDly assigns" the allowlist claims. The comparison is confirmed exact (a probe battery of prefix/suffix/substring variants of the allowed values all correctly fail); it recognizes only the one raw-expression spelling this template actually uses, so a reformat to a bare GUID or a resourceId()/concat() expression would be rejected rather than silently accepted — a drift-guard tradeoff already called out in the code comment, not a gap.

Self-tests go from 18 to 20. Both reproduced against the prior revision (99ea68759) first — confirmed exit 0 (space-inside-literal, wrongly accepted) and exit 1 (decorative-variables, wrongly rejected) respectively — and confirmed fixed after:

Results: 20 passed, 0 failed.

Regression guard, round 4 (independent review)

Independent review of round 3 (76abcc853) found 8 hostile templates that exit 0. Two fixes close six of them cheaply; the remaining two are filed as follow-up rather than fixed here.

# Bypass Fix
Fix A (4 findings) jq property-key access is case-sensitive; ARM's resource-provider JSON deserializers are documented case-insensitive. .type (a value) was already downcased for exactly this reason, but that reasoning never reached the property keys read from the same JSON. Confirmed live: a roleAssignment with "Scope" (capital S) set to /providers/Microsoft.Capacity — issue #1545 byte-for-byte apart from one capital letter — passed, along with "RoleDefinitionId" on an Owner grant, "Properties" wrapping an Owner grant, and "AssignableScopes" on a second role definition. Each made the node invisible to the check that key feeds — silence, not refusal. Every object key in the ARM document is lowercased once into a scratch copy before any query runs; every jq field access naming a non-lowercase ARM property (roleDefinitionId, assignableScopes, notActions, dataActions, notDataActions) rewritten to match.
Fix B (2 findings) Microsoft.Authorization/roleEligibilityScheduleRequests and roleAssignmentScheduleRequests (Azure PIM) genuinely grant a role the same way a plain roleAssignment does — confirmed by constructing one binding built-in Owner — but under a property shape the roleAssignments-only type match never saw, bypassing this PR's own roleDefinitionId allowlist by changing the resource type. Also added: the legacy Microsoft.Storage/storageAccounts/providers/roleAssignments child-type spelling, invisible for the same reason. All three types refused outright, alongside the existing nested-deployment/deploymentScripts/deploymentStacks refusal.
Fix C The code comment claimed this check "errs towards refusing anything it cannot reason about" — false for any resource type outside the two lists it recognizes/refuses (which is the entirety of the finding set above). Rewritten to say what the guard actually does: a fixed recognize-list plus a fixed refuse-list, silent on everything else.

Caveat recorded honestly, per reviewer request: whether ARM itself accepts a miscased property key is Azure-side behaviour that was not independently verified against a live subscription — that would require deploying a miscased template to a real tenant, which was not done. The resource-provider JSON deserializers are documented as case-insensitive by default, which is the basis for fixing this fail-closed regardless: if ARM does accept it, this closes a real hole; if not, the guard is merely redundant with a deploy-time rejection, never wrong.

Filed rather than fixed, per reviewer guidance (the template diff in this PR is the real #1545 remediation; these are separate defense-in-depth guard gaps and shouldn't hold up the fix): #1681, covering (1) the guard constrains which role and where, but never who — a roleAssignment binding the allowed custom role at the correct scope to a hardcoded foreign principalId passes cleanly, money-spending permission handed to an arbitrary principal; and (2) the systemic fix — assert the template's exact expected set of grant tuples (4, today) rather than enumerating everything to refuse, which would close every finding above and future ones in one stroke. Also noted there as low severity: assignableScopes given as a bare string exits 5 via a raw jq error rather than a clean 1 (still fails closed).

Self-tests go from 20 to 27 (seven round-4 cases: four for Fix A, three for Fix B). Each reproduced against the prior revision (76abcc853) first — confirmed exit 0 — and confirmed fixed after:

Results: 27 passed, 0 failed.

shellcheck on both scripts: exit 0. bash -n: exit 0. Verified against the real template and TF module: still exit 0.

Live-Azure confirmation (round 4's Fix A premise): an independent reviewer ran az deployment sub what-if (validate/what-if only, nothing deployed) against a template carrying "Scope" (capital S) on a roleAssignment. ARM resolved it to /providers/Microsoft.Capacity — the miscased key was honoured, not ignored, confirming this is a live Azure behaviour and not a theoretical jq artifact. "Properties" and "AssignableScopes" capitalized were also confirmed to produce plan output byte-identical to the correctly-cased control, meaning nothing warns anyone at plan time either. Fix A closes all of these (see below for confirmation the has() trap — downcasing only the extraction expressions while leaving has("scope")/has("roleDefinitionId") case-sensitive, which would look fixed while staying open — was avoided: the whole document is downcased once via walk() before any query, including the two has() calls, so there was never a partial-fix window).

Regression guard, round 5 (explicit case-folding)

Two comparisons were already case-insensitive in practice, but only as a side effect of running inside the shopt -s nocasematch scope set for the (unrelated, already-documented) ESCAPE_TOKENS regex match: the canonical-scope comparison in normalize_scope_expr, and the ALLOWED_ROLE_DEFINITION_IDS allowlist comparison. Confirmed empirically that this dependency was real (a probe with shopt -u nocasematch correctly stopped matching). That's fragile — moving either comparison outside that scope, or narrowing what nocasematch covers, would silently drop the case-insensitivity with no visible change to the comparison line itself.

Both now fold case explicitly instead: normalize_scope_expr lowercases as an explicit final step, and the roleDefinitionId loop lowercases both sides before comparing — matching the same "ARM identifiers are case-insensitive" reasoning already applied to resource type values via ascii_downcase elsewhere in the script. Safe unconditionally: lowercasing is a 1:1 character transform that can't collapse two distinct values into one, unlike the whitespace-stripping this same function already had to be careful about (round 3).

Self-tests go from 27 to 29: an uppercase-spelled canonical scope expression (CONCAT/SUBSCRIPTION/SUBSCRIPTIONID) and an uppercase-spelled allowed roleDefinitionId ([Variables('Roles').Reader]) are both confirmed still accepted (exit 0) — re-verified the full adversarial battery from round 3 (foreign literals, evil concat args, management-group escapes, whitespace-in-literal) still correctly rejected after adding the lowercasing step.

Results: 29 passed, 0 failed.

Sibling paths audited

Checked every path that generates, copies or embeds this grant. Only template.json was affected, so Closes is safe here:

Path Scope Status
iac/federation/azure-target/bicep/azure-wif.bicep + .arm.json subscription clean
iac/federation/azure-target/terraform/main.tf subscription clean
terraform/modules/compute/azure/container-apps/main.tf subscription clean
terraform/modules/iam/azure/cudly-reservation-role flag defaults false clean (now guarded)
internal/iacfiles/templates/azure-* no role grants clean
arm/CUDly-CrossSubscription/setup.sh deploys template, no extra grants clean
go:embed sets template not embedded n/a

Verification (exit codes)

Check Exit
az deployment sub validate (post-fix) 0, provisioningState: Succeeded, error: null — all 4 validated resources under /subscriptions/<subId>/
az deployment sub validate (pre-fix control) 0, but produced a 5th resource at the doubled-providers path
jq empty template.json 0
shellcheck (both scripts) 0
check-azure-role-parity.sh on fixed template 0
check-azure-role-parity.sh on pre-fix template 1 (fails on the real bug)
test-azure-role-parity.sh 0 — 4 passed, 0 failed

az bicep decompile on the pre-fix template also emitted BCP036: The property "scope" expected a value of type "resource | tenant" for the tenant assignment; that error is gone post-fix. Remaining BCP034 decompiler warnings on dependsOn are pre-existing and present in both.

Not verified: whether the pre-fix assignment ever resolved to a genuine tenant-scope grant at apply time. Establishing that requires actually applying the template to a live tenant, which was not done.

Action required for existing deployments

Customers who already deployed the earlier template are not fixed by this merge. ARM deployments are incremental, so removing the resource from the template does not revoke anything already created. They must:

  1. List assignments at or under /providers/Microsoft.Capacity for the CUDly SP, projecting the assignment id.
  2. Delete any found by --ids, before redeploying. Not by --scope: the CLI rejects the malformed doubled-providers shape as an invalid scope, which would leave an operator with a row that is listed but undeletable.
  3. Then redeploy the corrected template to narrow assignableScopes.

Order matters: Azure refuses to drop an assignable scope from a role definition while assignments still exist at it. Commands and ordering are in known-issues.md.

Severity

I'd rate this HIGH rather than CRITICAL, agreeing with the original reviewer. The escalation to CRITICAL assumed a silent tenant-wide grant; ARM in fact rewrites the scope subscription-relative.

Review strengthened this: az deployment sub validate does not preflight authorization, but az deployment sub what-if does, and it returns a hard AuthorizationFailed on exactly that resource over the malformed scope. So the doubled-providers path is not a validator artifact: ARM genuinely computes that target and preflight fails on it. Separately, per this repo's own variables.tf:12, declaring an assignable scope above your own subscription 403s for a subscription-scoped principal, so for a normal customer the pre-fix template most likely failed on its first resource. The realistic failure mode is "onboarding breaks", not "silent tenant-wide grant".

It stays high-priority: assignableScopes was genuinely widened, the outcome is unconfirmed for tenant-root-privileged deployers, and it is drift from a deliberately narrowed decision. The fix is identical under either rating. Flagging the judgement rather than relabelling the issue.

Follow-up filed: #1666 (unused roleAssignmentGuidPrefix parameter, pre-existing, deliberately kept out of this p0 diff).

Closes #1545

Summary by CodeRabbit

  • Bug Fixes

    • Removed unintended tenant-wide capacity permissions from Azure deployments.
    • Limited custom role assignments to the target subscription.
    • Prevented deployment paths that could create unauthorized broader-scope access.
  • Tests

    • Expanded validation for permission parity, deployment scope, nested resources, wildcard permissions, and scope-escape scenarios.
    • Added coverage for subscription and tenant-scope edge cases.
  • Documentation

    • Added remediation guidance for previously created tenant-wide grants.
    • Documented the deployment security issue and resolution.

The ARM cross-subscription onboarding template granted the CUDly service
principal more than the customer consented to. Alongside the intended
subscription-scope assignment it declared:

  * a second roleAssignment with "scope": "/providers/Microsoft.Capacity",
    an absolute path denoting tenant-wide reservation orders; and
  * "/providers/Microsoft.Capacity" in the custom role definition's
    assignableScopes, declaring the role eligible for assignment at tenant
    scope by anyone able to create role assignments there.

Customers deploy this template into their own Azure tenants, so a customer
onboarding one non-production subscription appeared to be granting access
across every subscription in the tenant, including ones never onboarded.

What the assignment actually produced is narrower than it reads. Running
`az deployment sub validate` on the pre-fix template resolves it to
/subscriptions/<subId>/providers/providers/Microsoft.Capacity/... , with a
doubled "providers" segment: in a subscription-scoped deployment ARM appends
the scope beneath the subscription instead of treating it as absolute. The
likely apply-time outcome is a malformed target rather than a live tenant
grant. That has not been confirmed either way without applying to a real
tenant, so the grant is treated as possibly live. The assignableScopes entry
is a real widening regardless of how the assignment resolved.

Both are removed. Every scope in the template now derives from
subscription().subscriptionId, the deployment target itself, so there is no
scope parameter a caller can supply or widen and no default that can fall
back to a broader scope. This matches the paths that were already correct:
iac/federation/azure-target/terraform assigns only at subscription scope, and
terraform/modules/iam/azure/cudly-reservation-role defaults
include_capacity_provider_scope to false, documenting the tenant-root scope as
deliberately removed. The ARM path never picked that decision up.

Purchases are unaffected. Azure authorises a reservation purchase against the
subscription named in the request body's billingScopeId, not against the
tenant-level Microsoft.Capacity path, which is why the Terraform onboarding
path has always worked on subscription scope alone. The tenant assignment
originated as a workaround for the built-in Reservation Reader role not
existing in every tenant; the custom role added by issue #731 removed that
need, but the assignment was left behind.

EXISTING DEPLOYMENTS REMAIN AFFECTED UNTIL THEY ACT. ARM deployments are
incremental, so deleting the resource from the template does not revoke
anything already created. Operators who deployed the earlier template must
list assignments scoped at or under /providers/Microsoft.Capacity, delete any
found, and only then redeploy. The order matters: Azure refuses to drop an
assignable scope from a role definition while assignments still exist at it.
known-issues.md carries the commands and the ordering.

Regression guard: check-azure-role-parity.sh previously compared only the
actions lists, which is why this drift stayed green in CI. It now also asserts
that every ARM grant scope is subscription-anchored, and that the Terraform
opt-in flag still defaults to false. The scope check is allowlist-anchored
(a value must visibly contain /subscriptions/) plus a denylist for tokens
above the subscription, so it rejects an expression that mentions
/subscriptions/ while still expanding to tenant scope, not just the blunt
literal. Two fixtures and two self-test cases cover both shapes; the check
fails on the pre-fix template and passes on the fixed one.

Verified: az deployment sub validate exit 0, provisioningState Succeeded,
all four validated resources under /subscriptions/<subId>/ (pre-fix produced
a fifth at the doubled-providers path); jq parse, shellcheck, parity check
and self-tests all exit 0.

Closes #1545
@cristim cristim added effort/s Hours impact/all-users Affects every user priority/p0 Drop everything; same-day fix severity/critical Major harm when it happens triaged Item has been triaged type/security Security finding urgency/now Drop other things labels Jul 28, 2026
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The ARM template now grants the custom role only at the target subscription. Documentation records remediation for prior tenant-wide grants, while the parity checker validates Terraform/ARM permissions, subscription-only scopes, deployment structure, and related escape scenarios through expanded regression fixtures.

Changes

Subscription Scope Security and Parity Validation

Layer / File(s) Summary
Remove tenant-capacity assignment
arm/CUDly-CrossSubscription/template.json, known-issues.md
Restricts the custom role to the deployment subscription, removes the tenant-capacity role assignment, and documents cleanup of earlier grants.
Expand permission parity extraction
scripts/check-azure-role-parity.sh
Compares normalized actions, not_actions, data_actions, and not_data_actions lists between Terraform and ARM.
Enforce deployment scope invariants
scripts/check-azure-role-parity.sh
Validates subscription deployment schemas, role scopes, role definition IDs, nested-resource escapes, and the Terraform capacity-scope default.
Cover scope escape regressions
scripts/test-azure-role-parity.sh, scripts/testdata/role-parity/*
Adds regression cases for tenant and foreign-subscription scopes, wildcard permissions, alternate canonical expressions, nested resources, and unallowed bindings.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ParityCheck
  participant Terraform
  participant ARM
  participant jq
  Terraform->>ParityCheck: Provide normalized permission lists
  ARM->>jq: Provide role definitions and assignments
  jq->>ParityCheck: Extract ARM permissions and scopes
  ParityCheck->>ParityCheck: Compare permissions and validate subscription scope
  ParityCheck-->>Terraform: Report parity result
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The parity-guard rewrite, expanded tests, and new test fixtures go beyond the linked issue's scope-fix request. Trim the PR to the subscription-scope fix or split the parity-guard hardening, tests, and fixtures into a separate change.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The ARM template now removes the tenant-scope role assignment and tenant-wide assignable scope, matching the issue's required subscription-only grant.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: narrowing the Azure purchase role scope to the onboarded subscription.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sec/1545-arm-role-scope

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added 3 commits July 28, 2026 21:53
Review of the #1545 guard found eight reproducible ways to reintroduce the
tenant-wide grant while keeping the check green. This is a CI drift guard
rather than a security boundary, since anyone able to edit the template can
edit the script, so the yardstick is accidental reintroduction. Fixed in that
order of value.

Case-insensitive matching. Azure provider namespaces are case-insensitive but
bash =~ is not, so "/providers/microsoft.capacity" is a fully functional
tenant scope that sailed through, and lowercasing alone defeated the existing
fixtures. Matching now runs under shopt -s nocasematch.

Recursive, case-insensitive resource extraction. The jq walk covered only the
top-level resources array and compared types exactly, so an assignment nested
in a parent resource, or typed in different casing, was invisible. It now
walks recursively with ascii_downcase on the type. A nested deployment is
refused outright: its inner template cannot be reasoned about here, and
assigning at a different scope from a subscription deployment is exactly what
a future author with a legitimate cross-scope need would reach for.

Exact-match allowlist. Accepting any value containing "/subscriptions/" was
much weaker than the comment claimed: "[concat('/subscriptions/',
parameters('otherSubscriptionId'))]" satisfied it while granting in a
subscription the customer never targeted, in a template named
CrossSubscription, as would an escape assembled in a variables block the
script never reads. Only the canonical expression bound to the deployment
target, or a bare literal /subscriptions/<guid> for the fixtures, is accepted
now. Role assignments must carry no explicit scope at all, which is the shape
the template actually needs. The token denylist stays as defence in depth and
the comment no longer overstates what is enforced.

Fail closed on a missing variables.tf. The default-false assertion was gated
on the file existing, so copying main.tf alone made the whole assertion vanish
silently. A referenced flag whose default cannot be read is now an error.

$schema pinned. Every assignment inherits the deployment scope, so repointing
$schema at the management-group template would land all of them at
management-group scope, covering every child subscription, without changing
one scope string. The invariant is now explicit rather than incidental.

Remediation runbook. Step 1 projected no id while step 2 deleted by
--scope /providers/Microsoft.Capacity, which the CLI rejects as an invalid
scope for the malformed doubled-providers shape, leaving an operator with a
row that is listed but undeletable. Step 1 now projects id and step 2 deletes
by --ids.

Self-tests go from 2 cases to 9, one per bypass, each asserted to fail for its
own reason rather than incidentally. Existing fixtures gain the $schema key
the pin now requires.

The template itself is unchanged: server-side review confirmed the narrowing
holds, with all four validated resources under /subscriptions/<id>/.

Refs #1545
Refs #1666
An independent adversarial review defeated the ARM/TF role parity guard
(scripts/check-azure-role-parity.sh) three ways despite its 9-case suite
passing:

- The scope allowlist accepted any literal /subscriptions/<guid> by GUID
  shape alone, never checking the value, so a foreign subscription
  hard-coded into the template passed. The "needed for the test fixtures"
  justification for that literal branch was false: the fixtures moved to
  the canonical scope expression and the suite still passes. Literals are
  no longer accepted at all.
- The actions extractor walked only the top-level resources array with a
  case-sensitive type match and only permissions[0], so a second role
  definition (typed case-differently) or a second permissions[] entry
  could grant unreviewed actions while the guard reported success.
  dataActions/notActions/notDataActions were never compared either.
- Nested Microsoft.Resources/deploymentScripts and deploymentStacks
  resources were not refused, unlike plain nested deployments, despite
  being able to issue role assignments the guard never sees as JSON.
- A role assignment's roleDefinitionId was unconstrained as long as it
  carried no explicit scope, letting a built-in Owner grant inherit the
  (correctly) subscription-scoped deployment.

Also fixes the scope comparison's byte-exact brittleness (whitespace,
quote style, and a redundant empty-string concat argument now normalize
to the same canonical form) and adds nine self-test cases, one per
bypass, each reproduced against the prior revision and confirmed closed.
Self-review of the round-2 fix (99ea687) surfaced two more issues before
an external reviewer could:

- normalize_scope_expr() blanket-stripped all whitespace, including inside
  the '/subscriptions/' string literal itself, so a typo like
  '/sub scriptions/' normalized to the same text as the real canonical
  scope. Not exploitable for escalation (the corrupted literal fails to
  deploy rather than resolving elsewhere), but a normalizer that can't
  distinguish "reformatted" from "corrupted" is the wrong shape of tool.
  Whitespace is now collapsed only where it is immediately adjacent to
  structural punctuation ([ ] ( ) ,), which by construction cannot reach
  inside a string literal's content.

- The actions extractor, the nested-deployment refusal, and the scope walk
  all used an unrooted `..` over the whole document, so a decorative object
  under `variables` or `outputs` that merely happened to carry a
  roleDefinitions-shaped "type" field -- never actually deployed -- was
  treated as a real grant. That can only make the guard fail closed on a
  correct template, not accept a bad one, but a guard that reds valid input
  is exactly the pressure that gets a guard deleted or bypassed, which is
  how the original tenant-wide grant went unnoticed. All three walks are
  now rooted at `.resources`, which still finds a resource nested inside
  another resource's own `resources` array without ever leaving the tree of
  things ARM actually deploys.

Two more self-test cases (19-20), one per fix, each reproduced against the
prior revision and confirmed closed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@known-issues.md`:
- Around line 8-17: Update the historical grant wording in known-issues.md at
lines 8-17 and 139-145 to remain conditional: state that existing deployments
may retain, or the assignment could have granted, tenant-wide reservation
access. Preserve the existing verify-and-revoke remediation guidance at both
sites.
- Around line 56-71: Update the cleanup instructions around `az role assignment
list` to query `/providers/Microsoft.Capacity` directly with `--scope`, so
tenant-level assignments are included. Remove the case-sensitive
`contains(scope, 'Microsoft.Capacity')` filter or replace it with an appropriate
path match. Preserve projecting assignment IDs and deleting listed assignments
via `az role assignment delete --ids` before redeployment.

In `@scripts/test-azure-role-parity.sh`:
- Around line 90-99: Update Case 9 in the test fixture setup to append an actual
Terraform HCL reference to include_capacity_provider_scope, rather than only a
comment. Ensure the generated main.tf contains configuration that the checker
parses as a reference while variables.tf remains absent, preserving the expected
exit status of 1.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f5cc167c-d78b-4cda-92eb-e458b22f6bf3

📥 Commits

Reviewing files that changed from the base of the PR and between 887d51f and 99ea687.

📒 Files selected for processing (21)
  • arm/CUDly-CrossSubscription/template.json
  • known-issues.md
  • scripts/check-azure-role-parity.sh
  • scripts/test-azure-role-parity.sh
  • scripts/testdata/role-parity/canonical-scope-variants-arm.json
  • scripts/testdata/role-parity/dataactions-wildcard-arm.json
  • scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json
  • scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json
  • scripts/testdata/role-parity/drifted-arm.json
  • scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json
  • scripts/testdata/role-parity/lowercase-tenant-scope-arm.json
  • scripts/testdata/role-parity/lowercase-type-wildcard-actions-arm.json
  • scripts/testdata/role-parity/matching-arm.json
  • scripts/testdata/role-parity/mgmt-group-schema-arm.json
  • scripts/testdata/role-parity/nested-deployment-arm.json
  • scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json
  • scripts/testdata/role-parity/other-subscription-arm.json
  • scripts/testdata/role-parity/second-permissions-entry-arm.json
  • scripts/testdata/role-parity/tenant-scope-arm.json
  • scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json
  • scripts/testdata/role-parity/uppercase-guid-literal-arm.json

Comment thread known-issues.md
Comment on lines +8 to +17
### Existing Azure deployments carry a tenant-wide reservation grant (issue #1545)

Until issue #1545 was fixed, `arm/CUDly-CrossSubscription/template.json`
declared two things that reached past the subscription being onboarded:

1. A **second** role assignment carrying `"scope": "/providers/Microsoft.Capacity"`,
in addition to the intended subscription-scope assignment. Written as an
absolute path, that scope denotes tenant-wide reservation orders: an
assignment there covers every reservation order in the Azure AD tenant,
including subscriptions the customer never onboarded.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep the historical grant status conditional. These statements contradict Lines 30-37, which correctly says the pre-fix assignment’s actual outcome is unconfirmed.

  • known-issues.md#L8-L17: say existing deployments may retain a tenant-wide reservation grant.
  • known-issues.md#L139-L145: say the assignment could have granted tenant-wide access, while retaining the verify-and-revoke remediation.
📍 Affects 1 file
  • known-issues.md#L8-L17 (this comment)
  • known-issues.md#L139-L145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@known-issues.md` around lines 8 - 17, Update the historical grant wording in
known-issues.md at lines 8-17 and 139-145 to remain conditional: state that
existing deployments may retain, or the assignment could have granted,
tenant-wide reservation access. Preserve the existing verify-and-revoke
remediation guidance at both sites.

Comment thread known-issues.md
Comment on lines +56 to +71
```bash
# 1. Check what the pre-fix template actually left behind, tenant-wide.
# Any row scoped at or under /providers/Microsoft.Capacity is over-broad.
# Project the assignment id: it is what step 2 deletes by.
az role assignment list \
--assignee <SP-object-id> \
--all \
--query "[?contains(scope, 'Microsoft.Capacity')].{id:id, scope:scope, role:roleDefinitionName}" \
-o table

# 2. Revoke anything step 1 listed, FIRST, before redeploying.
# Delete by --ids, not by --scope: the pre-fix template could produce the
# malformed doubled-providers scope shown above, and `az role assignment
# delete --scope /providers/Microsoft.Capacity` rejects that as an invalid
# scope, leaving the row listed but undeletable. The id always works.
az role assignment delete --ids <id-from-step-1> [<id> ...]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target file around the referenced lines.
git ls-files known-issues.md
wc -l known-issues.md
sed -n '1,180p' known-issues.md

# Pull concise evidence from Azure CLI docs if available in repo notes/links.
python3 - <<'PY'
from pathlib import Path
p = Path('known-issues.md')
text = p.read_text()
for needle in ['az role assignment list', 'Microsoft.Capacity', '--all', 'contains(scope']:
    print(f"\n=== {needle} ===")
    for i, line in enumerate(text.splitlines(), 1):
        if needle in line:
            print(f"{i}: {line}")
PY

Repository: LeanerCloud/CUDly

Length of output: 1920


🌐 Web query:

Azure CLI role assignment list --all current subscription scope Microsoft.Capacity tenant scope case-sensitive contains docs

💡 Result:

The Azure CLI command az role assignment list is used to list role assignments, with specific behaviors regarding the --all parameter and scope. By default, az role assignment list displays role assignments for the current subscription [1]. Using the --all parameter allows you to list role assignments for the current subscription and below [1]. Historically, --all and --scope have been mutually exclusive in the CLI, where --all defaults to the subscription scope [2][3]. Recent updates have introduced --at-scope to provide more flexibility in listing assignments at specific custom scopes without the limitations previously associated with --all [2]. Regarding the /providers/Microsoft.Capacity scope, this is a valid tenant-level scope used for managing Azure Reservations and related resources [4][5][6]. When using this or any other resource provider scope, it is critical to use the correct casing for the resource provider path [7]. While the underlying Azure Resource Manager (ARM) is generally case-insensitive for resource paths, the Azure CLI and other tools often treat these identifiers as case-sensitive [7][8][9]. Specifically, providing resource paths with incorrect casing (e.g., lowercase instead of PascalCase) can result in commands returning empty results [7][8]. In summary: - --all: Used to list assignments at the subscription level and below [1]. It is often incompatible with explicit --scope arguments in older versions [2][10][3]. - /providers/Microsoft.Capacity: A recognized tenant-level scope for Reservation management [5][6]. - Case-sensitivity: You must use the exact, correct PascalCase for resource provider identifiers (e.g., Microsoft.Capacity, Microsoft.Authorization) in the CLI to avoid empty results [7][8].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "noop"

Repository: LeanerCloud/CUDly

Length of output: 1920


🌐 Web query:

site:learn.microsoft.com az role assignment list --all current subscription role assignment scope provider path Microsoft.Capacity

💡 Result:

To list role assignments at the Microsoft.Capacity provider scope using the Azure CLI, you should use the --scope parameter with the full provider path [1][2]. The correct command is: az role assignment list --scope "/providers/Microsoft.Capacity" Explanation: 1. Scope parameter: In Azure RBAC, the --scope parameter defines the level at which the role assignments are listed [2]. For tenant-level resources or specific providers, you provide the full resource ID path [3]. 2. Microsoft.Capacity: This path represents the tenant-level scope used for reservation management (such as PTU reservations) [3]. 3. The --all flag: While the --all flag is often used with az role assignment list to include inherited assignments or those below the current subscription [2], it is not strictly required when you explicitly define a specific scope using --scope. Ensure your account has sufficient permissions (e.g., User Access Administrator or Global Administrator) to view role assignments at this tenant-level scope [3]. If you wish to filter the output further, you can combine this command with --query or --output json to format the results as needed [2].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the file section around the review target.
sed -n '1,180p' known-issues.md

Repository: LeanerCloud/CUDly

Length of output: 1920


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the exact neighborhood around lines 56-71 with line numbers.
nl -ba known-issues.md | sed -n '48,78p'

Repository: LeanerCloud/CUDly

Length of output: 1920


Query /providers/Microsoft.Capacity directly. az role assignment list --all only covers the current subscription and below, so it can miss a surviving tenant-level assignment; use --scope "/providers/Microsoft.Capacity" and avoid the case-sensitive contains() filter if you keep a path match.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@known-issues.md` around lines 56 - 71, Update the cleanup instructions around
`az role assignment list` to query `/providers/Microsoft.Capacity` directly with
`--scope`, so tenant-level assignments are included. Remove the case-sensitive
`contains(scope, 'Microsoft.Capacity')` filter or replace it with an appropriate
path match. Preserve projecting assignment IDs and deleting listed assignments
via `az role assignment delete --ids` before redeployment.

Comment on lines +90 to +99
# Case 9: the default-false assertion must fail CLOSED when the TF module
# references include_capacity_provider_scope but its variables.tf is missing,
# rather than silently skipping the assertion.
TMP_TF_DIR="$(mktemp -d)"
trap 'rm -rf "$TMP_TF_DIR"' EXIT
cp "${FIXTURES}/matching-tf.tf.fixture" "${TMP_TF_DIR}/main.tf"
printf '\n# assignable_scopes uses include_capacity_provider_scope\n' >> "${TMP_TF_DIR}/main.tf"
run_case "TF flag without variables.tf exits 1" 1 \
--tf-file "${TMP_TF_DIR}/main.tf" \
--arm-file "${FIXTURES}/matching-arm.json"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use a real Terraform reference for this regression.

Line 96 appends only a comment, so this test passes only if the checker treats comments as configuration. Add an actual include_capacity_provider_scope reference in the copied HCL fixture; otherwise it cannot detect a regression in Terraform parsing or comment handling.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/test-azure-role-parity.sh` around lines 90 - 99, Update Case 9 in the
test fixture setup to append an actual Terraform HCL reference to
include_capacity_provider_scope, rather than only a comment. Ensure the
generated main.tf contains configuration that the checker parses as a reference
while variables.tf remains absent, preserving the expected exit status of 1.

cristim added 2 commits July 29, 2026 11:39
Independent review defeated round-3 (76abcc8) eight ways. Two fixes close
six of them:

- jq property-key access is case-sensitive; ARM's resource-provider JSON
  deserializers are documented case-insensitive. `.type` (a value) was
  already downcased for exactly this reason, but that reasoning never
  reached the property KEYS read from the same JSON: has("scope"),
  has("roleDefinitionId"), .properties, .assignableScopes were all
  case-sensitive, so a miscased key made the node invisible -- silence, not
  refusal. Confirmed live: "Scope" on a tenant-wide roleAssignment,
  "RoleDefinitionId" on an Owner grant, "Properties" wrapping an Owner
  grant, and "AssignableScopes" all passed. The first is issue #1545
  byte-for-byte apart from one capital letter. Every object key in the ARM
  document is now lowercased once into a scratch copy before any query
  runs; every jq field access naming a non-lowercase ARM property
  (roleDefinitionId, assignableScopes, notActions, dataActions,
  notDataActions) is rewritten to match.

- Two more resource types genuinely grant a role the way a plain
  roleAssignment does but were never matched by any type selector:
  Microsoft.Authorization/roleEligibilityScheduleRequests and
  roleAssignmentScheduleRequests (Azure PIM), plus the legacy
  storageAccounts/providers/roleAssignments child-type spelling. All three
  are now refused outright, alongside the existing nested-deployment refusal.

Also corrects a comment that claimed this check "errs towards refusing
anything it cannot reason about" -- false for any resource type outside the
two lists it recognizes/refuses. The general fix (assert the template's
exact expected set of grants, closing every enumerate-what-to-refuse gap in
one stroke) plus a separate, unrelated finding (principalId is never
checked) are filed as issue #1681 rather than attempted here, per review
guidance that the template diff in this PR is the real #1545 remediation
and shouldn't wait on defense-in-depth guard hardening.

Seven more self-test cases (21-27), one per bypass, each reproduced against
the prior revision and confirmed closed.
Both the canonical-scope comparison (normalize_scope_expr) and the
ALLOWED_ROLE_DEFINITION_IDS allowlist comparison were already
case-insensitive in practice, but only as a side effect of running inside
the ambient `shopt -s nocasematch` scope set for the ESCAPE_TOKENS regex
match. That's fragile: moving either comparison outside that scope, or
changing what nocasematch covers, would silently drop the case-insensitivity
with no visible change to the comparison line itself.

ARM function names, property accessors, and resource-path segments are
documented case-insensitive -- the same reasoning already applied to
resource `type` values via ascii_downcase. Both comparisons now fold case
explicitly: normalize_scope_expr lowercases as its final step, and the
roleDefinitionId loop lowercases both sides before comparing. Safe
unconditionally, since lowercasing is a 1:1 character transform that can't
collapse two distinct values into one (unlike the whitespace-stripping this
function already had to be careful about).

Two more self-test cases (28-29) confirming an uppercase-spelled canonical
scope expression and an uppercase-spelled allowed roleDefinitionId are both
still accepted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/all-users Affects every user priority/p0 Drop everything; same-day fix severity/critical Major harm when it happens triaged Item has been triaged type/security Security finding urgency/now Drop other things

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sec(iac): ARM cross-subscription template assigns the purchase role at tenant Microsoft.Capacity scope

1 participant