From fad8d660c30cb143ebad0c82c77c5b0bddb3cafd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 13 Aug 2026 23:49:09 +0200 Subject: [PATCH 1/4] fix(iac/azure): grant Key Vault access via RBAC role assignments The Key Vault this project provisions sets enable_rbac_authorization = true (terraform/modules/secrets/azure/main.tf:39). An RBAC-enabled vault never consults its accessPolicies array: data-plane authorization comes from Azure RBAC role assignments and nothing else. The Azure cleanup function and the AKS workload identity both granted themselves vault access with azurerm_key_vault_access_policy. Azure's control plane accepts that write, so terraform apply reported success with no warning while the grant was silently inert. It surfaced only as a runtime 403: the cleanup function could not read db-password, so expired sessions and stuck executions were never cleaned up, and any pod depending on the AKS workload identity failed at its first secret fetch. Both are replaced with "Key Vault Secrets User" role assignments, the exact RBAC equivalent of the access policies they replace (dataActions getSecret + readMetadata, matching secret_permissions Get/List, with an empty actions list so no management-plane rights are conferred). This is the pattern every other consumer in the tree already used. RBAC propagation is asynchronous and takes up to 10 minutes, so the function app and the AKS deployment take an explicit depends_on edge that Terraform's implicit graph does not capture. Because the failure is invisible at plan and apply time, a repo guard prevents it from returning. scripts/check-azure-kv-access-policy.sh fails when any Terraform file under terraform/ or iac/ declares the resource type, and runs as its own CI job alongside the existing azure-role-parity, aws-iam-parity and gcp-secret-scope guards. The guard is tested in both directions, since a check that only ever reports "clean" is indistinguishable from a broken one: scripts/test-azure-kv-access-policy.sh asserts that clean input exits 0 and that secret-permission, key-permission and indented declarations each exit 1, that a violation is still found when mixed with clean files, and that usage errors and the unparseable Terraform JSON encoding exit 2 rather than being reported as clean. Its last three cases assert the two repaired modules and the default scan roots are free of the resource; those three fail by assertion against the unfixed tree. Closes #1621 --- .github/workflows/ci.yml | 22 +++ scripts/check-azure-kv-access-policy.sh | 156 ++++++++++++++++++ scripts/test-azure-kv-access-policy.sh | 97 +++++++++++ .../access-policy.tf.fixture | 18 ++ .../azure-kv-access-policy/clean.tf.fixture | 27 +++ .../indented.tf.fixture | 17 ++ .../json-encoding.tf.json | 21 +++ .../key-permissions.tf.fixture | 22 +++ terraform/modules/compute/azure/aks/main.tf | 37 +++-- .../compute/azure/cleanup-function/main.tf | 32 ++-- 10 files changed, 428 insertions(+), 21 deletions(-) create mode 100755 scripts/check-azure-kv-access-policy.sh create mode 100755 scripts/test-azure-kv-access-policy.sh create mode 100644 scripts/testdata/azure-kv-access-policy/access-policy.tf.fixture create mode 100644 scripts/testdata/azure-kv-access-policy/clean.tf.fixture create mode 100644 scripts/testdata/azure-kv-access-policy/indented.tf.fixture create mode 100644 scripts/testdata/azure-kv-access-policy/json-encoding.tf.json create mode 100644 scripts/testdata/azure-kv-access-policy/key-permissions.tf.fixture diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d6122da87..a0f00255a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -736,6 +736,27 @@ 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. + # 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). 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 +773,7 @@ jobs: - aws-iam-parity - gcp-secret-scope - ecr-delete-selection + - azure-kv-access-policy if: always() steps: diff --git a/scripts/check-azure-kv-access-policy.sh b/scripts/check-azure-kv-access-policy.sh new file mode 100755 index 000000000..a6295dab4 --- /dev/null +++ b/scripts/check-azure-kv-access-policy.sh @@ -0,0 +1,156 @@ +#!/usr/bin/env bash +# check-azure-kv-access-policy.sh +# +# Fails when any Terraform file declares an `azurerm_key_vault_access_policy`. +# +# 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 = +# } +# +# 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. It matches a +# `resource "azurerm_key_vault_access_policy"` block header at any indentation, +# which is the form the #1621 defect arrived in. It +# does NOT detect an inline `access_policy { ... }` block nested inside an +# `azurerm_key_vault` resource, which is a second way to express the same thing. +# No such block exists in the tree today (the sole vault declares none), and +# adding one would be a different edit than the copy-paste this ratchets +# against. 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. `terraform fmt` puts +# top-level blocks at column 0, but the fmt gate in CI only covers `terraform/` +# while this guard also scans `iac/`, so anchoring strictly at column 0 would +# leave one scan root relying on a normalization the other one enforces. +# 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 pattern="^[[:space:]]*resource[[:space:]]+\"${BANNED_RESOURCE}\"" ' + $0 ~ pattern { printf "%s:%d: %s\n", FILENAME, FNR, $0 } + ' "${files[@]}" +) + +if [[ -n "$violations" ]]; then + { + echo "FAILED: azurerm_key_vault_access_policy 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\" \"\" {" + echo " scope = var.key_vault_id" + echo " role_definition_name = \"Key Vault Secrets User\" # Get + List" + echo " 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 \"\"" + } >&2 + exit 1 +fi + +echo "OK: no azurerm_key_vault_access_policy resource declared in ${#files[@]}" \ + "Terraform file(s). See the LIMITATIONS header: an inline access_policy" \ + "block nested inside an azurerm_key_vault resource is not covered." diff --git a/scripts/test-azure-kv-access-policy.sh b/scripts/test-azure-kv-access-policy.sh new file mode 100755 index 000000000..3dd524625 --- /dev/null +++ b/scripts/test-azure-kv-access-policy.sh @@ -0,0 +1,97 @@ +#!/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 guard scans iac/ as well as +# terraform/, and only terraform/ is covered by the repo-wide `terraform fmt` +# gate that would otherwise normalize a block back to column 0. +run_case "indented access policy exits 1" 1 \ + "${FIXTURES}/indented.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. +# +# Deliberately NOT phrased as "no inert Key Vault grant can exist". An inline +# `access_policy { ... }` block nested inside an azurerm_key_vault resource +# expresses the same thing and this guard structurally cannot see it (see the +# LIMITATIONS header on the check). None exists today. This case asserts what +# the guard verifies, not a broader property it never inspects. +run_case "default scan roots declare no access-policy resource" 0 + +echo "" +echo "Results: ${pass} passed, ${fail} failed." +[[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/azure-kv-access-policy/access-policy.tf.fixture b/scripts/testdata/azure-kv-access-policy/access-policy.tf.fixture new file mode 100644 index 000000000..f66026e0e --- /dev/null +++ b/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" + ] +} diff --git a/scripts/testdata/azure-kv-access-policy/clean.tf.fixture b/scripts/testdata/azure-kv-access-policy/clean.tf.fixture new file mode 100644 index 000000000..84b9d5588 --- /dev/null +++ b/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 +} diff --git a/scripts/testdata/azure-kv-access-policy/indented.tf.fixture b/scripts/testdata/azure-kv-access-policy/indented.tf.fixture new file mode 100644 index 000000000..568e0a5c7 --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/indented.tf.fixture @@ -0,0 +1,17 @@ +# Fixture: an access policy declared at a non-zero indentation. Must exit 1. +# +# `terraform fmt` moves top-level blocks to column 0, but the repo-wide fmt gate +# in CI only covers `terraform/` while this guard also scans `iac/`. A guard +# anchored strictly at column 0 would therefore depend, on one of its two scan +# roots, on a normalization nothing enforces there. This fixture pins the +# tolerance so that dependency cannot creep back in. + + resource "azurerm_key_vault_access_policy" "indented" { + key_vault_id = var.key_vault_id + tenant_id = data.azurerm_client_config.current.tenant_id + object_id = azurerm_user_assigned_identity.app.principal_id + + secret_permissions = [ + "Get", + ] + } diff --git a/scripts/testdata/azure-kv-access-policy/json-encoding.tf.json b/scripts/testdata/azure-kv-access-policy/json-encoding.tf.json new file mode 100644 index 000000000..c5db136c3 --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/json-encoding.tf.json @@ -0,0 +1,21 @@ +{ + "_comment": [ + "Fixture: the Terraform JSON encoding. Must exit 2 (cannot check), never 0.", + "This file DOES declare an azurerm_key_vault_access_policy. The HCL scanner", + "matches block syntax and would find nothing here, so without the explicit", + "reject it would report clean, which is the exact failure this guard exists", + "to prevent. Kept as .tf.json so it is discovered by the same find as a real", + "one would be; it lives outside terraform/ and iac/, so it is only reachable", + "when passed explicitly by this test." + ], + "resource": { + "azurerm_key_vault_access_policy": { + "workload": { + "key_vault_id": "${var.key_vault_id}", + "tenant_id": "${data.azurerm_client_config.current.tenant_id}", + "object_id": "${azurerm_user_assigned_identity.workload.principal_id}", + "secret_permissions": ["Get", "List"] + } + } + } +} diff --git a/scripts/testdata/azure-kv-access-policy/key-permissions.tf.fixture b/scripts/testdata/azure-kv-access-policy/key-permissions.tf.fixture new file mode 100644 index 000000000..3f59ffe54 --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/key-permissions.tf.fixture @@ -0,0 +1,22 @@ +# Fixture: an access policy granting KEY permissions rather than secret ones. +# Must exit 1. +# +# The ban is on the resource type, not on any particular permission list. An +# RBAC-enabled vault ignores this one just as completely as it ignores a +# secret_permissions policy, and a guard keyed on "Get"/"List" or on +# secret_permissions would let this variant through. + +resource "azurerm_key_vault_access_policy" "signing" { + key_vault_id = var.key_vault_id + tenant_id = data.azurerm_client_config.current.tenant_id + object_id = azurerm_user_assigned_identity.app.principal_id + + key_permissions = [ + "Get", + "Sign", + ] + + certificate_permissions = [ + "Get", + ] +} diff --git a/terraform/modules/compute/azure/aks/main.tf b/terraform/modules/compute/azure/aks/main.tf index f9fe58ccc..f37ec9170 100644 --- a/terraform/modules/compute/azure/aks/main.tf +++ b/terraform/modules/compute/azure/aks/main.tf @@ -101,16 +101,25 @@ resource "azurerm_user_assigned_identity" "workload" { tags = local.common_tags } -# Grant workload identity access to Key Vault secrets -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" - ] +# Grant the workload identity access to Key Vault secrets. +# +# This MUST be an RBAC role assignment, not an azurerm_key_vault_access_policy. +# The vault this module is pointed at (terraform/modules/secrets/azure/main.tf) +# sets enable_rbac_authorization = true, and an RBAC-enabled vault never +# consults its accessPolicies array. Terraform applies an access policy against +# such a vault successfully, so the mistake is invisible at plan and apply time +# and only surfaces as a runtime 403 when a pod fetches a secret (#1621). +# +# "Key Vault Secrets User" is the exact RBAC equivalent of the access policy this +# replaced: its dataActions are getSecret + readMetadata, which is precisely +# secret_permissions = ["Get", "List"], and its actions list is empty so it +# confers no management-plane rights. Deliberately not "Key Vault Secrets +# Officer" (dataActions secrets/*), which would grant write and delete that the +# access policy never did. +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 } # Grant AKS cluster identity access to pull images from ACR @@ -424,9 +433,15 @@ resource "kubernetes_deployment" "app" { } } + # The Key Vault grant is listed here because these pods resolve + # ADMIN_PASSWORD_SECRET, CREDENTIAL_ENCRYPTION_KEY_SECRET_NAME and the + # AZURE_SMTP_* secrets from the vault at startup through the workload + # identity. Azure RBAC propagation is asynchronous and takes up to 10 + # minutes, and Terraform's implicit graph does not capture that edge. depends_on = [ kubernetes_namespace.app, - kubernetes_secret.database + kubernetes_secret.database, + azurerm_role_assignment.workload_kv_secrets_user ] } diff --git a/terraform/modules/compute/azure/cleanup-function/main.tf b/terraform/modules/compute/azure/cleanup-function/main.tf index 0455c5410..119cefbf0 100644 --- a/terraform/modules/compute/azure/cleanup-function/main.tf +++ b/terraform/modules/compute/azure/cleanup-function/main.tf @@ -27,16 +27,23 @@ resource "azurerm_user_assigned_identity" "cleanup" { tags = var.tags } -# Grant Key Vault access to managed identity -resource "azurerm_key_vault_access_policy" "cleanup" { - key_vault_id = var.key_vault_id - tenant_id = azurerm_user_assigned_identity.cleanup.tenant_id - object_id = azurerm_user_assigned_identity.cleanup.principal_id - - secret_permissions = [ - "Get", - "List" - ] +# Grant Key Vault access to the managed identity. +# +# This MUST be an RBAC role assignment, not an azurerm_key_vault_access_policy. +# The vault this module is pointed at (terraform/modules/secrets/azure/main.tf) +# sets enable_rbac_authorization = true, and an RBAC-enabled vault never +# consults its accessPolicies array. Terraform applies an access policy against +# such a vault successfully, so the mistake is invisible at plan and apply time +# and only surfaces as a runtime 403 when the function reads db-password (#1621). +# +# "Key Vault Secrets User" is the exact RBAC equivalent of the access policy this +# replaced: its dataActions are getSecret + readMetadata, which is precisely +# secret_permissions = ["Get", "List"], and its actions list is empty so it +# confers no management-plane rights. +resource "azurerm_role_assignment" "cleanup_kv_secrets_user" { + scope = var.key_vault_id + role_definition_name = "Key Vault Secrets User" + principal_id = azurerm_user_assigned_identity.cleanup.principal_id } # Linux Function App with container @@ -96,6 +103,11 @@ resource "azurerm_linux_function_app" "cleanup" { virtual_network_subnet_id = var.subnet_id != "" ? var.subnet_id : null tags = var.tags + + # Azure RBAC propagation is asynchronous and takes up to 10 minutes. Without + # this edge Terraform may create the function app in parallel with its Key + # Vault grant, and the first invocation 403s on db-password. + depends_on = [azurerm_role_assignment.cleanup_kv_secrets_user] } # Timer trigger function (defined in host.json and function.json) From df5b11eb05cec2b79136d531e426f0c2c8760985 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 14 Aug 2026 00:59:11 +0200 Subject: [PATCH 2/4] fix(scripts): close the nested access_policy gap in the Key Vault guard The guard matched only a top-level `resource "azurerm_key_vault_access_policy"` header. An `access_policy { ... }` block nested inside `azurerm_key_vault`, and its `dynamic` form, express exactly the same inert grant against an RBAC-enabled vault and were both reported clean. Add a second axis covering both, with fixtures asserting exit 1, and remove the LIMITATIONS paragraph and success-message clause that conceded the gap: a stale limitation note is its own defect. Also: - The header justified tolerating leading whitespace by claiming the fmt gate in CI covers only `terraform/`. It does not: the pre-commit `terraform_fmt` hook matches every `.tf` in the repo and CI runs `pre-commit run --all-files`. Correct the premise in the check header, the test harness and `indented.tf.fixture`, and keep the tolerance for the reason that does hold: the nested form is indented by construction. - The pattern required whitespace between `resource` and the type, but `resource"azurerm_key_vault_access_policy""x"{` is valid HCL and exited 0. Match `resource[[:space:]]*"` and pin it with `nospace.tf.fixture`. - The self-test discarded stderr, so a guard exiting 1 with a blank or wrong message passed all cases. Assert the report for the #1621 shape names the file and the line. - The `depends_on` comments on the AKS deployment and the cleanup function app claimed `depends_on` covers a propagation window that "takes up to 10 minutes". It orders creation, it does not wait. Reword to the ordering edge Terraform's implicit graph does not provide. The `depends_on` itself is correct and stays. Guard: 15/15 self-test cases pass, exit 0 across all 199 Terraform files in the scan roots with zero false positives from the new axis. --- .github/workflows/ci.yml | 11 ++-- scripts/check-azure-kv-access-policy.sh | 51 +++++++++++-------- scripts/test-azure-kv-access-policy.sh | 51 +++++++++++++++---- .../dynamic-policy.tf.fixture | 26 ++++++++++ .../indented.tf.fixture | 12 +++-- .../inline-policy.tf.fixture | 26 ++++++++++ .../azure-kv-access-policy/nospace.tf.fixture | 18 +++++++ terraform/modules/compute/azure/aks/main.tf | 7 ++- .../compute/azure/cleanup-function/main.tf | 8 +-- 9 files changed, 164 insertions(+), 46 deletions(-) create mode 100644 scripts/testdata/azure-kv-access-policy/dynamic-policy.tf.fixture create mode 100644 scripts/testdata/azure-kv-access-policy/inline-policy.tf.fixture create mode 100644 scripts/testdata/azure-kv-access-policy/nospace.tf.fixture diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a0f00255a..7c58d178b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -736,11 +736,12 @@ 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. - # 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). Fast (shell only), so it - # always runs. + # Assert that no Terraform file declares a Key Vault access-policy grant, + # either as an azurerm_key_vault_access_policy resource or as an access_policy + # block nested in the vault. 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). Fast (shell only), so it always runs. azure-kv-access-policy: name: Azure Key Vault grant model runs-on: ubuntu-latest diff --git a/scripts/check-azure-kv-access-policy.sh b/scripts/check-azure-kv-access-policy.sh index a6295dab4..dac5fd97d 100755 --- a/scripts/check-azure-kv-access-policy.sh +++ b/scripts/check-azure-kv-access-policy.sh @@ -1,7 +1,10 @@ #!/usr/bin/env bash # check-azure-kv-access-policy.sh # -# Fails when any Terraform file declares an `azurerm_key_vault_access_policy`. +# Fails when any Terraform file declares a Key Vault access-policy grant, in +# either of the two forms HCL offers: a top-level +# `azurerm_key_vault_access_policy` resource, or an `access_policy` block nested +# inside an `azurerm_key_vault` resource (including the `dynamic` form). # # WHY A BLANKET BAN IS CORRECT HERE. This repo provisions exactly one Key Vault # (terraform/modules/secrets/azure/main.tf) and it hardcodes @@ -32,18 +35,15 @@ # 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 0 = no access-policy grant declared, in either form. # 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. It matches a -# `resource "azurerm_key_vault_access_policy"` block header at any indentation, -# which is the form the #1621 defect arrived in. It -# does NOT detect an inline `access_policy { ... }` block nested inside an -# `azurerm_key_vault` resource, which is a second way to express the same thing. -# No such block exists in the tree today (the sole vault declares none), and -# adding one would be a different edit than the copy-paste this ratchets -# against. The Terraform JSON encoding (`.tf.json`) is not parsed; rather than +# LIMITATIONS. This is a textual guard, not a policy engine. It matches block +# headers at any indentation: a `resource "azurerm_key_vault_access_policy"` +# header (the form the #1621 defect arrived in) and a nested `access_policy` or +# `dynamic "access_policy"` header (the second way to express the same inert +# grant). 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: @@ -67,6 +67,12 @@ SCAN_ROOTS=( BANNED_RESOURCE='azurerm_key_vault_access_policy' +# The nested form: an `access_policy { ... }` block inside an azurerm_key_vault +# resource, or its `dynamic "access_policy"` equivalent. `access_policy` is a +# block name unique to azurerm_key_vault, so anchoring at the start of the line +# has no false-positive surface in this tree. +BANNED_BLOCK='access_policy' + files=() if [[ $# -gt 0 ]]; then for arg in "$@"; do @@ -115,21 +121,28 @@ if [[ ${#files[@]} -eq 0 ]]; then exit 2 fi -# Match the block header, tolerating leading whitespace. `terraform fmt` puts -# top-level blocks at column 0, but the fmt gate in CI only covers `terraform/` -# while this guard also scans `iac/`, so anchoring strictly at column 0 would -# leave one scan root relying on a normalization the other one enforces. +# Match both block headers, tolerating leading whitespace and not requiring +# whitespace between tokens: `resource"azurerm_key_vault_access_policy""x"{` is +# valid HCL. The nested form is necessarily indented, and the top-level form is +# normalized to column 0 by the pre-commit `terraform_fmt` hook, which 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 cannot open a bypass. # 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 pattern="^[[:space:]]*resource[[:space:]]+\"${BANNED_RESOURCE}\"" ' - $0 ~ pattern { printf "%s:%d: %s\n", FILENAME, FNR, $0 } + awk \ + -v resource_pattern="^[[:space:]]*resource[[:space:]]*\"${BANNED_RESOURCE}\"" \ + -v block_pattern="^[[:space:]]*(dynamic[[:space:]]*\")?${BANNED_BLOCK}" ' + $0 ~ resource_pattern || $0 ~ block_pattern { + printf "%s:%d: %s\n", FILENAME, FNR, $0 + } ' "${files[@]}" ) if [[ -n "$violations" ]]; then { - echo "FAILED: azurerm_key_vault_access_policy declared." + echo "FAILED: Key Vault access-policy grant declared." echo "" echo "$violations" echo "" @@ -151,6 +164,4 @@ if [[ -n "$violations" ]]; then exit 1 fi -echo "OK: no azurerm_key_vault_access_policy resource declared in ${#files[@]}" \ - "Terraform file(s). See the LIMITATIONS header: an inline access_policy" \ - "block nested inside an azurerm_key_vault resource is not covered." +echo "OK: no Key Vault access-policy grant declared in ${#files[@]} Terraform file(s)." diff --git a/scripts/test-azure-kv-access-policy.sh b/scripts/test-azure-kv-access-policy.sh index 3dd524625..70c00ad9e 100755 --- a/scripts/test-azure-kv-access-policy.sh +++ b/scripts/test-azure-kv-access-policy.sh @@ -51,12 +51,29 @@ run_case "secret_permissions access policy exits 1" 1 \ run_case "key_permissions access policy exits 1" 1 \ "${FIXTURES}/key-permissions.tf.fixture" -# Indentation must not hide a declaration. The guard scans iac/ as well as -# terraform/, and only terraform/ is covered by the repo-wide `terraform fmt` -# gate that would otherwise normalize a block back to column 0. +# 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, +# and the nested access_policy form below is indented by construction. 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" + +# The second form of the same inert grant: an access_policy block nested inside +# the vault resource, and its dynamic equivalent. Neither declares the banned +# resource type, so a guard that only knew the top-level header would report +# both clean. +run_case "inline access_policy block exits 1" 1 \ + "${FIXTURES}/inline-policy.tf.fixture" + +run_case "dynamic access_policy block exits 1" 1 \ + "${FIXTURES}/dynamic-policy.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" @@ -83,14 +100,26 @@ run_case "cleanup-function module declares no access policy" 0 \ 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. -# -# Deliberately NOT phrased as "no inert Key Vault grant can exist". An inline -# `access_policy { ... }` block nested inside an azurerm_key_vault resource -# expresses the same thing and this guard structurally cannot see it (see the -# LIMITATIONS header on the check). None exists today. This case asserts what -# the guard verifies, not a broader property it never inspects. -run_case "default scan roots declare no access-policy resource" 0 +# The default scan roots (terraform/ + iac/) must hold no violation, in either +# form. This also pins the false-positive surface of the nested-block axis: 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 grant" 0 + +# Every 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 for the #1621 shape: it must name the file and the +# line, which is what makes a CI failure actionable. +expected_report='access-policy\.tf\.fixture:9: resource "azurerm_key_vault_access_policy"' +report_stderr="$("$CHECK" "${FIXTURES}/access-policy.tf.fixture" 2>&1 >/dev/null || true)" +if [[ "$report_stderr" =~ $expected_report ]]; then + echo "PASS: violation report names the file and line" + (( pass++ )) || true +else + echo "FAIL: violation report names the file and line" + printf ' expected to match: %s\n' "$expected_report" + printf ' stderr was:\n%s\n' "$report_stderr" + (( fail++ )) || true +fi echo "" echo "Results: ${pass} passed, ${fail} failed." diff --git a/scripts/testdata/azure-kv-access-policy/dynamic-policy.tf.fixture b/scripts/testdata/azure-kv-access-policy/dynamic-policy.tf.fixture new file mode 100644 index 000000000..4015722d5 --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/dynamic-policy.tf.fixture @@ -0,0 +1,26 @@ +# Fixture: the nested grant generated through a dynamic block. Must exit 1. +# +# Same inert grant as inline-policy.tf.fixture, reached through the `dynamic` +# form so the block name is quoted rather than bare. Asserted separately: a +# pattern anchored on a bare `access_policy` at the start of the line would +# catch the inline fixture and still miss this one. + +resource "azurerm_key_vault" "main" { + name = "cudly-kv" + location = var.location + resource_group_name = var.resource_group_name + tenant_id = data.azurerm_client_config.current.tenant_id + sku_name = "standard" + + enable_rbac_authorization = true + + dynamic "access_policy" { + for_each = var.reader_object_ids + + content { + tenant_id = data.azurerm_client_config.current.tenant_id + object_id = access_policy.value + secret_permissions = ["Get"] + } + } +} diff --git a/scripts/testdata/azure-kv-access-policy/indented.tf.fixture b/scripts/testdata/azure-kv-access-policy/indented.tf.fixture index 568e0a5c7..876da33c0 100644 --- a/scripts/testdata/azure-kv-access-policy/indented.tf.fixture +++ b/scripts/testdata/azure-kv-access-policy/indented.tf.fixture @@ -1,10 +1,12 @@ # Fixture: an access policy declared at a non-zero indentation. Must exit 1. # -# `terraform fmt` moves top-level blocks to column 0, but the repo-wide fmt gate -# in CI only covers `terraform/` while this guard also scans `iac/`. A guard -# anchored strictly at column 0 would therefore depend, on one of its two scan -# roots, on a normalization nothing enforces there. This fixture pins the -# tolerance so that dependency cannot creep back in. +# `terraform fmt` moves top-level blocks to column 0, and the pre-commit +# `terraform_fmt` hook (.pre-commit-config.yaml) covers every .tf file in the +# repo, both of this guard's scan roots included, because CI runs `pre-commit +# run --all-files` (.github/workflows/pre-commit.yml). The guard does not lean +# on that: a guard anchored strictly at column 0 would depend on a gate that +# could be narrowed later, and the nested access_policy form is indented by +# construction. This fixture pins the tolerance so it cannot be lost. resource "azurerm_key_vault_access_policy" "indented" { key_vault_id = var.key_vault_id diff --git a/scripts/testdata/azure-kv-access-policy/inline-policy.tf.fixture b/scripts/testdata/azure-kv-access-policy/inline-policy.tf.fixture new file mode 100644 index 000000000..5cc6dfd46 --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/inline-policy.tf.fixture @@ -0,0 +1,26 @@ +# Fixture: the nested form of the same inert grant. Must exit 1. +# +# An `access_policy { ... }` block declared inside the vault itself expresses +# exactly what azurerm_key_vault_access_policy does, and Azure ignores it for +# the same reason: enable_rbac_authorization = true, right above it. A guard +# that only knew the top-level resource header would report this file clean. + +resource "azurerm_key_vault" "main" { + name = "cudly-kv" + location = var.location + resource_group_name = var.resource_group_name + tenant_id = data.azurerm_client_config.current.tenant_id + sku_name = "standard" + + enable_rbac_authorization = true + + access_policy { + tenant_id = data.azurerm_client_config.current.tenant_id + object_id = azurerm_user_assigned_identity.workload.principal_id + + secret_permissions = [ + "Get", + "List" + ] + } +} diff --git a/scripts/testdata/azure-kv-access-policy/nospace.tf.fixture b/scripts/testdata/azure-kv-access-policy/nospace.tf.fixture new file mode 100644 index 000000000..03bac591d --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/nospace.tf.fixture @@ -0,0 +1,18 @@ +# Fixture: a declaration with no whitespace between the block header tokens. +# Must exit 1. +# +# `resource"azurerm_key_vault_access_policy""nospace"{` is valid HCL: the +# parser does not require the separators, and `terraform fmt` reports it as +# needing reformatting rather than as a parse error. A guard requiring +# whitespace after `resource` would report this file clean, so the tolerance is +# pinned here rather than left to the fmt gate that would normally rewrite it. + +resource"azurerm_key_vault_access_policy""nospace"{ + key_vault_id = var.key_vault_id + tenant_id = data.azurerm_client_config.current.tenant_id + object_id = azurerm_user_assigned_identity.workload.principal_id + + secret_permissions = [ + "Get", + ] +} diff --git a/terraform/modules/compute/azure/aks/main.tf b/terraform/modules/compute/azure/aks/main.tf index f37ec9170..c186309b7 100644 --- a/terraform/modules/compute/azure/aks/main.tf +++ b/terraform/modules/compute/azure/aks/main.tf @@ -436,8 +436,11 @@ resource "kubernetes_deployment" "app" { # The Key Vault grant is listed here because these pods resolve # ADMIN_PASSWORD_SECRET, CREDENTIAL_ENCRYPTION_KEY_SECRET_NAME and the # AZURE_SMTP_* secrets from the vault at startup through the workload - # identity. Azure RBAC propagation is asynchronous and takes up to 10 - # minutes, and Terraform's implicit graph does not capture that edge. + # identity. Nothing in the deployment references the role assignment, so + # Terraform's implicit graph would otherwise be free to create the two in + # parallel. This buys ordering only, not a propagation wait: the assignment + # returns as soon as ARM accepts the write, so a pod started immediately + # after can still 403 until the grant propagates, and recovers on restart. depends_on = [ kubernetes_namespace.app, kubernetes_secret.database, diff --git a/terraform/modules/compute/azure/cleanup-function/main.tf b/terraform/modules/compute/azure/cleanup-function/main.tf index 119cefbf0..b09546ca3 100644 --- a/terraform/modules/compute/azure/cleanup-function/main.tf +++ b/terraform/modules/compute/azure/cleanup-function/main.tf @@ -104,9 +104,11 @@ resource "azurerm_linux_function_app" "cleanup" { tags = var.tags - # Azure RBAC propagation is asynchronous and takes up to 10 minutes. Without - # this edge Terraform may create the function app in parallel with its Key - # Vault grant, and the first invocation 403s on db-password. + # Nothing in this resource references the Key Vault grant, so without this + # edge Terraform is free to create the function app in parallel with it. + # Ordering only, not a propagation wait: the role assignment returns as soon + # as ARM accepts the write, so an invocation immediately after can still 403 + # on db-password until the grant propagates. depends_on = [azurerm_role_assignment.cleanup_kv_secrets_user] } From 704bdd3a7c8cc6bd7da2ae64aed5833351afd1cd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 00:42:41 +0200 Subject: [PATCH 3/4] fix(scripts): match the whole access_policy block header, not its prefix The nested-block axis of the Key Vault guard matched the bare `access_policy` token, so it fired on any identifier merely starting with it. `access_policy_enabled = false`, `access_policy = "metadata"` and the `access_policy.value` traversal inside a dynamic block's content were all reported as access-policy grants, failing CI on valid Terraform. Require the complete block header instead: `access_policy {` or `dynamic "access_policy" {`, with the opening brace on the same line, which is what HCL requires of a real block. Both spellings still match at any indentation and with no whitespace between tokens. The two axes of this guard have now failed in opposite directions for the same reason, once too narrow to see a nested grant and once too broad to tell one from a longer identifier, so pin both directions with fixtures: add prefix-only.tf.fixture asserting exit 0 for the three non-granting shapes above. Also assert the report text on the nested axis, not just the exit code. The suite compared exit codes alone, so a guard that exited 1 with a blank or wrong message would have passed every case while telling a developer nothing. The two report assertions share a helper. Verified under mawk, gawk and original-awk on ubuntu:24.04, the CI runner image: 17/17 self-tests and a clean scan of the real tree under each. --- scripts/check-azure-kv-access-policy.sh | 23 ++++++-- scripts/test-azure-kv-access-policy.sh | 52 ++++++++++++++----- .../prefix-only.tf.fixture | 30 +++++++++++ 3 files changed, 86 insertions(+), 19 deletions(-) create mode 100644 scripts/testdata/azure-kv-access-policy/prefix-only.tf.fixture diff --git a/scripts/check-azure-kv-access-policy.sh b/scripts/check-azure-kv-access-policy.sh index dac5fd97d..7ed39e0dd 100755 --- a/scripts/check-azure-kv-access-policy.sh +++ b/scripts/check-azure-kv-access-policy.sh @@ -43,7 +43,11 @@ # headers at any indentation: a `resource "azurerm_key_vault_access_policy"` # header (the form the #1621 defect arrived in) and a nested `access_policy` or # `dynamic "access_policy"` header (the second way to express the same inert -# grant). The Terraform JSON encoding (`.tf.json`) is not parsed; rather than +# grant). The nested header must carry its opening brace on the same line, which +# is what HCL requires of a real block but not of an inline comment wedged before +# it (`access_policy /* c */ {`); that contrived spelling is out of scope, since +# the defect this guards against is an accidental grant, not an obfuscated one. +# 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: @@ -68,9 +72,7 @@ SCAN_ROOTS=( BANNED_RESOURCE='azurerm_key_vault_access_policy' # The nested form: an `access_policy { ... }` block inside an azurerm_key_vault -# resource, or its `dynamic "access_policy"` equivalent. `access_policy` is a -# block name unique to azurerm_key_vault, so anchoring at the start of the line -# has no false-positive surface in this tree. +# resource, or its `dynamic "access_policy"` equivalent. BANNED_BLOCK='access_policy' files=() @@ -130,10 +132,21 @@ fi # that gate: tolerating indentation means narrowing it cannot open a bypass. # 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. +# +# The nested axis matches the COMPLETE block header, up to and including the +# opening brace, rather than the `access_policy` token alone. A bare prefix also +# fires on any longer identifier that starts with it (`access_policy_enabled`) +# and on an attribute assignment (`access_policy = "metadata"`, or the +# `access_policy.value` reference inside a dynamic block's content), none of +# which grant anything. Requiring the brace does not let a real block hide by +# wrapping: hclsyntax rejects a block whose body does not open on the header +# line ("Argument or block definition required"), so the header cannot be split +# across two lines to evade this. It does narrow the guard by the one spelling +# noted under LIMITATIONS above. violations=$( awk \ -v resource_pattern="^[[:space:]]*resource[[:space:]]*\"${BANNED_RESOURCE}\"" \ - -v block_pattern="^[[:space:]]*(dynamic[[:space:]]*\")?${BANNED_BLOCK}" ' + -v block_pattern="^[[:space:]]*(${BANNED_BLOCK}|dynamic[[:space:]]*\"${BANNED_BLOCK}\")[[:space:]]*[{]" ' $0 ~ resource_pattern || $0 ~ block_pattern { printf "%s:%d: %s\n", FILENAME, FNR, $0 } diff --git a/scripts/test-azure-kv-access-policy.sh b/scripts/test-azure-kv-access-policy.sh index 70c00ad9e..be130b5da 100755 --- a/scripts/test-azure-kv-access-policy.sh +++ b/scripts/test-azure-kv-access-policy.sh @@ -41,6 +41,12 @@ run_case() { run_case "role assignments and a prose mention exit 0" 0 \ "${FIXTURES}/clean.tf.fixture" +# The other half of the negative direction, and the reason the nested-block axis +# cannot be a prefix match: `access_policy_enabled`, `access_policy = ...` and +# `access_policy.value` all start with the block name and grant nothing. +run_case "identifiers that only share the access_policy prefix exit 0" 0 \ + "${FIXTURES}/prefix-only.tf.fixture" + # Positive direction: the anti-pattern must be caught. run_case "secret_permissions access policy exits 1" 1 \ "${FIXTURES}/access-policy.tf.fixture" @@ -105,21 +111,39 @@ run_case "aks module declares no access policy" 0 \ # runs over every real Terraform file in the tree, none of which may trip it. run_case "default scan roots declare no access-policy grant" 0 -# Every case above compares exit codes only, so a guard that exited 1 with a +# 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 for the #1621 shape: it must name the file and the -# line, which is what makes a CI failure actionable. -expected_report='access-policy\.tf\.fixture:9: resource "azurerm_key_vault_access_policy"' -report_stderr="$("$CHECK" "${FIXTURES}/access-policy.tf.fixture" 2>&1 >/dev/null || true)" -if [[ "$report_stderr" =~ $expected_report ]]; then - echo "PASS: violation report names the file and line" - (( pass++ )) || true -else - echo "FAIL: violation report names the file and line" - printf ' expected to match: %s\n' "$expected_report" - printf ' stderr was:\n%s\n' "$report_stderr" - (( fail++ )) || true -fi +# 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 +} + +# The #1621 shape, on the top-level resource axis. +run_report_case "top-level violation report names the file and line" \ + 'access-policy\.tf\.fixture:9: resource "azurerm_key_vault_access_policy"' \ + "${FIXTURES}/access-policy.tf.fixture" + +# The nested axis, pinned separately: it must report the block header line, not +# some other line in the file that happens to contain the token. +run_report_case "nested violation report names the block header line" \ + 'inline-policy\.tf\.fixture:17:[[:space:]]+access_policy[[:space:]]*[{]' \ + "${FIXTURES}/inline-policy.tf.fixture" echo "" echo "Results: ${pass} passed, ${fail} failed." diff --git a/scripts/testdata/azure-kv-access-policy/prefix-only.tf.fixture b/scripts/testdata/azure-kv-access-policy/prefix-only.tf.fixture new file mode 100644 index 000000000..6a49cb262 --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/prefix-only.tf.fixture @@ -0,0 +1,30 @@ +# Fixture: identifiers that merely START with `access_policy`. Must exit 0. +# +# None of these declares a block, so none grants anything. They exist because +# the nested-block axis was first written as a bare prefix match and fired on +# all of them, failing CI on valid Terraform. The two axes of this guard have +# now failed in opposite directions for the same reason -- once too narrow to +# see a nested grant, once too broad to tell one from a longer identifier -- so +# both directions are pinned by fixtures. + +resource "azurerm_key_vault" "main" { + name = "cudly-kv" + location = var.location + resource_group_name = var.resource_group_name + tenant_id = data.azurerm_client_config.current.tenant_id + sku_name = "standard" + + enable_rbac_authorization = true + + # A longer identifier that happens to share the prefix. + access_policy_enabled = false +} + +resource "some_other" "x" { + # An attribute assignment, not a block: no brace on the header line. + access_policy = "metadata" + + # The same token as a traversal, the shape a dynamic block's content uses to + # reference its iterator. + object_id = access_policy.value +} From b147c044b5806986f4d8f98c0d96111524f08eea Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 02:09:17 +0200 Subject: [PATCH 4/4] fix(scripts): scope the Key Vault guard to the resource form Remove the nested `access_policy` block axis from the Key Vault guard, keeping the `resource "azurerm_key_vault_access_policy"` ban that #1621 needs. The nested matcher has now been wrong three times in a row, each fix surfacing the next: too narrow to see a real block, then broad enough to fail CI on `access_policy_enabled = false`, then still matching an `access_policy {` header in any resource without checking the enclosing type is `azurerm_key_vault`. Getting that last one right needs scope tracking, which wants an HCL parse rather than a fourth regex. Zero instances of either nested form exist under terraform/ or iac/, so the axis protects nothing today while blocking a security fix behind rounds on optional hardening. #1839 tracks closing the gap properly with hclparse/hclsyntax or `terraform show -json`. The script header, the CI job comment and the success message now state what the guard actually covers and name the nested form as a known gap. Fixtures and cases that existed only for that axis are removed; `.tf.json` still fails closed, and the report is still asserted to name the file and line. --- .github/workflows/ci.yml | 12 ++-- scripts/check-azure-kv-access-policy.sh | 68 ++++++++----------- scripts/test-azure-kv-access-policy.sh | 36 ++-------- .../dynamic-policy.tf.fixture | 26 ------- .../indented.tf.fixture | 3 +- .../inline-policy.tf.fixture | 26 ------- .../prefix-only.tf.fixture | 30 -------- 7 files changed, 42 insertions(+), 159 deletions(-) delete mode 100644 scripts/testdata/azure-kv-access-policy/dynamic-policy.tf.fixture delete mode 100644 scripts/testdata/azure-kv-access-policy/inline-policy.tf.fixture delete mode 100644 scripts/testdata/azure-kv-access-policy/prefix-only.tf.fixture diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7c58d178b..14b31a9cd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -736,12 +736,12 @@ jobs: - name: Run selector self-tests run: bash scripts/test-select-ecr-repos-to-delete.sh - # Assert that no Terraform file declares a Key Vault access-policy grant, - # either as an azurerm_key_vault_access_policy resource or as an access_policy - # block nested in the vault. 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). Fast (shell only), so it always runs. + # 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 diff --git a/scripts/check-azure-kv-access-policy.sh b/scripts/check-azure-kv-access-policy.sh index 7ed39e0dd..ce64aca2d 100755 --- a/scripts/check-azure-kv-access-policy.sh +++ b/scripts/check-azure-kv-access-policy.sh @@ -1,10 +1,8 @@ #!/usr/bin/env bash # check-azure-kv-access-policy.sh # -# Fails when any Terraform file declares a Key Vault access-policy grant, in -# either of the two forms HCL offers: a top-level -# `azurerm_key_vault_access_policy` resource, or an `access_policy` block nested -# inside an `azurerm_key_vault` resource (including the `dynamic` form). +# 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 @@ -35,18 +33,25 @@ # a matching string in a sibling file is not evidence that a role or an action # is real (issue #1794). # -# Exit 0 = no access-policy grant declared, in either form. +# 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. It matches block -# headers at any indentation: a `resource "azurerm_key_vault_access_policy"` -# header (the form the #1621 defect arrived in) and a nested `access_policy` or -# `dynamic "access_policy"` header (the second way to express the same inert -# grant). The nested header must carry its opening brace on the same line, which -# is what HCL requires of a real block but not of an inline comment wedged before -# it (`access_policy /* c */ {`); that contrived spelling is out of scope, since -# the defect this guards against is an accidental grant, not an obfuscated one. +# 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. # @@ -71,10 +76,6 @@ SCAN_ROOTS=( BANNED_RESOURCE='azurerm_key_vault_access_policy' -# The nested form: an `access_policy { ... }` block inside an azurerm_key_vault -# resource, or its `dynamic "access_policy"` equivalent. -BANNED_BLOCK='access_policy' - files=() if [[ $# -gt 0 ]]; then for arg in "$@"; do @@ -123,31 +124,19 @@ if [[ ${#files[@]} -eq 0 ]]; then exit 2 fi -# Match both block headers, tolerating leading whitespace and not requiring +# Match the block header, tolerating leading whitespace and not requiring # whitespace between tokens: `resource"azurerm_key_vault_access_policy""x"{` is -# valid HCL. The nested form is necessarily indented, and the top-level form is -# normalized to column 0 by the pre-commit `terraform_fmt` hook, which 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 cannot open a bypass. +# 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. -# -# The nested axis matches the COMPLETE block header, up to and including the -# opening brace, rather than the `access_policy` token alone. A bare prefix also -# fires on any longer identifier that starts with it (`access_policy_enabled`) -# and on an attribute assignment (`access_policy = "metadata"`, or the -# `access_policy.value` reference inside a dynamic block's content), none of -# which grant anything. Requiring the brace does not let a real block hide by -# wrapping: hclsyntax rejects a block whose body does not open on the header -# line ("Argument or block definition required"), so the header cannot be split -# across two lines to evade this. It does narrow the guard by the one spelling -# noted under LIMITATIONS above. violations=$( awk \ - -v resource_pattern="^[[:space:]]*resource[[:space:]]*\"${BANNED_RESOURCE}\"" \ - -v block_pattern="^[[:space:]]*(${BANNED_BLOCK}|dynamic[[:space:]]*\"${BANNED_BLOCK}\")[[:space:]]*[{]" ' - $0 ~ resource_pattern || $0 ~ block_pattern { + -v resource_pattern="^[[:space:]]*resource[[:space:]]*\"${BANNED_RESOURCE}\"" ' + $0 ~ resource_pattern { printf "%s:%d: %s\n", FILENAME, FNR, $0 } ' "${files[@]}" @@ -155,7 +144,7 @@ violations=$( if [[ -n "$violations" ]]; then { - echo "FAILED: Key Vault access-policy grant declared." + echo "FAILED: azurerm_key_vault_access_policy resource declared." echo "" echo "$violations" echo "" @@ -177,4 +166,5 @@ if [[ -n "$violations" ]]; then exit 1 fi -echo "OK: no Key Vault access-policy grant declared in ${#files[@]} Terraform file(s)." +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)" diff --git a/scripts/test-azure-kv-access-policy.sh b/scripts/test-azure-kv-access-policy.sh index be130b5da..3537af914 100755 --- a/scripts/test-azure-kv-access-policy.sh +++ b/scripts/test-azure-kv-access-policy.sh @@ -41,12 +41,6 @@ run_case() { run_case "role assignments and a prose mention exit 0" 0 \ "${FIXTURES}/clean.tf.fixture" -# The other half of the negative direction, and the reason the nested-block axis -# cannot be a prefix match: `access_policy_enabled`, `access_policy = ...` and -# `access_policy.value` all start with the block name and grant nothing. -run_case "identifiers that only share the access_policy prefix exit 0" 0 \ - "${FIXTURES}/prefix-only.tf.fixture" - # Positive direction: the anti-pattern must be caught. run_case "secret_permissions access policy exits 1" 1 \ "${FIXTURES}/access-policy.tf.fixture" @@ -59,8 +53,7 @@ run_case "key_permissions access policy exits 1" 1 \ # 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, -# and the nested access_policy form below is indented by construction. +# 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" @@ -70,16 +63,6 @@ run_case "indented access policy exits 1" 1 \ run_case "access policy with no inter-token whitespace exits 1" 1 \ "${FIXTURES}/nospace.tf.fixture" -# The second form of the same inert grant: an access_policy block nested inside -# the vault resource, and its dynamic equivalent. Neither declares the banned -# resource type, so a guard that only knew the top-level header would report -# both clean. -run_case "inline access_policy block exits 1" 1 \ - "${FIXTURES}/inline-policy.tf.fixture" - -run_case "dynamic access_policy block exits 1" 1 \ - "${FIXTURES}/dynamic-policy.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" @@ -106,10 +89,10 @@ run_case "cleanup-function module declares no access policy" 0 \ 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, in either -# form. This also pins the false-positive surface of the nested-block axis: 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 grant" 0 +# 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 @@ -134,17 +117,10 @@ run_report_case() { fi } -# The #1621 shape, on the top-level resource axis. -run_report_case "top-level violation report names the file and line" \ +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" -# The nested axis, pinned separately: it must report the block header line, not -# some other line in the file that happens to contain the token. -run_report_case "nested violation report names the block header line" \ - 'inline-policy\.tf\.fixture:17:[[:space:]]+access_policy[[:space:]]*[{]' \ - "${FIXTURES}/inline-policy.tf.fixture" - echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/azure-kv-access-policy/dynamic-policy.tf.fixture b/scripts/testdata/azure-kv-access-policy/dynamic-policy.tf.fixture deleted file mode 100644 index 4015722d5..000000000 --- a/scripts/testdata/azure-kv-access-policy/dynamic-policy.tf.fixture +++ /dev/null @@ -1,26 +0,0 @@ -# Fixture: the nested grant generated through a dynamic block. Must exit 1. -# -# Same inert grant as inline-policy.tf.fixture, reached through the `dynamic` -# form so the block name is quoted rather than bare. Asserted separately: a -# pattern anchored on a bare `access_policy` at the start of the line would -# catch the inline fixture and still miss this one. - -resource "azurerm_key_vault" "main" { - name = "cudly-kv" - location = var.location - resource_group_name = var.resource_group_name - tenant_id = data.azurerm_client_config.current.tenant_id - sku_name = "standard" - - enable_rbac_authorization = true - - dynamic "access_policy" { - for_each = var.reader_object_ids - - content { - tenant_id = data.azurerm_client_config.current.tenant_id - object_id = access_policy.value - secret_permissions = ["Get"] - } - } -} diff --git a/scripts/testdata/azure-kv-access-policy/indented.tf.fixture b/scripts/testdata/azure-kv-access-policy/indented.tf.fixture index 876da33c0..8ec77d604 100644 --- a/scripts/testdata/azure-kv-access-policy/indented.tf.fixture +++ b/scripts/testdata/azure-kv-access-policy/indented.tf.fixture @@ -5,8 +5,7 @@ # repo, both of this guard's scan roots included, because CI runs `pre-commit # run --all-files` (.github/workflows/pre-commit.yml). The guard does not lean # on that: a guard anchored strictly at column 0 would depend on a gate that -# could be narrowed later, and the nested access_policy form is indented by -# construction. This fixture pins the tolerance so it cannot be lost. +# could be narrowed later. This fixture pins the tolerance so it cannot be lost. resource "azurerm_key_vault_access_policy" "indented" { key_vault_id = var.key_vault_id diff --git a/scripts/testdata/azure-kv-access-policy/inline-policy.tf.fixture b/scripts/testdata/azure-kv-access-policy/inline-policy.tf.fixture deleted file mode 100644 index 5cc6dfd46..000000000 --- a/scripts/testdata/azure-kv-access-policy/inline-policy.tf.fixture +++ /dev/null @@ -1,26 +0,0 @@ -# Fixture: the nested form of the same inert grant. Must exit 1. -# -# An `access_policy { ... }` block declared inside the vault itself expresses -# exactly what azurerm_key_vault_access_policy does, and Azure ignores it for -# the same reason: enable_rbac_authorization = true, right above it. A guard -# that only knew the top-level resource header would report this file clean. - -resource "azurerm_key_vault" "main" { - name = "cudly-kv" - location = var.location - resource_group_name = var.resource_group_name - tenant_id = data.azurerm_client_config.current.tenant_id - sku_name = "standard" - - enable_rbac_authorization = true - - access_policy { - tenant_id = data.azurerm_client_config.current.tenant_id - object_id = azurerm_user_assigned_identity.workload.principal_id - - secret_permissions = [ - "Get", - "List" - ] - } -} diff --git a/scripts/testdata/azure-kv-access-policy/prefix-only.tf.fixture b/scripts/testdata/azure-kv-access-policy/prefix-only.tf.fixture deleted file mode 100644 index 6a49cb262..000000000 --- a/scripts/testdata/azure-kv-access-policy/prefix-only.tf.fixture +++ /dev/null @@ -1,30 +0,0 @@ -# Fixture: identifiers that merely START with `access_policy`. Must exit 0. -# -# None of these declares a block, so none grants anything. They exist because -# the nested-block axis was first written as a bare prefix match and fired on -# all of them, failing CI on valid Terraform. The two axes of this guard have -# now failed in opposite directions for the same reason -- once too narrow to -# see a nested grant, once too broad to tell one from a longer identifier -- so -# both directions are pinned by fixtures. - -resource "azurerm_key_vault" "main" { - name = "cudly-kv" - location = var.location - resource_group_name = var.resource_group_name - tenant_id = data.azurerm_client_config.current.tenant_id - sku_name = "standard" - - enable_rbac_authorization = true - - # A longer identifier that happens to share the prefix. - access_policy_enabled = false -} - -resource "some_other" "x" { - # An attribute assignment, not a block: no brace on the header line. - access_policy = "metadata" - - # The same token as a traversal, the shape a dynamic block's content uses to - # reference its iterator. - object_id = access_policy.value -}