net/portmapper: fix UPnP probing, work against all ports

Prior to Tailscale 1.12 it detected UPnP on any port.
Starting with Tailscale 1.11.x, it stopped detecting UPnP on all ports.

Then start plumbing its discovered Location header port number to the
code that was assuming port 5000.

Fixes #2109

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

net/portmapper/upnp.go

@@ -8,15 +8,22 @@
 package portmapper
 
 import (
+	"bufio"
+	"bytes"
 	"context"
 	"fmt"
 	"math/rand"
+	"net/http"
 	"net/url"
+	"strings"
 	"time"
 
+	"github.com/tailscale/goupnp"
 	"github.com/tailscale/goupnp/dcps/internetgateway2"
 	"inet.af/netaddr"
 	"tailscale.com/control/controlknobs"
+	"tailscale.com/net/netns"
+	"tailscale.com/types/logger"
 )
 
 // References:
@@ -44,7 +51,8 @@ func (u *upnpMapping) Release(ctx context.Context) {
 }
 
 // upnpClient is an interface over the multiple different clients exported by goupnp,
-// exposing the functions we need for portmapping. They are auto-generated from XML-specs.
+// exposing the functions we need for portmapping. Those clients are auto-generated from XML-specs,
+// which is why they're not very idiomatic.
 type upnpClient interface {
 	AddPortMapping(
 		ctx context.Context,
@@ -77,7 +85,7 @@ type upnpClient interface {
 		// greater than 0. From the spec, it appears if it is set to 0, it will switch to using
 		// 604800 seconds, but not sure why this is desired. The recommended time is 3600 seconds.
 		leaseDurationSec uint32,
-	) (err error)
+	) error
 
 	DeletePortMapping(ctx context.Context, remoteHost string, externalPort uint16, protocol string) error
 	GetExternalIPAddress(ctx context.Context) (externalIPAddress string, err error)
@@ -92,6 +100,8 @@ const tsPortMappingDesc = "tailscale-portmap"
 // behavior of calling AddPortMapping with port = 0 to specify a wildcard port.
 // It returns the new external port (which may not be identical to the external port specified),
 // or an error.
+//
+// TODO(bradfitz): also returned the actual lease duration obtained. and check it regularly.
 func addAnyPortMapping(
 	ctx context.Context,
 	upnp upnpClient,
@@ -130,51 +140,89 @@ func addAnyPortMapping(
 	return externalPort, err
 }
 
-// getUPnPClients gets a client for interfacing with UPnP, ignoring the underlying protocol for
+// getUPnPClient gets a client for interfacing with UPnP, ignoring the underlying protocol for
 // now.
 // Adapted from https://github.com/huin/goupnp/blob/master/GUIDE.md.
-func getUPnPClient(ctx context.Context, gw netaddr.IP) (upnpClient, error) {
+//
+// The gw is the detected gateway.
+//
+// The meta is the most recently parsed UDP discovery packet response
+// from the Internet Gateway Device.
+//
+// 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, gw netaddr.IP, meta uPnPDiscoResponse) (client upnpClient, err error) {
 	if controlknobs.DisableUPnP() {
 		return nil, nil
 	}
-	ctx, cancel := context.WithTimeout(ctx, 250*time.Millisecond)
-	defer cancel()
-	// Attempt to connect over the multiple available connection types concurrently,
-	// returning the fastest.
 
-	// TODO(jknodt): this url seems super brittle? maybe discovery is better but this is faster
-	u, err := url.Parse(fmt.Sprintf("http://%s:5000/rootDesc.xml", gw))
+	if meta.Location == "" {
+		return nil, nil
+	}
+
+	if VerboseLogs {
+		logf("fetching %v", meta.Location)
+	}
+	u, err := url.Parse(meta.Location)
 	if err != nil {
 		return nil, err
 	}
 
-	clients := make(chan upnpClient, 3)
-	go func() {
-		var err error
-		ip1Clients, err := internetgateway2.NewWANIPConnection1ClientsByURL(ctx, u)
-		if err == nil && len(ip1Clients) > 0 {
-			clients <- ip1Clients[0]
-		}
-	}()
-	go func() {
-		ip2Clients, err := internetgateway2.NewWANIPConnection2ClientsByURL(ctx, u)
-		if err == nil && len(ip2Clients) > 0 {
-			clients <- ip2Clients[0]
-		}
-	}()
-	go func() {
-		ppp1Clients, err := internetgateway2.NewWANPPPConnection1ClientsByURL(ctx, u)
-		if err == nil && len(ppp1Clients) > 0 {
-			clients <- ppp1Clients[0]
+	ipp, err := netaddr.ParseIPPort(u.Host)
+	if err != nil {
+		return nil, fmt.Errorf("unexpected host %q in %q", u.Host, meta.Location)
+	}
+	if ipp.IP() != gw {
+		return nil, fmt.Errorf("UPnP discovered root %q does not match gateway IP %v; ignoring UPnP",
+			meta.Location, gw)
+	}
+
+	// We're fetching a smallish XML document over plain HTTP
+	// across the local LAN, without using DNS.  There should be
+	// very few round trips and low latency, so one second is a
+	// long time.
+	ctx, cancel := context.WithTimeout(ctx, time.Second)
+	defer cancel()
+
+	// This part does a network fetch.
+	root, err := goupnp.DeviceByURL(ctx, u)
+	if err != nil {
+		return nil, err
+	}
+
+	defer func() {
+		if client == nil {
+			return
 		}
+		logf("saw UPnP type %v at %v; %v (%v)",
+			strings.TrimPrefix(fmt.Sprintf("%T", client), "*internetgateway2."),
+			meta.Location, root.Device.FriendlyName, root.Device.Manufacturer)
 	}()
 
-	select {
-	case client := <-clients:
-		return client, nil
-	case <-ctx.Done():
-		return nil, ctx.Err()
+	// These parts don't do a network fetch.
+	// Pick the best service type available.
+	if cc, _ := internetgateway2.NewWANIPConnection2ClientsFromRootDevice(ctx, root, u); len(cc) > 0 {
+		return cc[0], nil
 	}
+	if cc, _ := internetgateway2.NewWANIPConnection1ClientsFromRootDevice(ctx, root, u); len(cc) > 0 {
+		return cc[0], nil
+	}
+	if cc, _ := internetgateway2.NewWANPPPConnection1ClientsFromRootDevice(ctx, root, u); len(cc) > 0 {
+		return cc[0], nil
+	}
+	return nil, nil
+}
+
+func (c *Client) upnpHTTPClientLocked() *http.Client {
+	if c.uPnPHTTPClient == nil {
+		c.uPnPHTTPClient = &http.Client{
+			Transport: &http.Transport{
+				DialContext:     netns.NewDialer().DialContext,
+				IdleConnTimeout: 2 * time.Second, // LAN is cheap
+			},
+		}
+	}
+	return c.uPnPHTTPClient
 }
 
 // getUPnPPortMapping attempts to create a port-mapping over the UPnP protocol. On success,
@@ -199,11 +247,17 @@ func (c *Client) getUPnPPortMapping(
 	var err error
 	c.mu.Lock()
 	oldMapping, ok := c.mapping.(*upnpMapping)
+	meta := c.uPnPMeta
+	httpClient := c.upnpHTTPClientLocked()
 	c.mu.Unlock()
 	if ok && oldMapping != nil {
 		client = oldMapping.client
 	} else {
-		client, err = getUPnPClient(ctx, gw)
+		ctx := goupnp.WithHTTPClient(ctx, httpClient)
+		client, err = getUPnPClient(ctx, c.logf, gw, meta)
+		if VerboseLogs {
+			c.logf("getUPnPClient: %T, %v", client, err)
+		}
 		if err != nil {
 			return netaddr.IPPort{}, false
 		}
@@ -221,11 +275,17 @@ func (c *Client) getUPnPPortMapping(
 		internal.IP().String(),
 		time.Second*pmpMapLifetimeSec,
 	)
+	if VerboseLogs {
+		c.logf("addAnyPortMapping: %v, %v", newPort, err)
+	}
 	if err != nil {
 		return netaddr.IPPort{}, false
 	}
 	// TODO cache this ip somewhere?
 	extIP, err := client.GetExternalIPAddress(ctx)
+	if VerboseLogs {
+		c.logf("client.GetExternalIPAddress: %v, %v", extIP, err)
+	}
 	if err != nil {
 		// TODO this doesn't seem right
 		return netaddr.IPPort{}, false
@@ -246,3 +306,18 @@ func (c *Client) getUPnPPortMapping(
 	c.localPort = newPort
 	return upnp.external, true
 }
+
+type uPnPDiscoResponse struct {
+	Location string
+}
+
+// parseUPnPDiscoResponse parses a UPnP HTTP-over-UDP discovery response.
+func parseUPnPDiscoResponse(body []byte) (uPnPDiscoResponse, error) {
+	var r uPnPDiscoResponse
+	res, err := http.ReadResponse(bufio.NewReaderSize(bytes.NewReader(body), 128), nil)
+	if err != nil {
+		return r, err
+	}
+	r.Location = res.Header.Get("Location")
+	return r, nil
+}