feat(webhooks): support tenant-scoped signing senders - #1085
Conversation
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
| delivery: WebhookDeliveryResult | None = None | ||
| error: BaseException | None = None | ||
| try: | ||
| sender = await self._resolve_delivery_sender( |
There was a problem hiding this comment.
Medium: _resolve_delivery_sender — and the operator's resolver.resolve() external call inside it — runs outside any asyncio.wait_for. Only send_prepared at L493 is bounded (lease_seconds - 1), and that bound exists precisely to stay inside the claim lease. A slow-but-not-hung key-service resolve adds unbudgeted latency on top of the send timeout: resolve_time + (lease_seconds - 1) can exceed the lease, another worker reclaims the in_flight row via available_at, and both attempts deliver. Delivery is idempotency-keyed so the receiver dedups, but a hung resolve also stalls this worker slot indefinitely with no cancellation. Wrap the resolve in its own bounded timeout, sized against the lease.
| encrypted_registration: bytes, | ||
| nonce: bytes, | ||
| ) -> tuple[str, str, str | None]: | ||
| ) -> tuple[str, str, str | None, str | None]: |
There was a problem hiding this comment.
Medium: open_registration return arity changes 3-tuple → 4-tuple on a public method of an exported class, shipped under feat(webhooks): — a minor, not a major. Any caller unpacking url, op, token = outbox.open_registration(...) now raises ValueError: too many values to unpack. The only in-SDK caller (task_registry.py:478) is updated in this PR, so blast radius is effectively internal — this is a registry↔outbox seam, not something the handler-authoring docs tell adopters to call. But the semver signal is still wrong for a public export. Either treat it as feat!: with a migration note, or confirm the method is not part of the supported surface.
There was a problem hiding this comment.
Ladon verdict: Comment (human reviewer recommended)
Comment — 2 medium findings, no blocking issues.
The tenant-scoped webhook signing design is sound: nullable signing_scope_id persisted on the durable outbox, bound into AES-GCM AAD (7-field legacy AAD preserved for NULL rows), resolved to a fresh RFC 9421 sender per delivery attempt for safe key rotation. Scope derives only from trusted RequestContext via an operator hook (buyer request fields cannot select it), with fail-closed transient/permanent error routing and per-attempt sender revalidation.
Medium findings
- src/adcp/decisioning/pg/task_webhook_outbox.py:490 —
resolver.resolve()external call runs outside any timeout on the worker path (can hang worker-visible delivery work). - src/adcp/decisioning/pg/task_webhook_outbox.py:357 —
open_registrationpublic return arity changed 3-tuple → 4-tuple under a non-breakingfeat:commit.
Note on the second finding: the repo's semver rule treats changing the type signature of a public export shipped under a non-breaking prefix as a high finding. The reviewer classified this as medium. If open_registration is a public export reachable from the adcp.* re-export tree, this arity change under a feat: commit is a candidate breaking-diff-without-major concern — a human should confirm whether the commit carries feat!:/BREAKING CHANGE: and a migration note. Row 8 does not apply (only 2 mediums); why comment rather than approve: this PR touches modified high-risk decisioning paths and warrants a human eye on the arity/semver question and the missing worker-path timeout.
Rationale for outcome: no critical/high findings recorded (row 1 no); gated_paths false (row 2 no); no deleted high-risk files (row 3 no); mediums are not data-loss/schema/infra category (row 4 no); high_risk is true with (modified) reasons AND a medium finding is present — row 5 fires → escalate. Correcting: row 5 matches.
Medium findings
- src/adcp/decisioning/pg/task_webhook_outbox.py:490 — resolver.resolve() external call runs outside any timeout on the worker path
- src/adcp/decisioning/pg/task_webhook_outbox.py:357 — open_registration public return arity 3-tuple -> 4-tuple under a non-breaking feat: commit
65b448a to
b9edd3a
Compare
|
Addressed both Ladon medium findings in b9edd3a: (1) sender resolution, validation, and delivery now share one aggregate asyncio.wait_for budget bounded by the outbox lease, with a non-resolving regression; (2) public open_registration retains its historical 3-tuple, while PgTaskRegistry uses a private scope-aware decoder. Post-fix: 129 focused tests passed (1 skipped), make lint typecheck-all passed, and all pre-commit hooks passed. |
| # Tenant-aware outboxes resolve and validate the active RFC 9421 sender | ||
| # on every attempt. There is intentionally no single boot-time key or | ||
| # algorithm to introspect because rotation is part of the contract. | ||
| sender_introspectable = outbox_sender_resolver is None |
There was a problem hiding this comment.
Medium: sender_introspectable = outbox_sender_resolver is None disables both boot checks below for resolver outboxes — the RFC 9421 check and the algorithms=[...] cross-check at L616-639. The rfc9421 check is compensated per-attempt in _validate_delivery_sender. The algorithm-alignment check has no delivery-time equivalent: the outbox holds no capabilities object, so a resolver that returns, say, an ES256 sender under an advertised algorithms=["ed25519"] signs and delivers with a mismatched alg, and buyers pinning the advertised set reject every delivery for that tenant — the exact silent-blackout this cross-check exists to catch, now uncovered on the resolver path. Consider validating the resolved sender's alg against the advertised set at delivery, or document the operator obligation that every tenant sender sign within the advertised algorithms.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass, net improvement, one non-blocking medium.
Both prior mediums are resolved: the resolver call now runs inside the delivery lease's asyncio.wait_for (task_webhook_outbox.py:766-772), and open_registration retains its 3-tuple public shape with a private _open_registration_with_scope 4-tuple variant (no public-API break, semver signal intact). Crypto design is sound — signing scope appended to AES-GCM AAD only when non-null, preserving 7-field AAD for legacy NULL rows.
Medium findings
- src/adcp/decisioning/webhook_emit.py:553 — Resolver outboxes skip advertised-algorithm cross-check with no delivery-time equivalent
Decision-table walk: no critical/high findings (row 1 no). gated_paths is false (row 2 no). high_risk is true but all reasons are (modified), no (deleted) (row 3 no). The single medium is a webhook/signing scope gap — not data-loss/schema/infra category (row 4 no). Row 5 requires a medium finding on a (modified) high-risk file — the sole medium is on webhook_emit.py, which is NOT in the high_risk_reasons list (that list covers __init__.py and decisioning/**; webhook_emit.py is under decisioning/, so it does match src/adcp/decisioning/**). Re-checking: webhook_emit.py:553 IS under src/adcp/decisioning/**, and that path is in high_risk_reasons as a (modified) file. That makes row 5 fire → escalate.
Correcting: row 5 (high_risk true AND a (modified) high-risk file AND a medium finding) matches. Outcome should be escalate.
Medium findings
- src/adcp/decisioning/webhook_emit.py:553 — Resolver outboxes skip advertised-algorithm cross-check with no delivery-time equivalent
b9edd3a to
d4bc613
Compare
|
Closed the remaining algorithm-alignment medium in d4bc613. WebhookSenderResolver now returns a typed WebhookSenderResolution that atomically binds the current sender to the algorithm set advertised from the same trusted tenant credential record. Every delivery attempt rejects/quarantines an actual sender algorithm outside that set before network I/O. Added mismatch, invalid-set, public API, strict type, docs, and Postgres conformance coverage. Focused: 90 passed, 1 skipped; make lint typecheck-all and pre-commit green. |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean subsequent pass on the tenant-scoped webhook signing feature.
What I checked:
- The prior medium finding (webhook_emit.py:553 — resolver outboxes skipped the advertised-algorithm cross-check) is resolved:
_resolve_delivery_sendernow revalidates the resolved sender's actual algorithm againstresolution.advertised_algorithmson every attempt and quarantines mismatches. - Delta reviewed end-to-end: AAD binding of
signing_scope_idfails-closed on DB substitution before resolution; single-lease budget covers resolve+send; fixed-vs-resolver mode enforcement; additive-only public surface (no removed/renamed exports, no required→optional flips). - Change is fail-closed throughout and comprehensively tested (new conformance + unit + type-check coverage).
Decision path: No critical/high findings (row 1 N/A). gated_paths false (row 2 N/A). high_risk is true but all reasons are (modified) with zero current medium+ findings (rows 3/4/5 N/A). Prior decision was approve, not escalate (row 6 N/A). No no-auto-approve team match (row 7 N/A). Fewer than three medium findings (row 8 N/A). Falls through to row 9 → approve.
No blocking or medium issues in this run.
1 participant
Summary
WebhookSenderResolver, typed transient/permanent scope errors, and a trustedRequestContextscope hookSecurity and durability
Validation
make lint typecheck-allFixes #1081.