diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 566129bf3..43bfa8338 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1680,12 +1680,14 @@ jobs: - name: Run rollback verify-step self-tests run: bash scripts/test-rollback-ecr-registry.sh - # Assert that no Terraform file declares an azurerm_key_vault_access_policy - # resource. This project's only Key Vault sets enable_rbac_authorization = - # true, and an RBAC-enabled vault ignores access policies entirely, so such a - # grant applies cleanly and then does nothing at runtime (#1621). The nested - # `access_policy` block form is deliberately not covered; #1839 tracks it. - # Fast (shell only), so it always runs. + # Assert that no Terraform file declares a Key Vault access policy: neither + # an azurerm_key_vault_access_policy resource nor an access_policy block + # (static or dynamic) inside azurerm_key_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). The files are parsed as HCL by a small Go + # program (cmd/check-azure-kv-access-policy, #204), so the job needs Go; it + # is still fast, so it always runs. azure-kv-access-policy: name: Azure Key Vault grant model runs-on: ubuntu-latest @@ -1698,6 +1700,12 @@ jobs: with: persist-credentials: false + - name: Set up Go + uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 + with: + go-version: ${{ env.GO_VERSION }} + cache: true + - name: Assert no Key Vault access policies run: bash scripts/check-azure-kv-access-policy.sh diff --git a/cmd/check-azure-kv-access-policy/main.go b/cmd/check-azure-kv-access-policy/main.go new file mode 100644 index 000000000..80b328cb0 --- /dev/null +++ b/cmd/check-azure-kv-access-policy/main.go @@ -0,0 +1,200 @@ +// check-azure-kv-access-policy parses Terraform files with the HCL native-syntax +// parser and reports every Azure Key Vault access-policy grant they declare. +// +// It is the parsing half of scripts/check-azure-kv-access-policy.sh, which +// selects the files, rejects the JSON encoding and explains the remediation. +// Run that script rather than this binary directly. +// +// Three spellings express the same grant, and all three are reported: +// +// - a top-level `resource "azurerm_key_vault_access_policy"` block; +// - an `access_policy { ... }` block directly inside an +// `azurerm_key_vault` resource; +// - a `dynamic "access_policy" { ... }` block directly inside an +// `azurerm_key_vault` resource. +// +// The parse is what makes the nested forms decidable (issue #204). An +// `access_policy` block is only a Key Vault grant when the resource that +// encloses it is an azurerm_key_vault: other providers' resources +// (azurerm_storage_share's acl, for one) have an unrelated block of the same +// name. A line matcher has no enclosing type to check, and it cannot tell a +// comment or heredoc body from a declaration. The parser gives both directly. +// +// Output: one "FILE:LINE: HEADER" line per grant on stdout. +// Exit 0 = no grant found, 1 = at least one found, 2 = cannot check (usage +// error, unreadable file, JSON encoding, or a file that does not parse). A file +// that does not parse is never reported clean. +// +// Usage: +// +// go run ./cmd/check-azure-kv-access-policy FILE... +package main + +import ( + "bytes" + "fmt" + "io" + "os" + "strconv" + "strings" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" +) + +const ( + bannedResource = "azurerm_key_vault_access_policy" + keyVaultResource = "azurerm_key_vault" + nestedBlock = "access_policy" +) + +const ( + exitClean = 0 + exitViolation = 1 + exitCannotCheck = 2 +) + +// finding is one access-policy grant located in a parsed file. +type finding struct { + File string + Header string + // Inside names the enclosing resource for a nested grant; it is empty for + // the top-level resource form. + Inside string + Line int +} + +// String renders the finding as FILE:LINE: HEADER, naming the enclosing +// resource for a nested grant. +func (f finding) String() string { + s := fmt.Sprintf("%s:%d: %s", f.File, f.Line, f.Header) + if f.Inside != "" { + s += " (inside " + f.Inside + ")" + } + return s +} + +func main() { + os.Exit(run(os.Args[1:], os.Stdout, os.Stderr)) +} + +// run scans every file named in args and returns the process exit code. +func run(args []string, stdout, stderr io.Writer) int { + if len(args) == 0 { + fmt.Fprintln(stderr, "usage: check-azure-kv-access-policy FILE...") + return exitCannotCheck + } + var all []finding + for _, path := range args { + found, err := scanFile(path) + if err != nil { + fmt.Fprintf(stderr, "ERROR: %v\n", err) + return exitCannotCheck + } + all = append(all, found...) + } + for _, f := range all { + fmt.Fprintln(stdout, f) // #nosec G705 -- plain-text CI report on stdout, never rendered as HTML + } + if len(all) > 0 { + return exitViolation + } + return exitClean +} + +// scanFile reads and scans one file. Any file it cannot fully parse is an +// error, never an empty result. +func scanFile(path string) ([]finding, error) { + if strings.HasSuffix(path, ".json") { + return nil, fmt.Errorf("%s: the Terraform JSON encoding is not checked; refusing to report it clean", path) + } + // #nosec G304 G703 -- a local CI tool reading the files it was asked to + // check; the paths come from its own argv, not from a remote caller. + src, err := os.ReadFile(path) + if err != nil { + return nil, err + } + found, diags := scanSource(src, path) + if diags.HasErrors() { + return nil, fmt.Errorf("%s does not parse as HCL, so it cannot be checked:\n%s", path, diags.Error()) + } + return found, nil +} + +// scanSource parses src as HCL native syntax and returns every grant in it. +func scanSource(src []byte, filename string) ([]finding, hcl.Diagnostics) { + file, diags := hclsyntax.ParseConfig(src, filename, hcl.InitialPos) + if diags.HasErrors() { + return nil, diags + } + body, ok := file.Body.(*hclsyntax.Body) + if !ok { + return nil, hcl.Diagnostics{{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("unexpected body type %T", file.Body), + }} + } + lines := bytes.Split(src, []byte("\n")) + var out []finding + for _, block := range body.Blocks { + out = append(out, scanTopLevel(block, filename, lines)...) + } + return out, diags +} + +// scanTopLevel checks one top-level block. Only resource blocks can carry a +// grant: data sources expose access_policy as a read-only attribute. +func scanTopLevel(block *hclsyntax.Block, filename string, lines [][]byte) []finding { + if block.Type != "resource" || len(block.Labels) == 0 { + return nil + } + switch block.Labels[0] { + case bannedResource: + return []finding{newFinding(block, filename, lines, "")} + case keyVaultResource: + return scanKeyVault(block, filename, lines) + } + return nil +} + +// scanKeyVault reports the access_policy blocks declared directly inside an +// azurerm_key_vault resource, static or dynamic. +func scanKeyVault(vault *hclsyntax.Block, filename string, lines [][]byte) []finding { + inside := blockAddress(vault) + var out []finding + for _, child := range vault.Body.Blocks { + if isAccessPolicyBlock(child) { + out = append(out, newFinding(child, filename, lines, inside)) + } + } + return out +} + +func isAccessPolicyBlock(b *hclsyntax.Block) bool { + if b.Type == nestedBlock { + return true + } + return b.Type == "dynamic" && len(b.Labels) == 1 && b.Labels[0] == nestedBlock +} + +// blockAddress renders a block header as written in HCL, e.g. +// resource "azurerm_key_vault" "main". +func blockAddress(b *hclsyntax.Block) string { + parts := make([]string, 0, len(b.Labels)+1) + parts = append(parts, b.Type) + for _, l := range b.Labels { + parts = append(parts, strconv.Quote(l)) + } + return strings.Join(parts, " ") +} + +// newFinding records the block's header line, quoting the source text of that +// line so the report shows what the developer wrote. +func newFinding(b *hclsyntax.Block, filename string, lines [][]byte, inside string) finding { + line := b.TypeRange.Start.Line + header := blockAddress(b) + if line >= 1 && line <= len(lines) { + header = strings.TrimSpace(string(lines[line-1])) + } + return finding{File: filename, Line: line, Header: header, Inside: inside} +} diff --git a/cmd/check-azure-kv-access-policy/main_test.go b/cmd/check-azure-kv-access-policy/main_test.go new file mode 100644 index 000000000..ef264b972 --- /dev/null +++ b/cmd/check-azure-kv-access-policy/main_test.go @@ -0,0 +1,214 @@ +package main + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Each case asserts the exact rendered findings, file and line included: an +// exit-code-only assertion would accept a guard that fires on the wrong line. +func TestScanSource(t *testing.T) { + cases := []struct { + name string + src string + want []string + }{ + { + name: "resource form", + src: `resource "azurerm_key_vault_access_policy" "workload" { + key_vault_id = var.key_vault_id +} +`, + want: []string{`f.tf:1: resource "azurerm_key_vault_access_policy" "workload" {`}, + }, + { + name: "resource form without separators or quotes", + src: `resource"azurerm_key_vault_access_policy""a"{ +} +resource azurerm_key_vault_access_policy b { +} +`, + want: []string{ + `f.tf:1: resource"azurerm_key_vault_access_policy""a"{`, + `f.tf:3: resource azurerm_key_vault_access_policy b {`, + }, + }, + { + name: "nested static and dynamic forms at any indentation", + src: `resource "azurerm_key_vault" "main" { + name = "kv" +access_policy { + object_id = "x" + } + dynamic "access_policy" { + for_each = var.policies + content { + object_id = access_policy.value + } + } + network_acls { + default_action = "Deny" + } + dynamic "network_acls" { + for_each = [] + content {} + } + dynamic { + } +} +`, + want: []string{ + `f.tf:3: access_policy { (inside resource "azurerm_key_vault" "main")`, + `f.tf:6: dynamic "access_policy" { (inside resource "azurerm_key_vault" "main")`, + }, + }, + { + name: "access_policy block in another resource is not a vault grant", + src: `resource "azurerm_storage_share" "share" { + acl { + id = "x" + access_policy { + permissions = "r" + } + } +} +resource "azurerm_container_app" "app" { + access_policy { + mode = "metadata" + } + dynamic "access_policy" { + for_each = [] + content {} + } +} +`, + }, + { + name: "prefix-named and same-named attributes", + src: `resource "azurerm_key_vault" "main" { + access_policy_enabled = false +} +resource "azurerm_other" "x" { + access_policy = "metadata" +} +`, + }, + { + name: "access_policy deeper than a direct child of the vault", + src: `resource "azurerm_key_vault" "main" { + network_acls { + access_policy {} + } +} +`, + }, + { + name: "data source and other block types", + src: `data "azurerm_key_vault" "existing" { + name = "kv" +} +module "azurerm_key_vault_access_policy" { + source = "./x" +} +locals { + access_policy = {} +} +`, + }, + { + name: "comments", + src: `# resource "azurerm_key_vault_access_policy" "c" { +// resource "azurerm_key_vault_access_policy" "c" { +/* +resource "azurerm_key_vault_access_policy" "c" { +} +*/ +resource "azurerm_key_vault" "main" { + # access_policy { + /* dynamic "access_policy" { */ +} +`, + }, + { + name: "heredoc text is ignored and an adjacent declaration is caught", + src: `locals { + doc = <<-EOT + resource "azurerm_key_vault_access_policy" "in_heredoc" { + } + access_policy { + } + EOT +} +resource "azurerm_key_vault_access_policy" "real" { +} +`, + want: []string{`f.tf:9: resource "azurerm_key_vault_access_policy" "real" {`}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + found, diags := scanSource([]byte(tc.src), "f.tf") + require.False(t, diags.HasErrors(), diags.Error()) + got := make([]string, 0, len(found)) + for _, f := range found { + got = append(got, f.String()) + } + assert.Equal(t, tc.want, emptyAsNil(got)) + }) + } +} + +func emptyAsNil(s []string) []string { + if len(s) == 0 { + return nil + } + return s +} + +func writeFile(t *testing.T, dir, name, content string) string { + t.Helper() + p := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(p, []byte(content), 0o600)) + return p +} + +func TestRunExitCodes(t *testing.T) { + dir := t.TempDir() + clean := writeFile(t, dir, "clean.tf", "resource \"azurerm_key_vault\" \"main\" {\n}\n") + bad := writeFile(t, dir, "bad.tf", "resource \"azurerm_key_vault\" \"main\" {\n access_policy {\n }\n}\n") + broken := writeFile(t, dir, "broken.tf", "resource \"azurerm_key_vault\" \"main\" {\n") + jsonFile := writeFile(t, dir, "x.tf.json", "{}\n") + + cases := []struct { + name string + args []string + wantCode int + wantStdout string + wantStderr string + }{ + {name: "clean", args: []string{clean}, wantCode: exitClean}, + {name: "violation", args: []string{clean, bad}, wantCode: exitViolation, + wantStdout: bad + `:2: access_policy { (inside resource "azurerm_key_vault" "main")`}, + {name: "no args", wantCode: exitCannotCheck, wantStderr: "usage:"}, + {name: "missing file", args: []string{filepath.Join(dir, "nope.tf")}, wantCode: exitCannotCheck, wantStderr: "nope.tf"}, + // A file that does not parse must not be reported clean, even next to + // one with a real finding. + {name: "parse error", args: []string{bad, broken}, wantCode: exitCannotCheck, wantStderr: "does not parse as HCL"}, + {name: "json encoding", args: []string{jsonFile}, wantCode: exitCannotCheck, wantStderr: "JSON encoding"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + code := run(tc.args, &stdout, &stderr) + assert.Equal(t, tc.wantCode, code) + assert.Equal(t, tc.wantStdout, strings.TrimSpace(stdout.String())) + assert.Contains(t, stderr.String(), tc.wantStderr) + }) + } +} diff --git a/go.mod b/go.mod index 0b2c557b1..638570253 100644 --- a/go.mod +++ b/go.mod @@ -100,6 +100,7 @@ require ( github.com/go-jose/go-jose/v4 v4.1.4 github.com/golang-migrate/migrate/v4 v4.19.1 github.com/google/uuid v1.6.0 + github.com/hashicorp/hcl/v2 v2.25.0 github.com/jackc/pgx/v5 v5.9.2 github.com/pashagolub/pgxmock/v4 v4.9.0 github.com/testcontainers/testcontainers-go v0.42.0 @@ -122,6 +123,9 @@ require ( github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.54.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 // indirect github.com/Microsoft/go-winio v0.6.2 // indirect + github.com/agext/levenshtein v1.2.1 // indirect + github.com/apparentlymart/go-textseg/v15 v15.0.0 // indirect + github.com/apparentlymart/go-textseg/v17 v17.0.1 // indirect github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.8 // indirect github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.22 // indirect github.com/cenkalti/backoff/v4 v4.3.0 // indirect @@ -147,6 +151,7 @@ require ( github.com/klauspost/compress v1.18.7 // indirect github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect github.com/magiconair/properties v1.8.10 // indirect + github.com/mitchellh/go-wordwrap v1.0.1 // indirect github.com/moby/docker-image-spec v1.3.1 // indirect github.com/moby/go-archive v0.3.0 // indirect github.com/moby/moby/api v1.54.1 // indirect @@ -166,8 +171,11 @@ require ( github.com/tklauser/go-sysconf v0.3.16 // indirect github.com/tklauser/numcpus v0.11.0 // indirect github.com/yusufpapurcu/wmi v1.2.4 // indirect + github.com/zclconf/go-cty v1.19.0 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/contrib/detectors/gcp v1.44.0 // indirect go.opentelemetry.io/otel/sdk v1.45.0 // indirect go.opentelemetry.io/otel/sdk/metric v1.45.0 // indirect + golang.org/x/mod v0.41.0 // indirect + golang.org/x/tools v0.49.0 // indirect ) diff --git a/go.sum b/go.sum index 3a32f2465..3d4c40f45 100644 --- a/go.sum +++ b/go.sum @@ -102,6 +102,12 @@ github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261017134002- github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261017134002-3def3f898617/go.mod h1:03vjHt5wlHtZEFAFOhosA+paDSqacWnKg9cVa68QnJ0= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= +github.com/agext/levenshtein v1.2.1 h1:QmvMAjj2aEICytGiWzmxoE0x2KZvE0fvmqMOfy2tjT8= +github.com/agext/levenshtein v1.2.1/go.mod h1:JEDfjyjHDjOF/1e4FlBE/PkbqA9OfWu2ki2W0IB5558= +github.com/apparentlymart/go-textseg/v15 v15.0.0 h1:uYvfpb3DyLSCGWnctWKGj857c6ew1u1fNQOlOtuGxQY= +github.com/apparentlymart/go-textseg/v15 v15.0.0/go.mod h1:K8XmNZdhEBkdlyDdvbmmsvpAG721bKi0joRfFdHIWJ4= +github.com/apparentlymart/go-textseg/v17 v17.0.1 h1:bpMXRgQ5cEoRNuQke1a80/Nl6w3G5eoIbWo9f3gXkAs= +github.com/apparentlymart/go-textseg/v17 v17.0.1/go.mod h1:fa8X4jgGeevslICIY6LcdjkSecWnXmYd9Lk34z/VxZs= github.com/aws/aws-lambda-go v1.47.0 h1:0H8s0vumYx/YKs4sE7YM0ktwL2eWse+kfopsRI1sXVI= github.com/aws/aws-lambda-go v1.47.0/go.mod h1:dpMpZgvWx5vuQJfBt0zqBha60q7Dd7RfgJv23DymV8A= github.com/aws/aws-sdk-go-v2 v1.41.5 h1:dj5kopbwUsVUVFgO4Fi5BIT3t4WyqIDjGKCangnV/yY= @@ -216,6 +222,8 @@ github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/go-ole/go-ole v1.2.6 h1:/Fpf6oFPoeFik9ty7siob0G6Ke8QvQEuVcuChpwXzpY= github.com/go-ole/go-ole v1.2.6/go.mod h1:pprOEPIfldk/42T2oK7lQ4v4JSDwmV0As9GaiUsvbm0= +github.com/go-test/deep v1.0.3 h1:ZrJSEWsXzPOxaZnFteGEfooLba+ju3FYIbOrS+rQd68= +github.com/go-test/deep v1.0.3/go.mod h1:wGDj63lr65AM2AQyKZd/NYHGb0R+1RLqB8NKt3aSFNA= github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang-migrate/migrate/v4 v4.19.1 h1:OCyb44lFuQfYXYLx1SCxPZQGU7mcaZ7gH9yH4jSFbBA= @@ -235,6 +243,8 @@ github.com/googleapis/enterprise-certificate-proxy v0.3.14 h1:yh8ncqsbUY4shRD5dA github.com/googleapis/enterprise-certificate-proxy v0.3.14/go.mod h1:vqVt9yG9480NtzREnTlmGSBmFrA+bzb0yl0TxoBQXOg= github.com/googleapis/gax-go/v2 v2.21.0 h1:h45NjjzEO3faG9Lg/cFrBh2PgegVVgzqKzuZl/wMbiI= github.com/googleapis/gax-go/v2 v2.21.0/go.mod h1:But/NJU6TnZsrLai/xBAQLLz+Hc7fHZJt/hsCz3Fih4= +github.com/hashicorp/hcl/v2 v2.25.0 h1:HmmQVYRny4MaBo4b20TjmL46wyuUxpnMWkPZ4+NTbWk= +github.com/hashicorp/hcl/v2 v2.25.0/go.mod h1:vR+FKETxoZAmRlHgFfKmuqivj+C4Izm/c66XkmZ3r7M= github.com/jackc/pgerrcode v0.0.0-20220416144525-469b46aa5efa h1:s+4MhCQ6YrzisK6hFJUX53drDT4UsSW3DEhKn0ifuHw= github.com/jackc/pgerrcode v0.0.0-20220416144525-469b46aa5efa/go.mod h1:a/s9Lp5W7n/DD0VrVoyJ00FbP2ytTPDVOivvn2bMlds= github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM= @@ -263,6 +273,8 @@ github.com/magiconair/properties v1.8.10 h1:s31yESBquKXCV9a/ScB3ESkOjUYYv+X0rg8S github.com/magiconair/properties v1.8.10/go.mod h1:Dhd985XPs7jluiymwWYZ0G4Z61jb3vdS329zhj2hYo0= github.com/mdelapenya/tlscert v0.2.0 h1:7H81W6Z/4weDvZBNOfQte5GpIMo0lGYEeWbkGp5LJHI= github.com/mdelapenya/tlscert v0.2.0/go.mod h1:O4njj3ELLnJjGdkN7M/vIVCpZ+Cf0L6muqOG4tLSl8o= +github.com/mitchellh/go-wordwrap v1.0.1 h1:TLuKupo69TCn6TQSyGxwI1EblZZEsQ0vMlAFQflz0v0= +github.com/mitchellh/go-wordwrap v1.0.1/go.mod h1:R62XHJLzvMFRBbcrT7m7WgmE1eOyTSsCt+hzestvNj0= github.com/moby/docker-image-spec v1.3.1 h1:jMKff3w6PgbfSa69GfNg+zN/XLhfXJGnEx3Nl2EsFP0= github.com/moby/docker-image-spec v1.3.1/go.mod h1:eKmb5VW8vQEh/BAr2yvVNvuiJuY6UIocYsFu/DxxRpo= github.com/moby/go-archive v0.3.0 h1:nos4BtzzUIqB406BgQnWGMI4qib9BZ8XUHU+ucv/n1c= @@ -325,6 +337,10 @@ github.com/tklauser/numcpus v0.11.0 h1:nSTwhKH5e1dMNsCdVBukSZrURJRoHbSEQjdEbY+9R github.com/tklauser/numcpus v0.11.0/go.mod h1:z+LwcLq54uWZTX0u/bGobaV34u6V7KNlTZejzM6/3MQ= github.com/yusufpapurcu/wmi v1.2.4 h1:zFUKzehAFReQwLys1b/iSMl+JQGSCSjtVqQn9bBrPo0= github.com/yusufpapurcu/wmi v1.2.4/go.mod h1:SBZ9tNy3G9/m5Oi98Zks0QjeHVDvuK0qfxQmPyzfmi0= +github.com/zclconf/go-cty v1.19.0 h1:IV8WdqYZc2c5rLX9bEoLNXKojBAp0MZPBHMIrCoa/s4= +github.com/zclconf/go-cty v1.19.0/go.mod h1:12W89jGn3JCOIQi7infWr9m80rOkb5RNYJqXMZcN4c8= +github.com/zclconf/go-cty-debug v0.0.0-20240509010212-0d6042c53940 h1:4r45xpDWB6ZMSMNJFMOjqrGHynW3DIBuR2H9j0ug+Mo= +github.com/zclconf/go-cty-debug v0.0.0-20240509010212-0d6042c53940/go.mod h1:CmBdvvj3nqzfzJ6nTCIwDTPZ56aVGvDrmztiO5g3qrM= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= go.opentelemetry.io/contrib/detectors/gcp v1.44.0 h1:NmLfL734pJhM0JKaYd2Y28+nY9dPRWYAAbxhRCrKXPw= @@ -349,6 +365,8 @@ go.opentelemetry.io/otel/trace v1.45.0 h1:l/mP6Uv7oNO7/TblbhpbgMidxhq1uO/rPsikOy go.opentelemetry.io/otel/trace v1.45.0/go.mod h1:qoJJA2xNMnxRrdISU/kLtfUH2wNeQbiv+jhs/CxI8bc= golang.org/x/crypto v0.57.0 h1:3ZVCjf8Ggz7zneR/EHRVx68Ctf+2pmIMP2UFhh9cC6M= golang.org/x/crypto v0.57.0/go.mod h1:Fdz0i5U6CoizGwLda9DttjSk6qlZo25zYNtR+ycvuZA= +golang.org/x/mod v0.41.0 h1:qJmnOUb4YB+FsEuM3HcWucdZASCPGhsX6uljO6pog0c= +golang.org/x/mod v0.41.0/go.mod h1:Ek9pY8RKWXwsWvd3rQiHYtMqkjSUV+s1Rj7j4H5Ur6o= golang.org/x/net v0.60.0 h1:79p50tfZlm0J9YfoDsSi639qSXNGVwEzOPLCxM2FsYU= golang.org/x/net v0.60.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= @@ -367,6 +385,8 @@ golang.org/x/text v0.42.0 h1:JbOZXgfeCPU9gacVtYliJqOhD+zhrEqK4LfdpmlUZqI= golang.org/x/text v0.42.0/go.mod h1:ojzP1Z+2QtioaF8DTtO8K5q7JWVVYwZKenzujK0Zd0E= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= +golang.org/x/tools v0.49.0 h1:3NI7VXzL9+1WZD52Dx2ttoPwD5DWrFGpl9mFZDlmisI= +golang.org/x/tools v0.49.0/go.mod h1:SJNXV9DBKT0UbdttsQjbfJlAE/q+y36++zo3uL3N0Oo= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E= diff --git a/scripts/check-azure-kv-access-policy.sh b/scripts/check-azure-kv-access-policy.sh index ce64aca2d..3023571b5 100755 --- a/scripts/check-azure-kv-access-policy.sh +++ b/scripts/check-azure-kv-access-policy.sh @@ -1,8 +1,12 @@ #!/usr/bin/env bash # check-azure-kv-access-policy.sh # -# Fails when any Terraform file declares a top-level -# `azurerm_key_vault_access_policy` resource. +# Fails when any Terraform file declares an Azure Key Vault access-policy grant, +# in any of its three spellings: +# +# resource "azurerm_key_vault_access_policy" "..." { ... } +# resource "azurerm_key_vault" "..." { access_policy { ... } } +# resource "azurerm_key_vault" "..." { dynamic "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 @@ -33,28 +37,34 @@ # 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 any of the three spellings. # Exit 1 = at least one found; each is printed to stderr. -# Exit 2 = usage error, or a file this scanner cannot read. +# Exit 2 = usage error, or a file this scanner cannot read or parse. # -# 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. +# HOW IT DECIDES. The files are parsed with HashiCorp's HCL parser (the one +# Terraform uses), by cmd/check-azure-kv-access-policy, rather than matched line +# by line. An `access_policy` block is a Key Vault grant only when the resource +# enclosing it is an `azurerm_key_vault`; other providers' resources +# (azurerm_storage_share's acl, for one) have an unrelated block of that name, +# and the parse names the enclosing type directly. It also means text inside a +# comment or a heredoc is never read as a declaration, block headers are found +# at any indentation and with any token spacing, and attributes such as +# `access_policy_enabled = false` are not blocks at all. Four successive line +# matchers each got one of those cases wrong (issue #204). # -# 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. +# LIMITATIONS. Only the direct children of an azurerm_key_vault are checked, +# which is where the provider schema puts access_policy. The attribute spelling +# of the same argument (`access_policy = [...]`, the provider's +# attributes-as-blocks mode, whose documented use is `= []` to clear every +# policy) is not reported. Modules are checked as source files, not expanded. # -# The Terraform JSON encoding (`.tf.json`) is not parsed; rather than +# A file the parser rejects is reported as exit 2 (cannot check), never clean. +# The Terraform JSON encoding (`.tf.json`) is not parsed either; rather than # report such a file as clean, the guard exits 2. None exist today. # +# The parser needs Go on PATH (the version in go.mod); without it the guard +# exits 2. +# # Usage: # scripts/check-azure-kv-access-policy.sh # scan the default roots # scripts/check-azure-kv-access-policy.sh FILE... # scan exactly these files @@ -74,8 +84,6 @@ SCAN_ROOTS=( "${REPO_ROOT}/iac" ) -BANNED_RESOURCE='azurerm_key_vault_access_policy' - files=() if [[ $# -gt 0 ]]; then for arg in "$@"; do @@ -124,34 +132,41 @@ if [[ ${#files[@]} -eq 0 ]]; then exit 2 fi -# Match the block header, tolerating leading whitespace and not requiring -# whitespace between tokens: `resource"azurerm_key_vault_access_policy""x"{` is -# valid HCL. The pre-commit `terraform_fmt` hook normalizes a top-level block -# back to column 0, and it covers every .tf file in the repo (both scan roots) -# because CI runs `pre-commit run --all-files` in -# .github/workflows/pre-commit.yml. The guard does not lean on that gate: -# tolerating indentation means narrowing it later cannot open a bypass here. -# Anchoring on `resource` still means a mention of the type inside a comment -# (this file's own guidance, for one) does not trip the guard. -violations=$( - awk \ - -v resource_pattern="^[[:space:]]*resource[[:space:]]*\"${BANNED_RESOURCE}\"" ' - $0 ~ resource_pattern { - printf "%s:%d: %s\n", FILENAME, FNR, $0 - } - ' "${files[@]}" -) +if ! command -v go >/dev/null 2>&1; then + echo "ERROR: go is not on PATH; the HCL parser this guard runs is a Go program" >&2 + exit 2 +fi + +work_dir="$(mktemp -d)" +trap 'rm -rf "$work_dir"' EXIT +checker="${work_dir}/check-azure-kv-access-policy" +if ! (cd "$REPO_ROOT" && go build -o "$checker" ./cmd/check-azure-kv-access-policy); then + echo "ERROR: could not build cmd/check-azure-kv-access-policy" >&2 + exit 2 +fi + +# The checker prints one FILE:LINE: HEADER line per grant on stdout, and exits +# 0 (none), 1 (found) or 2 (cannot check). Anything other than 0 or 1 is passed +# on as 2, so a crash can never read as a pass or as a finding. +checker_exit=0 +violations="$("$checker" "${files[@]}")" || checker_exit=$? +case "$checker_exit" in + 0|1) ;; + *) exit 2 ;; +esac -if [[ -n "$violations" ]]; then +if [[ "$checker_exit" -eq 1 ]]; then { - echo "FAILED: azurerm_key_vault_access_policy resource declared." + echo "FAILED: Azure 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 "does nothing at runtime. That holds for a standalone resource and for an" + echo "access_policy block inside the vault alike. Remove it and use an RBAC" + echo "role assignment instead:" echo "" echo " resource \"azurerm_role_assignment\" \"\" {" echo " scope = var.key_vault_id" @@ -166,5 +181,4 @@ if [[ -n "$violations" ]]; then exit 1 fi -echo "OK: no azurerm_key_vault_access_policy resource declared in ${#files[@]} Terraform file(s)." -echo " (a nested access_policy block is NOT checked -- see #1839)" +echo "OK: no Key Vault access policy (resource or nested block) 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 3537af914..e33c55c88 100755 --- a/scripts/test-azure-kv-access-policy.sh +++ b/scripts/test-azure-kv-access-policy.sh @@ -63,6 +63,34 @@ 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 nested spellings (issue #204): an access_policy block, static or +# dynamic, inside azurerm_key_vault is the same inert grant as the resource. +run_case "nested access_policy block in a key vault exits 1" 1 \ + "${FIXTURES}/nested-inline.tf.fixture" + +run_case "nested dynamic access_policy block in a key vault exits 1" 1 \ + "${FIXTURES}/nested-dynamic.tf.fixture" + +# The negatives every earlier line matcher got wrong: the block name inside a +# resource that is not a vault, prefix-named and same-named attributes. +run_case "access_policy outside a key vault exits 0" 0 \ + "${FIXTURES}/other-resources.tf.fixture" + +# The banned spellings as comment and heredoc text are not declarations. +run_case "comment and heredoc text exits 0" 0 \ + "${FIXTURES}/comments-heredocs.tf.fixture" + +# Skipping heredoc text must not skip what follows it. +run_case "declaration after a heredoc quoting it exits 1" 1 \ + "${FIXTURES}/heredoc-adjacent.tf.fixture" + +# A file the parser rejects cannot be checked, so it is exit 2, never 0. +run_case "unparseable file exits 2" 2 \ + "${FIXTURES}/syntax-error.tf.fixture" + +run_case "unparseable file next to a clean one exits 2" 2 \ + "${FIXTURES}/clean.tf.fixture" "${FIXTURES}/syntax-error.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" @@ -90,9 +118,9 @@ run_case "aks module declares no access policy" 0 \ "${REPO_ROOT}/terraform/modules/compute/azure/aks/main.tf" # The default scan roots (terraform/ + iac/) must hold no violation. This also -# pins the guard's false-positive surface: it runs over every real Terraform -# file in the tree, none of which may trip it. -run_case "default scan roots declare no access-policy resource" 0 +# pins the guard's false-positive surface: it parses every real Terraform file +# in the tree, none of which may trip it or fail to parse. +run_case "default scan roots declare no access-policy grant" 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 @@ -121,6 +149,49 @@ 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" +run_report_case "nested block report names the line and the enclosing vault" \ + 'nested-inline\.tf\.fixture:16: access_policy \{ \(inside resource "azurerm_key_vault" "main"\)' \ + "${FIXTURES}/nested-inline.tf.fixture" + +run_report_case "nested dynamic block report names the line and the enclosing vault" \ + 'nested-dynamic\.tf\.fixture:23: dynamic "access_policy" \{ \(inside resource "azurerm_key_vault" "main"\)' \ + "${FIXTURES}/nested-dynamic.tf.fixture" + +run_report_case "heredoc-adjacent report names the declaration" \ + 'heredoc-adjacent\.tf\.fixture:15: resource "azurerm_key_vault_access_policy" "real"' \ + "${FIXTURES}/heredoc-adjacent.tf.fixture" + +# The report must list exactly the grants, one line each: a line for the +# heredoc's quoted text (line 10) or for the vault's network_acls block would +# mean the guard is reading text or the wrong block. +run_count_case() { + local label="$1" + local expected="$2" + shift 2 + + local count + count="$("$CHECK" "$@" 2>&1 >/dev/null | grep -cE '\.tf\.fixture:[0-9]+: ' || true)" + + if [[ "$count" -eq "$expected" ]]; then + echo "PASS: $label" + (( pass++ )) || true + else + echo "FAIL: $label (expected $expected reported line(s), got $count)" + (( fail++ )) || true + fi +} + +run_count_case "heredoc-adjacent reports only the real declaration" 1 \ + "${FIXTURES}/heredoc-adjacent.tf.fixture" + +run_count_case "nested-dynamic reports only the access_policy block" 1 \ + "${FIXTURES}/nested-dynamic.tf.fixture" + +run_count_case "all violating fixtures report one line each" 6 \ + "${FIXTURES}/access-policy.tf.fixture" "${FIXTURES}/key-permissions.tf.fixture" \ + "${FIXTURES}/indented.tf.fixture" "${FIXTURES}/nospace.tf.fixture" \ + "${FIXTURES}/nested-inline.tf.fixture" "${FIXTURES}/nested-dynamic.tf.fixture" + echo "" echo "Results: ${pass} passed, ${fail} failed." [[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/azure-kv-access-policy/comments-heredocs.tf.fixture b/scripts/testdata/azure-kv-access-policy/comments-heredocs.tf.fixture new file mode 100644 index 000000000..e600e7fee --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/comments-heredocs.tf.fixture @@ -0,0 +1,33 @@ +# Fixture: the banned spellings as text, never as declarations. Must exit 0. +# +# resource "azurerm_key_vault_access_policy" "in_a_hash_comment" { +// resource "azurerm_key_vault_access_policy" "in_a_slash_comment" { +/* +resource "azurerm_key_vault_access_policy" "in_a_block_comment" { +} +*/ + +resource "azurerm_key_vault" "main" { + name = "kv-fixture" + 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 { + /* + dynamic "access_policy" { + } + */ +} + +output "migration_note" { + value = <<-EOT + Grants used to be written as: + resource "azurerm_key_vault_access_policy" "in_a_heredoc" { + access_policy { + } + } + EOT +} diff --git a/scripts/testdata/azure-kv-access-policy/heredoc-adjacent.tf.fixture b/scripts/testdata/azure-kv-access-policy/heredoc-adjacent.tf.fixture new file mode 100644 index 000000000..6105dfeed --- /dev/null +++ b/scripts/testdata/azure-kv-access-policy/heredoc-adjacent.tf.fixture @@ -0,0 +1,21 @@ +# Fixture: a real declaration right after a heredoc quoting the same literal. +# Must exit 1, naming the declaration on line 15 and NOT the heredoc's line 10. +# +# Ignoring heredoc text is only half the requirement: a guard that skipped the +# whole region (or the rest of the file) after a heredoc would report this file +# clean. + +locals { + example = <