SSL context handling, #318

Author: akpw

mktxp/flow/router_connection.py

@@ -54,26 +54,11 @@ def wrapper(self, *args, **kwargs):
 
 class RouterAPIConnection:
     ''' Base wrapper interface for the routeros_api library
-    '''            
+    '''
     def __init__(self, router_name, config_entry):
         self.router_name = router_name
         self.config_entry = config_entry
         self.last_failure_timestamp = self.successive_failure_count = 0
-        
-        ctx = None
-        if self.config_entry.use_ssl:
-            ctx = ssl.create_default_context(
-                cafile=self.config_entry.ssl_ca_file if self.config_entry.ssl_ca_file else None
-            )
-            if self.config_entry.no_ssl_certificate:
-                ctx.check_hostname = False
-                ctx.set_ciphers('ADH:@SECLEVEL=0')
-            elif not self.config_entry.ssl_certificate_verify:
-                ctx.check_hostname = False
-                ctx.verify_mode = ssl.CERT_NONE
-            else:
-                ctx.check_hostname = self.config_entry.ssl_check_hostname
-                ctx.verify_mode = ssl.CERT_REQUIRED
 
         username = self.config_entry.username
         password = self.config_entry.password
@@ -85,7 +70,7 @@ def __init__(self, router_name, config_entry):
                         raise yaml.YAMLError("Credentials file is not a valid key-value map.")
                 except yaml.YAMLError as e:
                     print(f"Error parsing credentials file: {e}\nCheck that it is valid YAML (e.g., 'username: user').")
-                    exit(1)                    
+                    exit(1)
                 username = credentials.get('username', username)
                 password = credentials.get('password', password)
 
@@ -95,11 +80,30 @@ def __init__(self, router_name, config_entry):
                 password = password,
                 port = self.config_entry.port,
                 plaintext_login = True,
-                ssl_context = ctx)
-        
+                ssl_context = None)
+
         self.connection.socket_timeout = config_handler.system_entry.socket_timeout
         self.api = None
-    
+
+    def _build_ssl_context(self):
+        if not self.config_entry.use_ssl:
+            return None
+
+        ctx = ssl.create_default_context(
+            cafile = self.config_entry.ssl_ca_file if self.config_entry.ssl_ca_file else None
+        )
+        if self.config_entry.no_ssl_certificate:
+            ctx.check_hostname = False
+            ctx.set_ciphers('ADH:@SECLEVEL=0')
+        elif not self.config_entry.ssl_certificate_verify:
+            ctx.check_hostname = False
+            ctx.verify_mode = ssl.CERT_NONE
+        else:
+            ctx.check_hostname = self.config_entry.ssl_check_hostname
+            ctx.verify_mode = ssl.CERT_REQUIRED
+
+        return ctx
+
     def is_connected(self):
         if self.connection and self.connection.connected and self.api:
             return True
@@ -113,6 +117,8 @@ def connect(self):
             print(f'Connecting to router {self.router_name}@{self.config_entry.hostname}')
             if self.config_entry.use_ssl and self.config_entry.no_ssl_certificate:
                 print(f'Warning: API_SSL connect without router SSL certificate is insecure and should not be used in production environments!')
+            if self.config_entry.use_ssl:
+                self.connection.ssl_context = self._build_ssl_context()
             self.connection.plaintext_login = self.config_entry.plaintext_login
             self.api = self.connection.get_api()
             self._set_connect_state(success = True, connect_time = connect_time)
@@ -132,11 +138,11 @@ def router_api(self):
     def _in_connect_timeout(self, connect_timestamp):
         connect_delay = self._connect_delay()
         if (connect_timestamp - self.last_failure_timestamp) < connect_delay:
-            if config_handler.system_entry.verbose_mode: 
+            if config_handler.system_entry.verbose_mode:
                 print(f'{self.router_name}@{self.config_entry.hostname}: in connect timeout, {int(connect_delay - (connect_timestamp - self.last_failure_timestamp))}secs remaining')
                 print(f'Successive failure count: {self.successive_failure_count}')
             return True
-        if config_handler.system_entry.verbose_mode: 
+        if config_handler.system_entry.verbose_mode:
             print(f'{self.router_name}@{self.config_entry.hostname}: OK to connect')
             if self.last_failure_timestamp > 0:
                 print(f'Seconds since last failure: {connect_timestamp - self.last_failure_timestamp}')
@@ -157,5 +163,5 @@ def _set_connect_state(self, success = False, connect_time = datetime.now(), exc
         else:
             self.api = None
             self.successive_failure_count += 1
-            self.last_failure_timestamp = connect_time.timestamp() 
+            self.last_failure_timestamp = connect_time.timestamp()
             print(f'{connect_time.strftime("%Y-%m-%d %H:%M:%S")} Connection to router {self.router_name}@{self.config_entry.hostname} has failed: {exc}')

tests/flow/test_router_connection.py

@@ -13,16 +13,19 @@
 
 import pytest
 import routeros_api.api_structure
+from types import SimpleNamespace
+from unittest.mock import MagicMock, patch
 # monkey patch via importing the module
 import mktxp.flow.router_connection
+from mktxp.flow.router_connection import RouterAPIConnection, RouterAPIConnectionError
 
 @pytest.mark.parametrize("input_bytes, expected_string", [
-    (b'But are these differences really that small?', 'But are these differences really that small?'),  
-    (b'\xe4\xbd\x86\xe6\x98\xaf\xef\xbc\x8c\xe8\xbf\x99\xe4\xba\x9b\xe5\xb7\xae\xe5\xbc\x82\xe7\x9c\x9f\xe7\x9a\x84\xe9\x82\xa3\xe4\xb9\x88\xe5\xb0\x8f\xe5\x90\x97\xef\xbc\x9f', '但是，这些差异真的那么小吗？'),  
-    (b'Mas ser\xc3\xa3o estas diferen\xc3\xa7as assim t\xc3\xa3o pequenas?', 'Mas serão estas diferenças assim tão pequenas?'),  
-    (b'\xd0\x9d\xd0\xbe \xd0\xb4\xd0\xb5\xd0\xb9\xd1\x81\xd1\x82\xd0\xb2\xd0\xb8\xd1\x82\xd0\xb5\xd0\xbb\xd1\x8c\xd0\xbd\xd0\xbe \xd0\xbb\xd0\xb8 \xd1\x8d\xd1\x82\xd0\xb8 \xd1\x80\xd0\xb0\xd0\xb7\xd0\xbb\xd0\xb8\xd1\x87\xd0\xb8\xd1\x8f \xd1\x82\xd0\xb0\xd0\xba \xd0\xbc\xd0\xb0\xd0\xbb\xd1\x8b?', 'Но действительно ли эти различия так малы?'),  
-    (b'\xd0\x90\xd0\xbb\xd0\xb5 \xd1\x87\xd0\xb8 \xd1\x81\xd0\xbf\xd1\x80\xd0\xb0\xd0\xb2\xd0\xb4\xd1\x96 \xd1\x86\xd1\x96 \xd0\xb2\xd1\x96\xd0\xb4\xd0\xbc\xd1\x96\xd0\xbd\xd0\xbd\xd0\xbe\xd1\x81\xd1\x82\xd1\x96 \xd1\x82\xd0\xb0\xd0\xba\xd1\x96 \xd0\xbd\xd0\xb5\xd0\xb7\xd0\xbd\xd0\xb0\xd1\x87\xd0\xbd\xd1\x96?', 'Але чи справді ці відмінності такі незначні?'),  
-    (b'Jsou v\xc5\xa1ak tyto rozd\xc3\xadly opravdu tak mal\xc3\xa9?', 'Jsou však tyto rozdíly opravdu tak malé?'),  
+    (b'But are these differences really that small?', 'But are these differences really that small?'),
+    (b'\xe4\xbd\x86\xe6\x98\xaf\xef\xbc\x8c\xe8\xbf\x99\xe4\xba\x9b\xe5\xb7\xae\xe5\xbc\x82\xe7\x9c\x9f\xe7\x9a\x84\xe9\x82\xa3\xe4\xb9\x88\xe5\xb0\x8f\xe5\x90\x97\xef\xbc\x9f', '但是，这些差异真的那么小吗？'),
+    (b'Mas ser\xc3\xa3o estas diferen\xc3\xa7as assim t\xc3\xa3o pequenas?', 'Mas serão estas diferenças assim tão pequenas?'),
+    (b'\xd0\x9d\xd0\xbe \xd0\xb4\xd0\xb5\xd0\xb9\xd1\x81\xd1\x82\xd0\xb2\xd0\xb8\xd1\x82\xd0\xb5\xd0\xbb\xd1\x8c\xd0\xbd\xd0\xbe \xd0\xbb\xd0\xb8 \xd1\x8d\xd1\x82\xd0\xb8 \xd1\x80\xd0\xb0\xd0\xb7\xd0\xbb\xd0\xb8\xd1\x87\xd0\xb8\xd1\x8f \xd1\x82\xd0\xb0\xd0\xba \xd0\xbc\xd0\xb0\xd0\xbb\xd1\x8b?', 'Но действительно ли эти различия так малы?'),
+    (b'\xd0\x90\xd0\xbb\xd0\xb5 \xd1\x87\xd0\xb8 \xd1\x81\xd0\xbf\xd1\x80\xd0\xb0\xd0\xb2\xd0\xb4\xd1\x96 \xd1\x86\xd1\x96 \xd0\xb2\xd1\x96\xd0\xb4\xd0\xbc\xd1\x96\xd0\xbd\xd0\xbd\xd0\xbe\xd1\x81\xd1\x82\xd1\x96 \xd1\x82\xd0\xb0\xd0\xba\xd1\x96 \xd0\xbd\xd0\xb5\xd0\xb7\xd0\xbd\xd0\xb0\xd1\x87\xd0\xbd\xd1\x96?', 'Але чи справді ці відмінності такі незначні?'),
+    (b'Jsou v\xc5\xa1ak tyto rozd\xc3\xadly opravdu tak mal\xc3\xa9?', 'Jsou však tyto rozdíly opravdu tak malé?'),
     (b'\xa1\xa3', '¡£'),  # Some latin-1 characters
     (b'J\xf6rgensen', 'Jörgensen'),
     (b'fran\xe7aise', 'française'),
@@ -33,3 +36,58 @@
 def test_decode_bytes_monkey_patch(input_bytes, expected_string):
     field = routeros_api.api_structure.StringField()
     assert field.get_python_value(input_bytes) == expected_string
+
+
+def _config_entry(use_ssl = True):
+    return SimpleNamespace(
+        hostname = '192.0.2.1',
+        port = 8729,
+        username = 'user',
+        password = 'pass',
+        credentials_file = '',
+        use_ssl = use_ssl,
+        no_ssl_certificate = False,
+        ssl_certificate_verify = False,
+        ssl_check_hostname = True,
+        ssl_ca_file = '',
+        plaintext_login = True,
+    )
+
+
+def test_router_connection_builds_fresh_ssl_context_for_each_connect_attempt():
+    ctx1 = MagicMock()
+    ctx2 = MagicMock()
+    pool = MagicMock()
+    pool.connected = False
+    pool.get_api.side_effect = [Exception('ssl boom'), object()]
+
+    with patch('mktxp.flow.router_connection.RouterOsApiPool', return_value = pool) as routeros_pool, \
+         patch('mktxp.flow.router_connection.ssl.create_default_context', side_effect = [ctx1, ctx2]):
+        connection = RouterAPIConnection('router-a', _config_entry())
+
+        # SSL context should no longer be created eagerly in __init__.
+        assert routeros_pool.call_args.kwargs['ssl_context'] is None
+
+        connection._in_connect_timeout = lambda *_: False
+
+        with pytest.raises(RouterAPIConnectionError):
+            connection.connect()
+        assert pool.ssl_context is ctx1
+
+        connection.connect()
+        assert pool.ssl_context is ctx2
+
+
+def test_router_connection_skips_ssl_context_building_when_ssl_disabled():
+    pool = MagicMock()
+    pool.connected = False
+    pool.get_api.return_value = object()
+
+    with patch('mktxp.flow.router_connection.RouterOsApiPool', return_value = pool) as routeros_pool, \
+         patch('mktxp.flow.router_connection.ssl.create_default_context') as create_context:
+        connection = RouterAPIConnection('router-a', _config_entry(use_ssl = False))
+        connection._in_connect_timeout = lambda *_: False
+        connection.connect()
+
+    create_context.assert_not_called()
+    assert routeros_pool.call_args.kwargs['ssl_context'] is None