fix(integrations): pin api_call to the SSRF-validated IP#8
Closed
ashvinctrl wants to merge 1 commit into
Closed
Conversation
execute_api_call validates the joined URL with check_outbound_url, which resolves the host and classifies every address, but it only returns (ok, reason) — no IP. The request then goes out on a plain httpx.AsyncClient, which resolves the host again when it opens the socket. Between those two resolutions the answer can change, so a low-TTL host that returns a public address for the guard and 169.254.169.254 for the connect reaches cloud metadata with the integration's stored auth headers attached. The agent can drive this through the api_call tool. Every other outbound path in the tree already pins against exactly this: web fetch has _PinnedTransport in services/search/content.py and webhook delivery has _PinnedAsyncTransport in src/webhook_manager.py. api_call had the validation half but not the pin. Wrap the resolver so the guard's own resolution is captured, then send the request through a local pinned transport bound to those addresses. Connect falls back address-by-address across the approved set on a dead first address, sharing one deadline so N dead addresses cannot stretch the connect phase to N * timeout, and never re-resolves. Only the socket destination is pinned — SNI and the Host header stay the original hostname. The transport builds its SSL context with httpx.create_ssl_context() so CA trust matches what the default client would have used. The two helpers in test_integrations_api_call_truncation.py stubbed check_outbound_url open, which skipped the resolution the pin reads; they now point the resolver at a public address so the real guard still runs.
Author
|
Closing - opened against the snapshot mirror by mistake. The api_call SSRF pin is tracked on the main repo as odysseus-dev/odysseus#5727. |
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
execute_api_callvalidates the URL withcheck_outbound_url, which resolves the host and classifies every address it gets back. But that function returnsTuple[bool, str], so the addresses it validated never reach the caller. The request then goes out on a defaulthttpx.AsyncClient, which resolves the host again when it opens the socket.Two independent lookups, nothing tying them together. The guard's answer has no bearing on where the socket ends up, as long as the first answer looked acceptable. By then
headersalready carries the integration's stored API key or bearer token. Reachable from the agent throughdo_api_callinsrc/tools/system.py.Everywhere else already closes this.
services/search/content.pyhas_PinnedTransportplus manual redirect handling that re-checks each hop, andsrc/webhook_manager.pyhas_PinnedAsyncBackend/_PinnedAsyncTransport. api_call had the validation half and not the pin.So: wrap the resolver so the guard's own resolution gets captured, then send the request through a pinned transport bound to those addresses.
A few details worth calling out, mostly things I got wrong on the first pass and had to fix:
Connect walks the approved addresses in order instead of taking only the first, otherwise a host whose first A record happens to be dead fails the whole call even though the guard approved several. Every address came out of the same resolution, so this is normal multi-address fallback and not a second lookup. The attempts share one deadline, so N dead addresses can't turn a 30s connect budget into N × 30s.
Only the socket destination is pinned. httpcore takes SNI and the
Hostheader from the request URL rather than the connect address, so certificate validation and vhost routing still point at the real hostname.The pool builds its SSL context with
httpx.create_ssl_context()rather thanssl.create_default_context(). Those aren't the same thing: the httpx one gives you certifi plusSSL_CERT_FILE/SSL_CERT_DIRwhentrust_envis set, which is what the default client would have used. The stdlib one quietly switches you to system roots, which would be a behaviour change nobody asked for.httpcore's exceptions get mapped back to their httpx equivalents so the existing
except httpx.*blocks further down behave the way they already did.I kept the transport local to the module instead of importing webhook_manager's, matching how web fetch and webhook delivery each carry their own. Didn't want api_call to start depending on the webhook subsystem for this.
Redirects aren't an issue here:
follow_redirectsdefaults to False and this call site doesn't override it, so the transport never sees a second host. That's also why this doesn't need the per-hop revalidation the web fetch path does.Two helpers in
tests/test_integrations_api_call_truncation.pywere stubbingcheck_outbound_urlopen, which skips the resolution the pin reads from. They now point_default_resolverat a public address instead, so the real guard runs and those tests still test the truncation and URL-joining behaviour they were written for.Target branch
dev, notmain.Linked Issue
Fixes #6
Type of Change
Checklist
devdocker compose uporuvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.How to Test
python -m pytest tests/test_integration_api_call_ssrf.py \ tests/test_integrations_api_call_truncation.py \ tests/test_integrations_url_join.py30 pass. Nine of them fail on dev: the pin assertions, the fallback ones, the CA-source one and the literal/zone-id ones. The four original guard tests pass before and after, so nothing that already existed changed behaviour.
Some of the coverage is there specifically because I didn't trust the fake-transport tests on their own:
test_real_socket_falls_back_from_dead_first_to_live_seconduses actual sockets, a dead first address and a live asyncio server second, and checks theHostheader survives the fallback.test_connect_raises_when_every_validated_address_is_deadpins the all-dead path so the fallback loop can't silently swallow failures.test_ip_literal_base_url_still_pins_and_is_not_rejecteduses the real resolver, no monkeypatch, to confirm a bare-IPbase_urldoesn't trip the new "did not resolve" branch.getaddrinfohands a literal straight back, so the captured list is populated and the pin is a no-op rather than a rejection. This one exists because that branch was the most likely way for me to have broken something.test_validated_ips_strips_zone_id_and_drops_junkcovers scoped v6 likefe80::1%eth0.To watch the actual rebind instead of a unit test: run two local HTTP servers on 127.0.0.1 and 127.0.0.2, point an integration's
base_urlat a hostname, have the guard's resolver return 127.0.0.1, and make the name answer 127.0.0.2 by connect time. On dev the request lands on the second server with theAuthorizationheader attached. On this branch it stays on the first. I ran it both ways.httpx 0.28.1 / httpcore 1.0.9.
One CI note: pytest can't go green here until the
Anyimport fix lands,src/agent_loop.pycan't be imported and it takestests/test_api_call_integration_routing.pydown with it during collection. Unrelated to this.