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-IGD support for NAT traversal #741

Closed
ichorid opened this issue May 9, 2020 · 14 comments
Closed

UPnP-IGD support for NAT traversal #741

ichorid opened this issue May 9, 2020 · 14 comments
Labels
priority: none Duplicate, wont address, on hold or unreproducible

Comments

@ichorid
Copy link
Contributor

ichorid commented May 9, 2020

UPnP provides a way for an application/device to request a port forwarding through the network's NAT from a UPnP-enabled router. This is another technique to enable better NAT traversal.
UPnP-IGD uses broadcast HTTP requests over UDP (and is already enabled in Libtorrent). We can copy the logic for announcing the port from there.

@ichorid ichorid added the priority: medium Enhancements, features or exotic bugs label May 9, 2020
@qstokkink
Copy link
Collaborator

In order to make sure the source code doesn't turn into full-on spaghetti, this should be postponed until after #615.

@qstokkink
Copy link
Collaborator

qstokkink commented Jul 20, 2020

@qstokkink
Copy link
Collaborator

UPnPy only required one patch, here's my test for service discovery:

import logging
import socket

import upnpy
from upnpy.ssdp.SSDPRequest import SSDPDevice, SSDPRequest


class PatchedRequest(SSDPRequest):

    def _send_request(self, message):
        self.socket.sendto(message.encode(), (self.SSDP_MCAST_ADDR, self.SSDP_PORT))
        devices = []
        try:
            while True:
                response, addr = self.socket.recvfrom(65507)
                try:
                    device = SSDPDevice(addr, response.decode())
                    devices.append(device)
                except:
                    logging.warning("Failed to connect to UPnP device at %s", addr)
        except socket.timeout:
            pass

        return devices


upnp = upnpy.UPnP()
upnp.ssdp = PatchedRequest()

devices = upnp.discover()

print(devices)

device = upnp.get_igd()

print(device.get_services())

for service in device.get_services():
    print("=" * 2, service, "=" * 8)
    for action in service.get_actions():
        print(action, "INPUTS:", action.get_input_arguments(), "OUTPUTS:", action.get_output_arguments())

@qstokkink
Copy link
Collaborator

Here is the same script with uPnPclient, which holds my preference:

import upnpclient

devices = upnpclient.discover()

print(devices)

for device in devices:
    print(device.friendly_name, device.services)

    for service in device.services:
        print("=" * 2, service, "=" * 8)
        for action in service.actions:
            print(action, "INPUTS:", action.argsdef_in, "OUTPUTS:", action.argsdef_out)

@ichorid
Copy link
Contributor Author

ichorid commented Jul 20, 2020

What's the upsides of upnpclient vs upnpy? Brevity?
Which one is better supported?

@qstokkink
Copy link
Collaborator

upnpclient works out-of-the-box and has more stars on GitHub, but was last updated 10 months ago. upnpy has 1/10th of the stars, but was last updated a month ago.

Judging from the repository activity, neither has active support.

@ichorid
Copy link
Contributor Author

ichorid commented Jul 20, 2020

Is is possible to create a simple test for the stuff? Like, starting a upnp server and verify that the thing really works as intended?

@qstokkink
Copy link
Collaborator

Isolated and local: no.

However, pretty much any commercial router supports UPnP. So, usually it will probably work. Downside being that no single router supports it the same way and there are all sorts of vendor-specific differences. Actually, most routers don't even fully support UPnP because it is full of critical security flaws. After reading about and playing with UPnP, I wonder if we really want it. Also, it's a pain to work with.

@ichorid
Copy link
Contributor Author

ichorid commented Jul 20, 2020

As long as we do not require UPnP to be enabled on the router, it is fine to exploit the opportunity for better NAT traversal. Of course, we should be able to fallback to non-UPnP solution in case the router's UPnP implementation is faulty.

Does it worth it? We'll see when we enable it.

@qstokkink
Copy link
Collaborator

It is not the opportunity I oppose, it is the fact that we may be opening up unsuspecting users to some terrible security leaks.

Ultimately, I think we can do more with a simple UDP broadcast over the LAN.

@ichorid
Copy link
Contributor Author

ichorid commented Jul 20, 2020

By using UPnP correctly we're not opening anyone to a security leak.

@qstokkink
Copy link
Collaborator

Let me quote the 5th link I posted (feel free to read them all in-depth though):

You might assume that you’re secure as long as no malware is running on any local devices – but you’re probably wrong.

UPnP is utterly flawed and a huge security risk to the point of pretty much every security expert in the field advising to disable it on your local router.

I don't want to give users any reason to enable it again.

@ichorid
Copy link
Contributor Author

ichorid commented Jul 20, 2020

I don't want to give users any reason to enable it again.

That's so responsible of you! Maybe we should ask Arvid to remove UPnP support from Libtorrent for the same reason, BTW? 😉

@qstokkink
Copy link
Collaborator

Alright, it seems we have enough information. I'll close this issue and round up this information here.

Reasons to not support UPnP:

  • There is overwhelming evidence that UPnP hardware is half-implemented and full of security holes.
  • UPnP APIs are inconsistent and therefore fragile.
  • UPnP support is non-trivial and we will have to maintain this code.
  • UPnP support requires an additional dependency (or a few hundred lines of code).
  • UPnP does not offer anything we cannot get from UDP broadcasts.
  • We are responsible.

If any of these reasons can be shown to be factually incorrect, we can reopen this issue and have a discussion.

Should we ever want to enable UPnP anyway:

@qstokkink qstokkink added priority: none Duplicate, wont address, on hold or unreproducible and removed priority: medium Enhancements, features or exotic bugs on hold labels Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: none Duplicate, wont address, on hold or unreproducible
Projects
None yet
Development

No branches or pull requests

2 participants