Skip to content

Azure role-parity guard: principalId unchecked; no assertion on expected grant set #1681

Description

@cristim

Problem

scripts/check-azure-role-parity.sh (the ARM/TF role-parity CI drift guard added for #1545, hardened across several review rounds in PR #1658) constrains which role and where, but never who. It also has no assertion on the template's expected set of grants — only a growing list of things it knows to refuse or recognize.

Finding 1 (primary): principalId is never checked

A fourth Microsoft.Authorization/roleAssignments resource binding the allowed custom purchaser role (passes the F6 roleDefinitionId allowlist), at the correctly inherited subscription scope (passes the scope check), to a hardcoded foreign principalId, passes the guard cleanly: valid ARM, actions in parity, canonical scope, allowed role. That role carries Microsoft.Capacity/reservationOrders/purchase/action — money-spending permission — and the guard never looks at who it's granted to.

This is the same reasoning that justified adding the ALLOWED_ROLE_DEFINITION_IDS allowlist in PR #1658 (constrain what can be granted), just not carried through to who it can be granted to. The only principal that should ever appear in this template is parameters('servicePrincipalObjectId') (the CUDly service principal the customer supplies at deploy time) — a hardcoded GUID literal anywhere in a principalId field is definitionally wrong for this template.

Finding 2 (systemic): the guard has no assertion on the expected set of grants

The real template emits exactly 4 grant tuples (1 custom role assignment + 2 built-in role assignments + 1 role definition's assignable scope). Every hostile fixture built during PR #1658's review rounds emitted 1 or 2 tuples and still printed OK: all 1 … / OK: all 2 … — the success message asserts a check happened when almost nothing was examined relative to the real template's shape.

Proposed fix: assert the template grants exactly the N known tuples (4, today), each matching an expected (type, roleDefinitionId-or-scope-list, principalId) shape, rather than iterating "is every tuple I found individually OK?". This closes finding 1 above and every enumerate-what-to-refuse finding from PR #1658's review rounds in one stroke, including ones nobody has thought of yet — a resource type this script has never heard of would fail the count/shape assertion automatically instead of silently passing.

Also noted (low severity, same script)

  • assignableScopes given as a bare JSON string instead of an array causes jq: error (...) Cannot iterate over string and the script exits 5 (a raw jq error surfacing through set -e) rather than a clean 1. Still fails closed — CI still reds — but the exit code is undocumented (the header comment says "Exit 1 = drift") and the message is not one of this script's own diagnostics. Low-cost fix: validate assignableScopes is an array before iterating it, with the script's own error format.
  • Several other malformed-input self-test variants in scripts/test-azure-role-parity.sh also exit 5 for the same underlying reason (a jq error surfacing through set -e on unexpected JSON shapes). Same fix would likely normalize all of them at once.

Context

Both items were identified during an independent adversarial review of PR #1658 (the #1545 fix + guard hardening) and deliberately deferred rather than folded into that PR, per reviewer guidance: the template diff in #1658 is the real #1545 remediation (removes the tenant-wide grant), and these are separate defense-in-depth gaps in the CI guard, not reasons to hold that fix.

Related: #1545 (closed by #1658), PR #1658.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions