Skip to content

Commit

Permalink
net/portmapper: relax handling of UPnP resp (tailscale#6946)
Browse files Browse the repository at this point in the history
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 tailscale#5502

Signed-off-by: Jordan Whited <jordan@tailscale.com>
  • Loading branch information
jwhited authored and coadler committed Feb 2, 2023
1 parent 13574c6 commit d194994
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 5 deletions.
12 changes: 8 additions & 4 deletions net/portmapper/portmapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -799,7 +799,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)
Expand Down
5 changes: 4 additions & 1 deletion net/portmapper/upnp.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"context"
"fmt"
"math/rand"
"net"
"net/http"
"net/netip"
"net/url"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit d194994

Please sign in to comment.