commit 590d548cad596df44674d68099ba69a7b230a5e7 Author: Damian Johnson atagar@torproject.org Date: Sat Dec 13 14:32:07 2014 -0800

Changing HiddenServicePort to be a three value tuple

Our Controller hidden service method's HiddenServicePort was a string. This is clunky because it left parsing that string to our caller. The string might be three different formats, anything from a simple port to two ports and an address.

Our get_hidden_service_conf() now provides a list of three value tuples...

"HiddenServicePort": [ (port, target_address, target_port), ]

This both normalizes and means no parsing necessary. --- stem/control.py | 110 ++++++++++++++++++++++++++------------ test/integ/control/controller.py | 16 +++--- 2 files changed, 83 insertions(+), 43 deletions(-)

diff --git a/stem/control.py b/stem/control.py index cace3ad..a90dfcb 100644 --- a/stem/control.py +++ b/stem/control.py @@ -2106,8 +2106,8 @@ class Controller(BaseController): "/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" + (8020, "127.0.0.1", 8020), # the ports order is kept + (8021, "127.0.0.1", 8021) ], "HiddenServiceVersion": "2" }, @@ -2150,7 +2150,25 @@ class Controller(BaseController): directory = v service_dir_map[directory] = {'HiddenServicePort': []} elif k == 'HiddenServicePort': - service_dir_map[directory]['HiddenServicePort'].append(v) + port = target_port = v + target_address = '127.0.0.1' + + if not v.isdigit(): + port, target = v.split() + + if target.isdigit(): + target_port = target + else: + target_address, target_port = target.split(':') + + if not stem.util.connection.is_valid_port(port): + raise stem.ProtocolError('GETCONF provided an invalid HiddenServicePort port (%s): %s' % (port, content)) + elif not stem.util.connection.is_valid_ipv4_address(target_address): + raise stem.ProtocolError('GETCONF provided an invalid HiddenServicePort target address (%s): %s' % (target_address, content)) + elif not stem.util.connection.is_valid_port(target_port): + raise stem.ProtocolError('GETCONF provided an invalid HiddenServicePort target port (%s): %s' % (target_port, content)) + + service_dir_map[directory]['HiddenServicePort'].append((int(port), target_address, int(target_port))) else: service_dir_map[directory][k] = v

@@ -2162,6 +2180,22 @@ class Controller(BaseController): the same format as :func:`~stem.control.Controller.get_hidden_service_conf`.

+ For convenience the HiddenServicePort entries can be an integer, string, or + tuple. If an **int** then we treat it as just a port. If a **str** we pass + that directly as the HiddenServicePort. And finally, if a **tuple** then + it's expected to be the **(port, target_address, target_port)** as provided + by :func:`~stem.control.Controller.get_hidden_service_conf`. + + This is to say the following three are equivalent... + + :: + + "HiddenServicePort": [ + 80, + '80 127.0.0.1:80', + (80, '127.0.0.1', 80), + ] + .. versionadded:: 1.3.0

:param dict conf: configuration dictionary @@ -2183,14 +2217,22 @@ class Controller(BaseController):

for k, v in conf[directory].items(): if k == 'HiddenServicePort': - for port in v: - hidden_service_options.append(('HiddenServicePort', port)) + for entry in v: + if isinstance(entry, int): + entry = '%s 127.0.0.1:%s' % (entry, entry) + elif isinstance(entry, str): + pass # just pass along what the user gave us + elif isinstance(entry, tuple): + port, target_address, target_port = entry + entry = '%s %s:%s' % (port, target_address, target_port) + + hidden_service_options.append(('HiddenServicePort', entry)) else: hidden_service_options.append((k, str(v)))

self.set_options(hidden_service_options)

- def create_hidden_service(self, path, port, target = None): + def create_hidden_service(self, path, port, target_address = None, target_port = None): """ Create a new hidden service. If the directory is already present, a new port is added. @@ -2199,7 +2241,9 @@ class Controller(BaseController):

:param str path: path for the hidden service's data directory :param int port: hidden service port - :param str target: optional ipaddr:port target e.g. '127.0.0.1:8080' + :param str target_address: address of the service, by default 127.0.0.1 + :param int target_port: port of the service, by default this is the same as + **port**

:returns: **True** if the hidden service is created, **False** if the hidden service port is already in use @@ -2209,26 +2253,31 @@ class Controller(BaseController):

if not stem.util.connection.is_valid_port(port): raise ValueError("%s isn't a valid port number" % port) + elif target_address and not stem.util.connection.is_valid_ipv4_address(target_address): + raise ValueError("%s isn't a valid IPv4 address" % target_address) + elif target_port is not None and not stem.util.connection.is_valid_port(target_port): + raise ValueError("%s isn't a valid port number" % target_port) + + port = int(port) + target_address = target_address if target_address else '127.0.0.1' + target_port = port if target_port is None else int(target_port)

- port, conf = str(port), self.get_hidden_service_conf() + conf = self.get_hidden_service_conf()

if path in conf: ports = conf[path]['HiddenServicePort']

- if target is None: - if port in ports or '%s 127.0.0.1:%s' % (port, port) in ports: - return False - elif '%s %s' % (port, target) in ports: + if (port, target_address, target_port) in ports: return False else: conf[path] = {'HiddenServicePort': []}

- conf[path]['HiddenServicePort'].append('%s %s' % (port, target) if target else port) + conf[path]['HiddenServicePort'].append((port, target_address, target_port)) self.set_hidden_service_conf(conf)

return True

- def remove_hidden_service(self, path, port = None, target = None): + def remove_hidden_service(self, path, port = None): """ Discontinues a given hidden service.

@@ -2236,7 +2285,6 @@ class Controller(BaseController):

:param str path: path for the hidden service's data directory :param int port: hidden service port - :param str target: optional ipaddr:port target e.g. '127.0.0.1:8080'

:returns: **True** if the hidden service is discontinued, **False** if it wasn't running in the first place @@ -2244,36 +2292,28 @@ class Controller(BaseController): :raises: :class:`stem.ControllerError` if the call fails """

- if not stem.util.connection.is_valid_port(port): + if port and not stem.util.connection.is_valid_port(port): raise ValueError("%s isn't a valid port number" % port)

- port, conf = str(port), self.get_hidden_service_conf() + port = int(port) if port else None + conf = self.get_hidden_service_conf()

if path not in conf: return False

- if port: - if not target: - longport = '%s 127.0.0.1:%s' % (port, port) + if not port: + del conf[path] + else: + to_remove = [entry for entry in conf[path]['HiddenServicePort'] if entry[0] == port]

- if port in conf[path]['HiddenServicePort']: - conf[path]['HiddenServicePort'].remove(port) - elif longport in conf[path]['HiddenServicePort']: - conf[path]['HiddenServicePort'].remove(longport) - else: - return False # wasn't configured to be a hidden service - else: - longport = '%s %s' % (port, target) + if not to_remove: + return False

- if longport in conf[path]['HiddenServicePort']: - conf[path]['HiddenServicePort'].remove(longport) - else: - return False # wasn't configured to be a hidden service + for entry in to_remove: + conf[path]['HiddenServicePort'].remove(entry)

if not conf[path]['HiddenServicePort']: - del(conf[path]) # no ports left, drop it entirely - else: - del(conf[path]) + del conf[path] # no ports left

self.set_hidden_service_conf(conf) return True diff --git a/test/integ/control/controller.py b/test/integ/control/controller.py index adaaf8d..386eb8e 100644 --- a/test/integ/control/controller.py +++ b/test/integ/control/controller.py @@ -482,17 +482,17 @@ class TestController(unittest.TestCase): initialconf = { 'test_hidden_service1/': { 'HiddenServicePort': [ - '8020 127.0.0.1:8020', - '8021 127.0.0.1:8021', + (8020, '127.0.0.1', 8020), + (8021, '127.0.0.1', 8021), ], 'HiddenServiceVersion': '2', }, 'test_hidden_service2/': { 'HiddenServiceAuthorizeClient': 'stealth a, b', 'HiddenServicePort': [ - '8030 127.0.0.1:8030', - '8031 127.0.0.1:8031', - '8032 127.0.0.1:8032', + (8030, '127.0.0.1', 8030), + (8031, '127.0.0.1', 8031), + (8032, '127.0.0.1', 8032), ] }, 'test_hidden_service_empty/': { @@ -506,13 +506,13 @@ class TestController(unittest.TestCase): # add already existing services, with/without explicit target

self.assertFalse(controller.create_hidden_service('test_hidden_service1/', 8020)) - self.assertFalse(controller.create_hidden_service('test_hidden_service1/', 8021, target = '127.0.0.1:8021')) + self.assertFalse(controller.create_hidden_service('test_hidden_service1/', 8021, target_port = 8021)) self.assertDictEqual(initialconf, controller.get_hidden_service_conf())

# add a new service, with/without explicit target

self.assertTrue(controller.create_hidden_service('test_hidden_serviceX/', 8888)) - self.assertTrue(controller.create_hidden_service('test_hidden_serviceX/', 8989, target = '127.0.0.1:8021')) + self.assertTrue(controller.create_hidden_service('test_hidden_serviceX/', 8989, target_port = 8021))

conf = controller.get_hidden_service_conf() self.assertEqual(4, len(conf)) @@ -525,7 +525,7 @@ class TestController(unittest.TestCase):

# remove a service completely, it should now be gone

- controller.remove_hidden_service('test_hidden_serviceX/', 8989, target = '127.0.0.1:8021') + controller.remove_hidden_service('test_hidden_serviceX/', 8989) self.assertEqual(3, len(controller.get_hidden_service_conf())) finally: controller.set_hidden_service_conf({}) # drop hidden services created during the test