Skip to content

start-tunnel: re-keying port forwards silently drops a colliding DNAT forward and can break the non-overlap invariant #3579

Description

@helix-nine

Found while investigating #3565. Independent of that issue's topology — reachable on any StartTunnel install — and it should be fixed before anything that makes two forwards' external IPs converge.

resync_forward_keys drops a DNAT forward on key collision, silently

TunnelContext::resync_forward_keys (shared-libs/crates/start-core/src/tunnel/context.rs:612-679) rebuilds the forward map by recomputing each entry's external IP from its target's WAN. The DNAT branch inserts without looking at what it displaced:

// context.rs:625-638
let ip = crate::tunnel::forward::igd::external_ipv4(self, *target.ip())
    .await
    .unwrap_or(*src.ip());
want.insert(
    SocketAddrV4::new(ip, src.port()),
    PortForward::Dnat { ... },
);

Two DNAT forwards on distinct external IPs but the same port — 1.2.3.4:443 and 5.6.7.8:443 — resolve to one key, and BTreeMap::insert returns the evicted entry into the void. The forward is then deleted from the DB and its nft rules GC'd as no-longer-wanted (context.rs:759-765, :782). The user is not told.

The SNI branch immediately below does handle its analogous case, warning on dropping SNI route {host} on {key} / dropping SNI fallback on {key} (context.rs:652-656, :672-676) — so the DNAT branch is the outlier, not the intent.

Reachable today: pin subnet A's WAN to IP1 and subnet B's to IP2, create a port-443 forward on a device in each, then clear B's pin. B's forward re-keys onto default_wan = IP1 and evicts A's.

The same loop can produce overlapping spans, which breaks the lookup helpers

want is built with no overlap check, but PortForwards::overlapping and PortForwards::occupied (shared-libs/crates/start-core/src/tunnel/db.rs:347-383) are written against the invariant their own comments state — "Forwards on one external IP never overlap, so only the nearest entry starting at or before new_hi ... can reach into [new_lo, new_hi]" — and each inspects exactly one candidate before stopping.

So collapse a count = 10 range at A:8000 onto the same IP as a single forward at B:8005 and the map holds genuinely overlapping spans. After that, overlapping() can return None for a real conflict, so add_forward's guard (tunnel/api.rs:1311-1319) passes and colliding DNAT rules get installed; occupied() can likewise miss a forward covering port 80, which is what keeps the HTTP redirect mutually exclusive with a port-80 forward.

Suggested shape

  • DNAT branch: warn and keep the incumbent, mirroring the SNI branch, rather than evicting silently.
  • Refuse to insert an entry whose span overlaps one already in want, so the invariant db.rs documents actually holds by construction.

Both are prerequisites for any change to how external IPs are resolved — including the ones under discussion in #3565, which all increase the number of forwards that can land on the same address.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions