commit 542fa1f60552c259e3510e27727a5a2d5f20b7e9 Author: Damian Johnson atagar@torproject.org Date: Sat Aug 4 14:55:40 2018 -0700

Provide None from get_exit_policy when not a relay

When not configured to be a tor relay tor's 'GETINFO exit-policy/full' query won't function. Rather than attempt to guess the exit policy from other configuration options providing None for 552 responses...

https://trac.torproject.org/projects/tor/ticket/25853 https://gitweb.torproject.org/torspec.git/commit/?id=c5453a0

The above spec addition makes it ambiguous if 552 means 'not a relay' or 'we had an error' so reached out for clarificaition on how these should be handled...

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

In the process also had to fix get_info to raise OperationFailed rather than a ProtocolError so callers can get the underlying response codes. --- docs/change_log.rst | 2 ++ stem/control.py | 41 +++++++++++++---------------------------- stem/response/getinfo.py | 8 ++++++++ test/unit/control/controller.py | 29 ++++++++++++++--------------- 4 files changed, 37 insertions(+), 43 deletions(-)

diff --git a/docs/change_log.rst b/docs/change_log.rst index ca8b6805..282a02c8 100644 --- a/docs/change_log.rst +++ b/docs/change_log.rst @@ -52,7 +52,9 @@ The following are only available within Stem's `git repository * Stacktrace if :func:`stem.connection.connect` had a string port argument * More reliable ExitPolicy resolution (:trac:`25739`) * More reliable caching during configuration changes, especially in multiple-controller situations (:trac:`25821`) + * :func:`~stem.control.Controller.get_info` commonly raised :class:`stem.ProtocolError` when it should provide :class:`stem.OperationFailed` * :func:`~stem.control.Controller.get_microdescriptors` reads descriptors from the control port if available (:spec:`b5396d5`) + * :func:`~stem.control.Controller.get_exit_policy` now provides None if not configured to be a relay (:trac:`25853`, :spec:`c5453a0`) * Added the delivered_read, delivered_written, overhead_read, and overhead_written attributes to :class:`~stem.response.events.CircuitBandwidthEvent` (:spec:`fbb38ec`) * The *config* attribute of :class:`~stem.response.events.ConfChangedEvent` couldn't represent tor configuration options with multiple values. It has been replaced with new *changed* and *unset* attributes. * Replaced socket's :func:`~stem.socket.ControlPort.get_address`, :func:`~stem.socket.ControlPort.get_port`, and :func:`~stem.socket.ControlSocketFile.get_socket_path` with attributes diff --git a/stem/control.py b/stem/control.py index 77e6140e..19a8759d 100644 --- a/stem/control.py +++ b/stem/control.py @@ -1127,6 +1127,10 @@ class Controller(BaseController): .. versionchanged:: 1.1.0 Added the get_bytes argument.

+ .. versionchanged:: 1.7.0 + Errors commonly provided a :class:`stem.ProtocolError` when we should + raise a :class:`stem.OperationFailed`. + :param str,list params: GETINFO option or options to be queried :param object default: response if the query fails :param bool get_bytes: provides **bytes** values rather than a **str** under python 3.x @@ -1275,10 +1279,13 @@ class Controller(BaseController): parsing the user's torrc entries. This should be more reliable for some edge cases. (:trac:`25739`)

+ .. versionchanged:: 1.7.0 + Returning **None** if not contigured to be a relay. + :param object default: response if the query fails

:returns: :class:`~stem.exit_policy.ExitPolicy` of the tor instance that - we're connected to + we're connected to, this is **None** if not configured to be a relay

:raises: * :class:`stem.ControllerError` if unable to query the policy @@ -1292,34 +1299,12 @@ class Controller(BaseController): if not policy: try: policy = stem.exit_policy.ExitPolicy(*self.get_info('exit-policy/full').splitlines()) - except stem.ProtocolError: - # When tor is unable to determine our address 'GETINFO - # exit-policy/full' fails with... - # - # ProtocolError: GETINFO response didn't have an OK status: - # router_get_my_routerinfo returned NULL - # - # https://trac.torproject.org/projects/tor/ticket/25842 - # - # Failing back to the legacy method we used for getting our exit - # policy. + self._set_cache({'exit_policy': policy}) + except stem.OperationFailed as exc: + if exc.code == '552': + return None # not configured to be a relay

- rules = [] - - if self.get_conf('ExitRelay') == '0': - rules.append('reject *:*') - - if self.get_conf('ExitPolicyRejectPrivate') == '1': - rules.append('reject private:*') - - for policy_line in self.get_conf('ExitPolicy', multiple = True): - rules += policy_line.split(',') - - rules += self.get_info('exit-policy/default').split(',') - - policy = stem.exit_policy.get_config_policy(rules, self.get_info('address', None)) - - self._set_cache({'exit_policy': policy}) + raise

return policy

diff --git a/stem/response/getinfo.py b/stem/response/getinfo.py index 03a2cf38..00c77b92 100644 --- a/stem/response/getinfo.py +++ b/stem/response/getinfo.py @@ -30,12 +30,20 @@ class GetInfoResponse(stem.response.ControlMessage):

if not self.is_ok() or not remaining_lines.pop() == b'OK': unrecognized_keywords = [] + error_code, error_msg = None, None + for code, _, line in self.content(): + if code != '250': + error_code = code + error_msg = line + if code == '552' and line.startswith('Unrecognized key "') and line.endswith('"'): unrecognized_keywords.append(line[18:-1])

if unrecognized_keywords: raise stem.InvalidArguments('552', 'GETINFO request contained unrecognized keywords: %s\n' % ', '.join(unrecognized_keywords), unrecognized_keywords) + elif error_code: + raise stem.OperationFailed(error_code, error_msg) else: raise stem.ProtocolError("GETINFO response didn't have an OK status:\n%s" % self)

diff --git a/test/unit/control/controller.py b/test/unit/control/controller.py index d8e1cc9d..3bdb7dce 100644 --- a/test/unit/control/controller.py +++ b/test/unit/control/controller.py @@ -58,19 +58,19 @@ class TestControl(unittest.TestCase): msg_mock.return_value = ControlMessage.from_str('551 Address unknown\r\n')

self.assertEqual(None, self.controller._last_address_exc) - self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') - self.assertEqual("GETINFO response didn't have an OK status:\nAddress unknown", str(self.controller._last_address_exc)) + self.assertRaisesRegexp(stem.OperationFailed, 'Address unknown', self.controller.get_info, 'address') + self.assertEqual('Address unknown', str(self.controller._last_address_exc)) self.assertEqual(1, msg_mock.call_count)

# now that we have a cached failure we should provide that back

- self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') + self.assertRaisesRegexp(stem.OperationFailed, 'Address unknown', self.controller.get_info, 'address') self.assertEqual(1, msg_mock.call_count)

# invalidates the cache, transitioning from no address to having one

msg_mock.return_value = ControlMessage.from_str('250-address=17.2.89.80\r\n250 OK\r\n', 'GETINFO') - self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') + self.assertRaisesRegexp(stem.OperationFailed, 'Address unknown', self.controller.get_info, 'address') self.controller._handle_event(ControlMessage.from_str('650 STATUS_SERVER NOTICE EXTERNAL_ADDRESS ADDRESS=17.2.89.80 METHOD=DIRSERV\r\n')) self.assertEqual('17.2.89.80', self.controller.get_info('address'))

@@ -88,19 +88,19 @@ class TestControl(unittest.TestCase): get_conf_mock.return_value = None

self.assertEqual(None, self.controller._last_fingerprint_exc) - self.assertRaisesRegexp(stem.ProtocolError, 'Not running in server mode', self.controller.get_info, 'fingerprint') - self.assertEqual("GETINFO response didn't have an OK status:\nNot running in server mode", str(self.controller._last_fingerprint_exc)) + self.assertRaisesRegexp(stem.OperationFailed, 'Not running in server mode', self.controller.get_info, 'fingerprint') + self.assertEqual('Not running in server mode', str(self.controller._last_fingerprint_exc)) self.assertEqual(1, msg_mock.call_count)

# now that we have a cached failure we should provide that back

- self.assertRaisesRegexp(stem.ProtocolError, 'Not running in server mode', self.controller.get_info, 'fingerprint') + self.assertRaisesRegexp(stem.OperationFailed, 'Not running in server mode', self.controller.get_info, 'fingerprint') self.assertEqual(1, msg_mock.call_count)

# ... but if we become a relay we'll call it again

get_conf_mock.return_value = '443' - self.assertRaisesRegexp(stem.ProtocolError, 'Not running in server mode', self.controller.get_info, 'fingerprint') + self.assertRaisesRegexp(stem.OperationFailed, 'Not running in server mode', self.controller.get_info, 'fingerprint') self.assertEqual(2, msg_mock.call_count)

@patch('stem.control.Controller.get_info') @@ -158,17 +158,11 @@ class TestControl(unittest.TestCase): self.controller._is_caching_enabled = True

@patch('stem.control.Controller.get_info') - @patch('stem.control.Controller.get_conf') - def test_get_exit_policy(self, get_conf_mock, get_info_mock): + def test_get_exit_policy(self, get_info_mock): """ Exercises the get_exit_policy() method. """

- get_conf_mock.side_effect = lambda param, **kwargs: { - 'ExitPolicyRejectPrivate': '1', - 'ExitPolicy': ['accept *:80, accept *:443', 'accept 43.5.5.5,reject *:22'], - }[param] - get_info_mock.side_effect = lambda param, default = None: { 'exit-policy/full': 'reject *:25,reject *:119,reject *:135-139,reject *:445,reject *:563,reject *:1214,reject *:4661-4666,reject *:6346-6429,reject *:6699,reject *:6881-6999,accept *:*', }[param] @@ -190,6 +184,11 @@ class TestControl(unittest.TestCase): self.assertEqual(expected, self.controller.get_exit_policy())

@patch('stem.control.Controller.get_info') + def test_get_exit_policy_if_not_relaying(self, get_info_mock): + get_info_mock.side_effect = stem.OperationFailed('552', 'Not running in server mode') + self.assertEqual(None, self.controller.get_exit_policy()) + + @patch('stem.control.Controller.get_info') @patch('stem.control.Controller.get_conf') def test_get_ports(self, get_conf_mock, get_info_mock): """