From 25a0091f69880169ef8359f6b59bf2c5d737f03d Mon Sep 17 00:00:00 2001 From: Jordan Whited Date: Thu, 12 Jan 2023 16:57:02 -0800 Subject: [PATCH] net/portmapper: relax handling of UPnP resp (#6946) Gateway devices operating as an HA pair w/VRRP or CARP may send UPnP replies from static addresses rather than the floating gateway address. This commit relaxes our source address verification such that we parse responses from non-gateway IPs, and re-point the UPnP root desc URL to the gateway IP. This ensures we are still interfacing with the gateway device (assuming L2 security intact), even though we got a root desc from a non-gateway address. This relaxed handling is required for ANY port mapping to work on certain OPNsense/pfsense distributions using CARP at the time of writing, as miniupnpd may only listen on the static, non-gateway interface address for PCP and PMP. Fixes #5502 Signed-off-by: Jordan Whited --- net/portmapper/portmapper.go | 12 ++++++++---- net/portmapper/upnp.go | 5 ++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/net/portmapper/portmapper.go b/net/portmapper/portmapper.go index 2eb06a026..9b0ec5ca7 100644 --- a/net/portmapper/portmapper.go +++ b/net/portmapper/portmapper.go @@ -100,11 +100,11 @@ type mapping interface { // but can be called asynchronously. Release should be idempotent, and thus even if called // multiple times should not cause additional side-effects. Release(context.Context) - // goodUntil will return the lease time that the mapping is valid for. + // GoodUntil will return the lease time that the mapping is valid for. GoodUntil() time.Time - // renewAfter returns the earliest time that the mapping should be renewed. + // RenewAfter returns the earliest time that the mapping should be renewed. RenewAfter() time.Time - // externalIPPort indicates what port the mapping can be reached from on the outside. + // External indicates what port the mapping can be reached from on the outside. External() netip.AddrPort } @@ -797,7 +797,11 @@ func (c *Client) Probe(ctx context.Context) (res ProbeResult, err error) { switch port { case c.upnpPort(): metricUPnPResponse.Add(1) - if ip == gw && mem.Contains(mem.B(buf[:n]), mem.S(":InternetGatewayDevice:")) { + if mem.Contains(mem.B(buf[:n]), mem.S(":InternetGatewayDevice:")) { + if ip != gw { + // https://github.com/tailscale/tailscale/issues/5502 + c.logf("UPnP discovery response from %v, but gateway IP is %v", ip, gw) + } meta, err := parseUPnPDiscoResponse(buf[:n]) if err != nil { metricUPnPParseErr.Add(1) diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index b59ec4f40..ceee6c8f3 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -14,6 +14,7 @@ import ( "context" "fmt" "math/rand" + "net" "net/http" "net/netip" "net/url" @@ -174,8 +175,10 @@ func getUPnPClient(ctx context.Context, logf logger.Logf, gw netip.Addr, meta uP return nil, fmt.Errorf("unexpected host %q in %q", u.Host, meta.Location) } if ipp.Addr() != gw { - return nil, fmt.Errorf("UPnP discovered root %q does not match gateway IP %v; ignoring UPnP", + // https://github.com/tailscale/tailscale/issues/5502 + logf("UPnP discovered root %q does not match gateway IP %v; repointing at gateway which is assumed to be floating", meta.Location, gw) + u.Host = net.JoinHostPort(gw.String(), u.Port()) } // We're fetching a smallish XML document over plain HTTP