[tor-bugs] #12533 [Stem]: multiple hidden services and get_conf_map('HiddenServiceOptions') response format

[Tor Bug Tracker & Wiki]

#12533: multiple hidden services and get_conf_map('HiddenServiceOptions') response
format
-------------------------+------------------------------
     Reporter:  jthayer  |      Owner:  atagar
         Type:  defect   |     Status:  new
     Priority:  normal   |  Milestone:
    Component:  Stem     |    Version:
   Resolution:           |   Keywords:  controller, easy
Actual Points:           |  Parent ID:
       Points:           |
-------------------------+------------------------------

Comment (by atagar):

 Hi federico3, sorry about the delay!

 ----

 {{{
 from collections import OrderedDict
 }}}

 Stem includes a python 2.6 compatible copy of OrderedDict...

 {{{
 try:
   # added in python 2.7
   from collections import OrderedDict
 except ImportError:
   from stem.util.ordereddict import OrderedDict
 }}}

 ----

 {{{
 def get_hidden_services_conf(self):
 }}}

 This should accept a defaut argument like the Controller's other getter
 methods.

 ----
 {{{
 """Get hidden services configuration
 }}}

 This could do with being a bit more descriptive. Maybe the following?

 This provides a mapping of hidden service directories to their attribute's
 key/value pairs.

 ----

 {{{
 * :class:`RuntimeError` if the configuration contains unexpected entries
 }}}

 It would probably be better to avoid throwing exceptions that aren't a
 stem.ControllerError subclass (makes for more work for callers). That said
 though do we really want to error in this case? Tor is free to add new
 hidden service values in the future so it seems like we might as well
 provide them in our dict. This would be simpler if we provide what Tor
 gave us. That is to say, for the pydoc example...

 {{{
 >>> controller.get_hidden_services_conf()
 {
   "/var/lib/tor/hidden_service_empty/": {
     "HiddenServicePort": [
     ]
   },
   "/var/lib/tor/hidden_service_with_two_ports/": {
   "HiddenServiceAuthorizeClient": "stealth a, b",
   "HiddenServicePort": [
     "8020 127.0.0.1:8020", # the ports order is kept
     "8021 127.0.0.1:8021"
   ],
   "HiddenServiceVersion": "2"
   },
 }
 }}}

 ----

 {{{
 try:
   response = self.msg('GETCONF HiddenServiceOptions')
   stem.response.convert('GETCONF', response)
   log.debug('GETCONF HiddenServiceOptions (runtime: %0.4f)' % (time.time()
 - start_time))
 except stem.ControllerError as exc:
   log.debug('GETCONF HiddenServiceOptions (failed: %s)' % exc)
   raise
 }}}

 Up to you but you might want to consider caching the response (like
 get_conf() does). More importantly though this should be re-raising exc.

 ----

 {{{
 service_dir_map = OrderedDict()
 directory = None
 ports = []
 }}}

 The ports is unused.

 ----

 {{{
 k, v = content.split('=', 1)
 }}}

 What if content doesn't have a '='?

 ----

 {{{
 service_dir_map[directory] = dict(
   ports = []
 )
 }}}

 Probably little reason not to go with the one-liner...

 {{{
 service_dir_map[directory] = {'ports': []}
 }}}

 ----

 {{{
 elif k == 'HiddenServiceVersion':
   service_dir_map[directory]['service_version'] = int(v)
 }}}

 The pydoc example says that the version is a string. Personally I think it
 should be (so we're consistent in providing string keys and values).

 ----

 {{{
 queryitems.append("HiddenServiceAuthorizeClient=\"%s\"" % v)
 }}}

 This would be a good spot to use single quotes to avoid the escaping...

 {{{
 queryitems.append('HiddenServiceAuthorizeClient="%s"' % v)
 }}}

 ----

 {{{
 query = 'SETCONF ' + ' '.join(queryitems)
 response = self.msg(query)
 stem.response.convert('SINGLELINE', response)
 }}}

 By calling SETCONF directly we *might* be failing to invalidate
 get_conf()'s cache.

 ----

 {{{
 assert 0 <= virtport <= 2 **16
 }}}

 Stem hasn't used assertions anywhere before. Maybe we should be raising a
 ValueError? Stem provides a helper method for checking if it's valid...

 {{{
 if stem.util.connection.is_valid_port(virtport):
   virtport = int(virtport)
 else:
   raise ValueError("'%s' isn't a valid port number" % virtport)
 }}}

 I know this violates what I said earlier about throwing
 stem.ControllerError subclasses. The reason is that this is purely an
 error on the side of our caller, so the best thing we can do to help them
 is be really, really noisy about letting them know. :)

 ----

 {{{
 if str(virtport) in ports:
   return False

 if "%d 127.0.0.1:%d" % (virtport, virtport) in ports:
   return False
 }}}

 This makes me wonder about the HiddenServicePort format. You seem to be
 expecting either port numbers or '<port> <ip>:<port>' (not sure why it has
 the port twice). Unfortunately the Tor docs around this are a bit vague...

 https://www.torproject.org/docs/tor-manual.html.en#HiddenServicePort

 Might warrant clarification and a ticket to the Tor component. In either
 case our get_hidden_services_conf() docs will need to be very clear about
 what it provides for the ports value so callers can know how to handle it.

 ----

 {{{
 self.set_hidden_services_conf(conf)
 return True
 }}}

 Presently this method isn't documented as raising any exceptions, but
 set_hidden_services_conf() could raise. We could either document the
 exceptions or suppress them. Probably better to go for the former here.

 {{{
 def delete_hidden_service(self, dirname, virtport, target=None):
   """Delete a hidden service+port.
   :param str dirname: directory name
   :param int virtport: virtual port
   :param str target: optional ipaddr:port target e.g. '127.0.0.1:8080'
   :raises:
   """
 }}}

 This starts a ':raises:' block but it's empty.

 ----

 {{{
 try:
   ports.pop(ports.index(str(virtport)))
 except ValueError:
   ports.pop(ports.index(longport))
 }}}

 This raises a ValueError if both the virtport and longport doesn't exist.
 In this case better to go with a stem.InvalidArguments.

 Cheers! -Damian

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12533#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online