diff --git a/src/ssh/HISTORY.md b/src/ssh/HISTORY.md index 1ae52967786..c5a7c78fadd 100644 --- a/src/ssh/HISTORY.md +++ b/src/ssh/HISTORY.md @@ -1,5 +1,9 @@ Release History =============== +1.1.1 +----- +* Undo changes to rely on PATH variable to find SSH executables on Windows. Look for executables under "C:\Windows\System32\OpenSSH" + 1.1.0 ----- * SSHArc Public Preview diff --git a/src/ssh/azext_ssh/_help.py b/src/ssh/azext_ssh/_help.py index 7850f465b59..78b3999a4b8 100644 --- a/src/ssh/azext_ssh/_help.py +++ b/src/ssh/azext_ssh/_help.py @@ -48,7 +48,7 @@ text: | az ssh vm --local-user username --resource-group myResourceGroup --name myArcServer - - name: Give a SSH Client Folder to use the ssh executables in that folder, like ssh-keygen.exe and ssh.exe. If not provided, the extension attempt to use pre-installed OpenSSH client (in that case, ensure OpenSSH client is installed and the Path environment variable is set correctly). + - name: Give a SSH Client Folder to use the ssh executables in that folder, like ssh-keygen.exe and ssh.exe. If not provided, the extension attempts to use pre-installed OpenSSH client (on Windows, extension looks for pre-installed executables under C:\\Windows\\System32\\OpenSSH). text: | az ssh vm --resource-group myResourceGroup --name myVM --ssh-client-folder "C:\\Program Files\\OpenSSH" """ @@ -91,9 +91,9 @@ rsync -e 'ssh -F ./sshconfig' -avP directory/ myvm:~/directory GIT_SSH_COMMAND="ssh -F ./sshconfig" git clone myvm:~/gitrepo - - name: Give SSH Client Folder to use the ssh executables in that folder. If not provided, the extension attempt to use pre-installed OpenSSH client (in that case, ensure OpenSSH client is installed and the Path environment variable is set correctly). + - name: Give a SSH Client Folder to use the ssh executables in that folder, like ssh-keygen.exe. If not provided, the extension attempts to use pre-installed OpenSSH client (on Windows, extension looks for pre-installed executables under C:\\Windows\\System32\\OpenSSH). text: | - az ssh config --file ./sshconfig --resource-group myResourceGroup --name myMachine --ssh-client-folder "C:\\Program Files\\OpenSSH" + az ssh config --file ./myconfig --resource-group myResourceGroup --name myVM --ssh-client-folder "C:\\Program Files\\OpenSSH" - name: Give the Resource Type of the target. Useful when there is an Azure VM and an Arc Server with the same name in the same resource group. Resource type can be either "Microsoft.HybridCompute" for Arc Servers or "Microsoft.Compute" for Azure Virtual Machines. text: | @@ -107,12 +107,15 @@ - name: Create a short lived ssh certificate signed by AAD text: | az ssh cert --public-key-file ./id_rsa.pub --file ./id_rsa-aadcert.pub + - name: Give a SSH Client Folder to use the ssh executables in that folder, like ssh-keygen.exe. If not provided, the extension attempts to use pre-installed OpenSSH client (on Windows, extension looks for pre-installed executables under C:\\Windows\\System32\\OpenSSH). + text: | + az ssh cert --file ./id_rsa-aadcert.pub --ssh-client-folder "C:\\Program Files\\OpenSSH" """ helps['ssh arc'] = """ type: command short-summary: SSH into Azure Arc Servers - long-summary: Users can login using AAD issued certificates or using local user credentials. We recommend login using AAD issued certificates as azure automatically rotate SSH CA keys. To SSH as a local user in the target machine, you must provide the local user name using the --local-user argument. + long-summary: Users can login using AAD issued certificates or using local user credentials. We recommend login using AAD issued certificates. To SSH using local user credentials you must provide the local user name using the --local-user parameter. examples: - name: Give a resource group name and machine name to SSH using AAD issued certificates text: | @@ -138,7 +141,7 @@ text: | az ssh arc --local-user username --resource-group myResourceGroup --name myMachine - - name: Give a SSH Client Folder to use the ssh executables in that folder, like ssh-keygen.exe and ssh.exe. If not provided, the extension attempt to use pre-installed OpenSSH client (ensure OpenSSH client is installed and the Path environment variable is set correctly). + - name: Give a SSH Client Folder to use the ssh executables in that folder, like ssh-keygen.exe and ssh.exe. If not provided, the extension attempts to use pre-installed OpenSSH client (on Windows, extension looks for pre-installed executables under C:\\Windows\\System32\\OpenSSH). text: | az ssh arc --resource-group myResourceGroup --name myMachine --ssh-client-folder "C:\\Program Files\\OpenSSH" """ diff --git a/src/ssh/azext_ssh/_params.py b/src/ssh/azext_ssh/_params.py index 2a8e749422e..9e9bf17d3ff 100644 --- a/src/ssh/azext_ssh/_params.py +++ b/src/ssh/azext_ssh/_params.py @@ -26,8 +26,8 @@ def load_arguments(self, _): help='Folder path that contains ssh executables (ssh.exe, ssh-keygen.exe, etc). ' 'Default to ssh pre-installed if not provided.') c.argument('delete_credentials', options_list=['--force-delete-credentials', '--delete-private-key'], - help=('This is an internal argument. This argument is used by Azure Portal to provide a one click ' - 'SSH login experience in Cloud shell.'), + help=('This is an internal argument. This argument is used by Azure Portal to ' + 'provide a one click SSH login experience in Cloud shell.'), deprecate_info=c.deprecate(hide=True), action='store_true') c.argument('ssh_proxy_folder', options_list=['--ssh-proxy-folder'], help=('Path to the folder where the ssh proxy should be saved. ' @@ -81,8 +81,8 @@ def load_arguments(self, _): help='Folder path that contains ssh executables (ssh.exe, ssh-keygen.exe, etc). ' 'Default to ssh pre-installed if not provided.') c.argument('delete_credentials', options_list=['--force-delete-credentials', '--delete-private-key'], - help=('This is an internal argument. This argument is used by Azure Portal to provide a one click ' - 'SSH login experience in Cloud shell.'), + help=('This is an internal argument. This argument is used by Azure Portal to ' + 'provide a one click SSH login experience in Cloud shell.'), deprecate_info=c.deprecate(hide=True), action='store_true') c.argument('ssh_proxy_folder', options_list=['--ssh-proxy-folder'], help=('Path to the folder where the ssh proxy should be saved. ' diff --git a/src/ssh/azext_ssh/constants.py b/src/ssh/azext_ssh/constants.py index ea044a918de..eaf98715102 100644 --- a/src/ssh/azext_ssh/constants.py +++ b/src/ssh/azext_ssh/constants.py @@ -13,8 +13,7 @@ CLEANUP_AWAIT_TERMINATION_IN_SECONDS = 30 RELAY_INFO_MAXIMUM_DURATION_IN_SECONDS = 3600 WINDOWS_INVALID_FOLDERNAME_CHARS = "\\/*:<>?\"|" -RECOMMENDATION_SSH_CLIENT_NOT_FOUND = (Fore.YELLOW + "Ensure OpenSSH is installed and the PATH Environment " - "Variable is set correctly.\nAlternatively, use " +RECOMMENDATION_SSH_CLIENT_NOT_FOUND = (Fore.YELLOW + "Ensure OpenSSH is installed correctly.\nAlternatively, use " "--ssh-client-folder to provide OpenSSH folder path." + Style.RESET_ALL) RECOMMENDATION_RESOURCE_NOT_FOUND = (Fore.YELLOW + "Please ensure the active subscription is set properly " "and resource exists." + Style.RESET_ALL) diff --git a/src/ssh/azext_ssh/custom.py b/src/ssh/azext_ssh/custom.py index dfe7ef59099..58c2033f0f3 100644 --- a/src/ssh/azext_ssh/custom.py +++ b/src/ssh/azext_ssh/custom.py @@ -408,6 +408,9 @@ def _decide_resource_type(cmd, op_info): if is_arc_server and arc and arc.properties and arc.properties and arc.properties.os_name: os_type = arc.properties.os_name + if os_type: + telemetry.add_extension_event('ssh', {'Context.Default.AzureCLI.TargetOSType': os_type}) + # Note 2: This is a temporary check while AAD login is not enabled for Windows. if os_type and os_type.lower() == 'windows' and not op_info.local_user: colorama.init() @@ -415,9 +418,6 @@ def _decide_resource_type(cmd, op_info): "for Windows.", Fore.YELLOW + "Please provide --local-user." + Style.RESET_ALL) - if os_type: - telemetry.add_extension_event('ssh', {'Context.Default.AzureCLI.TargetOSType': os_type}) - target_resource_type = "Microsoft.Compute" if is_arc_server: target_resource_type = "Microsoft.HybridCompute" diff --git a/src/ssh/azext_ssh/ssh_utils.py b/src/ssh/azext_ssh/ssh_utils.py index aa85f33687c..70eb16de634 100644 --- a/src/ssh/azext_ssh/ssh_utils.py +++ b/src/ssh/azext_ssh/ssh_utils.py @@ -59,11 +59,12 @@ def start_ssh_connection(op_info, delete_keys, delete_cert): # pylint: disable=subprocess-run-check try: if set(['-v', '-vv', '-vvv']).isdisjoint(ssh_arg_list) or log_file: - connection_status = subprocess.run(command, env=env, stderr=subprocess.PIPE, text=True) + connection_status = subprocess.run(command, shell=platform.system() == 'Windows', env=env, + stderr=subprocess.PIPE, encoding='utf-8') else: # Logs are sent to stderr. In that case, we shouldn't capture stderr. - connection_status = subprocess.run(command, env=env, text=True) - except Exception as e: + connection_status = subprocess.run(command, shell=platform.system() == 'Windows', env=env) + except OSError as e: colorama.init() raise azclierror.BadRequestError(f"Failed to run ssh command with error: {str(e)}.", const.RECOMMENDATION_SSH_CLIENT_NOT_FOUND) @@ -101,7 +102,7 @@ def create_ssh_keyfile(private_key_file, ssh_client_folder=None): logger.debug("Running ssh-keygen command %s", ' '.join(command)) try: subprocess.call(command) - except Exception as e: + except OSError as e: colorama.init() raise azclierror.BadRequestError(f"Failed to create ssh key file with error: {str(e)}.", const.RECOMMENDATION_SSH_CLIENT_NOT_FOUND) @@ -113,7 +114,7 @@ def get_ssh_cert_info(cert_file, ssh_client_folder=None): logger.debug("Running ssh-keygen command %s", ' '.join(command)) try: return subprocess.check_output(command).decode().splitlines() - except Exception as e: + except OSError as e: colorama.init() raise azclierror.BadRequestError(f"Failed to get certificate info with error: {str(e)}.", const.RECOMMENDATION_SSH_CLIENT_NOT_FOUND) @@ -219,9 +220,48 @@ def _get_ssh_client_path(ssh_command="ssh", ssh_client_folder=None): logger.debug("Attempting to run %s from path %s", ssh_command, ssh_path) return ssh_path logger.warning("Could not find %s in provided --ssh-client-folder %s. " - "Attempting to use pre-installed OpenSSH.", ssh_path, ssh_client_folder) + "Attempting to get pre-installed OpenSSH bits.", ssh_command, ssh_client_folder) ssh_path = ssh_command + + if platform.system() == 'Windows': + # If OS architecture is 64bit and python architecture is 32bit, + # look for System32 under SysNative folder. + machine = platform.machine() + os_architecture = None + # python interpreter architecture + platform_architecture = platform.architecture()[0] + sys_path = None + + if machine.endswith('64'): + os_architecture = '64bit' + elif machine.endswith('86'): + os_architecture = '32bit' + elif machine == '': + raise azclierror.BadRequestError("Couldn't identify the OS architecture.") + else: + raise azclierror.BadRequestError(f"Unsuported OS architecture: {machine} is not currently supported") + + if os_architecture == "64bit": + sys_path = 'SysNative' if platform_architecture == '32bit' else 'System32' + else: + sys_path = 'System32' + + system_root = os.environ['SystemRoot'] + system32_path = os.path.join(system_root, sys_path) + ssh_path = os.path.join(system32_path, "openSSH", (ssh_command + ".exe")) + logger.debug("Platform architecture: %s", platform_architecture) + logger.debug("OS architecture: %s", os_architecture) + logger.debug("System Root: %s", system_root) + logger.debug("Attempting to run %s from path %s", ssh_command, ssh_path) + + if not os.path.isfile(ssh_path): + raise azclierror.UnclassifiedUserFault( + "Could not find " + ssh_command + ".exe on path " + ssh_path + ". ", + Fore.YELLOW + "Make sure OpenSSH is installed correctly: " + "https://docs.microsoft.com/en-us/windows-server/administration/openssh/openssh_install_firstuse . " + "Or use --ssh-client-folder to provide folder path with ssh executables. " + Style.RESET_ALL) + return ssh_path 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 4d004a0b69d..e0adb1fa14d 100644 --- a/src/ssh/azext_ssh/tests/latest/test_ssh_utils.py +++ b/src/ssh/azext_ssh/tests/latest/test_ssh_utils.py @@ -18,7 +18,8 @@ class SSHUtilsTests(unittest.TestCase): @mock.patch.object(ssh_utils, '_get_ssh_client_path') @mock.patch('subprocess.run') @mock.patch('os.environ.copy') - def test_start_ssh_connection_compute(self, mock_copy_env, mock_call, mock_path, mock_terminatecleanup, mock_startcleanup): + @mock.patch('platform.system') + def test_start_ssh_connection_compute(self, mock_system, mock_copy_env, mock_call, mock_path, mock_terminatecleanup, mock_startcleanup): op_info = ssh_info.SSHSession("rg", "vm", "ip", None, None, False, "user", None, "port", None, ['arg1', 'arg2', 'arg3'], False, "Microsof.Compute", None, None) op_info.public_key_file = "pub" @@ -26,6 +27,7 @@ def test_start_ssh_connection_compute(self, mock_copy_env, mock_call, mock_path, op_info.cert_file = "cert" op_info.ssh_client_folder = "client" + mock_system.return_value = 'Windows' mock_call.return_value = 0 mock_path.return_value = 'ssh' mock_copy_env.return_value = {'var1':'value1', 'var2':'value2', 'var3':'value3'} @@ -37,7 +39,7 @@ def test_start_ssh_connection_compute(self, mock_copy_env, mock_call, mock_path, mock_path.assert_called_once_with('ssh', 'client') mock_startcleanup.assert_called_with('cert', 'priv', 'pub', False, True, True, ['arg1', 'arg2', 'arg3']) - mock_call.assert_called_once_with(expected_command, env=expected_env, stderr=mock.ANY, text=True) + mock_call.assert_called_once_with(expected_command, shell=True, env=expected_env, stderr=mock.ANY, encoding='utf-8') mock_terminatecleanup.assert_called_once_with(True, True, False, 'cleanup process', 'cert', 'priv', 'pub', 'log', 0) @mock.patch.object(ssh_utils, '_terminate_cleanup') @@ -45,7 +47,8 @@ def test_start_ssh_connection_compute(self, mock_copy_env, mock_call, mock_path, @mock.patch.object(ssh_utils, '_get_ssh_client_path') @mock.patch('subprocess.run') @mock.patch('azext_ssh.custom.connectivity_utils.format_relay_info_string') - def test_start_ssh_connection_arc(self, mock_relay_str, mock_call, mock_path, mock_copy_env, mock_terminatecleanup): + @mock.patch('platform.system') + def test_start_ssh_connection_arc(self, mock_system, mock_relay_str, mock_call, mock_path, mock_copy_env, mock_terminatecleanup): op_info = ssh_info.SSHSession("rg", "vm", None, None, None, False, "user", None, "port", None, ['arg1'], False, "Microsoft.HybridCompute", None, None) op_info.public_key_file = "pub" @@ -55,6 +58,7 @@ def test_start_ssh_connection_arc(self, mock_relay_str, mock_call, mock_path, mo op_info.proxy_path = "proxy" op_info.relay_info = "relay" + mock_system.return_value = 'Linux' mock_call.return_value = 0 mock_relay_str.return_value = 'relay_string' mock_copy_env.return_value = {'var1':'value1', 'var2':'value2', 'var3':'value3'} @@ -66,7 +70,7 @@ def test_start_ssh_connection_arc(self, mock_relay_str, mock_call, mock_path, mo mock_relay_str.assert_called_once_with('relay') mock_path.assert_called_once_with('ssh', 'client') - mock_call.assert_called_once_with(expected_command, env=expected_env, stderr=mock.ANY, text=True) + mock_call.assert_called_once_with(expected_command, shell=False, env=expected_env, stderr=mock.ANY, encoding='utf-8') mock_terminatecleanup.assert_called_once_with(False, False, False, None, 'cert', 'priv', 'pub', None, 0) @@ -139,23 +143,94 @@ def test_write_ssh_config_arc_overwrite(self, mock_create_file, mock_abspath, mo @mock.patch('os.path.join') @mock.patch('platform.system') @mock.patch('os.path.isfile') - def test_get_ssh_client_path_not_found(self, mock_isfile, mock_system, mock_join): + def test_get_ssh_client_path_with_client_folder_non_windows(self, mock_isfile, mock_system, mock_join): mock_join.return_value = "ssh_path" - mock_system.return_value = "Mac" - mock_isfile.return_value = False - path = ssh_utils._get_ssh_client_path("ssh", "folder") - self.assertEqual(path, "ssh") - + mock_system.return_value = "Linux" + mock_isfile.return_value = True + actual_path = ssh_utils._get_ssh_client_path(ssh_client_folder='/client/folder') + self.assertEqual(actual_path, "ssh_path") + mock_join.assert_called_once_with('/client/folder', 'ssh') + mock_isfile.assert_called_once_with("ssh_path") + + @mock.patch('os.path.join') + @mock.patch('platform.system') + @mock.patch('os.path.isfile') + def test_get_ssh_client_path_with_client_folder_windows(self, mock_isfile, mock_system, mock_join): + mock_join.return_value = "ssh_keygen_path" + mock_system.return_value = "Windows" + mock_isfile.return_value = True + actual_path = ssh_utils._get_ssh_client_path(ssh_command='ssh-keygen', ssh_client_folder='/client/folder') + self.assertEqual(actual_path, "ssh_keygen_path.exe") + mock_join.assert_called_once_with('/client/folder', 'ssh-keygen') + mock_isfile.assert_called_once_with("ssh_keygen_path.exe") + @mock.patch('os.path.join') @mock.patch('platform.system') @mock.patch('os.path.isfile') - def test_get_ssh_client_path_found(self, mock_isfile, mock_system, mock_join): + def test_get_ssh_client_path_with_client_folder_no_file(self, mock_isfile, mock_system, mock_join): mock_join.return_value = "ssh_path" + mock_system.return_value = "Mac" + mock_isfile.return_value = False + actual_path = ssh_utils._get_ssh_client_path(ssh_client_folder='/client/folder') + self.assertEqual(actual_path, "ssh") + mock_join.assert_called_once_with('/client/folder', 'ssh') + mock_isfile.assert_called_once_with("ssh_path") + + @mock.patch('platform.system') + def test_get_ssh_client_preinstalled_non_windows(self, mock_system): + mock_system.return_value = "Mac" + actual_path = ssh_utils._get_ssh_client_path() + self.assertEqual('ssh', actual_path) + mock_system.assert_called_once_with() + + def test_get_ssh_client_preinstalled_windows_32bit(self): + self._test_get_ssh_client_path_preinstalled_windows('32bit', 'x86', 'System32') + + def test_get_ssh_client_preinstalled_windows_64bitOS_32bitPlatform(self): + self._test_get_ssh_client_path_preinstalled_windows('32bit', 'x64', 'SysNative') + + def test_get_ssh_client_preinstalled_windows_64bitOS_64bitPlatform(self): + self._test_get_ssh_client_path_preinstalled_windows('64bit', 'x64', 'System32') + + @mock.patch('platform.system') + @mock.patch('platform.architecture') + @mock.patch('platform.machine') + @mock.patch('os.path.join') + @mock.patch('os.environ') + @mock.patch('os.path.isfile') + def _test_get_ssh_client_path_preinstalled_windows(self, platform_arch, os_arch, expected_sysfolder, mock_isfile, mock_environ, mock_join, mock_machine, mock_arch, mock_system): mock_system.return_value = "Windows" + mock_arch.return_value = (platform_arch, "foo", "bar") + mock_machine.return_value = os_arch + mock_environ.__getitem__.return_value = "rootpath" + mock_join.side_effect = ["system32path", "sshfilepath"] mock_isfile.return_value = True - path = ssh_utils._get_ssh_client_path("ssh-keygen", "folder") - self.assertEqual(path, "ssh_path.exe") - - def test_get_ssh_client_preinstalled(self): - path = ssh_utils._get_ssh_client_path("ssh-keygen", None) - self.assertEqual(path, "ssh-keygen") \ No newline at end of file + + expected_join_calls = [ + mock.call("rootpath", expected_sysfolder), + mock.call("system32path", "openSSH", "ssh.exe") + ] + + actual_path = ssh_utils._get_ssh_client_path() + + self.assertEqual("sshfilepath", actual_path) + mock_system.assert_called_once_with() + mock_arch.assert_called_once_with() + mock_environ.__getitem__.assert_called_once_with("SystemRoot") + mock_join.assert_has_calls(expected_join_calls) + mock_isfile.assert_called_once_with("sshfilepath") + + + @mock.patch('platform.system') + @mock.patch('platform.architecture') + @mock.patch('platform.machine') + @mock.patch('os.environ') + @mock.patch('os.path.isfile') + def test_get_ssh_path_windows_ssh_preinstalled_not_found(self, mock_isfile, mock_environ, mock_machine, mock_arch, mock_sys): + mock_sys.return_value = "Windows" + mock_arch.return_value = ("32bit", "foo", "bar") + mock_machine.return_value = "x64" + mock_environ.__getitem__.return_value = "rootpath" + mock_isfile.return_value = False + + self.assertRaises(azclierror.UnclassifiedUserFault, ssh_utils._get_ssh_client_path) diff --git a/src/ssh/setup.py b/src/ssh/setup.py index acf6f7c6c18..be1787cf28e 100644 --- a/src/ssh/setup.py +++ b/src/ssh/setup.py @@ -7,7 +7,7 @@ from setuptools import setup, find_packages -VERSION = "1.1.0" +VERSION = "1.1.1" CLASSIFIERS = [ 'Development Status :: 4 - Beta',