Skip to content

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

Closed
Neelagiri65 wants to merge 4 commits into
google:mainfrom
Neelagiri65:fix/ssrf-protection-rest-api-tool
Closed

Neelagiri65 wants to merge 4 commits into
google:mainfrom
Neelagiri65:fix/ssrf-protection-rest-api-tool

Conversation

@Neelagiri65

@Neelagiri65 Neelagiri65 commented Jun 7, 2026 •

Copy link
Copy Markdown

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 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 never given the same protection.

A malicious OpenAPI spec with servers[].url pointing at 127.0.0.1 or 169.254.169.254 reached 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 by RestApiTool.

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 via request.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. The httpx_client_factory parameter and ssl_verify semantics are untouched. Factory-supplied clients still manage their own SSL.

Scope note

An earlier version of this PR also migrated load_web_page.py to consume this shared module, for a single source of truth. That is dropped now. main has since given load_web_page.py its own inline SSRF implementation with a dedicated test suite that patches module-local names (load_web_page.socket and friends). Migrating that file breaks those tests and creates a standing conflict every time the file changes upstream. So this PR leaves load_web_page.py exactly as it is on main and only adds protection to RestApiTool. The two paths use different HTTP stacks anyway (requests vs httpx).

What gets blocked

is_blocked_address uses ipaddress.is_global, so one rule covers IPv4 and IPv6 without hand-maintained ranges:

  • Loopback (127.0.0.0/8, ::1)
  • Link-local (169.254.0.0/16, fe80::/10), which 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://, and the rest

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 pins to the pre-resolved IP:

  • validate_url rejects 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_async rewrites the URL to the validated IP literal, sets Host to the original hostname for routing, and sets sni_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 main the 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.

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

test_ssrf_protection.py covers hostname blocking, address blocking across IPv4/IPv6 classes, host resolution, validate_url for every blocked class plus the DNS-rebinding case, URL rewriting, and send_pinned_async pinning behaviour. test_rest_api_tool.py adds five tests that block localhost, loopback, the metadata endpoint, a private IP, and the file:// scheme through _request.

Note for reviewers: the SSRF tests use 8.8.8.8 / 8.8.4.4 as the stand-in public addresses. Recent CPython reclassifies the RFC 5737 documentation ranges (203.0.113.0/24 and friends) as is_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

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

adk-bot commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

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

adk-bot commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

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

adk-bot commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

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

adk-bot commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

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.

@rohityan rohityan self-assigned this Jun 9, 2026
@rohityan rohityan added the request clarification [Status] The maintainer need clarification or more information from the author label Jun 9, 2026
@rohityan

rohityan commented Jun 9, 2026 •

Copy link
Copy Markdown
Collaborator

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.
@Neelagiri65

Copy link
Copy Markdown
Author

Thanks @rohityan. Rebased onto current main and sorted the failing checks.

  • Merge conflict was in load_web_page.py. Main grew its own inline SSRF implementation there with tests that patch module-local names, so the earlier migration of that file no longer fits. I reverted load_web_page.py to match main exactly and scoped this PR to the RestApiTool fix plus the shared _ssrf_protection module. No behaviour change to load_web_page.
  • The unit test failures came from the SSRF tests using RFC 5737 documentation IPs (203.0.113.x) as stand-in public addresses. Recent CPython reports those as is_global=False, so the production check was correctly blocking them. Switched the tests to real globally-routable literals. All mocked, no traffic leaves the process.
  • Ran pre-commit (pyink, isort, ruff, addlicense, compliance) and the tool unit tests locally. Green.

Happy to adjust if you'd prefer the shared module folded directly into rest_api_tool.py instead of a separate file.

copybara-service Bot pushed a commit that referenced this pull request Oct 19, 2026
`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
@adk-bot

adk-bot commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

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.

@adk-bot adk-bot added the merged [Status] This PR is merged label Oct 19, 2026
@adk-bot adk-bot closed this Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged [Status] This PR is merged request clarification [Status] The maintainer need clarification or more information from the author tools [Component] This issue is related to tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants