Skip to content

fix(start-os): align PCP and UPnP port-check behavior behind a second NAT - #3840

Merged
Dominion5254 merged 4 commits into
masterfrom
os/port-check-probes-reachability
Sep 2, 2026
Merged

fix(start-os): align PCP and UPnP port-check behavior behind a second NAT#3840
Dominion5254 merged 4 commits into
masterfrom
os/port-check-probes-reachability

Conversation

@Dominion5254

@Dominion5254 Dominion5254 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

(Replaces the previous description, which described a wider rework: always-probe, mapping re-assertion, an RPC/sync-task ordering wait, and an Option<bool> tri-state. Per review — "we want to limit use of external echoip services where possible; sounds like we just need to fix the mismatch between upnp and pcp behavior when it comes to double-nat" — this is now the mismatch alone.)

What was wrong

check_port short-circuits to "open" whenever the port-map client holds a mapping for the port, and the two protocols disagreed about what the address in that record had to be:

  • PCP reported whatever external address the gateway returned. Behind a second NAT that address is private, so the check passed a port nothing outside could reach.
  • UPnP discarded a non-routable address at record time and fell through to the echo probe, so the identical topology answered red.

Same reachability, opposite verdicts by protocol — the issue as filed.

What this does

The shard's Command::ExternalIp handler (port_map/client.rs) is where both protocols' addresses converge. The routability filter moves there, so it applies to whichever protocol reported the address. A gateway with no public address of its own falls through to the probe on both protocols, which is what UPnP already did.

v6 passes through untouched — the GUA is the box's own address, and check_gua_port only tests that a pinhole exists, never reading its address.

gateway.rs and upnp.rs are untouched; is_wan_candidate is the filter UPnP already used, applied unchanged.

On echoip usage

No new echoip traffic beyond what a UPnP gateway already generates on this topology. The fall-through is the path UPnP has always taken; PCP now joins it only where the gateway's own reported address rules out a direct answer. The only alternative that would cut echoip further is answering false outright on a non-routable address — but that is wrong when the outer router carries a manual forward, and it would push a user whose port already works into the manual-forward modal.

Verified

  • cargo test -p start-core --features=test port_map: 80 passed. cargo fmt --check: clean.
  • Command::ExternalIp's body was inline in the shard's select loop and unreachable from a test; it is extracted as external_ip_of, with both protocol arms funnelling through one routable_external_ip. New tests cover the UPnP arm, key selection (right port, TCP only), a v6 pinhole passing through unfiltered, an addressless NAT-PMP grant, and the filter over private and public v4.
  • The PCP arm is not constructible in a test — crab_nat::PortMapping has private fields and no public constructor — so it reaches the filter by construction: external_ip_of is the single call site and wraps both arms.
  • Bench (double NAT, PCP): StartOS on the inner LAN of a router whose WAN holds an address from the outer router's LAN. With a PCP grant held for the port (confirmed on the router side), check_port reports the port not open. That is the premise of Port Forwarding check flips pass/fail behind double NAT depending on the granting protocol — PCP fast path trusts a private external IP, UPnP-held mappings discard it and probe the true public IP (net/gateway.rs:714) #3784 confirmed — a real PCP gateway behind a second NAT does report a private external address — and the verdict this changes. Master's green on the same setup is by inspection of the unfiltered short-circuit, not an A/B run.
  • The single-NAT accept path was not bench-run, and cannot be on that hardware: the inner router always reports a private address, and the outer router speaks neither PCP nor UPnP, so no gateway there reports a routable one. It rests on the unit tests, which is a short reach — check_port is byte-identical to master and the only difference is one is_wan_candidate call on the reported address. a_routable_address_is_reported_as_given pins that public addresses pass, and a_upnp_mapping_answers_only_for_its_own_tcp_port exercises it end to end through external_ip_of — the function check_port calls.
  • The UPnP arm was not bench-run (no IGD on that router) and cannot regress: master already filtered at record time (upnp.rs:109), and this adds a second, idempotent filter at read time.

Not in this PR

Real but separate; each wants its own issue:

Fixes #3784

https://claude.ai/code/session_01RwXsV4feB6gSVXTedUrzxT

@dr-bonez dr-bonez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong direction. we want to limit use of external echoip services where possible. sounds like we just need to fix the mismatch between upnp and pcp behavior when it comes to double-nat

@Dominion5254
Dominion5254 marked this pull request as draft September 1, 2026 19:13
…rotocols

Fixes #3784. `check_port` short-circuited to "open" on the port-map
client's record of its last grant, and the two protocols disagreed about
what the address in that record had to be. PCP reported whatever the
gateway returned, so behind a second NAT a private address passed the
check and nothing outside could connect. UPnP discarded a non-routable
address at record time and fell through to the echo probe, so the same
topology answered red. Same reachability, opposite verdicts by protocol.

The shard's `ExternalIp` handler is where both protocols' addresses
converge, so the filter moves there and applies to whichever protocol
reported it. v6 passes through untouched: the GUA is the box's own
address and `check_gua_port` never reads the mapping's.

No new echoip traffic beyond what a UPnP gateway already generates on
this topology: the fall-through is the path UPnP has always taken, and
PCP now joins it only where the gateway's own address rules out a direct
answer.

Claude-Session: https://claude.ai/code/session_01RwXsV4feB6gSVXTedUrzxT
`Command::ExternalIp`'s body was inline in the shard's select loop, so
nothing could reach it without spawning a shard against real interfaces.
Extract it as `external_ip_of`, with the per-protocol match funnelling
through one `routable_external_ip`.

The UPnP arm and the key selection (right port, TCP only) are covered
directly. The PCP arm is not constructible here — `crab_nat::PortMapping`
has private fields and no public constructor — so it reaches the filter
by construction: `external_ip_of` is the one call site and it wraps both
arms. A live check is what proves a real PCP gateway behind a second NAT
reports the private address this rejects.

Claude-Session: https://claude.ai/code/session_018ZmVQMf744jaqpKUm5gthK
@Dominion5254
Dominion5254 force-pushed the os/port-check-probes-reachability branch from 4a2a1dc to 8178ae7 Compare September 1, 2026 20:05
@Dominion5254 Dominion5254 changed the title fix(start-os): port-forwarding test probes reachability instead of trusting a mapping fix(start-os): align PCP and UPnB port-check behavior behind a second NAT Sep 1, 2026
@Dominion5254 Dominion5254 changed the title fix(start-os): align PCP and UPnB port-check behavior behind a second NAT fix(start-os): align PCP and UPnP port-check behavior behind a second NAT Sep 1, 2026
dr-bonez
dr-bonez previously approved these changes Sep 2, 2026
@Dominion5254
Dominion5254 merged commit df7e7db into master Sep 2, 2026
25 checks passed
@Dominion5254
Dominion5254 deleted the os/port-check-probes-reachability branch September 2, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants