[IAmJSD]

The Key Vault grant guard (scripts/check-azure-kv-access-policy.sh) now parses Terraform as HCL instead of matching lines. It reports the nested spellings of the inert grant only when they sit inside an azurerm_key_vault, and it no longer reads comment or heredoc text as a declaration.

Closes #204

Root cause

The guard matched one regex per line. A line can't tell you which resource encloses a block, or whether the text is inside a heredoc or a block comment. So the nested forms (access_policy { } and dynamic "access_policy" { } inside azurerm_key_vault) were left unchecked on main. The script said so itself (a nested access_policy block is NOT checked). The four earlier rounds in cloud-commitments-cli#1817 were each too narrow or too broad. Separately, main's resource-form regex still fired on the banned literal inside a heredoc or a /* ... */ comment.

Fix

• New cmd/check-azure-kv-access-policy (Go, about 200 lines). It parses each file with hashicorp/hcl/v2/hclsyntax, the parser Terraform uses, and walks the top-level blocks:
  • resource "azurerm_key_vault_access_policy" is reported. The parse also handles any indentation, no inter-token spacing, and unquoted labels.
  • In resource "azurerm_key_vault", a direct child block access_policy or dynamic "access_policy" is reported.
  • Nothing else is reported: the same block name in any other resource (for example azurerm_storage_share's acl { access_policy { } }), attributes such as access_policy_enabled = false or access_policy = "metadata", data sources, comments, and heredoc text.
  • Each finding is FILE:LINE: <header line as written>. Nested findings add (inside resource "azurerm_key_vault" "<name>").
  • Fail closed: a file that doesn't parse, .tf.json, a missing file, or no arguments gives exit 2, never 0.
• scripts/check-azure-kv-access-policy.sh is still the entry point. It keeps the argument checks, default scan roots (terraform/, iac/), .tf.json rejection and the remediation text. It now builds the checker into a mktemp -d dir and runs it. Exit 0 or 1 is passed through, and anything else (a build failure, a crash, Go missing from PATH) becomes exit 2. The header comment now describes the parse and its limits instead of the old "not checked" note.
• CI (.github/workflows/ci.yml, job azure-kv-access-policy) gains a Set up Go step, the same pinned actions/setup-go and GO_VERSION the other jobs use. The job was shell-only, so the checker needs this step.
• New dependency: github.com/hashicorp/hcl/v2 v2.25.0 (MPL-2.0), plus its transitive deps zclconf/go-cty, agext/levenshtein, apparentlymart/go-textseg/{v15,v17}, mitchellh/go-wordwrap, golang.org/x/{mod,tools}. Nothing already in the module graph parses HCL, since only HCL v1 was even in the local module cache. The issue explicitly asks for hclparse/hclsyntax over a fifth text matcher, and terraform show -json would need a terraform init/plan with providers. Only the CI checker imports it, so the server binary doesn't link it.

Verification

All of this is fixture/unit evidence: CI-only shell and Go checks, no cloud APIs, no terraform plan/apply.

Fail-before. I ran the new self-test against two old guards: main's guard, and the round-3 "whole block header" matcher from cli 704bdd3a, which is the one the issue describes. Each was dropped into a scratch copy of scripts/.

Self-test case	main's guard	round-3 regex	this PR
nested access_policy {} in a key vault exits 1	FAIL (exit 0)	pass	pass
nested dynamic "access_policy" in a key vault exits 1	FAIL (exit 0)	pass	pass
access_policy block/attribute outside a key vault exits 0	pass (not checked at all)	FAIL (exit 1, lines 19, 29, 33)	pass
comment and heredoc text exits 0	FAIL (exit 1: block comment line 6, heredoc line 28)	FAIL (exit 1: lines 6, 20, 28, 29)	pass
declaration after a heredoc reports only line 15	FAIL (2 lines: 10 and 15)	FAIL (2 lines)	pass
unparseable file exits 2 (alone, and next to a clean file)	FAIL (exit 1)	FAIL (exit 1)	pass
nested report names line and enclosing vault	FAIL	FAIL (has the line, no enclosing type)	pass
all six violating fixtures report one line each	FAIL (4 of 6)	pass	pass

Totals: main's guard 15 passed and 11 failed, round-3 18 passed and 8 failed, this PR 26 passed and 0 failed. I left one result out of those failure counts because it came from the harness: "default scan roots" also exited 2 under both old guards. The scratch tree symlinked terraform/ and iac/, and find doesn't follow symlinked roots. On the real tree it passes: OK: no Key Vault access policy (resource or nested block) declared in 207 Terraform file(s). That run also shows every real .tf file under both roots parses with hclsyntax.

Pass-after (this branch):

• bash scripts/test-azure-kv-access-policy.sh: Results: 26 passed, 0 failed. The 12 original cases still pass, and 14 are new.
• go test -count=1 ./cmd/check-azure-kv-access-policy/: ok. TestScanSource has 9 cases and asserts the exact rendered findings, with file and line. TestRunExitCodes has 6 cases, covering clean, violation, no args, missing file, a parse error next to a real finding (exit 2, no stdout), and JSON.
• Mutation probes on main.go, each restored afterwards:
  • Checking every resource's children, not only vaults: TestScanSource fails.
  • Treating a dynamic block of any label as a grant: TestScanSource fails. This one survived at first, so I added a dynamic "network_acls" case.
  • Ignoring parse errors: TestRunExitCodes fails.

Commands, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true and Go 1.26.9:

• go build ./..., go vet ./..., go vet -tags integration ./...: clean
• go mod tidy: stable after the change
• go test ./internal/mocks/ -count=1: ok
• golangci-lint v2.10.1 run --allow-parallel-runners ./cmd/check-azure-kv-access-policy/...: 0 issues. Two gosec taint findings (G703 on os.ReadFile(argv path), G705 on Fprintln to stdout) are suppressed with #nosec and a reason, following the repo's existing pattern.
• gocyclo -over 10 -ignore "_test\.go" cmd/check-azure-kv-access-policy/main.go: nothing over 10
• shellcheck 0.11.0 on scripts/check-azure-kv-access-policy.sh and scripts/test-azure-kv-access-policy.sh: clean. Six other untouched scripts/test-*.sh files have shellcheck findings on main; I left them alone.
• actionlint .github/workflows/ci.yml: clean. zizmor isn't installed locally.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ b56afb2b = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-204	76f85a4
fix/issue-438	78fe011
fix/issue-838	120b07f
fix/issue-143	aef1256
fix/issue-349	cfbb551
fix/issue-80	63405f9
fix/issue-199	36b9f8f
fix/issue-41	96fc237
fix/issue-299	6af14ed
fix/issue-324	3ab6729

fix/issue-324 and fix/issue-204 both edit .github/workflows/ci.yml next to the azure-kv-access-policy job. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on ed0c7174, which differs from b56afb2b only by one .gitallowed line on fix/issue-324:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1, 60-minute local timeout): internal/api, internal/purchase, internal/scheduler and internal/server all pass.
• Secrets: the first run caught a bare placeholder account ID in fix/issue-324's new tftest, now allowlisted with an anchored .gitallowed line. Re-run at b56afb2b: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Deploys

This changes only a CI guard, its self-test and a CI job step. Nothing deploys and no Terraform under terraform/ or iac/ changes. So the current Azure/GCP deploy pause doesn't block merging it.

Deliberately out of scope

• The attribute spelling inside a vault (access_policy = [ ... ], the azurerm provider's attributes-as-blocks mode) isn't reported. The issue's spec covers the block forms, and the documented use of the attribute form is access_policy = [] to clear every policy, which is harmless. Flagging non-empty forms would be a small follow-up if wanted. This is a judgment call for the owner.
• Only direct children of azurerm_key_vault are checked, which is where the provider schema puts access_policy. Modules are scanned as source files, not expanded, the same as before.
• .tf.json stays fail-closed (exit 2), as the issue requires. hcl/v2 could parse it, but that isn't needed today because none exist.
• The pre-existing shellcheck findings in other scripts/test-*.sh files.

Please mirror the issue's triage labels onto this PR: triaged, type/security, effort/m, priority/p3, severity/low, urgency/eventually, impact/internal.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[IAmJSD]

Merged origin/main (2374b4e) into this branch. New head: 05aa896. It still merges cleanly with the current main (8f186a8).

Conflict resolution:

• .github/workflows/ci.yml: main (fix(ci): verify the rollback image in AWS_ACCOUNT_ID's registry #1112) added the rollback-ecr-registry job directly above azure-kv-access-policy. This branch rewrote that job's header comment, because main's "the nested access_policy block form is deliberately not covered" note is no longer true once the guard parses HCL. I kept main's new job unchanged. This branch's comment and job, including the Set up Go step, follow it.

Checks on the merged head:

• build, vet (also with -tags integration) and e2e vet pass. go mod tidy -diff is clean.
• golangci-lint found 0 issues with and without the integration tag, and gocyclo is silent. go test -race -short ./cmd/check-azure-kv-access-policy passes.
• check-azure-kv-access-policy.sh reports OK on 207 Terraform files, test-azure-kv-access-policy.sh passes 26 of 26, and actionlint is clean.
• The secrets scan is clean and the planted key was flagged. The allowlist test passes.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 05aa8966390c8f235b668b04034b28ee0eb08a7b

Blocking:

• none

Non-blocking:

• scripts/check-azure-kv-access-policy.sh:143: the guard runs go build on every invocation, so the self-test builds the checker 26 times and an offline dev box without the hcl/v2 module cached gets exit 2 ("cannot check"). That fails closed, which is the right direction. It's only a speed/ergonomics point, and building once in the self-test (or honouring a prebuilt CHECKER= path) would fix it.
• Owner decision the PR surfaces correctly: the attribute spelling access_policy = [ {...} ] inside azurerm_key_vault (attributes-as-blocks mode) is still unreported. The issue's spec only covers the block forms, so leaving it out is within scope. A follow-up that flags only a non-empty list would close it.
• PR body says Go 1.26.9; CI pins 1.26.10. This is harmless.

Attacked:

• I read main.go against issue sec(iac/azure): scope nested access_policy detection to azurerm_key_vault, using an HCL parse rather than a fourth regex #204's must-catch/must-ignore lists. Scope is top-level resource blocks only, nested detection covers direct children of azurerm_key_vault only, dynamic requires the exact access_policy label, and parse errors, .json, a missing file or no args all return 2. The wrapper maps any other exit to 2, so a crash can't pass.
• Edge probe: unquoted labels (resource azurerm_key_vault kv {) plus a dynamic "access_policy" in a second vault. Both were reported with the right line and enclosing vault. An invalid single-line block in my first probe gave exit 2 (fail closed).
• Fail-before (mutation): I disabled the azurerm_key_vault case in scanTopLevel. go test ./cmd/check-azure-kv-access-policy/ FAILS, and scripts/test-azure-kv-access-policy.sh goes from 26/0 to 20 passed, 6 failed. After restoring it: 26 passed, 0 failed.
• Ran: go build ./..., go vet (with and without -tags integration) on ./cmd/..., go test -race -short -count=1 ./cmd/check-azure-kv-access-policy/, the guard on the real tree (207 files, exit 0), golangci-lint v2.10.1 (0 issues), gocyclo -over 10 (clean), shellcheck on both scripts (clean), and go mod tidy (no diff). In CI, the setup-go pin and env.GO_VERSION match the other jobs; the job keeps contents: read and stays in the CI Success needs list.

Body-stale:

• none

_{Reviewer: rB21. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 05aa8966390c8f235b668b04034b28ee0eb08a7b

Blocking:

• none

Non-blocking:

• Attribute spelling not reported. cmd/check-azure-kv-access-policy/main.go scanKeyVault skips the attribute spelling inside a vault (access_policy = [{ ... }], attributes-as-blocks mode). That spelling can carry real policies, not only = [], so it is a remaining bypass of the guard. The grant is inert on this repo's RBAC vault, so there is no security impact. It's an owner call whether to flag non-empty values (see owner notes). The script header documents the gap honestly.
• CI cost. scripts/test-azure-kv-access-policy.sh rebuilds the checker on every invocation (each self-test case calls the guard script, and the script runs go build into a fresh mktemp -d). It's cheap with the Go build cache, so this is only CI time.

Attacked:

• Ran the guard on the real tree. scripts/check-azure-kv-access-policy.sh reports OK across 207 files, so every .tf under both roots parses with hclsyntax and no current file trips it.
• Exit handling. Checked that the wrapper maps any checker exit other than 0 or 1 to 2. A missing go binary or a failed build also gives exit 2, and a parse error gives exit 2 even when the run also has a real finding. Only resource blocks are inspected, so data sources are excluded. Only direct children of azurerm_key_vault are checked, and dynamic blocks count only when their label is access_policy.
• CI wiring. setup-go is pinned to the same SHA and env.GO_VERSION as the other jobs. The guard isn't wired into pre-commit, so no other job newly needs Go. go mod tidy leaves go.mod and go.sum unchanged. hcl/v2 is imported only by the new cmd, so the server binary doesn't link it.
• Commands. go build ./..., go vet (with and without -tags integration) on the new package, go test -race -count=1 ./cmd/check-azure-kv-access-policy/ (ok), golangci-lint v2.10.1 (0 issues), gocyclo (silent), shellcheck on both scripts (clean), actionlint on ci.yml (clean), and bash scripts/test-azure-kv-access-policy.sh (26 passed).
• Fail-before. With the merge-base scripts/check-azure-kv-access-policy.sh restored, the new self-test gives 16 passed and 10 failed: nested and dynamic blocks exit 0, comment/heredoc exits 1, an unparseable file exits 1, and 4 of 6 fixtures are reported. With the fix it gives 26 passed and 0 failed.

Body-stale:

• The "Deploys" section says "Nothing deploys". The PR changes cmd/**, go.mod and go.sum, which are all in the push: branches: [main] path filter of .github/workflows/deploy-aws-lambda.yml. Merging will start the AWS Lambda deploy workflow, the same as any internal/** PR. The deployed app is functionally unchanged, but the merge is not deploy-free.

_{Reviewer: rB23. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 05aa8966390c8f235b668b04034b28ee0eb08a7b merged onto main (a9b7eb8f), together with #1247, #1248, #1295, #1303, #1310 in that order (combined tree a016458b, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 05aa8966 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 05aa8966390c8f235b668b04034b28ee0eb08a7b.