net/portmapper: be smarter about selecting a UPnP device

Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

RELNOTE=Improve UPnP portmapping when multiple UPnP services exist

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753

net/portmapper/portmapper.go

@@ -1015,6 +1015,30 @@ func (c *Client) Probe(ctx context.Context) (res ProbeResult, err error) {
 	// received a UPnP response from a port other than the UPnP port.
 	metricUPnPResponseAlternatePort = clientmetric.NewCounter("portmap_upnp_response_alternate_port")
 
+	// metricUPnPSelectSingle counts the number of times that only a single
+	// UPnP device was available in selectBestService.
+	metricUPnPSelectSingle = clientmetric.NewCounter("portmap_upnp_select_single")
+
+	// metricUPnPSelectMultiple counts the number of times that we need to
+	// select from among multiple UPnP devices in selectBestService.
+	metricUPnPSelectMultiple = clientmetric.NewCounter("portmap_upnp_select_multiple")
+
+	// metricUPnPSelectExternalPublic counts the number of times that
+	// selectBestService picked a UPnP device with an external public IP.
+	metricUPnPSelectExternalPublic = clientmetric.NewCounter("portmap_upnp_select_external_public")
+
+	// metricUPnPSelectExternalPrivate counts the number of times that
+	// selectBestService picked a UPnP device with an external private IP.
+	metricUPnPSelectExternalPrivate = clientmetric.NewCounter("portmap_upnp_select_external_private")
+
+	// metricUPnPSelectUp counts the number of times that selectBestService
+	// picked a UPnP device that was up but with no external IP.
+	metricUPnPSelectUp = clientmetric.NewCounter("portmap_upnp_select_up")
+
+	// metricUPnPSelectNone counts the number of times that selectBestService
+	// picked a UPnP device that is not up.
+	metricUPnPSelectNone = clientmetric.NewCounter("portmap_upnp_select_none")
+
 	// metricUPnPParseErr counts the number of times we failed to parse a UPnP response.
 	metricUPnPParseErr = clientmetric.NewCounter("portmap_upnp_parse_err")
 

net/portmapper/select_test.go

@@ -0,0 +1,360 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package portmapper
+
+import (
+	"context"
+	"encoding/xml"
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/tailscale/goupnp"
+	"github.com/tailscale/goupnp/dcps/internetgateway2"
+)
+
+// NOTE: this is in a distinct file because the various string constants are
+// pretty verbose.
+
+func TestSelectBestService(t *testing.T) {
+	mustParseURL := func(ss string) *url.URL {
+		u, err := url.Parse(ss)
+		if err != nil {
+			t.Fatalf("error parsing URL %q: %v", ss, err)
+		}
+		return u
+	}
+
+	// Run a fake IGD server to respond to UPnP requests.
+	igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer igd.Close()
+
+	testCases := []struct {
+		name     string
+		rootDesc string
+		control  map[string]map[string]any
+		want     string // controlURL field
+	}{
+		{
+			name:     "single_device",
+			rootDesc: testRootDesc,
+			control: map[string]map[string]any{
+				// Service that's up and should be selected.
+				"/ctl/IPConn": {
+					"GetExternalIPAddress": testGetExternalIPAddressResponse,
+					"GetStatusInfo":        testGetStatusInfoResponse,
+				},
+			},
+			want: "/ctl/IPConn",
+		},
+		{
+			name:     "first_device_disconnected",
+			rootDesc: testSelectRootDesc,
+			control: map[string]map[string]any{
+				// Service that's down; it's important that this is the
+				// one that's down since it's ordered first in the XML
+				// and we want to verify that our code properly queries
+				// and then skips it.
+				"/upnp/control/yomkmsnooi/wanipconn-1": {
+					"GetStatusInfo": testGetStatusInfoResponseDisconnected,
+					// NOTE: nothing else should be called
+					// if GetStatusInfo returns a
+					// disconnected result
+				},
+				// Service that's up and should be selected.
+				"/upnp/control/xstnsgeuyh/wanipconn-7": {
+					"GetExternalIPAddress": testGetExternalIPAddressResponse,
+					"GetStatusInfo":        testGetStatusInfoResponse,
+				},
+			},
+			want: "/upnp/control/xstnsgeuyh/wanipconn-7",
+		},
+		{
+			name:     "prefer_public_external_IP",
+			rootDesc: testSelectRootDesc,
+			control: map[string]map[string]any{
+				// Service with a private external IP; order matters as above.
+				"/upnp/control/yomkmsnooi/wanipconn-1": {
+					"GetStatusInfo":        testGetStatusInfoResponse,
+					"GetExternalIPAddress": testGetExternalIPAddressResponsePrivate,
+				},
+				// Service that's up and should be selected.
+				"/upnp/control/xstnsgeuyh/wanipconn-7": {
+					"GetExternalIPAddress": testGetExternalIPAddressResponse,
+					"GetStatusInfo":        testGetStatusInfoResponse,
+				},
+			},
+			want: "/upnp/control/xstnsgeuyh/wanipconn-7",
+		},
+		{
+			name:     "all_private_external_IPs",
+			rootDesc: testSelectRootDesc,
+			control: map[string]map[string]any{
+				"/upnp/control/yomkmsnooi/wanipconn-1": {
+					"GetStatusInfo":        testGetStatusInfoResponse,
+					"GetExternalIPAddress": testGetExternalIPAddressResponsePrivate,
+				},
+				"/upnp/control/xstnsgeuyh/wanipconn-7": {
+					"GetStatusInfo":        testGetStatusInfoResponse,
+					"GetExternalIPAddress": testGetExternalIPAddressResponsePrivate,
+				},
+			},
+			want: "/upnp/control/yomkmsnooi/wanipconn-1", // since this is first in the XML
+		},
+		{
+			name:     "nothing_connected",
+			rootDesc: testSelectRootDesc,
+			control: map[string]map[string]any{
+				"/upnp/control/yomkmsnooi/wanipconn-1": {
+					"GetStatusInfo": testGetStatusInfoResponseDisconnected,
+				},
+				"/upnp/control/xstnsgeuyh/wanipconn-7": {
+					"GetStatusInfo": testGetStatusInfoResponseDisconnected,
+				},
+			},
+			want: "/upnp/control/yomkmsnooi/wanipconn-1", // since this is first in the XML
+		},
+		{
+			name:     "GetStatusInfo_errors",
+			rootDesc: testSelectRootDesc,
+			control: map[string]map[string]any{
+				"/upnp/control/yomkmsnooi/wanipconn-1": {
+					"GetStatusInfo": func(_ string) (int, string) {
+						return http.StatusInternalServerError, "internal error"
+					},
+				},
+				"/upnp/control/xstnsgeuyh/wanipconn-7": {
+					"GetStatusInfo": func(_ string) (int, string) {
+						return http.StatusNotFound, "not found"
+					},
+				},
+			},
+			want: "/upnp/control/yomkmsnooi/wanipconn-1", // since this is first in the XML
+		},
+		{
+			name:     "GetExternalIPAddress_bad_ip",
+			rootDesc: testSelectRootDesc,
+			control: map[string]map[string]any{
+				"/upnp/control/yomkmsnooi/wanipconn-1": {
+					"GetStatusInfo":        testGetStatusInfoResponse,
+					"GetExternalIPAddress": testGetExternalIPAddressResponseInvalid,
+				},
+				"/upnp/control/xstnsgeuyh/wanipconn-7": {
+					"GetStatusInfo":        testGetStatusInfoResponse,
+					"GetExternalIPAddress": testGetExternalIPAddressResponse,
+				},
+			},
+			want: "/upnp/control/xstnsgeuyh/wanipconn-7",
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			// Ensure that we're using our test IGD server for all requests.
+			rootDesc := strings.ReplaceAll(tt.rootDesc, "@SERVERURL@", igd.ts.URL)
+
+			igd.SetUPnPHandler(&upnpServer{
+				t:       t,
+				Desc:    rootDesc,
+				Control: tt.control,
+			})
+			c := newTestClient(t, igd)
+			t.Logf("Listening on upnp=%v", c.testUPnPPort)
+			defer c.Close()
+
+			// Ensure that we're using the HTTP client that talks to our test IGD server
+			ctx := context.Background()
+			ctx = goupnp.WithHTTPClient(ctx, c.upnpHTTPClientLocked())
+
+			loc := mustParseURL(igd.ts.URL)
+			rootDev := mustParseRootDev(t, rootDesc, loc)
+
+			svc, err := selectBestService(ctx, t.Logf, rootDev, loc)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			var controlURL string
+			switch v := svc.(type) {
+			case *internetgateway2.WANIPConnection2:
+				controlURL = v.ServiceClient.Service.ControlURL.Str
+			case *internetgateway2.WANIPConnection1:
+				controlURL = v.ServiceClient.Service.ControlURL.Str
+			case *internetgateway2.WANPPPConnection1:
+				controlURL = v.ServiceClient.Service.ControlURL.Str
+			default:
+				t.Fatalf("unknown client type: %T", v)
+			}
+
+			if controlURL != tt.want {
+				t.Errorf("mismatched controlURL: got=%q want=%q", controlURL, tt.want)
+			}
+		})
+	}
+}
+
+func mustParseRootDev(t *testing.T, devXML string, loc *url.URL) *goupnp.RootDevice {
+	decoder := xml.NewDecoder(strings.NewReader(devXML))
+	decoder.DefaultSpace = goupnp.DeviceXMLNamespace
+	decoder.CharsetReader = goupnp.CharsetReaderDefault
+
+	root := new(goupnp.RootDevice)
+	if err := decoder.Decode(root); err != nil {
+		t.Fatalf("error decoding device XML: %v", err)
+	}
+
+	// Ensure the URLBase is set properly; this is how DeviceByURL does it.
+	var urlBaseStr string
+	if root.URLBaseStr != "" {
+		urlBaseStr = root.URLBaseStr
+	} else {
+		urlBaseStr = loc.String()
+	}
+	urlBase, err := url.Parse(urlBaseStr)
+	if err != nil {
+		t.Fatalf("error parsing URL %q: %v", urlBaseStr, err)
+	}
+	root.SetURLBase(urlBase)
+
+	return root
+}
+
+// Note: adapted from mikrotikRootDescXML with addresses replaced with
+// localhost, and unnecessary fields removed.
+const testSelectRootDesc = `<?xml version="1.0"?>
+<root xmlns="urn:schemas-upnp-org:device-1-0">
+  <specVersion>
+    <major>1</major>
+    <minor>0</minor>
+  </specVersion>
+  <device>
+    <deviceType>urn:schemas-upnp-org:device:InternetGatewayDevice:1</deviceType>
+    <friendlyName>MikroTik Router</friendlyName>
+    <manufacturer>MikroTik</manufacturer>
+    <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
+    <modelName>Router OS</modelName>
+    <UDN>uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-</UDN>
+    <serviceList>
+      <service>
+        <serviceType>urn:schemas-microsoft-com:service:OSInfo:1</serviceType>
+        <serviceId>urn:microsoft-com:serviceId:OSInfo1</serviceId>
+        <SCPDURL>/osinfo.xml</SCPDURL>
+        <controlURL>/upnp/control/oqjsxqshhz/osinfo</controlURL>
+        <eventSubURL>/upnp/event/cwzcyndrjf/osinfo</eventSubURL>
+      </service>
+    </serviceList>
+    <deviceList>
+      <device>
+        <deviceType>urn:schemas-upnp-org:device:WANDevice:1</deviceType>
+        <friendlyName>WAN Device</friendlyName>
+        <manufacturer>MikroTik</manufacturer>
+        <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
+        <modelName>Router OS</modelName>
+        <UDN>uuid:UUID-MIKROTIK-WAN-DEVICE--1</UDN>
+        <serviceList>
+          <service>
+            <serviceType>urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1</serviceType>
+            <serviceId>urn:upnp-org:serviceId:WANCommonIFC1</serviceId>
+            <SCPDURL>/wancommonifc-1.xml</SCPDURL>
+            <controlURL>/upnp/control/ivvmxhunyq/wancommonifc-1</controlURL>
+            <eventSubURL>/upnp/event/mkjzdqvryf/wancommonifc-1</eventSubURL>
+          </service>
+        </serviceList>
+        <deviceList>
+          <device>
+            <deviceType>urn:schemas-upnp-org:device:WANConnectionDevice:1</deviceType>
+            <friendlyName>WAN Connection Device</friendlyName>
+            <manufacturer>MikroTik</manufacturer>
+            <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
+            <modelName>Router OS</modelName>
+            <UDN>uuid:UUID-MIKROTIK-WAN-CONNECTION-DEVICE--1</UDN>
+            <serviceList>
+              <service>
+                <serviceType>urn:schemas-upnp-org:service:WANIPConnection:1</serviceType>
+                <serviceId>urn:upnp-org:serviceId:WANIPConn1</serviceId>
+                <SCPDURL>/wanipconn-1.xml</SCPDURL>
+                <controlURL>/upnp/control/yomkmsnooi/wanipconn-1</controlURL>
+                <eventSubURL>/upnp/event/veeabhzzva/wanipconn-1</eventSubURL>
+              </service>
+            </serviceList>
+          </device>
+        </deviceList>
+      </device>
+      <device>
+        <deviceType>urn:schemas-upnp-org:device:WANDevice:1</deviceType>
+        <friendlyName>WAN Device</friendlyName>
+        <manufacturer>MikroTik</manufacturer>
+        <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
+        <modelName>Router OS</modelName>
+        <UDN>uuid:UUID-MIKROTIK-WAN-DEVICE--7</UDN>
+        <serviceList>
+          <service>
+            <serviceType>urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1</serviceType>
+            <serviceId>urn:upnp-org:serviceId:WANCommonIFC1</serviceId>
+            <SCPDURL>/wancommonifc-7.xml</SCPDURL>
+            <controlURL>/upnp/control/vzcyyzzttz/wancommonifc-7</controlURL>
+            <eventSubURL>/upnp/event/womwbqtbkq/wancommonifc-7</eventSubURL>
+          </service>
+        </serviceList>
+        <deviceList>
+          <device>
+            <deviceType>urn:schemas-upnp-org:device:WANConnectionDevice:1</deviceType>
+            <friendlyName>WAN Connection Device</friendlyName>
+            <manufacturer>MikroTik</manufacturer>
+            <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
+            <modelName>Router OS</modelName>
+            <UDN>uuid:UUID-MIKROTIK-WAN-CONNECTION-DEVICE--7</UDN>
+            <serviceList>
+              <service>
+                <serviceType>urn:schemas-upnp-org:service:WANIPConnection:1</serviceType>
+                <serviceId>urn:upnp-org:serviceId:WANIPConn1</serviceId>
+                <SCPDURL>/wanipconn-7.xml</SCPDURL>
+                <controlURL>/upnp/control/xstnsgeuyh/wanipconn-7</controlURL>
+                <eventSubURL>/upnp/event/rscixkusbs/wanipconn-7</eventSubURL>
+              </service>
+            </serviceList>
+          </device>
+        </deviceList>
+      </device>
+    </deviceList>
+    <presentationURL>@SERVERURL@</presentationURL>
+  </device>
+  <URLBase>@SERVERURL@</URLBase>
+</root>`
+
+const testGetStatusInfoResponseDisconnected = `<?xml version="1.0"?>
+<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
+  <s:Body>
+    <u:GetStatusInfoResponse xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1">
+      <NewConnectionStatus>Disconnected</NewConnectionStatus>
+      <NewLastConnectionError>ERROR_NONE</NewLastConnectionError>
+      <NewUptime>0</NewUptime>
+    </u:GetStatusInfoResponse>
+  </s:Body>
+</s:Envelope>
+`
+
+const testGetExternalIPAddressResponsePrivate = `<?xml version="1.0"?>
+<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
+  <s:Body>
+    <u:GetExternalIPAddressResponse xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1">
+      <NewExternalIPAddress>10.9.8.7</NewExternalIPAddress>
+    </u:GetExternalIPAddressResponse>
+  </s:Body>
+</s:Envelope>
+`
+
+const testGetExternalIPAddressResponseInvalid = `<?xml version="1.0"?>
+<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
+  <s:Body>
+    <u:GetExternalIPAddressResponse xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1">
+      <NewExternalIPAddress>not-an-ip-addr</NewExternalIPAddress>
+    </u:GetExternalIPAddressResponse>
+  </s:Body>
+</s:Envelope>
+`

net/portmapper/upnp.go

@@ -29,6 +29,7 @@
 	"tailscale.com/envknob"
 	"tailscale.com/net/netns"
 	"tailscale.com/types/logger"
+	"tailscale.com/util/mak"
 )
 
 // References:
@@ -44,7 +45,14 @@ type upnpMapping struct {
 	goodUntil  time.Time
 	renewAfter time.Time
 
-	// client is a connection to a upnp device, and may be reused across different UPnP mappings.
+	// rootDev is the UPnP root device, and may be reused across different
+	// UPnP mappings.
+	rootDev *goupnp.RootDevice
+	// loc is the location used to fetch the rootDev
+	loc *url.URL
+	// client is the most recent UPnP client used, and should only be used
+	// to release an existing mapping; new mappings should be selected from
+	// the rootDev on each attempt.
 	client upnpClient
 }
 
@@ -104,6 +112,7 @@ type upnpClient interface {
 
 	DeletePortMapping(ctx context.Context, remoteHost string, externalPort uint16, protocol string) error
 	GetExternalIPAddress(ctx context.Context) (externalIPAddress string, err error)
+	GetStatusInfo(ctx context.Context) (status string, lastConnError string, uptime uint32, err error)
 }
 
 // tsPortMappingDesc gets sent to UPnP clients as a human-readable label for the portmapping.
@@ -182,24 +191,21 @@ func addAnyPortMapping(
 	return externalPort, err
 }
 
-// getUPnPClient gets a client for interfacing with UPnP, ignoring the underlying protocol for
+// getUPnPRootDevice fetches the UPnP root device given the discovery response,
-// now.
+// ignoring the underlying protocol for now.
 // Adapted from https://github.com/huin/goupnp/blob/master/GUIDE.md.
 //
 // The gw is the detected gateway.
 //
 // The meta is the most recently parsed UDP discovery packet response
 // from the Internet Gateway Device.
-//
+func getUPnPRootDevice(ctx context.Context, logf logger.Logf, debug DebugKnobs, gw netip.Addr, meta uPnPDiscoResponse) (rootDev *goupnp.RootDevice, loc *url.URL, err error) {
-// The provided ctx is not retained in the returned upnpClient, but
-// its associated HTTP client is (if set via goupnp.WithHTTPClient).
-func getUPnPClient(ctx context.Context, logf logger.Logf, debug DebugKnobs, gw netip.Addr, meta uPnPDiscoResponse) (client upnpClient, err error) {
 	if debug.DisableUPnP {
-		return nil, nil
+		return nil, nil, nil
 	}
 
 	if meta.Location == "" {
-		return nil, nil
+		return nil, nil, nil
 	}
 
 	if debug.VerboseLogs {
@@ -207,12 +213,12 @@ func getUPnPClient(ctx context.Context, logf logger.Logf, debug DebugKnobs, gw n
 	}
 	u, err := url.Parse(meta.Location)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	ipp, err := netip.ParseAddrPort(u.Host)
 	if err != nil {
-		return nil, fmt.Errorf("unexpected host %q in %q", u.Host, meta.Location)
+		return nil, nil, fmt.Errorf("unexpected host %q in %q", u.Host, meta.Location)
 	}
 	if ipp.Addr() != gw {
 		// https://github.com/tailscale/tailscale/issues/5502
@@ -231,30 +237,150 @@ func getUPnPClient(ctx context.Context, logf logger.Logf, debug DebugKnobs, gw n
 	// This part does a network fetch.
 	root, err := goupnp.DeviceByURL(ctx, u)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
+	return root, u, nil
+}
 
+// selectBestService picks the "best" service from the given UPnP root device
+// to use to create a port mapping.
+//
+// loc is the parsed location that was used to fetch the given RootDevice.
+//
+// The provided ctx is not retained in the returned upnpClient, but
+// its associated HTTP client is (if set via goupnp.WithHTTPClient).
+func selectBestService(ctx context.Context, logf logger.Logf, root *goupnp.RootDevice, loc *url.URL) (client upnpClient, err error) {
+	method := "none"
 	defer func() {
 		if client == nil {
 			return
 		}
-		logf("saw UPnP type %v at %v; %v (%v)",
+		logf("saw UPnP type %v at %v; %v (%v), method=%s",
 			strings.TrimPrefix(fmt.Sprintf("%T", client), "*internetgateway2."),
-			meta.Location, root.Device.FriendlyName, root.Device.Manufacturer)
+			loc, root.Device.FriendlyName, root.Device.Manufacturer,
+			method)
 	}()
 
-	// These parts don't do a network fetch.
+	// First, get all available clients from the device, and append to our
-	// Pick the best service type available.
+	// list of possible clients. Order matters here; we want to prefer
-	if cc, _ := internetgateway2.NewWANIPConnection2ClientsFromRootDevice(ctx, root, u); len(cc) > 0 {
+	// WANIPConnection2 over WANIPConnection1 or WANPPPConnection.
-		return cc[0], nil
+	wanIP2, _ := internetgateway2.NewWANIPConnection2ClientsFromRootDevice(ctx, root, loc)
+	wanIP1, _ := internetgateway2.NewWANIPConnection1ClientsFromRootDevice(ctx, root, loc)
+	wanPPP, _ := internetgateway2.NewWANPPPConnection1ClientsFromRootDevice(ctx, root, loc)
+
+	var clients []upnpClient
+	for _, v := range wanIP2 {
+		clients = append(clients, v)
 	}
-	if cc, _ := internetgateway2.NewWANIPConnection1ClientsFromRootDevice(ctx, root, u); len(cc) > 0 {
+	for _, v := range wanIP1 {
-		return cc[0], nil
+		clients = append(clients, v)
 	}
-	if cc, _ := internetgateway2.NewWANPPPConnection1ClientsFromRootDevice(ctx, root, u); len(cc) > 0 {
+	for _, v := range wanPPP {
-		return cc[0], nil
+		clients = append(clients, v)
 	}
+
+	// If we have no clients, then return right now; if we only have one,
+	// just select and return it.
+	if len(clients) == 0 {
 		return nil, nil
+	}
+	if len(clients) == 1 {
+		method = "single"
+		metricUPnPSelectSingle.Add(1)
+		return clients[0], nil
+	}
+
+	metricUPnPSelectMultiple.Add(1)
+
+	// In order to maximize the chances that we find a valid UPnP device
+	// that can give us a port mapping, we check a few properties:
+	//	1. Whether the device is "online", as defined by GetStatusInfo
+	//	2. Whether the device has an external IP address, as defined by
+	//	   GetExternalIPAddress
+	//	3. Whether the device's external IP address is a public address
+	//	   or a private one.
+	//
+	// We prefer a device where all of the above is true, and fall back if
+	// none are found.
+	//
+	// In order to save on network requests, iterate through all devices
+	// and determine how many "points" they have based on the above
+	// criteria, but return immediately if we find one that meets all
+	// three.
+	var (
+		connected   = make(map[upnpClient]bool)
+		externalIPs map[upnpClient]netip.Addr
+	)
+	for _, svc := range clients {
+		isConnected := serviceIsConnected(ctx, logf, svc)
+		connected[svc] = isConnected
+
+		// Don't bother checking for an external IP if the device isn't
+		// connected; technically this could happen with a misbehaving
+		// device, but that seems unlikely.
+		if !isConnected {
+			continue
+		}
+
+		// Check if the device has an external IP address.
+		extIP, err := svc.GetExternalIPAddress(ctx)
+		if err != nil {
+			continue
+		}
+		externalIP, err := netip.ParseAddr(extIP)
+		if err != nil {
+			continue
+		}
+		mak.Set(&externalIPs, svc, externalIP)
+
+		// If we get here, this device has a non-private external IP
+		// and is up, so we can just return it.
+		if !externalIP.IsPrivate() {
+			method = "ext-public"
+			metricUPnPSelectExternalPublic.Add(1)
+			return svc, nil
+		}
+	}
+
+	// Okay, we have no devices that meet all the available options. Fall
+	// back to first checking for devices that are up and have a private
+	// external IP (order matters), and then devices that are up, and then
+	// just anything at all.
+	//
+	//	try=0	Up + private external IP
+	//	try=1	Up
+	for try := 0; try <= 1; try++ {
+		for _, svc := range clients {
+			if !connected[svc] {
+				continue
+			}
+			_, hasExtIP := externalIPs[svc]
+			if hasExtIP {
+				method = "ext-private"
+				metricUPnPSelectExternalPrivate.Add(1)
+				return svc, nil
+			} else if try == 1 {
+				method = "up"
+				metricUPnPSelectUp.Add(1)
+				return svc, nil
+			}
+		}
+	}
+
+	// Nothing is up, but we have something (length of clients checked
+	// above); just return the first one.
+	metricUPnPSelectNone.Add(1)
+	return clients[0], nil
+}
+
+// serviceIsConnected returns whether a given UPnP service is connected, based
+// on the NewConnectionStatus field returned from GetStatusInfo.
+func serviceIsConnected(ctx context.Context, logf logger.Logf, svc upnpClient) bool {
+	status, _ /* NewLastConnectionError */, _ /* NewUptime */, err := svc.GetStatusInfo(ctx)
+	if err != nil {
+		return false
+	}
+	return status == "Connected" || status == "Up"
 }
 
 func (c *Client) upnpHTTPClientLocked() *http.Client {
@@ -295,26 +421,37 @@ func (c *Client) getUPnPPortMapping(
 		internal: internal,
 	}
 
-	var client upnpClient
+	var (
-	var err error
+		rootDev *goupnp.RootDevice
+		loc     *url.URL
+		err     error
+	)
 	c.mu.Lock()
 	oldMapping, ok := c.mapping.(*upnpMapping)
 	meta := c.uPnPMeta
-	httpClient := c.upnpHTTPClientLocked()
+	ctx = goupnp.WithHTTPClient(ctx, c.upnpHTTPClientLocked())
 	c.mu.Unlock()
 	if ok && oldMapping != nil {
-		client = oldMapping.client
+		rootDev = oldMapping.rootDev
+		loc = oldMapping.loc
 	} else {
-		ctx := goupnp.WithHTTPClient(ctx, httpClient)
+		rootDev, loc, err = getUPnPRootDevice(ctx, c.logf, c.debug, gw, meta)
-		client, err = getUPnPClient(ctx, c.logf, c.debug, gw, meta)
 		if c.debug.VerboseLogs {
-			c.logf("getUPnPClient: %T, %v", client, err)
+			c.logf("getUPnPRootDevice: loc=%q err=%v", loc, err)
 		}
 		if err != nil {
 			return netip.AddrPort{}, false
 		}
 	}
-	if client == nil {
+	if rootDev == nil {
+		return netip.AddrPort{}, false
+	}
+
+	// Now that we have a root device, select the best mapping service from
+	// it. This makes network requests, and can vary from mapping to
+	// mapping if the upstream device's connection status changes.
+	client, err := selectBestService(ctx, c.logf, rootDev, loc)
+	if err != nil {
 		return netip.AddrPort{}, false
 	}
 
@@ -384,6 +521,8 @@ func (c *Client) getUPnPPortMapping(
 	d := time.Duration(pmpMapLifetimeSec) * time.Second
 	upnp.goodUntil = now.Add(d)
 	upnp.renewAfter = now.Add(d / 2)
+	upnp.rootDev = rootDev
+	upnp.loc = loc
 	upnp.client = client
 	c.mu.Lock()
 	defer c.mu.Unlock()
@@ -471,7 +610,7 @@ func requestLogger(logf logger.Logf, client *http.Client) *http.Client {
 
 		resp, err := oldTransport.RoundTrip(req)
 		if err != nil {
-			logf("response[%d]: err=%v", err)
+			logf("response[%d]: err=%v", ctr, err)
 			return nil, err
 		}
 
@@ -480,7 +619,7 @@ func requestLogger(logf logger.Logf, client *http.Client) *http.Client {
 			body, err = io.ReadAll(resp.Body)
 			resp.Body.Close()
 			if err != nil {
-				logf("response[%d]: %d bodyErr=%v", resp.StatusCode, err)
+				logf("response[%d]: %d bodyErr=%v", ctr, resp.StatusCode, err)
 				return nil, err
 			}
 			resp.Body = io.NopCloser(bytes.NewReader(body))

net/portmapper/upnp_test.go

@@ -218,19 +218,19 @@ func TestGetUPnPClient(t *testing.T) {
 			"google",
 			googleWifiRootDescXML,
 			"*internetgateway2.WANIPConnection2",
-			"saw UPnP type WANIPConnection2 at http://127.0.0.1:NNN/rootDesc.xml; OnHub (Google)\n",
+			"saw UPnP type WANIPConnection2 at http://127.0.0.1:NNN/rootDesc.xml; OnHub (Google), method=single\n",
 		},
 		{
 			"pfsense",
 			pfSenseRootDescXML,
 			"*internetgateway2.WANIPConnection1",
-			"saw UPnP type WANIPConnection1 at http://127.0.0.1:NNN/rootDesc.xml; FreeBSD router (FreeBSD)\n",
+			"saw UPnP type WANIPConnection1 at http://127.0.0.1:NNN/rootDesc.xml; FreeBSD router (FreeBSD), method=single\n",
 		},
 		{
 			"mikrotik",
 			mikrotikRootDescXML,
 			"*internetgateway2.WANIPConnection1",
-			"saw UPnP type WANIPConnection1 at http://127.0.0.1:NNN/rootDesc.xml; MikroTik Router (MikroTik)\n",
+			"saw UPnP type WANIPConnection1 at http://127.0.0.1:NNN/rootDesc.xml; MikroTik Router (MikroTik), method=none\n",
 		},
 		// TODO(bradfitz): find a PPP one in the wild
 	}
@@ -246,13 +246,20 @@ func TestGetUPnPClient(t *testing.T) {
 			defer ts.Close()
 			gw, _ := netip.AddrFromSlice(ts.Listener.Addr().(*net.TCPAddr).IP)
 			gw = gw.Unmap()
+
+			ctx := context.Background()
+
 			var logBuf tstest.MemLogger
-			c, err := getUPnPClient(context.Background(), logBuf.Logf, DebugKnobs{}, gw, uPnPDiscoResponse{
+			dev, loc, err := getUPnPRootDevice(ctx, logBuf.Logf, DebugKnobs{}, gw, uPnPDiscoResponse{
 				Location: ts.URL + "/rootDesc.xml",
 			})
 			if err != nil {
 				t.Fatal(err)
 			}
+			c, err := selectBestService(ctx, logBuf.Logf, dev, loc)
+			if err != nil {
+				t.Fatal(err)
+			}
 			got := fmt.Sprintf("%T", c)
 			if got != tt.want {
 				t.Errorf("got %v; want %v", got, tt.want)
@@ -272,44 +279,10 @@ func TestGetUPnPPortMapping(t *testing.T) {
 	}
 	defer igd.Close()
 
-	rootDesc := ""
-
 	// This is a very basic fake UPnP server handler.
 	var sawRequestWithLease atomic.Bool
-	igd.SetUPnPHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	handlers := map[string]any{
-		t.Logf("got UPnP request %s %s", r.Method, r.URL.Path)
+		"AddPortMapping": func(body []byte) (int, string) {
-		switch r.URL.Path {
-		case "/rootDesc.xml":
-			io.WriteString(w, rootDesc)
-		case "/ctl/IPConn", "/upnp/control/yomkmsnooi/wanipconn-1":
-			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"`
@@ -318,10 +291,9 @@ func TestGetUPnPPortMapping(t *testing.T) {
 				InternalClient string `xml:"NewInternalClient"`
 				LeaseDuration  string `xml:"NewLeaseDuration"`
 			}
-				if err := xml.Unmarshal([]byte(upnpRequest), &req); err != nil {
+			if err := xml.Unmarshal(body, &req); err != nil {
 				t.Errorf("bad request: %v", err)
-					http.Error(w, "bad request", http.StatusBadRequest)
+				return http.StatusBadRequest, "bad request"
-					return
 			}
 
 			if req.Protocol != "UDP" {
@@ -329,33 +301,31 @@ func TestGetUPnPPortMapping(t *testing.T) {
 			}
 			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 {
+				return http.StatusOK, testAddPortMappingPermanentLease
+			}
+
 			// Success!
-					io.WriteString(w, testAddPortMappingResponse)
+			return http.StatusOK, testAddPortMappingResponse
+		},
+		"GetExternalIPAddress": testGetExternalIPAddressResponse,
+		"GetStatusInfo":        testGetStatusInfoResponse,
+		"DeletePortMapping":    "", // Do nothing for test
 	}
-			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()
 
 	rootDescsToTest := []string{testRootDesc, mikrotikRootDescXML}
+	for _, rootDesc := range rootDescsToTest {
+		igd.SetUPnPHandler(&upnpServer{
+			t:    t,
+			Desc: rootDesc,
+			Control: map[string]map[string]any{
+				"/ctl/IPConn":                          handlers,
+				"/upnp/control/yomkmsnooi/wanipconn-1": handlers,
+			},
+		})
 
-	for _, rootDesc = range rootDescsToTest {
 		c := newTestClient(t, igd)
 		t.Logf("Listening on upnp=%v", c.testUPnPPort)
 		defer c.Close()
@@ -391,6 +361,89 @@ func TestGetUPnPPortMapping(t *testing.T) {
 	}
 }
 
+type upnpServer struct {
+	t       *testing.T
+	Desc    string                    // root device XML
+	Control map[string]map[string]any // map["/url"]map["UPnPService"]response
+}
+
+func (u *upnpServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	u.t.Logf("got UPnP request %s %s", r.Method, r.URL.Path)
+	if r.URL.Path == "/rootDesc.xml" {
+		io.WriteString(w, u.Desc)
+		return
+	}
+	if control, ok := u.Control[r.URL.Path]; ok {
+		u.handleControl(w, r, control)
+		return
+	}
+
+	u.t.Logf("ignoring request")
+	http.NotFound(w, r)
+}
+
+func (u *upnpServer) handleControl(w http.ResponseWriter, r *http.Request, handlers map[string]any) {
+	body, err := io.ReadAll(r.Body)
+	if err != nil {
+		u.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 {
+		u.t.Errorf("bad request: %v", err)
+		http.Error(w, "bad request", http.StatusBadRequest)
+		return
+	}
+
+	requestType := outerRequest.Body.Request.XMLName.Local
+	upnpRequest := outerRequest.Body.Inner
+	u.t.Logf("UPnP request: %s", requestType)
+
+	handler, ok := handlers[requestType]
+	if !ok {
+		u.t.Errorf("unhandled UPnP request type %q", requestType)
+		http.Error(w, "bad request", http.StatusBadRequest)
+		return
+	}
+
+	switch v := handler.(type) {
+	case string:
+		io.WriteString(w, v)
+	case []byte:
+		w.Write(v)
+
+	// Function handlers
+	case func(string) string:
+		io.WriteString(w, v(upnpRequest))
+	case func([]byte) string:
+		io.WriteString(w, v([]byte(upnpRequest)))
+
+	case func(string) (int, string):
+		code, body := v(upnpRequest)
+		w.WriteHeader(code)
+		io.WriteString(w, body)
+	case func([]byte) (int, string):
+		code, body := v([]byte(upnpRequest))
+		w.WriteHeader(code)
+		io.WriteString(w, body)
+
+	default:
+		u.t.Fatalf("invalid handler type: %T", v)
+		http.Error(w, "invalid handler type", http.StatusInternalServerError)
+		return
+	}
+}
+
 const testRootDesc = `<?xml version="1.0"?>
 <root xmlns="urn:schemas-upnp-org:device-1-0" configId="1337">
   <specVersion>
@@ -486,3 +539,15 @@ func TestGetUPnPPortMapping(t *testing.T) {
   </s:Body>
 </s:Envelope>
 `
+
+const testGetStatusInfoResponse = `<?xml version="1.0"?>
+<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
+  <s:Body>
+    <u:GetStatusInfoResponse xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1">
+      <NewConnectionStatus>Connected</NewConnectionStatus>
+      <NewLastConnectionError>ERROR_NONE</NewLastConnectionError>
+      <NewUptime>9999</NewUptime>
+    </u:GetStatusInfoResponse>
+  </s:Body>
+</s:Envelope>
+`