fix(iac/azure): grant Key Vault access via RBAC role assignments

.github/workflows/ci.yml

@@ -736,6 +736,28 @@ jobs:
       - name: Run selector self-tests
         run: bash scripts/test-select-ecr-repos-to-delete.sh
 
+  # Assert that no Terraform file declares an azurerm_key_vault_access_policy
+  # resource. This project's only Key Vault sets enable_rbac_authorization =
+  # true, and an RBAC-enabled vault ignores access policies entirely, so such a
+  # grant applies cleanly and then does nothing at runtime (#1621). The nested
+  # `access_policy` block form is deliberately not covered; #1839 tracks it.
+  # Fast (shell only), so it always runs.
+  azure-kv-access-policy:
+    name: Azure Key Vault grant model
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1
+        with:
+          persist-credentials: false
+
+      - name: Assert no Key Vault access policies
+        run: bash scripts/check-azure-kv-access-policy.sh
+
+      - name: Run guard script self-tests
+        run: bash scripts/test-azure-kv-access-policy.sh
+
   # Summary job - all checks must pass
   ci-success:
     name: CI Success
@@ -752,6 +774,7 @@ jobs:
       - aws-iam-parity
       - gcp-secret-scope
       - ecr-delete-selection
+      - azure-kv-access-policy
     if: always()
 
     steps:

scripts/check-azure-kv-access-policy.sh

@@ -0,0 +1,170 @@
+#!/usr/bin/env bash
+# check-azure-kv-access-policy.sh
+#
+# Fails when any Terraform file declares a top-level
+# `azurerm_key_vault_access_policy` resource.
+#
+# WHY A BLANKET BAN IS CORRECT HERE. This repo provisions exactly one Key Vault
+# (terraform/modules/secrets/azure/main.tf) and it hardcodes
+# `enable_rbac_authorization = true` as a literal, not a variable. An
+# RBAC-enabled vault NEVER consults its accessPolicies array: data-plane
+# authorization comes from Azure RBAC role assignments and nothing else.
+#
+# The failure mode this guards is that Azure's control plane happily ACCEPTS the
+# access-policy write, so `terraform apply` reports success with no warning and
+# the grant is silently inert. It surfaces only as a runtime 403, which is the
+# worst possible place to discover it. That is exactly what happened to the
+# Azure cleanup-function and the AKS workload identity (issue #1621) while every
+# other consumer in the tree already used `azurerm_role_assignment`.
+#
+# The supported pattern is:
+#
+#   resource "azurerm_role_assignment" "..." {
+#     scope                = var.key_vault_id
+#     role_definition_name = "Key Vault Secrets User"
+#     principal_id         = <identity principal id>
+#   }
+#
+# Pick the role by mapping the access-policy permissions you would have written:
+#   secret_permissions Get/List          -> "Key Vault Secrets User"
+#   secret_permissions incl. Set/Delete  -> "Key Vault Secrets Officer"
+#   key_permissions Sign/Get             -> "Key Vault Crypto User"
+# Verify the role exists before using it (`az role definition list --name ...`);
+# a matching string in a sibling file is not evidence that a role or an action
+# is real (issue #1794).
+#
+# Exit 0 = no azurerm_key_vault_access_policy resource declared.
+# Exit 1 = at least one found; each is printed to stderr.
+# Exit 2 = usage error, or a file this scanner cannot read.
+#
+# LIMITATIONS. This is a textual guard, not a policy engine, and it bans exactly
+# one spelling of the grant: a `resource "azurerm_key_vault_access_policy"`
+# header, at any indentation and without requiring whitespace between the
+# tokens. That is the form the #1621 defect arrived in.
+#
+# It does NOT catch the nested spelling: an `access_policy { ... }` block (or its
+# `dynamic "access_policy"` equivalent) declared inside an `azurerm_key_vault`
+# resource, which expresses the same inert grant. Deciding whether such a header
+# is a real grant needs the type of the enclosing resource, which a line-oriented
+# matcher does not have -- three successive text matchers for it were each either
+# too narrow to see a real block or broad enough to fire on an unrelated
+# identifier. Issue #1839 tracks closing that gap with an HCL parse
+# (hclparse/hclsyntax, or `terraform show -json`) instead of a fourth regex.
+# Neither nested form appears anywhere under the scan roots today.
+#
+# The Terraform JSON encoding (`.tf.json`) is not parsed; rather than
+# report such a file as clean, the guard exits 2. None exist today.
+#
+# Usage:
+#   scripts/check-azure-kv-access-policy.sh              # scan the default roots
+#   scripts/check-azure-kv-access-policy.sh FILE...      # scan exactly these files
+#
+# Passing explicit files is how the test harness points the check at fixtures
+# without touching the real sources.
+
+set -euo pipefail
+
+REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+
+# Every directory holding Terraform this guard should cover. `iac/` carries the
+# customer-facing federation modules; they do not provision a vault today, but
+# scanning only `terraform/` would let one arrive there unnoticed.
+SCAN_ROOTS=(
+  "${REPO_ROOT}/terraform"
+  "${REPO_ROOT}/iac"
+)
+
+BANNED_RESOURCE='azurerm_key_vault_access_policy'
+
+files=()
+if [[ $# -gt 0 ]]; then
+  for arg in "$@"; do
+    case "$arg" in
+      -*) echo "Unknown flag: $arg" >&2; exit 2 ;;
+    esac
+    if [[ ! -f "$arg" ]]; then
+      echo "ERROR: file not found: $arg" >&2
+      exit 2
+    fi
+    files+=("$arg")
+  done
+else
+  for root in "${SCAN_ROOTS[@]}"; do
+    if [[ ! -d "$root" ]]; then
+      echo "ERROR: scan root not found: $root" >&2
+      exit 2
+    fi
+    # NUL-delimited so paths with spaces survive. `.tf.json` is collected so it
+    # can be REJECTED below, not because it can be parsed: silently returning
+    # "clean" for a file it cannot read is the exact failure mode this guard
+    # exists to prevent.
+    while IFS= read -r -d '' f; do
+      files+=("$f")
+    done < <(find "$root" -type f \( -name '*.tf' -o -name '*.tf.json' \) -print0 | sort -z)
+  done
+fi
+
+# Fail closed on the JSON encoding rather than under-reporting it.
+json_files=()
+for f in "${files[@]}"; do
+  [[ "$f" == *.tf.json ]] && json_files+=("$f")
+done
+if [[ ${#json_files[@]} -gt 0 ]]; then
+  {
+    echo "ERROR: this guard cannot parse the Terraform JSON encoding, and will not"
+    echo "       report a file it cannot read as clean. Found:"
+    printf '         %s\n' "${json_files[@]}"
+    echo "       Extend the scanner to handle .tf.json before adding one."
+  } >&2
+  exit 2
+fi
+
+if [[ ${#files[@]} -eq 0 ]]; then
+  echo "ERROR: no Terraform files to scan" >&2
+  exit 2
+fi
+
+# Match the block header, tolerating leading whitespace and not requiring
+# whitespace between tokens: `resource"azurerm_key_vault_access_policy""x"{` is
+# valid HCL. The pre-commit `terraform_fmt` hook normalizes a top-level block
+# back to column 0, and it covers every .tf file in the repo (both scan roots)
+# because CI runs `pre-commit run --all-files` in
+# .github/workflows/pre-commit.yml. The guard does not lean on that gate:
+# tolerating indentation means narrowing it later cannot open a bypass here.
+# Anchoring on `resource` still means a mention of the type inside a comment
+# (this file's own guidance, for one) does not trip the guard.
+violations=$(
+  awk \
+    -v resource_pattern="^[[:space:]]*resource[[:space:]]*\"${BANNED_RESOURCE}\"" '
+    $0 ~ resource_pattern {
+      printf "%s:%d: %s\n", FILENAME, FNR, $0
+    }
+  ' "${files[@]}"
+)
+
+if [[ -n "$violations" ]]; then
+  {
+    echo "FAILED: azurerm_key_vault_access_policy resource declared."
+    echo ""
+    echo "$violations"
+    echo ""
+    echo "This project's Key Vault sets enable_rbac_authorization = true"
+    echo "(terraform/modules/secrets/azure/main.tf). An RBAC-enabled vault ignores"
+    echo "access policies entirely, so the grant above applies cleanly and then"
+    echo "does nothing at runtime. Use an RBAC role assignment instead:"
+    echo ""
+    echo "  resource \"azurerm_role_assignment\" \"<name>\" {"
+    echo "    scope                = var.key_vault_id"
+    echo "    role_definition_name = \"Key Vault Secrets User\"  # Get + List"
+    echo "    principal_id         = <identity principal id>"
+    echo "  }"
+    echo ""
+    echo "See terraform/modules/compute/azure/container-apps/scheduled-tasks.tf"
+    echo "for the reference shape, and confirm any role name you pick actually"
+    echo "exists with: az role definition list --name \"<role>\""
+  } >&2
+  exit 1
+fi
+
+echo "OK: no azurerm_key_vault_access_policy resource declared in ${#files[@]} Terraform file(s)."
+echo "    (a nested access_policy block is NOT checked -- see #1839)"

scripts/test-azure-kv-access-policy.sh

@@ -0,0 +1,126 @@
+#!/usr/bin/env bash
+# test-azure-kv-access-policy.sh
+#
+# Exercises check-azure-kv-access-policy.sh against testdata fixtures, in BOTH
+# directions: a clean input must pass and a violating input must fail. A guard
+# that only ever returns "clean" is indistinguishable from a broken one, so the
+# failing cases are the ones that give this check its value.
+#
+# Exits 0 when all cases pass; exits 1 on any failure.
+
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)"
+CHECK="${SCRIPT_DIR}/check-azure-kv-access-policy.sh"
+FIXTURES="${SCRIPT_DIR}/testdata/azure-kv-access-policy"
+
+pass=0
+fail=0
+
+run_case() {
+  local label="$1"
+  local expected_exit="$2"
+  shift 2
+
+  local actual_exit=0
+  "$CHECK" "$@" >/dev/null 2>&1 || actual_exit=$?
+
+  if [[ "$actual_exit" -eq "$expected_exit" ]]; then
+    echo "PASS: $label"
+    (( pass++ )) || true
+  else
+    echo "FAIL: $label (expected exit $expected_exit, got $actual_exit)"
+    (( fail++ )) || true
+  fi
+}
+
+# Negative direction: the supported shape must not be reported. The fixture also
+# mentions the banned resource type inside a comment, so this case fails if the
+# guard degrades into a bare substring grep.
+run_case "role assignments and a prose mention exit 0" 0 \
+  "${FIXTURES}/clean.tf.fixture"
+
+# Positive direction: the anti-pattern must be caught.
+run_case "secret_permissions access policy exits 1" 1 \
+  "${FIXTURES}/access-policy.tf.fixture"
+
+# Asserted separately from the secret_permissions case: a guard keyed on
+# "Get"/"List" or on secret_permissions would pass the case above and still miss
+# this one. The ban is on the resource type.
+run_case "key_permissions access policy exits 1" 1 \
+  "${FIXTURES}/key-permissions.tf.fixture"
+
+# Indentation must not hide a declaration. The pre-commit `terraform_fmt` hook
+# would normally normalize a top-level block back to column 0 across both scan
+# roots, but the guard must not depend on that gate staying as wide as it is.
+run_case "indented access policy exits 1" 1 \
+  "${FIXTURES}/indented.tf.fixture"
+
+# HCL does not require whitespace between the block header tokens, and
+# `terraform fmt` treats the no-space form as a reformat rather than a parse
+# error, so the guard must not require a separator after `resource`.
+run_case "access policy with no inter-token whitespace exits 1" 1 \
+  "${FIXTURES}/nospace.tf.fixture"
+
+# A violation must still be found when mixed in with clean files.
+run_case "violation alongside a clean file exits 1" 1 \
+  "${FIXTURES}/clean.tf.fixture" "${FIXTURES}/access-policy.tf.fixture"
+
+# Usage errors are exit 2, distinct from "found a violation" (exit 1), so CI can
+# tell a broken invocation apart from a real finding.
+run_case "missing file exits 2" 2 \
+  "${FIXTURES}/does-not-exist.tf.fixture"
+
+run_case "unknown flag exits 2" 2 --bogus-flag
+
+# The JSON encoding is not parseable by this scanner, so it must be rejected
+# rather than reported clean. Exit 2 (cannot check) rather than 0 (checked and
+# clean) is the whole point.
+run_case "tf.json is rejected rather than silently passed" 2 \
+  "${FIXTURES}/json-encoding.tf.json"
+
+# The regression half of issue #1621: the two modules that carried the inert
+# access policy must stay free of one. This is a claim the guard can check
+# directly, and it is the reason the guard exists.
+run_case "cleanup-function module declares no access policy" 0 \
+  "${REPO_ROOT}/terraform/modules/compute/azure/cleanup-function/main.tf"
+
+run_case "aks module declares no access policy" 0 \
+  "${REPO_ROOT}/terraform/modules/compute/azure/aks/main.tf"
+
+# The default scan roots (terraform/ + iac/) must hold no violation. This also
+# pins the guard's false-positive surface: it runs over every real Terraform
+# file in the tree, none of which may trip it.
+run_case "default scan roots declare no access-policy resource" 0
+
+# Every run_case above compares exit codes only, so a guard that exited 1 with a
+# blank or wrong message would pass all of them while telling a developer
+# nothing. Pin the report itself: it must name the file and the line, which is
+# what makes a CI failure actionable.
+run_report_case() {
+  local label="$1"
+  local expected="$2"
+  shift 2
+
+  local report_stderr
+  report_stderr="$("$CHECK" "$@" 2>&1 >/dev/null || true)"
+
+  if [[ "$report_stderr" =~ $expected ]]; then
+    echo "PASS: $label"
+    (( pass++ )) || true
+  else
+    echo "FAIL: $label"
+    printf '  expected to match: %s\n' "$expected"
+    printf '  stderr was:\n%s\n' "$report_stderr"
+    (( fail++ )) || true
+  fi
+}
+
+run_report_case "violation report names the file and line" \
+  'access-policy\.tf\.fixture:9: resource "azurerm_key_vault_access_policy"' \
+  "${FIXTURES}/access-policy.tf.fixture"
+
+echo ""
+echo "Results: ${pass} passed, ${fail} failed."
+[[ "$fail" -eq 0 ]]

scripts/testdata/azure-kv-access-policy/access-policy.tf.fixture

@@ -0,0 +1,18 @@
+# Fixture: the #1621 anti-pattern verbatim. Must exit 1.
+#
+# This is the exact shape that shipped in
+# terraform/modules/compute/azure/cleanup-function/main.tf and
+# terraform/modules/compute/azure/aks/main.tf: an access policy declared against
+# a vault that has enable_rbac_authorization = true, which Azure accepts and
+# then ignores.
+
+resource "azurerm_key_vault_access_policy" "workload" {
+  key_vault_id = var.key_vault_id
+  tenant_id    = azurerm_user_assigned_identity.workload.tenant_id
+  object_id    = azurerm_user_assigned_identity.workload.principal_id
+
+  secret_permissions = [
+    "Get",
+    "List"
+  ]
+}

scripts/testdata/azure-kv-access-policy/clean.tf.fixture

@@ -0,0 +1,27 @@
+# Fixture: the supported shape. Must exit 0.
+#
+# An RBAC role assignment against the vault, plus an unrelated role assignment,
+# plus a prose mention of the banned resource type inside a comment. A guard
+# blunt enough to grep the bare string would fire on the comment below and on
+# nothing of substance, so this fixture is what keeps it anchored to the block
+# header.
+#
+# Do not replace this with an azurerm_key_vault_access_policy resource.
+
+resource "azurerm_role_assignment" "workload_kv_secrets_user" {
+  scope                = var.key_vault_id
+  role_definition_name = "Key Vault Secrets User"
+  principal_id         = azurerm_user_assigned_identity.workload.principal_id
+}
+
+resource "azurerm_role_assignment" "acr_pull" {
+  scope                = var.registry_id
+  role_definition_name = "AcrPull"
+  principal_id         = azurerm_user_assigned_identity.workload.principal_id
+}
+
+resource "azurerm_user_assigned_identity" "workload" {
+  name                = "workload-identity"
+  location            = var.location
+  resource_group_name = var.resource_group_name
+}