feat(signing): ship signing/signingtest subpackage + ObserveOnly mode - #487
Conversation
Every consumer writing a handler test that expects signed requests had to reverse-engineer the ~30-line keypair/JWK/StaticJWKSResolver/replay-store pattern in middleware_test.go. signingtest.NewTestAgent + SignAndSend collapse that into a two-line setup and a one-line send. Separately, MiddlewareOptions.ObserveOnly implements the spec's warn_for shadow-mode rollout stop (between supported_for and required_for): verification still runs, but a failing request reaches next.ServeHTTP with no VerifiedSigner in its context instead of getting a 401, and the failure is logged at INFO. A partial or malformed Signature/Signature-Input pair still hard-rejects even under ObserveOnly, per the spec's explicit carve-out that such a pair can't be safely read as signed or unsigned traffic. MIGRATION.md's step-B guidance, which previously described this as a not-yet-landed OnReject shim, is updated to use the real API. Built against adcp/v3/signing — the active module per README's "Modules & versioning" table; adcp/signing (v2) is frozen for security backports only, so this feature does not touch it. Closes #53
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — 1 medium finding, no blockers, no escalation triggers.
What I checked
- ObserveOnly shadow-mode signing branch (adcp/v3/signing/middleware.go): well-shaped and tested; does not weaken TMP signature verification, replay-window, or canonicalization semantics.
- tmproto SSRF URL validator (tmproto/urlsecurity.go, added): new file, thoroughly tested.
- Context-agent error sanitization (targeting/contextagent/handler.go): modification preserves contract; no medium-or-higher concern raised.
- Seller-agent newServer refactor + MCP tests: well-wired and covered.
Medium findings
- tmproto/urlsecurity.go — SSRF pre-flight's
net.ParseIPgate misses non-canonical IPv4 literals (decimal/octal/hex). Mitigated for Go stdlib callers; a completeness gap for the canonical validator, not a load-bearing break.
Decision path
- Row 1: no critical/high findings.
- Row 2: gated_paths is false — not applicable.
- Row 3: no
(deleted)reasons. - Row 4: the single medium is a URL/SSRF validation gap, not data-loss/schema/infra category — not applicable.
- Row 5: high_risk is true with
(modified)files, but this fires only if a finding is medium AND on a modified high-risk file. The SSRF medium is on tmproto/urlsecurity.go, which is(added), not(modified)— row 5 does not fire. - Row 6: no prior escalation.
- Row 7: no no-auto-approve team match.
- Row 8: only 1 medium finding (need ≥3).
- Row 9: approve.
The high_risk flag is a heuristic; the only medium sits on a newly added file, and modified high-risk files carry no medium-or-higher concerns. Falls through to approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean, additive security hardening.
Reviewed the delta since the prior clean pass. This PR adds a DNS-free SSRF pre-flight validator (tmproto/urlsecurity.go) wired into Artifact/asset Validate() and reachable via ValidateContextRequest, fixes a context-agent err.Error() leak (now returns a generic response and logs server-side with an elided unsafe request_id), and extracts a seller-agent newServer() with new MCP-level and ForceAccountStatus tests. Verified IP-range masks/boundaries, non-echoing error text, and validator reachability; the extraction is a no-logic move.
No Critical/High/Medium findings.
Decision-table walk: rows 1 (no critical/high) and 4/5 (no medium) do not fire. gated_paths is false, so row 2 does not apply. high_risk is true but the only high-risk-path changes are (added) (new files — inherently low risk) or (modified) files with no medium-or-higher findings, so rows 3 and 5 do not fire. Prior decision was approve, so sticky escalation (row 6) does not apply. No no-auto-approve team match (row 7). Fewer than three medium findings (row 8). Falls through to row 9 → approve.
The merge-base changed after approval.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking findings.
Checked:
- All changes confined to
adcp/v3/signing/**— documentation (MIGRATION.md, README.md, doc.go), one additiveMiddlewareOptions.ObserveOnlyshadow-mode field in middleware.go, a newsigningtesttest-helper subpackage, and accompanying tests. - No schema (
adcp/schemas/**) or generated-type (adcp/types_gen.go) changes — coherence gate N/A. - No
tmproto/*signing/verification/replay/canonicalization/header changes; the ObserveOnly field is additive and the CodeHeaderMalformed carve-out fails closed (rejects more, not fewer). Default enforcement is byte-identical and regression-covered; no VerifiedSigner leaks under observe mode. - No identity-agent TEE changes, no protocol-managed skills edits, no breaking wire/public-API removals — conventional-commit marker gate N/A.
Gates: gated_paths false, high_risk false, no no-auto-approve team match. Prior decision was approve. No critical/high/medium findings — falls through to row 9 (approve).
Policy-compliant relay of external contribution #479 so repository-default CodeQL can run. The original contributor remains the commit author; maintainer and Ladon reviews plus CI passed on #479.
Closes #53.
Summary
adcp/v3/signing/signingtest—NewTestAgent(t)builds a matched Ed25519*signing.Signer+signing.MiddlewareOptions(a freshStaticJWKSResolverseeded with the signer's public key, a freshNewMemoryReplayStore(0), no revocation).SignAndSend(t, signer, handler, req)signsreq(withCoverContentDigest: true) and delivers it tohandlerin-process viahttptest.NewRecorder, returning the*http.Response. Together these collapse the ~30-line keypair/JWK/resolver/replay-store patternmiddleware_test.gocurrently hand-rolls into two lines of setup + one line to send.MiddlewareOptions.ObserveOnly bool— a real behavioral branch inMiddleware(), not a passthrough field. When true,VerifyRequestSignaturestill runs; on failure the request is logged atslog.LevelInfo(vs the normalslog.LevelWarn) with anobserve_only=trueattribute and passed tonext.ServeHTTPwith noVerifiedSignerin its context — i.e., treated exactly as unsigned. On success, behavior is unchanged.ObserveOnly↔ spec mappingThis maps to the AdCP transport spec's
warn_forrollout stop (supported_for → warn_for → required_for, see Transport capability advertisement). Per spec: "A missing signature or a well-formed signature that fails verification or body binding MUST NOT establish verified-signer identity ... A partial or malformed Signature / Signature-Input pair always hard-rejects."ObserveOnlyimplements that split precisely, not just "log and let everything through":RequiredForop, ...) is observed: INFO log, request passes through unauthenticated.Signature/Signature-Inputheader pair — one header present without the other, or either header present but unparseable (*Error{Code: CodeHeaderMalformed}) — still hard-rejects with 401 even underObserveOnly, because the spec says it "cannot be safely interpreted as either signed or unsigned traffic."adcp/v3/signing/MIGRATION.md's "Step B —warn_for" section previously described this exact mode as not yet landed, with anOnReject-based logs-and-passes-through shim as the interim workaround (and a "delete this before enablingRequiredFor" warning). This PR replaces that shim guidance with the realObserveOnlyAPI throughout the migration guide (bootstrap → step B → common pitfalls → pre-enforcement checklist).Module note
This targets
adcp/v3/signing, notadcp/signing. Per the repo'sREADME.md"Modules & versioning" table andMIGRATING.md,adcp(the pre-v3 module) is frozen at v2.1.1 and receives security backports only —adcp/v3is the actively developed module for AdCP 3.x. A new DX/feature package belongs there, soadcp/signing(the frozen copy) is untouched by this PR.Testing
signingtest's own tests (adcp/v3/signing/signingtest/signingtest_test.go) proveNewTestAgent+SignAndSendproduce a request a realsigning.Middleware-wrapped handler accepts (VerifiedSignerFromContextpopulated, correct algorithm), that the wired verifier is real and not a stub (rejects unsigned traffic on aRequiredForop, rejects a replayed signature), and thatSignAndSendfails fast on a non-absolute request URL (verified via a re-exec'd subprocess, since a subtest'st.Fatalfcan't be observed without failing the parent test/package).middleware_test.gogets three new tests:ObserveOnlylets an unsigned request to aRequiredForoperation through (INFO log, noVerifiedSigner);ObserveOnlylets a well-formed-but-cryptographically-invalid signature through, with a matchingObserveOnly=falsesubtest proving the existing 401 behavior is unchanged; andObserveOnlystill hard-rejects a malformedSignature/Signature-Inputpair (WARN log, 401,WWW-Authenticate: ... request_signature_header_malformed), proving the spec carve-out is real.go build ./...,go vet ./...,gofmt -l, andgolangci-lint run ./signing/...are clean for every file this PR touches. The threestaticcheckfindingsgolangci-lintreports inadcp/v3/signing/jwk.go(deprecatedecdsa.PublicKey.X/.Y/.PrivateKey.Dfield access, Go 1.26) are pre-existing onmainand untouched by this PR.go test ./...passes for both theadcp/v3module (this change) and the root module (unaffected).On item 5 of the issue ("refactor an existing boilerplate-heavy test as a demonstration")
I looked for one and came up empty, honestly reported: the request-signing profile's
StaticJWKSResolver/NewMemoryReplayStoreboilerplate pattern this issue targets appears nowhere else in the repo outsideadcp/signingandadcp/v3/signingthemselves.adcp/v3/webhook's test files (signing_test.go,publisher_test.go,e2e_test.go) use the webhook-signing profile via a different, already-existing dedicated helper (webhookKeypair) and a different verifier entry point (webhook.HTTPHandler, notsigning.Middleware) — not a good refactor target forsigningtest, which is scoped to the request-signing/signing.Middlewarepath the issue describes.I also couldn't refactor
adcp/v3/signing/middleware_test.go's ownTestMiddlewareEndToEndSignAndVerify(the literal ~30-line pattern quoted in the issue) in place: that file ispackage signing(internal, white-box), andsigningtestimportssigning— importingsigningtestfrom insidepackage signingwould be a cyclic import. Instead,signingtest's ownTestNewTestAgentSignAndSendRoundTripreproduces the same scenario (sign, verify via middleware, assert onVerifiedSignerFromContext) using the new two-line + one-line helpers, as the concrete demonstration of the line-count reduction, and the package'sREADME.md/doc.gocross-reference it for discoverability.Test plan
cd adcp/v3 && go build ./... && go vet ./... && go test ./...cd adcp/v3 && golangci-lint run ./signing/...go build ./... && go vet ./... && go test ./...at repo root (unaffected — confirms no accidental frozen-module edits)git statusclean in the working tree aside from the files this PR touches