Security-review accuracy pass + decommission cron provisioner#42
Merged
Conversation
Prep for a real security review. Every claim verified against live GCP/1Password.
Docs (were describing earlier/abandoned designs):
- broker/README.md: rewrite to the real mTLS-forwarded-header auth model; it
documented a non-existent JWT/jti/CRL design (the live broker has none).
- README.md: fix the false "LB strips client headers" claim (the LB OVERWRITES
the injected X-Client-Cert-* names; internal-LB ingress is the load-bearing
protection, and the broker trusts Chain-Verified without re-verifying the
chain). Explain allUsers+ingress. Reconcile the SecureToken mint (runner
auto-mints over on-host ssh). Correct role-rollout status. Fix the 1Password
vault name/example. Caveat per-instance rate limiting.
- docs/{deep-dive,index}.html: same corrections; mark Linux "scaffolded, not
live" (secrets empty); note 24h cert lifetime is step-ca's default (unpinned);
flag the dead acme-no-sip provisioner + bootstrap-step-ca.sh IaC drift.
- docs/reprovision-architecture.html: drop the untrue "Security-reviewed" chip.
- SECURITY.md (new): reviewer companion - every claim -> file:line -> verify
command, residual risks, rollout status, terraform-apply-safety notes.
- Fix stale docstrings (broker config.py SPIFFE comment; orchestrator ssh.py).
Code / infra:
- broker/app/main.py: remove the unauthenticated /_debug/headers endpoint.
- Decommission the superseded, armed cron provisioner (was DRY_RUN=false,
enabled, firing every 5 min): remove terraform/provisioner.tf + provisioner/
app dir; retain the shared simplemdm-api-token secret (relocated to
terraform/simplemdm_secret.tf, same address). Live infra already torn down.
- terraform/step_ca.tf: PIN the Debian boot image (was family="debian-12",
which made every `terraform apply` want to replace the CA VM).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prep for the upcoming security review. @aerickson 's doc review surfaced discrepancies; this reconciles all the docs with the real code/infra — every claim was verified against live GCP and 1Password, not assumed.
Highlights
broker/README.mdwas describing an auth model that doesn't exist (Bearer JWT +jtireplay cache + CRL). The live broker is pure mTLS-forwarded-header auth. Fully rewritten to matchbroker/app/.README.mdclaimed the LB "strips client-supplied headers" — it doesn't. The LB overwrites the 9X-Client-Cert-*names it injects; the load-bearing protection is Cloud Run ingress (internal-and-cloud-load-balancing), since the broker trustsX-Client-Cert-Chain-Verifiedand never re-verifies the chain. Now stated precisely, incl. whyallUsersrun.invoker is safe./_debug/headersendpoint from the broker.acme-no-sipis dead config.SECURITY.md— reviewer's companion mapping every security claim → file/line → the exact command to verify it live, plus honest residual risks and terraform-apply-safety notes.Infra changes (already applied to live GCP)
relops-provisioner+relops-provisioner-tick) — it was armed (DRY_RUN=false, enabled, firing every 5 min) and is a needless second reprovision path. Removedterraform/provisioner.tf+ theprovisioner/app; retained the sharedsimplemdm-api-tokensecret (relocated toterraform/simplemdm_secret.tf, same resource address → no-op). Live service/scheduler/secrets/bucket already torn down via targeted apply + gcloud.terraform/step_ca.tf. It usedfamily = "debian-12", so everyterraform applywanted to replace the CA VM (Debian auto-bumps). Now pinned to the deployed version. Post-changeterraform planis clean except one benignvault_broker scaling{}in-place drift (deferred deliberately).Verification
terraform plan→0 add, 1 change (benign broker), 0 destroy. Broker tests unchanged and still cover the negative cases (401/403/429, URL-encoded CN). SeeSECURITY.mdfor per-claim verify commands.Left as follow-ups (deliberately not in this PR)
Reconcile the benign
vault_broker scaling{}drift; close the step-ca IaC drift (codify the 6 Linux SCEP provisioners, prune the deadacme-no-sip); populate/validate the Linux path if it's ever wanted.🤖 Generated with Claude Code