Repository navigation
fix: add SSRF protection to RestApiTool._request() - #6003
Neelagiri65 wants to merge 4 commits into
Conversation
|
Response from ADK Triaging Agent Hello @Neelagiri65, thank you for your pull request and for completing the CLA! We noticed a couple of items from our contribution guidelines are currently missing:
Thank you for contributing to the ADK project! Your updates are highly appreciated. |
|
I have completed a thorough, read-only analysis of Pull Request #6003 ( First, the mandatory Google CLA check has successfully passed ( A premium and structured report has been compiled and saved to analysis_results.md. Key Findings & Action Items
Summary of Work Done
|
bd870cd to
4ab5256
Compare
|
I have completed a comprehensive, read-only analysis of Pull Request #6003 ( The mandatory Google CLA Signature Check has successfully verified ( I have generated a premium, structured analysis report and saved it to analysis_results.md. 💡 Key Findings & Action Items
📦 Summary of Work Done
|
RestApiTool._request() passes URLs straight to httpx with no validation. load_web_page got SSRF protections after google#4368 (hostname blocking, DNS pre-resolution, IP range filtering, scheme restriction) but RestApiTool was not updated. This extracts the SSRF validation logic into a shared _ssrf_protection module and applies it in _request() before making HTTP calls. Also sets a finite timeout (30s) instead of None. Blocked: localhost, *.localhost, loopback, link-local (169.254.x.x), private ranges (10.x, 172.16-31.x, 192.168.x), non-http(s) schemes. Related: google#4368
4ab5256 to
88690cf
Compare
|
I have completed a thorough, read-only architectural and security analysis of Pull Request #6003 ( First, the mandatory Google CLA Signature Check has successfully verified ( I have generated a premium, detailed analysis report and saved it directly to your workspace artifacts: 💡 Highlight Summary & Findings
📋 Work Completed
|
|
Hi @Neelagiri65 , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please address the failing CI tests. |
…hange - Revert load_web_page.py to upstream: it now has its own inline SSRF implementation with a test suite that patches module-local names, so migrating it to the shared module broke those tests. Keep the shared _ssrf_protection module for RestApiTool only. - Replace RFC 5737 documentation IPs (203.0.113.x) in SSRF tests with globally routable IPs. Recent CPython reclassifies the documentation ranges as is_global=False, so they were being blocked as private.
|
Thanks @rohityan. Rebased onto current main and sorted the failing checks.
Happy to adjust if you'd prefer the shared module folded directly into |
`RestApiTool` and `OpenAPIToolset` now refuse loopback, private, link-local (including cloud metadata), and `*.internal` / `*.local` request targets by default and return an error response. Pass `allow_private_network_access=True` to `RestApiTool`, `RestApiTool.from_parsed_operation`, `OpenAPIToolset`, `APIHubToolset`, or `ApplicationIntegrationToolset` to opt out when targeting localhost, private networks, or custom non-DNS transports. Merge #6003 PiperOrigin-RevId: 1002544850
|
Thank you @Neelagiri65 for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit 30726ea. Closing this PR as the changes are now in the main branch. |
Re-opening of #5760 after signing the Google Individual CLA. Branch rebased onto current main, merge conflict resolved, and the failing CI addressed.
What
RestApiTool._request()passed URLs tohttpx.AsyncClient.request()with no validation. After #4368 fixed SSRF inload_web_page(hostname blocking, DNS pre-resolution, IP range filtering, scheme restriction),RestApiToolwas never given the same protection.A malicious OpenAPI spec with
servers[].urlpointing at127.0.0.1or169.254.169.254reached the HTTP client untouched.Changes
New private module
src/google/adk/tools/_ssrf_protection.py. It carries the SSRF checks for the httpx-based request path used byRestApiTool.Public surface:
validate_url(url) -> ValidatedTarget— scheme check, hostname pattern block, DNS resolution, IP range filter. Returns the resolved addresses.send_pinned_async(client, target, **params) -> httpx.Response— issues the request against the validated IP literal, preserves the Host header, sets TLS SNI viarequest.extensions["sni_hostname"].is_blocked_hostname,is_blocked_address,resolve_host_addresses,rewrite_url_host— helpers.RestApiTool._request()now validates the URL and pins the connection before any request goes out. Thehttpx_client_factoryparameter andssl_verifysemantics are untouched. Factory-supplied clients still manage their own SSL.Scope note
An earlier version of this PR also migrated
load_web_page.pyto consume this shared module, for a single source of truth. That is dropped now.mainhas since givenload_web_page.pyits own inline SSRF implementation with a dedicated test suite that patches module-local names (load_web_page.socketand friends). Migrating that file breaks those tests and creates a standing conflict every time the file changes upstream. So this PR leavesload_web_page.pyexactly as it is onmainand only adds protection toRestApiTool. The two paths use different HTTP stacks anyway (requests vs httpx).What gets blocked
is_blocked_addressusesipaddress.is_global, so one rule covers IPv4 and IPv6 without hand-maintained ranges:127.0.0.0/8,::1)169.254.0.0/16,fe80::/10), which covers cloud metadata endpoints10.0.0.0/8,172.16.0.0/12,192.168.0.0/16,fc00::/7)localhost,*.localhost, with case folding and trailing-dot handlingfile://,ftp://,gopher://, and the restDNS rebinding
Validating the URL just before sending leaves a TOCTOU window. Even if
getaddrinforeturns a public IP at validation, the second resolution at connect time could return a private IP. The fix pins to the pre-resolved IP:validate_urlrejects the hostname if any record points to a non-globally-routable IP. An attacker cannot sneak past with[8.8.8.8, 127.0.0.1].send_pinned_asyncrewrites the URL to the validated IP literal, setsHostto the original hostname for routing, and setssni_hostname(consumed by httpcore) so TLS cert verification still works. The socket goes to the IP that was vetted, not whatever DNS returns at connect time.Testing plan
Repro of the original vulnerability on pre-fix
main:{ "openapi": "3.0.0", "info": {"title": "Internal API", "version": "1.0.0"}, "servers": [{"url": "http://169.254.169.254"}], "paths": { "/latest/meta-data/iam/security-credentials/": { "get": {"operationId": "leak_creds", "responses": {"200": {"description": "ok"}}} } } }On pre-fix
mainthe request goes to169.254.169.254unimpeded. On this branch:The exception is raised by
validate_urlbefore any HTTP request hits the wire. Same outcome for127.0.0.1,10.0.0.1,localhost,192.168.x.x, and IPv6 loopback / link-local.Running the tests:
test_ssrf_protection.pycovers hostname blocking, address blocking across IPv4/IPv6 classes, host resolution,validate_urlfor every blocked class plus the DNS-rebinding case, URL rewriting, andsend_pinned_asyncpinning behaviour.test_rest_api_tool.pyadds five tests that block localhost, loopback, the metadata endpoint, a private IP, and thefile://scheme through_request.Note for reviewers: the SSRF tests use
8.8.8.8/8.8.4.4as the stand-in public addresses. Recent CPython reclassifies the RFC 5737 documentation ranges (203.0.113.0/24and friends) asis_global=False, so the earlier version of the tests that used those addresses was correctly blocked by the production code and failed. Real globally-routable literals avoid that. All of them are mocked, no request leaves the process.Related: #4368, #5760