(Following up on tor-relays@l.tpo.)

Hi Damian,

Many thanks for this review. The previous time it was already useful, and you strike again with more neat comments. :)

Damian Johnson:

Sathyanarayanan suggested putting together a page with stem examples (https://trac.torproject.org/8322). If you wouldn't mind check_tor would be perfect for that. Some more inline comments would be nice, but otherwise it's a fine script. :)

I am not sure what is your proposal, but yes, it looks like a pretty good example of what can be done with Stem. I'm pretty happy with the current amount of comments, so patches are welcome if you want to make the script more didactic.

...

[1] https://stem.readthedocs.org/

As mentioned on irc this moved a long while back to...

https://stem.torproject.org/

Fixed.

...

self.tempdir = None

It looks like tempdir is solely used to determine if self.datadir was created via mkdtemp(), and hence if it should be cleaned up afterward. Maybe it would make sense for this to be a 'self.datadir_is_temp' boolean?

With my Haskell background, I would rather use an "Either FilePath FilePath" datatype. But this is not Haskell…

Suggestion implemented.

...

'ControlSocket': self.control_socket_path(),

If you're using a control socket then there's little point in also setting CookieAuthentication. Control sockets are based on filesystem permissions, like an auth cookie, so it doesn't provide any added protection.

I was seting things up in a similar manner than what is done by the Debian package. But I realize now that with a fully controlled torrc, it does not add any extra security because the permissions on the data directory are properly set.

Cookie authentication removed.

FYI, there's a catch with socket permissions, tough. Quoting Tor changelog for version 0.2.2.26-beta:

- Tor now refuses to create a ControlSocket in a directory that is world-readable (or group-readable if ControlSocketsGroupWritable is 0). This is necessary because some operating systems do not enforce permissions on an AF_UNIX sockets. Permissions on the directory holding the socket, however, seems to work everywhere.

See also #2972.

...

'CookieAuthFile': self.cookie_auth_file_path(),

Why do you set this explicitely? The cookie path is part of the PROTOCOLINFO response so it isn't needed to authenticate.

Good to know. I've removed it along with the cookie authentication though.

...

self.socks_addr = self.controller.get_info('net/listeners/socks')[1:-1] # skip " at begining and end

You might want to either wrap get_info() calls in try/catch blocks or provide a default argument (ie, something like "get_info('net/listeners/socks', '"0"')").

If the SOCKS port cannot be determined, then the rest of the script will fail. I have added a try/except block to output a better error message.

...

if self.popen: try: self.popen.poll() if not self.popen.returncode: self.popen.kill() self.popen.communicate() except OSError, ex: if ex.errno != errno.ESRCH: raise self.popen = None

Nice, I didn't think of checking the errno. The above will only work on python 2.6 and above. If you want 2.5 compatability for non-windows then you can fall back on os.kill(). Here's what I do in stem's tests...

Debian stable (released on February 2011) has Python 2.6 by default. I welcome patches from anyone who would like to support earlier systems.

...

def get_url(self, socks_addr, url): ... time.sleep(30)

There's quite a few spots in this script that could benefit from comments. This line especially I'm not sure why a thirty second sleep is necessary.

That sleep was there to leave enough time to build a circuit in the MiddleTest. I have removed it now that the later has been rewritten to use await_build.

...

self.nickname = event.path[0][1]

You might want to call 'self.controller.enable_feature("VERBOSE_NAMES")' during setup, otherwise the nickname may be None prior to tor 0.2.2.1.

Thanks for the tip. Added.

...

class MiddleTest(ConnectivityTest): ... def create_circuit(self): ...

When you call self.controller.new_circuit() would the await_build argument help? You're presently listening for CIRC events to call attach_stream() when the circuit is done being built, and that's exactly what await_build is intended to help with.

Amazing! When I originaly wrote the script new_circuit() did not have await_build parameter. :)

Using this new feature made the code a lot simplier!

I felt into a trap while implementing though: one should not call new_circuit(…, await_build=True) inside an event listener.

Btw, if circuits always fail to be created then the retry in handle_circ() will produce an infinite loop. Also, new_circuit() can potentially raise a ControllerError.

...

def get_exits(self): for desc in self.controller.get_server_descriptors(): if not desc.hibernating and desc.exit_policy.can_exit_to(address, port): exits.append(desc.fingerprint)

I was about to warn that this would break for new tor versions due to the switch to microdescriptors, but I suppose that's what FetchUselessDescriptors is for. 'UseMicrodescriptors 0' might be a little clearer, or falling back on microdescriptors. Stem presently does not have microdescriptor support, but that's what I'm currently working on (it should be done sometime this week)...

https://trac.torproject.org/projects/tor/ticket/8253

I think I'll leave the code as it is until Stem fully supports microdescriptors. That will be a good test for the API.

I have pushed all the aforementioned changes.

Thanks again for the review.