Skip to content

fix: enforce manifest fetch policy at connect time - #19

Open
FllipEis wants to merge 2 commits into
manifest-ssrffrom
fix/manifest-fetch-hardening
Open

FllipEis wants to merge 2 commits into
manifest-ssrffrom
fix/manifest-fetch-hardening

Conversation

@FllipEis

@FllipEis FllipEis commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #18, before manifest-ssrf is merged into main. Claude Opus 5.5 and GPT-6.1-Sol each reviewed #18 separately and reached the same conclusions; this PR applies their findings.

Problems in #18

  • Tests: the 2 existing ManifestServerUrlResolverTest tests failed (their mock server uses http://127.0.0.1). The 4 tests listed in the PR description were never added.
  • Startup: the resolver resolved DNS in its constructor, so CloudApi construction failed whenever DNS was unavailable, even if inline blueprints were never used.
  • Address check: DNS was resolved in a separate lookup from OkHttp's actual connection, so a DNS answer could change between check and connect (DNS rebinding). The blocklist also missed IPv6 ULA (fc00::/7, e.g. AWS IMDS fd00:ec2::254), multicast, 0.0.0.0/8 and IPv4 embedded in IPv6.
  • Size cap: BoundedReader counted characters ×2, so the effective cap was about 512 KiB of ASCII. A body of exactly the limit was rejected, and the overflow error came out as a JsonSyntaxException.
  • serverUrl checks: checking serverUrl and manifest download links in the SDK protects nothing, because the SDK only forwards them. It also blocks http and internal mirrors.

Changes

  • New PublicAddressDns (OkHttp Dns): rejects any hostname that resolves to a non-public address. The checked addresses are the ones OkHttp connects to, on every redirect hop. IP literals, which skip Dns, are checked explicitly.
  • Manifest URL: parsed lazily with HttpUrl; https is enforced on every hop. The client never follows redirects automatically, including clients injected in tests.
  • Redirects: resolved with HttpUrl.resolve and capped at 5.
  • Size cap: 1 MiB of real bytes via Okio source.request(MAX + 1). Works with or without Content-Length. JSON and fetch errors are now consistently IllegalStateException.
  • Reverted: the CloudApiOptions builder check (duplicated, and skipped for the env var) and the serverUrl/download-link checks in InlineBlueprintSupport.
  • Tests: ManifestServerUrlResolverTest now uses an HTTPS MockWebServer (new test deps mockwebserver and okhttp-tls). It covers https enforcement, private literals and private DNS answers, redirect following, redirect-to-http, redirect-to-metadata IP, the redirect cap, the size limit (exact, over, chunked) and malformed JSON. A new PublicAddressDnsTest table-tests the address policy without network access.

Threat model / scope

This PR only guards the one request the SDK makes itself: fetching server_versions.json. Download links (explicit serverUrl or from the manifest) are forwarded unchanged, because the serverhost downloads them. That is where scheme, address and redirect checks have to run; that work is tracked as a separate PR in platform-serverhost.

Known limitation (documented on CloudApiOptions.Builder#serverVersionManifestUrl): if a JVM-wide HTTP proxy is configured, the proxy resolves the target host, so address filtering applies to the proxy host instead. A proxy reached through a private hostname will therefore be refused.

Second review round

Two fresh reviewers (Claude Opus 5.5 and GPT-6.1-Sol) with no prior context reviewed the first commit. Fixed in c71b889:

  • Mapped-address bypass: IPv4-mapped/translated IPv6 resolver answers (e.g. an AAAA record ::ffff:127.0.0.1) slipped past the check, because the JDK only converts mapped literals to IPv4. Reproduced end to end and fixed. Local-use NAT64 64:ff9b:1::/48 is now rejected too.
  • Ranges: 192.0.0.0/24 and the TEST-NET ranges are blocked again.
  • Numeric hosts: ambiguous numeric hosts (127.1, 2130706433, 1.2.3.4.5) are rejected instead of reaching system DNS outside Dns.
  • Deadline: the whole fetch, across redirects, is bounded by a 30 s deadline, so a slow server can't hold the manifest lock indefinitely.
  • Error messages: userinfo and query strings are stripped from URLs; a missing Location header is reported separately from an invalid one.
  • Tests: https tests now assert the failure reason (they previously passed because of a TLS handshake failure). New tests cover mapped resolver answers, the deadline, redaction and the production client's Dns wiring.

Tests

./gradlew :api:check :api:javadoc: 140 tests, 0 failures.

🤖 Generated with Claude Code

FllipEis and others added 2 commits October 9, 2026 12:55
Rework the SSRF hardening from #18 so it guards the one request the SDK
actually makes, without breaking startup or legitimate blueprint URLs.

- Filter resolved addresses in an OkHttp Dns (PublicAddressDns) so the
  checked address is the one connected to, for every redirect hop. Also
  covers IPv6 ULA, multicast, 0.0.0.0/8, 198.18/15, 240/4 and IPv4
  embedded in IPv6; IP literals are checked explicitly.
- Validate the manifest URL lazily instead of resolving DNS while the
  SDK is constructed.
- Follow redirects with HttpUrl.resolve, re-checking https on each hop,
  capped at 5.
- Cap the manifest at 1 MiB of real bytes via Okio instead of the
  char-counting BoundedReader, and wrap JSON errors consistently.
- Drop the SDK-side serverUrl/download-link checks and the duplicated
  builder check: the serverhost performs the download and must enforce
  the policy there.
- Replace the plain-http test server with an HTTPS MockWebServer and
  add the redirect, size and address policy tests #18 described.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Check IPv4 embedded in IPv4-mapped (::ffff:0:0/96) and translated
  (::ffff:0:0:0/96) resolver answers; the JDK only converts mapped
  literals, so AAAA answers like ::ffff:127.0.0.1 bypassed the policy.
  Reject local-use NAT64 64:ff9b:1::/48.
- Block 192.0.0.0/24 and the TEST-NET ranges again.
- Reject ambiguous numeric hosts (127.1, 1.2.3.4.5) that OkHttp would
  hand to InetAddress.getByName and system DNS, bypassing Dns.
- Bound the whole fetch, across redirects, by a 30s deadline so a slow
  server can't hold the manifest lock indefinitely.
- Strip userinfo and query from URLs in error messages; distinguish a
  missing Location header from an invalid one.
- Tests: assert why https enforcement fails rather than relying on a
  TLS handshake failure, cover resolver-form IPv4-mapped answers, the
  deadline, redaction, and the production client's Dns wiring.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant