From 66a61e1b32a30030b81c793e1ddd9ff04d6db49c Mon Sep 17 00:00:00 2001 From: julianknodt Date: Wed, 16 Jun 2021 11:53:23 -0700 Subject: [PATCH] Move upnp portmap to separate fn This isolates the upnp portmapping to another function Signed-off-by: julianknodt --- cmd/tailscale/depaware.txt | 12 ++--- cmd/tailscaled/depaware.txt | 14 +++--- net/netcheck/netcheck.go | 3 +- net/portmapper/portmapper.go | 68 +++++++-------------------- net/portmapper/portmapper_test.go | 3 +- net/portmapper/probe.go | 57 +++++++++++++---------- net/portmapper/upnp.go | 76 ++++++++++++++++++++++++++++++- 7 files changed, 139 insertions(+), 94 deletions(-) diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 6b345e532..c2a2b9b32 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -36,16 +36,16 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep tailscale.com/net/tlsdial from tailscale.com/derp/derphttp tailscale.com/net/tsaddr from tailscale.com/net/interfaces+ 💣 tailscale.com/net/tshttpproxy from tailscale.com/derp/derphttp+ - tailscale.com/net/upnp from tailscale.com/net/upnp/dcps/internetgateway2 - tailscale.com/net/upnp/dcps/internetgateway2 from tailscale.com/net/portmapper - tailscale.com/net/upnp/httpu from tailscale.com/net/upnp - tailscale.com/net/upnp/scpd from tailscale.com/net/upnp - tailscale.com/net/upnp/soap from tailscale.com/net/upnp+ - tailscale.com/net/upnp/ssdp from tailscale.com/net/upnp tailscale.com/paths from tailscale.com/cmd/tailscale/cli+ tailscale.com/safesocket from tailscale.com/cmd/tailscale/cli+ tailscale.com/syncs from tailscale.com/net/interfaces+ tailscale.com/tailcfg from tailscale.com/cmd/tailscale/cli+ + tailscale.com/tempfork/upnp from tailscale.com/tempfork/upnp/dcps/internetgateway2 + tailscale.com/tempfork/upnp/dcps/internetgateway2 from tailscale.com/net/portmapper + tailscale.com/tempfork/upnp/httpu from tailscale.com/tempfork/upnp + tailscale.com/tempfork/upnp/scpd from tailscale.com/tempfork/upnp + tailscale.com/tempfork/upnp/soap from tailscale.com/tempfork/upnp+ + tailscale.com/tempfork/upnp/ssdp from tailscale.com/tempfork/upnp W tailscale.com/tsconst from tailscale.com/net/interfaces tailscale.com/types/empty from tailscale.com/ipn tailscale.com/types/ipproto from tailscale.com/net/flowtrack+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 8f84a8b35..c091bbf86 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -112,18 +112,18 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/net/tsaddr from tailscale.com/ipn/ipnlocal+ 💣 tailscale.com/net/tshttpproxy from tailscale.com/control/controlclient+ tailscale.com/net/tstun from tailscale.com/cmd/tailscaled+ - tailscale.com/net/upnp from tailscale.com/net/upnp/dcps/internetgateway2 - tailscale.com/net/upnp/dcps/internetgateway2 from tailscale.com/net/portmapper - tailscale.com/net/upnp/httpu from tailscale.com/net/upnp - tailscale.com/net/upnp/scpd from tailscale.com/net/upnp - tailscale.com/net/upnp/soap from tailscale.com/net/upnp+ - tailscale.com/net/upnp/ssdp from tailscale.com/net/upnp tailscale.com/paths from tailscale.com/cmd/tailscaled+ tailscale.com/portlist from tailscale.com/ipn/ipnlocal tailscale.com/safesocket from tailscale.com/ipn/ipnserver tailscale.com/smallzstd from tailscale.com/ipn/ipnserver+ tailscale.com/syncs from tailscale.com/net/interfaces+ tailscale.com/tailcfg from tailscale.com/control/controlclient+ + tailscale.com/tempfork/upnp from tailscale.com/tempfork/upnp/dcps/internetgateway2 + tailscale.com/tempfork/upnp/dcps/internetgateway2 from tailscale.com/net/portmapper + tailscale.com/tempfork/upnp/httpu from tailscale.com/tempfork/upnp + tailscale.com/tempfork/upnp/scpd from tailscale.com/tempfork/upnp + tailscale.com/tempfork/upnp/soap from tailscale.com/tempfork/upnp+ + tailscale.com/tempfork/upnp/ssdp from tailscale.com/tempfork/upnp W tailscale.com/tsconst from tailscale.com/net/interfaces tailscale.com/tstime from tailscale.com/wgengine/magicsock tailscale.com/types/empty from tailscale.com/control/controlclient+ @@ -238,7 +238,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de encoding/hex from crypto/x509+ encoding/json from expvar+ encoding/pem from crypto/tls+ - encoding/xml from tailscale.com/net/upnp+ + encoding/xml from tailscale.com/tempfork/upnp+ errors from bufio+ expvar from tailscale.com/derp+ flag from tailscale.com/cmd/tailscaled+ diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go index 084467bd1..84ecd79bc 100644 --- a/net/netcheck/netcheck.go +++ b/net/netcheck/netcheck.go @@ -693,8 +693,7 @@ func (rs *reportState) probePortMapServices() { rs.setOptBool(&rs.report.PMP, false) rs.setOptBool(&rs.report.PCP, false) - rs.c.PortMapper.NewProber(context.Background()) - res, err := rs.c.PortMapper.Prober.StatusBlock() + res, err := rs.c.PortMapper.Probe(context.Background()) if err != nil { rs.c.logf("probePortMapServices: %v", err) return diff --git a/net/portmapper/portmapper.go b/net/portmapper/portmapper.go index a9a0b6e82..9a6a57e7b 100644 --- a/net/portmapper/portmapper.go +++ b/net/portmapper/portmapper.go @@ -58,12 +58,18 @@ type Client struct { localPort uint16 - mapping Mapping // non-nil if we have a mapping + mapping // non-nil if we have a mapping - Prober *Prober + // Prober is this portmappers stateful mechanism for detecting when portmapping services are + // available on the current network. It is exposed so that clients can pause or stop probing. + // In order to create a prober, either call `Probe()` or `NewProber()`, which will populate + // this field. + *Prober } -type Mapping interface { +// Mapping represents a created port-mapping over some protocol. It specifies a lease duration, +// how to release the mapping, and whether the map is still valid. +type mapping interface { isCurrent() bool release() validUntil() time.Time @@ -252,6 +258,11 @@ var ( ErrGatewayNotFound = errors.New("failed to look up gateway address") ) +// Probe starts a periodic probe and blocks until the first result of probing. +func (c *Client) Probe(ctx context.Context) (ProbeResult, error) { + return c.NewProber(ctx).StatusBlock() +} + // CreateOrGetMapping either creates a new mapping or returns a cached // valid one. // @@ -265,9 +276,10 @@ func (c *Client) CreateOrGetMapping(ctx context.Context) (external netaddr.IPPor c.mu.Lock() localPort := c.localPort + internalAddr := netaddr.IPPortFrom(myIP, localPort) m := &pmpMapping{ gw: gw, - internal: netaddr.IPPortFrom(myIP, localPort), + internal: internalAddr, } // prevPort is the port we had most previously, if any. We try @@ -368,53 +380,7 @@ func (c *Client) CreateOrGetMapping(ctx context.Context) (external netaddr.IPPor } } - // If did not see UPnP within the past 5 seconds then bail - haveRecentUPnP := c.sawUPnPRecently() - if c.lastProbe.After(now.Add(-5*time.Second)) && !haveRecentUPnP { - return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} - } - // Otherwise try a uPnP mapping if PMP did not work - mpnp := &upnpMapping{ - gw: m.gw, - internal: m.internal, - } - - var client upnpClient - c.mu.Lock() - oldMapping, ok := c.mapping.(*upnpMapping) - c.mu.Unlock() - if ok { - client = oldMapping.client - } else if c.Prober != nil && c.Prober.upnpClient != nil { - client = c.Prober.upnpClient - } else { - client, err = getUPnPClient(ctx) - if err != nil { - return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} - } - } - if client == nil { - return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} - } - - var newPort uint16 - newPort, err = AddAnyPortMapping( - ctx, client, - "", prevPort, "UDP", localPort, m.internal.IP().String(), true, - "tailscale-portfwd", pmpMapLifetimeSec, - ) - if err != nil { - return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} - } - mpnp.external = netaddr.IPPortFrom(gw, newPort) - d := time.Duration(pmpMapLifetimeSec) * time.Second / 2 - mpnp.useUntil = time.Now().Add(d) - mpnp.client = client - c.mu.Lock() - defer c.mu.Unlock() - c.mapping = mpnp - c.localPort = newPort - return mpnp.external, nil + return c.getUPnPPortMapping(ctx, gw, internalAddr, prevPort) } type pmpResultCode uint16 diff --git a/net/portmapper/portmapper_test.go b/net/portmapper/portmapper_test.go index b9c2a5935..ed447e597 100644 --- a/net/portmapper/portmapper_test.go +++ b/net/portmapper/portmapper_test.go @@ -48,8 +48,7 @@ func TestClientProbeThenMap(t *testing.T) { } c := NewClient(t.Logf) c.SetLocalPort(1234) - c.NewProber(context.Background()) - res, err := c.Prober.StatusBlock() + res, err := c.Probe(context.Background()) t.Logf("Probe: %+v, %v", res, err) ext, err := c.CreateOrGetMapping(context.Background()) t.Logf("CreateOrGetMapping: %v, %v", ext, err) diff --git a/net/portmapper/probe.go b/net/portmapper/probe.go index 5988e3efc..a8ef5cc03 100644 --- a/net/portmapper/probe.go +++ b/net/portmapper/probe.go @@ -12,25 +12,34 @@ import ( "tailscale.com/net/netns" ) +// Prober periodically pings the network and checks for port-mapping services. type Prober struct { // pause signals the probe to either pause temporarily (true), or stop entirely (false) // to restart the probe, send another pause to it. pause chan<- bool - PMP *ProbeSubResult - PCP *ProbeSubResult + // Each of the SubResults below is intended to expose whether a specific service is available + // for use on a client, and the most recent seen time. Should not be modified externally, and + // will be periodically updated. + // PMP stores the result of probing pmp services and is populated by prober. + PMP ProbeSubResult + // PCP stores the result of probing pcp services and is populated by prober. + PCP ProbeSubResult + + // upnpClient is a reused upnpClient for probing upnp results. upnpClient upnpClient - UPnP *ProbeSubResult + // PCP stores the result of probing pcp services and is populated by prober. + UPnP ProbeSubResult } // NewProber creates a new prober for a given client. -func (c *Client) NewProber(ctx context.Context) (p *Prober) { +func (c *Client) NewProber(ctx context.Context) *Prober { if c.Prober != nil { return c.Prober } pause := make(chan bool) - p = &Prober{ + p := &Prober{ pause: pause, PMP: NewProbeSubResult(), @@ -41,8 +50,8 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) { go func() { for { - pmp_ctx, cancel := context.WithTimeout(ctx, portMapServiceTimeout) - hasPCP, hasPMP, err := c.probePMPAndPCP(pmp_ctx) + pmpCtx, cancel := context.WithTimeout(ctx, portMapServiceTimeout) + hasPCP, hasPMP, err := c.probePMPAndPCP(pmpCtx) if err != nil { if ctx.Err() == context.DeadlineExceeded { err = nil @@ -50,7 +59,7 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) { cancel() return } - if pmp_ctx.Err() == context.DeadlineExceeded { + if pmpCtx.Err() == context.DeadlineExceeded { err = nil } } @@ -92,19 +101,19 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) { }() // TODO maybe do something fancy/dynamic with more delay (exponential back-off) for { - upnp_ctx, cancel := context.WithTimeout(ctx, portMapServiceTimeout*5) + upnpCtx, cancel := context.WithTimeout(ctx, portMapServiceTimeout*5) retries := 0 hasUPnP := false const num_connect_retries = 5 for retries < num_connect_retries { - status, _, _, statusErr := p.upnpClient.GetStatusInfo(upnp_ctx) + status, _, _, statusErr := p.upnpClient.GetStatusInfo(upnpCtx) if statusErr != nil { err = statusErr break } hasUPnP = hasUPnP || status == "Connected" if status == "Disconnected" { - upnpClient.RequestConnection(upnp_ctx) + upnpClient.RequestConnection(upnpCtx) } retries += 1 } @@ -115,7 +124,7 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) { cancel() return } - if upnp_ctx.Err() == context.DeadlineExceeded { + if upnpCtx.Err() == context.DeadlineExceeded { err = nil } cancel() @@ -139,18 +148,14 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) { } }() - return + return p } -// Stop gracefully turns the Prober off. -func (p *Prober) Stop() { - close(p.pause) -} +// Stop gracefully turns the Prober off, completing the current probes before exiting. +func (p *Prober) Stop() { close(p.pause) } -// Pauses the prober if currently running, or starts if it was previously paused -func (p *Prober) Toggle() { - p.pause <- true -} +// Pauses the prober if currently running, or starts if it was previously paused. +func (p *Prober) Toggle() { p.pause <- true } // CurrentStatus returns the current results of the prober, regardless of whether they have // completed or not. @@ -173,6 +178,7 @@ func (p *Prober) CurrentStatus() (res ProbeResult, err error) { return } +// Blocks until the current probe gets any result. func (p *Prober) StatusBlock() (res ProbeResult, err error) { hasPMP, errPMP := p.PMP.PresentBlock() res.PMP = hasPMP @@ -192,6 +198,7 @@ func (p *Prober) StatusBlock() (res ProbeResult, err error) { return } +// ProbeSubResult is a result for a single probing service. type ProbeSubResult struct { cond *sync.Cond // If this probe has finished, regardless of success or failure @@ -202,12 +209,12 @@ type ProbeSubResult struct { // most recent error err error - // time we last saw it to be available. + // Time we last saw the service to be available. sawTime time.Time } -func NewProbeSubResult() *ProbeSubResult { - return &ProbeSubResult{ +func NewProbeSubResult() ProbeSubResult { + return ProbeSubResult{ cond: &sync.Cond{ L: &sync.Mutex{}, }, @@ -232,6 +239,8 @@ func (psr *ProbeSubResult) PresentCurrent() (bool, error) { return present, psr.err } +// Assigns the result of the probe and any error seen, signalling to any items waiting for this +// result that it is now available. func (psr *ProbeSubResult) Set(present bool, err error) { saw := time.Now() psr.cond.L.Lock() diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index fb9d660a6..b3c527296 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -43,14 +43,18 @@ type upnpClient interface { newLeaseDuration uint32, ) (err error) - DeletePortMapping(ctx context.Context, NewRemoteHost string, NewExternalPort uint16, NewProtocol string) error + DeletePortMapping(ctx context.Context, newRemoteHost string, newExternalPort uint16, newProtocol string) error GetStatusInfo(ctx context.Context) (status string, lastErr string, uptime uint32, err error) + GetExternalIPAddress(ctx context.Context) (externalIPAddress string, err error) RequestTermination(ctx context.Context) error RequestConnection(ctx context.Context) error } -func AddAnyPortMapping( +// addAnyPortMapping abstracts over different UPnP client connections, calling the available +// AddAnyPortMapping call if available, otherwise defaulting to the old behavior of calling +// AddPortMapping with port = 0 to specify a wildcard port. +func addAnyPortMapping( ctx context.Context, upnp upnpClient, newRemoteHost string, @@ -94,6 +98,7 @@ func AddAnyPortMapping( // Adapted from https://github.com/huin/goupnp/blob/master/GUIDE.md. func getUPnPClient(ctx context.Context) (upnpClient, error) { tasks, _ := errgroup.WithContext(ctx) + // Attempt to connect over the multiple available connection types. var ip1Clients []*internetgateway2.WANIPConnection1 tasks.Go(func() error { var err error @@ -128,3 +133,70 @@ func getUPnPClient(ctx context.Context) (upnpClient, error) { return nil, err } } + +// getUPnPPortMapping will attempt to create a port-mapping over the UPnP protocol. On success, +// it will return the externally exposed IP and port. Otherwise, it will return a zeroed IP and +// port and an error. +func (c *Client) getUPnPPortMapping(ctx context.Context, gw netaddr.IP, internal netaddr.IPPort, + prevPort uint16) (external netaddr.IPPort, err error) { + // If did not see UPnP within the past 5 seconds then bail + haveRecentUPnP := c.sawUPnPRecently() + now := time.Now() + if c.lastProbe.After(now.Add(-5*time.Second)) && !haveRecentUPnP { + return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} + } + // Otherwise try a uPnP mapping if PMP did not work + mpnp := &upnpMapping{ + gw: gw, + internal: internal, + } + + var client upnpClient + c.mu.Lock() + oldMapping, ok := c.mapping.(*upnpMapping) + c.mu.Unlock() + if ok && oldMapping != nil { + client = oldMapping.client + } else if c.Prober != nil && c.Prober.upnpClient != nil { + client = c.Prober.upnpClient + } else { + client, err = getUPnPClient(ctx) + if err != nil { + return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} + } + } + if client == nil { + return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} + } + + var newPort uint16 + newPort, err = addAnyPortMapping( + ctx, client, + "", prevPort, "UDP", internal.Port(), internal.IP().String(), true, + // string below is just a name for reporting on device. + "tailscale-portmap", pmpMapLifetimeSec, + ) + if err != nil { + return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} + } + // TODO cache this ip somewhere? + extIP, err := client.GetExternalIPAddress(ctx) + if err != nil { + // TODO this doesn't seem right + return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} + } + externalIP, err := netaddr.ParseIP(extIP) + if err != nil { + return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices} + } + + mpnp.external = netaddr.IPPortFrom(externalIP, newPort) + d := time.Duration(pmpMapLifetimeSec) * time.Second / 2 + mpnp.useUntil = time.Now().Add(d) + mpnp.client = client + c.mu.Lock() + defer c.mu.Unlock() + c.mapping = mpnp + c.localPort = newPort + return mpnp.external, nil +}