[tor-commits] [stem/master] Provide None from get_exit_policy when not a relay

atagar at torproject.org atagar at torproject.org
Sat Aug 4 22:04:43 UTC 2018


commit 542fa1f60552c259e3510e27727a5a2d5f20b7e9
Author: Damian Johnson <atagar at 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):
     """



More information about the tor-commits mailing list