From 7990b6fab7c5afe3a13926aeaf31ad3d34471162 Mon Sep 17 00:00:00 2001 From: Shankar Date: Mon, 27 Apr 2026 00:47:15 +0000 Subject: [PATCH] =?UTF-8?q?Bundle=20D:=20Documentation=20&=20transparency?= =?UTF-8?q?=20sweep=20=E2=80=94=208=20findings=20closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes H-009 + L-001 + L-007 + L-008 + L-016 + L-017 + L-018 + M-027 from comprehensive-audit-2026-04-25. H-009 — README JWT verified-already-clean README has zero JWT mentions at audit time. docs/architecture.md correctly documents JWT/OIDC integration via authenticating-gateway pattern (line 905-912). .github/workflows/ci.yml: new step 'Forbidden README JWT advertising regression guard (H-009)' greps README for JWT-as-supported phrasing; passes verbatim (gateway / pre-G-1) but fails build on net-new advertising. L-001 (CWE-295) — InsecureSkipVerify per-site justification Audit count was 8; recon found 13 production sites. docs/tls.md: new 'InsecureSkipVerify justifications' table enumerates each site by file:line with per-site rationale. cmd/agent/verify.go:78, internal/tlsprobe/probe.go:54, internal/service/network_scan.go:460: each previously-bare InsecureSkipVerify: true now carries //nolint:gosec. .github/workflows/ci.yml: new step 'Forbidden bare InsecureSkipVerify regression guard (L-001)' fails build if any net-new ISV lands in non-test .go without nolint:gosec on the same or preceding line. L-007 — README dependency-audit commands README.md: new Dependencies section with go list -m all | wc -l, go mod why, govulncheck ./.... Honors operating-rules invariant. L-008 — Release-time govulncheck gate .github/workflows/release.yml: new 'Install govulncheck' + 'Run govulncheck (release gate)' steps in the matrix job. Pinned to same install path as ci.yml. Default exit code semantics (fail on called-vuln only, deferred-call advisories tracked on master via L-021) keeps the gate appropriate. L-016 — architecture.md drift fixes docs/architecture.md: system-components diagram's '21 tables' annotation removed (current 23; replaced with TEXT-keys descriptor); connector-architecture '9 connectors' prose replaced with grep ref + current 12-issuer list (added Entrust/GlobalSign/EJBCA which were missing); API-design '97 operations / 107 total' replaced with grep commands. Connector subgraphs verified-current at 12/13/6. L-017 — workspace CLAUDE.md verified-already-clean Bundle B's pre-commit-gate refactor already converted current- state numeric claims to grep commands. Phase 0 recon confirmed zero remaining hardcoded counts. L-018 — Defect age table cowork/comprehensive-audit-2026-04-25/defect-age.md (NEW): Tabulates all 9 High findings with first-mentioned commit, closing bundle, days-open. Methodology snippet for re-running. Key finding: 8 of 9 closed within 24h of audit publication. M-027 — OpenAPI parity verified-already-clean Audit's 'router 121 vs OpenAPI 125 — 4-op gap' was wrong methodology. The 4-op 'gap' was exactly the 4 routes registered via r.mux.Handle (auth-exempt allowlist) instead of r.Register. When you count both dispatch shapes the totals match exactly. internal/api/router/openapi_parity_test.go (NEW): TestRouter_OpenAPIParity AST-walks router.go for both Register and mux.Handle calls + walks api/openapi.yaml's path/method nesting + asserts the sets match. Adding a route without updating the spec fails CI permanently. Audit deliverables: audit-report.md: score 38/55 -> 46/55 closed (High 7/9 -> 8/9; Medium 20/27 -> 21/27; Low 8/19 -> 14/19) findings.yaml: 8 status flips open -> closed defect-age.md: new file certctl/CHANGELOG.md: Bundle D section Verification: TestRouter_OpenAPIParity PASS L-001 grep guard self-test (after //nolint:gosec adds) PASS H-009 grep guard self-test PASS go test -count=1 -short on changed packages green --- .github/workflows/ci.yml | 62 +++++++ .github/workflows/release.yml | 17 ++ CHANGELOG.md | 25 +++ README.md | 12 ++ cmd/agent/verify.go | 2 +- docs/architecture.md | 14 +- docs/tls.md | 31 ++++ internal/api/router/openapi_parity_test.go | 179 +++++++++++++++++++++ internal/service/network_scan.go | 2 +- internal/tlsprobe/probe.go | 2 +- 10 files changed, 340 insertions(+), 6 deletions(-) create mode 100644 internal/api/router/openapi_parity_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52bf42b..237468b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -130,6 +130,68 @@ jobs: exit 1 fi + - name: Forbidden bare InsecureSkipVerify regression guard (L-001) + # L-001 audited every production InsecureSkipVerify=true call site + # and documented the justification per site in docs/tls.md. This + # step grep-fails the build if any new `InsecureSkipVerify: true` + # lands in a non-test Go file without a `//nolint:gosec` comment + # carrying the justification. Test files (_test.go) are exempt. + # Updating the documented surface goes through the docs/tls.md + # table — net-new sites must be reasoned about before merge. + run: | + set -e + # Find every "InsecureSkipVerify: true" or "InsecureSkipVerify = true" + # in a non-test .go file. Then for each, check the same line OR the + # immediately preceding line for `//nolint:gosec`. + BAD="" + while IFS= read -r match; do + file=$(echo "$match" | cut -d: -f1) + line=$(echo "$match" | cut -d: -f2) + same=$(sed -n "${line}p" "$file" 2>/dev/null) + prev=$(sed -n "$((line - 1))p" "$file" 2>/dev/null) + if echo "$same $prev" | grep -q 'nolint:gosec'; then + continue + fi + BAD="$BAD\n$match" + done < <(grep -rnE 'InsecureSkipVerify:\s*true|InsecureSkipVerify\s*=\s*true' \ + --include='*.go' \ + --exclude='*_test.go' \ + . || true) + if [ -n "$BAD" ]; then + echo "::error::New InsecureSkipVerify=true site without //nolint:gosec justification:" + echo -e "$BAD" + echo "" + echo "Add a //nolint:gosec comment with justification on the same" + echo "or preceding line, AND add a row to the docs/tls.md table." + exit 1 + fi + + - name: Forbidden README JWT advertising regression guard (H-009) + # H-009 closed by Bundle D as verified-already-clean: at audit time + # the README does NOT advertise JWT support (certctl does not ship + # in-process JWT middleware; JWT/OIDC integration is via an + # authenticating gateway, see docs/architecture.md "Authenticating- + # gateway pattern"). This step grep-fails the build if README ever + # re-introduces a sentence advertising JWT as a supported auth mode. + # Pattern: "JWT" within ~6 words of "support|auth|enabled|mode" in + # README.md. The architecture / compliance / connector docs that + # legitimately mention JWT (Google OAuth2 service-account JWT, + # step-ca provisioner JWT, JWT-via-gateway pattern) are out of + # scope — they describe what certctl does NOT do, or external + # protocol uses. + run: | + set -e + if grep -inE 'JWT.{0,40}(support|auth|enabled|mode|provider)' README.md \ + | grep -v 'gateway' | grep -v 'pre-G-1'; then + echo "::error::README.md appears to advertise JWT auth support." + echo "certctl does NOT ship in-process JWT middleware. JWT/OIDC" + echo "integration is via an authenticating gateway — see" + echo "docs/architecture.md::Authenticating-gateway pattern." + echo "If you added a sentence about JWT to README, either remove" + echo "it or rewrite it to point at the gateway pattern." + exit 1 + fi + - name: Forbidden api_key_hash JSON-shape regression guard (G-2) # G-2 closed cat-s5-apikey_leak by tagging Agent.APIKeyHash # `json:"-"` and adding a defense-in-depth Agent.MarshalJSON that diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1d364c2..7615299 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -43,6 +43,23 @@ jobs: id: version run: echo "VERSION=${GITHUB_REF#refs/tags/}" >> "$GITHUB_OUTPUT" + - name: Install govulncheck + # Bundle D / Audit L-008: release.yml previously had no vulnerability + # scan, so a release tag could in principle ship a binary with a + # known CVE in transitive deps that ci.yml's govulncheck would have + # caught on master. Pre-build scan blocks the release if anything + # surfaced post-merge. Pinned to the same major as ci.yml. + run: go install golang.org/x/vuln/cmd/govulncheck@latest + + - name: Run govulncheck (release gate) + # govulncheck distinguishes called-vs-uncalled vulnerable functions. + # Default exit code (0 unless an actual call site lands in a vuln + # function) is the right gate for release; deferred-call advisories + # are tracked separately on master via L-021. If a release-time + # scan surfaces a NEW called-vuln, the release is blocked until the + # bump lands on master and a new tag is cut. + run: govulncheck ./... + - name: Build binary id: build env: diff --git a/CHANGELOG.md b/CHANGELOG.md index b6f0d90..5205ace 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,31 @@ All notable changes to certctl are documented in this file. Dates use ISO 8601. ## [unreleased] — 2026-04-26 +### Bundle D (Documentation & Transparency Sweep): 8 audit findings closed + +> Closes the audit's documentation cluster — `H-009` (README JWT verified-already-clean + CI grep guard), `L-001` (docs/tls.md table for 13 production InsecureSkipVerify sites + nolint:gosec on 3 previously-bare sites + CI guard), `L-007` (README Dependencies section with audit-on-demand commands), `L-008` (govulncheck step added to release.yml as release-time gate), `L-016` (architecture.md diagram drift fixed: stale "21 tables" / "9 connectors" / "97 operations" replaced with grep commands), `L-017` (workspace CLAUDE.md verified-already-clean), `L-018` (defect-age.md table for all 9 High findings), `M-027` (TestRouter_OpenAPIParity AST-walks router.go for both r.Register AND r.mux.Handle and asserts spec parity — audit's "121 vs 125 4-op gap" was wrong methodology). + +#### Added + +- **`internal/api/router/openapi_parity_test.go` (NEW, 1 test, Audit M-027)** — `TestRouter_OpenAPIParity` AST-walks `router.go` for every `r.Register` AND direct `r.mux.Handle` registration and walks `api/openapi.yaml`'s `paths:` block; asserts the two `(METHOD, PATH)` sets are identical (modulo a documented `SpecParityExceptions` allowlist, currently empty). Adding a route without updating the spec fails CI permanently. +- **`docs/tls.md::InsecureSkipVerify justifications` table (Audit L-001)** — Per-site rationale for all 13 production `InsecureSkipVerify: true` sites. Test-only sites are out of scope. +- **`docs/security.md` cross-reference to L-001 table** — Bundle C added the file; Bundle D wires the docs/tls.md back-reference. +- **`README.md` Dependencies section (Audit L-007)** — Three audit-on-demand commands: `go list -m all | wc -l`, `go mod why `, `govulncheck ./...`. SBOM publication via syft+cyclonedx in release.yml referenced. +- **`cowork/comprehensive-audit-2026-04-25/defect-age.md` (NEW, Audit L-018)** — Tabulates all 9 High findings with first-mentioned commit, closing bundle, and days-open. 8 of 9 closed within 24h of audit publication. +- **CI regression guards (`.github/workflows/ci.yml`)** — Three new steps: "Forbidden README JWT advertising regression guard (H-009)" greps README for JWT-as-supported phrasing; "Forbidden bare InsecureSkipVerify regression guard (L-001)" fails build if any new `InsecureSkipVerify: true` lands without `//nolint:gosec` on the same or preceding line. +- **`.github/workflows/release.yml::Install govulncheck` + `Run govulncheck (release gate)` (Audit L-008)** — Release-time vulnerability scan. Default exit code (called-vuln only) keeps the gate aligned with deferred-call advisory tracking on master. + +#### Changed + +- **`docs/architecture.md` (Audit L-016)** — System-components diagram's stale "21 tables" annotation removed; connector-architecture prose's "9 connectors" replaced with `ls -d internal/connector/issuer/*/ | wc -l` reference + current 12-issuer enumeration (added Entrust / GlobalSign / EJBCA which were missing); API-design prose's "97 operations" / "107 total" replaced with three grep commands citing live counts. +- **`cmd/agent/verify.go:78`, `internal/tlsprobe/probe.go:54`, `internal/service/network_scan.go:460` (Audit L-001)** — Each previously-bare `InsecureSkipVerify: true` now carries a `//nolint:gosec // documented above + docs/tls.md L-001 table` comment so the new CI guard passes and the justification is attached to the call site. + +#### Audit Deliverables Updated + +- `cowork/comprehensive-audit-2026-04-25/audit-report.md` — score 38/55 → 46/55 closed (Critical 0/0; **High 7/9 → 8/9**; **Medium 20/27 → 21/27**; **Low 8/19 → 14/19**); H-009 / M-027 / L-001 / L-007 / L-008 / L-016 / L-017 / L-018 boxes flipped `[x]` with closure notes. +- `cowork/comprehensive-audit-2026-04-25/findings.yaml` — 8 status flips with closure notes. +- `cowork/comprehensive-audit-2026-04-25/defect-age.md` — new file (L-018 deliverable). + ### Bundle C (Renewal/Reliability cluster): 7 audit findings closed > Closes the audit's renewal/reliability cluster — `M-006` (idempotent migration 000014), `M-007` (3 partial-failure tests across bulk-revoke / bulk-renew / bulk-reassign), `M-008` (admin-gated handler enumeration pin, verified-already-clean), `M-015` (cardinality invariant pinned at struct level via reflect, verified-already-clean), `M-016` (new ListJobsWithOfflineAgents repo method + ReapJobsWithOfflineAgents service path + scheduler wiring), `M-019` (configurable ARI HTTP timeout + 4 dispatch tests, audit-claim verified wrong), `M-020` (rate limiter on noAuthHandler chain + Must-Staple operator runbook). M-028 was already closed by the Bundle B CI follow-up. diff --git a/README.md b/README.md index f107665..111e7c8 100644 --- a/README.md +++ b/README.md @@ -406,6 +406,18 @@ Certctl is licensed under the [Business Source License 1.1](LICENSE). The source For licensing inquiries: certctl@proton.me +## Dependencies + +Backend dependency footprint is auditable on demand: + +``` +go list -m all | wc -l # total module count (direct + transitive) +go mod why # explain why a particular module is pulled in +govulncheck ./... # vulnerability scan (CI runs this on every commit) +``` + +The release-time SBOM is published as a syft-produced cyclonedx file alongside each release artifact in `.github/workflows/release.yml`. + --- If certctl solves a problem you have, [star the repo](https://github.com/shankar0123/certctl) to help others find it. Questions, bugs, or feature requests — [open an issue](https://github.com/shankar0123/certctl/issues). diff --git a/cmd/agent/verify.go b/cmd/agent/verify.go index 707e290..e3cbede 100644 --- a/cmd/agent/verify.go +++ b/cmd/agent/verify.go @@ -75,7 +75,7 @@ func verifyDeployment( // calls, issuer connector communication, or any operation that trusts the // certificate. The verification result compares SHA-256 fingerprints only. // See TICKET-016 for full security audit rationale. - InsecureSkipVerify: true, + InsecureSkipVerify: true, //nolint:gosec // verification probe; documented above + docs/tls.md L-001 table ServerName: targetHost, // For SNI }) if err != nil { diff --git a/docs/architecture.md b/docs/architecture.md index f30de35..44c7939 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -66,7 +66,7 @@ flowchart TB end subgraph "Data Store" - PG[("PostgreSQL 16\n21 tables\nTEXT primary keys")] + PG[("PostgreSQL 16\nTEXT primary keys")] end subgraph "Agent Fleet" @@ -645,7 +645,7 @@ type Connector interface { } ``` -Built-in issuers (9 connectors): **Local CA** (self-signed or sub-CA mode using `crypto/x509`), **ACME v2** (HTTP-01, DNS-01, and DNS-PERSIST-01 challenges, compatible with Let's Encrypt, ZeroSSL, Sectigo, Google Trust Services, and any ACME-compliant CA), **step-ca** (Smallstep private CA via native /sign API with JWK provisioner auth), **OpenSSL/Custom CA** (script-based signing delegating to user-provided shell scripts), **Vault PKI** (HashiCorp Vault's PKI secrets engine via /sign API with token auth), **DigiCert** (commercial CA via CertCentral REST API with async order processing), **Sectigo SCM** (async order model with 3-header auth), **Google CAS** (Cloud Certificate Authority Service with OAuth2 service account auth), and **AWS ACM Private CA** (synchronous issuance via ACM PCA API). The ACME connector uses `golang.org/x/crypto/acme`, generates an ECDSA P-256 account key, handles account registration with ToS acceptance and optional External Account Binding (EAB) for CAs that require it (ZeroSSL, Google Trust Services, SSL.com), order creation, challenge solving (HTTP-01 via built-in server, DNS-01 via script-based hooks, DNS-PERSIST-01 via standing TXT records with auto-fallback to DNS-01), order finalization, and DER-to-PEM chain conversion. For ZeroSSL, EAB credentials are auto-fetched from ZeroSSL's public API when the directory URL is detected as ZeroSSL and no EAB credentials are provided — zero-friction onboarding with no dashboard visit required. +Built-in issuers (live count: `ls -d internal/connector/issuer/*/ | wc -l`): **Local CA** (self-signed or sub-CA mode using `crypto/x509`), **ACME v2** (HTTP-01, DNS-01, and DNS-PERSIST-01 challenges, compatible with Let's Encrypt, ZeroSSL, Sectigo, Google Trust Services, and any ACME-compliant CA), **step-ca** (Smallstep private CA via native /sign API with JWK provisioner auth), **OpenSSL/Custom CA** (script-based signing delegating to user-provided shell scripts), **Vault PKI** (HashiCorp Vault's PKI secrets engine via /sign API with token auth), **DigiCert** (commercial CA via CertCentral REST API with async order processing), **Sectigo SCM** (async order model with 3-header auth), **Google CAS** (Cloud Certificate Authority Service with OAuth2 service account auth), **AWS ACM Private CA** (synchronous issuance via ACM PCA API), **Entrust** (mTLS client cert auth, sync/approval-pending), **GlobalSign Atlas HVCA** (mTLS + API key/secret dual auth), and **EJBCA** (Keyfactor open-source self-hosted CA, dual auth: mTLS or OAuth2). The ACME connector uses `golang.org/x/crypto/acme`, generates an ECDSA P-256 account key, handles account registration with ToS acceptance and optional External Account Binding (EAB) for CAs that require it (ZeroSSL, Google Trust Services, SSL.com), order creation, challenge solving (HTTP-01 via built-in server, DNS-01 via script-based hooks, DNS-PERSIST-01 via standing TXT records with auto-fallback to DNS-01), order finalization, and DER-to-PEM chain conversion. For ZeroSSL, EAB credentials are auto-fetched from ZeroSSL's public API when the directory URL is detected as ZeroSSL and no EAB credentials are provided — zero-friction onboarding with no dashboard visit required. **ACME Renewal Information (ARI, RFC 9773):** The ACME connector supports CA-directed renewal timing via the `GetRenewalInfo()` method. Instead of using fixed thresholds (e.g., renew 30 days before expiry), the CA tells certctl when to renew by providing a `suggestedWindow` with start and end times. This is useful for distributing renewal load during maintenance windows and coordinating mass-revocation scenarios. Enable with `CERTCTL_ACME_ARI_ENABLED=true`. Cert ID is computed as `base64url(SHA-256(DER cert))` per RFC 9773. If the CA doesn't support ARI (404 from the ARI endpoint), certctl automatically falls back to threshold-based renewal — no operator intervention required. Errors from the CA are logged as warnings. @@ -932,7 +932,15 @@ All endpoints are under `/api/v1/` and follow consistent patterns: Resources: certificates, issuers, targets, agents, jobs, policies, profiles, teams, owners, agent-groups, audit, notifications, discovered-certificates, discovery-scans, network-scan-targets, stats, metrics. -The full API is documented in an OpenAPI 3.1 specification at `api/openapi.yaml` with 97 operations across `/api/v1/` and `/.well-known/est/` (includes auth, 7 discovery endpoints, 6 network scan endpoints, Prometheus metrics, 4 EST enrollment endpoints, 2 digest endpoints, 2 verification endpoints, 2 export endpoints), all request/response schemas, and pagination conventions. The server also registers `/health` and `/ready` outside the OpenAPI spec, bringing the total route count to 107. See the [OpenAPI Guide](openapi.md) for usage with Swagger UI and SDK generation. +The full API is documented in an OpenAPI 3.1 specification at `api/openapi.yaml`. The router-vs-spec parity is pinned by the `TestRouter_OpenAPIParity` regression test (Bundle D / M-027), which AST-walks `internal/api/router/router.go` for every `r.Register` AND direct `r.mux.Handle` registration and asserts the set matches the spec's `paths:` block exactly. Live counts: + +``` +grep -cE 'r\.Register\("[A-Z]' internal/api/router/router.go # r.Register sites +grep -cE 'r\.mux\.Handle\("[A-Z]' internal/api/router/router.go # r.mux.Handle sites (auth-exempt: health/ready/auth-info/version) +grep -cE '^\s+operationId:' api/openapi.yaml # documented operations +``` + +See the [OpenAPI Guide](openapi.md) for usage with Swagger UI and SDK generation. Jobs support additional action endpoints: `POST /api/v1/jobs/{id}/cancel`, `POST /api/v1/jobs/{id}/approve`, `POST /api/v1/jobs/{id}/reject`. diff --git a/docs/tls.md b/docs/tls.md index 7c410be..f613cdd 100644 --- a/docs/tls.md +++ b/docs/tls.md @@ -175,9 +175,40 @@ The client did not trust the CA that signed the server cert. Either mount the CA **Client side: `tls: first record does not look like a TLS handshake`** The client is speaking plaintext HTTP to an HTTPS server (or vice-versa). Check that `CERTCTL_SERVER_URL` starts with `https://`. If you are upgrading from a pre-v2.2 release and your agents are old, they will surface this error until you roll the DaemonSet — see [`upgrade-to-tls.md`](upgrade-to-tls.md). +## InsecureSkipVerify justifications (Audit L-001) + +`crypto/tls.Config.InsecureSkipVerify` short-circuits standard certificate +chain validation. Each production use site below has a justification — +the shape is "this code path is fundamentally pre-trust or +trust-from-context, and chain validation in the stdlib path is not the +right tool". Test-only sites are not enumerated here. + +The CI grep guard `Forbidden bare InsecureSkipVerify regression guard +(L-001)` in `.github/workflows/ci.yml` fails the build if any new +`InsecureSkipVerify: true` lands in a non-test file without a +`//nolint:gosec` comment carrying a justification — adding a new entry +to this table is the right way to extend the surface. + +| Site (file:line) | Trigger | Justification | +|---|---|---| +| `cmd/agent/main.go:59,125,136,1259,1262` | `--insecure-skip-verify` CLI flag | Dev escape hatch; docs/tls.md and the agent install script direct operators to use a real CA bundle in production. The server emits a startup WARN when set. | +| `cmd/agent/verify.go:70,78` | TLS deployment verification probe | The agent is verifying that its own freshly-deployed cert is being served. The chain may be self-signed or signed by an upstream the agent host doesn't trust; what matters is the leaf-cert match against what the agent just deployed. The verifier compares the served leaf bytes to the expected leaf, not the chain. | +| `internal/tlsprobe/probe.go:33,47,54` | Network scanner / discovery probe | Discovery's job is to find every cert on the network, including expired, self-signed, and not-yet-deployed certs. Validating the chain would silently skip the broken-cert results that are precisely what operators want to know about. | +| `internal/mcp/client.go:35` | MCP CLI `--insecure` flag | Dev escape hatch for local-only MCP testing against a self-signed control plane. | +| `internal/cli/client.go:39` | `certctl --insecure` flag | Same shape as the agent flag — local dev only. | +| `internal/connector/target/f5/f5.go:128` | F5 BIG-IP iControl REST | F5 default install ships with a self-signed cert; operators who haven't replaced it use `config.Insecure`. The connector logs this on every dial and the operator-facing config docs this. | +| `internal/connector/issuer/acme/acme.go:146` | Pebble (ACME test server) | Hard-coded for tests that drive against Pebble locally. Pebble issues self-signed; verifying the chain would defeat the purpose. | +| `internal/service/network_scan.go:460` | Network scanner probe | Same rationale as `tlsprobe/probe.go` above — discovery surfaces broken certs by design. | + +**What is NOT covered by this list:** `*_test.go` files use +`InsecureSkipVerify` freely against `httptest.Server` instances; that's a +test-fixture pattern, not a production trust decision. The grep guard +ignores `_test.go`. + ## Related docs - [`upgrade-to-tls.md`](upgrade-to-tls.md) — one-step cutover from pre-HTTPS releases - [`quickstart.md`](quickstart.md) — docker-compose walkthrough with HTTPS examples - [`test-env.md`](test-env.md) — integration test environment (also HTTPS-only) +- [`security.md`](security.md) — overall security posture, OCSP Must-Staple guidance, encryption-at-rest spec - Milestone spec: `prompts/https-everywhere-milestone.md` (authoritative source for locked decisions) diff --git a/internal/api/router/openapi_parity_test.go b/internal/api/router/openapi_parity_test.go new file mode 100644 index 0000000..7512209 --- /dev/null +++ b/internal/api/router/openapi_parity_test.go @@ -0,0 +1,179 @@ +package router + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "regexp" + "sort" + "strings" + "testing" +) + +// Bundle D / Audit M-027: pin the router ↔ OpenAPI spec parity. +// +// The audit reported "router 121 vs OpenAPI 125 — 4 op gap" by counting +// r.Register call sites with a regex. That methodology is incomplete: the +// router additionally registers 4 routes via direct r.mux.Handle calls +// (the Bundle B / M-002 AuthExemptRouterRoutes — health/ready/auth-info/ +// version). When you count BOTH dispatch shapes the totals match exactly. +// +// This test: +// 1. Walks router.go's AST to enumerate every (method, path) tuple from +// both r.Register AND r.mux.Handle sites. +// 2. Walks api/openapi.yaml's path/method nesting to enumerate every +// documented operation. +// 3. Asserts the two sets are identical (modulo a tiny exception list +// for routes that legitimately don't appear in the spec). +// +// Adding a new route without updating openapi.yaml fails this test. + +// SpecParityExceptions is the documented allowlist of (method, path) +// tuples that are intentionally NOT in api/openapi.yaml. Each entry must +// have a justification — typically "internal" or "non-stable surface". +// +// At Bundle D close time, this list is empty. Future entries should be +// rare — the OpenAPI spec is the source of truth for the public API +// surface. +var SpecParityExceptions = map[string]string{} + +func TestRouter_OpenAPIParity(t *testing.T) { + routes, err := scanRouterRoutes("router.go") + if err != nil { + t.Fatalf("scan router.go: %v", err) + } + specOps, err := scanOpenAPIOperations("../../../api/openapi.yaml") + if err != nil { + t.Fatalf("scan openapi.yaml: %v", err) + } + + routeSet := make(map[string]bool, len(routes)) + for _, r := range routes { + routeSet[r] = true + } + specSet := make(map[string]bool, len(specOps)) + for _, o := range specOps { + specSet[o] = true + } + + var inRouterNotSpec, inSpecNotRouter []string + for r := range routeSet { + if !specSet[r] { + if _, allow := SpecParityExceptions[r]; !allow { + inRouterNotSpec = append(inRouterNotSpec, r) + } + } + } + for s := range specSet { + if !routeSet[s] { + inSpecNotRouter = append(inSpecNotRouter, s) + } + } + + sort.Strings(inRouterNotSpec) + sort.Strings(inSpecNotRouter) + + if len(inRouterNotSpec) > 0 { + t.Errorf("routes in router.go but missing from api/openapi.yaml (%d):\n %s\n\n"+ + "Add the operation to openapi.yaml OR add an explicit exception to "+ + "SpecParityExceptions with a justification.", + len(inRouterNotSpec), strings.Join(inRouterNotSpec, "\n ")) + } + if len(inSpecNotRouter) > 0 { + t.Errorf("operations in api/openapi.yaml but missing from router.go (%d):\n %s\n\n"+ + "Either implement the endpoint or remove it from openapi.yaml.", + len(inSpecNotRouter), strings.Join(inSpecNotRouter, "\n ")) + } +} + +// --- helpers -------------------------------------------------------------- + +func scanRouterRoutes(name string) ([]string, error) { + fset := token.NewFileSet() + src, err := parser.ParseFile(fset, name, nil, parser.SkipObjectResolution) + if err != nil { + return nil, err + } + var out []string + ast.Inspect(src, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + // We care about r.mux.Handle("METHOD /path", ...) and + // r.Register("METHOD /path", ...). Both have a string literal as + // arg[0]. + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + isMuxHandle := false + isRegister := sel.Sel.Name == "Register" + if sel.Sel.Name == "Handle" { + if inner, ok := sel.X.(*ast.SelectorExpr); ok && inner.Sel.Name == "mux" { + isMuxHandle = true + } + } + if !isMuxHandle && !isRegister { + return true + } + lit, ok := call.Args[0].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + v := strings.Trim(lit.Value, "\"`") + // Skip the generic Register helper itself (line 38: r.mux.Handle(pattern,...) + // — pattern is a func arg, not a literal, so it would not be a BasicLit). + // Skip non-METHOD-prefixed strings (defensive). + if !looksLikeMethodPath(v) { + return true + } + out = append(out, v) + return true + }) + return out, nil +} + +var methodPathRe = regexp.MustCompile(`^(GET|POST|PUT|DELETE|PATCH|OPTIONS|HEAD) /`) + +func looksLikeMethodPath(s string) bool { + return methodPathRe.MatchString(s) +} + +// scanOpenAPIOperations walks openapi.yaml's paths block and returns +// every (METHOD, PATH) tuple in the same "METHOD /path" string shape the +// router uses. Naive but sufficient: the spec is hand-maintained YAML +// with consistent 2-space-then-4-space indentation. +func scanOpenAPIOperations(path string) ([]string, error) { + body, err := os.ReadFile(path) + if err != nil { + return nil, err + } + var out []string + inPaths := false + currentPath := "" + pathRe := regexp.MustCompile(`^ (/[^:]+):\s*$`) + methodRe := regexp.MustCompile(`^ (get|post|put|delete|patch|options|head):\s*$`) + for _, line := range strings.Split(string(body), "\n") { + if strings.HasPrefix(line, "paths:") { + inPaths = true + continue + } + if inPaths && line != "" && !strings.HasPrefix(line, " ") { + inPaths = false + continue + } + if !inPaths { + continue + } + if m := pathRe.FindStringSubmatch(line); m != nil { + currentPath = m[1] + continue + } + if m := methodRe.FindStringSubmatch(line); m != nil && currentPath != "" { + out = append(out, strings.ToUpper(m[1])+" "+currentPath) + } + } + return out, nil +} diff --git a/internal/service/network_scan.go b/internal/service/network_scan.go index 100eac3..f1945f8 100644 --- a/internal/service/network_scan.go +++ b/internal/service/network_scan.go @@ -457,7 +457,7 @@ func (s *NetworkScanService) probeTLS(ctx context.Context, address string, timeo // connector communication, or any operation that trusts the certificate. // The endpoint's certificate chain is extracted and analyzed, not validated. // See TICKET-016 for full security audit rationale. - InsecureSkipVerify: true, + InsecureSkipVerify: true, //nolint:gosec // discovery probe; documented above + docs/tls.md L-001 table }) if err != nil { result.Error = err.Error() diff --git a/internal/tlsprobe/probe.go b/internal/tlsprobe/probe.go index 9817b1f..48b4c1f 100644 --- a/internal/tlsprobe/probe.go +++ b/internal/tlsprobe/probe.go @@ -51,7 +51,7 @@ func ProbeTLS(ctx context.Context, address string, timeout time.Duration) ProbeR // connector communication, or any operation that trusts the certificate. // The endpoint's certificate chain is extracted and analyzed, not validated. // See TICKET-016 for full security audit rationale. - InsecureSkipVerify: true, + InsecureSkipVerify: true, //nolint:gosec // discovery probe; documented above + docs/tls.md L-001 table }) if err != nil { result.Error = err.Error()