From 13155846ad0755e89d625640ef92149515424faa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 21:30:30 +0200 Subject: [PATCH 01/10] sec(iac): scope Azure purchase role to onboarded subscription 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//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// (pre-fix produced a fifth at the doubled-providers path); jq parse, shellcheck, parity check and self-tests all exit 0. Closes #1545 --- arm/CUDly-CrossSubscription/template.json | 22 +-- known-issues.md | 97 ++++++++++++ scripts/check-azure-role-parity.sh | 146 ++++++++++++++++-- scripts/test-azure-role-parity.sh | 17 ++ .../obfuscated-tenant-scope-arm.json | 34 ++++ .../role-parity/tenant-scope-arm.json | 44 ++++++ 6 files changed, 324 insertions(+), 36 deletions(-) create mode 100644 scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json create mode 100644 scripts/testdata/role-parity/tenant-scope-arm.json diff --git a/arm/CUDly-CrossSubscription/template.json b/arm/CUDly-CrossSubscription/template.json index b7163cbd6..8cdbdb03d 100644 --- a/arm/CUDly-CrossSubscription/template.json +++ b/arm/CUDly-CrossSubscription/template.json @@ -3,7 +3,8 @@ "contentVersion": "1.0.0.0", "metadata": { - "description": "CUDly Cross-Subscription Role Assignments — deploy this in every target Azure subscription that CUDly should manage. It grants the CUDly service principal the permissions needed to query reservation recommendations and purchase Azure Reservations and Savings Plans." + "description": "CUDly Cross-Subscription Role Assignments — deploy this in every target Azure subscription that CUDly should manage. It grants the CUDly service principal the permissions needed to query reservation recommendations and purchase Azure Reservations and Savings Plans.", + "consentSurface": "Every grant in this template is confined to the single subscription targeted by 'az deployment sub create'. The scope is derived from subscription().subscriptionId, the deployment target itself, so there is no scope parameter a caller can widen, and deploying to one subscription can never grant access to another. The tenant-wide '/providers/Microsoft.Capacity' scope is deliberately NOT used: an assignment there covers every reservation order in the Azure AD tenant, including subscriptions the customer never onboarded. Purchases authorise against the subscription named in the request body's billingScopeId, so subscription scope is sufficient; this matches terraform/modules/iam/azure/cudly-reservation-role, whose include_capacity_provider_scope flag defaults to false for the same reason. See known-issues.md if a tenant-wide grant is ever genuinely required: it is a separate, manual, explicitly consented step, never a default." }, "parameters": { @@ -62,8 +63,7 @@ } ], "assignableScopes": [ - "[concat('/subscriptions/', subscription().subscriptionId)]", - "/providers/Microsoft.Capacity" + "[concat('/subscriptions/', subscription().subscriptionId)]" ] } }, @@ -83,22 +83,6 @@ } }, - { - "type": "Microsoft.Authorization/roleAssignments", - "apiVersion": "2022-04-01", - "scope": "/providers/Microsoft.Capacity", - "name": "[guid(parameters('servicePrincipalObjectId'), 'cudlyCustomRoleCapacity')]", - "dependsOn": [ - "[variables('customRoleDefinitionId')]" - ], - "properties": { - "roleDefinitionId": "[variables('customRoleDefinitionId')]", - "principalId": "[parameters('servicePrincipalObjectId')]", - "principalType": "ServicePrincipal", - "description": "CUDly — tenant-capacity-scope assignment of custom role; required for reservationOrders/purchase/action at /providers/Microsoft.Capacity scope" - } - }, - { "type": "Microsoft.Authorization/roleAssignments", "apiVersion": "2022-04-01", diff --git a/known-issues.md b/known-issues.md index df36aac8d..389557b45 100644 --- a/known-issues.md +++ b/known-issues.md @@ -5,6 +5,96 @@ action. Resolved items are moved to the Resolved section at the bottom. ## Outstanding +### 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. +2. `/providers/Microsoft.Capacity` in the custom role definition's + `assignableScopes`, which declares the role *eligible* to be assigned at + tenant scope by anyone who can create role assignments there. + +What (1) actually produced is worth recording, because it is not what the +template appears to say. Running `az deployment sub validate` against the +pre-fix template resolves that assignment to + +```text +/subscriptions//providers/providers/Microsoft.Capacity/providers/Microsoft.Authorization/roleAssignments/ +``` + +Note the doubled `providers/providers`. In a subscription-scoped deployment +ARM appends the `scope` value beneath the subscription rather than treating it +as an absolute tenant path, so the most likely apply-time outcome is a +malformed target that fails or lands somewhere meaningless, not a clean +tenant-wide grant. Whether it ever resolved to a real tenant-scope assignment +(for example when deployed by a principal holding tenant-root authority) has +not been established, and cannot be without applying the template to a live +tenant. + +Treat it as possibly live rather than assuming either way: **verify, then +revoke if present.** (2) is a real widening regardless of how (1) resolved. + +The template no longer creates that assignment, and `/providers/Microsoft.Capacity` +has been dropped from the role definition's `assignableScopes`. This matches +`terraform/modules/iam/azure/cudly-reservation-role`, whose +`include_capacity_provider_scope` flag has always defaulted to `false`, and +`iac/federation/azure-target/terraform`, which has only ever assigned at +subscription scope. + +**ARM deployments are incremental: removing the resource from the template does +not revoke anything it previously created.** Anyone who deployed the template +before this fix keeps whatever it granted until they delete it by hand. +Redeploying alone is not sufficient. + +Remediation, in this order: + +```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. +az role assignment list \ + --assignee \ + --all \ + --query "[?contains(scope, 'Microsoft.Capacity')].{scope:scope, role:roleDefinitionName}" \ + -o table + +# 2. Revoke anything step 1 listed, FIRST, before redeploying. +az role assignment delete \ + --assignee \ + --role "CUDly Reservation Purchaser (custom)" \ + --scope /providers/Microsoft.Capacity + +# 3. Then redeploy the corrected template to narrow assignableScopes. +az deployment sub create \ + --location eastus \ + --template-file arm/CUDly-CrossSubscription/template.json \ + --parameters servicePrincipalObjectId= \ + --name CUDly-CrossSubscription \ + --no-prompt +``` + +Step 1 returning nothing is a good outcome, and the expected one if the +malformed target described above simply failed to apply. Step 3 is still +required either way: it is what removes the tenant entry from +`assignableScopes`. + +The order matters: Azure refuses to remove an assignable scope from a role +definition while assignments still exist at that scope, so redeploying before +step 1 can fail on the role-definition update. + +Purchases are unaffected by the narrower grant. Azure authorises a reservation +purchase against the subscription named in the request body's `billingScopeId`, +not against the tenant-level `Microsoft.Capacity` provider path, which is why +the Terraform onboarding path has always worked with subscription scope alone. +If a deployment ever does need a tenant-wide grant, it must be applied manually +as an explicitly consented step (the `az role assignment create` mirror of +step 1) and must not be reintroduced into the template. +`scripts/check-azure-role-parity.sh` fails CI if it is. + ### Azure ARM template re-deployment required for purchase support (issue #731) The built-in "Reservation Purchaser" role (f7b75c60-3036-4b75-91c3-6b41c27c1689) @@ -44,6 +134,13 @@ will continue to return 403. applied the buggy template may need to clean up the orphaned subscription-scoped `Reservation Reader` assignment manually with `az role assignment delete --assignee --role "Reservation Reader" --scope /subscriptions/`. + **Superseded by issue #1545**: the `/providers/Microsoft.Capacity` + assignment described here was a workaround for `Reservation Reader` not + existing in every tenant. The custom role introduced by issue #731 removed + that need, but the tenant-wide assignment was left behind and silently + granted access across the whole tenant. It has since been removed; do not + reintroduce it. See the #1545 entry under Outstanding for the revocation + steps existing deployments still need. - **Azure ACS SMTP credential generation requires manual portal step**: Microsoft's API gap remains (no REST endpoint generates ACS SMTP diff --git a/scripts/check-azure-role-parity.sh b/scripts/check-azure-role-parity.sh index 3f4a242f4..d157bd8ac 100755 --- a/scripts/check-azure-role-parity.sh +++ b/scripts/check-azure-role-parity.sh @@ -1,14 +1,24 @@ #!/usr/bin/env bash # check-azure-role-parity.sh # -# Asserts that the Azure custom-role actions list is identical (case-insensitively) -# in both sources of truth: +# Asserts that the Azure custom role stays in parity across both sources of +# truth, on two axes: +# +# 1. ACTIONS: the permission list is identical (case-insensitively). +# 2. SCOPE: no grant escapes the subscription being onboarded. # # TF module : terraform/modules/iam/azure/cudly-reservation-role/main.tf # ARM template: arm/CUDly-CrossSubscription/template.json # -# Exit 0 = lists match. -# Exit 1 = lists differ; the diff is printed to stderr. +# The scope axis exists because the actions axis alone did not catch issue +# #1545: the ARM template assigned the (correct) actions at the tenant-wide +# "/providers/Microsoft.Capacity" scope, which covers every reservation order +# in the Azure AD tenant (including subscriptions the customer never +# onboarded), while the TF module deliberately granted subscription scope +# only. Both files agreed on actions, so the parity gate stayed green. +# +# Exit 0 = actions match and every scope is subscription-anchored. +# Exit 1 = drift; the offending values are printed to stderr. # # Usage: # scripts/check-azure-role-parity.sh [--tf-file ] [--arm-file ] @@ -96,18 +106,120 @@ fi DIFF=$(diff <(echo "$TF_ACTIONS") <(echo "$ARM_ACTIONS") || true) -if [[ -z "$DIFF" ]]; then - echo "OK: ARM and TF actions lists match (${#TF_ACTIONS} bytes, case-insensitive)." - exit 0 +if [[ -n "$DIFF" ]]; then + echo "ERROR: ARM template and TF module actions lists differ." >&2 + echo "" >&2 + echo " TF source : $TF_FILE" >&2 + echo " ARM source: $ARM_FILE" >&2 + echo "" >&2 + echo "Diff (< TF > ARM):" >&2 + echo "$DIFF" >&2 + echo "" >&2 + echo "Update the lagging file so both lists match." >&2 + exit 1 +fi + +echo "OK: ARM and TF actions lists match (${#TF_ACTIONS} bytes, case-insensitive)." + +# --- scope invariant (issue #1545) ------------------------------------------- +# Every scope the ARM template grants at (both the role definition's +# assignableScopes and any explicit `scope` on a role assignment) must stay +# inside the subscription that `az deployment sub create` targets. +# +# The rule is allowlist-anchored rather than a denylist of bad strings: a value +# is accepted only if it visibly contains "/subscriptions/", so anything that is +# not plainly subscription-anchored ("/", a management-group path, a bare +# tenant-level provider path) is rejected by default. The extra token denylist +# then catches escape scopes that would otherwise smuggle "/subscriptions/" +# past the anchor, and catches ARM expressions that assemble an escape scope +# piecewise (e.g. "[concat('/providers/', 'Microsoft.Capacity')]") instead of +# writing it as one literal. +# +# Escape tokens, all of which denote a scope ABOVE a single subscription: +# Microsoft.Capacity -> /providers/Microsoft.Capacity, tenant-wide +# reservation orders +# Microsoft.Management -> /providers/Microsoft.Management/managementGroups/* +# managementGroups -> same, matched independently of the provider spelling +# Microsoft.Billing -> /providers/Microsoft.Billing/billingAccounts/* +ESCAPE_TOKENS='Microsoft\.Capacity|Microsoft\.Management|managementGroups|Microsoft\.Billing' + +# Collect every scope value the template grants at, one per line, tagged with +# where it came from so the error message points at the right JSON node. +SCOPES=$( + jq -r ' + ( .resources[] + | select(.type == "Microsoft.Authorization/roleDefinitions") + | .properties.assignableScopes[] + | "assignableScopes\t" + . ), + ( .resources[] + | select(.type == "Microsoft.Authorization/roleAssignments") + | select(has("scope")) + | "roleAssignment.scope\t" + .scope ) + ' "$ARM_FILE" +) + +if [[ -z "$SCOPES" ]]; then + echo "ERROR: No assignableScopes found in ARM template: $ARM_FILE" >&2 + echo " Expected a Microsoft.Authorization/roleDefinitions resource with" >&2 + echo " a properties.assignableScopes array." >&2 + exit 1 +fi + +SCOPE_VIOLATIONS="" +while IFS=$'\t' read -r origin value; do + [[ -z "$origin" ]] && continue + reason="" + if [[ ! "$value" == */subscriptions/* ]]; then + reason="not anchored to /subscriptions/" + elif [[ "$value" =~ $ESCAPE_TOKENS ]]; then + reason="contains a scope token above the subscription" + fi + if [[ -n "$reason" ]]; then + SCOPE_VIOLATIONS+=" ${origin}: ${value}"$'\n'" -> ${reason}"$'\n' + fi +done <<< "$SCOPES" + +if [[ -n "$SCOPE_VIOLATIONS" ]]; then + echo "ERROR: ARM template grants outside the onboarded subscription." >&2 + echo "" >&2 + echo " ARM source: $ARM_FILE" >&2 + echo "" >&2 + printf '%s' "$SCOPE_VIOLATIONS" >&2 + echo "" >&2 + echo "Every scope must be derived from subscription().subscriptionId. A grant at" >&2 + echo "a tenant, management-group or billing-account scope reaches subscriptions" >&2 + echo "the customer never onboarded (issue #1545). If a wider grant is genuinely" >&2 + echo "required it must be a separate, manually applied, explicitly consented" >&2 + echo "step, never part of this template. See known-issues.md." >&2 + exit 1 +fi + +# The TF module offers include_capacity_provider_scope as an opt-in escape +# hatch. It must stay default-false, otherwise the TF path silently reacquires +# the tenant-wide grant this check removes from the ARM path. +if grep -q 'include_capacity_provider_scope' "$TF_FILE"; then + TF_VARS_FILE="$(dirname "$TF_FILE")/variables.tf" + if [[ -f "$TF_VARS_FILE" ]]; then + CAPACITY_DEFAULT=$( + awk ' + /^variable[[:space:]]+"include_capacity_provider_scope"/ { in_var=1 } + in_var && /^[[:space:]]*default[[:space:]]*=/ { + gsub(/^[[:space:]]*default[[:space:]]*=[[:space:]]*|[[:space:]]*$/, "") + print; exit + } + in_var && /^\}/ { exit } + ' "$TF_VARS_FILE" + ) + if [[ "$CAPACITY_DEFAULT" != "false" ]]; then + echo "ERROR: include_capacity_provider_scope must default to false." >&2 + echo " Found default: ${CAPACITY_DEFAULT:-} in $TF_VARS_FILE" >&2 + echo " A true default grants the tenant-wide /providers/Microsoft.Capacity" >&2 + echo " scope to every consumer of the module (issue #1545)." >&2 + exit 1 + fi + fi fi -echo "ERROR: ARM template and TF module actions lists differ." >&2 -echo "" >&2 -echo " TF source : $TF_FILE" >&2 -echo " ARM source: $ARM_FILE" >&2 -echo "" >&2 -echo "Diff (< TF > ARM):" >&2 -echo "$DIFF" >&2 -echo "" >&2 -echo "Update the lagging file so both lists match." >&2 -exit 1 +SCOPE_COUNT=$(echo "$SCOPES" | grep -c . || true) +echo "OK: all ${SCOPE_COUNT} ARM grant scopes are subscription-anchored." +exit 0 diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index 4d22df28f..847339aa6 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -40,6 +40,23 @@ run_case "drifted ARM exits 1" 1 \ --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ --arm-file "${FIXTURES}/drifted-arm.json" +# Case 3 (issue #1545): actions match, but the role is assignable at (and +# assigned at) the tenant-wide /providers/Microsoft.Capacity scope. This is +# the exact shape that shipped in arm/CUDly-CrossSubscription/template.json: +# the actions check passes, so only the scope check can catch it. +run_case "tenant-scope ARM exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/tenant-scope-arm.json" + +# Case 4 (issue #1545): the same tenant-wide escape, but written as an ARM +# expression that also mentions /subscriptions/. A check that merely required +# the scope to look subscription-anchored would admit this while rejecting the +# blunt literal in case 3, accepting the fail-OPEN form and blocking only the +# cosmetically-bad one. The token denylist is what must reject it. +run_case "obfuscated tenant-scope ARM exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/obfuscated-tenant-scope-arm.json" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json b/scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json new file mode 100644 index 000000000..9d7842d68 --- /dev/null +++ b/scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json @@ -0,0 +1,34 @@ +{ + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[if(equals(parameters('tenantWide'), 'yes'), '/providers/Microsoft.Capacity', concat('/subscriptions/', subscription().subscriptionId))]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/tenant-scope-arm.json b/scripts/testdata/role-parity/tenant-scope-arm.json new file mode 100644 index 000000000..af2d9edc5 --- /dev/null +++ b/scripts/testdata/role-parity/tenant-scope-arm.json @@ -0,0 +1,44 @@ +{ + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "/subscriptions/00000000-0000-0000-0000-000000000001", + "/providers/Microsoft.Capacity" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "scope": "/providers/Microsoft.Capacity", + "name": "tenant-wide", + "properties": { + "principalId": "00000000-0000-0000-0000-0000000000aa" + } + } + ] +} From 51a15f0d5ca52f92f79296da93188859d451722a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 21:53:16 +0200 Subject: [PATCH 02/10] sec(iac): close scope-guard bypasses in Azure role parity check 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/ 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//. Refs #1545 Refs #1666 --- known-issues.md | 12 +- scripts/check-azure-role-parity.sh | 168 +++++++++++++----- scripts/test-azure-role-parity.sh | 43 ++++- scripts/testdata/role-parity/drifted-arm.json | 5 +- .../lowercase-tenant-scope-arm.json | 35 ++++ .../testdata/role-parity/matching-arm.json | 5 +- .../role-parity/mgmt-group-schema-arm.json | 35 ++++ .../role-parity/nested-deployment-arm.json | 58 ++++++ .../obfuscated-tenant-scope-arm.json | 1 + .../role-parity/other-subscription-arm.json | 35 ++++ .../role-parity/tenant-scope-arm.json | 1 + 11 files changed, 345 insertions(+), 53 deletions(-) create mode 100644 scripts/testdata/role-parity/lowercase-tenant-scope-arm.json create mode 100644 scripts/testdata/role-parity/mgmt-group-schema-arm.json create mode 100644 scripts/testdata/role-parity/nested-deployment-arm.json create mode 100644 scripts/testdata/role-parity/other-subscription-arm.json diff --git a/known-issues.md b/known-issues.md index 389557b45..04331fc8f 100644 --- a/known-issues.md +++ b/known-issues.md @@ -56,17 +56,19 @@ Remediation, in this order: ```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 \ --all \ - --query "[?contains(scope, 'Microsoft.Capacity')].{scope:scope, role:roleDefinitionName}" \ + --query "[?contains(scope, 'Microsoft.Capacity')].{id:id, scope:scope, role:roleDefinitionName}" \ -o table # 2. Revoke anything step 1 listed, FIRST, before redeploying. -az role assignment delete \ - --assignee \ - --role "CUDly Reservation Purchaser (custom)" \ - --scope /providers/Microsoft.Capacity +# 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 [ ...] # 3. Then redeploy the corrected template to narrow assignableScopes. az deployment sub create \ diff --git a/scripts/check-azure-role-parity.sh b/scripts/check-azure-role-parity.sh index d157bd8ac..d1b7b0876 100755 --- a/scripts/check-azure-role-parity.sh +++ b/scripts/check-azure-role-parity.sh @@ -122,18 +122,31 @@ fi echo "OK: ARM and TF actions lists match (${#TF_ACTIONS} bytes, case-insensitive)." # --- scope invariant (issue #1545) ------------------------------------------- -# Every scope the ARM template grants at (both the role definition's -# assignableScopes and any explicit `scope` on a role assignment) must stay -# inside the subscription that `az deployment sub create` targets. +# Every scope the ARM template grants at must stay inside the subscription that +# `az deployment sub create` targets. # -# The rule is allowlist-anchored rather than a denylist of bad strings: a value -# is accepted only if it visibly contains "/subscriptions/", so anything that is -# not plainly subscription-anchored ("/", a management-group path, a bare -# tenant-level provider path) is rejected by default. The extra token denylist -# then catches escape scopes that would otherwise smuggle "/subscriptions/" -# past the anchor, and catches ARM expressions that assemble an escape scope -# piecewise (e.g. "[concat('/providers/', 'Microsoft.Capacity')]") instead of -# writing it as one literal. +# This is a CI drift guard, not a security boundary: anyone who can edit the +# template can edit this script. It exists to stop the grant being widened by +# ACCIDENT, so it is tuned to catch the shapes a well-meaning author actually +# reaches for, and it errs towards refusing anything it cannot reason about. +# +# The allowlist is EXACT-MATCH, not substring-anchored. An earlier revision +# accepted any value containing "/subscriptions/", which is far weaker than it +# reads: "[concat('/subscriptions/', parameters('otherSubscriptionId'))]" +# satisfies it while granting in a subscription the customer never targeted, +# in a template named CrossSubscription. Only two shapes are accepted: +# +# 1. the canonical ARM expression binding the scope to the deployment target +# 2. a bare literal /subscriptions/, used by the test fixtures +# +# Anything else is rejected, including an escape assembled from a `variables` +# block that this script never reads. +CANONICAL_SCOPE_EXPR="[concat('/subscriptions/', subscription().subscriptionId)]" +LITERAL_SUBSCRIPTION_RE='^/subscriptions/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' + +# Retained as defence in depth. The exact-match allowlist above already rejects +# every one of these, but they name the specific scopes that motivated the +# check, and they keep failing loudly if the allowlist is ever loosened. # # Escape tokens, all of which denote a scope ABOVE a single subscription: # Microsoft.Capacity -> /providers/Microsoft.Capacity, tenant-wide @@ -143,18 +156,57 @@ echo "OK: ARM and TF actions lists match (${#TF_ACTIONS} bytes, case-insensitive # Microsoft.Billing -> /providers/Microsoft.Billing/billingAccounts/* ESCAPE_TOKENS='Microsoft\.Capacity|Microsoft\.Management|managementGroups|Microsoft\.Billing' +# The deployment scope is an invariant of this template, not a detail: the +# three role assignments carry no `scope` property and therefore inherit it. +# Repointing $schema at the management-group template would silently land all +# of them at management-group scope, covering every child subscription, without +# changing a single scope string. Pin it explicitly. +if ! jq -e '.["$schema"] | test("subscriptionDeploymentTemplate")' "$ARM_FILE" >/dev/null 2>&1; then + ACTUAL_SCHEMA=$(jq -r '.["$schema"] // ""' "$ARM_FILE") + echo "ERROR: ARM template is not a subscription-scoped deployment." >&2 + echo " \$schema: ${ACTUAL_SCHEMA}" >&2 + echo " Role assignments here carry no explicit scope, so they inherit the" >&2 + echo " deployment scope. A management-group or tenant schema would widen" >&2 + echo " every one of them without changing a scope string (issue #1545)." >&2 + exit 1 +fi + +# A nested deployment can carry an inner template with its own role +# assignments, at its own scope. That is the idiomatic ARM way to assign at a +# different scope from a subscription deployment, so it is exactly what a +# future author with a legitimate cross-scope need would reach for. This script +# cannot reason about inner templates, so refuse rather than pass them silently. +NESTED_COUNT=$( + jq '[.. | objects | select(has("type")) | select((.type|type) == "string") + | select((.type|ascii_downcase) == "microsoft.resources/deployments")] | length' "$ARM_FILE" +) +if [[ "$NESTED_COUNT" != "0" ]]; then + echo "ERROR: ARM template contains ${NESTED_COUNT} nested deployment(s)." >&2 + echo " This check cannot inspect the scopes inside a nested template, so a" >&2 + echo " tenant-scoped assignment could hide there (issue #1545). Either" >&2 + echo " inline the resources, or extend this script to recurse into" >&2 + echo " properties.template before adding one." >&2 + exit 1 +fi + # Collect every scope value the template grants at, one per line, tagged with # where it came from so the error message points at the right JSON node. +# +# The walk is recursive (`..`) rather than over the top-level `resources` array +# only, so an assignment nested inside a parent resource's own `resources` +# array is still seen. Type matching is case-insensitive because ARM resource +# types are, while jq's `==` is not. SCOPES=$( jq -r ' - ( .resources[] - | select(.type == "Microsoft.Authorization/roleDefinitions") - | .properties.assignableScopes[] - | "assignableScopes\t" + . ), - ( .resources[] - | select(.type == "Microsoft.Authorization/roleAssignments") - | select(has("scope")) - | "roleAssignment.scope\t" + .scope ) + [.. | objects | select(has("type")) | select((.type|type) == "string")] as $all + | ( $all[] + | select((.type|ascii_downcase) == "microsoft.authorization/roledefinitions") + | (.properties.assignableScopes // [])[] + | "assignableScopes\t" + . ), + ( $all[] + | select((.type|ascii_downcase) == "microsoft.authorization/roleassignments") + | select(has("scope")) + | "roleAssignment.scope\t" + (.scope|tostring) ) ' "$ARM_FILE" ) @@ -165,19 +217,33 @@ if [[ -z "$SCOPES" ]]; then exit 1 fi +# Azure provider namespaces and ARM function names are case-insensitive, so +# "/providers/microsoft.capacity" is a fully functional tenant scope. Match +# case-insensitively or lowercasing alone would defeat every check below. SCOPE_VIOLATIONS="" +shopt -s nocasematch while IFS=$'\t' read -r origin value; do [[ -z "$origin" ]] && continue reason="" - if [[ ! "$value" == */subscriptions/* ]]; then - reason="not anchored to /subscriptions/" + if [[ "$origin" == "roleAssignment.scope" ]]; then + # Every assignment in this template inherits the deployment scope. An + # explicit `scope` is how #1545 shipped, and there is no legitimate use + # for one here. + reason="role assignments must inherit the deployment scope, not set one" + elif [[ "$value" == "$CANONICAL_SCOPE_EXPR" ]]; then + : + elif [[ "$value" =~ $LITERAL_SUBSCRIPTION_RE ]]; then + : elif [[ "$value" =~ $ESCAPE_TOKENS ]]; then - reason="contains a scope token above the subscription" + reason="names a scope above the subscription" + else + reason="not the canonical subscription scope" fi if [[ -n "$reason" ]]; then SCOPE_VIOLATIONS+=" ${origin}: ${value}"$'\n'" -> ${reason}"$'\n' fi done <<< "$SCOPES" +shopt -u nocasematch if [[ -n "$SCOPE_VIOLATIONS" ]]; then echo "ERROR: ARM template grants outside the onboarded subscription." >&2 @@ -186,37 +252,49 @@ if [[ -n "$SCOPE_VIOLATIONS" ]]; then echo "" >&2 printf '%s' "$SCOPE_VIOLATIONS" >&2 echo "" >&2 - echo "Every scope must be derived from subscription().subscriptionId. A grant at" >&2 - echo "a tenant, management-group or billing-account scope reaches subscriptions" >&2 - echo "the customer never onboarded (issue #1545). If a wider grant is genuinely" >&2 - echo "required it must be a separate, manually applied, explicitly consented" >&2 - echo "step, never part of this template. See known-issues.md." >&2 + echo "assignableScopes must be exactly:" >&2 + echo " ${CANONICAL_SCOPE_EXPR}" >&2 + echo "and role assignments must carry no explicit scope. A grant at a tenant," >&2 + echo "management-group or billing-account scope, or at another subscription," >&2 + echo "reaches subscriptions the customer never onboarded (issue #1545). If a" >&2 + echo "wider grant is genuinely required it must be a separate, manually" >&2 + echo "applied, explicitly consented step, never part of this template." >&2 + echo "See known-issues.md." >&2 exit 1 fi # The TF module offers include_capacity_provider_scope as an opt-in escape # hatch. It must stay default-false, otherwise the TF path silently reacquires # the tenant-wide grant this check removes from the ARM path. +# +# Fails closed: if the flag is referenced but its default cannot be read, that +# is a failure, not a skip. Gating this on `-f variables.tf` previously let the +# whole assertion vanish when the variable moved or the file was absent. if grep -q 'include_capacity_provider_scope' "$TF_FILE"; then TF_VARS_FILE="$(dirname "$TF_FILE")/variables.tf" - if [[ -f "$TF_VARS_FILE" ]]; then - CAPACITY_DEFAULT=$( - awk ' - /^variable[[:space:]]+"include_capacity_provider_scope"/ { in_var=1 } - in_var && /^[[:space:]]*default[[:space:]]*=/ { - gsub(/^[[:space:]]*default[[:space:]]*=[[:space:]]*|[[:space:]]*$/, "") - print; exit - } - in_var && /^\}/ { exit } - ' "$TF_VARS_FILE" - ) - if [[ "$CAPACITY_DEFAULT" != "false" ]]; then - echo "ERROR: include_capacity_provider_scope must default to false." >&2 - echo " Found default: ${CAPACITY_DEFAULT:-} in $TF_VARS_FILE" >&2 - echo " A true default grants the tenant-wide /providers/Microsoft.Capacity" >&2 - echo " scope to every consumer of the module (issue #1545)." >&2 - exit 1 - fi + if [[ ! -f "$TF_VARS_FILE" ]]; then + echo "ERROR: $TF_FILE references include_capacity_provider_scope but" >&2 + echo " $TF_VARS_FILE does not exist, so its default cannot be checked." >&2 + echo " Point this check at wherever the variable now lives; do not let" >&2 + echo " the default-false assertion silently disappear (issue #1545)." >&2 + exit 1 + fi + CAPACITY_DEFAULT=$( + awk ' + /^variable[[:space:]]+"include_capacity_provider_scope"/ { in_var=1 } + in_var && /^[[:space:]]*default[[:space:]]*=/ { + gsub(/^[[:space:]]*default[[:space:]]*=[[:space:]]*|[[:space:]]*$/, "") + print; exit + } + in_var && /^\}/ { exit } + ' "$TF_VARS_FILE" + ) + if [[ "$CAPACITY_DEFAULT" != "false" ]]; then + echo "ERROR: include_capacity_provider_scope must default to false." >&2 + echo " Found default: ${CAPACITY_DEFAULT:-} in $TF_VARS_FILE" >&2 + echo " A true default grants the tenant-wide /providers/Microsoft.Capacity" >&2 + echo " scope to every consumer of the module (issue #1545)." >&2 + exit 1 fi fi diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index 847339aa6..2c9c69f93 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -52,11 +52,52 @@ run_case "tenant-scope ARM exits 1" 1 \ # expression that also mentions /subscriptions/. A check that merely required # the scope to look subscription-anchored would admit this while rejecting the # blunt literal in case 3, accepting the fail-OPEN form and blocking only the -# cosmetically-bad one. The token denylist is what must reject it. +# cosmetically-bad one. run_case "obfuscated tenant-scope ARM exits 1" 1 \ --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ --arm-file "${FIXTURES}/obfuscated-tenant-scope-arm.json" +# Case 5: the same tenant scope in lowercase. Azure provider namespaces are +# case-insensitive, so /providers/microsoft.capacity is a fully functional +# tenant scope; a case-sensitive check would pass it while failing case 3. +run_case "lowercase tenant-scope ARM exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/lowercase-tenant-scope-arm.json" + +# Case 6: a scope pointing at a DIFFERENT subscription via a parameter. It +# contains /subscriptions/ and names no escape provider, so only an exact-match +# allowlist rejects it. This is the cross-subscription grant the template's own +# name invites. +run_case "other-subscription ARM exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/other-subscription-arm.json" + +# Case 7: a tenant-scoped assignment hidden inside a nested deployment. This is +# the idiomatic ARM way to assign at a scope other than the deployment's own, +# so it is what an author with a legitimate cross-scope need would reach for. +# A check that walked only the top-level resources array would not see it. +run_case "nested-deployment ARM exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/nested-deployment-arm.json" + +# Case 8: repointing $schema at the management-group template. Every role +# assignment here inherits the deployment scope, so this widens all of them to +# cover every child subscription without altering one scope string. +run_case "management-group schema ARM exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/mgmt-group-schema-arm.json" + +# 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" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/role-parity/drifted-arm.json b/scripts/testdata/role-parity/drifted-arm.json index 2724190cb..cdcd06580 100644 --- a/scripts/testdata/role-parity/drifted-arm.json +++ b/scripts/testdata/role-parity/drifted-arm.json @@ -1,4 +1,5 @@ { + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", "resources": [ { "type": "Microsoft.Authorization/roleDefinitions", @@ -24,7 +25,9 @@ "notActions": [] } ], - "assignableScopes": ["/subscriptions/00000000-0000-0000-0000-000000000001"] + "assignableScopes": [ + "/subscriptions/00000000-0000-0000-0000-000000000001" + ] } } ] diff --git a/scripts/testdata/role-parity/lowercase-tenant-scope-arm.json b/scripts/testdata/role-parity/lowercase-tenant-scope-arm.json new file mode 100644 index 000000000..34884bdbd --- /dev/null +++ b/scripts/testdata/role-parity/lowercase-tenant-scope-arm.json @@ -0,0 +1,35 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "/providers/microsoft.capacity" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/matching-arm.json b/scripts/testdata/role-parity/matching-arm.json index b3ff3fecf..9767aad9b 100644 --- a/scripts/testdata/role-parity/matching-arm.json +++ b/scripts/testdata/role-parity/matching-arm.json @@ -1,4 +1,5 @@ { + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", "resources": [ { "type": "Microsoft.Authorization/roleDefinitions", @@ -25,7 +26,9 @@ "notActions": [] } ], - "assignableScopes": ["/subscriptions/00000000-0000-0000-0000-000000000001"] + "assignableScopes": [ + "/subscriptions/00000000-0000-0000-0000-000000000001" + ] } } ] diff --git a/scripts/testdata/role-parity/mgmt-group-schema-arm.json b/scripts/testdata/role-parity/mgmt-group-schema-arm.json new file mode 100644 index 000000000..c91eb6d8d --- /dev/null +++ b/scripts/testdata/role-parity/mgmt-group-schema-arm.json @@ -0,0 +1,35 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2019-08-01/managementGroupDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "/subscriptions/00000000-0000-0000-0000-000000000001" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/nested-deployment-arm.json b/scripts/testdata/role-parity/nested-deployment-arm.json new file mode 100644 index 000000000..4fd6f4ae5 --- /dev/null +++ b/scripts/testdata/role-parity/nested-deployment-arm.json @@ -0,0 +1,58 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "/subscriptions/00000000-0000-0000-0000-000000000001" + ] + } + }, + { + "type": "Microsoft.Resources/deployments", + "apiVersion": "2022-09-01", + "name": "nested-scope-escape", + "properties": { + "mode": "Incremental", + "template": { + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "contentVersion": "1.0.0.0", + "resources": [ + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "scope": "/providers/Microsoft.Capacity", + "name": "hidden-tenant-grant", + "properties": { + "principalId": "00000000-0000-0000-0000-0000000000aa" + } + } + ] + } + } + } + ] +} diff --git a/scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json b/scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json index 9d7842d68..96c952326 100644 --- a/scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json +++ b/scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json @@ -1,4 +1,5 @@ { + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", "resources": [ { "type": "Microsoft.Authorization/roleDefinitions", diff --git a/scripts/testdata/role-parity/other-subscription-arm.json b/scripts/testdata/role-parity/other-subscription-arm.json new file mode 100644 index 000000000..96d5c9ece --- /dev/null +++ b/scripts/testdata/role-parity/other-subscription-arm.json @@ -0,0 +1,35 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', parameters('otherSubscriptionId'))]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/tenant-scope-arm.json b/scripts/testdata/role-parity/tenant-scope-arm.json index af2d9edc5..aabccaf64 100644 --- a/scripts/testdata/role-parity/tenant-scope-arm.json +++ b/scripts/testdata/role-parity/tenant-scope-arm.json @@ -1,4 +1,5 @@ { + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", "resources": [ { "type": "Microsoft.Authorization/roleDefinitions", From 3fcb92ab39f5b2500c424b9107493ba15e0102af Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:10:26 +0200 Subject: [PATCH 03/10] sec(iac): close remaining Azure role parity guard bypasses 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/ 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. --- scripts/check-azure-role-parity.sh | 259 +++++++++++++----- scripts/test-azure-role-parity.sh | 74 +++++ .../canonical-scope-variants-arm.json | 38 +++ .../role-parity/dataactions-wildcard-arm.json | 39 +++ .../deploymentscript-scope-escape-arm.json | 47 ++++ .../deploymentstack-scope-escape-arm.json | 49 ++++ ...bscription-literal-with-canonical-arm.json | 36 +++ .../lowercase-type-wildcard-actions-arm.json | 54 ++++ .../testdata/role-parity/matching-arm.json | 2 +- .../second-permissions-entry-arm.json | 40 +++ .../unallowed-roledefinitionid-arm.json | 45 +++ .../uppercase-guid-literal-arm.json | 35 +++ 12 files changed, 642 insertions(+), 76 deletions(-) create mode 100644 scripts/testdata/role-parity/canonical-scope-variants-arm.json create mode 100644 scripts/testdata/role-parity/dataactions-wildcard-arm.json create mode 100644 scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json create mode 100644 scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json create mode 100644 scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json create mode 100644 scripts/testdata/role-parity/lowercase-type-wildcard-actions-arm.json create mode 100644 scripts/testdata/role-parity/second-permissions-entry-arm.json create mode 100644 scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json create mode 100644 scripts/testdata/role-parity/uppercase-guid-literal-arm.json diff --git a/scripts/check-azure-role-parity.sh b/scripts/check-azure-role-parity.sh index d1b7b0876..683eab161 100755 --- a/scripts/check-azure-role-parity.sh +++ b/scripts/check-azure-role-parity.sh @@ -4,7 +4,8 @@ # Asserts that the Azure custom role stays in parity across both sources of # truth, on two axes: # -# 1. ACTIONS: the permission list is identical (case-insensitively). +# 1. ACTIONS: the permission lists (actions, notActions, dataActions, +# notDataActions) are identical (case-insensitively). # 2. SCOPE: no grant escapes the subscription being onboarded. # # TF module : terraform/modules/iam/azure/cudly-reservation-role/main.tf @@ -55,22 +56,39 @@ if [[ ! -f "$ARM_FILE" ]]; then exit 1 fi -# --- extract actions from TF ------------------------------------------------- -# Match lines inside the `actions = [ ... ]` block of the azurerm_role_definition -# resource and extract the quoted string values. +if ! command -v jq &>/dev/null; then + echo "ERROR: jq is required but not installed." >&2 + exit 2 +fi -TF_ACTIONS=$( - awk ' - /^[[:space:]]*permissions[[:space:]]*\{/ { in_perms=1 } - in_perms && /^[[:space:]]*actions[[:space:]]*=/ { in_actions=1; next } - in_actions && /^[[:space:]]*\]/ { in_actions=0; in_perms=0; next } - in_actions { - # Strip leading/trailing whitespace, quotes, and trailing commas. +# --- extract permission lists from TF ----------------------------------------- +# Pulls each of actions / not_actions / data_actions / not_data_actions out of +# the `permissions { ... }` block of the azurerm_role_definition resource. +# Handles both the multi-line `attr = [ ... ]` form and the single-line +# `attr = []` empty form. `in_perms` stays set for the whole permissions block +# (reset only on the block's own closing brace), not on the first list's +# closing bracket, so an attribute appearing after `actions` in the same block +# is still seen. An attribute that never appears extracts as empty, which +# matches the azurerm provider's own default for data_actions/not_data_actions. +extract_tf_list() { + local attr="$1" file="$2" + awk -v attr="$attr" ' + /^[[:space:]]*permissions[[:space:]]*\{/ { in_perms=1; next } + in_perms && $0 ~ ("^[[:space:]]*" attr "[[:space:]]*=[[:space:]]*\\[[[:space:]]*\\][[:space:]]*$") { next } + in_perms && $0 ~ ("^[[:space:]]*" attr "[[:space:]]*=") { in_list=1; next } + in_list && /^[[:space:]]*\]/ { in_list=0; next } + in_list { gsub(/^[[:space:]"]+|[",[:space:]]+$/, "") if (length($0) > 0) print tolower($0) } - ' "$TF_FILE" | sort -) + in_perms && /^[[:space:]]*\}/ { in_perms=0 } + ' "$file" | sort -u +} + +TF_ACTIONS=$(extract_tf_list "actions" "$TF_FILE") +TF_NOT_ACTIONS=$(extract_tf_list "not_actions" "$TF_FILE") +TF_DATA_ACTIONS=$(extract_tf_list "data_actions" "$TF_FILE") +TF_NOT_DATA_ACTIONS=$(extract_tf_list "not_data_actions" "$TF_FILE") if [[ -z "$TF_ACTIONS" ]]; then echo "ERROR: No actions extracted from TF module: $TF_FILE" >&2 @@ -78,23 +96,37 @@ if [[ -z "$TF_ACTIONS" ]]; then exit 1 fi -# --- extract actions from ARM JSON ------------------------------------------- -# Pull .resources[] where .type == "Microsoft.Authorization/roleDefinitions", -# then walk into .properties.permissions[0].actions. - -if ! command -v jq &>/dev/null; then - echo "ERROR: jq is required but not installed." >&2 - exit 2 -fi - -ARM_ACTIONS=$( - jq -r ' - .resources[] - | select(.type == "Microsoft.Authorization/roleDefinitions") - | .properties.permissions[0].actions[] +# --- extract permission lists from ARM JSON ------------------------------------ +# Walks every Microsoft.Authorization/roleDefinitions resource ANYWHERE in the +# template (`..`, not just the top-level .resources[] array), matching the +# resource type case-insensitively since ARM resource types are. A prior +# revision matched only the top-level array with a case-sensitive `==`, so a +# second role definition typed "microsoft.authorization/roleDefinitions" +# (lowercase) was invisible to this axis even though the case-insensitive scope +# walk below saw it fine -- actions and scope disagreeing about how many role +# definitions exist is exactly the kind of drift this check exists to catch. +# +# Also unions every entry of permissions[], not just permissions[0]: ARM unions +# permissions across the whole array, so a second entry appended after the +# canonical one silently added grants that comparing only index 0 missed. +# actions/notActions/dataActions/notDataActions each default to [] when absent, +# matching ARM's own semantics for an omitted key. +extract_arm_list() { + local key="$1" file="$2" + jq -r --arg key "$key" ' + [.. | objects | select(has("type")) | select((.type|type) == "string") + | select((.type|ascii_downcase) == "microsoft.authorization/roledefinitions")] + | map(.properties.permissions // [] | .[] | (.[$key] // [])[]) + | flatten + | .[] | ascii_downcase - ' "$ARM_FILE" | sort -) + ' "$file" | sort -u +} + +ARM_ACTIONS=$(extract_arm_list "actions" "$ARM_FILE") +ARM_NOT_ACTIONS=$(extract_arm_list "notActions" "$ARM_FILE") +ARM_DATA_ACTIONS=$(extract_arm_list "dataActions" "$ARM_FILE") +ARM_NOT_DATA_ACTIONS=$(extract_arm_list "notDataActions" "$ARM_FILE") if [[ -z "$ARM_ACTIONS" ]]; then echo "ERROR: No actions extracted from ARM template: $ARM_FILE" >&2 @@ -102,24 +134,32 @@ if [[ -z "$ARM_ACTIONS" ]]; then exit 1 fi -# --- compare ----------------------------------------------------------------- +# --- compare ------------------------------------------------------------------- -DIFF=$(diff <(echo "$TF_ACTIONS") <(echo "$ARM_ACTIONS") || true) +compare_action_lists() { + local label="$1" tf_list="$2" arm_list="$3" + local d + d=$(diff <(echo "$tf_list") <(echo "$arm_list") || true) + if [[ -n "$d" ]]; then + echo "ERROR: ARM template and TF module ${label} lists differ." >&2 + echo "" >&2 + echo " TF source : $TF_FILE" >&2 + echo " ARM source: $ARM_FILE" >&2 + echo "" >&2 + echo "Diff (< TF > ARM):" >&2 + echo "$d" >&2 + echo "" >&2 + echo "Update the lagging file so both ${label} lists match." >&2 + exit 1 + fi +} -if [[ -n "$DIFF" ]]; then - echo "ERROR: ARM template and TF module actions lists differ." >&2 - echo "" >&2 - echo " TF source : $TF_FILE" >&2 - echo " ARM source: $ARM_FILE" >&2 - echo "" >&2 - echo "Diff (< TF > ARM):" >&2 - echo "$DIFF" >&2 - echo "" >&2 - echo "Update the lagging file so both lists match." >&2 - exit 1 -fi +compare_action_lists "actions" "$TF_ACTIONS" "$ARM_ACTIONS" +compare_action_lists "notActions" "$TF_NOT_ACTIONS" "$ARM_NOT_ACTIONS" +compare_action_lists "dataActions" "$TF_DATA_ACTIONS" "$ARM_DATA_ACTIONS" +compare_action_lists "notDataActions" "$TF_NOT_DATA_ACTIONS" "$ARM_NOT_DATA_ACTIONS" -echo "OK: ARM and TF actions lists match (${#TF_ACTIONS} bytes, case-insensitive)." +echo "OK: ARM and TF actions/notActions/dataActions/notDataActions lists match (case-insensitive)." # --- scope invariant (issue #1545) ------------------------------------------- # Every scope the ARM template grants at must stay inside the subscription that @@ -130,19 +170,39 @@ echo "OK: ARM and TF actions lists match (${#TF_ACTIONS} bytes, case-insensitive # ACCIDENT, so it is tuned to catch the shapes a well-meaning author actually # reaches for, and it errs towards refusing anything it cannot reason about. # -# The allowlist is EXACT-MATCH, not substring-anchored. An earlier revision -# accepted any value containing "/subscriptions/", which is far weaker than it -# reads: "[concat('/subscriptions/', parameters('otherSubscriptionId'))]" -# satisfies it while granting in a subscription the customer never targeted, -# in a template named CrossSubscription. Only two shapes are accepted: +# The allowlist is EXACT-MATCH (after normalization, see normalize_scope_expr +# below), not substring-anchored. An earlier revision accepted any value +# containing "/subscriptions/", which is far weaker than it reads: +# "[concat('/subscriptions/', parameters('otherSubscriptionId'))]" satisfies it +# while granting in a subscription the customer never targeted, in a template +# named CrossSubscription. # -# 1. the canonical ARM expression binding the scope to the deployment target -# 2. a bare literal /subscriptions/, used by the test fixtures -# -# Anything else is rejected, including an escape assembled from a `variables` -# block that this script never reads. +# A later revision of THIS check also accepted a bare literal +# /subscriptions/, on the theory that the test fixtures needed one. That +# was never true and the claim was disproved directly: the fixtures were moved +# to the canonical expression below and the full self-test suite still passes. +# Constraining the literal to GUID *shape* accepts any GUID, including one +# hard-coding a foreign subscription -- exactly the cross-subscription +# escalation this check exists to catch (issue #1545), and it slips through +# silently because a foreign literal has the same shape as a legitimate one; +# this script has no way to know the deployment target's own subscription id +# at review time, so it cannot tell them apart. Only the canonical ARM +# expression -- or its `subscription().id` equivalent -- is accepted; a +# literal, however it is spelled, never is. CANONICAL_SCOPE_EXPR="[concat('/subscriptions/', subscription().subscriptionId)]" -LITERAL_SUBSCRIPTION_RE='^/subscriptions/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' +CANONICAL_SCOPE_EXPR_ALT="[subscription().id]" + +# Byte-exact comparison against CANONICAL_SCOPE_EXPR is brittle: whitespace +# placement, quote style, and a redundant empty-string concat argument are all +# spellings a well-meaning author could reach for that denote the identical +# runtime value. Normalize both sides before comparing so the guard reasons +# about the expression's meaning, not its formatting -- a pure reformat must +# not be able to red CI, or the guard invites being deleted out of frustration. +normalize_scope_expr() { + printf '%s' "$1" | tr -d '[:space:]' | tr '"' "'" | sed -E "s/,''\)\]\$/)]/" +} +CANONICAL_SCOPE_NORM="$(normalize_scope_expr "$CANONICAL_SCOPE_EXPR")" +CANONICAL_SCOPE_ALT_NORM="$(normalize_scope_expr "$CANONICAL_SCOPE_EXPR_ALT")" # Retained as defence in depth. The exact-match allowlist above already rejects # every one of these, but they name the specific scopes that motivated the @@ -156,6 +216,21 @@ LITERAL_SUBSCRIPTION_RE='^/subscriptions/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0- # Microsoft.Billing -> /providers/Microsoft.Billing/billingAccounts/* ESCAPE_TOKENS='Microsoft\.Capacity|Microsoft\.Management|managementGroups|Microsoft\.Billing' +# The three roleDefinitionId expressions this template ever assigns (issue +# #1545, finding F6): the custom purchaser role it defines, and the two +# built-in roles it looks up via the `roles` variable. A role assignment +# binding anything else -- e.g. the built-in Owner role -- while correctly +# inheriting the subscription-scope deployment (no explicit `scope`, so the +# check above finds nothing wrong) would still grant far more than the +# calculatePrice -> purchase flow needs. Compared as raw ARM expression text, +# the same way CANONICAL_SCOPE_EXPR is: this is a drift guard tied to this +# template's own variables, not a general ARM evaluator. +ALLOWED_ROLE_DEFINITION_IDS=( + "[variables('customRoleDefinitionId')]" + "[variables('roles').reader]" + "[variables('roles').costManagementReader]" +) + # The deployment scope is an invariant of this template, not a detail: the # three role assignments carry no `scope` property and therefore inherit it. # Repointing $schema at the management-group template would silently land all @@ -172,25 +247,38 @@ if ! jq -e '.["$schema"] | test("subscriptionDeploymentTemplate")' "$ARM_FILE" > fi # A nested deployment can carry an inner template with its own role -# assignments, at its own scope. That is the idiomatic ARM way to assign at a +# assignments, at its own scope -- the idiomatic ARM way to assign at a # different scope from a subscription deployment, so it is exactly what a -# future author with a legitimate cross-scope need would reach for. This script -# cannot reason about inner templates, so refuse rather than pass them silently. +# future author with a legitimate cross-scope need would reach for. A +# deployment script is the same problem in a different shape: its runtime +# `az`/az-cli commands can issue role assignments this check never sees as +# JSON at all. This check cannot inspect either, so refuse rather than pass +# them silently. NESTED_COUNT=$( - jq '[.. | objects | select(has("type")) | select((.type|type) == "string") - | select((.type|ascii_downcase) == "microsoft.resources/deployments")] | length' "$ARM_FILE" + jq ' + [.. | objects | select(has("type")) | select((.type|type) == "string") + | select((.type|ascii_downcase) as $t + | ["microsoft.resources/deployments", + "microsoft.resources/deploymentscripts", + "microsoft.resources/deploymentstacks"] + | index($t) != null)] + | length + ' "$ARM_FILE" ) if [[ "$NESTED_COUNT" != "0" ]]; then - echo "ERROR: ARM template contains ${NESTED_COUNT} nested deployment(s)." >&2 - echo " This check cannot inspect the scopes inside a nested template, so a" >&2 + echo "ERROR: ARM template contains ${NESTED_COUNT} nested deployment(s), deployment" >&2 + echo " script(s), or deployment stack(s)." >&2 + echo " This check cannot inspect the scopes inside a nested template, nor the" >&2 + echo " role assignments a deployment script issues at runtime, so a" >&2 echo " tenant-scoped assignment could hide there (issue #1545). Either" >&2 echo " inline the resources, or extend this script to recurse into" >&2 echo " properties.template before adding one." >&2 exit 1 fi -# Collect every scope value the template grants at, one per line, tagged with -# where it came from so the error message points at the right JSON node. +# Collect every scope value and roleDefinitionId the template grants, +# one per line, tagged with where it came from so the error message points at +# the right JSON node. # # The walk is recursive (`..`) rather than over the top-level `resources` array # only, so an assignment nested inside a parent resource's own `resources` @@ -206,7 +294,11 @@ SCOPES=$( ( $all[] | select((.type|ascii_downcase) == "microsoft.authorization/roleassignments") | select(has("scope")) - | "roleAssignment.scope\t" + (.scope|tostring) ) + | "roleAssignment.scope\t" + (.scope|tostring) ), + ( $all[] + | select((.type|ascii_downcase) == "microsoft.authorization/roleassignments") + | select(has("properties") and (.properties|has("roleDefinitionId"))) + | "roleAssignment.roleDefinitionId\t" + (.properties.roleDefinitionId|tostring) ) ' "$ARM_FILE" ) @@ -230,9 +322,20 @@ while IFS=$'\t' read -r origin value; do # explicit `scope` is how #1545 shipped, and there is no legitimate use # for one here. reason="role assignments must inherit the deployment scope, not set one" - elif [[ "$value" == "$CANONICAL_SCOPE_EXPR" ]]; then + elif [[ "$origin" == "roleAssignment.roleDefinitionId" ]]; then + allowed_match=0 + for allowed in "${ALLOWED_ROLE_DEFINITION_IDS[@]}"; do + if [[ "$value" == "$allowed" ]]; then + allowed_match=1 + break + fi + done + if [[ "$allowed_match" -eq 0 ]]; then + reason="roleDefinitionId is not one of the three roles CUDly assigns (custom purchaser, Reader, Cost Management Reader)" + fi + elif [[ "$(normalize_scope_expr "$value")" == "$CANONICAL_SCOPE_NORM" ]]; then : - elif [[ "$value" =~ $LITERAL_SUBSCRIPTION_RE ]]; then + elif [[ "$(normalize_scope_expr "$value")" == "$CANONICAL_SCOPE_ALT_NORM" ]]; then : elif [[ "$value" =~ $ESCAPE_TOKENS ]]; then reason="names a scope above the subscription" @@ -252,14 +355,20 @@ if [[ -n "$SCOPE_VIOLATIONS" ]]; then echo "" >&2 printf '%s' "$SCOPE_VIOLATIONS" >&2 echo "" >&2 - echo "assignableScopes must be exactly:" >&2 + echo "assignableScopes must be the canonical deployment-scope expression" >&2 + echo "(whitespace and quote-style differences are tolerated):" >&2 echo " ${CANONICAL_SCOPE_EXPR}" >&2 - echo "and role assignments must carry no explicit scope. A grant at a tenant," >&2 - echo "management-group or billing-account scope, or at another subscription," >&2 - echo "reaches subscriptions the customer never onboarded (issue #1545). If a" >&2 - echo "wider grant is genuinely required it must be a separate, manually" >&2 - echo "applied, explicitly consented step, never part of this template." >&2 - echo "See known-issues.md." >&2 + echo " or equivalently: ${CANONICAL_SCOPE_EXPR_ALT}" >&2 + echo "A bare literal /subscriptions/ is never accepted, even for the" >&2 + echo "onboarded subscription itself: accepting a literal by GUID shape alone" >&2 + echo "cannot distinguish it from a foreign subscription hard-coded into the" >&2 + echo "template. Role assignments must carry no explicit scope, and" >&2 + echo "roleDefinitionId must be one of the three roles CUDly assigns. A grant" >&2 + echo "at a tenant, management-group or billing-account scope, or at another" >&2 + echo "subscription, reaches subscriptions the customer never onboarded" >&2 + echo "(issue #1545). If a wider grant is genuinely required it must be a" >&2 + echo "separate, manually applied, explicitly consented step, never part of" >&2 + echo "this template. See known-issues.md." >&2 exit 1 fi @@ -299,5 +408,5 @@ if grep -q 'include_capacity_provider_scope' "$TF_FILE"; then fi SCOPE_COUNT=$(echo "$SCOPES" | grep -c . || true) -echo "OK: all ${SCOPE_COUNT} ARM grant scopes are subscription-anchored." +echo "OK: all ${SCOPE_COUNT} ARM grant scopes/roleDefinitionIds are subscription-anchored." exit 0 diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index 2c9c69f93..b4b31d972 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -98,6 +98,80 @@ run_case "TF flag without variables.tf exits 1" 1 \ --tf-file "${TMP_TF_DIR}/main.tf" \ --arm-file "${FIXTURES}/matching-arm.json" +# --- adversarial-review bypass regressions (PR #1658) ----------------------- +# The cases above passed 9/9 while three bypasses (F1, F3, F4) were still live. +# Each case below reproduces one adversarial-review finding and must fail for +# its own specific reason, not incidentally alongside an unrelated one. + +# Case 10 (F1): the deployable shape of the literal-subscription bypass -- the +# canonical expression is retained (so in-subscription assignments keep +# working) with a foreign-subscription literal appended to the same array. +# A GUID-shape-only allowlist accepted this; only rejecting literals outright +# catches it. +run_case "foreign-subscription literal alongside canonical exits 1 (F1)" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/foreign-subscription-literal-with-canonical-arm.json" + +# Case 11 (F1): an uppercase GUID literal. `nocasematch` was live across the +# old LITERAL_SUBSCRIPTION_RE match, so `[0-9a-f]` also matched uppercase; +# rejecting literals outright makes case sufficiency moot. +run_case "uppercase GUID literal exits 1 (F1)" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/uppercase-guid-literal-arm.json" + +# Case 12 (F2): whitespace, quote-style, a redundant empty-string concat arg, +# and the `subscription().id` equivalent are all spellings of the same +# canonical scope. A byte-exact comparison rejected every one of them; this +# fixture carries all four in one assignableScopes array and must pass. +run_case "canonical scope spelling variants exit 0 (F2)" 0 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/canonical-scope-variants-arm.json" + +# Case 13 (F3): a second role definition typed +# "microsoft.authorization/roleDefinitions" (lowercase) granting actions:["*"]. +# The actions extractor matched .resources[] at the top level with a +# case-sensitive `==`, so this role was invisible to the actions axis even +# though the case-insensitive scope walk counted it (the old "all 2 ARM grant +# scopes are subscription-anchored" message proved the scope axis saw what the +# actions axis did not). +run_case "lowercase-typed second role def with wildcard actions exits 1 (F3)" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/lowercase-type-wildcard-actions-arm.json" + +# Case 14 (F4): a second permissions[] entry appended after the canonical one, +# granting actions:["*"]. ARM unions permissions across the whole array; only +# comparing permissions[0] missed the second entry entirely. +run_case "second permissions entry with wildcard actions exits 1 (F4)" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/second-permissions-entry-arm.json" + +# Case 15 (F4): dataActions:["*"] on the (otherwise matching) first permissions +# entry. dataActions/notActions/notDataActions were never compared at all. +run_case "dataActions wildcard exits 1 (F4)" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/dataactions-wildcard-arm.json" + +# Case 16 (F5): a Microsoft.Resources/deploymentScripts resource alongside the +# (matching, canonical-scope) role definition. Only "deployments" was refused; +# a deployment script's runtime az-cli commands can issue role assignments this +# check never sees as JSON at all. +run_case "deploymentScripts resource exits 1 (F5)" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/deploymentscript-scope-escape-arm.json" + +# Case 17 (F5): the same refusal, for Microsoft.Resources/deploymentStacks. +run_case "deploymentStacks resource exits 1 (F5)" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/deploymentstack-scope-escape-arm.json" + +# Case 18 (F6): a role assignment binding the built-in Owner role, carrying no +# explicit `scope` so it correctly inherits the subscription-scope deployment +# (the scope axis finds nothing wrong). Only an explicit `scope` was ever +# checked; roleDefinitionId itself was unconstrained. +run_case "unallowed roleDefinitionId (Owner) exits 1 (F6)" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/unallowed-roledefinitionid-arm.json" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/role-parity/canonical-scope-variants-arm.json b/scripts/testdata/role-parity/canonical-scope-variants-arm.json new file mode 100644 index 000000000..a79f213c2 --- /dev/null +++ b/scripts/testdata/role-parity/canonical-scope-variants-arm.json @@ -0,0 +1,38 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[ concat('/subscriptions/', subscription().subscriptionId) ]", + "[concat(\"/subscriptions/\", subscription().subscriptionId)]", + "[concat('/subscriptions/', subscription().subscriptionId, '')]", + "[subscription().id]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/dataactions-wildcard-arm.json b/scripts/testdata/role-parity/dataactions-wildcard-arm.json new file mode 100644 index 000000000..29af37e51 --- /dev/null +++ b/scripts/testdata/role-parity/dataactions-wildcard-arm.json @@ -0,0 +1,39 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [], + "dataActions": [ + "*" + ], + "notDataActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json b/scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json new file mode 100644 index 000000000..6c474de5b --- /dev/null +++ b/scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json @@ -0,0 +1,47 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Resources/deploymentScripts", + "apiVersion": "2020-10-01", + "name": "escalate-scope", + "location": "eastus", + "kind": "AzureCLI", + "properties": { + "azCliVersion": "2.50.0", + "retentionInterval": "P1D", + "scriptContent": "az role assignment create --role Owner --scope /providers/Microsoft.Capacity --assignee $PRINCIPAL_ID" + } + } + ] +} diff --git a/scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json b/scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json new file mode 100644 index 000000000..738a409a5 --- /dev/null +++ b/scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json @@ -0,0 +1,49 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Resources/deploymentStacks", + "apiVersion": "2022-08-01-preview", + "name": "nested-stack", + "properties": { + "actionOnUnmanage": { + "resources": "detach" + }, + "denySettings": { + "mode": "none" + }, + "template": {} + } + } + ] +} diff --git a/scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json b/scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json new file mode 100644 index 000000000..03730c27d --- /dev/null +++ b/scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json @@ -0,0 +1,36 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]", + "/subscriptions/11111111-1111-1111-1111-111111111111" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/lowercase-type-wildcard-actions-arm.json b/scripts/testdata/role-parity/lowercase-type-wildcard-actions-arm.json new file mode 100644 index 000000000..dc24c3b00 --- /dev/null +++ b/scripts/testdata/role-parity/lowercase-type-wildcard-actions-arm.json @@ -0,0 +1,54 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "microsoft.authorization/roledefinitions", + "apiVersion": "2022-04-01", + "name": "test-role-escalation", + "properties": { + "roleName": "CUDly Test Role Escalation", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "*" + ] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/matching-arm.json b/scripts/testdata/role-parity/matching-arm.json index 9767aad9b..516dc12cd 100644 --- a/scripts/testdata/role-parity/matching-arm.json +++ b/scripts/testdata/role-parity/matching-arm.json @@ -27,7 +27,7 @@ } ], "assignableScopes": [ - "/subscriptions/00000000-0000-0000-0000-000000000001" + "[concat('/subscriptions/', subscription().subscriptionId)]" ] } } diff --git a/scripts/testdata/role-parity/second-permissions-entry-arm.json b/scripts/testdata/role-parity/second-permissions-entry-arm.json new file mode 100644 index 000000000..9891bf4ba --- /dev/null +++ b/scripts/testdata/role-parity/second-permissions-entry-arm.json @@ -0,0 +1,40 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + }, + { + "actions": [ + "*" + ] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json b/scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json new file mode 100644 index 000000000..9189f0ae7 --- /dev/null +++ b/scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json @@ -0,0 +1,45 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "name": "owner-grant", + "properties": { + "roleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +} diff --git a/scripts/testdata/role-parity/uppercase-guid-literal-arm.json b/scripts/testdata/role-parity/uppercase-guid-literal-arm.json new file mode 100644 index 000000000..0ae919a83 --- /dev/null +++ b/scripts/testdata/role-parity/uppercase-guid-literal-arm.json @@ -0,0 +1,35 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "/subscriptions/AAAAAAAA-BBBB-CCCC-DDDD-EEEEEEEEEEEE" + ] + } + } + ] +} From af8bbc3d2b9ea62d4a52a3233042c9b882776936 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:24:33 +0200 Subject: [PATCH 04/10] sec(iac): harden normalizer and scope recursive walks to .resources Self-review of the round-2 fix (99ea68759) 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. --- scripts/check-azure-role-parity.sh | 71 ++++++++++++++----- scripts/test-azure-role-parity.sh | 26 +++++++ .../role-parity/decorative-variables-arm.json | 52 ++++++++++++++ .../role-parity/space-inside-literal-arm.json | 35 +++++++++ 4 files changed, 168 insertions(+), 16 deletions(-) create mode 100644 scripts/testdata/role-parity/decorative-variables-arm.json create mode 100644 scripts/testdata/role-parity/space-inside-literal-arm.json diff --git a/scripts/check-azure-role-parity.sh b/scripts/check-azure-role-parity.sh index 683eab161..7b16f8107 100755 --- a/scripts/check-azure-role-parity.sh +++ b/scripts/check-azure-role-parity.sh @@ -97,14 +97,24 @@ if [[ -z "$TF_ACTIONS" ]]; then fi # --- extract permission lists from ARM JSON ------------------------------------ -# Walks every Microsoft.Authorization/roleDefinitions resource ANYWHERE in the -# template (`..`, not just the top-level .resources[] array), matching the -# resource type case-insensitively since ARM resource types are. A prior -# revision matched only the top-level array with a case-sensitive `==`, so a -# second role definition typed "microsoft.authorization/roleDefinitions" -# (lowercase) was invisible to this axis even though the case-insensitive scope -# walk below saw it fine -- actions and scope disagreeing about how many role -# definitions exist is exactly the kind of drift this check exists to catch. +# Walks every Microsoft.Authorization/roleDefinitions resource under the +# template's `resources` array (`.resources // [] | ..`, not just the +# top-level .resources[] array itself), matching the resource type +# case-insensitively since ARM resource types are. A prior revision matched +# only the top-level array with a case-sensitive `==`, so a second role +# definition typed "microsoft.authorization/roleDefinitions" (lowercase) was +# invisible to this axis even though the case-insensitive scope walk below saw +# it fine -- actions and scope disagreeing about how many role definitions +# exist is exactly the kind of drift this check exists to catch. +# +# The walk is rooted at `.resources`, not the whole document (`..` from `.`): +# a `..` from the root would also match a decorative object under `variables` +# or `outputs` that merely happens to carry `"type": +# "Microsoft.Authorization/roleDefinitions"` for documentation purposes but is +# never deployed, and red a template whose actions genuinely match. Rooting at +# `.resources` still finds a role definition nested inside a parent resource's +# own `resources` array (the same reason the scope walk below is recursive), +# it just never leaves the tree of things ARM actually deploys. # # Also unions every entry of permissions[], not just permissions[0]: ARM unions # permissions across the whole array, so a second entry appended after the @@ -114,7 +124,7 @@ fi extract_arm_list() { local key="$1" file="$2" jq -r --arg key "$key" ' - [.. | objects | select(has("type")) | select((.type|type) == "string") + [.resources // [] | .. | objects | select(has("type")) | select((.type|type) == "string") | select((.type|ascii_downcase) == "microsoft.authorization/roledefinitions")] | map(.properties.permissions // [] | .[] | (.[$key] // [])[]) | flatten @@ -198,8 +208,29 @@ CANONICAL_SCOPE_EXPR_ALT="[subscription().id]" # runtime value. Normalize both sides before comparing so the guard reasons # about the expression's meaning, not its formatting -- a pure reformat must # not be able to red CI, or the guard invites being deleted out of frustration. +# +# Whitespace is collapsed only where it is immediately adjacent to structural +# punctuation ([ ] ( ) ,), never between two ordinary characters. A blanket +# `tr -d '[:space:]'` over the whole expression would also strip whitespace +# INSIDE the '/subscriptions/' string literal, so a typo like +# "'/sub scriptions/'" would normalize to the same text as the real literal -- +# tolerating a corrupted value as if it were a reformat of the canonical one. +# It happens not to be exploitable (the corrupted literal doesn't resolve to a +# different, wider scope; it just fails to deploy), but a normalizer's whole +# job is telling "different formatting" apart from "different value", and this +# blurred that line. Punctuation-adjacent collapsing can't reach inside a +# literal's content, because that content by construction contains no +# whitespace next to `[`, `]`, `(`, `)`, or `,`. normalize_scope_expr() { - printf '%s' "$1" | tr -d '[:space:]' | tr '"' "'" | sed -E "s/,''\)\]\$/)]/" + local v="$1" + v="$(printf '%s' "$v" | sed -E 's/^[[:space:]]+//; s/[[:space:]]+$//')" # trim the whole value only + v="$(printf '%s' "$v" | tr '"' "'")" # quote style is not meaningful + v="$(printf '%s' "$v" | sed -E ' + s/\[[[:space:]]+/[/g; s/[[:space:]]+\]/]/g; + s/\([[:space:]]+/(/g; s/[[:space:]]+\)/)/g; + s/,[[:space:]]+/,/g; s/[[:space:]]+,/,/g; + ')" + printf '%s' "$v" | sed -E "s/,''\)\]\$/)]/" # drop a redundant ,'' arg } CANONICAL_SCOPE_NORM="$(normalize_scope_expr "$CANONICAL_SCOPE_EXPR")" CANONICAL_SCOPE_ALT_NORM="$(normalize_scope_expr "$CANONICAL_SCOPE_EXPR_ALT")" @@ -254,9 +285,14 @@ fi # `az`/az-cli commands can issue role assignments this check never sees as # JSON at all. This check cannot inspect either, so refuse rather than pass # them silently. +# +# Rooted at `.resources`, same reasoning as extract_arm_list above: a +# decorative object elsewhere in the template (variables, outputs) is never +# deployed, so it must not be able to trip this refusal on a template that is +# otherwise fine. NESTED_COUNT=$( jq ' - [.. | objects | select(has("type")) | select((.type|type) == "string") + [.resources // [] | .. | objects | select(has("type")) | select((.type|type) == "string") | select((.type|ascii_downcase) as $t | ["microsoft.resources/deployments", "microsoft.resources/deploymentscripts", @@ -280,13 +316,16 @@ fi # one per line, tagged with where it came from so the error message points at # the right JSON node. # -# The walk is recursive (`..`) rather than over the top-level `resources` array -# only, so an assignment nested inside a parent resource's own `resources` -# array is still seen. Type matching is case-insensitive because ARM resource -# types are, while jq's `==` is not. +# The walk is rooted at `.resources` and recursive (`.resources // [] | ..`) +# from there, rather than over the top-level `resources` array's direct +# elements only, so an assignment nested inside a parent resource's own +# `resources` array is still seen -- but a decorative object under +# `variables` or `outputs` that is never actually deployed is not, so it +# cannot red an otherwise-correct template. Type matching is case-insensitive +# because ARM resource types are, while jq's `==` is not. SCOPES=$( jq -r ' - [.. | objects | select(has("type")) | select((.type|type) == "string")] as $all + [.resources // [] | .. | objects | select(has("type")) | select((.type|type) == "string")] as $all | ( $all[] | select((.type|ascii_downcase) == "microsoft.authorization/roledefinitions") | (.properties.assignableScopes // [])[] diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index b4b31d972..3702968cc 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -172,6 +172,32 @@ run_case "unallowed roleDefinitionId (Owner) exits 1 (F6)" 1 \ --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ --arm-file "${FIXTURES}/unallowed-roledefinitionid-arm.json" +# --- self-review of the round-2 fix itself ----------------------------------- +# A normalizer exists to make different strings equal, which is exactly what +# an attacker wants; a recursive JSON walk exists to see more of the +# template, which is exactly what a false positive comes from. Both cut +# points identified during self-review before any external reviewer got to +# them. + +# Case 19: a typo'd '/sub scriptions/' (a space inside the string literal, not +# adjacent to any [ ] ( ) , ) must NOT normalize to the canonical scope. It +# doesn't grant anything wider -- the corrupted path just fails to deploy -- +# but a normalizer that can't tell "reformatted" from "corrupted" is the +# textbook shape of the next bypass, so this is refused rather than tolerated. +run_case "whitespace inside scope string literal exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/space-inside-literal-arm.json" + +# Case 20: a decorative object under `variables` that merely happens to carry +# "type": "Microsoft.Authorization/roleDefinitions" (e.g. left behind as +# documentation) and grants wildcard actions at a tenant scope. Never +# deployed, so it must not be visible to this check at all -- an otherwise +# clean, matching template must still exit 0. A guard that reds valid input +# invites being deleted, which is how issue #1545 shipped in the first place. +run_case "decorative variables object is invisible, exits 0" 0 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/decorative-variables-arm.json" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/role-parity/decorative-variables-arm.json b/scripts/testdata/role-parity/decorative-variables-arm.json new file mode 100644 index 000000000..bfced043b --- /dev/null +++ b/scripts/testdata/role-parity/decorative-variables-arm.json @@ -0,0 +1,52 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "variables": { + "decorativeDocOnly": { + "type": "Microsoft.Authorization/roleDefinitions", + "properties": { + "permissions": [ + { + "actions": [ + "*" + ] + } + ], + "assignableScopes": [ + "/providers/Microsoft.Capacity" + ] + } + } + }, + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/space-inside-literal-arm.json b/scripts/testdata/role-parity/space-inside-literal-arm.json new file mode 100644 index 000000000..d56de7492 --- /dev/null +++ b/scripts/testdata/role-parity/space-inside-literal-arm.json @@ -0,0 +1,35 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/sub scriptions/', subscription().subscriptionId)]" + ] + } + } + ] +} From 53f5c94b1c60004b21a99106641f1de8730e53e2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:39:54 +0200 Subject: [PATCH 05/10] sec(iac): normalize ARM key casing, refuse PIM/legacy grant types Independent review defeated round-3 (76abcc853) 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. --- scripts/check-azure-role-parity.sh | 129 +++++++++++++----- scripts/test-azure-role-parity.sh | 64 +++++++++ ...legacy-child-roleassignment-owner-arm.json | 45 ++++++ .../miscased-assignablescopes-arm.json | 65 +++++++++ .../role-parity/miscased-properties-arm.json | 45 ++++++ .../miscased-roledefinitionid-arm.json | 45 ++++++ .../role-parity/miscased-scope-arm.json | 44 ++++++ ...pim-roleassignment-schedule-owner-arm.json | 50 +++++++ .../pim-roleeligibility-owner-arm.json | 50 +++++++ 9 files changed, 501 insertions(+), 36 deletions(-) create mode 100644 scripts/testdata/role-parity/legacy-child-roleassignment-owner-arm.json create mode 100644 scripts/testdata/role-parity/miscased-assignablescopes-arm.json create mode 100644 scripts/testdata/role-parity/miscased-properties-arm.json create mode 100644 scripts/testdata/role-parity/miscased-roledefinitionid-arm.json create mode 100644 scripts/testdata/role-parity/miscased-scope-arm.json create mode 100644 scripts/testdata/role-parity/pim-roleassignment-schedule-owner-arm.json create mode 100644 scripts/testdata/role-parity/pim-roleeligibility-owner-arm.json diff --git a/scripts/check-azure-role-parity.sh b/scripts/check-azure-role-parity.sh index 7b16f8107..6e1221875 100755 --- a/scripts/check-azure-role-parity.sh +++ b/scripts/check-azure-role-parity.sh @@ -61,6 +61,33 @@ if ! command -v jq &>/dev/null; then exit 2 fi +# --- normalize ARM property-key casing --------------------------------------- +# Azure Resource Manager's resource-provider JSON deserializers are documented +# as case-insensitive by default, so a resource typed correctly but with a +# miscased property key -- "Scope" instead of "scope", "RoleDefinitionId", +# "Properties", "AssignableScopes" -- was invisible to every jq query below: +# silence, not refusal. The first of those is issue #1545 byte-for-byte apart +# from one capital letter. +# +# Whether ARM itself is genuinely lenient about a specific miscased key (as +# opposed to accepting a template at all) was not independently verified +# against a live subscription -- that would require deploying a miscased +# template to a real tenant, which was not done. Fixing this fail-closed +# regardless is still correct either way: if ARM does accept the miscased +# key, this closes a real hole; if it does not, the guard is merely redundant +# with a deploy-time rejection, never wrong. +# +# Lowercase every object key once, into a scratch copy, and query that copy +# from here on. Every jq field access below that names a non-lowercase ARM +# property (roleDefinitionId, assignableScopes, notActions, dataActions, +# notDataActions) is written in lowercase to match; `type`, `properties`, +# `permissions`, `scope`, `resources`, and `actions` are already all-lowercase +# in correct ARM spelling, so normalizing them is a no-op either way. +ARM_FILE_NORM="$(mktemp)" +trap 'rm -f "$ARM_FILE_NORM"' EXIT +jq 'walk(if type == "object" then with_entries(.key |= ascii_downcase) else . end)' \ + "$ARM_FILE" > "$ARM_FILE_NORM" + # --- extract permission lists from TF ----------------------------------------- # Pulls each of actions / not_actions / data_actions / not_data_actions out of # the `permissions { ... }` block of the azurerm_role_definition resource. @@ -126,17 +153,17 @@ extract_arm_list() { jq -r --arg key "$key" ' [.resources // [] | .. | objects | select(has("type")) | select((.type|type) == "string") | select((.type|ascii_downcase) == "microsoft.authorization/roledefinitions")] - | map(.properties.permissions // [] | .[] | (.[$key] // [])[]) + | map(.properties.permissions // [] | .[] | (.[$key|ascii_downcase] // [])[]) | flatten | .[] | ascii_downcase ' "$file" | sort -u } -ARM_ACTIONS=$(extract_arm_list "actions" "$ARM_FILE") -ARM_NOT_ACTIONS=$(extract_arm_list "notActions" "$ARM_FILE") -ARM_DATA_ACTIONS=$(extract_arm_list "dataActions" "$ARM_FILE") -ARM_NOT_DATA_ACTIONS=$(extract_arm_list "notDataActions" "$ARM_FILE") +ARM_ACTIONS=$(extract_arm_list "actions" "$ARM_FILE_NORM") +ARM_NOT_ACTIONS=$(extract_arm_list "notActions" "$ARM_FILE_NORM") +ARM_DATA_ACTIONS=$(extract_arm_list "dataActions" "$ARM_FILE_NORM") +ARM_NOT_DATA_ACTIONS=$(extract_arm_list "notDataActions" "$ARM_FILE_NORM") if [[ -z "$ARM_ACTIONS" ]]; then echo "ERROR: No actions extracted from ARM template: $ARM_FILE" >&2 @@ -178,7 +205,18 @@ echo "OK: ARM and TF actions/notActions/dataActions/notDataActions lists match ( # This is a CI drift guard, not a security boundary: anyone who can edit the # template can edit this script. It exists to stop the grant being widened by # ACCIDENT, so it is tuned to catch the shapes a well-meaning author actually -# reaches for, and it errs towards refusing anything it cannot reason about. +# reaches for. +# +# It is NOT a general ARM evaluator, and does not refuse everything it cannot +# reason about: it recognizes a fixed set of resource types +# (Microsoft.Authorization/roleDefinitions, Microsoft.Authorization/ +# roleAssignments) and separately refuses outright a second fixed set it +# knows are grant-bearing or opaque and cannot safely inspect (see +# REFUSED_TYPES below). Any OTHER resource type -- including ones nobody has +# thought to add to either list yet -- is not inspected at all and passes +# silently. Closing that gap in general means asserting the template's exact +# expected set of grants, not enumerating everything to refuse; that is +# tracked separately in issue #1681 rather than attempted here. # # The allowlist is EXACT-MATCH (after normalization, see normalize_scope_expr # below), not substring-anchored. An earlier revision accepted any value @@ -267,8 +305,8 @@ ALLOWED_ROLE_DEFINITION_IDS=( # Repointing $schema at the management-group template would silently land all # of them at management-group scope, covering every child subscription, without # changing a single scope string. Pin it explicitly. -if ! jq -e '.["$schema"] | test("subscriptionDeploymentTemplate")' "$ARM_FILE" >/dev/null 2>&1; then - ACTUAL_SCHEMA=$(jq -r '.["$schema"] // ""' "$ARM_FILE") +if ! jq -e '.["$schema"] | test("subscriptionDeploymentTemplate")' "$ARM_FILE_NORM" >/dev/null 2>&1; then + ACTUAL_SCHEMA=$(jq -r '.["$schema"] // ""' "$ARM_FILE_NORM") echo "ERROR: ARM template is not a subscription-scoped deployment." >&2 echo " \$schema: ${ACTUAL_SCHEMA}" >&2 echo " Role assignments here carry no explicit scope, so they inherit the" >&2 @@ -277,38 +315,57 @@ if ! jq -e '.["$schema"] | test("subscriptionDeploymentTemplate")' "$ARM_FILE" > exit 1 fi -# A nested deployment can carry an inner template with its own role -# assignments, at its own scope -- the idiomatic ARM way to assign at a -# different scope from a subscription deployment, so it is exactly what a -# future author with a legitimate cross-scope need would reach for. A -# deployment script is the same problem in a different shape: its runtime -# `az`/az-cli commands can issue role assignments this check never sees as -# JSON at all. This check cannot inspect either, so refuse rather than pass -# them silently. +# Resource types this check knows it cannot safely inspect, refused outright +# rather than silently passed: +# +# deployments / deploymentScripts / deploymentStacks -- a nested deployment +# can carry an inner template with its own role assignments at its own +# scope (the idiomatic ARM way to assign at a different scope from a +# subscription deployment), and a deployment script's runtime `az`/az-cli +# commands can issue role assignments neither one exposes as JSON this +# check can walk. +# +# roleEligibilityScheduleRequests / roleAssignmentScheduleRequests -- Azure +# PIM (Privileged Identity Management) resources. These grant a role the +# same way a plain roleAssignment does (confirmed: a PIM request binding +# the built-in Owner role deploys and grants it) but under a completely +# different property shape this check's roleAssignment selectors never +# match, so the roleDefinitionId allowlist above is bypassed simply by +# changing the resource type. +# +# storageAccounts/providers/roleAssignments -- the legacy ARM spelling for +# a role assignment as a child resource (a full `.../providers/...` type +# path) rather than a separate top-level roleAssignments resource with a +# `scope` property. Same grant, invisible to the same selectors for the +# same reason. # # Rooted at `.resources`, same reasoning as extract_arm_list above: a # decorative object elsewhere in the template (variables, outputs) is never # deployed, so it must not be able to trip this refusal on a template that is # otherwise fine. -NESTED_COUNT=$( - jq ' +REFUSED_TYPES='["microsoft.resources/deployments", + "microsoft.resources/deploymentscripts", + "microsoft.resources/deploymentstacks", + "microsoft.authorization/roleeligibilityschedulerequests", + "microsoft.authorization/roleassignmentschedulerequests", + "microsoft.storage/storageaccounts/providers/roleassignments"]' +REFUSED_TYPE_COUNT=$( + jq --argjson types "$REFUSED_TYPES" ' [.resources // [] | .. | objects | select(has("type")) | select((.type|type) == "string") - | select((.type|ascii_downcase) as $t - | ["microsoft.resources/deployments", - "microsoft.resources/deploymentscripts", - "microsoft.resources/deploymentstacks"] - | index($t) != null)] + | select((.type|ascii_downcase) as $t | $types | index($t) != null)] | length - ' "$ARM_FILE" + ' "$ARM_FILE_NORM" ) -if [[ "$NESTED_COUNT" != "0" ]]; then - echo "ERROR: ARM template contains ${NESTED_COUNT} nested deployment(s), deployment" >&2 - echo " script(s), or deployment stack(s)." >&2 - echo " This check cannot inspect the scopes inside a nested template, nor the" >&2 - echo " role assignments a deployment script issues at runtime, so a" >&2 - echo " tenant-scoped assignment could hide there (issue #1545). Either" >&2 - echo " inline the resources, or extend this script to recurse into" >&2 - echo " properties.template before adding one." >&2 +if [[ "$REFUSED_TYPE_COUNT" != "0" ]]; then + echo "ERROR: ARM template contains ${REFUSED_TYPE_COUNT} resource(s) of a type this" >&2 + echo " check cannot safely inspect: a nested deployment, deployment script or" >&2 + echo " stack, a PIM role-eligibility/role-assignment schedule request, or a" >&2 + echo " legacy child-scoped role assignment. Any of these can grant a role this" >&2 + echo " check never sees as a plain Microsoft.Authorization/roleAssignments" >&2 + echo " resource, so a tenant-scoped or otherwise unreviewed grant could hide" >&2 + echo " there (issue #1545). Either inline the resources as a plain role" >&2 + echo " assignment, or extend this script to recognize the new shape before" >&2 + echo " adding one." >&2 exit 1 fi @@ -328,7 +385,7 @@ SCOPES=$( [.resources // [] | .. | objects | select(has("type")) | select((.type|type) == "string")] as $all | ( $all[] | select((.type|ascii_downcase) == "microsoft.authorization/roledefinitions") - | (.properties.assignableScopes // [])[] + | (.properties.assignablescopes // [])[] | "assignableScopes\t" + . ), ( $all[] | select((.type|ascii_downcase) == "microsoft.authorization/roleassignments") @@ -336,9 +393,9 @@ SCOPES=$( | "roleAssignment.scope\t" + (.scope|tostring) ), ( $all[] | select((.type|ascii_downcase) == "microsoft.authorization/roleassignments") - | select(has("properties") and (.properties|has("roleDefinitionId"))) - | "roleAssignment.roleDefinitionId\t" + (.properties.roleDefinitionId|tostring) ) - ' "$ARM_FILE" + | select(has("properties") and (.properties|has("roledefinitionid"))) + | "roleAssignment.roleDefinitionId\t" + (.properties.roledefinitionid|tostring) ) + ' "$ARM_FILE_NORM" ) if [[ -z "$SCOPES" ]]; then diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index 3702968cc..e2bec6466 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -198,6 +198,70 @@ run_case "decorative variables object is invisible, exits 0" 0 \ --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ --arm-file "${FIXTURES}/decorative-variables-arm.json" +# --- round-4: independent review, key-casing and unmatched-type bypasses ---- +# ARM's resource-provider JSON deserializers are documented case-insensitive +# for property names; this script's jq queries were not, so a correctly +# recognized resource with one miscased property key was invisible to the +# specific check that key feeds -- silence, not refusal. Cases 21-24 are one +# per miscased key the independent review found live (each on its own, +# isolated from the others, with a second clean role definition/assignment +# alongside so the failure is attributable to the miscasing and nothing +# else). Cases 25-27 are grant-bearing resource types this check's +# roleAssignments-only type match never saw at all. + +# Case 21: a roleAssignment with "Scope" (capital S) set to the tenant-wide +# Microsoft.Capacity path. has("scope") never matched "Scope", so the +# assignment's explicit-scope violation -- the exact shape issue #1545 +# shipped as -- was invisible. +run_case "miscased 'Scope' key on tenant-wide assignment exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/miscased-scope-arm.json" + +# Case 22: a roleAssignment with "RoleDefinitionId" (capitalized) binding +# built-in Owner, no explicit scope. has("roleDefinitionId") never matched +# "RoleDefinitionId", so the roleDefinitionId allowlist (F6) was invisible. +run_case "miscased 'RoleDefinitionId' key on Owner grant exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/miscased-roledefinitionid-arm.json" + +# Case 23: a roleAssignment with "Properties" (capitalized) wrapping an Owner +# roleDefinitionId. has("properties") never matched "Properties", so nothing +# inside it -- roleDefinitionId included -- was ever reached. +run_case "miscased 'Properties' key on Owner grant exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/miscased-properties-arm.json" + +# Case 24: a second, actions-matching role definition whose "AssignableScopes" +# (capitalized) is the tenant-wide Microsoft.Capacity path. The canonical +# first role definition kept SCOPES non-empty, so the miscased entry's +# absence didn't even trip the "no assignableScopes found" fallback -- it +# just silently contributed nothing, and the template passed. +run_case "miscased 'AssignableScopes' key exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/miscased-assignablescopes-arm.json" + +# Case 25 (Fix B): Microsoft.Authorization/roleEligibilityScheduleRequests +# (Azure PIM) binding built-in Owner. Grants a role the same way a plain +# roleAssignment does, under a property shape this check's +# roleAssignments-only type match never saw. +run_case "PIM roleEligibilityScheduleRequests (Owner) exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/pim-roleeligibility-owner-arm.json" + +# Case 26 (Fix B): Microsoft.Authorization/roleAssignmentScheduleRequests +# (Azure PIM), same reasoning. +run_case "PIM roleAssignmentScheduleRequests (Owner) exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/pim-roleassignment-schedule-owner-arm.json" + +# Case 27 (Fix B): the legacy ARM spelling for a role assignment as a child +# resource type path (Microsoft.Storage/storageAccounts/providers/ +# roleAssignments) rather than a top-level roleAssignments resource with a +# scope property. Same grant, invisible to the same type-string match. +run_case "legacy child-type roleAssignments (Owner) exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/legacy-child-roleassignment-owner-arm.json" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/role-parity/legacy-child-roleassignment-owner-arm.json b/scripts/testdata/role-parity/legacy-child-roleassignment-owner-arm.json new file mode 100644 index 000000000..2bce079e1 --- /dev/null +++ b/scripts/testdata/role-parity/legacy-child-roleassignment-owner-arm.json @@ -0,0 +1,45 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Storage/storageAccounts/providers/roleAssignments", + "apiVersion": "2022-04-01", + "name": "cudlystorage/Microsoft.Authorization/owner-grant", + "properties": { + "roleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +} diff --git a/scripts/testdata/role-parity/miscased-assignablescopes-arm.json b/scripts/testdata/role-parity/miscased-assignablescopes-arm.json new file mode 100644 index 000000000..0598ec9a1 --- /dev/null +++ b/scripts/testdata/role-parity/miscased-assignablescopes-arm.json @@ -0,0 +1,65 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role-miscased-key", + "properties": { + "roleName": "CUDly Test Role Miscased Key", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "AssignableScopes": [ + "/providers/Microsoft.Capacity" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/miscased-properties-arm.json b/scripts/testdata/role-parity/miscased-properties-arm.json new file mode 100644 index 000000000..22dd483c0 --- /dev/null +++ b/scripts/testdata/role-parity/miscased-properties-arm.json @@ -0,0 +1,45 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "name": "owner-grant-miscased-properties-key", + "Properties": { + "roleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +} diff --git a/scripts/testdata/role-parity/miscased-roledefinitionid-arm.json b/scripts/testdata/role-parity/miscased-roledefinitionid-arm.json new file mode 100644 index 000000000..6796472df --- /dev/null +++ b/scripts/testdata/role-parity/miscased-roledefinitionid-arm.json @@ -0,0 +1,45 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "name": "owner-grant-miscased-roledefinitionid-key", + "properties": { + "RoleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +} diff --git a/scripts/testdata/role-parity/miscased-scope-arm.json b/scripts/testdata/role-parity/miscased-scope-arm.json new file mode 100644 index 000000000..819979f7f --- /dev/null +++ b/scripts/testdata/role-parity/miscased-scope-arm.json @@ -0,0 +1,44 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "Scope": "/providers/Microsoft.Capacity", + "name": "tenant-wide-miscased-scope-key", + "properties": { + "principalId": "00000000-0000-0000-0000-0000000000aa" + } + } + ] +} diff --git a/scripts/testdata/role-parity/pim-roleassignment-schedule-owner-arm.json b/scripts/testdata/role-parity/pim-roleassignment-schedule-owner-arm.json new file mode 100644 index 000000000..c23c90b9a --- /dev/null +++ b/scripts/testdata/role-parity/pim-roleassignment-schedule-owner-arm.json @@ -0,0 +1,50 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignmentScheduleRequests", + "apiVersion": "2022-04-01-preview", + "name": "pim-owner-assignment-schedule", + "properties": { + "roleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "requestType": "AdminAssign", + "scheduleInfo": { + "expiration": { + "type": "NoExpiration" + } + } + } + } + ] +} diff --git a/scripts/testdata/role-parity/pim-roleeligibility-owner-arm.json b/scripts/testdata/role-parity/pim-roleeligibility-owner-arm.json new file mode 100644 index 000000000..0893d8ee2 --- /dev/null +++ b/scripts/testdata/role-parity/pim-roleeligibility-owner-arm.json @@ -0,0 +1,50 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleEligibilityScheduleRequests", + "apiVersion": "2022-04-01-preview", + "name": "pim-owner-eligibility", + "properties": { + "roleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "requestType": "AdminAssign", + "scheduleInfo": { + "expiration": { + "type": "NoExpiration" + } + } + } + } + ] +} From 61027bd53fbe299dee256559cbb4f92a151fde4d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:48:15 +0200 Subject: [PATCH 06/10] sec(iac): explicit case-folding for scope/roleDefinitionId comparisons 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. --- scripts/check-azure-role-parity.sh | 24 +++++++++- scripts/test-azure-role-parity.sh | 17 +++++++ ...ppercase-allowed-roledefinitionid-arm.json | 45 +++++++++++++++++++ .../uppercase-canonical-scope-arm.json | 35 +++++++++++++++ 4 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 scripts/testdata/role-parity/uppercase-allowed-roledefinitionid-arm.json create mode 100644 scripts/testdata/role-parity/uppercase-canonical-scope-arm.json diff --git a/scripts/check-azure-role-parity.sh b/scripts/check-azure-role-parity.sh index 6e1221875..9f73a898a 100755 --- a/scripts/check-azure-role-parity.sh +++ b/scripts/check-azure-role-parity.sh @@ -259,6 +259,18 @@ CANONICAL_SCOPE_EXPR_ALT="[subscription().id]" # blurred that line. Punctuation-adjacent collapsing can't reach inside a # literal's content, because that content by construction contains no # whitespace next to `[`, `]`, `(`, `)`, or `,`. +# +# Also lowercased as an explicit final step, not left to the ambient +# `shopt -s nocasematch` the comparison happens to run under: ARM function +# names, property accessors, and resource-path segments (subscription IDs, +# provider namespaces) are all documented case-insensitive, the same reason +# resource `type` values are matched via ascii_downcase elsewhere in this +# script. Folding case here explicitly, inside the function whose whole job +# is "normalize equivalent spellings," keeps that guarantee from silently +# depending on unrelated code (the roleDefinitionId loop below, or the +# ESCAPE_TOKENS match) staying inside the same shell-option scope. Safe to +# do unconditionally: unlike whitespace-stripping, lowercasing is a 1:1 +# character transform that can't collapse two distinct values into one. normalize_scope_expr() { local v="$1" v="$(printf '%s' "$v" | sed -E 's/^[[:space:]]+//; s/[[:space:]]+$//')" # trim the whole value only @@ -268,7 +280,8 @@ normalize_scope_expr() { s/\([[:space:]]+/(/g; s/[[:space:]]+\)/)/g; s/,[[:space:]]+/,/g; s/[[:space:]]+,/,/g; ')" - printf '%s' "$v" | sed -E "s/,''\)\]\$/)]/" # drop a redundant ,'' arg + v="$(printf '%s' "$v" | sed -E "s/,''\)\]\$/)]/")" # drop a redundant ,'' arg + printf '%s' "$v" | tr '[:upper:]' '[:lower:]' } CANONICAL_SCOPE_NORM="$(normalize_scope_expr "$CANONICAL_SCOPE_EXPR")" CANONICAL_SCOPE_ALT_NORM="$(normalize_scope_expr "$CANONICAL_SCOPE_EXPR_ALT")" @@ -419,9 +432,16 @@ while IFS=$'\t' read -r origin value; do # for one here. reason="role assignments must inherit the deployment scope, not set one" elif [[ "$origin" == "roleAssignment.roleDefinitionId" ]]; then + # Compared explicitly lowercased, not left to the ambient nocasematch + # this loop happens to run under: the same "ARM identifiers are + # case-insensitive" reasoning as normalize_scope_expr above, made + # explicit here too so it doesn't silently depend on this comparison + # staying inside that shell-option scope. allowed_match=0 + value_lower="$(printf '%s' "$value" | tr '[:upper:]' '[:lower:]')" for allowed in "${ALLOWED_ROLE_DEFINITION_IDS[@]}"; do - if [[ "$value" == "$allowed" ]]; then + allowed_lower="$(printf '%s' "$allowed" | tr '[:upper:]' '[:lower:]')" + if [[ "$value_lower" == "$allowed_lower" ]]; then allowed_match=1 break fi diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index e2bec6466..15dda387e 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -262,6 +262,23 @@ run_case "legacy child-type roleAssignments (Owner) exits 1" 1 \ --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ --arm-file "${FIXTURES}/legacy-child-roleassignment-owner-arm.json" +# Case 28: the canonical scope expression written with uppercase ARM function +# and property names (CONCAT / SUBSCRIPTION / SUBSCRIPTIONID). ARM identifiers +# are case-insensitive the same way resource types are; normalize_scope_expr +# now folds case explicitly rather than relying on the ambient nocasematch +# scope, so this must still be accepted as canonical. +run_case "uppercase canonical scope expression exits 0" 0 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/uppercase-canonical-scope-arm.json" + +# Case 29: an allowed roleDefinitionId ("[Variables('Roles').Reader]") written +# with different capitalization than ALLOWED_ROLE_DEFINITION_IDS's own +# spelling. Must still match -- same reasoning, made explicit rather than +# implicit in the roleDefinitionId comparison loop. +run_case "case-varied allowed roleDefinitionId exits 0" 0 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/uppercase-allowed-roledefinitionid-arm.json" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/role-parity/uppercase-allowed-roledefinitionid-arm.json b/scripts/testdata/role-parity/uppercase-allowed-roledefinitionid-arm.json new file mode 100644 index 000000000..2fdcdfed0 --- /dev/null +++ b/scripts/testdata/role-parity/uppercase-allowed-roledefinitionid-arm.json @@ -0,0 +1,45 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "name": "reader-grant-uppercase-expr", + "properties": { + "roleDefinitionId": "[Variables('Roles').Reader]", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +} diff --git a/scripts/testdata/role-parity/uppercase-canonical-scope-arm.json b/scripts/testdata/role-parity/uppercase-canonical-scope-arm.json new file mode 100644 index 000000000..7e0acee8f --- /dev/null +++ b/scripts/testdata/role-parity/uppercase-canonical-scope-arm.json @@ -0,0 +1,35 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[CONCAT('/subscriptions/', SUBSCRIPTION().SUBSCRIPTIONID)]" + ] + } + } + ] +} From ac1d2589be5518ca21c6e60915da81a13f33fd4f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 13:24:10 +0200 Subject: [PATCH 07/10] test(iac/azure): use a real HCL reference in the TF-flag regression Case 9 asserts check-azure-role-parity.sh fails CLOSED when the TF module references include_capacity_provider_scope but variables.tf is absent. It appended only a comment mentioning the flag name, so it passed solely because the checker's trigger is a plain `grep -q` that also matches comment text -- it proved nothing about a real Terraform reference and would have stayed green if the trigger were ever tightened to skip comments. Append a `locals` block wiring var.include_capacity_provider_scope the way the real module does (terraform/modules/iam/azure/cudly-reservation-role/ main.tf:68-71) instead. A `locals` block rather than a second azurerm_role_definition: that resource requires a `permissions` block whose actions extract_tf_list would pick up, making the case fail on the actions axis rather than on the missing variables.tf. Verified by mutation rather than by the case passing: - guard reverted to the pre-fix "skip the assertion when variables.tf is absent" form -> checker exits 0, case FAILS (expected 1, got 0) - trigger made comment-aware (`grep -qE '^[^#]*...'`) -> the old comment-only fixture exits 0 (vacuous), the new fixture exits 1 Suite: 29 passed, 0 failed. shellcheck and bash -n clean (exit 0) on both scripts. Refs #1545 --- scripts/test-azure-role-parity.sh | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index 15dda387e..8c61febf7 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -93,7 +93,26 @@ run_case "management-group schema ARM exits 1" 1 \ 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" +# A GENUINE HCL reference, not a comment mentioning the name. An earlier +# revision appended only `# assignable_scopes uses include_capacity_provider_scope`, +# which made this case prove nothing about a real reference: it passed solely +# because the checker's trigger is a plain `grep -q` that also matches comment +# text. The block below mirrors how the real module wires the flag +# (terraform/modules/iam/azure/cudly-reservation-role/main.tf:68-71). +# +# Deliberately a `locals` block rather than a second azurerm_role_definition: +# that resource requires a `permissions { ... }` block, whose actions would be +# picked up by extract_tf_list and red the actions axis, making this case fail +# for an unrelated reason instead of the missing variables.tf. +cat >> "${TMP_TF_DIR}/main.tf" <<'HCL' + +locals { + capacity_assignable_scopes = compact([ + "/subscriptions/00000000-0000-0000-0000-000000000001", + var.include_capacity_provider_scope ? "/providers/Microsoft.Capacity" : "", + ]) +} +HCL run_case "TF flag without variables.tf exits 1" 1 \ --tf-file "${TMP_TF_DIR}/main.tf" \ --arm-file "${FIXTURES}/matching-arm.json" From 8ebcdb18b5045c8d0c174ddc92454a762bb46574 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 13:27:27 +0200 Subject: [PATCH 08/10] docs(known-issues): make the #1545 revocation check able to see a tenant grant Step 1 of the remediation runbook used a single `az role assignment list --all` query filtered by JMESPath `contains(scope, 'Microsoft.Capacity')`. Both halves of that could return an empty result while an over-broad grant was still live, which on a p0 revocation runbook reads as an all-clear: - `--all` is documented by the CLI (2.88.0) as "show all assignments under the current subscription". /providers/Microsoft.Capacity sits outside any subscription, so a surviving tenant-wide assignment -- the exact thing this step exists to find -- never appears. It is not a parent scope of the subscription either, so --include-inherited does not surface it. - JMESPath contains() is case-sensitive; ARM provider namespaces are not. A row stored as /providers/microsoft.capacity is a fully functional grant that silently fails the filter. Split into two queries, because neither shape is visible to the other: 1a. --scope "/providers/Microsoft.Capacity" for the tenant-level path. Everything returned is over-broad by definition, so there is no filter to get wrong. Notes that reading this scope needs tenant-level rights and that an authorization error is not an all-clear. 1b. --all for the subscription and below, where the malformed doubled-providers target would land, filtered with `grep -i` instead of a case-sensitive JMESPath match. Projected as a JMESPath list rather than a hash so the tsv column order is fixed by the query and step 2 can delete by the first column. The two are kept as separate invocations rather than combined: --all and --scope have historically been mutually exclusive in the CLI. Refs #1545 --- known-issues.md | 45 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 7 deletions(-) diff --git a/known-issues.md b/known-issues.md index 04331fc8f..28abb8511 100644 --- a/known-issues.md +++ b/known-issues.md @@ -54,15 +54,44 @@ Redeploying alone is not sufficient. Remediation, in this order: ```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. +# 1. Check what the pre-fix template actually left behind. This takes TWO +# queries, because neither one alone can see both shapes described above. +# Project the assignment id in each: it is what step 2 deletes by. +# +# 1a. The tenant-level provider path. `--all` cannot reach this: the CLI +# documents it as "show all assignments under the current subscription", +# and /providers/Microsoft.Capacity sits outside any subscription, so a +# surviving tenant-wide grant would not appear in 1b at all. Nor is it a +# parent scope of the subscription, so --include-inherited does not +# surface it either. Query the scope directly. Anything returned here is +# over-broad by definition, so there is no filter to get wrong. +# Reading at this scope needs tenant-level rights (User Access +# Administrator at tenant root, or Global Administrator with elevated +# access); an authorization error here is NOT an all-clear -- re-run it +# with a principal that can read the scope. az role assignment list \ --assignee \ - --all \ - --query "[?contains(scope, 'Microsoft.Capacity')].{id:id, scope:scope, role:roleDefinitionName}" \ + --scope "/providers/Microsoft.Capacity" \ + --query "[].{id:id, scope:scope, role:roleDefinitionName}" \ -o table +# 1b. The subscription and below, which is where the malformed +# doubled-providers target shown above would land. Run once per onboarded +# subscription (`az account set --subscription ` between runs). +# Filtered with `grep -i`, not a JMESPath `--query "[?contains(...)]"`: +# JMESPath's contains() is case-sensitive, while ARM provider namespaces +# are not, so a row stored as /providers/microsoft.capacity satisfies the +# grant and silently fails the filter. Do not reintroduce a +# case-sensitive path match here. +# Projected as a JMESPath list, not a hash, so the tsv column order is +# fixed by the query rather than by key ordering: id, scope, role. Step 2 +# deletes by the FIRST column. +az role assignment list \ + --assignee \ + --all \ + --query "[].[id, scope, roleDefinitionName]" \ + -o tsv | grep -i 'microsoft\.capacity' + # 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 @@ -79,8 +108,10 @@ az deployment sub create \ --no-prompt ``` -Step 1 returning nothing is a good outcome, and the expected one if the -malformed target described above simply failed to apply. Step 3 is still +Both step-1 queries returning nothing is a good outcome, and the expected one +if the malformed target described above simply failed to apply. Only 1a and 1b +together are a clean result: 1b alone cannot see a tenant-level grant, and 1a +alone cannot see the malformed subscription-relative one. Step 3 is still required either way: it is what removes the tenant entry from `assignableScopes`. From ed8e6a3ffdae211278620a012df1f2a987931870 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 14:20:38 +0200 Subject: [PATCH 09/10] sec(iac/azure): refuse ARM templates with case-variant key collisions check-azure-role-parity.sh's key-casing normalization (with_entries(.key |= ascii_downcase)) closed the single-miscased-key bypass by folding every object key to lowercase before any selector runs. But jq's from_entries (which with_entries is built on) keeps the LAST entry when two entries fold to the same key, so an object that already carries BOTH case-variant spellings of a property in the same object -- not one miscased key, but two keys -- collapses to whichever is spelled last. The discarded value is deleted before any selector runs, not merely unmatched. A template with the hostile value spelled first and a correctly-spelled, canonical-looking value spelled second in the same object was therefore invisible: the normalizer threw away the evidence before the scope check, the roleDefinitionId check, and REFUSED_TYPES ever ran. Reproduced against four shapes, each a live bypass of a distinct part of the guard: AssignableScopes/assignableScopes (issue #1545 itself), RoleDefinitionId/ roleDefinitionId (built-in Owner grant), Properties/properties (same, wrapped one level up), and Type/type (also evades REFUSED_TYPES, since the resource ends up looking like an ordinary compliant roleAssignment). Pre-existing, not introduced by any change on this branch -- the same four fixtures exit 0 against 99ea68759 too. Order-dependence confirms the mechanism and rules out a coincidental fix: the same collision with the two keys reversed (benign-first, evil-last) already exits 1 today, because the surviving (evil) value trips the pre-existing tenant-scope check. Fix: detect any same-object case-variant key collision on the raw file, before normalization, and refuse outright rather than pick a winner. Which of the two spellings ARM itself honors for a duplicate property in one object is not verified here and is not something this script can determine from the file alone -- unlike the single-miscased-key case, where ARM's documented case-insensitive deserialization justifies treating the miscased spelling as equivalent to the correct one. If ARM takes the first entry, this is a live escalation; if it takes the last, this script's prior behavior was coincidentally right rather than correctly reasoned about. A template ambiguous about its own grants must not be the thing that decides whether CI is green. Verified: the new collision check returns 0 on the real arm/CUDly-CrossSubscription/template.json and every one of the 28 pre-existing role-parity fixtures (no false positives), and 1 on all five new fixtures. Five new regression fixtures added, one per bypass shape plus the benign-first control, each confirmed to fail for its own reason via its printed reason line. Full suite: 34/34 pass (29 pre-existing + 5 new). bash -n and shellcheck 0.11.0 clean on both scripts. --- scripts/check-azure-role-parity.sh | 62 +++++++++++++++++++ scripts/test-azure-role-parity.sh | 53 ++++++++++++++++ ...ion-assignablescopes-benign-first-arm.json | 38 ++++++++++++ ...ision-assignablescopes-evil-first-arm.json | 38 ++++++++++++ .../collision-properties-evil-first-arm.json | 50 +++++++++++++++ ...ision-roledefinitionid-evil-first-arm.json | 46 ++++++++++++++ .../collision-type-evil-first-arm.json | 46 ++++++++++++++ 7 files changed, 333 insertions(+) create mode 100644 scripts/testdata/role-parity/collision-assignablescopes-benign-first-arm.json create mode 100644 scripts/testdata/role-parity/collision-assignablescopes-evil-first-arm.json create mode 100644 scripts/testdata/role-parity/collision-properties-evil-first-arm.json create mode 100644 scripts/testdata/role-parity/collision-roledefinitionid-evil-first-arm.json create mode 100644 scripts/testdata/role-parity/collision-type-evil-first-arm.json diff --git a/scripts/check-azure-role-parity.sh b/scripts/check-azure-role-parity.sh index 9f73a898a..031539878 100755 --- a/scripts/check-azure-role-parity.sh +++ b/scripts/check-azure-role-parity.sh @@ -61,6 +61,68 @@ if ! command -v jq &>/dev/null; then exit 2 fi +# --- refuse ambiguous same-object key collisions, before normalization ------- +# The normalization step just below folds every object key to lowercase with +# `with_entries(.key |= ascii_downcase)`. jq's `from_entries` (which +# `with_entries` is built on) keeps the LAST entry for a given key when two +# entries produce the same key, so an object that already carries two +# case-variant spellings of the same property in the SAME object -- e.g. both +# "AssignableScopes" (ARM's tenant-wide grant) and "assignableScopes" (the +# canonical one) -- collapses to whichever one is spelled LAST in the file. +# Every selector below runs against the normalized copy, so the discarded +# value is invisible to the entire rest of this script: not merely unmatched +# by a case-sensitive selector (that failure mode is what the normalization +# step above exists to close), but deleted before any selector runs. A +# template carrying the hostile value first and a correctly-spelled, +# canonical-looking value second passes clean. +# +# This is checked BEFORE $ARM_FILE is normalized, against the raw file: by +# the time normalization has run, the collision has already been resolved and +# there is nothing left to detect. +# +# Refuse rather than pick a winner. Which of two case-variant keys ARM itself +# honors for a duplicate property in the same JSON object is not verified +# here (doing so would require deploying a miscased template to a live +# tenant) and is not something this script can determine from the file +# alone -- unlike the single-miscased-key case above, where ARM's documented +# case-insensitive deserialization justifies treating the miscased spelling +# as equivalent to the correct one. If ARM takes the first entry, a hostile +# value ahead of a benign one is a live escalation this script would +# otherwise report as clean. If ARM takes the last entry the same way jq +# does, this script's current behavior is coincidentally right, not +# correctly reasoned about -- and either way, a template that is ambiguous +# about its own grants must not be the thing that decides whether CI is +# green. +COLLISION_DETAIL=$(jq -r ' + [.resources // [] | .. | objects + | . as $obj + | ($obj | keys_unsorted) as $keys + | ($keys | group_by(ascii_downcase) | map(select(length > 1))) as $collisions + | select($collisions | length > 0) + | { type: ($obj.type // $obj.Type // ""), + name: ($obj.name // $obj.Name // ""), + colliding_key_groups: ($collisions | map(join(" / "))) } + ] + | .[] + | " resource type=" + .type + " name=" + .name + ": " + (.colliding_key_groups | join(", ")) +' "$ARM_FILE") + +if [[ -n "$COLLISION_DETAIL" ]]; then + echo "ERROR: ARM template contains an object with two case-variant spellings of" >&2 + echo " the same property key. Which one ARM itself honors is not something" >&2 + echo " this script can determine, and this is exactly the shape of issue" >&2 + echo " #1545: a hostile value spelled first and a benign, canonical-looking" >&2 + echo " value spelled second in the same object is deleted by this script's" >&2 + echo " own key-casing normalization before any check below ever sees it." >&2 + echo "" >&2 + echo " ARM source: $ARM_FILE" >&2 + echo "" >&2 + echo "$COLLISION_DETAIL" >&2 + echo "" >&2 + echo "Remove the duplicate spelling and keep exactly one key per property." >&2 + exit 1 +fi + # --- normalize ARM property-key casing --------------------------------------- # Azure Resource Manager's resource-provider JSON deserializers are documented # as case-insensitive by default, so a resource typed correctly but with a diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index 8c61febf7..c0e0db0e5 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -298,6 +298,59 @@ run_case "case-varied allowed roleDefinitionId exits 0" 0 \ --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ --arm-file "${FIXTURES}/uppercase-allowed-roledefinitionid-arm.json" +# --- round-5: adversarial-review key-collision bypass ----------------------- +# Round 4's key-casing normalization (`with_entries(.key |= ascii_downcase)`) +# closed the single-miscased-key bypass, but `from_entries` (which +# `with_entries` is built on) keeps the LAST entry when two entries produce +# the same key. An object that already carries BOTH case-variant spellings of +# a property in the same object -- not just one miscased key, but two keys -- +# collapses to whichever is spelled last, and the discarded value is deleted +# before any selector runs: not unmatched, gone. A template with the hostile +# value first and a benign, canonical-looking value second was invisible. +# +# Cases 30-33 are one collision shape each, all evil-first (the hostile value +# spelled first, discarded on normalization), all against the fix added in +# this round; each must now be refused by name, not incidentally. Case 34 is +# the benign-first control: the same collision with the two keys reversed, so +# the hostile value survives normalization instead and the PRE-EXISTING +# tenant-scope check already caught it -- proving the bypass really is +# order-dependent (only evil-first was silently admitted) and that the new +# refusal does not regress an already-caught shape. + +# Case 30: AssignableScopes (evil, tenant-wide) then assignableScopes (benign, +# canonical) on the same roleDefinitions.properties object -- issue #1545 +# itself, reached via key collision instead of a single miscased key. +run_case "key-collision AssignableScopes/assignableScopes exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/collision-assignablescopes-evil-first-arm.json" + +# Case 31: RoleDefinitionId (evil, built-in Owner) then roleDefinitionId +# (benign, allowed Reader) on the same roleAssignment.properties object. +run_case "key-collision RoleDefinitionId/roleDefinitionId exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/collision-roledefinitionid-evil-first-arm.json" + +# Case 32: Properties (evil, wraps Owner) then properties (benign, wraps +# allowed Reader) as two TOP-LEVEL keys on the same roleAssignment resource. +run_case "key-collision Properties/properties exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/collision-properties-evil-first-arm.json" + +# Case 33: Type (evil, Microsoft.Resources/deployments) then type (benign, +# Microsoft.Authorization/roleAssignments) as two TOP-LEVEL keys. If the +# collision drops the "Type"/deployments evidence, the resource looks like an +# ordinary, fully-compliant roleAssignments grant -- invisible not just to +# REFUSED_TYPES but to every other check too, which is why this fixture +# carries an otherwise-allowed roleDefinitionId and no scope override. +run_case "key-collision Type/type (evades REFUSED_TYPES too) exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/collision-type-evil-first-arm.json" + +# Case 34: benign-first control -- same collision as case 30, keys reversed. +run_case "key-collision benign-first control still exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/collision-assignablescopes-benign-first-arm.json" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/role-parity/collision-assignablescopes-benign-first-arm.json b/scripts/testdata/role-parity/collision-assignablescopes-benign-first-arm.json new file mode 100644 index 000000000..a4793ccc8 --- /dev/null +++ b/scripts/testdata/role-parity/collision-assignablescopes-benign-first-arm.json @@ -0,0 +1,38 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ], + "AssignableScopes": [ + "/providers/Microsoft.Capacity" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/collision-assignablescopes-evil-first-arm.json b/scripts/testdata/role-parity/collision-assignablescopes-evil-first-arm.json new file mode 100644 index 000000000..06c03d9ca --- /dev/null +++ b/scripts/testdata/role-parity/collision-assignablescopes-evil-first-arm.json @@ -0,0 +1,38 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "AssignableScopes": [ + "/providers/Microsoft.Capacity" + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/collision-properties-evil-first-arm.json b/scripts/testdata/role-parity/collision-properties-evil-first-arm.json new file mode 100644 index 000000000..ea1fac29b --- /dev/null +++ b/scripts/testdata/role-parity/collision-properties-evil-first-arm.json @@ -0,0 +1,50 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "name": "owner-grant-properties-collision", + "Properties": { + "roleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + }, + "properties": { + "roleDefinitionId": "[variables('roles').reader]", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +} diff --git a/scripts/testdata/role-parity/collision-roledefinitionid-evil-first-arm.json b/scripts/testdata/role-parity/collision-roledefinitionid-evil-first-arm.json new file mode 100644 index 000000000..e617dba27 --- /dev/null +++ b/scripts/testdata/role-parity/collision-roledefinitionid-evil-first-arm.json @@ -0,0 +1,46 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "name": "owner-grant-roledefinitionid-collision", + "properties": { + "RoleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "roleDefinitionId": "[variables('roles').reader]", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +} diff --git a/scripts/testdata/role-parity/collision-type-evil-first-arm.json b/scripts/testdata/role-parity/collision-type-evil-first-arm.json new file mode 100644 index 000000000..76b630cfb --- /dev/null +++ b/scripts/testdata/role-parity/collision-type-evil-first-arm.json @@ -0,0 +1,46 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "Type": "Microsoft.Resources/deployments", + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "name": "roleassignment-type-collision", + "properties": { + "roleDefinitionId": "[variables('roles').reader]", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +} From 34b4138b862c0d57f8bc5eda00a4bfa18e674b61 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 18:32:50 +0200 Subject: [PATCH 10/10] sec(iac): scan root object and every provider for legacy child-scoped roleAssignments CodeRabbit found two gaps in the round-5 fixes to check-azure-role-parity.sh: - The key-collision scan (line 96) was rooted at .resources, so it never treated the root document object itself as a candidate for collision. A template with both "Resources" (evil, first) and "resources" (benign, last) at the top level folds to the benign array during normalization (last-entry-wins), the same silent-discard bypass this script exists to refuse, just one level higher in the document tree. Fixed by including the root object (.) alongside the .resources walk. - REFUSED_TYPES named only microsoft.storage/storageaccounts/providers/ roleassignments for the legacy child-scoped role-assignment spelling, so any other parent (Microsoft.KeyVault/vaults, Microsoft.Resources/ resourceGroups, etc.) granting the same way via the same */providers/roleAssignments shape passed unrefused. Fixed by matching the suffix instead of enumerating parents, the same pattern ESCAPE_TOKENS already uses for managementGroups. Both fixes preserve the ordering invariant this script depends on: collision detection still runs on the raw file before normalization. Added two permanent regression fixtures under scripts/testdata/role-parity/ and their test cases (35, 36), following the existing per-finding pattern. --- scripts/check-azure-role-parity.sh | 36 ++++++--- scripts/test-azure-role-parity.sh | 28 +++++++ ...llision-root-resources-evil-first-arm.json | 77 +++++++++++++++++++ ...ild-keyvault-roleassignment-owner-arm.json | 45 +++++++++++ 4 files changed, 177 insertions(+), 9 deletions(-) create mode 100644 scripts/testdata/role-parity/collision-root-resources-evil-first-arm.json create mode 100644 scripts/testdata/role-parity/legacy-child-keyvault-roleassignment-owner-arm.json diff --git a/scripts/check-azure-role-parity.sh b/scripts/check-azure-role-parity.sh index 031539878..312e18dde 100755 --- a/scripts/check-azure-role-parity.sh +++ b/scripts/check-azure-role-parity.sh @@ -93,8 +93,19 @@ fi # correctly reasoned about -- and either way, a template that is ambiguous # about its own grants must not be the thing that decides whether CI is # green. +# +# Scanned objects are the root document itself, plus everything under +# `.resources`, not `.resources` alone: the normalization step below is +# `walk(...)` from the true root, so a root-level collision -- e.g. both +# "resources" and "Resources", or both "$schema" and "$Schema" -- is folded +# by the SAME last-entry-wins rule and is just as invisible to every +# selector below once normalized. A hostile "Resources" array spelled ahead +# of a benign "resources" array at the top of the file is this same bypass +# one level higher in the document tree, and was unreachable by a scan +# rooted at `.resources` because that scan never treats the root object +# itself as one of the objects being inspected for collisions. COLLISION_DETAIL=$(jq -r ' - [.resources // [] | .. | objects + [., (.resources // [] | ..) | objects | . as $obj | ($obj | keys_unsorted) as $keys | ($keys | group_by(ascii_downcase) | map(select(length > 1))) as $collisions @@ -408,11 +419,17 @@ fi # match, so the roleDefinitionId allowlist above is bypassed simply by # changing the resource type. # -# storageAccounts/providers/roleAssignments -- the legacy ARM spelling for -# a role assignment as a child resource (a full `.../providers/...` type -# path) rather than a separate top-level roleAssignments resource with a -# `scope` property. Same grant, invisible to the same selectors for the -# same reason. +# */providers/roleAssignments -- the legacy ARM spelling for a role +# assignment as a child resource (a full `.../providers/...` type path) +# rather than a separate top-level roleAssignments resource with a `scope` +# property. Same grant, invisible to the same selectors for the same +# reason, under ANY parent resource type -- storageAccounts is only the +# example issue #1545 happened to use. Matched by suffix rather than +# enumerated per parent type, the same way ESCAPE_TOKENS above matches +# `managementGroups` independently of its provider spelling: an allowlist +# naming one parent (e.g. only microsoft.storage/storageaccounts) leaves +# every other parent's `.../providers/roleAssignments` child free to grant +# silently, and there is no fixed set of parent types to enumerate here. # # Rooted at `.resources`, same reasoning as extract_arm_list above: a # decorative object elsewhere in the template (variables, outputs) is never @@ -422,12 +439,13 @@ REFUSED_TYPES='["microsoft.resources/deployments", "microsoft.resources/deploymentscripts", "microsoft.resources/deploymentstacks", "microsoft.authorization/roleeligibilityschedulerequests", - "microsoft.authorization/roleassignmentschedulerequests", - "microsoft.storage/storageaccounts/providers/roleassignments"]' + "microsoft.authorization/roleassignmentschedulerequests"]' REFUSED_TYPE_COUNT=$( jq --argjson types "$REFUSED_TYPES" ' [.resources // [] | .. | objects | select(has("type")) | select((.type|type) == "string") - | select((.type|ascii_downcase) as $t | $types | index($t) != null)] + | select((.type|ascii_downcase) as $t + | ($types | index($t) != null) + or ($t | endswith("/providers/roleassignments")))] | length ' "$ARM_FILE_NORM" ) diff --git a/scripts/test-azure-role-parity.sh b/scripts/test-azure-role-parity.sh index c0e0db0e5..1dbb5a244 100755 --- a/scripts/test-azure-role-parity.sh +++ b/scripts/test-azure-role-parity.sh @@ -351,6 +351,34 @@ run_case "key-collision benign-first control still exits 1" 1 \ --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ --arm-file "${FIXTURES}/collision-assignablescopes-benign-first-arm.json" +# --- round-6: adversarial-review, root scope and refusal-list gaps ---------- +# Two gaps CodeRabbit found in round 5's own fixes: the key-collision scan +# only looked inside `.resources`, and the legacy-child-roleAssignment +# refusal only named one parent resource type. + +# Case 35: a root-level case-variant key collision -- "Resources" (evil, +# carries the tenant-wide grant from issue #1545) spelled first, "resources" +# (benign, canonical) spelled last. The round-5 collision scan was rooted at +# `.resources`, so it never treated the root document object itself as one of +# the objects being inspected; normalization then folds the two root keys the +# same last-entry-wins way as any other collision, silently keeping only the +# benign array and discarding the evil one before any downstream check runs. +run_case "root-level key-collision Resources/resources exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/collision-root-resources-evil-first-arm.json" + +# Case 36 (Fix B follow-up): the same legacy child-scoped roleAssignment shape +# as case 27, under Microsoft.KeyVault/vaults instead of +# Microsoft.Storage/storageAccounts. REFUSED_TYPES named only the storage +# parent by exact string, so any other parent's +# "*/providers/roleAssignments" child -- granting the same way, invisible to +# the same roleAssignments-only type match -- passed silently. Matched by +# suffix now, the same way ESCAPE_TOKENS matches managementGroups independently +# of its provider spelling. +run_case "legacy child-type roleAssignments under non-storage parent exits 1" 1 \ + --tf-file "${FIXTURES}/matching-tf.tf.fixture" \ + --arm-file "${FIXTURES}/legacy-child-keyvault-roleassignment-owner-arm.json" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/role-parity/collision-root-resources-evil-first-arm.json b/scripts/testdata/role-parity/collision-root-resources-evil-first-arm.json new file mode 100644 index 000000000..28d5bdff4 --- /dev/null +++ b/scripts/testdata/role-parity/collision-root-resources-evil-first-arm.json @@ -0,0 +1,77 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "Resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "/subscriptions/00000000-0000-0000-0000-000000000001", + "/providers/Microsoft.Capacity" + ] + } + }, + { + "type": "Microsoft.Authorization/roleAssignments", + "apiVersion": "2022-04-01", + "scope": "/providers/Microsoft.Capacity", + "name": "tenant-wide", + "properties": { + "principalId": "00000000-0000-0000-0000-0000000000aa" + } + } + ], + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + } + ] +} diff --git a/scripts/testdata/role-parity/legacy-child-keyvault-roleassignment-owner-arm.json b/scripts/testdata/role-parity/legacy-child-keyvault-roleassignment-owner-arm.json new file mode 100644 index 000000000..5065983d9 --- /dev/null +++ b/scripts/testdata/role-parity/legacy-child-keyvault-roleassignment-owner-arm.json @@ -0,0 +1,45 @@ +{ + "$schema": "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + "resources": [ + { + "type": "Microsoft.Authorization/roleDefinitions", + "apiVersion": "2022-04-01", + "name": "test-role", + "properties": { + "roleName": "CUDly Test Role", + "type": "CustomRole", + "permissions": [ + { + "actions": [ + "Microsoft.Capacity/register/action", + "Microsoft.Capacity/calculatePrice/action", + "Microsoft.Capacity/catalogs/read", + "Microsoft.Capacity/reservationOrders/read", + "Microsoft.Capacity/reservationOrders/write", + "Microsoft.Capacity/reservationOrders/purchase/action", + "Microsoft.Capacity/reservationOrders/reservations/read", + "Microsoft.BillingBenefits/savingsPlanOrderAliases/write", + "Microsoft.BillingBenefits/savingsPlanOrders/read", + "Microsoft.BillingBenefits/savingsPlanOrders/savingsPlans/read", + "Microsoft.BillingBenefits/savingsPlanOrders/action" + ], + "notActions": [] + } + ], + "assignableScopes": [ + "[concat('/subscriptions/', subscription().subscriptionId)]" + ] + } + }, + { + "type": "Microsoft.KeyVault/vaults/providers/roleAssignments", + "apiVersion": "2022-04-01", + "name": "cudlyvault/Microsoft.Authorization/owner-grant", + "properties": { + "roleDefinitionId": "/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635", + "principalId": "00000000-0000-0000-0000-0000000000aa", + "principalType": "ServicePrincipal" + } + } + ] +}