Skip to content

feat(webhook): make the receiver correct, authenticated and reachable (Refs PMAT-201) - #205

Open
noahgift wants to merge 1 commit into
mainfrom
feat/webhook-receiver-pmat-201
Open

feat(webhook): make the receiver correct, authenticated and reachable (Refs PMAT-201)#205
noahgift wants to merge 1 commit into
mainfrom
feat/webhook-receiver-pmat-201

Conversation

@noahgift

Copy link
Copy Markdown
Contributor

Root cause: it was never reachable

run_webhook_server had zero non-test callers and no Commands variant dispatched it. The receiver could not be started, dogfooded, or reached by any sender — which is why ~35 passing tests coexisted with a receiver no real client could use. Every one of them called the functions directly.

Found by a 3-lens adversarial audit plus a 3-system design quorum (GitHub / Stripe / Kubernetes admission). Every claim below was measured.

The stop-the-line defect

compute_hmac_hex was named HMAC-SHA256 and its doc comment described the ipad/opad construction. It computed keyed BLAKE3:

old (keyed BLAKE3) 30f3b0f1f2b72e19eefe2a08fc3af2bc66c69f1f9689b585ded84cb33c2d8366
new (HMAC-SHA256)  5bdcc146bf60754e6a042426089575c75a003f089d2739839dec58b964ec3843
RFC 4231 TC2       5bdcc146bf60754e6a042426089575c75a003f089d2739839dec58b964ec3843

No sender using standard tooling could ever authenticate. The primitive was a sound MAC — the defect was truthfulness and interoperability.

Why it survived: every signature test derived its expected value by calling the function under test. f(x) == f(x) holds for any function. hmac_deterministic asserted only h1 == h2 and h1.len() == 64 — BLAKE3-256 hex is also 64 chars, so even the length couldn't discriminate.

The stated reason for hand-rolling ("avoid heavyweight crypto dependencies") didn't hold: sha2 was already a direct dep and hmac/digest/subtle were already in Cargo.lock. This is +1 pure-Rust crate, no new transitive deps.

The rest, all measured

Defect Measured behaviour
No HTTP framing One read(); Content-Length parsed and never read back. Body in a 2nd TCP segment → 400, event silently dropped. Its tests wrote 19 bytes in one write_all on loopback
MAC over lossy String body: String made byte-exact verification impossible by construction
Two fail-open defaults secret: None, and empty allowed_paths = allow-every-path (POST /anything-at-allValid)
Body-only MAC A digest for /hooks/deploy verified unchanged at /hooks/destroy, and replayed forever
Pre-auth DoS One idle socket delayed a legitimate signed delivery by 5383ms, upstream of any signature check
Inert size cap max_body_bytes > 64 KiB silently clamped; BodyTooLarge unreachable from the server
Invalid JSON responses {"status":"PathNotAllowed { path: "/evil" }"} under application/json, reflecting attacker input; all six outcomes collapsed to 403 while 401/405/413 were dead code
Lying acknowledgement sender.send errors discarded, 200 returned anyway
Two clocks A private now_iso8601 returned epoch seconds + Z, so 1785348550Z and 2026-07-29T…Z landed in one audit field

Dogfooded with an independent oracle

forjar rules serve, verified against the built binary with a signature generated by openssl — outside forjar entirely:

200 OK {"status":"accepted"}
event WebhookReceived at 2026-07-30T08:47:00Z path=/webhook → 1 rulebook(s) matched

Negative paths, live: tampered body → 401 signature_invalid; replay → 200 duplicate_ignored; 1h-old t401 signature_stale; unsigned → 401 signature_missing; GET → 405 + Allow: POST. Fail-closed startup refuses a missing secret and a non-loopback bind.

⚠️ Actions are deliberately NOT executed

rules serve reports matching actions and does not run them. rulebook_template::expand_action substitutes attacker-controlled payload keys into RulebookAction.script via String::replace with no shell quoting — wiring an executor to a network listener would turn an inbound request into command execution.

That injection is currently unreachable (expand_action has no callers; cli::trigger only prints action_type()) and must stay so until the quoting is fixed. Receiver auth/freshness/idempotency landing first is a hard sequencing gate, pinned as INV-ACTIONS-ARE-NOT-EXECUTED-HERE.

Verification

contracts/webhook-receiver-v1.yaml — 16 invariants, 13 falsification tests, 5 known gaps; pv validate clean. 12,622 lib tests (57 new), every integration suite green, clippy --all-targets --all-features -D warnings clean, fmt clean, cargo deny advisories ok.

One thing to review: the baseline bump

The pre-commit gate flagged a regression on tests/falsification_migrate_webhook.rs (99.98653 → 90.99304). I believe it's a pmat arithmetic inconsistency, not a code regression — every component is unchanged or better:

duplication  14.9865 -> 15.0923   (improved; 25.1% -> 24.5%)
all others   unchanged at maximum
sum          99.9865 -> 100.0923
total        99.9865 ->  90.9930   <-- no longer equals the sum

The old total equalled its component sum exactly; the new one is ~9.1 below it, while pmat simultaneously reports "Excellent code quality! No major issues". Updated via pmat tdg baseline update — the affordance the hook itself names — not --no-verify. Worth a look in case it's a real pmat bug.

🤖 Generated with Claude Code

… (Refs PMAT-201)

The webhook receiver had never been reachable. `run_webhook_server` had ZERO
non-test callers and no `Commands` variant dispatched it, so it could not be
started, dogfooded, or reached by any sender. That is the root cause of
everything below: ~35 tests passed while the receiver was unusable, because every
one of them called the functions directly.

Found by a 3-lens adversarial audit plus a 3-system design quorum (GitHub /
Stripe / Kubernetes admission). Every claim below was measured, not read.

## The stop-the-line defect, and why it survived

`compute_hmac_hex` was named HMAC-SHA256, and its doc comment described the
ipad/opad construction. It computed a keyed BLAKE3 hash. Against RFC 4231 TC2:

    old (keyed BLAKE3) 30f3b0f1f2b72e19eefe2a08fc3af2bc66c69f1f9689b585ded84cb33c2d8366
    new (HMAC-SHA256)  5bdcc146bf60754e6a042426089575c75a003f089d2739839dec58b964ec3843
    RFC 4231 TC2       5bdcc146bf60754e6a042426089575c75a003f089d2739839dec58b964ec3843

So no sender using standard tooling could ever authenticate. The primitive was a
sound MAC; the defect was truthfulness and interoperability.

It survived because EVERY signature test computed its expected value by calling
the function under test — `f(x) == f(x)`, which holds for any function and cannot
detect a substituted algorithm. `hmac_deterministic` asserted only `h1 == h2` and
`h1.len() == 64`, and BLAKE3-256 hex is also 64 chars, so even the length could
not discriminate. Every expected value now comes from outside this crate: RFC 4231
vectors and digests generated with `openssl dgst -sha256 -hmac`.

The stated justification for hand-rolling ("avoid heavyweight crypto
dependencies") did not hold: `sha2` was already a direct dependency and
hmac/digest/subtle were already in Cargo.lock, so this is +1 pure-Rust crate with
no new transitive deps.

## Everything else, measured

* NO HTTP FRAMING. One `read()`; `Content-Length` was parsed into the header map
  and never read back. A correctly-framed delivery whose body landed in a second
  TCP segment returned 400 and the event was silently dropped — with a secret set
  it degraded to SignatureInvalid, because the MAC covered the prefix. Its tests
  wrote a 19-byte body in one `write_all` on loopback, which always lands in one
  segment.
* The MAC covered a `from_utf8_lossy` String, so `body: String` made byte-exact
  verification impossible by construction, and `body.len()` measured the inflated
  replacement string.
* TWO FAIL-OPEN DEFAULTS: `secret` defaulted to None, and an EMPTY
  `allowed_paths` meant allow-EVERY-path (measured: `POST /anything-at-all` →
  Valid). The config an operator would write to lock the endpoint down was the
  least restrictive available.
* The MAC bound only the body, so a digest minted for /hooks/deploy verified
  unchanged at /hooks/destroy, and replayed forever.
* PRE-AUTH DoS: connections were handled inline on the accept loop; one idle
  socket delayed a legitimate signed delivery by 5383ms, upstream of any
  signature check.
* `max_body_bytes` above 64 KiB was silently inert (buffer clamped to
  `min(max, 65536)+4096`), making BodyTooLarge unreachable from the server.
* Rejections emitted `{"status":"PathNotAllowed { path: "/evil" }"}` — invalid
  JSON, under Content-Type: application/json, reflecting attacker input — and all
  six outcomes collapsed to 403 while the 401/405/413 arms were dead code.
* `sender.send` errors were discarded and 200 returned anyway, so a sender was
  told its delivery was accepted when nothing would process it.
* A private `now_iso8601` returned `format!("{}Z", secs)` — epoch seconds, not ISO
  8601 — putting `1785348550Z` and `2026-07-29T…Z` in the same audit-log field.

## What landed

New `webhook_sig` (HMAC-SHA256, constant-time `Mac::verify_slice`, `t=/v1=`
parsing, bidirectional freshness, bounded expiring replay guard) and
`webhook_http` (Content-Length framing, 413-before-buffering, serde_json
responses). `webhook_source` keeps types plus a fail-closed policy;
`webhook_server` is a bounded worker pool. All eight files stay under the
500-line cap.

Signed payload is `t=<unix>\n<METHOD>\n<path>\n<body>` — newline-separated rather
than Stripe's `t.payload`, because a path may contain `.` and must not be able to
shift the field boundary. GitHub's `X-Hub-Signature-256` is accepted over the
bare body for interop (weaker guarantees, recorded as a known gap).

Design decisions taken against the quorum: HMAC-SHA256 over renaming the BLAKE3
(interoperability); std over axum/hyper (~60 lines vs a permanent HTTP+TLS stack
in an IaC binary); a fixed thread pool over tokio; in-memory replay set over
rusqlite (the freshness window already bounds the exposure to ≤300s).

## Reachable, and dogfooded

`forjar rules serve` under the existing RulesCmd. Verified against the built
binary with a signature generated by OPENSSL — an oracle entirely outside forjar:

    200 OK {"status":"accepted"}
    event WebhookReceived at 2026-07-30T08:47:00Z path=/webhook → 1 rulebook(s) matched

and the negative paths: tampered body → 401 signature_invalid; replay → 200
duplicate_ignored; 1h-old t → 401 signature_stale; unsigned → 401
signature_missing; GET → 405 with Allow: POST. Fail-closed startup refuses a
missing secret and a non-loopback bind, with actionable messages.

## ⚠️ Actions are deliberately NOT executed

`rules serve` reports matching actions and does not run them.
`rulebook_template::expand_action` substitutes attacker-controlled payload keys
into `RulebookAction.script` via `String::replace` with no shell quoting, so
wiring an executor to a network listener would convert an inbound request into
command execution. The injection is currently unreachable (`expand_action` has no
callers; `cli::trigger` only prints `action_type()`) and must stay so until the
quoting is fixed. Receiver auth/freshness/idempotency landing first is a hard
sequencing gate, recorded as INV-ACTIONS-ARE-NOT-EXECUTED-HERE.

## Verification

contracts/webhook-receiver-v1.yaml — 16 invariants, 13 falsification tests, 5
known gaps; `pv validate` 0 errors 0 warnings. 12,622 lib tests pass (57 new
webhook tests), every integration suite green, clippy --all-targets
--all-features -D warnings clean, fmt clean, cargo deny advisories ok.

Refs PMAT-201
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

## .pmat/baseline.json — updated, with evidence

The pre-commit gate flagged a "quality regression" on
tests/falsification_migrate_webhook.rs (99.98653 -> 90.99304). It is a pmat
scoring inconsistency, not a code regression. Every component is unchanged or
BETTER:

    structural   25.0000 -> 25.0000
    semantic     20.0000 -> 20.0000
    duplication  14.9865 -> 15.0923   (improved; 25.1% -> 24.5%)
    coupling     15.0000 -> 15.0000
    doc          10.0000 -> 10.0000
    consistency  10.0000 -> 10.0000
    entropy       5.0000 ->  5.0000
    ------------------------------
    sum         99.9865 -> 100.0923
    total       99.9865 ->  90.9930   <-- no longer equals the sum

The OLD total equalled its component sum exactly; the new one is ~9.1 below it.
pmat also reports "Excellent code quality! No major issues" for the file at the
same time. Baseline updated via `pmat tdg baseline update` — the affordance the
hook itself names for an intentional change — rather than `--no-verify`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant