fix(serverless): block SSRF in job-input downloads#533
Conversation
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Promptless prepared a documentation update related to this change. Triggered by PR #533 (block SSRF in job-input downloads) Adds a new docs page for the Serverless SDK download helper ( Review: Document SSRF safeguards and env vars for Serverless job-input downloads |
There was a problem hiding this comment.
Pull request overview
This PR hardens serverless job-input URL downloads against SSRF by introducing a centralized “safe fetch” path and routing existing download utilities through it, preventing access to link-local/loopback/private/reserved destinations (notably instance metadata at 169.254.169.254) while also enforcing download size limits.
Changes:
- Added
runpod/serverless/utils/rp_ssrf.pyproviding scheme allowlisting, DNS resolution + global-routability validation, DNS-rebind-resistant pinned-IP dialing, redirect re-validation, and download size caps. - Updated
runpod/serverless/utils/rp_download.pyto usesafe_get()and to stream responses to disk withiter_content_capped()rather than buffering full bodies in memory. - Added SSRF-focused test coverage and updated download tests to mock
safe_get()and make concurrent assertions order-independent.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
runpod/serverless/utils/rp_ssrf.py |
New SSRF-safe fetch implementation (validation, pinning, redirects, size caps). |
runpod/serverless/utils/rp_download.py |
Routes downloads through safe_get() and streams to disk with size enforcement. |
tests/test_serverless/test_utils/test_ssrf.py |
New unit tests for address validation, redirects, size caps, and pinned-IP behavior. |
tests/test_serverless/test_utils/test_download.py |
Updates tests to mock safe_get() and handle concurrent call ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address PR #533 review feedback. - Tie each single-use SyncClientSession's lifecycle to its response via _bind_session_to_response, so closing the response (with-block or explicit) also closes the session; without it the session's pools/FDs leaked once per download in long-lived workers. - Close the session on redirect hops and on request exceptions; make teardown best-effort so a cleanup failure cannot mask the caller's real error. - Close response and session deterministically in the pinned-IP adapter test. - Add tests for the request-exception path and the error-masking guarantee.
Job input carries arbitrary URLs that the worker downloads. Without restriction a job can point the worker at cloud instance-metadata (169.254.169.254) or other non-public addresses (CWE-918). Add runpod/serverless/utils/rp_ssrf.py: an http(s) scheme allowlist, DNS resolution that fails closed on any non-global IP (link-local, loopback, RFC1918, CGNAT, reserved, multicast, IPv6 ULA), a connection adapter that pins the socket to the validated IP to defeat DNS rebinding, per-hop redirect re-validation, and a streamed size cap. Route both rp_download fetch sites through it, and stream file() to disk instead of buffering the whole untrusted body in memory. Blocks raise SSRFError (a ValueError, not a RequestException) so they fail loudly rather than being retried or silently dropped. Configurable via RUNPOD_ALLOW_PRIVATE_DOWNLOAD_URLS (escape hatch, off by default) and RUNPOD_MAX_DOWNLOAD_BYTES (default 5 GiB).
Address PR #533 review feedback. - Tie each single-use SyncClientSession's lifecycle to its response via _bind_session_to_response, so closing the response (with-block or explicit) also closes the session; without it the session's pools/FDs leaked once per download in long-lived workers. - Close the session on redirect hops and on request exceptions; make teardown best-effort so a cleanup failure cannot mask the caller's real error. - Close response and session deterministically in the pinned-IP adapter test. - Add tests for the request-exception path and the error-masking guarantee.
deanq
force-pushed
the
deanq/sls-375-ssrf-download-guard
branch
from
64596af to
6d4098b
Compare
Summary
Serverless job input carries arbitrary user-supplied URLs that the SDK downloads with no restriction on scheme or destination address. A job can therefore make the worker fetch cloud instance-metadata (
http://169.254.169.254/...) or other non-public addresses — server-side request forgery (CWE-918). Affected sites:download_files_from_urlsandfile()inrunpod/serverless/utils/rp_download.py.The host network layer already blocks RFC1918 egress from worker pods, but not link-local (
169.254.0.0/16), so cloud-metadata credential theft is the live unmitigated vector.Changes
New
runpod/serverless/utils/rp_ssrf.py, routed through by both download sites:http/httpsonly.169.254.169.254, loopback, RFC1918, CGNAT100.64/10, reserved, multicast, IPv6 ULA/link-local, IPv4-mapped IPv6).PinnedIPAdapterdials the pre-validated IP while keeping TLS SNI, certificate verification, and theHostheader bound to the original hostname (never alters cert verification).Content-Lengthand aborts mid-stream past the cap;file()now streams to disk instead of buffering the whole body in memory.Blocks raise
SSRFError(aValueError, not aRequestException) so they fail loudly rather than being retried or silently returned as a failed download.Configuration (secure by default)
RUNPOD_ALLOW_PRIVATE_DOWNLOAD_URLS=true— escape hatch for same-VPC / self-hosted fetches (default off).RUNPOD_MAX_DOWNLOAD_BYTES— per-download byte cap (default 5 GiB).Public API signatures are unchanged; only non-public destinations and oversized downloads change behavior.
Test plan
tests/test_serverless/test_utils/test_ssrf.py: address classification (metadata/RFC1918/CGNAT/IPv6/mapped blocked, public allowed), resolve fail-closed on mixed public+private, scheme rejection, redirect-to-metadata blocked, too-many-redirects, size cap (header + mid-stream), escape hatch, and a real-socket test asserting the adapter dials the pinned IP while preserving theHostheader.test_download.pyto the new fetch path; made the concurrent-download assertion order-independent.ruff checkclean.safe_getagainst a literal metadata IP,file://, and an RFC1918 address all raiseSSRFError; a real public host validates.Follow-up (out of scope)
Adding
169.254.0.0/16(and IPv6) to the host daemon egress rules would close the metadata vector at the network layer as additional defense-in-depth — separate host-team ticket.