feat(signing): PostgreSQL-backed PgReplayStore for multi-instance verifiers#203
Merged
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
…ifiers Ships a production ReplayStore backed by PostgreSQL so multi-instance AdCP verifiers share nonce-seen state. A replay accepted on worker A can no longer land on worker B within the signature window. Closes #187. Postgres, not Redis: aligns with the idempotency PgBackend scaffolding direction, avoids a second infra dep, Redis is a clean follow-up. Shape: from psycopg_pool import ConnectionPool from adcp.signing import PgReplayStore, VerifyOptions pool = ConnectionPool('postgresql://...') replay = PgReplayStore(pool=pool) options = VerifyOptions(..., replay_store=replay) Design: - Caller owns the pool - Sync (matches current verifier); async lands with async verifier - Three single-statement queries; ON CONFLICT ... DO UPDATE handles legitimate nonce refresh - Lazy-only sweep; public sweep_expired() for cron - Fail-closed on errors - Table-name kwarg identifier-validated (zero injection surface) - COLLATE 'C' avoids locale case-folding - at_capacity via COUNT(*) >= cap on partial index Schema ships as plain SQL at src/adcp/signing/pg/replay_store.sql. Optional dep via [pg] extra (psycopg[binary] + psycopg-pool). Core SDK stays installable; adcp.signing.PgReplayStore resolves to None without the extra. Tests (+16) gated on ADCP_PG_TEST_URL. New pg-replay-store CI job runs against a Postgres 16 service container. Covers Protocol methods, TTL, at_capacity, per-keyid isolation, sweep, 10-thread ON CONFLICT correctness, case-variant isolation, identifier validation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ac6e63f to
3ad3951
Compare
MUST FIX: - _is_safe_identifier now uses re.fullmatch for byte-ASCII check. str.islower/isalpha accept Unicode homoglyphs (fullwidth Latin, café, Greek µ) that format into SQL as DIFFERENT tables than operators configured — a replay-bypass vector under multi-tenant config. - Dropped invalid commented partial index from replay_store.sql (now() is STABLE, not IMMUTABLE — the DDL was uncompilable). - Softened module-docstring fail-closed claim to match reality: psycopg errors propagate unchanged; frameworks return 5xx. DX P0: - PgReplayStore = None trap replaced with stub class raising ImportError + install hint (incl. Poetry command). - Module docstring example now shows full verify_request_signature wiring, not just construction. - New PgReplayStore.create_schema(pool) classmethod runs packaged DDL via importlib.resources — one-line bootstrap. - REQUIRED-labelled sweep callout with pg_cron + in-process snippets. SHOULD FIX: - ON CONFLICT DO UPDATE gated on EXCLUDED.expires_at > current to avoid MVCC write amp on shorter-TTL refreshes. - Removed dead import logging / logger scaffold. - Error message matches validator (ASCII-only). Tests (+3): - test_non_ascii_table_name_rejected (fullwidth, accented, Greek µ). - test_remember_twice_with_shorter_ttl_keeps_longer_expiry. - test_create_schema_idempotent. E2e verified: sign → verify (accept) → 2nd verify (rejected with request_signature_replayed) via PgReplayStore through verify_request_signature. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After shipping the base PR I ran a fresh-integrator walkthrough via subagent: pretend you've never seen the library, wire up PgReplayStore from scratch, report friction. Surfaced 5 real issues — all fixed — plus added genuine HTTP-over-the-wire e2e coverage. Full-wire e2e ------------- New test_pg_replay_store_e2e.py — signed HTTP requests over httpx.ASGITransport to a Starlette app running verify_starlette_request with PgReplayStore. Four scenarios: - happy path → 200 - replay → 401 with WWW-Authenticate: Signature error="request_signature_replayed" - fresh nonce after replay → 200 - cross-instance replay — two PgReplayStore instances on same pool; worker B rejects a replay that landed on worker A. The load-bearing property Postgres exists to provide over InMemoryReplayStore. CI's pg-replay-store job now runs both the unit + e2e files. DX fixes -------- 1. PgReplayStore.create_schema() now instance method honoring table_name. Previously a classmethod that silently created adcp_replay only — integrators with per-tenant table names had no working bootstrap path. Real bug, not just DX. 2. New load_private_key_pem(pem, *, password=None) helper on the public API. Closes the loop between adcp-keygen (writes PEM) and sign_request (takes PrivateKey) without requiring a direct cryptography import. 3. Package docstring now has a Quickstart section listing the ~10 names buyer / seller / governance paths actually reach for. 4. verify_starlette_request docstring corrected — previously promised request.state / VerifiedSigner body re-read that didn't exist. Now accurately documents Starlette's body caching and the Raises: contract. 5. New test_create_schema_honors_table_name exercises the bootstrap path with a custom table and asserts remember/seen both target the right table. Verified -------- Re-ran the fresh-integrator walkthrough — integrator reached green in 1 iteration, 0 tracebacks (down from 2 iterations + 1 traceback before). Custom table_name worked end-to-end. 262 signing tests pass (4 new e2e + 1 reshaped bootstrap test). Ruff + mypy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ships a production
ReplayStorebacked by PostgreSQL so multi-instance AdCP verifiers share nonce-seen state. A replay accepted on worker A can no longer land on worker B within the signature's validity window — the in-memory default left that gap on any load-balanced deployment.Closes #187.
Backend choice: Postgres (not Redis)
Issue #187 originally proposed Redis. Since then the idempotency work (PR #196) landed
PgBackendscaffolding as the intended shared-state backend, signaling the library's direction. Starting with Postgres:Shape
Design notes
seen/remember/at_capacity) + optionalsweep_expired()ON CONFLICT ... DO UPDATEhandles legitimate nonce refresh without error pathsseenself-filters; publicsweep_expired()for cronCOLLATE "C"onkeyid/nonce— blocks locale-dependent case folding that could collapse distinct slotsat_capacityusesCOUNT(*) >= capagainst a partial index — pays for accuracy exactly when a signer is misbehavingSchema
Ships as plain SQL at
src/adcp/signing/pg/replay_store.sql. Run it through whatever migration tool you already use — Alembic, Flyway, psql. Partial index forat_capacityis commented out in the DDL so integrators opt in per deployment.Optional dep via
[pg]extraCore SDK stays installable without SQL deps;
adcp.signing.PgReplayStoreresolves toNonewhen the extra isn't installed. Constructor raisesImportErrorwith the install hint if called without the extra.Testing
test_pg_replay_store.py— 16 tests, skipped whenADCP_PG_TEST_URLis unsetpg-replay-storeruns against a Postgres 16 service container on every PRat_capacitythreshold + per-keyid isolation,sweep_expired, 10-thread concurrentrememberon the same nonce (testsON CONFLICTcorrectness),COLLATE "C"case-variant isolation, identifier validationNot in scope (follow-ups)
PgReplayStore— needs an async verifier first (tracked separately)#182Pg idempotency backend — separate issue; this PR paves the psycopg3 dep path they'll shareTest plan
pytest tests/conformance/signing/ -q— 239 passed, 2 skipped (Pg tests skip without env var)ADCP_PG_TEST_URL=... pytest tests/conformance/signing/test_pg_replay_store.py -v— all 16 pass locallyruff check src/adcp/signing/ tests/conformance/signing/— cleanmypy src/adcp/signing/— clean (18 source files)pg-replay-storejob runs full suite against Postgres 16 service🤖 Generated with Claude Code