Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UPnP port map fails on Mikrotik CHR v7.10 with a UPnPError #8364

Open
tylerjwatson opened this issue Jun 17, 2023 · 18 comments
Open

UPnP port map fails on Mikrotik CHR v7.10 with a UPnPError #8364

tylerjwatson opened this issue Jun 17, 2023 · 18 comments
Labels
bug Bug L1 Very few Likelihood P1 Nuisance Priority level portmap Pertaining to NAT-PMP, PCP, UPnP T5 Usability Issue type

Comments

@tylerjwatson
Copy link

What is the issue?

UPNP fails to map ports from a Mikrotik CHR using PnP fails with a UPnP error:

[tw@nuc ~]$ sudo tailscale debug portmap
gw=10.0.0.1; self=10.0.0.34
portmapper: [v1] UPnP reply {Location:http://10.0.0.1:2828/gateway.xml Server:RouterOS/7.7UPnP/1.0 MikroTik UPnP/1.0 USN:uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1}, "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=3600\r\nEXT: \r\nLOCATION: http://10.0.0.1:2828/gateway.xml\r\nSERVER: RouterOS/7.7UPnP/1.0 MikroTik UPnP/1.0\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nUSN: uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\n\r\n"
portmapper: UPnP meta changed: {Location:http://10.0.0.1:2828/gateway.xml Server:RouterOS/7.7UPnP/1.0 MikroTik UPnP/1.0 USN:uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1}
portmapper: [v1] UPnP reply {Location:http://10.0.0.1:2828/gateway.xml Server:RouterOS/7.7UPnP/1.0 MikroTik UPnP/1.0 USN:uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1}, "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=3600\r\nEXT: \r\nLOCATION: http://10.0.0.1:2828/gateway.xml\r\nSERVER: RouterOS/7.7UPnP/1.0 MikroTik UPnP/1.0\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nUSN: uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\n\r\n"
portmapper: [v1] UPnP reply {Location:http://10.0.0.1:2828/gateway.xml Server:RouterOS/7.7UPnP/1.0 MikroTik UPnP/1.0 USN:uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1}, "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=3600\r\nEXT: \r\nLOCATION: http://10.0.0.1:2828/gateway.xml\r\nSERVER: RouterOS/7.7UPnP/1.0 MikroTik UPnP/1.0\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nUSN: uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\n\r\n"
Probe: {PCP:false PMP:false UPnP:true}
no mapping
portmapper: fetching http://10.0.0.1:2828/gateway.xml
portmapper: saw UPnP type WANIPConnection1 at http://10.0.0.1:2828/gateway.xml; MikroTik Router (MikroTik)
portmapper: getUPnPClient: *internetgateway2.WANIPConnection1, <nil>
portmapper: addAnyPortMapping: 5050, err="SOAP fault: UPnPError"
serveDebugPortmap: context done: context deadline exceeded

Steps to reproduce

No response

Are there any recent changes that introduced the issue?

No response

OS

Linux, macOS, Windows, Android

OS version

Archlinux

Tailscale version

1.42.0-dev20230524

Other software

No response

Bug report

BUG-ee4bbbf9cbe5d3499aea8dc1c3bb73ea626a554b7838ef8a0aa6b988964f7bf4-20230617120209Z-eae3a7b4231b6bf8

@bradfitz
Copy link
Member

cc @andrew-d

@DentonGentry
Copy link
Contributor

Would you be able to fetch http://10.0.0.1:2828/gateway.xml on your local network and attach it here?
If you're not comfortable posting it publicly, you could email it to support@tailscale.com and reference #8364

@DentonGentry DentonGentry added L1 Very few Likelihood P1 Nuisance Priority level T5 Usability Issue type and removed needs-triage labels Jun 17, 2023
@DentonGentry DentonGentry added the portmap Pertaining to NAT-PMP, PCP, UPnP label Sep 11, 2023
@tylerjwatson
Copy link
Author

tylerjwatson commented Dec 3, 2023

Of course! Apologies for the massive delay, I had forgotten all about it.

<?xml version="1.0"?>
<root xmlns="urn:schemas-upnp-org:device-1-0">
  <specVersion>
    <major>1</major>
    <minor>0</minor>
  </specVersion>
  <device>
    <deviceType>urn:schemas-upnp-org:device:InternetGatewayDevice:1</deviceType>
    <friendlyName>MikroTik Router</friendlyName>
    <manufacturer>MikroTik</manufacturer>
    <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
    <modelName>Router OS</modelName>
    <UDN>uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-</UDN>
    <iconList>
      <icon>
        <mimetype>image/gif</mimetype>
        <width>16</width>
        <height>16</height>
        <depth>8</depth>
        <url>/logo16.gif</url>
      </icon>
      <icon>
        <mimetype>image/gif</mimetype>
        <width>32</width>
        <height>32</height>
        <depth>8</depth>
        <url>/logo32.gif</url>
      </icon>
      <icon>
        <mimetype>image/gif</mimetype>
        <width>48</width>
        <height>48</height>
        <depth>8</depth>
        <url>/logo48.gif</url>
      </icon>
    </iconList>
    <serviceList>
      <service>
        <serviceType>urn:schemas-microsoft-com:service:OSInfo:1</serviceType>
        <serviceId>urn:microsoft-com:serviceId:OSInfo1</serviceId>
        <SCPDURL>/osinfo.xml</SCPDURL>
        <controlURL>/upnp/control/oqjsxqshhz/osinfo</controlURL>
        <eventSubURL>/upnp/event/cwzcyndrjf/osinfo</eventSubURL>
      </service>
    </serviceList>
    <deviceList>
      <device>
        <deviceType>urn:schemas-upnp-org:device:WANDevice:1</deviceType>
        <friendlyName>WAN Device</friendlyName>
        <manufacturer>MikroTik</manufacturer>
        <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
        <modelName>Router OS</modelName>
        <UDN>uuid:UUID-MIKROTIK-WAN-DEVICE--1</UDN>
        <serviceList>
          <service>
            <serviceType>urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1</serviceType>
            <serviceId>urn:upnp-org:serviceId:WANCommonIFC1</serviceId>
            <SCPDURL>/wancommonifc-1.xml</SCPDURL>
            <controlURL>/upnp/control/ivvmxhunyq/wancommonifc-1</controlURL>
            <eventSubURL>/upnp/event/mkjzdqvryf/wancommonifc-1</eventSubURL>
          </service>
        </serviceList>
        <deviceList>
          <device>
            <deviceType>urn:schemas-upnp-org:device:WANConnectionDevice:1</deviceType>
            <friendlyName>WAN Connection Device</friendlyName>
            <manufacturer>MikroTik</manufacturer>
            <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
            <modelName>Router OS</modelName>
            <UDN>uuid:UUID-MIKROTIK-WAN-CONNECTION-DEVICE--1</UDN>
            <serviceList>
              <service>
                <serviceType>urn:schemas-upnp-org:service:WANIPConnection:1</serviceType>
                <serviceId>urn:upnp-org:serviceId:WANIPConn1</serviceId>
                <SCPDURL>/wanipconn-1.xml</SCPDURL>
                <controlURL>/upnp/control/yomkmsnooi/wanipconn-1</controlURL>
                <eventSubURL>/upnp/event/veeabhzzva/wanipconn-1</eventSubURL>
              </service>
            </serviceList>
          </device>
        </deviceList>
      </device>
      <device>
        <deviceType>urn:schemas-upnp-org:device:WANDevice:1</deviceType>
        <friendlyName>WAN Device</friendlyName>
        <manufacturer>MikroTik</manufacturer>
        <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
        <modelName>Router OS</modelName>
        <UDN>uuid:UUID-MIKROTIK-WAN-DEVICE--7</UDN>
        <serviceList>
          <service>
            <serviceType>urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1</serviceType>
            <serviceId>urn:upnp-org:serviceId:WANCommonIFC1</serviceId>
            <SCPDURL>/wancommonifc-7.xml</SCPDURL>
            <controlURL>/upnp/control/vzcyyzzttz/wancommonifc-7</controlURL>
            <eventSubURL>/upnp/event/womwbqtbkq/wancommonifc-7</eventSubURL>
          </service>
        </serviceList>
        <deviceList>
          <device>
            <deviceType>urn:schemas-upnp-org:device:WANConnectionDevice:1</deviceType>
            <friendlyName>WAN Connection Device</friendlyName>
            <manufacturer>MikroTik</manufacturer>
            <manufacturerURL>https://www.mikrotik.com/</manufacturerURL>
            <modelName>Router OS</modelName>
            <UDN>uuid:UUID-MIKROTIK-WAN-CONNECTION-DEVICE--7</UDN>
            <serviceList>
              <service>
                <serviceType>urn:schemas-upnp-org:service:WANIPConnection:1</serviceType>
                <serviceId>urn:upnp-org:serviceId:WANIPConn1</serviceId>
                <SCPDURL>/wanipconn-7.xml</SCPDURL>
                <controlURL>/upnp/control/xstnsgeuyh/wanipconn-7</controlURL>
                <eventSubURL>/upnp/event/rscixkusbs/wanipconn-7</eventSubURL>
              </service>
            </serviceList>
          </device>
        </deviceList>
      </device>
    </deviceList>
    <presentationURL>http://10.0.0.1/</presentationURL>
  </device>
  <URLBase>http://10.0.0.1:2828</URLBase>
</root>

DentonGentry added a commit that referenced this issue Dec 3, 2023
Unfortunately in the test we can't reproduce the failure seen
in the real system ("SOAP fault: UPnPError")

Updates #8364

Signed-off-by: Denton Gentry <dgentry@tailscale.com>
@tylerjwatson
Copy link
Author

The error condition has changed slightly with later versions of tailscale:

root@pve:~# tailscale version
1.54.1
  tailscale commit: 0a01efc8f894db55d0975d1926fd5347c548a7af
  other commit: 3d05984255c1a1eff11920ebd04033439926aaf8
  go version: go1.21.4

root@pve:~# tailscale debug portmap
monitor: monitor: gateway and self IP changed: gw=10.0.0.1 self=172.17.0.1
gw=10.0.0.1; self=172.17.0.1
portmapper: [v1] UPnP reply {Location:http://10.0.0.1:2828/gateway.xml Server:RouterOS/7.11.2UPnP/1.0 MikroTik UPnP/1.0 USN:uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1}, "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=3600\r\nEXT: \r\nLOCATION: http://10.0.0.1:2828/gateway.xml\r\nSERVER: RouterOS/7.11.2UPnP/1.0 MikroTik UPnP/1.0\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nUSN: uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\n\r\n"
portmapper: UPnP meta changed: {Location:http://10.0.0.1:2828/gateway.xml Server:RouterOS/7.11.2UPnP/1.0 MikroTik UPnP/1.0 USN:uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1}
portmapper: [v1] UPnP reply {Location:http://10.0.0.1:2828/gateway.xml Server:RouterOS/7.11.2UPnP/1.0 MikroTik UPnP/1.0 USN:uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1}, "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=3600\r\nEXT: \r\nLOCATION: http://10.0.0.1:2828/gateway.xml\r\nSERVER: RouterOS/7.11.2UPnP/1.0 MikroTik UPnP/1.0\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nUSN: uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\n\r\n"
portmapper: [v1] UPnP reply {Location:http://10.0.0.1:2828/gateway.xml Server:RouterOS/7.11.2UPnP/1.0 MikroTik UPnP/1.0 USN:uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1}, "HTTP/1.1 200 OK\r\nCACHE-CONTROL: max-age=3600\r\nEXT: \r\nLOCATION: http://10.0.0.1:2828/gateway.xml\r\nSERVER: RouterOS/7.11.2UPnP/1.0 MikroTik UPnP/1.0\r\nST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\nUSN: uuid:UUID-MIKROTIK-INTERNET-GATEWAY-DEVICE-::urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\n\r\n"
Probe: {PCP:false PMP:false UPnP:true}
no mapping
portmapper: fetching http://10.0.0.1:2828/gateway.xml
portmapper: saw UPnP type WANIPConnection1 at http://10.0.0.1:2828/gateway.xml; MikroTik Router (MikroTik)
portmapper: getUPnPClient: *internetgateway2.WANIPConnection1, <nil>
portmapper: addAnyPortMapping: 17004, err="goupnp: error decoding response body: EOF"
serveDebugPortmap: context done: context deadline exceeded

DentonGentry added a commit that referenced this issue Dec 4, 2023
Unfortunately in the test we can't reproduce the failure seen
in the real system ("SOAP fault: UPnPError")

Updates #8364

Signed-off-by: Denton Gentry <dgentry@tailscale.com>
@andrew-d
Copy link
Member

andrew-d commented Dec 4, 2023

@tylerjwatson - One more question, if you don't mind: could you run tailscale debug portmap -log-http? That will log all HTTP requests and responses that are made during the UPnP process to your terminal (and can be quite verbose), so I recommend doing something like tailscale debug portmap -log-http | tee /tmp/portmap.log. After doing so, can you please email that log to me at andrew@tailscale.com? It may contain sensitive data, so you may not want to post it here.

@tylerjwatson
Copy link
Author

Done mate.

Doesn't appear that the log contains any sensitive information so I'll also attach it here.

Another interesting tidbit is the self IP has been detected as one of the docker bridges and not the interface where the default route is:

tw@pve:~$ ip route
default via 10.0.0.1 dev vmbr1 proto kernel onlink 
10.0.0.0/24 dev vmbr1 proto kernel scope link src 10.0.0.6 
172.17.0.0/16 dev docker0 proto kernel scope link src 172.17.0.1 linkdown 
172.18.0.0/16 dev br-903ffb19a348 proto kernel scope link src 172.18.0.1 

May be triggering the 404 on the mikrotik side?

portmap.log

@andrew-d
Copy link
Member

andrew-d commented Dec 5, 2023

Another interesting tidbit is the self IP has been detected as one of the docker bridges and not the interface where the default route is:

Yeah, that's not right 😓 What happens if you specify the correct IP/gateway manually?

$ tailscale debug portmap --log-http --self-addr 10.0.0.6 --gateway-addr 10.0.0.1 | tee /tmp/portmap.2.log

(or whatever your real self IP is)

@tylerjwatson
Copy link
Author

No discernable change.

portmap.2.log

@andrew-d
Copy link
Member

andrew-d commented Dec 5, 2023

@tylerjwatson First off, thanks for the log and the route table; this actually surfaced an older bug that appears to be unrelated, but still isn't great (fix in #10467).

No discernable change.

portmap.2.log

As for your actual issue... this is pretty puzzling to me. Your Mikrotik is telling us that the UPnP endpoint is at http://10.0.0.1:2828/upnp/control/yomkmsnooi/wanipconn-1, but trying to make a request to that endpoint fails with a 404. Could you make a few requests to that (and the other, similar endpoint) with curl, and report back with the results?

$ curl -i http://10.0.0.1:2828/upnp/control/yomkmsnooi/wanipconn-1
$ curl -i -XPOST http://10.0.0.1:2828/upnp/control/yomkmsnooi/wanipconn-1

$ curl -i http://10.0.0.1:2828/upnp/control/xstnsgeuyh/wanipconn-7
$ curl -i -XPOST http://10.0.0.1:2828/upnp/control/xstnsgeuyh/wanipconn-7

@tylerjwatson
Copy link
Author

Will do soon.

Feel free to flick me a public key if you want to have a play around for yourself, could probably just port forward 2828 over the tunnel and see what's going on if it helps

DentonGentry added a commit that referenced this issue Dec 6, 2023
Unfortunately in the test we can't reproduce the failure seen
in the real system ("SOAP fault: UPnPError")

Updates #8364

Signed-off-by: Denton Gentry <dgentry@tailscale.com>
@tylerjwatson
Copy link
Author

I spent tonight hacking on miniupnpc because it works and I can add a port mapping using the commandline client.

It appears to choose an IGD by probing it with a GetExternalIPAddress SOAP action and it returns the following:

SOAPAction: "urn:schemas-upnp-org:service:WANIPConnection:1#AddPortMapping"
Path: "/upnp/control/rwnwuamutp/wanipconn-7"
<?xml version="1.0"?>
<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
    <s:Body>
        <u:AddPortMapping xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1">
            <NewRemoteHost></NewRemoteHost>
            <NewExternalPort>41641</NewExternalPort>
            <NewProtocol>UDP</NewProtocol>
            <NewInternalPort>41641</NewInternalPort>
            <NewInternalClient>10.0.0.6</NewInternalClient>
            <NewEnabled>1</NewEnabled>
            <NewPortMappingDescription>libminiupnpc</NewPortMappingDescription>
            <NewLeaseDuration>0</NewLeaseDuration>
        </u:AddPortMapping>
    </s:Body>
</s:Envelope>

To which the server responds:

<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
  <s:Body>
    <u:AddPortMappingResponse xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1"></u:AddPortMappingResponse>
  </s:Body>
</s:Envelope>

what's interesting is tailscale chooses a different endpoint. In the router I've only set one external interface (which has the public address on it) so that is quite perplexing as to why there are two.

@andrew-d
Copy link
Member

andrew-d commented Dec 6, 2023

Will do soon.

Feel free to flick me a public key if you want to have a play around for yourself, could probably just port forward 2828 over the tunnel and see what's going on if it helps

Sure, if you don't mind:

ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBENvsX+06ybrIJPt25YfKL3B4OFGcjdhG/JUFmah+/ysqHv5Yo3ac5OpEYFquPFaytxKYrymi6xm0RaiLZ8WCiE=

It appears to choose an IGD by probing it with a GetExternalIPAddress SOAP action and it returns the following:

Yeah, it looks like it first tries to check whether a given UPnP "device" is connected (GetStatusInfo) and has an external IP address (GetExternalIPAddress) before trying to create a port mapping, whereas Tailscale currently always uses the device from the root:

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
}

Looks like something we could add without too much trouble, I think, assuming I can actually test it 😅 Access to a test device would be super helpful, if that's okay with you!

andrew-d added a commit that referenced this issue Dec 6, 2023
Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753
andrew-d added a commit that referenced this issue Dec 6, 2023
Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753
andrew-d added a commit that referenced this issue Dec 6, 2023
Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753
@tylerjwatson
Copy link
Author

@andrew-d

ssh ts@home.tw.id.au -p 24071

You'll find some goodies in the home dir. If you need more resources in the container let me know.

andrew-d added a commit that referenced this issue Dec 7, 2023
Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753
@andrew-d
Copy link
Member

andrew-d commented Dec 7, 2023

@tylerjwatson Great! Just confirmed that the changes in #10489 work for your Mikrotik; it was able to probe both URLs and pick the working one, then successfully obtain a portmapping. I left the log in the homedir as portmap.log, if you're curious.

Once that PR is reviewed + merged, I'll build an unstable release and leave another comment here, and it'll end up in the stable release 1.58, which we'll build sometime in early 2024.

Also: I really appreciate the help with debugging this; it was great. Folks like you are a maintainers' dream–so thanks 😃

andrew-d added a commit that referenced this issue Dec 7, 2023
Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753
@tylerjwatson
Copy link
Author

tylerjwatson commented Dec 8, 2023

I really appreciate the help with debugging this; it was great. Folks like you are a maintainers' dream–so thanks 😃

Compliment much appreciated - a bit of team work always works out the best, and likewise; It's easy to just ignore annoyance issues like these but it was picked up straight away. Massive kudos to you.

Edit: I'll leave the container open for now in case the story develops as the unstable release gets ...well, released. Happy to run a dev build in the meantime.

andrew-d added a commit that referenced this issue Dec 12, 2023
Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

RELNOTE=Improve UPnP portmapping on devices with multiple interfaces

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753
andrew-d added a commit that referenced this issue Dec 13, 2023
Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

RELNOTE=Improve UPnP portmapping when multiple UPnP services exist

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753
andrew-d added a commit that referenced this issue Dec 13, 2023
Previously, we would select the first WANIPConnection2 (and related)
client from the root device, without any additional checks. However,
some routers expose multiple UPnP devices in various states, and simply
picking the first available one can result in attempting to perform a
portmap with a device that isn't functional.

Instead, mimic what the miniupnpc code does, and prefer devices that are
(a) reporting as Connected, and (b) have a valid external IP address.
For our use-case, we additionally prefer devices that have an external
IP address that's a public address, to increase the likelihood that we
can obtain a direct connection from peers.

Finally, we split out fetching the root device (getUPnPRootDevice) from
selecting the best service within that root device (selectBestService),
and add some extensive tests for various UPnP server behaviours.

RELNOTE=Improve UPnP portmapping when multiple UPnP services exist

Updates #8364

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I71795cd80be6214dfcef0fe83115a5e3fe4b8753
@andrew-d
Copy link
Member

@tylerjwatson - We just built an unstable Tailscale build (1.57.25) which contains the fix for this issue; when you get a moment, would you mind installing that and checking if it works? If so, we can close this issue as completed 😄

@tylerjwatson
Copy link
Author

Can do, I will have to replicate the exact conditions tonight when I get home, as my WANscape(ugh) has changed slightly

Reading over the patches, come to think of it, instead of selecting the first external gateway, wouldn't it be better to add portmaps to all endpoints that are offered and externally reachable in the nodemap?

@andrew-d
Copy link
Member

Reading over the patches, come to think of it, instead of selecting the first external gateway, wouldn't it be better to add portmaps to all endpoints that are offered and externally reachable in the nodemap?

Yep, that's something I want to work on down the road. It's a bit complicated right now since much of the codebase assumes we have a single port mapping, so there's a bit of refactoring required. That's definitely the "right" longer-term solution though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug L1 Very few Likelihood P1 Nuisance Priority level portmap Pertaining to NAT-PMP, PCP, UPnP T5 Usability Issue type
Projects
None yet
Development

No branches or pull requests

4 participants