Skip to content

fix(proto): canonicalize remote IP in early_discard_packet's peer check (noq#738) - #783

Closed
cuzic wants to merge 5 commits into
n0-computer:mainfrom
cuzic:pr/noq-738-remote-canonicalize
Closed

cuzic wants to merge 5 commits into
n0-computer:mainfrom
cuzic:pr/noq-738-remote-canonicalize

Conversation

@cuzic

@cuzic cuzic commented Aug 5, 2026

Copy link
Copy Markdown

Closes #738.

Problem

4bae6edd (the hotfix posted in #738) canonicalizes the local_ip
comparison in Connection::early_discard_packet, to handle dual-stack
sockets that report a peer as an IPv4-mapped-IPv6 address
(::ffff:a.b.c.d) on one side of the comparison and a plain IPv4 address
on the other. We applied it and confirmed on a real Android device
(WiFi + cellular) that it is not sufficient on its ownopen_path()
with an explicit local_ip still gets abandoned with ValidationFailed.

The remote comparison a few lines above the one 4bae6edd touches
has the exact same problem, and is not canonicalized:

if network_path.remote != known_path.network_path.remote && !peer_may_probe {

An incoming datagram's reported remote SocketAddr can compare unequal
to the known path's remote purely due to mapped-vs-plain representation,
so early_discard_packet silently drops every packet on that path before
it ever reaches PATH_RESPONSE / frame processing — same failure mode as
the local_ip bug, just one comparison earlier in the same function.

Fix

Mirror the existing local_ip fix: canonicalize both sides of the
remote comparison with IpAddr::to_canonical() before comparing.

This is the minimal, surgical fix — one comparison site, matching the
style of 4bae6edd. See #784 for an alternative that canonicalizes
once at FourTuple::new() construction time instead, which also covers
a few other latent comparison sites (Endpoint's
HashMap<FourTuple, ConnectionHandle> routing table,
is_probably_same_path, PATH_CHALLENGE on-path detection, OBSERVED_ADDRESS
matching, peer migration detection) that weren't yet observed failing in
our testing but share the same root cause.

Testing

  • Includes the regression test from the noq-738 branch
    (noq-proto/src/tests/multipath.rs::open_path_with_explicit_local_ip),
    adapted to build its ManyToManyRouting via add_client_route/
    add_server_route instead of from_routesfrom_routes now rejects
    the duplicate server_addr this test intentionally uses (invariant
    added by feat(proto): use path idle timeout for validation when opening a new path #721 after the regression test was originally written).
    Fails on unpatched main, passes with this fix.
  • cargo test -p noq-proto: 388 passed, 0 failed (full suite, not just
    the new test).
  • Verified on a real Android device: Secondary (local_ip: None)
    established, then PhysicalWifi/PhysicalCellular (local_ip
    explicit) both validate on the first attempt instead of retrying
    3x and getting abandoned:
    opening path "physical-wifi" -> <redacted>:45823 (local_ip=Some(192.168.10.80), attempt 1/3)
    path "physical-wifi" established: id=PathId(2)
    opening path "physical-cellular" -> <redacted>:45823 (local_ip=Some(10.209.235.90), attempt 1/3)
    path "physical-cellular" established: id=PathId(3)
    

divagant-martian and others added 5 commits July 30, 2026 22:31
…ck (noq#738)

The local_ip comparison a few lines below already canonicalizes both
sides via IpAddr::to_canonical() (4bae6ed) to handle dual-stack
sockets reporting IPv4-mapped-IPv6 addresses. The remote comparison
just above it does not, and can trip the same class of bug: an
incoming datagram's reported remote SocketAddr (e.g. [::ffff:a.b.c.d]:p)
compares unequal to the known path's remote (a.b.c.d:p) purely due to
representation, causing early_discard_packet to silently drop every
packet on that path before it ever reaches PATH_RESPONSE / frame
processing.
@n0bot n0bot Bot added this to iroh Aug 5, 2026
@github-project-automation github-project-automation Bot moved this to 🚑 Needs Triage in iroh Aug 5, 2026
cuzic added a commit to cuzic/noq that referenced this pull request Aug 8, 2026
`4bae6edd` (the hotfix posted in n0-computer#738) and n0-computer#783 both patch individual
`==`/`!=` comparison sites in `Connection::early_discard_packet` to
canonicalize IPv4-mapped-IPv6 addresses (`::ffff:a.b.c.d`) before
comparing, since dual-stack sockets can report the same peer in either
form depending on the code path. This closes the same class of bug at
every site where `FourTuple`'s `PartialEq`/`Hash` or a raw `remote`
comparison is used, not just the two `early_discard_packet` sites.

An earlier version of this branch canonicalized `remote`/`local_ip`
inside `FourTuple::new()`, mutating the stored/emitted address so every
downstream comparison would see one canonical form automatically. That
had two real problems, both found by running the existing test suite:

- It changes the address family of what gets handed to the OS for
  sending (`Transmit::destination` becomes plain IPv4 where it used to
  be IPv4-mapped IPv6 on dual-stack sockets). This crate's CI is
  Linux-only, so whether that's safe on Windows/macOS was unverified.
  It also broke the `noq` crate's `normalize_network_path` IPv6
  autodetection, silently narrowing genuine mixed v4/v6 multipath.
- It broke the test harness's own simulated network routing (21
  proptest regressions), because `tests/util.rs` compares raw
  `SocketAddr`s captured before canonicalization against ones captured
  after.

This version instead canonicalizes only for comparison/hashing, never
for storage: `FourTuple::new()` is unchanged from `main`, and
`PartialEq`/`Hash` are hand-written to canonicalize via a `remote_key()`
helper — `(ip.to_canonical(), port, scope_id)`, keeping `scope_id` for
addresses that stay IPv6 (a mapped address canonicalizes to plain IPv4
and has no scope). Dropping `scope_id` unconditionally was tried first
and collapsed two genuinely different link-local interfaces into one
path for equality/hashing — the same class of bug this fix is meant to
prevent, just for a different field. Covered by a new regression test.

`is_probably_same_path` and the other raw `remote == remote`
comparisons that don't go through `FourTuple`'s whole-struct equality
(`early_discard_packet`, PATH_CHALLENGE on-path detection,
OBSERVED_ADDRESS matching, the peer-migration trigger, and
`PathResponses::push`'s dedup) now all route through a shared
`same_remote()` helper instead of open-coding the comparison.

Includes and builds on the regression test from the `noq-738` branch
(`noq-proto/src/tests/multipath.rs::open_path_with_explicit_local_ip`),
adapted to build its `ManyToManyRouting` via `add_client_route`/
`add_server_route` instead of `from_routes` (which now rejects the
duplicate `server_addr` this test intentionally uses, an invariant
added by n0-computer#721 after the test was originally written), and to construct
its `FourTuple` via `FourTuple::new()` rather than a struct literal.

Testing:
- Two new unit tests in `noq-proto/src/lib.rs` (`four_tuple_tests`):
  `four_tuple_eq_ignores_mapped_v4_representation` (the n0-computer#738 case) and
  `four_tuple_eq_preserves_link_local_scope_id` (regression test for
  the scope_id issue found during review).
- `cargo test -p noq-proto`: 390 passed, 0 failed.
- `cargo test -p noq --lib`: 32 passed, 3 ignored, 0 failed, including
  `echo_dualstack` (which the construction-time version of this fix
  broke).
- Verified on a real Android device (WiFi + cellular): `Secondary`
  established, then `PhysicalWifi`/`PhysicalCellular` both validate on
  the first attempt instead of retrying 3x and getting abandoned.

Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
@cuzic

cuzic commented Aug 9, 2026

Copy link
Copy Markdown
Author

Closing in favor of #784, which now also covers the additional comparison sites beyond early_discard_packet (is_probably_same_path, PathResponses::push, OBSERVED_ADDRESS matching, peer-migration detection) — see the comparison writeup on the issue.

#784 has been refactored per @divagant-martian's review: the comparison logic now lives as FourTuple::same_remote(&self, other: &Self) / FourTuple::same_local_ip(&self, other: &Self) methods instead of free-standing functions.

@cuzic cuzic closed this Aug 9, 2026
@github-project-automation github-project-automation Bot moved this from 🚑 Needs Triage to ✅ Done in iroh Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

open_path() with explicit local_ip: PATH_RESPONSE never reaches on_path_response_received, path stuck ValidationFailed

3 participants