Skip to content

sec(iac/azure): detect nested Key Vault access_policy blocks with an HCL parse - #1303

Draft
IAmJSD wants to merge 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-204
Draft

IAmJSD wants to merge 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-204

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

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

…HCL parse

The Key Vault grant guard matched lines, so it could not see the nested
spellings of the inert grant (an access_policy block, static or dynamic,
inside azurerm_key_vault), and it read comment and heredoc text as
declarations. Four earlier line matchers each got one of those cases
wrong because a line has no enclosing resource type.

Parse the files with hashicorp/hcl/v2 (hclsyntax) in a small checker,
cmd/check-azure-kv-access-policy, which the existing script now runs:

- report the azurerm_key_vault_access_policy resource, and access_policy
  or dynamic "access_policy" blocks that are direct children of an
  azurerm_key_vault, naming file, line and the enclosing vault;
- ignore the same block name in any other resource, prefix-named and
  same-named attributes, and comment and heredoc text;
- exit 2 for a file that does not parse, as for .tf.json, never 0.

The CI job gains a setup-go step. The self-test adds fixtures for each
case and asserts the reported lines, not only exit codes.

Closes LeanerCloud#204

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added impact/internal Team-internal only triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/m Days type/security Security finding labels Oct 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

This branch has not been deployed

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

Labels

effort/m Days impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/security Security finding urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sec(iac/azure): scope nested access_policy detection to azurerm_key_vault, using an HCL parse rather than a fourth regex

1 participant