From 4d003eb709dcc4286d7781159513a4ee66b22889 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Thu, 12 Jan 2023 22:11:01 +0100 Subject: [PATCH 1/7] allow to use any port as TLS port fixes #140 --- adafruit_minimqtt/adafruit_minimqtt.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/adafruit_minimqtt/adafruit_minimqtt.py b/adafruit_minimqtt/adafruit_minimqtt.py index 2d74dd92..240bd510 100644 --- a/adafruit_minimqtt/adafruit_minimqtt.py +++ b/adafruit_minimqtt/adafruit_minimqtt.py @@ -222,11 +222,12 @@ def __init__( self.port = MQTT_TCP_PORT if is_ssl: + self._is_ssl = True self.port = MQTT_TLS_PORT if port: self.port = port - # define client identifer + # define client identifier if client_id: # user-defined client_id MAY allow client_id's > 23 bytes or # non-alpha-numeric characters @@ -282,12 +283,12 @@ def _get_connect_socket(self, host, port, *, timeout=1): if not isinstance(port, int): raise RuntimeError("Port must be an integer") - if port == MQTT_TLS_PORT and not self._ssl_context: + if self._is_ssl and not self._ssl_context: raise RuntimeError( "ssl_context must be set before using adafruit_mqtt for secure MQTT." ) - if port == MQTT_TLS_PORT: + if self._is_ssl: self.logger.info(f"Establishing a SECURE SSL connection to {host}:{port}") else: self.logger.info(f"Establishing an INSECURE connection to {host}:{port}") @@ -306,7 +307,7 @@ def _get_connect_socket(self, host, port, *, timeout=1): raise TemporaryError from exc connect_host = addr_info[-1][0] - if port == MQTT_TLS_PORT: + if self._is_ssl: sock = self._ssl_context.wrap_socket(sock, server_hostname=host) connect_host = host sock.settimeout(timeout) From e95d9f285afc57af4022d9e1780939875554ccde Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Thu, 12 Jan 2023 23:04:35 +0100 Subject: [PATCH 2/7] fixup the examples --- examples/cpython/minimqtt_adafruitio_cpython.py | 3 +-- examples/ethernet/minimqtt_simpletest_eth.py | 6 +++++- .../minimqtt_adafruitio_native_networking.py | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/examples/cpython/minimqtt_adafruitio_cpython.py b/examples/cpython/minimqtt_adafruitio_cpython.py index 7eb4f5fb..18bfd251 100644 --- a/examples/cpython/minimqtt_adafruitio_cpython.py +++ b/examples/cpython/minimqtt_adafruitio_cpython.py @@ -46,8 +46,7 @@ def message(client, topic, message): # Set up a MiniMQTT Client mqtt_client = MQTT.MQTT( - broker=secrets["broker"], - port=1883, + broker="io.adafruit.com", username=secrets["aio_username"], password=secrets["aio_key"], socket_pool=socket, diff --git a/examples/ethernet/minimqtt_simpletest_eth.py b/examples/ethernet/minimqtt_simpletest_eth.py index c585cf78..3e91c8fc 100644 --- a/examples/ethernet/minimqtt_simpletest_eth.py +++ b/examples/ethernet/minimqtt_simpletest_eth.py @@ -67,8 +67,12 @@ def publish(client, userdata, topic, pid): MQTT.set_socket(socket, eth) # Set up a MiniMQTT Client +# NOTE: We'll need to connect insecurely for ethernet configurations. client = MQTT.MQTT( - broker=secrets["broker"], username=secrets["user"], password=secrets["pass"] + broker=secrets["broker"], + username=secrets["user"], + password=secrets["pass"], + is_ssl=False, ) # Connect callback handlers to client diff --git a/examples/native_networking/minimqtt_adafruitio_native_networking.py b/examples/native_networking/minimqtt_adafruitio_native_networking.py index a4f5ecaa..21661d31 100644 --- a/examples/native_networking/minimqtt_adafruitio_native_networking.py +++ b/examples/native_networking/minimqtt_adafruitio_native_networking.py @@ -62,7 +62,7 @@ def message(client, topic, message): # Set up a MiniMQTT Client mqtt_client = MQTT.MQTT( - broker=secrets["broker"], + broker="io.adafruit.com", port=secrets["port"], username=secrets["aio_username"], password=secrets["aio_key"], From 831420a5fa4394d66afdd0322b1f1978fa6154e4 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Wed, 15 Feb 2023 20:33:26 +0100 Subject: [PATCH 3/7] change the behavior - insecure is default also add tests --- adafruit_minimqtt/adafruit_minimqtt.py | 11 ++- tests/test_port_ssl.py | 132 +++++++++++++++++++++++++ tox.ini | 11 +++ 3 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 tests/test_port_ssl.py create mode 100644 tox.ini diff --git a/adafruit_minimqtt/adafruit_minimqtt.py b/adafruit_minimqtt/adafruit_minimqtt.py index 240bd510..519aae2f 100644 --- a/adafruit_minimqtt/adafruit_minimqtt.py +++ b/adafruit_minimqtt/adafruit_minimqtt.py @@ -171,7 +171,7 @@ def __init__( username=None, password=None, client_id=None, - is_ssl=True, + is_ssl=None, keep_alive=60, recv_timeout=10, socket_pool=None, @@ -220,9 +220,14 @@ def __init__( ): # [MQTT-3.1.3.5] raise MMQTTException("Password length is too large.") + # The connection will be insecure unless is_ssl is set to True. + # If the port is not specified, the security will be set based on the is_ssl parameter. + # If the port is specified, the is_ssl parameter will be honored. self.port = MQTT_TCP_PORT - if is_ssl: - self._is_ssl = True + if is_ssl is None: + is_ssl = False + self._is_ssl = is_ssl + if self._is_ssl: self.port = MQTT_TLS_PORT if port: self.port = port diff --git a/tests/test_port_ssl.py b/tests/test_port_ssl.py new file mode 100644 index 00000000..9bbb36f7 --- /dev/null +++ b/tests/test_port_ssl.py @@ -0,0 +1,132 @@ +# SPDX-FileCopyrightText: 2023 Vladimír Kotal +# +# SPDX-License-Identifier: Unlicense + +"""tests that verify the connect behavior w.r.t. port number and TLS""" + +import socket +import ssl +from unittest import TestCase, main +from unittest.mock import Mock, call, patch + +import adafruit_minimqtt.adafruit_minimqtt as MQTT + + +class PortSslSetup(TestCase): + """This class contains tests that verify how host/port and TLS is set for connect(). + These tests assume that there is no MQTT broker running on the hosts/ports they connect to. + """ + + def test_default_port(self) -> None: + """verify default port value and that TLS is not used""" + host = "127.0.0.1" + port = 1883 + + with patch.object(socket.socket, "connect") as connect_mock: + ssl_context = ssl.create_default_context() + mqtt_client = MQTT.MQTT( + broker=host, + socket_pool=socket, + ssl_context=ssl_context, + connect_retries=1, + ) + + ssl_mock = Mock() + ssl_context.wrap_socket = ssl_mock + + with self.assertRaises(OSError): + expected_port = port + mqtt_client.connect() + + ssl_mock.assert_not_called() + connect_mock.assert_called() + # Assuming the repeated calls will have the same arguments. + connect_mock.assert_has_calls( + [call((host, expected_port))] + ) + + def test_connect_override(self): + """Test that connect() can override host and port.""" + host = "127.0.0.1" + port = 1883 + + with patch.object(socket.socket, "connect") as connect_mock: + connect_mock.side_effect = OSError("artificial error") + mqtt_client = MQTT.MQTT( + broker=host, + port=port, + socket_pool=socket, + connect_retries=1, + ) + + with self.assertRaises(OSError): + expected_host = "127.0.0.2" + expected_port = 1884 + self.assertNotEqual( + expected_port, port, "port override should differ" + ) + self.assertNotEqual( + expected_host, host, "host override should differ" + ) + mqtt_client.connect(host=expected_host, port=expected_port) + + connect_mock.assert_called() + # Assuming the repeated calls will have the same arguments. + connect_mock.assert_has_calls( + [call((expected_host, expected_port))] + ) + + def test_tls_port(self) -> None: + """verify that when is_ssl=True is set, the default port is 8883 + and the socket is TLS wrapped. Also test that the TLS port can be overridden.""" + host = "127.0.0.1" + + for port in [None, 8884]: + if port is None: + expected_port = 8883 + else: + expected_port = port + with self.subTest(): + ssl_mock = Mock() + mqtt_client = MQTT.MQTT( + broker=host, + port=port, + socket_pool=socket, + is_ssl=True, + ssl_context=ssl_mock, + connect_retries=1, + ) + + socket_mock = Mock() + connect_mock = Mock(side_effect=OSError) + socket_mock.connect = connect_mock + ssl_mock.wrap_socket = Mock(return_value=socket_mock) + + with self.assertRaises(RuntimeError): + mqtt_client.connect() + + ssl_mock.wrap_socket.assert_called() + + connect_mock.assert_called() + # Assuming the repeated calls will have the same arguments. + connect_mock.assert_has_calls([call((host, expected_port))]) + + def test_tls_without_ssl_context(self) -> None: + """verify that when is_ssl=True is set, the code will check that ssl_context is not None""" + host = "127.0.0.1" + + mqtt_client = MQTT.MQTT( + broker=host, + socket_pool=socket, + is_ssl=True, + ssl_context=None, + connect_retries=1, + ) + + with self.assertRaises(RuntimeError) as context: + mqtt_client.connect() + self.assertTrue("ssl_context must be set" in str(context)) + + +if __name__ == "__main__": + main() diff --git a/tox.ini b/tox.ini new file mode 100644 index 00000000..6a9584b3 --- /dev/null +++ b/tox.ini @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: 2023 Vladimír Kotal +# +# SPDX-License-Identifier: MIT + +[tox] +envlist = py39 + +[testenv] +changedir = {toxinidir}/tests +deps = pytest==6.2.5 +commands = pytest -v From f304d7f621b1f5fb54c77c85495924818518b04e Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Wed, 15 Feb 2023 20:52:06 +0100 Subject: [PATCH 4/7] fix tests --- tests/test_port_ssl.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_port_ssl.py b/tests/test_port_ssl.py index 9bbb36f7..3fdf6a3b 100644 --- a/tests/test_port_ssl.py +++ b/tests/test_port_ssl.py @@ -34,7 +34,7 @@ def test_default_port(self) -> None: ssl_mock = Mock() ssl_context.wrap_socket = ssl_mock - with self.assertRaises(OSError): + with self.assertRaises(MQTT.MMQTTException): expected_port = port mqtt_client.connect() @@ -59,7 +59,7 @@ def test_connect_override(self): connect_retries=1, ) - with self.assertRaises(OSError): + with self.assertRaises(MQTT.MMQTTException): expected_host = "127.0.0.2" expected_port = 1884 self.assertNotEqual( @@ -102,7 +102,7 @@ def test_tls_port(self) -> None: socket_mock.connect = connect_mock ssl_mock.wrap_socket = Mock(return_value=socket_mock) - with self.assertRaises(RuntimeError): + with self.assertRaises(MQTT.MMQTTException): mqtt_client.connect() ssl_mock.wrap_socket.assert_called() From 5f3fc076f5fd9ba43d3a698b71687fce76719a73 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Wed, 15 Feb 2023 20:59:28 +0100 Subject: [PATCH 5/7] apply black --- tests/test_port_ssl.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/tests/test_port_ssl.py b/tests/test_port_ssl.py index 3fdf6a3b..8474b56d 100644 --- a/tests/test_port_ssl.py +++ b/tests/test_port_ssl.py @@ -41,9 +41,7 @@ def test_default_port(self) -> None: ssl_mock.assert_not_called() connect_mock.assert_called() # Assuming the repeated calls will have the same arguments. - connect_mock.assert_has_calls( - [call((host, expected_port))] - ) + connect_mock.assert_has_calls([call((host, expected_port))]) def test_connect_override(self): """Test that connect() can override host and port.""" @@ -62,19 +60,13 @@ def test_connect_override(self): with self.assertRaises(MQTT.MMQTTException): expected_host = "127.0.0.2" expected_port = 1884 - self.assertNotEqual( - expected_port, port, "port override should differ" - ) - self.assertNotEqual( - expected_host, host, "host override should differ" - ) + self.assertNotEqual(expected_port, port, "port override should differ") + self.assertNotEqual(expected_host, host, "host override should differ") mqtt_client.connect(host=expected_host, port=expected_port) connect_mock.assert_called() # Assuming the repeated calls will have the same arguments. - connect_mock.assert_has_calls( - [call((expected_host, expected_port))] - ) + connect_mock.assert_has_calls([call((expected_host, expected_port))]) def test_tls_port(self) -> None: """verify that when is_ssl=True is set, the default port is 8883 From ed4d2ae75f9174e7fc2fbfb0bfe44cb0d1312a31 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Wed, 15 Feb 2023 21:04:10 +0100 Subject: [PATCH 6/7] use TLS when possible --- examples/cpython/minimqtt_adafruitio_cpython.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/examples/cpython/minimqtt_adafruitio_cpython.py b/examples/cpython/minimqtt_adafruitio_cpython.py index 18bfd251..dbe62bdb 100644 --- a/examples/cpython/minimqtt_adafruitio_cpython.py +++ b/examples/cpython/minimqtt_adafruitio_cpython.py @@ -1,8 +1,10 @@ # SPDX-FileCopyrightText: 2021 ladyada for Adafruit Industries # SPDX-License-Identifier: MIT -import time import socket +import ssl +import time + import adafruit_minimqtt.adafruit_minimqtt as MQTT ### Secrets File Setup ### @@ -50,6 +52,8 @@ def message(client, topic, message): username=secrets["aio_username"], password=secrets["aio_key"], socket_pool=socket, + is_ssl=True, + ssl_context=ssl.create_default_context() ) # Setup the callback methods above From 768c046daa53253b7caf5615a426ad48f7f417b7 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Wed, 15 Feb 2023 21:21:49 +0100 Subject: [PATCH 7/7] apply black --- examples/cpython/minimqtt_adafruitio_cpython.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/cpython/minimqtt_adafruitio_cpython.py b/examples/cpython/minimqtt_adafruitio_cpython.py index dbe62bdb..3667329b 100644 --- a/examples/cpython/minimqtt_adafruitio_cpython.py +++ b/examples/cpython/minimqtt_adafruitio_cpython.py @@ -53,7 +53,7 @@ def message(client, topic, message): password=secrets["aio_key"], socket_pool=socket, is_ssl=True, - ssl_context=ssl.create_default_context() + ssl_context=ssl.create_default_context(), ) # Setup the callback methods above