Commit 84b837e
feat(signing): close 4 SSRF gaps and add opt-in port hardening (foundation audit)
* fix(signing): close 5 SSRF and tenant-isolation gaps from foundation audit
Pre-foundation cleanup surfacing from the v6.0 DecisioningPlatform
foundation audit. Each fix closes a real bug or spec gap in the existing
adcp.signing surface independently of the framework work that builds on
top.
1. Port allowlist for SSRF-validated outbound HTTP
(adcp.signing.jwks.{validate_jwks_uri, resolve_and_validate_host},
ip_pinned_transport.{build_ip_pinned_transport, build_async_ip_pinned_transport})
- Default permits {443, 8443}; rejects :25, :6379, :11211, etc. on
resolved public IPs. Buyers can no longer smuggle traffic to
internal SMTP / Redis / Memcached via webhook URLs on non-standard
ports even when the IP itself is routable.
- Configurable via allowed_ports kwarg; empty frozenset is the
test-only escape hatch.
- Test: tests/conformance/signing/test_jwks.py
(test_ssrf_rejects_disallowed_ports + parametrized matrix)
2. WebhookSender owned-client path uses pin-and-bind transport
(adcp.webhook_sender.WebhookSender._send_bytes)
- Previous implementation reused a single httpx.AsyncClient across
all destinations and bypassed the IP-pinned transport entirely.
A buyer-supplied webhook URL pointing at 127.0.0.1 or AWS metadata
would deliver successfully.
- Now: when the sender owns its httpx client (default), every
delivery builds a per-request AsyncIpPinnedTransport. Per-request
re-resolution is intentional — keeping a pinned transport alive
across deliveries to the same hostname would defeat the rebinding
defense.
- When the operator supplies their own client (vetted egress proxy,
ASGI test transport), the framework trusts them completely; the
operator owns SSRF guarantees on their transport.
- Tests: test_owned_client_rejects_loopback_destination,
test_owned_client_rejects_disallowed_port
3. Tenant-scoped JWKS resolver Protocol
(adcp.signing.jwks.{JwksResolver, AsyncJwksResolver},
adcp.signing.verifier.VerifyOptions, webhook_verifier.WebhookVerifyOptions)
- Adds optional ``tenant_id`` kwarg to the resolver Protocol
so a resolver instance shared across tenants can refuse keys
outside the active tenant's published JWKS. Cross-tenant key
confusion (a buyer signing for tenant B who knows tenant A's
key_id) is closed at the resolver layer, not the verifier.
- Single-tenant in-tree impls (Static, Caching, AsyncCaching)
accept the kwarg as a pass-through — tenant scoping is a wrapper
concern, and adopters compose tenant-scoped resolvers around
existing single-tenant resolvers.
- VerifyOptions.tenant_id and WebhookVerifyOptions.tenant_id thread
the value through; verifier.py:227 passes it on resolver call.
- Test: tests/conformance/signing/test_jwks.py
(test_tenant_scoping_wrapper_pattern — reference pattern for
adopters; test_static_resolver_accepts_tenant_id_kwarg —
backward-compat invariant)
4. content-digest required-by-default for inbound request signing
(adcp.signing.verifier.VerifierCapability)
- Default already correct (covers_content_digest="required" at
verifier.py:95). Adds a regression test pinning the default so a
future "make it lenient" refactor surfaces in CI.
- Body integrity must be authenticated end-to-end; "either" or
"forbidden" lets a MITM inside TLS termination swap bodies on
signed requests whose digest isn't covered.
- Test: tests/conformance/signing/test_verifier_defaults.py
(test_default_covers_content_digest_is_required)
5. WebhookPayload.operation_id docstring fix
(adcp.webhooks.create_mcp_webhook_payload)
- Docstring previously said "deprecated from payload, should be in
URL routing, but included for backward compatibility."
Contradicted the schema at mcp-webhook-payload.json which says
publishers MUST echo this back so buyers correlate notifications
without parsing URL paths.
- Field already supported in the payload constructor; only the
docstring needed correction. Adds a test confirming the
end-to-end echo from send_mcp(operation_id=...) into the
delivered payload.
- Test: test_send_mcp_threads_operation_id_into_payload
All 5 fixes ship together as a security-prep PR before the v6.0
DecisioningPlatform foundation work lands. Each is independent of the
others; reviewers can evaluate by gap.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signing): scope down per expert review
Expert review of the prep PR (security-reviewer, code-reviewer,
python-expert, ad-tech-protocol-expert) flagged 3 changes that should
move to the foundation PR or be reversed:
1. JWKS tenant_id kwarg removed
- Protocol expert: tenant_id is the wrong axis for JWKS isolation;
AdCP anchors keys at agents[].jwks_uri, not at a seller-internal
tenant_id. CachingJwksResolver(jwks_uri=...) already isolates by
URL.
- Security: the kwarg was opt-in with no enforcement signal —
adopters who built a tenant-scoping resolver and forgot to thread
tenant_id on every call would silently fall back to single-tenant.
- Python: the JWKS document verifier (jws.py) didn't thread the
kwarg, so the multi-tenant guarantee only held on the RFC 9421
path — undercutting the stated security improvement.
- Code review: Protocol shape change with no compat shim was a
breaking change for external resolver implementations.
- Resolution: drop the kwarg from this prep PR. Reintroduce in the
foundation PR with the spec-correct axis (likely (jwks_uri, key_id)
or (agent_url, key_id)) and after the spec project clarifies
multi-tenant key isolation guidance.
2. Port allowlist default flipped to permissive
- Protocol expert: AdCP doesn't constrain pushNotificationConfig.url
ports (push-notification-config.json:7-11). Defaulting to {443,
8443} silently rejects legitimate buyers on :9443 (Tomcat),
:4443 (Spring Boot), or path-routed multi-tenant gateways.
- Security M2: implicit-HTTP rejection (port 80) wasn't documented
as scheme enforcement; adopters hitting it would widen the
allowlist and re-enable plaintext.
- Resolution: default allowed_ports=None (no port filter); operators
opt INTO {443, 8443} hardening by passing
allowed_ports=DEFAULT_ALLOWED_PORTS. The constant is exported
from adcp.signing for adopters who want the recommended posture.
The IP-range check + IP pinning still apply regardless of port —
the smuggle vector to internal services on the same routable IP
is closed by IP-range rejection, not by port enforcement.
3. covers_content_digest='required' regression test dropped
- Protocol expert: AdCP 3.0 spec explicitly sets
"default": "either" (get-adcp-capabilities-response.json:912-921)
with the rationale "'required' is recommended for spend-committing
operations in production; 4.0 recommends 'required' for those
operations."
- Existing code shipped "required" as the default before this PR —
a pre-existing divergence from the spec that's not my PR's bug to
pin. Drop the regression test that locked in the wrong default.
File a separate issue to address the spec divergence.
Also fixed (real bugs from the same review):
4. WebhookSender.from_jwk / from_pem now forward
allow_private_destinations + allowed_destination_ports to the
constructor. Documented happy-path adopters can now configure SSRF
policy without dropping to __init__.
5. Replaced `assert self._client is not None` (mypy-narrowing) on the
operator-supplied client path with an explicit RuntimeError. The
state is reachable (aclose() then re-send) and python -O strips
asserts, leaving the call to silently NoneType.post().
Net result for this PR (now 3 fixes instead of 5):
- IP-pinned webhook delivery (the actual security hole)
- Optional port allowlist as opt-in operator hardening
- operation_id docstring fix (schema-mandated echo)
Plus the from_jwk/from_pem ergonomics + assert-to-raise fix.
Tests: 2254 passing locally. New tests:
- test_jwks.py::test_ssrf_default_imposes_no_port_filter (parametrized)
— confirms :9443/:4443/:8080/:80 all pass without explicit allowlist
- test_jwks.py::test_ssrf_default_allowlist_passes_canonical_https_ports
- test_jwks.py::test_ssrf_empty_allowlist_rejects_every_port (sentinel)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(signing): apply 2nd-pass review — trust_env, validate-before-sign, tests
Second-pass expert review (security-reviewer, code-reviewer,
python-expert, ad-tech-protocol-expert) on the scope-down commit
3fd2c49 surfaced 1 ship-blocker, 2 real bugs, 3 missing tests, and a
nit. All addressed.
Ship-blocker (security-reviewer):
1. WebhookSender's per-request httpx.AsyncClient missed trust_env=False.
httpx defaults trust_env=True, which routes the signed webhook
through any HTTPS_PROXY / HTTP_PROXY env var, bypassing the
AsyncIpPinnedTransport entirely. Every other pinned-transport
callsite in this codebase explicitly sets trust_env=False
(default_jwks_fetcher, async_default_jwks_fetcher,
revocation_fetcher); the webhook sender was the outlier. An
attacker who controls process env (sidecar config, dotenv,
malicious cluster egress policy) could otherwise pivot to receiving
the signed webhook body. One-line fix at webhook_sender.py:577 with
a regression test that asserts the kwarg is set on the per-request
client.
Bugs (python-expert):
2. Stale docstrings in build_ip_pinned_transport and
build_async_ip_pinned_transport claimed allowed_ports defaults to
DEFAULT_ALLOWED_PORTS ({443, 8443}) — but the scope-down flipped
the default to None (no port filter). Adopters reading the
docstring would hit confusing rejections. Updated both to describe
the actual behavior.
3. _send_bytes signed the body before SSRF-validating the URL.
Restructured so the pinned-transport build (which runs SSRF + port
validation) happens first; signing only after validation succeeds.
Hostile URLs no longer leave a signed payload in process memory
for faulthandler / custom logging hooks to capture on exception.
New regression tests (code-reviewer + security-reviewer):
4. test_owned_client_default_allows_non_standard_ports — sender-level
positive analog of the validator-level test_ssrf_default_imposes_no_port_filter.
Confirms the permissive port default reaches the actual delivery
path; AdCP-spec-compliant buyers on :9443 (Tomcat) and similar
non-standard ports succeed without explicit allowlist.
5. test_operator_supplied_client_bypasses_ssrf_guard — named regression
guard for the documented contract. Without this, a future refactor
that mistakenly applies pin-and-bind to both branches would break
ASGI-based unit tests and any vetted-egress-proxy deployment that
routes via private networks.
6. test_owned_client_ignores_https_proxy_env — regression guard for
trust_env=False. Patches HTTPS_PROXY in env, asserts the per-request
client constructs with trust_env=False so the proxy is ignored.
Code-reviewer nit:
7. Deduplicated DEFAULT_ALLOWED_PORTS rationale block-comment between
adcp.signing.jwks (constant definition) and tests/conformance/signing/test_jwks.py.
Kept at the constant-definition site; test file points to it.
Commit type changed from fix(signing) to feat(signing):
The PR adds public surface (DEFAULT_ALLOWED_PORTS export, new kwargs
on validate_jwks_uri / resolve_and_validate_host / build_*_pinned_transport /
WebhookSender / from_jwk / from_pem) and changes WebhookSender._send_bytes
behavior on the owned-client path (now SSRF-validates and pin-binds
every delivery). Per semver, additive public-API surface = minor;
the security-fix-via-strictening-default is also conventionally a
minor bump. release-please should tag this as 4.1.0, not 4.0.1.
If squash-merging, the maintainer should use a feat(signing): PR title
so the squash subject carries the conventional-commit type that
release-please reads.
Tests: 2257 passing locally (3 new). Pre-commit clean (black, ruff,
mypy, bandit).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(signing): tighten test claims and add validate-before-sign assertion
Final-pass review on 50ae54d surfaced three test-quality nits, all addressed.
1. test_owned_client_default_allows_non_standard_ports docstring overclaim:
the test patches build_async_ip_pinned_transport with a fake, so the
IP-range check inside the real builder doesn't fire. Soften the claim
and point at test_owned_client_rejects_loopback_destination for the
IP-range coverage.
2. test_owned_client_ignores_https_proxy_env now also asserts
"transport" in captured_kwargs. Without this, a future refactor that
moves trust_env=False to the eager __aenter__ construction (away
from the per-request construction where the proxy-bypass guard
actually lives) would pass the test while leaving the per-request
client vulnerable.
3. test_owned_client_rejects_hostile_url_before_signing: new test for
the validate-before-sign claim made in the _send_bytes docstring.
Patches sign_webhook to a MagicMock, points the URL at 127.0.0.1,
asserts SSRFValidationError raises AND mock_sign.called is False.
No Ed25519/ES256 signature ever materializes for a URL that fails
the SSRF guard.
Tests: 2258 passing locally (up from 2257). Pre-commit clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>1 parent 2efa423 commit 84b837e
8 files changed
Lines changed: 558 additions & 36 deletions
File tree
- src/adcp
- signing
- tests/conformance/signing
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
157 | 157 | | |
158 | 158 | | |
159 | 159 | | |
| 160 | + | |
160 | 161 | | |
161 | 162 | | |
162 | 163 | | |
| |||
251 | 252 | | |
252 | 253 | | |
253 | 254 | | |
| 255 | + | |
254 | 256 | | |
255 | 257 | | |
256 | 258 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
289 | 289 | | |
290 | 290 | | |
291 | 291 | | |
| 292 | + | |
292 | 293 | | |
293 | 294 | | |
294 | 295 | | |
295 | 296 | | |
296 | 297 | | |
297 | | - | |
298 | | - | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
299 | 306 | | |
300 | 307 | | |
301 | 308 | | |
302 | 309 | | |
303 | 310 | | |
304 | 311 | | |
305 | 312 | | |
306 | | - | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
307 | 318 | | |
308 | 319 | | |
309 | 320 | | |
310 | 321 | | |
311 | 322 | | |
312 | 323 | | |
313 | 324 | | |
| 325 | + | |
314 | 326 | | |
315 | 327 | | |
316 | 328 | | |
317 | 329 | | |
318 | 330 | | |
319 | 331 | | |
320 | 332 | | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
321 | 337 | | |
322 | | - | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
323 | 343 | | |
324 | 344 | | |
325 | 345 | | |
326 | 346 | | |
327 | 347 | | |
328 | 348 | | |
329 | 349 | | |
| 350 | + | |
330 | 351 | | |
331 | 352 | | |
332 | 353 | | |
| |||
343 | 364 | | |
344 | 365 | | |
345 | 366 | | |
346 | | - | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
56 | 67 | | |
57 | 68 | | |
58 | 69 | | |
| |||
104 | 115 | | |
105 | 116 | | |
106 | 117 | | |
107 | | - | |
108 | | - | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
109 | 125 | | |
110 | | - | |
111 | | - | |
112 | | - | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
113 | 129 | | |
114 | | - | |
| 130 | + | |
115 | 131 | | |
116 | 132 | | |
117 | 133 | | |
118 | 134 | | |
119 | 135 | | |
120 | 136 | | |
| 137 | + | |
121 | 138 | | |
122 | 139 | | |
123 | 140 | | |
| |||
138 | 155 | | |
139 | 156 | | |
140 | 157 | | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
141 | 165 | | |
142 | 166 | | |
143 | 167 | | |
| |||
148 | 172 | | |
149 | 173 | | |
150 | 174 | | |
151 | | - | |
152 | | - | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
153 | 178 | | |
154 | 179 | | |
155 | 180 | | |
| |||
177 | 202 | | |
178 | 203 | | |
179 | 204 | | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
180 | 210 | | |
181 | 211 | | |
182 | 212 | | |
| |||
461 | 491 | | |
462 | 492 | | |
463 | 493 | | |
| 494 | + | |
464 | 495 | | |
465 | 496 | | |
466 | 497 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
47 | 51 | | |
48 | 52 | | |
49 | 53 | | |
| |||
112 | 116 | | |
113 | 117 | | |
114 | 118 | | |
| 119 | + | |
| 120 | + | |
115 | 121 | | |
116 | 122 | | |
117 | 123 | | |
118 | 124 | | |
119 | 125 | | |
120 | 126 | | |
121 | 127 | | |
| 128 | + | |
| 129 | + | |
122 | 130 | | |
123 | 131 | | |
124 | 132 | | |
| |||
128 | 136 | | |
129 | 137 | | |
130 | 138 | | |
| 139 | + | |
| 140 | + | |
131 | 141 | | |
132 | 142 | | |
133 | 143 | | |
134 | 144 | | |
135 | 145 | | |
136 | 146 | | |
137 | 147 | | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
138 | 151 | | |
139 | 152 | | |
140 | 153 | | |
| |||
163 | 176 | | |
164 | 177 | | |
165 | 178 | | |
| 179 | + | |
| 180 | + | |
166 | 181 | | |
167 | 182 | | |
168 | 183 | | |
| |||
175 | 190 | | |
176 | 191 | | |
177 | 192 | | |
| 193 | + | |
| 194 | + | |
178 | 195 | | |
179 | 196 | | |
180 | 197 | | |
| |||
194 | 211 | | |
195 | 212 | | |
196 | 213 | | |
| 214 | + | |
| 215 | + | |
197 | 216 | | |
198 | 217 | | |
199 | 218 | | |
| |||
239 | 258 | | |
240 | 259 | | |
241 | 260 | | |
| 261 | + | |
| 262 | + | |
242 | 263 | | |
243 | 264 | | |
244 | 265 | | |
| |||
471 | 492 | | |
472 | 493 | | |
473 | 494 | | |
474 | | - | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
| 517 | + | |
| 518 | + | |
| 519 | + | |
| 520 | + | |
| 521 | + | |
| 522 | + | |
| 523 | + | |
| 524 | + | |
| 525 | + | |
| 526 | + | |
| 527 | + | |
| 528 | + | |
| 529 | + | |
| 530 | + | |
475 | 531 | | |
476 | 532 | | |
477 | 533 | | |
| |||
495 | 551 | | |
496 | 552 | | |
497 | 553 | | |
498 | | - | |
499 | | - | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
| 562 | + | |
| 563 | + | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
| 568 | + | |
| 569 | + | |
| 570 | + | |
| 571 | + | |
| 572 | + | |
| 573 | + | |
| 574 | + | |
| 575 | + | |
| 576 | + | |
| 577 | + | |
| 578 | + | |
| 579 | + | |
| 580 | + | |
500 | 581 | | |
501 | 582 | | |
502 | 583 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
107 | 107 | | |
108 | 108 | | |
109 | 109 | | |
110 | | - | |
111 | | - | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
112 | 116 | | |
113 | 117 | | |
114 | 118 | | |
| |||
0 commit comments