Merge request: UPnP support

added component::archived/nyx owner::atagar priority::medium resolution::user disappeared status::closed type::enhancement labels

Hi Kamran - very nice! I'm honestly very impressed that you did this without any external dependencies. Great job!

At the moment your changes are built on previous changes in your repository which could make merging an issue. Please cut a new branch off of the official master and rebase your changes (c9a61f5, fac6a23, 982f8a5, 5cf57f7) on that.

Now onto the code review feedback...

======================================================================

GENERAL FEEDBACK

======================================================================

As I've nudged on irc this doesn't need to be an arm-specific feature. With some polish and testing this would be a very nice addition to stem which would make this available to other python controllers. Automated testing would also benefit arm and make me more comfortable with this change.

Stem is the future for arm's codebase. Bit by bit I've been rewriting arm's utils, adding tests, and moving them to stem so I'd rather if we were implementing UPnP support there first instead.

======================================================================

We should be wary of doing anything UPnP related when the user simply runs arm to look at their relay. This is a feature that users should opt into via an armrc option...

uses UPnP to make our ORPort reachable

features.useUpnp false

with UPnP calls wrapped in a check for both that config option and that the user has an ORPort (there's no point in trying to negotiate UPnP when they aren't running a relay).

This actually has more value to users as a wizard option since that's an "arm managed" tor instance, more similar to running tor via vidalia. I'd suggest looking at the 'Options.PORTFORWARD' option in src/cli/wizard.py (this would be a great fallback for when tor-fw-helper is unavailable).

======================================================================

This adds the UPnP configuration but never removes it. Maybe this will help...

08:18 < atagar> I'm a little curious after reading this how vidalia handles upnp - does it
somehow clear the upnp entry when it shuts down?
08:21 < atagar> chiiph: ^
08:24 < edmanm> atagar: yes, it does.
08:25 < atagar> ahh - thanks
08:26 < edmanm> (see the last few lines of UPNPControlThread::run() in
src/vidalia/config/UPNPControlThread.cpp if you want to poke further though. :)
08:28 < atagar> got it -
https://gitweb.torproject.org/vidalia.git/blob/HEAD:/src/vidalia/config/UPNPControlThread.cpp#l56
08:28 < atagar> is that the same as an 'AddPortMapping' action for port zero?
08:30 < edmanm> it'll end up calling miniupnpc's UPNP_DeletePortMapping. not sure if that's
the same as AddPortMapping internally within miniupnpc though.

======================================================================

I have UPnP disabled on my home router. However, when I run your changes it displays... UPnP device available: Actiontec Electronics, Inc. 331 found at 192.168.0.1

This is... spooky. Does this mean that my router's lying to me about UPnP being disabled or is this message incorrect? I'm happy to debug this with you if you'd like.

======================================================================

CHANGE SPECIFIC FEEDBACK

======================================================================

src/util/upnp.py

Pylint spotted a few valid minor issues...

W: 26:create_soap_xml: Dangerous default value {} as argument
W: 34:UPnPError.__init__: __init__ method from base class 'Exception' is not called
W:103:UpnpDevice.get_port_mappings: Unused variable 'e'
W:126:UpnpDevice.add_port_mapping: Unused variable 'e'
W:  8: Unused import time
W: 10: Unused import urllib
W:  7: Unused import sys

def init(self, ip, response):

The self.response is never used, and the response arg is only used to determine 'location' which is in turn used to get 'fd'. This object would be much easier to understand if its input argument was the 'location' and you had a comment giving an example of what one looks like.

sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP)

sock.connect(('18.0.0.1', 9))

self.ifip, _ = sock.getsockname()

I'm guessing that you're trying to figure out our internal ip address? If so then this method can be shortened a tiny bit...

>>> s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
>>> s.connect(('18.0.0.1', 0))
>>> s.getsockname()[0]
'192.168.0.2'
>>> s.close()

It looks like tor does something like this... https://gitweb.torproject.org/tor.git/blob/HEAD:/src/common/address.c#l1089

however, it confuses users so they're trying to find an alternative... https://trac.torproject.org/projects/tor/ticket/1827

I'm a little frustrated that there doesn't seem to be a less hacky method for doing this with python. In practice this seems to match the local ip from... https://gitweb.torproject.org/arm.git/blob/HEAD:/src/cli/connections/connPanel.py#l427

However, using that would be pretty hacky too and won't work if tor isn't running or connection resolution is disabled (ie, if the getConnections() results are empty). If we could figure out the system's dns resolvers and connect to that instead it would be the least alarming to users though I'm not spotting a nice way of doing that... :/

Please move this to a "getMyIpAddress()" function in the connection util.

match = re.findall(r"LOCATION: (.*)", response)

if not match:
 return
location = match[0].strip()

Do we have a validly constructed UpnpDevice object if we prematurely return here? This'll be moot if we get rid of the response arg as mentioned earlier.

fd = None

Not needed.

```
 if 'url' in self.control:
```
```
   self.get_port_mappings()
```
```
   self.add_port_mapping()
```

It seems inappropriate for the constructor to actually turn on UPnP.

def get_port_mappings(self):
action = 'GetGenericPortMappingEntry'
for i in range(3):
```
 args = { 'NewPortMappingIndex' : i }
```
```
 try:
```

   retvals = self.get_soap_response(action, args)

```
 except UPnPError, e:
```
```
   pass
```
```
 else:
```

   log.log(log.NOTICE, 'Port mapping found: ' + retvals['NewPortMappingDescription'])

This function doesn't set or return anything. Is its only purpose to log the 'NewPortMappingDescription'? Why is this done three times?

def add_port_mapping(self):

Is this idempotent? How long does this UPnP entry last? Until tor is shut down or does it remain until the router's restarted? We should be careful not to mess with the user's router configuration without reverting it afterward.

 'SOAPAction': '"' + self.control['serviceType'] + '#' + action + '"',

The serviceType mapping comes from parse_desc which may not have happened if we got an urllib2.URLError when we called "fd = urllib2.urlopen(location)".

======================================================================

src/util/connections.py

Please move these changes into 'upnp.py' (the UpnpResolver is upnp specific).

Pylint spotted some more minor issues...

W:663:AppResolver._queryApplications: Dangerous default value [] as argument
W: 23: Unused import struct
W: 20: Unused import fcntl

+import fcntl import os +import select +import struct +import socket

Minor convention note but I order imports by the length of the module's name.

+class UpnpResolver(threading.Thread):

You never actually use this as a thread. Instead resolve spawns subthreads.

"""

Constructs a resolver instance.

"""

threading.Thread.init(self)

Missing a blank line between the doc and code.

self._cond = threading.Condition() # used for pausing when waiting for devices

self.isResolving = False # flag set if we're in the process of making a query

self.failureCount = 0 # -1 if we've made a successful query

Conventions are that private variables are self._foo and public are self.foo. I'm guessing that a few more should have underscores.

self.devicesLock.acquire()

devices = self.queryDevices

self.devicesLock.release()

I'm pretty sure that assignments are atomic so locking here isn't really necessary.

def resolve(self):

"""

This clears the last set of devices when completed.

"""

if self.failureCount < 3:
 self.isResolving = True
 t = threading.Thread(target = self._queryRootDevices)
 t.setDaemon(True)
 t.start()

Unfortunately this is likely gonna cause occasional problems when shutting down. If a python process terminates with leftover threads (even daemon threads) then it will give a nebulous 'syshook' error. You need to keep track of all running threads and make sure that when arm quits the main thread joins on them. If you really want to spawn resolver threads then see the _Resolver class of hostnames.py for an example of doing this safely (though it looks like in practice you don't want more than one).

Also, you aren't checking the isResolving flag before making a thread so you may already have a thread. Hence if, say, you call resolve five times you'll have five resolving threads and the first one to finish will set isResolving back to False.

sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP)

sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)

sock.setsockopt(socket.IPPROTO_IP, socket.IP_MULTICAST_TTL, 5)

sock.bind(('', 1900))

Please add a comment saying what this is doing and the importance of 1900.

      'HOST: 239.255.255.250:1900\r\n'
sock.sendto(msg, ('239.255.255.250', 1900))

Please add a comment for what this address is for. Also, this will fail (with a stacktrace) if the user's disconnected from their network...

File "/home/atagar/Desktop/arm/src/util/connection.py", line 843, in _queryRootDevices sock.sendto(msg, ('239.255.255.250', 1900)) error: [Errno 101] Network is unreachable

 self.failureCount = self.failureCount + 1

This would read a bit better as... self.failureCount += 1

self.devicesLock.acquire()

self.queryDevices = devices

self.isResolving = False

self.devicesLock.release()

Each time resolve() is called (every ten seconds) you're setting queryDevices to a new upnp.UpnpDevice. Constructing upnp.UpnpDevice causes us to add a UPnP entry for our relay - I doubt this is what you want.

======================================================================

src/cli/connections/connPanel.py

Hmmm, I see the reasoning that led you to add this to the connection panel but this really concerns the user's ORPort, not their current connections. Also, this takes quite a bit of room and looks a little odd since it prevents the scroll bar from reaching the bottom of the screen.

If the user both has an ORPort and opted into the UPnP config option then I'd suggest...

Call the UPnP resolve method until we either (1) have a single success or (2) fail three times.
Upon success log the user's UPnP device name.
If we successfully enable UPnP then note this in the header panel besides the ORPort, maybe something like... morrigan - 216.254.20.162:9050, UPnP Enabled, Control Port (cookie): 9051

For posterity (and since I'm kinda bugged that I spent hours code reviewing this file before realizing that we should go with something else) here's my feedback for the file. Feel free to ignore it. :)

+from util import connections, enum, log, panel, torTools, uiTools

It doesn't look like you're using that new log import.

self._upnpDevices = [] # upnp devices found on network

Very, very minor but the comment's indent is off by a space.

rate limits appResolver queries to once per update

self.upnpResolveSinceUpdate = False

rate limits appResolver queries to once per update

self.upnpResolveSinceUpdate = False

Paste error. ;)

listHeight = height - 3

That certainly does the trick though I'd rather if you had a upnpLabelHeight attribute that's subtracted where appropriate, like the detailPanelOffset. This will make it trivial to toggle the label to be on or off via a config option.

 if currentTime - self._lastUpnpResolve > 10:

The ten seconds should be a constant at the top of the file. More importantly, though, all lenthy operations should be in run() or _update() rather than the draw loop. There's a couple reasons for this...

having the update in the draw() function means that it will only be executed if the user is looking at that page
rendering the panel should never do 'work' since any blocking call will cause this part of the interface to seize (looking buggy to the user), though it looks like you're accounting for this via a timeout

You're probably basing this on _resolveApps() which is a good idea, but in this case not right. I wanted _resolveApps() to only run if the user was actively looking at the connection panel since it was pointless to do those lsof lookups when the user would never see the results.

 msg = "UPnP device%s available: " % ("s" if len(self._upnpDevices) > 1 else "")

 self.addstr(listHeight + 2, xOffset, msg, curses.A_STANDOUT | uiTools.getColor('green'))

```
 xOffset = xOffset + len(msg)
```

 msg = ", ".join([device.name for device in self._upnpDevices])

 self.addstr(listHeight + 2, xOffset, msg, curses.A_STANDOUT | uiTools.getColor('green'))

Using an xOffset like this is the pattern I use when multiple strings need different formatting. However, in this case it's equivilant to doing a single addstr call, no?

msg = "UPnP device%s available: " % ("s" if len(self._upnpDevices) > 1 else "") msg += ", ".join([device.name for device in self._upnpDevices]) self.addstr(listHeight + 2, xOffset, msg, curses.A_STANDOUT | uiTools.getColor('green'))

if flagQuery:
```
 self.appResolveSinceUpdate = True
```

Paste error (setting the wrong flag).

Cheers! -Damian

These code review comments were never addressed. Resolving - feel free to reopen if you return. :)

Trac:
Resolution: N/A to user disappeared
Status: new to closed

closed

Merge request: UPnP support

Child items 0

Activity

uses UPnP to make our ORPort reachable

rate limits appResolver queries to once per update

rate limits appResolver queries to once per update