Repository navigation
feat(upnp): map ports through double NAT - #6074
EnriqueWood wants to merge 6 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Signed-off-by: Enrique Wood <ewoodg@hotmail.com>
Signed-off-by: Enrique Wood <ewoodg@hotmail.com>
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
One correctness issue in the new mapping-reuse path: disabled manual mappings can be reported as successful without opening the port. Please preserve those rules but report the mapping as unavailable, and cover the disabled case in regression tests.
Reviewed head a13d24b4ca452944e4301d3bcca7d2a0b73e6e1e; GitHub currently reports no merge conflict. This is source-level verification, not a real-router reproduction or a full application/test build. CI, CodeQL and common lint for this head await maintainer approval; those missing results are a verification limit, not the reason for requesting changes. The PR's real-router results predate the ownership follow-up.
Review completed by Codex | Model: unavailable | Thinking: unavailable
| // A manual rule already does what we need. Never renew or remove it. | ||
| return true; |
There was a problem hiding this comment.
[P2] Do not count a disabled manual mapping as successful
UPNP_GetSpecificPortMappingEntry fills enabled, but this new early return ignores it. If a router returns a matching destination/internal port, a non-Sunshine description, enabled = "0", and a nonzero lease, this branch returns true without enabling the mapping. The mapping pass can therefore log completion while the port remains closed. This extends the problem beyond permanent rules: the target branch previously reached UPNP_AddPortMapping for nonzero leases instead of returning early.
Preserving a manual rule is appropriate, but a disabled one should be reported as unavailable rather than reused successfully. A minimal conservative correction for this branch is:
| // A manual rule already does what we need. Never renew or remove it. | |
| return true; | |
| // Never renew or remove a manual rule, but do not count a disabled rule as usable. | |
| if (std::string_view {enabled.data()} == "0"sv) { | |
| BOOST_LOG(warning) << "UPnP port "sv << mapping.port.wan << " has a disabled manual mapping; leaving it unchanged"sv; | |
| return false; | |
| } | |
| return true; |
This suggestion has not been compiled or hardware-tested. Please add a production-path regression with a successful lookup returning the matching target/port, a manual description, enabled = "0", and lease "3600": expect false, no add/delete calls, and an incomplete mapping pass. Keep an enabled manual-rule control case returning true. Disabled Sunshine-owned permanent rules also deserve coverage, although that early-success behavior predates this PR.
Review completed by Codex | Model: unavailable | Thinking: unavailable
There was a problem hiding this comment.
Thanks, good catch. Fixed in 3a682f0.
A rule from someone else that forwards to the right place but is disabled is now left alone and reported as not open (UPnP port N has a disabled manual mapping; leaving it unchanged), so the pass stays incomplete. An enabled one is still used without renewing it. enabled counts as disabled for 0, false or no in any case, and an empty value still counts as enabled. A disabled Sunshine rule without expiry isn't taken as open either: it goes through the add path again, so it gets enabled again (with the 725 fallback if the router needs it).
To test the real path without a router, I moved the per-port logic out of deinit_t into upnp::map_port/upnp::map_ports in upnp_mapping.h. They take the gateway calls through a small mapping_gateway concept: Sunshine passes a MiniUPnPc wrapper and the tests pass a fake that records every call. New tests:
- disabled manual rule (manual description,
enabled = "0", lease"3600"):false, no add or delete calls, nothing recorded as owned - enabled manual rule:
true, no add or delete calls - disabled manual rule next to a missing one: pass incomplete, the other port is still added
- disabled Sunshine rule without expiry: added again (
3600, then0after 725), still recorded as permanent - enabled Sunshine rule without expiry: kept, no add calls
0/false/False/NOcount as disabled,1/true/YES/empty don't
The same commit passes the IPs and URLs in test_upnp_mapping.cpp as helper arguments, the way test_upnp.cpp does, for the Sonar findings on that file.
Since my real-router results were from before the ownership changes, I ran the branch again on my double NAT (TP-Link Archer behind the ISP router) and found one more problem, fixed in 3cf711d. The ISP router answers GetSpecificPortMappingEntry with NewInternalPort = 0 for every rule. With a13d24b the first pass mapped both routers, but from the first refresh on all 6 rules on the ISP router were logged as already forwards elsewhere, and on exit they were left behind. Now a port reported as 0 or empty isn't taken as another destination. With 3cf711d on the same network:
- both routers got their 6 rules, and
/serverinfoanswered through the public address from another ISP - on refresh there were no conflicts; the ISP router answers 718 when an existing rule is added again, same as before this PR
- an enabled rule I added by hand on each router, with another description, was used and left untouched, also after exit
- on exit, all of Sunshine's rules were removed from both routers
- I couldn't try a disabled rule on real hardware: both routers store
NewEnabled=0as enabled, so that case is only covered by the tests
Signed-off-by: Enrique Wood <ewoodg@hotmail.com>
Signed-off-by: Enrique Wood <ewoodg@hotmail.com>
|
| * @return Rule on the inner gateway. | ||
| */ | ||
| upnp::owned_mapping_t sample_mapping() { | ||
| return make_rule("http://192.168.1.1/control", "192.168.1.20"); |
| * @return Rule on the inner gateway. | ||
| */ | ||
| upnp::owned_mapping_t sample_mapping() { | ||
| return make_rule("http://192.168.1.1/control", "192.168.1.20"); |
| * @return Rule on the inner gateway. | ||
| */ | ||
| upnp::owned_mapping_t sample_mapping() { | ||
| return make_rule("http://192.168.1.1/control", "192.168.1.20"); |
|
|
||
| TEST(UpnpMappingJournalTest, KeepsBothGatewaysAndOldDestinationsUntilRemoved) { | ||
| const auto inner = sample_mapping(); | ||
| const auto outer = make_rule("http://10.0.0.1/control", "10.0.0.42"); |
|
|
||
| TEST(UpnpMappingJournalTest, KeepsBothGatewaysAndOldDestinationsUntilRemoved) { | ||
| const auto inner = sample_mapping(); | ||
| const auto outer = make_rule("http://10.0.0.1/control", "10.0.0.42"); |
| } | ||
|
|
||
| TEST(UpnpMappingJournalTest, PreservesQuotedDescriptionsAndEndpointCharacters) { | ||
| auto mapping = make_rule("http://192.168.1.1/control?value=\\quoted", "192.168.1.20"); |
| } | ||
|
|
||
| TEST(UpnpMappingJournalTest, PreservesQuotedDescriptionsAndEndpointCharacters) { | ||
| auto mapping = make_rule("http://192.168.1.1/control?value=\\quoted", "192.168.1.20"); |
|
|
||
| TEST(UpnpMapPortTest, DisabledManualRuleDoesNotStopTheOtherPorts) { | ||
| const auto manual = sample_mapping(); | ||
| const auto other = make_rule("http://192.168.1.1/control", "192.168.1.20", "47984"); |
|
|
||
| TEST(UpnpMapPortTest, DisabledManualRuleDoesNotStopTheOtherPorts) { | ||
| const auto manual = sample_mapping(); | ||
| const auto other = make_rule("http://192.168.1.1/control", "192.168.1.20", "47984"); |
|
|
||
| TEST(UpnpMapPortTest, DisabledManualRuleDoesNotStopTheOtherPorts) { | ||
| const auto manual = sample_mapping(); | ||
| const auto other = make_rule("http://192.168.1.1/control", "192.168.1.20", "47984"); |
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
The disabled-manual-rule fix is present and the extracted production-path checks now reject disabled manual rules without changing them. The new internal-port workaround introduces a separate ownership problem: an unreported port cannot establish that a rule forwards to the requested port. Please keep that uncertainty distinct from a verified match.
Reviewed 3cf711da; GitHub reports it mergeable. Focused C++ checks against the actual header passed, including ASan/UBSan with leak detection disabled because the runner cannot support it. These reproduce the unknown-port false-reuse/classification behavior, not a real-router failure. I have not run the full application or native test suite. CI, common lint and CodeQL await maintainer approval.
Review completed by Codex | Model: unavailable | Thinking: unavailable
| // Some gateways report the internal port of an existing rule as 0 or leave it empty; that is not another destination. | ||
| const bool port_reported = !port.empty() && port != "0"; | ||
| if (client != wanted.target || (port_reported && port != wanted.internal_port)) { | ||
| return mapping_state_e::conflict; | ||
| } | ||
| return description == wanted.description ? mapping_state_e::ours : mapping_state_e::existing; |
There was a problem hiding this comment.
[P2] Keep an unreported internal port distinct from a verified match
I understand the ISP router reports 0 for every internal port, but this wildcard also accepts rules Sunshine did not create. For example, an enabled manual rule 47989 -> same host:48000 on that router is now classified existing, so map_port returns reused and reports the requested 47989 -> host:47989 forwarding complete without changing anything. Matching the static Sunshine description instead yields ours; if a recorded rule was edited to another internal port while retaining its description, cleanup can delete that replacement.
I reproduced both classifications with the actual header. Missing information is not proof of a conflict, but it is not proof of a match either. A conservative guard is:
| // Some gateways report the internal port of an existing rule as 0 or leave it empty; that is not another destination. | |
| const bool port_reported = !port.empty() && port != "0"; | |
| if (client != wanted.target || (port_reported && port != wanted.internal_port)) { | |
| return mapping_state_e::conflict; | |
| } | |
| return description == wanted.description ? mapping_state_e::ours : mapping_state_e::existing; | |
| if (client != wanted.target) { | |
| return mapping_state_e::conflict; | |
| } | |
| if (port.empty() || port == "0") { | |
| return mapping_state_e::unknown; | |
| } | |
| if (port != wanted.internal_port) { | |
| return mapping_state_e::conflict; | |
| } | |
| return description == wanted.description ? mapping_state_e::ours : mapping_state_e::existing; |
This suggestion has not been router-tested and deliberately leaves ambiguous rules untouched. If support for this router needs a fallback, separate a verified rule/provenance path from this general classifier rather than using an unknown port as a wildcard for manual reuse and deletion. Add regressions for unknown-port manual rules (incomplete pass, no mutation) and recorded rules whose internal destination has changed but whose lookup hides that port (no deletion).
Review completed by Codex | Model: unavailable | Thinking: unavailable
Description
With
upnp = enabled, Sunshine now opens its ports behind two routers (double NAT): on the user's router, and on the ISP router in front of it.This is a very common setup. People put their own router (often a gaming router) behind the ISP's for what the ISP box lacks or locks down: QoS or SQM to keep game traffic ahead of bufferbloat, better Wi-Fi coverage or mesh, newer Wi-Fi (6/7, MU-MIMO), a VPN on the router, parental controls, DHCP reservations, or firmware such as OpenWrt. The ISP router often can't be put in bridge mode, and some ISPs don't allow replacing it at all. Sunshine already speaks UPnP, so completing it for double NAT lets these users stream over the internet without forwarding ports by hand, as long as both routers support UPnP. I also tried Tailscale: it often works, but sometimes the connection stays on DERP relays, and then streaming over the internet isn't stable enough to play.
What happens today. The ISP router usually answers discovery from the inner network too, and
UPNP_GetValidIGD()prefers it because it reports a public address. Sunshine then maps the ports there to its own LAN address, which that router can't reach, and logsCompleted UPnP port mappings, but nothing is reachable from the internet. The router Sunshine is actually attached to gets no mappings.What this changes (
src/upnp.cpp):UPNP_GetValidIGD(), Sunshine reads the description of the other IGDs that answered discovery and maps on the one whose address shares the most leading bits with its own address (the gateway of its network). On a tie it keeps the IGD chosen byUPNP_GetValidIGD(), so with a single router nothing changes.docs/configuration.mddescribes the double NAT behavior underupnp. There is no new option: the second hop is only used when the first router's external address is private and another IGD answers in its network.Security follow-up
Merge #6098 first: it fixes the published MiniUPnPc CVEs in the pinned Flatpak source and hardens the HTTP parser used by Sunshine. This PR does not change or roll back those dependency fixes; the patched MiniUPnPc covers both single- and double-NAT calls. Other system packages still require their distributor's MiniUPnPc backports.
The mapping lifecycle now applies the same ownership rules to both gateways:
Screenshot
Issues Fixed or Closed
Roadmap Issues
Testing
Unit tests (
tests/unit/test_upnp.cpp) for the new helpers:shared_prefix_length,url_host,select_inner_igd(single IGD, the ISP router chosen first, ties) andselect_outer_igd(outer router found, public or invalid external address, unrelated IGDs, closest IGD). Built on Fedora 44 (GCC 15.3, Release,-DBUILD_WERROR=ON, CUDA off);test_sunshine --gtest_filter='Upnp*:Network*': 19 tests passed.uv run --locked lb-update-clang-formatapplied.The discovery and mapping calls need a router, so they were checked by hand:
Fake IGDs in an isolated Docker network (a small Python IGD answering SSDP and the WANIPConnection SOAP actions, logging every add/delete). The IGD in front reports a public address, so
UPNP_GetValidIGD()picks it first, as with a real ISP router.In every case the mappings Sunshine added were removed on exit.
Real double NAT (TP-Link Archer C5400X behind the ISP's Askey fiber router, both with UPnP on; Sunshine on Linux, KWin capture, NVENC, ports moved to 37989 to stay clear of other hosts). The client was Moonlight on Windows on another ISP's network, connecting to the public address with no VPN.
/serverinfofrom outside timed out.Completed UPnP port mappings to <host> via <Archer>and… to <Archer WAN> via <ISP router>. Checked withupnpc -lon both routers. From outside:/serverinfoanswered (HTTP 200); pairing over the public address worked; a 20 s stream (1280×720 H.264) delivered video and audio, with 22–60 ms average network latency in Moonlight's overlay and 2 video frames lost on the way (each recovered with an IDR request, per Moonlight's log). The host's screen was idle and dark, so this checks the connection, not image quality. On exit, both routers' mappings were removed, and/serverinfofrom outside timed out again.Not tested: Windows and macOS hosts, other router models, IPv6 (pinhole code unchanged), and an outer router that changes while Sunshine runs.
Security-follow-up validation (the real-router results above predate this follow-up):
Type of Change
Checklist
AI Usage
See our AI usage policy.