Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#12533 closed defect (implemented)

multiple hidden services and get_conf_map('HiddenServiceOptions') response format

Reported by: jthayer Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords: controller, easy
Cc: micahlee, benedikt_V@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Hi, I'm trying to add a new hidden service to a client which is already running one (or more) service. The naive approach,

    controller.set_options([
        ('HiddenServiceDir', hs_dir),
        ('HiddenServicePort', hs_port)
    ])

clobbers the existing services. Since there doesn't seem to be an API call expressly for adding (not replacing) hidden services, I started investigating the idea of fetching the existing hidden services from the client, then re-adding those in addition to the new service.

However, the data returned from get_conf_map('HiddenServiceOptions') isn't appropriate for that, as far as I can tell. That call returns two flat lists (associated with HiddenServiceDir and HiddenServicePort), which leaves us in the dark about which ports are associated with which dirs.

    >>> controller.set_options([('HiddenServiceDir','/tmp/hiddenservice2/'),
                                ('HiddenServicePort','1235 127.0.0.1:1235'),
                                ('HiddenServicePort','1236 127.0.0.1:1236'),
                                ('HiddenServiceDir','/tmp/hiddenservice3/'),
                                ('HiddenServicePort','1237 127.0.0.1:1237')])

    >>> controller.get_conf_map('HiddenServiceOptions')
    {'HiddenServiceDir': ['/tmp/hiddenservice2/', '/tmp/hiddenservice3/'],
     'HiddenServicePort': ['1235 127.0.0.1:1235', '1236 127.0.0.1:1236', '1237 127.0.0.1:1237']}

More useful in this case would be a list of Dir->[Port] mappings:

    >>> controller.get_conf_map('HiddenServiceOptionsMap')
    {'/tmp/hiddenservice2/': ['1235 127.0.0.1:1235','1236 127.0.0.1:1236'],
     '/tmp/hiddenservice3/': ['1237 127.0.0.1:1237']}

With that data, then, it would be easy to make the appropriate SetOptions calls not only to add a new service, but to re-add the existing services.

I'm not very familiar with Tor's control protocol, so perhaps this isn't a limitation of Stem but of the protocol in general.

Child Tickets

Change History (20)

comment:1 Changed 3 years ago by micahlee

Cc: micahlee added

comment:2 Changed 3 years ago by micahlee

This bug affects OnionShare too. If you open an onionshare process to share a file, then open a second onionshare process to share a second file, it breaks the first process's hidden service.

Here's the relevant bug: https://github.com/micahflee/onionshare/issues/87

comment:3 Changed 3 years ago by atagar

Keywords: controller easy added; get_conf hidden service removed

Hi jthayer, this is a good point. If you call 'GETCONF HiddenServiceOptions' with the above interpretor session what does it respond with? I like your idea for the alternate get_conf_map() output, though I'll probably make is a hidden service specific method since those options are such a special snowflake. :P

comment:4 Changed 3 years ago by federico2

Maybe it could make more sense to add primitives to add/remove hidden services, e.g.:

  • add_hidden_service(tor_port, internal_port) --> returns onion hostname
  • del_hidden_service(onion_hostname)

It would be useful to have a CLI tool (maybe tor itself) to do this without being root and maybe without having the control password.

comment:6 Changed 3 years ago by atagar

Sorry about the delay. I was hoping to whip this up but other things are coming up, and will continue to for a while. Patches would be most welcome if you'd like to improve this in Stem. If not then it may be a while before I can get to this.

comment:7 Changed 3 years ago by meejah

For what it's worth, the way I've done this in txtorcon is via the TorConfig abstraction -- the user can add/delete/whatever to variables and lists in there and call save() and it takes care of making sure the relevant SETCONF call has *all* the hidden-serivces etc.

I'm not completely sold on the "magic getattr" business in there, and hidden-services are the most-complex since they are basically a "single thing" that has multiple lines (that need to be in the right order, etc).

But basically, adding a hidden service is basically "TorConfig.hiddenservices.append(HiddenService(...))". It probably wouldn't be *that* hard to port to Stem -- you'd have to take out the getconf and setconf calls via TorControlProtocol and make the appropriate Stem calls instead. And make save() synchronous (instead of async). And probably add a "bootstrap()" synchronous method instead of the "post_bootstrap" Deferred.

comment:8 Changed 3 years ago by atagar

Hi msvb-lab. I spotted a while back that you had a patch for this on irc. That's awesome!

First I need to ask - are you ok with your Stem contributions being in the public domain? If so then I'll try to get you a code review for this over the weekend.

Thanks again for the patch!

comment:9 Changed 3 years ago by federico3

Patch author here: I am ok with the Stem patch being public domain and/or LGPLv3

comment:10 Changed 3 years ago 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

comment:11 Changed 3 years ago by x4fyr

Cc: benedikt_V@… added

comment:12 Changed 3 years ago by patrickod

Hey all,

Decided to take on implementing atagar's patch feedback as a first step in contributing to the tor project. Thanks federico2 for your work thus far, it's awesome standing on the shoulders of giants.

The patch w/ revisions is here: https://gist.github.com/patrickod/3ea33c76f903ced87f80

Would love any feedback on what further changes are needed.

Thanks!

Patrick

comment:13 Changed 3 years ago by atagar

Hi Patrick, thanks for pushing this forward! This doesn't look to be a proper Stem repository. Instead of commits it contains a patch file (12533.patch). Please clone Stem, apply your changes, then push that to Gist...

https://stem.torproject.org/faq.html#how-do-i-get-started

comment:14 Changed 3 years ago by patrickod

Ah. I seemingly glossed over that part of the manual. Thanks!

Heres' a hosted version of this commit. I've rebased onto the latest stem master. https://github.com/patrickod/stem/pull/1/files

Would love to hear your feedback.

P

comment:15 Changed 3 years ago by atagar

Resolution: implemented
Status: newclosed

Fantastic work, thanks Patrick! Pushed with a few revisions.

Cheers! -Damian

comment:16 Changed 3 years ago by federico3

Thank you Damian and Patrick!

comment:17 Changed 3 years ago by patrickod

+1 thanks guys. A fun one to tackle to get started. :)

comment:18 Changed 3 years ago by patrickod

atagar: Any chance we could cut a patch level release of stem so that we can bump the version in OnionShare and fix the issue that this ticket solves? Would be a very clean patch to apply w/ a patch release

comment:19 Changed 3 years ago by atagar

Hi patrickod. I'd rather not cut a hotfix release for just this. What is wrong with using the git repo? Does OnionShare use the deb or something like that?

comment:20 Changed 3 years ago by atagar

Hi Patrick, hi federico. I just wanted to give you a quick heads up that I plan to soon cut stem's 1.3.0 release of which this is the main feature. I've tweaked these methods to be a little better for common scenarios and made a tutorial (based on one by Jordan Wright) featuring these...

https://stem.torproject.org/tutorials/over_the_river.html

Thanks again! -Damian

Note: See TracTickets for help on using tickets.