[tor-relays] [ANNOUNCE] check_tor.py: a Nagios check for Tor relays

Jérémy Bobbio lunar at debian.org
Tue Feb 26 19:25:31 UTC 2013


(Following up on tor-relays at 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.

-- 
Jérémy Bobbio                        .''`. 
lunar at debian.org                    : :Ⓐ  :  # apt-get install anarchism
                                    `. `'` 
                                      `-   
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.torproject.org/pipermail/tor-relays/attachments/20130226/3ff00069/attachment.pgp>


More information about the tor-relays mailing list