From 9ee173c256cce3c0fbf98f20a92a535a059ee51f Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Mon, 11 Sep 2023 12:15:02 -0400 Subject: [PATCH] net/portmapper: fall back to permanent UPnP leases if necessary Some routers don't support lease times for UPnP portmapping; let's fall back to adding a permanent lease in these cases. Additionally, add a proper end-to-end test case for the UPnP portmapping behaviour. Updates #9343 Signed-off-by: Andrew Dunham Change-Id: I17dec600b0595a5bfc9b4d530aff6ee3109a8b12 --- net/portmapper/igd_test.go | 13 ++- net/portmapper/upnp.go | 56 +++++++++- net/portmapper/upnp_test.go | 216 ++++++++++++++++++++++++++++++++++++ 3 files changed, 283 insertions(+), 2 deletions(-) diff --git a/net/portmapper/igd_test.go b/net/portmapper/igd_test.go index 3551a7fa8..5cb6b0755 100644 --- a/net/portmapper/igd_test.go +++ b/net/portmapper/igd_test.go @@ -16,6 +16,7 @@ "tailscale.com/control/controlknobs" "tailscale.com/net/netaddr" + "tailscale.com/syncs" "tailscale.com/types/logger" ) @@ -25,6 +26,7 @@ type TestIGD struct { upnpConn net.PacketConn // for UPnP discovery pxpConn net.PacketConn // for NAT-PMP and/or PCP ts *httptest.Server + upnpHTTP syncs.AtomicValue[http.Handler] logf logger.Logf closed atomic.Bool @@ -126,8 +128,17 @@ func (d *TestIGD) stats() igdCounters { return d.counters } +func (d *TestIGD) SetUPnPHandler(h http.Handler) { + d.upnpHTTP.Store(h) +} + func (d *TestIGD) serveUPnPHTTP(w http.ResponseWriter, r *http.Request) { - http.NotFound(w, r) // TODO + if handler := d.upnpHTTP.Load(); handler != nil { + handler.ServeHTTP(w, r) + return + } + + http.NotFound(w, r) } func (d *TestIGD) serveUPnPDiscovery() { diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index 54c1c2f1a..7350becae 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -11,6 +11,7 @@ "bufio" "bytes" "context" + "encoding/xml" "fmt" "io" "math/rand" @@ -24,6 +25,7 @@ "github.com/tailscale/goupnp" "github.com/tailscale/goupnp/dcps/internetgateway2" + "github.com/tailscale/goupnp/soap" "tailscale.com/envknob" "tailscale.com/net/netns" "tailscale.com/types/logger" @@ -316,6 +318,7 @@ func (c *Client) getUPnPPortMapping( return netip.AddrPort{}, false } + // Start by trying to make a temporary lease with a duration. var newPort uint16 newPort, err = addAnyPortMapping( ctx, @@ -323,14 +326,37 @@ func (c *Client) getUPnPPortMapping( prevPort, internal.Port(), internal.Addr().String(), - time.Second*pmpMapLifetimeSec, + pmpMapLifetimeSec*time.Second, ) if c.debug.VerboseLogs { c.logf("addAnyPortMapping: %v, err=%q", newPort, err) } + + // If this is an error and the code is + // "OnlyPermanentLeasesSupported", then we retry with no lease + // duration; see the following issue for details: + // https://github.com/tailscale/tailscale/issues/9343 + if err != nil { + // From the UPnP spec: http://upnp.org/specs/gw/UPnP-gw-WANIPConnection-v2-Service.pdf + // 725: OnlyPermanentLeasesSupported + if isUPnPError(err, 725) { + newPort, err = addAnyPortMapping( + ctx, + client, + prevPort, + internal.Port(), + internal.Addr().String(), + 0, // permanent + ) + if c.debug.VerboseLogs { + c.logf("addAnyPortMapping: 725 retry %v, err=%q", newPort, err) + } + } + } if err != nil { return netip.AddrPort{}, false } + // TODO cache this ip somewhere? extIP, err := client.GetExternalIPAddress(ctx) if c.debug.VerboseLogs { @@ -346,6 +372,10 @@ func (c *Client) getUPnPPortMapping( } upnp.external = netip.AddrPortFrom(externalIP, newPort) + + // NOTE: this time might not technically be accurate if we created a + // permanent lease above, but we should still re-check the presence of + // the lease on a regular basis so we use it anyway. d := time.Duration(pmpMapLifetimeSec) * time.Second upnp.goodUntil = now.Add(d) upnp.renewAfter = now.Add(d / 2) @@ -357,6 +387,30 @@ func (c *Client) getUPnPPortMapping( return upnp.external, true } +// isUPnPError returns whether the provided error is a UPnP error response with +// the given error code. It returns false if the error is not a SOAP error, or +// the inner error details are not a UPnP error. +func isUPnPError(err error, errCode int) bool { + soapErr, ok := err.(*soap.SOAPFaultError) + if !ok { + return false + } + + var upnpErr struct { + XMLName xml.Name + Code int `xml:"errorCode"` + Description string `xml:"errorDescription"` + } + if err := xml.Unmarshal([]byte(soapErr.Detail.Raw), &upnpErr); err != nil { + return false + } + if upnpErr.XMLName.Local != "UPnPError" { + return false + } + + return upnpErr.Code == errCode +} + type uPnPDiscoResponse struct { Location string // Server describes what version the UPnP is, such as MiniUPnPd/2.x.x diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go index c962a123c..aac2ab95c 100644 --- a/net/portmapper/upnp_test.go +++ b/net/portmapper/upnp_test.go @@ -5,6 +5,7 @@ import ( "context" + "encoding/xml" "fmt" "io" "net" @@ -13,6 +14,7 @@ "net/netip" "reflect" "regexp" + "sync/atomic" "testing" "tailscale.com/tstest" @@ -129,3 +131,217 @@ func TestGetUPnPClient(t *testing.T) { }) } } + +func TestGetUPnPPortMapping(t *testing.T) { + igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true}) + if err != nil { + t.Fatal(err) + } + defer igd.Close() + + c := newTestClient(t, igd) + t.Logf("Listening on upnp=%v", c.testUPnPPort) + defer c.Close() + + c.debug.VerboseLogs = true + + // This is a very basic fake UPnP server handler. + var sawRequestWithLease atomic.Bool + igd.SetUPnPHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Logf("got UPnP request %s %s", r.Method, r.URL.Path) + switch r.URL.Path { + case "/rootDesc.xml": + io.WriteString(w, testRootDesc) + case "/ctl/IPConn": + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("error reading request body: %v", err) + http.Error(w, "bad request", http.StatusBadRequest) + return + } + + // Decode the request type. + var outerRequest struct { + Body struct { + Request struct { + XMLName xml.Name + } `xml:",any"` + Inner string `xml:",innerxml"` + } `xml:"Body"` + } + if err := xml.Unmarshal(body, &outerRequest); err != nil { + t.Errorf("bad request: %v", err) + http.Error(w, "bad request", http.StatusBadRequest) + return + } + + requestType := outerRequest.Body.Request.XMLName.Local + upnpRequest := outerRequest.Body.Inner + t.Logf("UPnP request: %s", requestType) + + switch requestType { + case "AddPortMapping": + // Decode a minimal body to determine whether we skip the request or not. + var req struct { + Protocol string `xml:"NewProtocol"` + InternalPort string `xml:"NewInternalPort"` + ExternalPort string `xml:"NewExternalPort"` + InternalClient string `xml:"NewInternalClient"` + LeaseDuration string `xml:"NewLeaseDuration"` + } + if err := xml.Unmarshal([]byte(upnpRequest), &req); err != nil { + t.Errorf("bad request: %v", err) + http.Error(w, "bad request", http.StatusBadRequest) + return + } + + if req.Protocol != "UDP" { + t.Errorf(`got Protocol=%q, want "UDP"`, req.Protocol) + } + if req.LeaseDuration != "0" { + // Return a fake error to ensure that we fall back to a permanent lease. + io.WriteString(w, testAddPortMappingPermanentLease) + sawRequestWithLease.Store(true) + } else { + // Success! + io.WriteString(w, testAddPortMappingResponse) + } + case "GetExternalIPAddress": + io.WriteString(w, testGetExternalIPAddressResponse) + + case "DeletePortMapping": + // Do nothing for test + + default: + t.Errorf("unhandled UPnP request type %q", requestType) + http.Error(w, "bad request", http.StatusBadRequest) + } + default: + t.Logf("ignoring request") + http.NotFound(w, r) + } + })) + + ctx := context.Background() + res, err := c.Probe(ctx) + if err != nil { + t.Fatalf("Probe: %v", err) + } + if !res.UPnP { + t.Errorf("didn't detect UPnP") + } + + gw, myIP, ok := c.gatewayAndSelfIP() + if !ok { + t.Fatalf("could not get gateway and self IP") + } + t.Logf("gw=%v myIP=%v", gw, myIP) + + ext, ok := c.getUPnPPortMapping(ctx, gw, netip.AddrPortFrom(myIP, 12345), 0) + if !ok { + t.Fatal("could not get UPnP port mapping") + } + if got, want := ext.Addr(), netip.MustParseAddr("123.123.123.123"); got != want { + t.Errorf("bad external address; got %v want %v", got, want) + } + if !sawRequestWithLease.Load() { + t.Errorf("wanted request with lease, but didn't see one") + } + t.Logf("external IP: %v", ext) +} + +const testRootDesc = ` + + + 1 + 1 + + + urn:schemas-upnp-org:device:InternetGatewayDevice:1 + Tailscale Test Router + Tailscale + https://tailscale.com + Tailscale Test Router + Tailscale Test Router + 2.5.0-RELEASE + https://tailscale.com + 1234 + uuid:1974e83b-6dc7-4635-92b3-6a85a4037294 + + + urn:schemas-upnp-org:device:WANDevice:1 + WANDevice + MiniUPnP + http://miniupnp.free.fr/ + WAN Device + WAN Device + 20990102 + http://miniupnp.free.fr/ + 1234 + uuid:1974e83b-6dc7-4635-92b3-6a85a4037294 + 000000000000 + + + urn:schemas-upnp-org:device:WANConnectionDevice:1 + WANConnectionDevice + MiniUPnP + http://miniupnp.free.fr/ + MiniUPnP daemon + MiniUPnPd + 20210205 + http://miniupnp.free.fr/ + 1234 + uuid:1974e83b-6dc7-4635-92b3-6a85a4037294 + 000000000000 + + + urn:schemas-upnp-org:service:WANIPConnection:1 + urn:upnp-org:serviceId:WANIPConn1 + /WANIPCn.xml + /ctl/IPConn + /evt/IPConn + + + + + + + https://127.0.0.1/ + + +` + +const testAddPortMappingPermanentLease = ` + + + + s:Client + UPnPError + + + 725 + OnlyPermanentLeasesSupported + + + + + +` + +const testAddPortMappingResponse = ` + + + + + +` + +const testGetExternalIPAddressResponse = ` + + + + 123.123.123.123 + + + +`