diff --git a/src/ssh/HISTORY.md b/src/ssh/HISTORY.md index c681d5706c2..a6b2340a2a4 100644 --- a/src/ssh/HISTORY.md +++ b/src/ssh/HISTORY.md @@ -1,5 +1,10 @@ Release History =============== +2.0.7 +----- +* Upgrade Azure Arc SSH Proxy Version to latest (1.3.033291) +* [Bug Fix] Ensure connection will not crash if logs contain non utf-8 characters + 2.0.6 ----- * Remove msrestazure dependency diff --git a/src/ssh/azext_ssh/constants.py b/src/ssh/azext_ssh/constants.py index b75c616cf7c..7f6581aae1b 100644 --- a/src/ssh/azext_ssh/constants.py +++ b/src/ssh/azext_ssh/constants.py @@ -7,7 +7,7 @@ AGENT_MINIMUM_VERSION_MAJOR = 1 AGENT_MINIMUM_VERSION_MINOR = 31 -CLIENT_PROXY_VERSION = "1.3.026973" +CLIENT_PROXY_VERSION = "1.3.033291" CLIENT_PROXY_MCR_TARGET = "mcr.microsoft.com/azureconnectivity/proxy" CLEANUP_TOTAL_TIME_LIMIT_IN_SECONDS = 120 CLEANUP_TIME_INTERVAL_IN_SECONDS = 10 diff --git a/src/ssh/azext_ssh/ssh_utils.py b/src/ssh/azext_ssh/ssh_utils.py index ea4a24ec69d..24db32509cb 100644 --- a/src/ssh/azext_ssh/ssh_utils.py +++ b/src/ssh/azext_ssh/ssh_utils.py @@ -117,7 +117,7 @@ def _check_ssh_logs_for_common_errors(ssh_sub, op_info, delete_cert, delete_keys connection_established = False t0 = time.time() service_config_delay_error = False - next_line = ssh_sub.stderr.readline() + next_line = _read_ssh_log_lines(ssh_sub) while next_line: log_list.append(next_line) if not next_line.startswith("debug1:") and \ @@ -139,12 +139,25 @@ def _check_ssh_logs_for_common_errors(ssh_sub, op_info, delete_cert, delete_keys do_cleanup(delete_keys, delete_cert, op_info.delete_credentials, op_info.cert_file, op_info.private_key_file, op_info.public_key_file) - next_line = ssh_sub.stderr.readline() + next_line = _read_ssh_log_lines(ssh_sub) ssh_sub.wait() return service_config_delay_error +def _read_ssh_log_lines(ssh_sub): + retries = 0 + max_retries = 5 + + while retries < max_retries: + try: + return ssh_sub.stderr.readline() + except UnicodeDecodeError: + retries += 1 + + return None + + def _wait_to_delete_credentials(ssh_sub, op_info, delete_cert, delete_keys): # wait for 2 minutes. If the process isn't closed until then, delete credentials. if delete_cert or op_info.delete_credentials: diff --git a/src/ssh/azext_ssh/tests/latest/test_connectivity_utils.py b/src/ssh/azext_ssh/tests/latest/test_connectivity_utils.py index d1d52a7d8ac..8ddc4004d50 100644 --- a/src/ssh/azext_ssh/tests/latest/test_connectivity_utils.py +++ b/src/ssh/azext_ssh/tests/latest/test_connectivity_utils.py @@ -55,19 +55,19 @@ def test_get_client_os_unsupported(self, mock_plat): def test_get_proxy_filename_amd_windows(self): name = connectivity_utils._get_proxy_filename('Windows', 'amd64') - self.assertEqual(name, 'sshProxy_windows_amd64_1_3_026973.exe') + self.assertEqual(name, 'sshProxy_windows_amd64_1_3_033291.exe') def test_get_proxy_filename_arm_linux(self): name = connectivity_utils._get_proxy_filename('Linux', 'arm64') - self.assertEqual(name, 'sshProxy_linux_arm64_1_3_026973') + self.assertEqual(name, 'sshProxy_linux_arm64_1_3_033291') def test_get_proxy_filename_arm_Darwin(self): name = connectivity_utils._get_proxy_filename('Darwin', 'arm64') - self.assertEqual(name, 'sshProxy_darwin_arm64_1_3_026973') + self.assertEqual(name, 'sshProxy_darwin_arm64_1_3_033291') def test_get_proxy_filename_386_linuux(self): name = connectivity_utils._get_proxy_filename('Linux', '386') - self.assertEqual(name, 'sshProxy_linux_386_1_3_026973') + self.assertEqual(name, 'sshProxy_linux_386_1_3_033291') def test_get_proxy_filename_386_darwin(self): with self.assertRaises(azclierror.BadRequestError): @@ -136,5 +136,5 @@ def test_install_proxy_create_dir(self, mock_check, mock_download, mock_dir, moc connectivity_utils.install_client_side_proxy(None) mock_dir.assert_called_once_with("/dir/proxy", "Failed to create client proxy directory \'/dir/proxy\'.") - mock_download.assert_called_once_with("/dir/proxy", "sshProxy_linux_arm64_1_3_026973", "linux", "arm64") - mock_check.assert_called_once_with("/dir/proxy", "sshProxy_linux_arm64_1_3_026973") + mock_download.assert_called_once_with("/dir/proxy", "sshProxy_linux_arm64_1_3_033291", "linux", "arm64") + mock_check.assert_called_once_with("/dir/proxy", "sshProxy_linux_arm64_1_3_033291") diff --git a/src/ssh/azext_ssh/tests/latest/test_ssh_utils.py b/src/ssh/azext_ssh/tests/latest/test_ssh_utils.py index f00784fa8c6..7630f18c52c 100644 --- a/src/ssh/azext_ssh/tests/latest/test_ssh_utils.py +++ b/src/ssh/azext_ssh/tests/latest/test_ssh_utils.py @@ -305,3 +305,95 @@ def test_get_ssh_path_windows_ssh_preinstalled_not_found(self, mock_isfile, mock mock_isfile.return_value = False self.assertRaises(azclierror.UnclassifiedUserFault, ssh_utils.get_ssh_client_path) + + @mock.patch.object(ssh_utils, 'do_cleanup') + def test_check_ssh_logs_no_errors(self, mock_cleanup): + lines = [ + "debug1: OpenSSH_8.9, OpenSSL 1.1.1 11 Sep 2018\n", + "debug1: some debug info\n", + "debug2: more debug info\n", + "Authenticated to host\n", + "debug1: Entering interactive session.\n", + ] + ssh_sub = mock.Mock() + ssh_sub.stderr.readline.side_effect = lines + [''] + + op_info = mock.Mock() + op_info.delete_credentials = False + op_info.cert_file = "cert" + op_info.private_key_file = "priv" + op_info.public_key_file = "pub" + + result = ssh_utils._check_ssh_logs_for_common_errors(ssh_sub, op_info, delete_cert=False, delete_keys=False) + + self.assertFalse(result) + # Cleanup should be called once when "Entering interactive session." is found + mock_cleanup.assert_called_once_with(False, False, False, "cert", "priv", "pub") + ssh_sub.wait.assert_called_once() + + @mock.patch.object(ssh_utils, 'do_cleanup') + def test_check_ssh_logs_service_config_delay_error(self, mock_cleanup): + error_line = ('{"level":"fatal","msg":"sshproxy: error connecting to the address: ' + '404 Endpoint does not exist","time":"2024-01-01T00:00:00Z"}\n') + lines = [ + "debug1: OpenSSH_8.9, OpenSSL 1.1.1 11 Sep 2018\n", + error_line, + ] + ssh_sub = mock.Mock() + ssh_sub.stderr.readline.side_effect = lines + [''] + + op_info = mock.Mock() + op_info.delete_credentials = False + op_info.cert_file = "cert" + op_info.private_key_file = "priv" + op_info.public_key_file = "pub" + + result = ssh_utils._check_ssh_logs_for_common_errors(ssh_sub, op_info, delete_cert=False, delete_keys=False) + + self.assertTrue(result) + ssh_sub.wait.assert_called_once() + + def test_readline_ssh_logs_returns_line(self): + ssh_sub = mock.Mock() + ssh_sub.stderr.readline.return_value = "some log line\n" + result = ssh_utils._read_ssh_log_lines(ssh_sub) + self.assertEqual(result, "some log line\n") + + def test_readline_ssh_logs_skips_errors_and_returns_next_line(self): + ssh_sub = mock.Mock() + ssh_sub.stderr.readline.side_effect = [ + UnicodeDecodeError('utf-8', b'\x80invalid', 0, 1, 'invalid start byte'), + UnicodeDecodeError('utf-8', b'\x81invalid', 0, 1, 'invalid start byte'), + "valid line\n", + ] + result = ssh_utils._read_ssh_log_lines(ssh_sub) + self.assertEqual(result, "valid line\n") + self.assertEqual(ssh_sub.stderr.readline.call_count, 3) + + def test_readline_ssh_logs_returns_empty_string_when_stream_ends(self): + ssh_sub = mock.Mock() + ssh_sub.stderr.readline.return_value = '' + result = ssh_utils._read_ssh_log_lines(ssh_sub) + self.assertEqual(result, '') + + def test_readline_ssh_logs_returns_none(self): + ssh_sub = mock.Mock() + ssh_sub.stderr.readline.return_value = None + result = ssh_utils._read_ssh_log_lines(ssh_sub) + self.assertIsNone(result) + + def test_readline_ssh_logs_skips_errors_and_returns_next_line(self): + ssh_sub = mock.Mock() + ssh_sub.stderr.readline.side_effect = [ + UnicodeDecodeError('utf-8', b'\x80invalid', 0, 1, 'invalid start byte'), + UnicodeDecodeError('utf-8', b'\x81invalid', 0, 1, 'invalid start byte'), + UnicodeDecodeError('utf-8', b'\x81invalid', 0, 1, 'invalid start byte'), + UnicodeDecodeError('utf-8', b'\x81invalid', 0, 1, 'invalid start byte'), + UnicodeDecodeError('utf-8', b'\x81invalid', 0, 1, 'invalid start byte'), + "valid line\n", + ] + result = ssh_utils._read_ssh_log_lines(ssh_sub) + self.assertIsNone(result) + self.assertEqual(ssh_sub.stderr.readline.call_count, 5) + +