feat(signing): add Postgres-backed ReplayStore for distributed verifier deployments - #480
Conversation
…er deployments Adds adcp/signing/pgreplay, a Postgres-backed adcp/signing.ReplayStore for verifier deployments running more than one process behind a load balancer. MemoryReplayStore dedups (keyid, nonce) per process; RFC 9421 §11.1 requires replay rejection, which a per-process cache can't deliver once there is more than one process. Closes adcontextprotocol#105, closes adcontextprotocol#54. Both ask for the same feature with different specs; this reconciles them: - Schema and API surface follow adcontextprotocol#105 (adcp_replay_cache with a (keyid, scope, nonce) primary key, NewPostgresReplayStore/HitCap/Seen/ Insert/GetReplayStoreMigration/SweepExpiredReplays) since it's cross-validated against the already-shipped JS (adcp-client#1018) and Python reference stores, and the scope column gives real value here: it lets one Postgres pool back multiple RFC 9421 tag profiles (adcp/request-signing/v1, adcp/webhook-signing/v1) without nonce collisions. - Package structure follows adcontextprotocol#54's pgreplay naming and its explicit ask for a separate go.mod submodule. Note: adcp/idempotency's own Postgres adapter (the thing adcontextprotocol#54 says to mirror) actually lives inside the shared adcp module today, not a separate module — it doesn't need one, since it only imports database/sql. pgreplay's production code is equally dependency-free, so the module boundary here is a deliberate, compiler-enforced guarantee rather than a load-bearing necessity. - adcontextprotocol#54's proposed Insert(...) (bool, error) interface change is not made: ReplayStore is a public interface with implementers outside this repo, and a signature change can't be verified against them from here. Instead, PostgresReplayStore exposes InsertContext(ctx, keyid, nonce, ttl) (bool, error) plus LastInsertError/LastSeenError/LastHitCapError so callers can distinguish "rejected" from "database unreachable" via errors.Is(err, ErrConnDown) without changing the shared interface. The interface-satisfying Insert/Seen/HitCap methods fail closed (reject) on any Postgres error either way. Bakes in the adcontextprotocol#105 gotcha: NewPostgresReplayStore probes the connection eagerly and panics with an actionable message on failure, so a misconfigured pool fails at wire-up instead of failing closed identically to "the verifier is broken" on the first signed request (adcp#3379). Package doc documents the recommended test/CI pattern (NewMemoryReplayStore(0), gated on an explicit production/staging check). Tests: unit tests (sqlmock, no Docker) plus a real Postgres 16 integration suite via testcontainers-go (-tags=integration, mirroring registry/redisstore's convention) proving the actual race the issues exist to close — concurrent Insert calls for an identical (keyid, scope, nonce) yield exactly one winner — plus HitCap enforcement, SweepExpiredReplays, scope isolation, and the constructor's eager-probe panic against a real closed connection. Both suites run and pass in this environment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc
adcp/signing/pgreplay was built against the frozen v2 module. The repo root README's "Modules & versioning" table and MIGRATING.md say adcp/signing is frozen at v2.1.1 and receives security backports only — a new distributed-store feature doesn't qualify, and the live development target for AdCP 3.x is adcp/v3/signing. Both adcontextprotocol#105 and adcontextprotocol#54's issue text names the pre-v3 path (adcp/signing/replay.go), predating the v3 split (the same issue a sibling PR against adcontextprotocol#53 found and corrected the same way). Moves the package to adcp/v3/signing/pgreplay, updates the module path, go.work.example, ci.yml's test step, and doc/README references accordingly. PostgresReplayStore doesn't import either signing package (a deliberate zero-dependency design choice, unrelated to this move), so it remains structurally usable as an adcp/signing.ReplayStore too — noted explicitly in the package doc and README for anyone still on the frozen module. Also points adcp/v3/signing/README.md's replay-cache section at the new package instead of the (reverted) edit to the frozen module's README. Rebuilt, re-vetted, and re-ran both the unit and -tags=integration test suites (real Postgres 16 via testcontainers-go) from the new location — all green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc
| if ttl <= 0 { | ||
| return false, fmt.Errorf("pgreplay: InsertContext: ttl must be positive, got %s", ttl) | ||
| } | ||
| expiresAt := time.Now().UTC().Add(ttl) |
There was a problem hiding this comment.
Medium: expires_at is computed on the app clock (time.Now().UTC().Add(ttl)), but every liveness check compares it against the Postgres server clock — hitCapSQL, seenSQL, and insertSQL's capped CTE all filter expires_at > now(). MemoryReplayStore is single-clock; this store spans two. If the app clock runs behind the DB clock by skew S, the effective replay-rejection window shrinks by S: Seen stops reporting a genuine nonce ~S early while the underlying signature is still inside its own validity window, so a tail-window replay is accepted — a fail-open on the RFC 9421 §11.1 MUST this package exists to enforce. Bounded by NTP skew in practice, but the dependency is silent. Consider computing expiry server-side (now() + $ttl::interval) so one clock governs both write and read, or document the app↔DB clock-sync assumption.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — new additive adcp/v3/signing/pgreplay Postgres-backed ReplayStore module.
What I checked:
- Purely additive new module (1468 additions, 1 deletion); no existing wire/public-API contracts touched, so no conventional-commit breaking marker required.
- No changes under
adcp/schemas/**oradcp/types_gen.go, so schema↔generated-types coherence is not implicated. - No
tmproto/*signing/verification semantics touched; the new ReplayStore verified for interface fidelity, atomic nonce race handling (PK + ON CONFLICT DO NOTHING), bounded timeouts, and fail-closed posture. - No identity-agent TEE or protocol-managed skills paths touched.
high_riskfalse;gated_pathsfalse; no no-auto-approve team match.
Medium findings (1):
adcp/v3/signing/pgreplay/store.go— expires_at computed on app clock while liveness checked against Postgresnow(); a two-clock replay window that can fail open under clock skew.
Decision table: no critical/high findings; gated_paths false; high_risk false; single medium finding is not in {data-loss, schema, infra} and count < 3; no team gate. Falls through to row 9 → approve. The lone medium is surfaced above for the developer to consider but does not block.
bokelley
left a comment
There was a problem hiding this comment.
Please address the inline replay-clock finding before merge. Expiry creation and liveness checks need one authoritative clock; computing expiry in PostgreSQL avoids shortening the replay window under app/DB skew.
Summary
Adds
adcp/v3/signing/pgreplay, a Postgres-backedadcp/v3/signing.ReplayStorefor verifier deployments running more than one process behind a load balancer.MemoryReplayStorededups(keyid, nonce)per process; RFC 9421 §11.1 requires replay rejection, which a per-process cache can't deliver once there's more than one process — a captured signature replayed against a sibling instance whose cache hasn't seen the nonce is accepted.Closes #105, closes #54.
Which module: adcp/v3/signing, not adcp/signing
Both issues' text names the pre-v3 path (
adcp/signing/replay.go). The repo root README's "Modules & versioning" table andMIGRATING.mdsay that module is frozen at v2.1.1, security-backports-only — the actively developed module for AdCP 3.x isadcp/v3/signing, and a new distributed-store feature isn't a security backport. (This wasn't obvious from the issue text alone; a sibling PR against #53 hit and disclosed the identical correction.)pgreplaywas built againstadcp/v3/signing.Because
PostgresReplayStoredoesn't import eithersigningpackage at all (see "Module boundary" below), it remains structurally usable as anadcp/signing.ReplayStoretoo — documented explicitly in the package doc and README for anyone still on the frozen module.Reconciling #105 and #54
Both issues ask for the same feature with different specs. Concretely:
adcp_replay_cache, PK(keyid, scope, nonce)adcp_signing_replay, PK(keyid, nonce)adcp/signing/pgreplayadcp/v3/signing/(see above)go.modInsertsignature(keyid, nonce, ttl) bool(matches current interface)(bool, error)bool; see belowSchema — #105 wins.
#105's(keyid, scope, nonce)primary key is cross-validated against two already-shipped reference implementations: the JS SDK'sPostgresReplayStore(adcp-client#1018, in production at agenticadvertising.org) and Python'sadcp-client-python(src/adcp/signing/pg/replay_store.py). Both use the identical schema and the identical atomicINSERT ... ON CONFLICT DO NOTHINGidiom. Keeping Go consistent with them matters for operators running more than one SDK. Thescopecolumn also earns its keep in this codebase specifically:adcp/v3/signingalready has two RFC 9421 tag profiles (adcp/request-signing/v1,adcp/webhook-signing/v1, seeProfile/Taginadcp/v3/signing/types.go) that could plausibly share one Postgres pool —scopekeeps their nonce namespaces isolated (pgreplay.WithScope(profile.Tag)).Package/module structure — #54 wins, with a caveat disclosed in the code.
#54explicitly asks for apgreplaypackage as its owngo.mod, "to keep the zero-third-party-deps guarantee of the main signing package — matches howadcp/idempotencyis structured." I checked:adcp/idempotency's own Postgres adapter (adcp/idempotency/postgres.go) actually lives inside the sharedadcpmodule today, not a separate module — it doesn't need one, because it imports onlydatabase/sql.pgreplay's production code is equally dependency-free (verified:go build ./...insideadcp/v3/signing/pgreplaywith zero external requires). So a separate module isn't load-bearing here the way #54's stated rationale implies. I built it as its own module anyway, because #54 asked for it explicitly and it's a real, if modest, benefit: a compiler-enforced guarantee that this package can never accidentally grow a dependency (a pgx-specific error-type check, say) that leaks into coresigning's import graph. This reasoning is written intoadcp/v3/signing/pgreplay/doc.goandREADME.mdso it isn't silently reverted later.Insertsignature — not changed; investigated and disclosed.#54raises a real point:Insert's bareboolreturn can't distinguish "cap rejected" from "couldn't reach the DB." I checked every call site ofReplayStore.Insertin this repo (adcp/v3/signing/verify.go:350and the frozenadcp/signing/verify.go:350, byte-for-byte identical). Changing it would be a one-line mechanical fix inside this repo — butReplayStoreis a public exported interface, and this repo can't see or verify external implementers (a hand-rolled Redis-backedReplayStore, for instance) who would silently break on a signature change the compiler here has no way to catch.adcp/signingandadcp/v3/signingexisting as two parallel copies of the same interface across a major-version split is itself evidence the maintainers already treat this kind of change as major-version-worthy rather than a same-PR edit.Instead:
PostgresReplayStore.Insert(andSeen/HitCap) fail closed on any Postgres error — same posture, samerequest_signature_rate_abusecode the caller sees either way, since RFC 9421 §11.1 makes a fail-open outcome unacceptable regardless of why the store failed. For operators who need to tell the two apart (alerting, health checks),PostgresReplayStoreadditionally exposes:InsertContext(ctx, keyid, nonce, ttl) (bool, error)— a non-nil error, wrappingErrConnDown, only on an actual round-trip failure.LastInsertError()/LastSeenError()/LastHitCapError()— the most recent round-trip error per method.Widening the shared
ReplayStore.Insertinterface itself is left as a natural, separately-reviewable follow-up (its own PR, ideally movingadcp/v3/signingonly, since the frozen module is backports-only).The #105 gotcha
NewPostgresReplayStoreprobes the connection eagerly (db.PingContext, 5s timeout) and panics with an actionable message on failure — mirrorsadcp/idempotency.New's existing "must not start in a state where cache writes silently fail" convention, and matches the suggested API's single-return-value constructor signature. This is the fix for the exact incident #105 describes (adcp#3379): aPostgresReplayStoreconstructed against a pool that doesn't exist in the current environment fails closed on every signed request, indistinguishable from "the verifier is broken," unless construction itself fails loudly first.The package doc and README spell out the corollary: don't construct a
PostgresReplayStorein tests or local dev — usesigning.NewMemoryReplayStore(0), gated behind an explicit production/staging environment check (mirrors the JS adopter'sgetReplayStore()pattern gated onNODE_ENV !== 'production').Testing — what's actually verified, not just asserted
Docker was available in the environment this PR was developed in, so both suites below were run for real, not just written:
Both pass. The integration suite (
adcp/v3/signing/pgreplay/integration_test.go) proves, against a live container:Insertcalls for the identical(keyid, scope, nonce)— modeling a captured signature replayed against every instance in a verifier pool at once — yield exactly one winner. Run with-race.Seen-then-Insertverifier flow (steps 12/13) under concurrency: multiple goroutines can pass theSeenpre-check for the same nonce, but at most one may winInsert.HitCapobserves rowsInsertactually wrote.SweepExpiredReplaysremoves only expired rows, leaves live ones untouched, and is idempotent (a second sweep removes nothing).NewPostgresReplayStorepanics against a real closed connection, not just a mocked one.scopes don't see each other's nonces.Integration tests are gated behind
-tags=integrationand skip (not fail) when Docker isn't reachable (t.Skipf), mirroringregistry/redisstore's andregistry/glidestore's existing convention — and, like those two packages, aren't wired intoci.yml's defaultgo test ./...step. I did add a plain (non-integration)go testCI step for the new module, since the sqlmock unit tests need no Docker and this seemed better than the precedent of registry/redisstore having no CI step at all.adcp/v3/signing/README.mdandgo.work.exampleare updated;scripts/check-goworkexample.shpasses locally. (An earlier revision of this PR placed the package under the frozenadcp/signing/pgreplayand editedadcp/signing/README.md; that revision was reverted in favor of theadcp/v3/signingplacement described above, andadcp/signing/README.mdis back to its original content.)Test plan
go build ./...andgo vet ./...at repo root, inadcp, inadcp/v3, and inadcp/v3/signing/pgreplay(plain and-tags=integration)golangci-lint run ./...inadcp/v3/signing/pgreplay— 0 issuesgo test -race -count=1 ./...inadcp/v3/signing/pgreplay— passgo test -race -tags=integration -count=1 -v ./...inadcp/v3/signing/pgreplayagainst a realpostgres:16-alpinecontainer — pass, including the concurrent-insert race proofgo test -race -count=1 ./...inadcpand inadcp/v3(unchangedsigning/idempotency/webhookpackages still pass)bash scripts/check-goworkexample.sh— passCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com