security(alerts): preserve and surface an unusable webhook signing secret (SEC-WHSIGN-1)

[KidCarmi]

סיכום / Summary

Security regression review of the 2026-08-24 window (baseline daf66d9 — the tip of the 2026-08-23 review — through 9b1ba86; 27 commits / 59 files). One regression found and fixed.

It is not a bad line in the diff — it is a feature interaction. This window's nightly-QA backup fix added alert_webhooks.json to the archive (4117bca). Webhook HMAC signing secrets in that file are AES-GCM ciphertext (RISK-003) under a node-local key, <dataDir>/.alert_webhook_key, which is deliberately never archived — the same rule that excludes .kek files, since a key must not share a tarball with the material it unwraps. That exclusion is correct. What it collides with is the alert store's long-standing response to a secret it cannot unwrap:

1. Signing goes off, silently. deliverAttempt sets X-Culvert-Signature only when Secret != "". A receiver that verifies the HMAC then quietly stops accepting this node's threat_detected / policy_block / cert_expiry alerts — a monitoring blind spot where quiet looks exactly like healthy.
2. The UI cannot show it. List() redacts Secret (correctly), so "no signing secret was ever configured" and "the secret is unusable and deliveries are unsigned" serialised to byte-identical JSON. The only signal was one obs.Printf at boot — and the alerting plane cannot page about its own signing being broken.
3. The ciphertext is destroyed. save() rewrites the whole list, so the next add/edit/delete/toggle of any webhook re-encrypted the blanked cleartext and persisted "" over a blob that restoring the key file would have recovered. The code's own comment claimed "no data loss on a transient key-read failure"; that intent was not upheld.

Reachability is what changed: from "essentially never" to every rebuild-from-backup, a supported and documented operator workflow (an unreadable key file at boot — permissions, or the EMFILE window CHAOS-54 documents in this same window — gets there too). All three steps were reproduced against the pre-fix tree before the fix, and three of the new tests fail against it.

סוג שינוי / Change type:

• security — תיקון אבטחה / security fix

The fix

Security-first and minimal; no delivery behaviour widened, nothing made more permissive:

• The ciphertext survives. Webhook.sealedSecret (unexported) holds the value that failed to decrypt, and save() writes it back verbatim instead of re-encrypting the blanked cleartext. The in-memory Secret stays empty — we still never sign with a value that failed authentication — but the failure is now recoverable rather than destructive. Update carries it across a secret-less edit; supplying a new secret replaces it.
• The state is visible. List() reports a derived, read-only SigningDegraded (signing_degraded on the wire) — the one bit an operator needs, carrying no key material. It is derived, never accepted: Add, Update and the on-disk document cannot assert it, and it is never persisted.
• It reaches the operator. New alert_webhook_signing diagnostics row (OK/warn, counts only — no name, URL or ciphertext on that viewer-role surface) plus an amber "Unsigned — secret unusable" badge in Security → Alert Webhooks.
• SEC-BKP-2 (Low): packOne now refuses .alert_webhook_key alongside .kek, so a future dataDir walk or glob can never ship the key with the material it unwraps. The backup runbook's "what an unencrypted archive contains" list now names webhook endpoint URLs (bearer credentials for most receivers), and a new § 9.5 documents the non-portability and how to recover.

Deliberately not done: refusing delivery outright when the secret is unusable. That trades a visible authenticity failure for total alert silence, which is worse against the realistic scenario. Recorded as an owner decision in the review doc.

The rest of the window: reviewed, found safe

Full write-up in docs/engineering/security-reviews/2026-08-24-socks5-ipfilter-and-backup-surface-window.md. Highlights:

• IP-filter lock-free read view (PR perf(security): make the per-request IP-filter gate lock-free and flat in entry count #1207) — the first gate on every proxied request, so it was re-verified independently of its own suite: a throwaway differential harness re-implemented the pre-change algorithm verbatim and compared verdicts across hand-picked divergence shapes (::ffff:10.0.0.0/104, ::ffff:0.0.0.0/96, 4-in-6 probes, zoned addresses, /0, /32, /128, malformed probes) plus 400 randomised taxonomies × 60 probes. No divergence. Fail-closed default arm intact; every mutator republishes; ClearAll ≡ the List+Remove loop it replaced.
• CHAOS-54 SOCKS5 (PR chaos(CHAOS-54): the SOCKS5 accept loop under listener faults #1208) — no per-connection control bypassed; a dead socket closes the port (connection-refused beats a bound black hole) and reports DOWN; unauthenticated /health + /readyz carry a fixed enum / fixed detail while the reason class and counts stay role-gated; bounded alert Detail; separate degraded/down latches.
• Pre-auth TLS-fallback redaction — server-side wall intact, and the shipped frontend/dist bundle was checked directly (the "Server detail:" rendering is gone); the decoder relaxation keeps the ui_tls_fallback flag required.
• Diagnostics login-state row, /healthz + /metrics SOCKS5 series, ADR-number comment corrections, the PAC label clarification, and four forward dependency/action bumps: all safe.

---

בדיקות שבוצעו / Testing Done

go vet ./...                                   # clean
go test -count=1 ./...                         # PASS (all packages)
go test -race -count=1 -timeout=30m ./internal/alerts/ .   # PASS (852s root, 2.1s alerts)
go run ./cmd/apibundle -spec api/openapi/openapi.yaml -check  # api artifacts up to date
go test -count=1 -run 'TestOpenAPI_Gate|TestAPIContract|TestSecrets|TestConfig' .   # PASS

golangci-lint could not run in this environment (the installed binary is built with Go 1.25; the module requires Go 1.26) — left to the Fast PR Gate.

New coverage, by kind:

Kind	Test
Regression (fails pre-fix)	TestSigningDegraded_UndecryptableCiphertextSurvivesUnrelatedSave, …_DeleteOfAnotherHookDoesNotDestroyTheCiphertext, …_UpdateWithoutSecretPreservesCiphertext
Positive	TestSigningDegraded_HealthyStoreIsNotDegraded, TestDiagnostics_AlertWebhookSigningRow_OKWhenHealthy
Negative / visibility	TestSigningDegraded_ListReportsItWithoutLeaking, TestDiagnostics_AlertWebhookSigningRow_WarnsWhenDegraded, TestAlertWebhookList_ExposesSigningDegradedWithoutSecrets
Recovery	TestSigningDegraded_ReEnteringTheSecretRecovers, TestSigningDegraded_RestoringTheKeyFileRecoversSigning
Boundary / malformed input	TestSigningDegraded_MalformedStoredSecret (non-base64, empty, truncated)
Authorization / trust boundary	TestSigningDegraded_StatusIsNeverAcceptedFromACaller (create, update, hand-edited store file)
Concurrency (-race)	TestSigningDegraded_ConcurrentReadersAndWriters
Secret containment	TestBackup_NeverPacksTheWebhookSigningKey, TestIsNodeLocalKeyArtifactPath
Contract	alert_webhook_signing pinned into diagnostics_test.go's required rows + TestDiagnostics_AlertWebhookSigningRow_InDefaultReport

---

רשימת בדיקות לפני Merge / Pre-Merge Checklist

קוד / Code Quality

• go vet ./... + go build ./... pass
• golangci-lint run — could not run locally (Go-version mismatch in this environment); deferred to the Fast PR Gate
• No dead code, stray TODOs, or debug log.Printf (the scratch harnesses used during the review were deleted)
• New fields and functions documented in comments
• go.mod unchanged

אבטחה / Security

• No secrets, passwords, tokens or keys in code or tests (test secrets are literals in temp dirs; the containment test asserts key material never reaches an archive)
• Input validated: signing_degraded is server-derived and explicitly ignored from callers and from the file
• No new file paths accepted from input; the backup packer's exclusion was widened, never narrowed
• No new InsecureSkipVerify
• No change to auth*.go, ca.go or proxy.go

אם נגעת ב-Policy Engine

• n/a — no policy-engine change.

אם נגעת ב-Frontend (index.html)

• The new badge interpolates only static strings — no server data reaches innerHTML unescaped
• No CSP change

אם הוספת/שינית endpoint של ה-API

• No new route; the change adds a response field to the existing GET /api/alerts/webhooks (AlertWebhookList items are additionalProperties: true, so the shape stays valid), documented in api/openapi/openapi.yaml
• make api-bundle run; the commit includes the regenerated openapi.json (and -check reports up to date)
• x-culvert-* metadata unchanged and still matches handler behaviour
• Backward compatible — the field is omitempty and read-only

תיעוד / Documentation

• docs/operator/docker-compose-backup-restore.md — § 4 contents list + new § 9.5
• CLAUDE.md — the invariant recorded beside the other alerts-plane rules
• docs/engineering/security-reviews/2026-08-24-… — the full review
• No CHANGELOG.md in this repo; config.example.yaml unaffected (no new config field — enablement is not operator-configurable)

---

סיכון ו-Rollback / Risk & Rollback

רמת סיכון / Risk level: 🟢 Low

The change is confined to internal/alerts persistence bookkeeping plus three read-only surfaces. On a healthy node — every webhook secret decrypting normally — sealedSecret is always empty, save() takes exactly the previous branch, and signing_degraded is omitted from the wire, so behaviour is byte-identical. The only behavioural delta is on a node that was already broken, where it now preserves rather than destroys.

Rollback: revert the commit. No migration, no on-disk format change (the preserved value is the same enc:v1: blob the previous code wrote), no config surface touched. A reverted node simply returns to blanking the ciphertext on the next save.

---

Residual risk (recorded, not fixed here)

1. A config import still discards the sealed ciphertext — it replaces the webhook list wholesale. Pre-existing, consistent with "secrets are not exported", now documented in § 9.5.
2. Unencrypted backups remain sensitive by construction (webhook URLs, ui_users hashes, TOTP secrets). Dev/lab only; the doc now enumerates the URLs.
3. Legacy cleartext webhook secrets (a store written before RISK-003 and never re-saved) are archived as cleartext until one mutation migrates them.
4. ipf is a plain package global reassigned by the DP snapshot apply while request goroutines read it — an unsynchronised pointer swap, pre-existing, same shape as the other snapshot-applied globals.
5. The degraded-webhook posture stays fail-open-but-loud rather than signed-or-nothing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FcJECueYSXcU6H3GXkrumw

---

Generated by Claude Code

[KidCarmi]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[aegisdiff]

❌ AegisDiff Security Triage — ERROR

Field	Value
Verdict	ERROR
Severity	✅ N/A
CWE	N/A
Confidence	0%
Analyzed by	unknown
Sanitizer Found	❌ None detected

All LLM providers exhausted. Last error: Client error '404 Not Found' for url 'h

All LLM providers exhausted. Last error: Client error '404 Not Found' for url 'https://api.groq.com/openai/v1/chat/completions'
For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/404

📦 Large PR Risk Triage Mode coverage

AegisDiff ran in Large PR Risk Triage Mode. This PR exceeded the full-scan budget, so AegisDiff prioritized security-relevant changed hunks instead of scanning every line.

• Files changed: 14
• Files analyzed: 11
• Files skipped: 3
• Skip reasons:
  • docs: 3
• LLM calls used: 12 / 40
• Chunks errored: 12 / 12 ⚠️
• Budget exhausted: no
• Large PR triggers: added_lines=1194 > 1000
• Inline comments posted: 0

_{Commit d176aec · PR #1222 · Powered by AegisDiff}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e28b449aa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".