Skip to content

fix: add SSRF protection to RestApiTool._request()#6003

Open
Neelagiri65 wants to merge 1 commit into
google:mainfrom
Neelagiri65:fix/ssrf-protection-rest-api-tool
Open

fix: add SSRF protection to RestApiTool._request()#6003
Neelagiri65 wants to merge 1 commit into
google:mainfrom
Neelagiri65:fix/ssrf-protection-rest-api-tool

Conversation

@Neelagiri65
Copy link
Copy Markdown

@Neelagiri65 Neelagiri65 commented Jun 7, 2026

Re-opening of #5760 after signing the Google Individual CLA. Commit author updated to the CLA-covered identity, branch rebased onto current main, and the bot's review feedback addressed.

What

RestApiTool._request() was passing URLs to httpx.AsyncClient.request() with no validation. After #4368 fixed SSRF in load_web_page (hostname blocking, DNS pre-resolution, IP range filtering, scheme restriction), RestApiTool was not updated with the same protections.

A malicious OpenAPI spec with servers[].url pointing at 127.0.0.1 or 169.254.169.254 was reaching the HTTP client untouched.

Changes

New shared module src/google/adk/tools/_ssrf_protection.py

Both load_web_page and rest_api_tool need the same SSRF checks. The shared module exposes one source of truth.

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 Host header, sets TLS SNI via request.extensions["sni_hostname"]
  • is_blocked_hostname, is_blocked_address, resolve_host_addresses, rewrite_url_host — helpers reusable across stacks

Address bot's review feedback on PR #5760

The previous version of this PR had three issues flagged by the ADK triaging agent:

  1. Signature regression: the new httpx_client_factory parameter from main was being dropped. Now preserved; SSRF protection wraps both the factory-supplied client path and the default client path.

  2. DNS rebinding: validating the URL just before sending leaves a TOCTOU window. Even if getaddrinfo returns a public IP at validation, the second resolution at connect time could return a private IP. The fix uses pre-resolved IP pinning:

    • validate_url rejects the hostname if any record points to a non-globally-routable IP. So an attacker can't sneak past with [8.8.8.8, 127.0.0.1].
    • send_pinned_async rewrites the URL to use the validated IP literally, sets Host to the original hostname (HTTP routing), and sets sni_hostname (consumed by httpcore for TLS verification). The TCP socket goes to the IP we vetted, not whatever DNS returns at connect time.
  3. from __future__ import annotations added to tests/unittests/tools/test_ssrf_protection.py per the bot's style note.

Migrated load_web_page.py to the shared module

The original PR said "load_web_page can be migrated to use it in a follow-up if wanted". Doing it now: the duplicate copies of _is_blocked_hostname, _is_blocked_address, _resolve_host_addresses, _parse_ip_literal, _build_host_header, _format_host, and _rewrite_url_host are gone. load_web_page.py keeps its requests-specific _PinnedAddressAdapter (different HTTP stack) but reuses the shared resolution and blocking logic. Single source of truth across both tools.

What gets blocked

is_blocked_address uses ipaddress.is_global, so the same list applies to IPv4 and IPv6 without hand-maintained ranges:

  • Loopback (127.0.0.0/8, ::1)
  • Link-local (169.254.0.0/16, fe80::/10) — covers cloud metadata endpoints
  • Private (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7)
  • Multicast, reserved, unspecified
  • localhost, *.localhost, with case folding and trailing-dot handling
  • Non-http(s) schemes: file://, ftp://, gopher://, etc.

Testing Plan

Repro of the original vulnerability (pre-fix)

# malicious_spec.json
{
  "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 main:

$ python -c "
import asyncio, json
from google.adk.tools.openapi_tool.openapi_spec_parser import OpenAPIToolset
toolset = OpenAPIToolset(spec_dict=json.load(open('malicious_spec.json')))
tool = toolset.get_tools()[0]
asyncio.run(tool.call(args={}, tool_context=None))
"
# Request goes to 169.254.169.254 unimpeded.

On this branch:

ValueError: Blocked host: 169.254.169.254

The exception is raised by validate_url before any HTTP request hits the wire. Same outcome for 127.0.0.1, 10.0.0.1, localhost, 192.168.x.x, and IPv6 loopback / link-local.

DNS rebinding

A hostname that resolves to [8.8.8.8, 127.0.0.1] is rejected up front (multi-record check). Even if the attacker controls authoritative DNS and serves a public record at validation time, the validated IP is what the socket connects to (URL rewritten, Host header preserved). The TLS handshake uses the original hostname via sni_hostname so cert verification still works.

Test: tests/unittests/tools/test_ssrf_protection.py::TestValidateUrl::test_rebinder_blocked_when_any_record_is_private.

Running the tests

pip install -e .
pytest tests/unittests/tools/test_ssrf_protection.py -v
pytest tests/unittests/tools/openapi_tool/openapi_spec_parser/test_rest_api_tool.py -v

Coverage in test_ssrf_protection.py:

  • TestIsBlockedHostname (6 tests) — localhost, trailing dot, subdomain, case, normal, contains-localhost-but-not
  • TestIsBlockedAddress (8 tests) — loopback, link-local, private, IPv6 loopback / link-local / ULA, global IPv4, global IPv6
  • TestResolveHostAddresses (4 tests) — IPv4 literal, IPv6 literal, multi-record resolution, resolution failure
  • TestValidateUrl (10 tests) — every blocked class, public URL passes, DNS-rebinding case (multi-record with one private IP), ValidatedTarget has populated addresses
  • TestRewriteUrlHost (3 tests) — basic, explicit port preserved, IPv6 bracket formatting
  • TestSendPinnedAsync (3 tests) — URL pinned to IP literal, Host header preserved, sni_hostname extension set, body and headers passed through, fallback to next address on connect failure

New tests in test_rest_api_tool.py::TestRequestSsrfProtection (5 tests) — _request blocks localhost, loopback, metadata endpoint, private IP, file scheme.

What doesn't change

  • httpx_client_factory parameter and its semantics: still accepted, still used to construct the client when provided.
  • ssl_verify semantics: same as before; the verify value is read out and passed to the default httpx.AsyncClient (factory-supplied clients manage their own SSL).
  • Default request timeout: still None. Issue fix: OpenAPI tool _request missing timeout causes ReadTimeout with default httpx 5s #4431 explicitly disabled the httpx default. SSRF protection is orthogonal to timeout policy.
  • Public API of RestApiTool and OpenAPIToolset: no signature changes.

Related: #4368, #5760

@adk-bot adk-bot added the tools [Component] This issue is related to tools label Jun 7, 2026
@adk-bot
Copy link
Copy Markdown
Collaborator

adk-bot commented Jun 7, 2026

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:

  1. Testing Plan: Please add a testing plan section in your PR description outlining how the changes can be verified (or how you have verified them).
  2. Logs or Screenshots: For bug fixes and new features, providing logs or screenshots demonstrating the behavior after applying the fix would be extremely helpful for our reviewers.

Thank you for contributing to the ADK project! Your updates are highly appreciated.

@adk-bot
Copy link
Copy Markdown
Collaborator

adk-bot commented Jun 7, 2026

I have completed a thorough, read-only analysis of Pull Request #6003 (fix: add SSRF protection to RestApiTool._request()).

First, the mandatory Google CLA check has successfully passed (SUCCESS), indicating that it is fully compliant for review.

A premium and structured report has been compiled and saved to analysis_results.md.

Key Findings & Action Items

  • Signature Regression (Critical): The PR's _request signature in rest_api_tool.py is outdated and completely overwrites the httpx_client_factory parameter currently on main. This must be updated to avoid regression.
  • DNS Rebinding Flaw: While the PR filters localhost, private IPs, and link-locals, it only validates the URL prior to calling the client. It does not pin the connection to the resolved IP (unlike the robust _PinnedAddressAdapter mechanism found in load_web_page.py).
  • Documentation style: The test file tests/unittests/tools/test_ssrf_protection.py is missing from __future__ import annotations.

Summary of Work Done

  1. Compliance Verification: Successfully retrieved the check rollup status and verified the contributor's CLA.
  2. Diff & Codebase Review: Pulled the pull request branches, analyzed the diff line-by-line, and investigated how SSRF filter logic aligns with standard ADK patterns.
  3. Conflict Identification: Found critical divergence between the PR's baseline code block version and the target workspace main branch.
  4. Premium Reporting: Drafted a structured documentation report inside analysis_results.md for full

@Neelagiri65 Neelagiri65 force-pushed the fix/ssrf-protection-rest-api-tool branch from bd870cd to 4ab5256 Compare June 7, 2026 17:35
@adk-bot
Copy link
Copy Markdown
Collaborator

adk-bot commented Jun 7, 2026

I have completed a comprehensive, read-only analysis of Pull Request #6003 (fix: add SSRF protection to RestApiTool._request()).

The mandatory Google CLA Signature Check has successfully verified (SUCCESS), indicating that the changes are safe and eligible for detailed review.

I have generated a premium, structured analysis report and saved it to analysis_results.md.

💡 Key Findings & Action Items

  • Core Value: Extremely high value security improvement. The PR extracts duplicating logic out of load_web_page.py and builds a shared internal module _ssrf_protection.py to prevent SSRF and DNS rebinding attacks on rest_api_tool.py.
  • Architecture Alignment: Fully compliant alignment. All new module files are correctly private (prefixed with an underscore) and self-contained. No public API surfaces were broken.
  • Typing Nits: A few minor style guide violations remain in _ssrf_protection.py regarding legacy Optional typing. Replacing these with the modern union syntax | None is recommended (e.g., Optional[int]int | None).

📦 Summary of Work Done

  1. Compliance Verification: Checked eligibility status verifying the contributor's Google CLA is signed and approved.
  2. Review Changes: Pulled the pull request branch, analyzed the diff against main, and evaluated architectural alignment.
  3. Drafted Report: Wrote a premium architectural and stylistic assessment at analysis_results.md.

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
@Neelagiri65 Neelagiri65 force-pushed the fix/ssrf-protection-rest-api-tool branch from 4ab5256 to 88690cf Compare June 7, 2026 17:39
@adk-bot
Copy link
Copy Markdown
Collaborator

adk-bot commented Jun 7, 2026

I have completed a thorough, read-only architectural and security analysis of Pull Request #6003 (fix: add SSRF protection to RestApiTool._request()).

First, the mandatory Google CLA Signature Check has successfully verified (SUCCESS from git state metadata and description analysis), indicating that the patch is completely compliant and eligible for a deep review.

I have generated a premium, detailed analysis report and saved it directly to your workspace artifacts:
👉 analysis_results.md


💡 Highlight Summary & Findings

  • Core Value (High Quality): This PR introduces extremely robust SSRF and DNS-rebinding protection to RestApiTool. It does so extremely cleanly by centralizing the validation logic into a shared private module _ssrf_protection.py and migrating both RestApiTool and load_web_page to use it (removing previous duplicate helper code).
  • DNS-Rebinding Gate: The PR closes the TOCTOU DNS-rebinding window securely by validating multi-record responses (blocking if any target is private) and pinning requests directly to validated IP literals with matching Host header and TLS sni_hostname extensions.
  • Architecture & Principles Alignment: Fully compliant (Pass). The helper subsystem is kept strictly private (indicated by the leading underscore), avoids breaking any stable public APIs, and implements PEP 604 type unions alongside standard behavior-focused unit tests.

📋 Work Completed

  1. Repository Diff Analysis: Checked out and parsed PR 6003 diffs across all 5 affected files.
  2. Quality & Compliance Review: Assessed logic patterns in the new _ssrf_protection.py component for Pydantic/Python standards and correct exception cascades.
  3. Structured Reporting: Artifact saved to analysis_results.md. No changes are requested; this contribution is ready for approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tools [Component] This issue is related to tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants