From 5e0b58861860b1f2abd4f78cad5ad060e12ebb04 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 2 Aug 2021 22:09:50 -0700 Subject: [PATCH] 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 --- cmd/tailscaled/debug.go | 2 + go.mod | 2 +- go.sum | 4 +- net/portmapper/disabled_stubs.go | 4 - net/portmapper/portmapper.go | 59 ++++++++----- net/portmapper/upnp.go | 139 +++++++++++++++++++++++-------- net/portmapper/upnp_test.go | 95 +++++++++++++++++++++ 7 files changed, 239 insertions(+), 66 deletions(-) create mode 100644 net/portmapper/upnp_test.go diff --git a/cmd/tailscaled/debug.go b/cmd/tailscaled/debug.go index bf9842eed..7162b79f9 100644 --- a/cmd/tailscaled/debug.go +++ b/cmd/tailscaled/debug.go @@ -206,6 +206,8 @@ func debugPortmap(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, 3*time.Second) defer cancel() + portmapper.VerboseLogs = true + done := make(chan bool, 1) var c *portmapper.Client diff --git a/go.mod b/go.mod index e3f7c91f6..ab84f5245 100644 --- a/go.mod +++ b/go.mod @@ -31,7 +31,7 @@ require ( github.com/pkg/sftp v1.13.0 github.com/tailscale/certstore v0.0.0-20210528134328-066c94b793d3 github.com/tailscale/depaware v0.0.0-20201214215404-77d1e9757027 - github.com/tailscale/goupnp v1.0.1-0.20210710010003-1cf2d718bbb2 + github.com/tailscale/goupnp v1.0.1-0.20210804011211-c64d0f06ea05 github.com/tailscale/hujson v0.0.0-20200924210142-dde312d0d6a2 github.com/tcnksm/go-httpstat v0.2.0 github.com/toqueteos/webbrowser v1.2.0 diff --git a/go.sum b/go.sum index edfa05cc6..3a8f27dc8 100644 --- a/go.sum +++ b/go.sum @@ -581,8 +581,8 @@ github.com/tailscale/certstore v0.0.0-20210528134328-066c94b793d3 h1:fEubocuQkrl github.com/tailscale/certstore v0.0.0-20210528134328-066c94b793d3/go.mod h1:2P+hpOwd53e7JMX/L4f3VXkv1G+33ES6IWZSrkIeWNs= github.com/tailscale/depaware v0.0.0-20201214215404-77d1e9757027 h1:lK99QQdH3yBWY6aGilF+IRlQIdmhzLrsEmF6JgN+Ryw= github.com/tailscale/depaware v0.0.0-20201214215404-77d1e9757027/go.mod h1:p9lPsd+cx33L3H9nNoecRRxPssFKUwwI50I3pZ0yT+8= -github.com/tailscale/goupnp v1.0.1-0.20210710010003-1cf2d718bbb2 h1:AIJ8AF9O7jBmCwilP0ydwJMIzW5dw48Us8f3hLJhYBY= -github.com/tailscale/goupnp v1.0.1-0.20210710010003-1cf2d718bbb2/go.mod h1:PdCqy9JzfWMJf1H5UJW2ip33/d4YkoKN0r67yKH1mG8= +github.com/tailscale/goupnp v1.0.1-0.20210804011211-c64d0f06ea05 h1:4chzWmimtJPxRs2O36yuGRW3f9SYV+bMTTvMBI0EKio= +github.com/tailscale/goupnp v1.0.1-0.20210804011211-c64d0f06ea05/go.mod h1:PdCqy9JzfWMJf1H5UJW2ip33/d4YkoKN0r67yKH1mG8= github.com/tailscale/hujson v0.0.0-20200924210142-dde312d0d6a2 h1:reREUgl2FG+o7YCsrZB8XLjnuKv5hEIWtnOdAbRAXZI= github.com/tailscale/hujson v0.0.0-20200924210142-dde312d0d6a2/go.mod h1:STqf+YV0ADdzk4ejtXFsGqDpATP9JoL0OB+hiFQbkdE= github.com/tcnksm/go-httpstat v0.2.0 h1:rP7T5e5U2HfmOBmZzGgGZjBQ5/GluWUylujl0tJ04I0= diff --git a/net/portmapper/disabled_stubs.go b/net/portmapper/disabled_stubs.go index fb1572e78..f7003b0ed 100644 --- a/net/portmapper/disabled_stubs.go +++ b/net/portmapper/disabled_stubs.go @@ -15,10 +15,6 @@ import ( type upnpClient interface{} -func getUPnPClient(ctx context.Context, gw netaddr.IP) (upnpClient, error) { - return nil, nil -} - func (c *Client) getUPnPPortMapping( ctx context.Context, gw netaddr.IP, diff --git a/net/portmapper/portmapper.go b/net/portmapper/portmapper.go index e9ddf27c9..595ad2e08 100644 --- a/net/portmapper/portmapper.go +++ b/net/portmapper/portmapper.go @@ -14,9 +14,11 @@ import ( "fmt" "io" "net" + "net/http" "sync" "time" + "go4.org/mem" "inet.af/netaddr" "tailscale.com/net/interfaces" "tailscale.com/net/netns" @@ -62,8 +64,11 @@ type Client struct { pmpPubIPTime time.Time // time pmpPubIP last verified pmpLastEpoch uint32 - pcpSawTime time.Time // time we last saw PCP was available - uPnPSawTime time.Time // time we last saw UPnP was available + pcpSawTime time.Time // time we last saw PCP was available + + uPnPSawTime time.Time // time we last saw UPnP was available + uPnPMeta uPnPDiscoResponse // Location header from UPnP UDP discovery response + uPnPHTTPClient *http.Client // nil until needed localPort uint16 @@ -560,27 +565,9 @@ func (c *Client) Probe(ctx context.Context) (res ProbeResult, err error) { defer cancel() defer closeCloserOnContextDone(ctx, uc)() - if c.sawUPnPRecently() { - res.UPnP = true - } else { - hasUPnP := make(chan bool, 1) - defer func() { - res.UPnP = <-hasUPnP - }() - go func() { - client, err := getUPnPClient(ctx, gw) - if err == nil && client != nil { - hasUPnP <- true - c.mu.Lock() - c.uPnPSawTime = time.Now() - c.mu.Unlock() - } - close(hasUPnP) - }() - } - pcpAddr := netaddr.IPPortFrom(gw, pcpPort).UDPAddr() pmpAddr := netaddr.IPPortFrom(gw, pmpPort).UDPAddr() + upnpAddr := netaddr.IPPortFrom(gw, upnpPort).UDPAddr() // Don't send probes to services that we recently learned (for // the same gw/myIP) are available. See @@ -595,11 +582,16 @@ func (c *Client) Probe(ctx context.Context) (res ProbeResult, err error) { } else { uc.WriteTo(pcpAnnounceRequest(myIP), pcpAddr) } + if c.sawUPnPRecently() { + res.UPnP = true + } else { + uc.WriteTo(uPnPPacket, upnpAddr) + } buf := make([]byte, 1500) pcpHeard := false // true when we get any PCP response for { - if pcpHeard && res.PMP { + if pcpHeard && res.PMP && res.UPnP { // Nothing more to discover. return res, nil } @@ -612,6 +604,19 @@ func (c *Client) Probe(ctx context.Context) (res ProbeResult, err error) { } port := addr.(*net.UDPAddr).Port switch port { + case upnpPort: + if mem.Contains(mem.B(buf[:n]), mem.S(":InternetGatewayDevice:")) { + meta, err := parseUPnPDiscoResponse(buf[:n]) + if err != nil { + c.logf("unrecognized UPnP discovery response; ignoring") + } + // log.Printf("UPnP reply %+v, %q", meta, buf[:n]) + res.UPnP = true + c.mu.Lock() + c.uPnPSawTime = time.Now() + c.uPnPMeta = meta + c.mu.Unlock() + } case pcpPort: // same as pmpPort if pres, ok := parsePCPResponse(buf[:n]); ok { if pres.OpCode == pcpOpReply|pcpOpAnnounce { @@ -724,3 +729,13 @@ func parsePCPResponse(b []byte) (res pcpResponse, ok bool) { } var pmpReqExternalAddrPacket = []byte{0, 0} // version 0, opcode 0 = "Public address request" + +const ( + upnpPort = 1900 +) + +var uPnPPacket = []byte("M-SEARCH * HTTP/1.1\r\n" + + "HOST: 239.255.255.250:1900\r\n" + + "ST: ssdp:all\r\n" + + "MAN: \"ssdp:discover\"\r\n" + + "MX: 2\r\n\r\n") diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index 5ec91353e..9d3deb414 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -8,17 +8,27 @@ package portmapper import ( + "bufio" + "bytes" "context" "fmt" + "log" "math/rand" + "net/http" "net/url" "time" + "github.com/tailscale/goupnp" "github.com/tailscale/goupnp/dcps/internetgateway2" "inet.af/netaddr" "tailscale.com/control/controlknobs" + "tailscale.com/net/netns" ) +// VerboseLogs controls verbose debug logging. +// It exists for use by "tailscaled debug --portmap". +var VerboseLogs bool + // References: // // WANIP Connection v2: http://upnp.org/specs/gw/UPnP-gw-WANIPConnection-v2-Service.pdf @@ -44,7 +54,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 +88,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 +103,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 +143,76 @@ 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, gw netaddr.IP, meta uPnPDiscoResponse) (upnpClient, 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 { + log.Printf("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] - } - }() - - select { - case client := <-clients: - return client, nil - case <-ctx.Done(): - return nil, ctx.Err() + 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) + } + + ctx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + defer cancel() + + // This part does a network fetch. + root, err := goupnp.DeviceByURL(ctx, u) + if err != nil { + return nil, 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 +237,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, gw, meta) + if VerboseLogs { + log.Printf("getUPnPClient: %T, %v", client, err) + } if err != nil { return netaddr.IPPort{}, false } @@ -221,11 +265,17 @@ func (c *Client) getUPnPPortMapping( internal.IP().String(), time.Second*pmpMapLifetimeSec, ) + if VerboseLogs { + log.Printf("addAnyPortMapping: %v, %v", newPort, err) + } if err != nil { return netaddr.IPPort{}, false } // TODO cache this ip somewhere? extIP, err := client.GetExternalIPAddress(ctx) + if VerboseLogs { + log.Printf("client.GetExternalIPAddress: %v, %v", extIP, err) + } if err != nil { // TODO this doesn't seem right return netaddr.IPPort{}, false @@ -246,3 +296,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 +} diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go new file mode 100644 index 000000000..972ec50d9 --- /dev/null +++ b/net/portmapper/upnp_test.go @@ -0,0 +1,95 @@ +// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package portmapper + +import ( + "context" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "reflect" + "testing" + + "inet.af/netaddr" +) + +// Google Wifi +const ( + googleWifiUPnPDisco = "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=120\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:2\r\nUSN: uuid:a9708184-a6c0-413a-bbac-11bcf7e30ece::urn:schemas-upnp-org:device:InternetGatewayDevice:2\r\nEXT:\r\nSERVER: Linux/5.4.0-1034-gcp UPnP/1.1 MiniUPnPd/1.9\r\nLOCATION: http://192.168.86.1:5000/rootDesc.xml\r\nOPT: \"http://schemas.upnp.org/upnp/1/0/\"; ns=01\r\n01-NLS: 1\r\nBOOTID.UPNP.ORG: 1\r\nCONFIGID.UPNP.ORG: 1337\r\n\r\n" + + googleWifiRootDescXML = ` +10urn:schemas-upnp-org:device:InternetGatewayDevice:2OnHubGooglehttp://google.com/Wireless RouterOnHub1https://on.google.com/hub/00000000uuid:a9708184-a6c0-413a-bbac-11bcf7e30eceurn:schemas-upnp-org:service:Layer3Forwarding:1urn:upnp-org:serviceId:Layer3Forwarding1/ctl/L3F/evt/L3F/L3F.xmlurn:schemas-upnp-org:service:DeviceProtection:1urn:upnp-org:serviceId:DeviceProtection1/ctl/DP/evt/DP/DP.xmlurn:schemas-upnp-org:device:WANDevice:2WANDeviceMiniUPnPhttp://miniupnp.free.fr/WAN DeviceWAN Device20210414http://miniupnp.free.fr/00000000uuid:a9708184-a6c0-413a-bbac-11bcf7e30ecf000000000000urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1urn:upnp-org:serviceId:WANCommonIFC1/ctl/CmnIfCfg/evt/CmnIfCfg/WANCfg.xmlurn:schemas-upnp-org:device:WANConnectionDevice:2WANConnectionDeviceMiniUPnPhttp://miniupnp.free.fr/MiniUPnP daemonMiniUPnPd20210414http://miniupnp.free.fr/00000000uuid:a9708184-a6c0-413a-bbac-11bcf7e30ec0000000000000urn:schemas-upnp-org:service:WANIPConnection:2urn:upnp-org:serviceId:WANIPConn1/ctl/IPConn/evt/IPConn/WANIPCn.xmlhttp://testwifi.here/` +) + +// pfSense 2.5.0-RELEASE / FreeBSD 12.2-STABLE +const ( + pfSenseUPnPDisco = "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=120\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nUSN: uuid:bee7052b-49e8-3597-b545-55a1e38ac11::urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nEXT:\r\nSERVER: FreeBSD/12.2-STABLE UPnP/1.1 MiniUPnPd/2.2.1\r\nLOCATION: http://192.168.1.1:2189/rootDesc.xml\r\nOPT: \"http://schemas.upnp.org/upnp/1/0/\"; ns=01\r\n01-NLS: 1627958564\r\nBOOTID.UPNP.ORG: 1627958564\r\nCONFIGID.UPNP.ORG: 1337\r\n\r\n" + + pfSenseRootDescXML = ` +11urn:schemas-upnp-org:device:InternetGatewayDevice:1FreeBSD routerFreeBSDhttp://www.freebsd.org/FreeBSD routerFreeBSD router2.5.0-RELEASEhttp://www.freebsd.org/BEE7052Buuid:bee7052b-49e8-3597-b545-55a1e38ac11urn:schemas-upnp-org:service:Layer3Forwarding:1urn:upnp-org:serviceId:L3Forwarding1/L3F.xml/ctl/L3F/evt/L3Furn:schemas-upnp-org:device:WANDevice:1WANDeviceMiniUPnPhttp://miniupnp.free.fr/WAN DeviceWAN Device20210205http://miniupnp.free.fr/BEE7052Buuid:bee7052b-49e8-3597-b545-55a1e38ac12000000000000urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1urn:upnp-org:serviceId:WANCommonIFC1/WANCfg.xml/ctl/CmnIfCfg/evt/CmnIfCfgurn:schemas-upnp-org:device:WANConnectionDevice:1WANConnectionDeviceMiniUPnPhttp://miniupnp.free.fr/MiniUPnP daemonMiniUPnPd20210205http://miniupnp.free.fr/BEE7052Buuid:bee7052b-49e8-3597-b545-55a1e38ac13000000000000urn:schemas-upnp-org:service:WANIPConnection:1urn:upnp-org:serviceId:WANIPConn1/WANIPCn.xml/ctl/IPConn/evt/IPConnhttps://192.168.1.1/` +) + +func TestParseUPnPDiscoResponse(t *testing.T) { + tests := []struct { + name string + headers string + want uPnPDiscoResponse + }{ + {"google", googleWifiUPnPDisco, uPnPDiscoResponse{ + Location: "http://192.168.86.1:5000/rootDesc.xml", + }}, + {"pfsense", pfSenseUPnPDisco, uPnPDiscoResponse{ + Location: "http://192.168.1.1:2189/rootDesc.xml", + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseUPnPDiscoResponse([]byte(tt.headers)) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("unexpected result:\n got: %+v\nwant: %+v\n", got, tt.want) + } + }) + } +} + +func TestGetUPnPClient(t *testing.T) { + tests := []struct { + name string + xmlBody string + want string + }{ + {"google", googleWifiRootDescXML, "*internetgateway2.WANIPConnection2"}, + {"pfsense", pfSenseRootDescXML, "*internetgateway2.WANIPConnection1"}, + // TODO(bradfitz): find a PPP one in the wild + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.RequestURI == "/rootDesc.xml" { + io.WriteString(w, tt.xmlBody) + return + } + http.NotFound(w, r) + })) + defer ts.Close() + gw, _ := netaddr.FromStdIP(ts.Listener.Addr().(*net.TCPAddr).IP) + c, err := getUPnPClient(context.Background(), gw, uPnPDiscoResponse{ + Location: ts.URL + "/rootDesc.xml", + }) + if err != nil { + t.Fatal(err) + } + got := fmt.Sprintf("%T", c) + if got != tt.want { + t.Errorf("got %v; want %v", got, tt.want) + } + }) + } +}