Commit e4444d5
authored
refactor(ext/fetch): check net deny list against connected peer addr (#34418)
Follow-up to #34236, addressing
#34236 (comment).
#34236 introduced a `REQUEST_PORT` `tokio::task_local!` to smuggle the
real destination port from `PermissionedHttpConnector` (which sees the
`Uri`) into the DNS resolver (which is only handed a `Name` by
hyper-util's `HttpConnector`). The DNS resolver then ran the
post-resolution net-deny check against every resolved address with that
port. This was load-bearing but pretty ugly: it relied on the task-local
being read before each `tokio::spawn` inside the resolver and scoped
around the inner connector's future.
Instead, run both the resolution and the check in
`PermissionedHttpConnector` itself, which sees the full `Uri` (and
therefore the real port). The connector now owns the `Resolver` and the
connector config (local address) rather than a pre-built
`HttpConnector`:
- For hostnames, it resolves via the `Resolver`, checks every resolved
address with `check_net_resolved` before any socket is opened
(matching what `Deno.connect` does in `ext/net/ops.rs`), and then
hands the vetted addresses to a hyper-util `HttpConnector` through a
pre-resolved resolver. The connection goes to exactly the addresses
that were checked, so no second DNS query can race with a record
change.
- For IP-literal hosts, the literal is checked directly and resolution
is skipped (hyper-util connects to literals without consulting the
resolver).
- Resolution errors are wrapped in a `DnsError` type that mirrors
hyper-util's `ConnectError` display ("dns error" with the `io::Error`
as source), so fetch error messages keep their existing
`client error (Connect): dns error: ...` format.
This removes the `REQUEST_PORT` task-local, the `permissions` field on
`Resolver`, `Resolver::with_permissions`, and the in-resolver
`check_resolved_permissions` helper. Callers pass
`permissions: Option<PermissionsContainer>` via
`CreateHttpClientOptions`, and `ext/fetch` and `ext/websocket` set it
there.
No behavior change for the existing `tests/specs/permission/deny_net_*`
regression tests.
## Follow-up: proxied request destinations
A request routed through a proxy only opens a socket to the proxy, so
the post-resolution deny check on the connector previously only saw the
proxy address. An IP-level `--deny-net` rule on the destination could
therefore be bypassed by routing through an allowed proxy.
`ProxyConnector` now runs a best-effort net-deny check against the real
destination before connecting to the proxy: it resolves the destination
host (via the new `CheckDst` trait on `PermissionedHttpConnector`) and
checks every resolved address. It is best-effort because a proxy may be
able to reach a host this process cannot resolve locally, so a local
resolution failure is not fatal; the connection to the proxy itself is
still checked separately. Covered by a new `ext/fetch` test that denies
a proxied destination whose hostname resolves to a denied IP.1 parent a249752 commit e4444d5
10 files changed
Lines changed: 359 additions & 111 deletions
File tree
- ext
- fetch
- kv
- websocket
- tests/specs/permission/deny_net_fetch_resolved_ip
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
54 | 54 | | |
55 | 55 | | |
56 | 56 | | |
| 57 | + | |
57 | 58 | | |
58 | 59 | | |
59 | 60 | | |
| |||
0 commit comments