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 @@ import (
"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 @@ import (
"bufio"
"bytes"
"context"
+ "encoding/xml"
"fmt"
"io"
"math/rand"
@@ -24,6 +25,7 @@ import (
"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 @@ package portmapper
import (
"context"
+ "encoding/xml"
"fmt"
"io"
"net"
@@ -13,6 +14,7 @@ import (
"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
+
+
+
+`