From 06e86e6fcf6ceb7935e2dff401f7d11152a796b0 Mon Sep 17 00:00:00 2001 From: DuqueOM Date: Sat, 5 Sep 2026 12:22:41 -0600 Subject: [PATCH] fix(infra): GKE control plane could be public with no allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closing out ADR-046 meant re-reading the justification for the one surviving suppression, GCP-0061. One clause did not hold: Environment overlays MUST supply non-empty master_authorized_networks for staging/prod (enforced by the variable validation rule in variables.tf). There is no such validation rule, and nothing in the repo sets the variable — no overlay, no tfvars. It defaults to [], so the `dynamic` master_authorized_networks_config block never rendered, and GKE with no authorized-networks block applies NO restriction. That is harmless while enable_private_endpoint = true, which is the default. It is a publicly reachable control plane with no allowlist the moment an adopter takes the documented dev opt-out and sets it false. So the finding was not a false positive. It was pointing at a real gap, held open for two releases by a compensating control that had never been built. Fixed in the module rather than re-suppressed: - The block is now unconditional. An empty list means "enabled, no external CIDR allowed" — the restrictive reading — instead of "not configured", which GKE reads permissively. This also makes the configuration statically visible, which is why GCP-0061 clears. - A precondition on google_container_cluster.gke rejects enable_private_endpoint = false together with an empty list, at plan time rather than at apply. It lives on the resource rather than in a `validation` block because the condition spans two variables — variable validation could not reference other variables until Terraform 1.9, and this module targets >= 1.7. Verified across all three combinations in an isolated config: private endpoint passes, public with CIDRs passes, public without CIDRs fails with the intended message. terraform validate and fmt clean; trivy config reports 0 HIGH/CRITICAL across all five root modules with no ignore file at all. .security-baselines/trivy-config.trivyignore is now empty. Suppressions across the repo: zero, and the 2027-01-01 expiry no longer exists. docs/audit/baseline-review.md carries the third dated review, including what the review procedure itself missed: reviewing a suppression had meant reading its rationale, not verifying it. Step 3 now says to confirm the compensating control exists in the file the justification names — the step that would have caught this a review earlier. Adopter-visible, both rows in MIGRATION.md. A plan that starts failing with "public control plane must be constrained" is the fix working. Co-Authored-By: Claude Opus 5 --- .security-baselines/trivy-config.trivyignore | 40 +++++-------- CHANGELOG.md | 37 ++++++++++++ MIGRATION.md | 11 ++++ docs/audit/baseline-review.md | 60 ++++++++++++++++--- ...ADR-046-tfsec-to-trivy-config-migration.md | 27 +++++++++ .../service/infra/terraform/gcp/compute.tf | 54 ++++++++++++++--- .../service/infra/terraform/gcp/variables.tf | 13 +++- 7 files changed, 198 insertions(+), 44 deletions(-) diff --git a/.security-baselines/trivy-config.trivyignore b/.security-baselines/trivy-config.trivyignore index 8593d0e..d77e42a 100644 --- a/.security-baselines/trivy-config.trivyignore +++ b/.security-baselines/trivy-config.trivyignore @@ -1,28 +1,20 @@ # Trivy config (Terraform IaC) baseline — ADR-046. # -# Same contract as every other file in this directory: each accepted finding -# is explicit, dated and reviewable, and `scripts/check_baselines_expiry.py` -# fails CI once an entry is past its `# expiry:`. +# Currently EMPTY, and that is the intended steady state: no accepted +# misconfiguration findings. An empty baseline is valid — see +# `scripts/check_baselines_expiry.py`, which treats a file with no entries as +# nothing to expire. # -# Why the expiry lives in a comment rather than trivy's own `expiredAt:` -# field: trivy 0.71.0 accepts `expiredAt` in the YAML ignore format and does -# NOT honour it. Measured on this repo — an entry dated 2020-01-01 still -# suppressed its finding, in all three date formats tried, with no warning on -# stderr. Delegating expiry to the tool would have produced exactly the -# failure this directory exists to prevent: a suppression that outlives its -# justification while the gate reports green. - -# GKE master authorized networks. +# It held one entry, GCP-0061 (GKE master authorized networks), inherited +# from tfsec's google-gke-enable-master-networks. That suppression was +# justified on the grounds that the HCL was correct and only the scanner +# could not see it, because the block was `dynamic`. The justification also +# claimed the invariant was "enforced by the variable validation rule in +# variables.tf" — there was no such rule. The finding was pointing at a real +# gap in the `enable_private_endpoint = false` path, and the module was fixed +# rather than the finding re-suppressed. # -# `master_authorized_networks_config` IS configured, as a `dynamic` block at -# templates/service/infra/terraform/gcp/compute.tf:38, gated on -# `var.master_authorized_networks`. Static analysis does not evaluate dynamic -# blocks, so the finding fires against the cluster resource at compute.tf:10 -# even though the HCL is correct. Staging and prod MUST supply a non-empty -# `master_authorized_networks`, enforced by a variable validation rule in -# variables.tf. -# -# tfsec reported the same false positive as google-gke-enable-master-networks; -# it is the one suppression of three that did NOT dissolve in the migration. -# expiry: 2027-01-01 reason: ADR-046 — dynamic block not evaluated by static analysis -GCP-0061 +# Contract for anything added here: explicit, dated and reviewable. Each +# entry carries `# expiry: YYYY-MM-DD` on the line above it or inline, and +# CI fails once past that date. Do NOT use trivy's own `expiredAt:` field — +# 0.71.0 accepts it and does not honour it (measured; see ADR-046). diff --git a/CHANGELOG.md b/CHANGELOG.md index 7219b10..56c4e53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,43 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and [Sem ## [Unreleased] +### Fixed — the GKE control plane could be public with no allowlist, and the suppression cited a control that did not exist + +- `master_authorized_networks_config` was a `dynamic` block gated on the list + being non-empty. The variable defaults to `[]` and **nothing in the repo + sets it** — no overlay, no tfvars — so the block never rendered, and GKE + with no authorized-networks block applies **no restriction**. +- Harmless while `enable_private_endpoint = true`, which is the default. A + **publicly reachable control plane with no allowlist** the moment an + adopter takes the documented dev opt-out and sets it false. +- `GCP-0061` — and `google-gke-enable-master-networks` before it — had been + suppressed for two releases as a scanner limitation, on a justification + that read: *"enforced by the variable validation rule in `variables.tf`"*. + **There is no such rule.** The finding was not a false positive; it was + pointing at a real gap held open by a compensating control that was never + built. +- **Fixed in the module, not re-suppressed:** + - the block is now unconditional, so an empty list means *"enabled, no + external CIDR allowed"* — the restrictive reading — rather than *"not + configured"*; + - a `precondition` on `google_container_cluster.gke` rejects + `enable_private_endpoint = false` together with an empty list, at **plan** + time. It lives on the resource rather than in a `validation` block + because the condition spans two variables, which variable validation + could not do before Terraform 1.9 and this module targets `>= 1.7`. + - Verified across all three combinations: private endpoint passes, public + with CIDRs passes, public without CIDRs fails. +- `.security-baselines/trivy-config.trivyignore` is now **empty**. + Suppressions across the whole repo: **zero**, and the 2027-01-01 expiry no + longer exists. `docs/audit/baseline-review.md` carries the third dated + review, including what the review procedure itself missed: reviewing a + suppression had meant reading its rationale, not verifying it. Step 3 now + says to confirm the compensating control exists in the file the + justification names. +- Adopter-visible; both rows are in `MIGRATION.md`. A plan that starts + failing with *"public control plane must be constrained"* is the fix + working. + ### Changed — the security check is named for what it does, not for its tools - `Self-audit (gitleaks + tfsec + checkov + trivy fs)` → diff --git a/MIGRATION.md b/MIGRATION.md index c261a6c..6f835d6 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -17,6 +17,17 @@ contract that prevents future versions from breaking adopters silently. --- +## v0.26.0 → next (unreleased) + +| Change | Manual action required | +|--------|------------------------| +| **GKE `master_authorized_networks_config` is now always emitted** | Usually none. Previously the block was `dynamic` and rendered only when `master_authorized_networks` was non-empty; with the default empty list GKE received no authorized-networks configuration, which it reads as **no restriction**. The block is now unconditional, so an empty list means "enabled, no external CIDR allowed" — the restrictive reading. If your control plane is private (`enable_private_endpoint = true`, the default) this changes nothing you can observe. If it is public and you were relying on unrestricted access, `terraform plan` will now stop you — see the next row. | +| **`enable_private_endpoint = false` now requires `master_authorized_networks`** | A `precondition` on `google_container_cluster.gke` fails at **plan** time if you expose the control plane publicly without listing the CIDRs allowed to reach it. If your plan starts failing with *"public control plane must be constrained"*, that is the intended behaviour and it is telling you the cluster was reachable from anywhere. Either revert to the private endpoint, or supply `master_authorized_networks` with your VPN/office/CI ranges. The old suppression's justification claimed this pairing was already "enforced by the variable validation rule in `variables.tf`"; no such rule existed (ADR-046 §Amendment). | +| **tfsec replaced by `trivy config`** (ADR-046) | None for existing infrastructure — this is a scanner swap, not a resource change. Scaffolded services get `aquasecurity/trivy-action` in `ci-infra.yml` instead of `aquasecurity/tfsec-action`; tfsec is archived upstream. If you added your own entries to the old .security-baselines/tfsec.yml, port them to `.security-baselines/trivy-config.trivyignore` using Trivy check ids. **Do not use Trivy's `expiredAt:` field** — 0.71.0 accepts and ignores it; keep the `# expiry:` comment form the repo's own gate enforces. | +| **`make scaffold-update` requires `TEMPLATE_REF`** | Run `make scaffold-update TEMPLATE_REF=`. A bare invocation now refuses to run instead of updating from whatever tag sorts highest — and it previously reused `REF`, which `ci-green` defaults to `main`, so it could pull the moving development branch. | + +--- + ## v0.25.0 → v0.26.0 (2026-08-08) | Change | Manual action required | diff --git a/docs/audit/baseline-review.md b/docs/audit/baseline-review.md index 5110761..770b8d5 100644 --- a/docs/audit/baseline-review.md +++ b/docs/audit/baseline-review.md @@ -92,18 +92,60 @@ finding, silently. `scripts/check_baselines_expiry.py` remains the expiry authority, and it now discovers baseline files instead of naming three of them literally, so a future format swap cannot leave a file unwatched. +## Review — 2026-09-05 (second entry, same day): the last suppression is gone + +Closing ADR-046 out meant re-reading the one surviving justification, and a +clause in it did not hold: + +> Environment overlays MUST supply non-empty `master_authorized_networks` +> for staging/prod (enforced by the variable validation rule in +> `variables.tf`). + +There was no such rule, and nothing sets the variable anywhere — it defaults +to `[]`. The `dynamic` block therefore never rendered, and GKE with no +authorized-networks block applies *no restriction*. Safe while +`enable_private_endpoint = true` (the default); a public control plane with +no allowlist as soon as an adopter takes the documented dev opt-out. + +`GCP-0061` was not a false positive. It was pointing at a real gap, held open +by a compensating control that had never been built. + +**Fixed in the module rather than re-suppressed:** + +- the authorized-networks block is now unconditional, so an empty list means + "enabled, no external CIDR allowed" instead of "not configured"; +- a `precondition` on `google_container_cluster.gke` rejects + `enable_private_endpoint = false` together with an empty list, at plan + time. Verified across all three combinations, including that the safe ones + still pass. + +**Suppressions in force: zero.** +`.security-baselines/trivy-config.trivyignore` is empty, which is the +intended steady state. + +### What this says about the review itself + +The 2026-09-04 review recorded all three as "tool limitations, not accepted +risks" and I wrote that in good faith from the justifications on file. Two of +the three were. The third's justification asserted an enforcement that did +not exist, and no review had checked it — because reviewing a suppression had +meant reading its rationale, not verifying it. + +Step 3 of the procedure below now says to confirm the compensating control +exists **in the file the justification names**. That step is what would have +caught this, and it is why it is written the way it is. + ## Next review -**Due 2027-01-01**, when the single remaining suppression (`GCP-0061`) -expires and CI fails until it is re-justified or removed. +**No suppression is due.** Both baseline files are empty of accepted +findings, so `check_baselines_expiry.py` has nothing to expire and the +2027-01-01 date no longer exists. -It will almost certainly still be true: no static analyser evaluates -Terraform `dynamic` blocks, so the finding is structural rather than a gap -in one tool. The way to actually close it is to stop needing the dynamic -block — for example by making `master_authorized_networks` a required -variable with a non-empty default for staging and prod, so the block can be -static. That is a module design decision, not a scanner decision, and it is -the question the 2027-01-01 review should answer. +The next review is therefore calendar-driven rather than deadline-driven: +**due 2026-12-05**, one quarter out, to confirm the state is still zero. A +review with nothing to renew is the point of the exercise, not a reason to +skip it — the failure mode this document exists to prevent is a suppression +nobody remembers making. ## How to run a review diff --git a/docs/decisions/ADR-046-tfsec-to-trivy-config-migration.md b/docs/decisions/ADR-046-tfsec-to-trivy-config-migration.md index ab86f23..36f3c65 100644 --- a/docs/decisions/ADR-046-tfsec-to-trivy-config-migration.md +++ b/docs/decisions/ADR-046-tfsec-to-trivy-config-migration.md @@ -58,6 +58,33 @@ modules. They do not block, and triaging them is separate work. **Two of three suppressions dissolve. Three become one.** +### Amendment (2026-09-05): the third one dissolved too, and it was not a false positive + +The surviving suppression was carried on the grounds that the HCL is correct +and only the scanner cannot see it. Re-reading its justification while +closing this ADR out, one clause did not hold: + +> Environment overlays MUST supply non-empty `master_authorized_networks` +> for staging/prod (enforced by the variable validation rule in +> `variables.tf`). + +**There is no such validation rule**, and no overlay or tfvars file sets the +variable anywhere. It defaults to `[]`, the `dynamic` block therefore never +rendered, and GKE's default with no authorized-networks block is *no +restriction*. Harmless while `enable_private_endpoint = true` — the default — +and a publicly reachable control plane the moment an adopter takes the +documented dev opt-out and sets it false. + +So `GCP-0061` was pointing at a real gap in that path, justified by a +compensating control that had never been built. The module was fixed rather +than the finding re-suppressed: the block is now unconditional (an empty list +means "enabled, nothing allowed" instead of "not configured"), and a +`precondition` on the cluster rejects the dangerous pairing at plan time. + +`.security-baselines/trivy-config.trivyignore` is now empty. **Three +suppressions become zero**, and the 2027-01-01 review has nothing left to +re-justify. + ## Decision Replace tfsec with `trivy config` in the `Self-audit` job. diff --git a/templates/service/infra/terraform/gcp/compute.tf b/templates/service/infra/terraform/gcp/compute.tf index 644448c..0719816 100644 --- a/templates/service/infra/terraform/gcp/compute.tf +++ b/templates/service/infra/terraform/gcp/compute.tf @@ -35,15 +35,30 @@ resource "google_container_cluster" "gke" { master_ipv4_cidr_block = "172.16.0.0/28" } - dynamic "master_authorized_networks_config" { - for_each = length(var.master_authorized_networks) > 0 ? [1] : [] - content { - dynamic "cidr_blocks" { - for_each = var.master_authorized_networks - content { - cidr_block = cidr_blocks.value.cidr_block - display_name = cidr_blocks.value.display_name - } + # Always emitted, never conditional. + # + # This used to be a `dynamic` block gated on the list being non-empty, so + # with the default empty list the cluster got NO authorized-networks + # configuration at all — and GKE's default is then "no restriction". That + # is harmless while `enable_private_endpoint = true` (the default), and it + # is a publicly reachable control plane the moment an adopter takes the + # documented dev opt-out and sets it false without also supplying a list. + # + # Emitting the block unconditionally inverts the failure mode: an empty + # list now means "authorized networks enabled, no external CIDR allowed" — + # the restrictive reading rather than the permissive one. The precondition + # below makes the dangerous combination impossible outright. + # + # It also makes the configuration statically visible. GCP-0061 (and tfsec's + # google-gke-enable-master-networks before it) fired here because no static + # analyser evaluates `dynamic` blocks. The finding was suppressed for two + # releases on the grounds that the HCL was correct anyway; it was not. + master_authorized_networks_config { + dynamic "cidr_blocks" { + for_each = var.master_authorized_networks + content { + cidr_block = cidr_blocks.value.cidr_block + display_name = cidr_blocks.value.display_name } } } @@ -56,6 +71,27 @@ resource "google_container_cluster" "gke" { release_channel { channel = "REGULAR" } + + lifecycle { + # The invariant the old suppression's justification claimed was "enforced + # by the variable validation rule in variables.tf". No such rule existed. + # + # A public control plane is acceptable only when it is constrained. This + # is a cross-variable condition, so it cannot live in a `validation` + # block on either variable — those could not reference other variables + # until Terraform 1.9, and this module targets >= 1.7. A resource + # precondition can, and it fails at plan time rather than at apply. + precondition { + condition = var.enable_private_endpoint || length(var.master_authorized_networks) > 0 + error_message = <<-EOT + enable_private_endpoint = false exposes the GKE control plane on a + public endpoint, so master_authorized_networks must list the CIDRs + allowed to reach it. Either keep the private endpoint (the default), + or supply master_authorized_networks. Leaving both unset would put an + unrestricted public control plane in front of the cluster. + EOT + } + } } # ============================================================================ diff --git a/templates/service/infra/terraform/gcp/variables.tf b/templates/service/infra/terraform/gcp/variables.tf index 93bf189..49575ac 100644 --- a/templates/service/infra/terraform/gcp/variables.tf +++ b/templates/service/infra/terraform/gcp/variables.tf @@ -149,8 +149,17 @@ variable "node_oauth_scopes" { variable "master_authorized_networks" { description = <<-EOT - CIDR blocks allowed to reach the GKE control plane. Empty list = no - public access (only relevant when enable_private_endpoint=false). + CIDR blocks allowed to reach the GKE control plane. + + The authorized-networks block is always emitted, so an empty list means + "enabled, no external CIDR allowed" — the restrictive reading. It used to + mean the block was omitted entirely, which GKE reads as no restriction at + all. + + REQUIRED when enable_private_endpoint = false: a public control plane has + to be constrained. That pairing is enforced by a precondition on + google_container_cluster.gke and fails at plan time. + Format: list of objects with cidr_block + display_name. EOT type = list(object({