From 7acea3834eede000ee8f90ab176b8c5af0574275 Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Thu, 7 Apr 2022 11:58:33 -0400 Subject: [PATCH 1/9] Catch stderr --- src/ssh/azext_ssh/ssh_utils.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/ssh/azext_ssh/ssh_utils.py b/src/ssh/azext_ssh/ssh_utils.py index d8be733f3a6..7d64e49292a 100644 --- a/src/ssh/azext_ssh/ssh_utils.py +++ b/src/ssh/azext_ssh/ssh_utils.py @@ -10,6 +10,7 @@ import datetime import re import colorama +import psutil from colorama import Fore from colorama import Style @@ -55,11 +56,12 @@ def start_ssh_connection(op_info, delete_keys, delete_cert): connection_duration = time.time() logger.debug("Running ssh command %s", ' '.join(command)) - connection_status = subprocess.call(command, shell=platform.system() == 'Windows', env=env) + + connection_status = subprocess.run(command, shell=platform.system() == 'Windows', env=env, stderr=subprocess.PIPE, text=True) connection_duration = (time.time() - connection_duration) / 60 ssh_connection_data = {'Context.Default.AzureCLI.SSHConnectionDurationInMinutes': connection_duration} - if _get_connection_status(log_file, connection_status): + if _get_connection_status(log_file, connection_status.returncode): ssh_connection_data['Context.Default.AzureCLI.SSHConnectionStatus'] = "Success" telemetry.add_extension_event('ssh', ssh_connection_data) @@ -311,8 +313,15 @@ def _terminate_cleanup(delete_keys, delete_cert, delete_credentials, cleanup_pro while cleanup_process.is_alive() and (time.time() - t0) < const.CLEANUP_AWAIT_TERMINATION_IN_SECONDS: time.sleep(1) + if connection_status.returncode != 0: + # Check if stderr is a proxy error + regex = "{\"level\":\"fatal\",\"msg\":\"sshproxy: error copying information from the connection: .*\",\"time\":\".*\"}.*" + if re.search(regex, connection_status.stderr): + logger.error("Proxy Error. Check if SSHD is running in the target machine and the incoming connection is enabled in the hybrid agent.") + print(connection_status.stderr) + if log_file and os.path.isfile(log_file): - _print_error_messages_from_ssh_log(log_file, connection_status, delete_cert) + _print_error_messages_from_ssh_log(log_file, connection_status.returncode, delete_cert) # Make sure all files have been properly removed. do_cleanup(delete_keys or delete_credentials, delete_cert or delete_credentials, From 2a935e7f9c8ca22e4aa6822e76a728fb2075bcff Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Fri, 8 Apr 2022 13:55:34 -0400 Subject: [PATCH 2/9] Improve help, remove explicit exception telemetry, a few changes --- src/ssh/azext_ssh/_help.py | 62 ++++++++++++++++------ src/ssh/azext_ssh/connectivity_utils.py | 14 ----- src/ssh/azext_ssh/constants.py | 4 -- src/ssh/azext_ssh/custom.py | 3 ++ src/ssh/azext_ssh/ssh_utils.py | 68 ++++++++++++++----------- 5 files changed, 87 insertions(+), 64 deletions(-) diff --git a/src/ssh/azext_ssh/_help.py b/src/ssh/azext_ssh/_help.py index 2bda42be1b9..5f80f9e2dc9 100644 --- a/src/ssh/azext_ssh/_help.py +++ b/src/ssh/azext_ssh/_help.py @@ -13,13 +13,13 @@ helps['ssh vm'] = """ type: command short-summary: SSH into Azure VMs or Arc Servers. - long-summary: Users can login using AAD issued certificates or using local user credentials. We recommend login using AAD issued certificates. 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 and VM to SSH using AAD issued certificates + - name: Give a resource group name and resource name to SSH using AAD issued certificates text: | - az ssh vm --resource-group myResourceGroup --vm-name myVM + az ssh vm --resource-group myResourceGroup --name myVM - - name: Give the public IP (or hostname) of a VM to SSH to SSH using AAD issued certificates + - name: Give the public IP (or hostname) of a VM to SSH using AAD issued certificates text: | az ssh vm --ip 1.2.3.4 az ssh vm --hostname example.com @@ -32,36 +32,66 @@ text: | az ssh vm --ip 1.2.3.4 -- -A -o ForwardX11=yes - - name: Give the Resource Type of a VM to SSH using AAD issued certificates. Using the resource type is useful when there is an Azure VM and a Arc Server with the same name in the same resource group. + - 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: | - az ssh vm --resource-type Microsoft.Compute --resource-group myResourceGroup --vm-name myVM + # Azure Virtual Machine + az ssh vm --resource-type Microsoft.Compute --resource-group myResourceGroup --name myVM + # Arc Enbled Server + az ssh vm --resource-type Microsoft.HybridCompute --resource-group myResourceGroup --name myMachine - - name: Give a local user name to SSH using local user credentials on the target machine using certificate based authentication. + - name: Give a local user name to SSH with local user credentials using certificate based authentication. text: | az ssh vm --local-user username --ip 1.2.3.4 --certificate-file cert.pub --private-key key - - name: Give a local user name to SSH using local user credentials on the target machine using key based authentication. + - name: Give a local user name to SSH with local user credentials using key based authentication. text: | - az ssh vm --local-user username --resource-group myResourceGroup --vm-name myVM --private-key-file key + az ssh vm --local-user username --resource-group myResourceGroup --name myVM --private-key-file key - - name: Give a local user name to SSH using local user credentials on the target machine using password based authentication. + - name: Give a local user name to SSH with local user credentials using password based authentication. text: | - az ssh vm --local-user username --resource-group myResourceGroup --vm-name myArcServer + az ssh vm --local-user username --resource-group myResourceGroup --name myArcServer + + - name: Give a SSH Client Folder to use the ssh executables (ssh-keyge.exe, ssh.exe, etc) in that folder. If not provided, the extension will look for pre-installed ssh client (for Windows, it will look for ssh executables under C:\\Windows\\System32\\OpenSSH). + text: | + az ssh vm --resource-group myResourceGroup --name myVM --ssh-client-folder "C:\\Program Files\\OpenSSH" + + - name: When connecting to an Arc Server, optionally give a SSH Proxy Folder to indicate the folder where the SSH proxy will be stored. If not provided, the proxy will be saved in .clientsshproxy folder in user\'s home directory. + - text: | + az ssh vm --resource-group myResourceGroup --name myMachine --ssh-proxy-folder "/home/user/myproxyfolder" """ helps['ssh config'] = """ type: command - short-summary: Create an SSH config for resources (Azure VMs, etc) which can then be used by clients that support OpenSSH configs and certificates - long-summary: Other software (git/rsync/etc) that support setting an SSH command can be set to use the config file by setting the command to 'ssh -F /path/to/config' e.g. rsync -e 'ssh -F /path/to/config' + short-summary: Create an SSH config for resources (Azure VMs, Arc Servers, etc) which can then be used by clients that support OpenSSH configs and certificates + long-summary: Other software (git/rsync/etc) that support setting an SSH command can be set to use the config file by setting the command to 'ssh -F /path/to/config' e.g. rsync -e 'ssh -F /path/to/config'. Users can create ssh config files that use AAD issued certificates or local user credentials. examples: - - name: Give a resource group and VM for which to create a config, and save in a local file + - name: Give a resource group and resource name for which to create a config using AAD issued certificates, save in a local file, and then ssh into that resource + text: | + az ssh config --resource-group myResourceGroup --name myVm --file ./sshconfig + ssh -F ./sshconfig myResourceGroup-myVM + + - name: Give a resource group, resource name, and local user to create a config using local user credentials, save in local file, and then ssh into that resource text: | - az ssh config --resource-group myResourceGroup --vm-name myVm --file ./sshconfig + # password based authentication + az ssh config --resource-group myResourceGroup --name myVM --local-user username1 --file ./sshconfig + ssh -F ./sshconfig MyResourceGroup-myVM-username1 + + # key based authentication + az ssh config --resource-group myResourceGroup --name myVM --local-user username2 --private-key-file key --file ./sshconfig + ssh -F ./sshconfig MyResourceGroup-myVM-username2 + + # certificate based authentication + az ssh config --resource-group myResourceGroup --name myVM --local-user username3 --certificate-file cert.pub --private-key-file key --file ./sshconfig + ssh -F ./sshconfig MyResourceGroup-myVM-username3 - name: Give the public IP (or hostname) of a VM for which to create a config and then ssh text: | az ssh config --ip 1.2.3.4 --file ./sshconfig ssh -F ./sshconfig 1.2.3.4 + + - name: Give Keys Destination Folder to indicate where new generated keys and certificates will be saved. If not provided, keys will be saved to a new folder "az_ssh_config" next to the config file. + text: | + az ssh config --ip 1.2.3.4 --file ./sshconfig --keys-destination-folder /home/user/mykeys - name: Create a generic config for use with any host text: | @@ -90,7 +120,7 @@ helps['ssh arc'] = """ type: command short-summary: SSH into Azure Arc Servers - long-summary: Users can now 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 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. examples: - name: Give a resource group and Arc Server Name to SSH using AAD issued certificates text: | diff --git a/src/ssh/azext_ssh/connectivity_utils.py b/src/ssh/azext_ssh/connectivity_utils.py index da34fa39674..a7f0cf5bc37 100644 --- a/src/ssh/azext_ssh/connectivity_utils.py +++ b/src/ssh/azext_ssh/connectivity_utils.py @@ -53,14 +53,8 @@ def get_relay_information(cmd, resource_group, vm_name, certificate_validity_in_ time_elapsed = time.time() - t0 telemetry.add_extension_event('ssh', {'Context.Default.AzureCLI.SSHListCredentialsTime': time_elapsed}) except Exception as e: - telemetry.set_exception(exception='Call to listCredentials failed', - fault_type=consts.LIST_CREDENTIALS_FAILED_FAULT_TYPE, - summary=f'listCredentials failed with error: {str(e)}.') raise azclierror.ClientRequestError(f"Request for Azure Relay Information Failed:\n{str(e)}") except Exception as e: - telemetry.set_exception(exception='Call to listCredentials failed', - fault_type=consts.LIST_CREDENTIALS_FAILED_FAULT_TYPE, - summary=f'listCredentials failed with error: {str(e)}.') raise azclierror.ClientRequestError(f"Request for Azure Relay Information Failed:\n{str(e)}") return result @@ -91,8 +85,6 @@ def get_client_side_proxy(arc_proxy_folder): response_content = response.read() response.close() except Exception as e: - telemetry.set_exception(exception=e, fault_type=consts.PROXY_DOWNLOAD_FAILED_FAULT_TYPE, - summary=f'Failed to download proxy from {request_uri}') raise azclierror.ClientRequestError(f"Failed to download client proxy executable from {request_uri}. " "Error: " + str(e)) from e time_elapsed = time.time() - t0 @@ -136,9 +128,6 @@ def _get_proxy_filename_and_url(arc_proxy_folder): elif machine == '': raise azclierror.BadRequestError("Couldn't identify the platform architecture.") else: - telemetry.set_exception(exception='Unsuported architecture for installing proxy', - fault_type=consts.PROXY_UNSUPPORTED_ARCH_FAULT_TYPE, - summary=f'{machine} is not supported for installing client proxy') raise azclierror.BadRequestError(f"Unsuported architecture: {machine} is not currently supported") # define the request url and install location based on the os and architecture @@ -153,9 +142,6 @@ def _get_proxy_filename_and_url(arc_proxy_folder): install_location = install_location + ".exe" older_location = older_location + ".exe" elif operating_system not in ('Linux', 'Darwin'): - telemetry.set_exception(exception='Unsuported OS for installing ssh client proxy', - fault_type=consts.PROXY_UNSUPPORTED_OS_FAULT_TYPE, - summary=f'{operating_system} is not supported for installing client proxy') raise azclierror.BadRequestError(f"Unsuported OS: {operating_system} platform is not currently supported") if not arc_proxy_folder: diff --git a/src/ssh/azext_ssh/constants.py b/src/ssh/azext_ssh/constants.py index a1e03590450..17c7e94824e 100644 --- a/src/ssh/azext_ssh/constants.py +++ b/src/ssh/azext_ssh/constants.py @@ -10,8 +10,4 @@ CLEANUP_TIME_INTERVAL_IN_SECONDS = 10 CLEANUP_AWAIT_TERMINATION_IN_SECONDS = 30 RELAY_INFO_MAXIMUM_DURATION_IN_SECONDS = 3600 -PROXY_UNSUPPORTED_ARCH_FAULT_TYPE = 'client-proxy-unsupported-architecture-error' -PROXY_UNSUPPORTED_OS_FAULT_TYPE = 'client-proxy-unsupported-os-error' -PROXY_DOWNLOAD_FAILED_FAULT_TYPE = 'client-proxy-download-failed-error' -LIST_CREDENTIALS_FAILED_FAULT_TYPE = 'get-relay-information-failed-error' WINDOWS_INVALID_FOLDERNAME_CHARS = "\\/*:<>?\"|" diff --git a/src/ssh/azext_ssh/custom.py b/src/ssh/azext_ssh/custom.py index 98a2160f656..30b3258ec94 100644 --- a/src/ssh/azext_ssh/custom.py +++ b/src/ssh/azext_ssh/custom.py @@ -412,6 +412,9 @@ def _decide_resource_type(cmd, op_info): raise azclierror.RequiredArgumentMissingError("SSH Login to AAD user is not currently supported for Windows. " "Please provide --local-user.") + if os_type: + telemetry.add_extension_event('ssh', {'Context.Default.AzureCLI.TargetOSType': os_type}) + if is_arc_server: return "Microsoft.HybridCompute" return "Microsoft.Compute" diff --git a/src/ssh/azext_ssh/ssh_utils.py b/src/ssh/azext_ssh/ssh_utils.py index 7d64e49292a..87999e29227 100644 --- a/src/ssh/azext_ssh/ssh_utils.py +++ b/src/ssh/azext_ssh/ssh_utils.py @@ -10,7 +10,6 @@ import datetime import re import colorama -import psutil from colorama import Fore from colorama import Style @@ -57,11 +56,12 @@ def start_ssh_connection(op_info, delete_keys, delete_cert): connection_duration = time.time() logger.debug("Running ssh command %s", ' '.join(command)) - connection_status = subprocess.run(command, shell=platform.system() == 'Windows', env=env, stderr=subprocess.PIPE, text=True) + # pylint: disable=subprocess-run-check + connection_status = subprocess.run(command, env=env, stderr=subprocess.PIPE, text=True) connection_duration = (time.time() - connection_duration) / 60 ssh_connection_data = {'Context.Default.AzureCLI.SSHConnectionDurationInMinutes': connection_duration} - if _get_connection_status(log_file, connection_status.returncode): + if _get_connection_status(log_file, connection_status): ssh_connection_data['Context.Default.AzureCLI.SSHConnectionStatus'] = "Success" telemetry.add_extension_event('ssh', ssh_connection_data) @@ -145,6 +145,9 @@ def get_ssh_cert_principals(cert_file, ssh_client_folder=None): def _print_error_messages_from_ssh_log(log_file, connection_status, delete_cert): + if connection_status: + connection_status = connection_status.returncode + with open(log_file, 'r', encoding='utf-8') as ssh_log: log_text = ssh_log.read() log_lines = log_text.splitlines() @@ -304,34 +307,37 @@ def _start_cleanup(cert_file, private_key_file, public_key_file, delete_credenti def _terminate_cleanup(delete_keys, delete_cert, delete_credentials, cleanup_process, cert_file, private_key_file, public_key_file, log_file, connection_status): - - if delete_keys or delete_cert or delete_credentials: - if cleanup_process and cleanup_process.is_alive(): - cleanup_process.terminate() - # wait for process to terminate - t0 = time.time() - while cleanup_process.is_alive() and (time.time() - t0) < const.CLEANUP_AWAIT_TERMINATION_IN_SECONDS: - time.sleep(1) - - if connection_status.returncode != 0: - # Check if stderr is a proxy error - regex = "{\"level\":\"fatal\",\"msg\":\"sshproxy: error copying information from the connection: .*\",\"time\":\".*\"}.*" - if re.search(regex, connection_status.stderr): - logger.error("Proxy Error. Check if SSHD is running in the target machine and the incoming connection is enabled in the hybrid agent.") + try: + if connection_status: + if connection_status.returncode != 0: + # Check if stderr is a proxy error + regex = ("{\"level\":\"fatal\",\"msg\":\"sshproxy: error copying information from the connection: " + ".*\",\"time\":\".*\"}.*") + if re.search(regex, connection_status.stderr): + logger.error("SSH Proxy Error. Check if incoming connections are enabled on the hybrid agent " + "and SSHD is running in the target machine.") print(connection_status.stderr) - - if log_file and os.path.isfile(log_file): - _print_error_messages_from_ssh_log(log_file, connection_status.returncode, delete_cert) - - # Make sure all files have been properly removed. - do_cleanup(delete_keys or delete_credentials, delete_cert or delete_credentials, - cert_file, private_key_file, public_key_file) - if log_file: - file_utils.delete_file(log_file, f"Couldn't delete temporary log file {log_file}. ", True) - if delete_keys: - # This is only true if keys were generated, so they must be in a temp folder. - temp_dir = os.path.dirname(cert_file) - file_utils.delete_folder(temp_dir, f"Couldn't delete temporary folder {temp_dir}", True) + finally: + if delete_keys or delete_cert or delete_credentials: + if cleanup_process and cleanup_process.is_alive(): + cleanup_process.terminate() + # wait for process to terminate + t0 = time.time() + while cleanup_process.is_alive() and (time.time() - t0) < const.CLEANUP_AWAIT_TERMINATION_IN_SECONDS: + time.sleep(1) + + if log_file and os.path.isfile(log_file): + _print_error_messages_from_ssh_log(log_file, connection_status, delete_cert) + + # Make sure all files have been properly removed. + do_cleanup(delete_keys or delete_credentials, delete_cert or delete_credentials, + cert_file, private_key_file, public_key_file) + if log_file: + file_utils.delete_file(log_file, f"Couldn't delete temporary log file {log_file}. ", True) + if delete_keys: + # This is only true if keys were generated, so they must be in a temp folder. + temp_dir = os.path.dirname(cert_file) + file_utils.delete_folder(temp_dir, f"Couldn't delete temporary folder {temp_dir}", True) def _issue_config_cleanup_warning(delete_cert, delete_keys, is_arc, cert_file, relay_info_path, ssh_client_folder): @@ -372,6 +378,8 @@ def _issue_config_cleanup_warning(delete_cert, delete_keys, is_arc, cert_file, r def _get_connection_status(log_file, connection_status): + if connection_status: + connection_status = connection_status.returncode # pylint: disable=bare-except if log_file and os.path.isfile(log_file): try: From 0756512270a48a1cd13ebeb84a5a58a17660b5bf Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Fri, 8 Apr 2022 16:00:08 -0400 Subject: [PATCH 3/9] Address review comments --- src/ssh/azext_ssh/_help.py | 53 +++++++++-------- src/ssh/azext_ssh/ssh_utils.py | 102 +++++++++++---------------------- 2 files changed, 64 insertions(+), 91 deletions(-) diff --git a/src/ssh/azext_ssh/_help.py b/src/ssh/azext_ssh/_help.py index 5f80f9e2dc9..0dce0f943f3 100644 --- a/src/ssh/azext_ssh/_help.py +++ b/src/ssh/azext_ssh/_help.py @@ -15,7 +15,7 @@ short-summary: SSH into Azure VMs or Arc Servers. 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 resource name to SSH using AAD issued certificates + - name: Give a resource group name and machine name to SSH using AAD issued certificates text: | az ssh vm --resource-group myResourceGroup --name myVM @@ -34,10 +34,7 @@ - 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: | - # Azure Virtual Machine - az ssh vm --resource-type Microsoft.Compute --resource-group myResourceGroup --name myVM - # Arc Enbled Server - az ssh vm --resource-type Microsoft.HybridCompute --resource-group myResourceGroup --name myMachine + az ssh vm --resource-type [Microsoft.Compute|Microsoft.HybridCompute] --resource-group myResourceGroup --name myVM - name: Give a local user name to SSH with local user credentials using certificate based authentication. text: | @@ -50,14 +47,10 @@ - name: Give a local user name to SSH with local user credentials using password based authentication. text: | az ssh vm --local-user username --resource-group myResourceGroup --name myArcServer - - - name: Give a SSH Client Folder to use the ssh executables (ssh-keyge.exe, ssh.exe, etc) in that folder. If not provided, the extension will look for pre-installed ssh client (for Windows, it will look for ssh executables under C:\\Windows\\System32\\OpenSSH). + + - 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). text: | az ssh vm --resource-group myResourceGroup --name myVM --ssh-client-folder "C:\\Program Files\\OpenSSH" - - - name: When connecting to an Arc Server, optionally give a SSH Proxy Folder to indicate the folder where the SSH proxy will be stored. If not provided, the proxy will be saved in .clientsshproxy folder in user\'s home directory. - - text: | - az ssh vm --resource-group myResourceGroup --name myMachine --ssh-proxy-folder "/home/user/myproxyfolder" """ helps['ssh config'] = """ @@ -75,7 +68,7 @@ # password based authentication az ssh config --resource-group myResourceGroup --name myVM --local-user username1 --file ./sshconfig ssh -F ./sshconfig MyResourceGroup-myVM-username1 - + # key based authentication az ssh config --resource-group myResourceGroup --name myVM --local-user username2 --private-key-file key --file ./sshconfig ssh -F ./sshconfig MyResourceGroup-myVM-username2 @@ -88,10 +81,10 @@ text: | az ssh config --ip 1.2.3.4 --file ./sshconfig ssh -F ./sshconfig 1.2.3.4 - - - name: Give Keys Destination Folder to indicate where new generated keys and certificates will be saved. If not provided, keys will be saved to a new folder "az_ssh_config" next to the config file. + + - name: Give Keys Destination Folder to store the generated keys and certificates. If not provided, SSH keys are stored in new folder "az_ssh_config" next to the config file. text: | - az ssh config --ip 1.2.3.4 --file ./sshconfig --keys-destination-folder /home/user/mykeys + az ssh config --ip 1.2.3.4 --file ./sshconfig --keys-destination-folder /home/user/mykeys - name: Create a generic config for use with any host text: | @@ -106,6 +99,10 @@ az ssh config --ip \\* --file ./sshconfig rsync -e 'ssh -F ./sshconfig' -avP directory/ myvm:~/directory GIT_SSH_COMMAND="ssh -F ./sshconfig" git clone myvm:~/gitrepo + + - name: Give a SSH Client Folder to use the ssh executables in that folder. 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). + text: | + az ssh vm --resource-group myResourceGroup --name myMachine --ssh-client-folder "C:\\Program Files\\OpenSSH" """ helps['ssh cert'] = """ @@ -122,23 +119,31 @@ 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. examples: - - name: Give a resource group and Arc Server Name to SSH using AAD issued certificates + - name: Give a resource group name and machine name to SSH using AAD issued certificates text: | - az ssh arc --resource-group myResourceGroup --vm-name myArcServer + az ssh vm --resource-group myResourceGroup --name myMachine - name: Using a custom private key file text: | - az ssh arc --resource-group myResourceGroup --vm-name myArcServer --private-key-file key --public-key-file key.pub + az ssh vm --resource-group myResourceGroup --name myMachine --private-key-file key --public-key-file key.pub - - name: Give a local user name to SSH to a local user using certificate-based authentication + - name: Using additional ssh arguments text: | - az ssh arc --resource-group myResourceGroup --vm-name myArcServer --certificate-file cert.pub --private-key key --local-user name + az ssh vm --resource-group myResourceGroup --name myMachine -- -A -o ForwardX11=yes - - name: Give a local user name to SSH to a local user using key-based authentication + - name: Give a local user name to SSH with local user credentials using certificate based authentication. + text: | + az ssh vm --local-user username --resource-group myResourceGroup --name myMachine --certificate-file cert.pub --private-key key + + - name: Give a local user name to SSH with local user credentials using key based authentication. + text: | + az ssh vm --local-user username --resource-group myResourceGroup --name myMachine --private-key-file key + + - name: Give a local user name to SSH with local user credentials using password based authentication. text: | - az ssh arc --resource-group myRG --vm-name myVM --local-user name --private-key-file key + az ssh vm --local-user username --resource-group myResourceGroup --name myMachine - - name: Give a local user name to SSH to a local user using password-based authentication + - 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). text: | - az ssh arc --resource-group myResourceGroup --vm-name myArcServer --local-user username + az ssh vm --resource-group myResourceGroup --name myMachine --ssh-client-folder "C:\\Program Files\\OpenSSH" """ diff --git a/src/ssh/azext_ssh/ssh_utils.py b/src/ssh/azext_ssh/ssh_utils.py index 87999e29227..6d540e629f3 100644 --- a/src/ssh/azext_ssh/ssh_utils.py +++ b/src/ssh/azext_ssh/ssh_utils.py @@ -57,11 +57,22 @@ def start_ssh_connection(op_info, delete_keys, delete_cert): logger.debug("Running ssh command %s", ' '.join(command)) # pylint: disable=subprocess-run-check - connection_status = subprocess.run(command, env=env, stderr=subprocess.PIPE, text=True) + try: + if set(['-v', '-vv', '-vvv']).isdisjoint(ssh_arg_list): + connection_status = subprocess.run(command, env=env, stderr=subprocess.PIPE, text=True) + 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: + raise azclierror.BadRequestError(f"Failed to run ssh command with error: {str(e)}.", + "Ensure OpenSSH is installed, and that the " + "Path Environment Variable is set correctly. " + "Alternatively, use --ssh-client-folder to provide folder " + "path with OpenSSH executables.") connection_duration = (time.time() - connection_duration) / 60 ssh_connection_data = {'Context.Default.AzureCLI.SSHConnectionDurationInMinutes': connection_duration} - if _get_connection_status(log_file, connection_status): + if connection_status and connection_status.returncode == 0: ssh_connection_data['Context.Default.AzureCLI.SSHConnectionStatus'] = "Success" telemetry.add_extension_event('ssh', ssh_connection_data) @@ -90,14 +101,28 @@ def create_ssh_keyfile(private_key_file, ssh_client_folder=None): sshkeygen_path = _get_ssh_client_path("ssh-keygen", ssh_client_folder) command = [sshkeygen_path, "-f", private_key_file, "-t", "rsa", "-q", "-N", ""] logger.debug("Running ssh-keygen command %s", ' '.join(command)) - subprocess.call(command, shell=platform.system() == 'Windows') + try: + subprocess.call(command) + except Exception as e: + raise azclierror.BadRequestError(f"Failed to create ssh key file with error: {str(e)}.", + "Ensure OpenSSH is installed, and that the " + "Path Environment Variable is set correctly. " + "Alternatively, use --ssh-client-folder to provide folder " + "path with OpenSSH executables.") def get_ssh_cert_info(cert_file, ssh_client_folder=None): sshkeygen_path = _get_ssh_client_path("ssh-keygen", ssh_client_folder) command = [sshkeygen_path, "-L", "-f", cert_file] logger.debug("Running ssh-keygen command %s", ' '.join(command)) - return subprocess.check_output(command, shell=platform.system() == 'Windows').decode().splitlines() + try: + return subprocess.check_output(command).decode().splitlines() + except Exception as e: + raise azclierror.BadRequestError(f"Failed to get certificate info with error: {str(e)}.", + "Ensure OpenSSH is installed, and that the " + "Path Environment Variable is set correctly. " + "Alternatively, use --ssh-client-folder to provide folder " + "path with OpenSSH executables.") def _get_ssh_cert_validity(cert_file, ssh_client_folder=None): @@ -145,15 +170,12 @@ def get_ssh_cert_principals(cert_file, ssh_client_folder=None): def _print_error_messages_from_ssh_log(log_file, connection_status, delete_cert): - if connection_status: - connection_status = connection_status.returncode - with open(log_file, 'r', encoding='utf-8') as ssh_log: log_text = ssh_log.read() log_lines = log_text.splitlines() if ("debug1: Authentication succeeded" not in log_text and not re.search("^Authenticated to .*\n", log_text, re.MULTILINE)) \ - or connection_status: + or (connection_status and connection_status.returncode): for line in log_lines: if "debug1:" not in line: print(line) @@ -203,48 +225,9 @@ 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 get pre-installed OpenSSH bits.", ssh_path, ssh_client_folder) + "Attempting to use pre-installed OpenSSH.", ssh_path, 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 + ". " - "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. ") - return ssh_path @@ -308,14 +291,14 @@ def _start_cleanup(cert_file, private_key_file, public_key_file, delete_credenti def _terminate_cleanup(delete_keys, delete_cert, delete_credentials, cleanup_process, cert_file, private_key_file, public_key_file, log_file, connection_status): try: - if connection_status: + if connection_status and connection_status.stderr: if connection_status.returncode != 0: # Check if stderr is a proxy error regex = ("{\"level\":\"fatal\",\"msg\":\"sshproxy: error copying information from the connection: " ".*\",\"time\":\".*\"}.*") if re.search(regex, connection_status.stderr): - logger.error("SSH Proxy Error. Check if incoming connections are enabled on the hybrid agent " - "and SSHD is running in the target machine.") + logger.error("Please make sure SSH port is allowed using \"azcmagent config list\" in the target " + "Arc Server. Ensure SSHD is running on the target machine.") print(connection_status.stderr) finally: if delete_keys or delete_cert or delete_credentials: @@ -331,7 +314,7 @@ def _terminate_cleanup(delete_keys, delete_cert, delete_credentials, cleanup_pro # Make sure all files have been properly removed. do_cleanup(delete_keys or delete_credentials, delete_cert or delete_credentials, - cert_file, private_key_file, public_key_file) + cert_file, private_key_file, public_key_file) if log_file: file_utils.delete_file(log_file, f"Couldn't delete temporary log file {log_file}. ", True) if delete_keys: @@ -375,18 +358,3 @@ def _issue_config_cleanup_warning(delete_cert, delete_keys, is_arc, cert_file, r logger.warning("%s sensitive information%s. Please delete it once you no longer " "need this config file.", path_to_delete, items_to_delete) - - -def _get_connection_status(log_file, connection_status): - if connection_status: - connection_status = connection_status.returncode - # pylint: disable=bare-except - if log_file and os.path.isfile(log_file): - try: - with open(log_file, 'r', encoding='utf-8') as ssh_client_log: - match = "debug1: Authentication succeeded" in ssh_client_log.read() - ssh_client_log.close() - except: - return False - return match and connection_status == 0 - return connection_status == 0 From 5125936bc424d7b0ad5cc6970a961ad902f1f4c4 Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Fri, 8 Apr 2022 16:17:11 -0400 Subject: [PATCH 4/9] Change ssh not found error message --- src/ssh/azext_ssh/ssh_utils.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/ssh/azext_ssh/ssh_utils.py b/src/ssh/azext_ssh/ssh_utils.py index 6d540e629f3..a2f091ecea1 100644 --- a/src/ssh/azext_ssh/ssh_utils.py +++ b/src/ssh/azext_ssh/ssh_utils.py @@ -64,11 +64,10 @@ def start_ssh_connection(op_info, delete_keys, delete_cert): # 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: + recommendation = ("Ensure OpenSSH is installed, and that the PATH Environment Variable is set correctly.\n" + "Alternatively, use --ssh-client-folder to provide folder path with OpenSSH executables.") raise azclierror.BadRequestError(f"Failed to run ssh command with error: {str(e)}.", - "Ensure OpenSSH is installed, and that the " - "Path Environment Variable is set correctly. " - "Alternatively, use --ssh-client-folder to provide folder " - "path with OpenSSH executables.") + recommendation) connection_duration = (time.time() - connection_duration) / 60 ssh_connection_data = {'Context.Default.AzureCLI.SSHConnectionDurationInMinutes': connection_duration} @@ -104,11 +103,10 @@ def create_ssh_keyfile(private_key_file, ssh_client_folder=None): try: subprocess.call(command) except Exception as e: + recommendation = ("Ensure OpenSSH is installed, and that the PATH Environment Variable is set correctly.\n" + "Alternatively, use --ssh-client-folder to provide folder path with OpenSSH executables.") raise azclierror.BadRequestError(f"Failed to create ssh key file with error: {str(e)}.", - "Ensure OpenSSH is installed, and that the " - "Path Environment Variable is set correctly. " - "Alternatively, use --ssh-client-folder to provide folder " - "path with OpenSSH executables.") + recommendation) def get_ssh_cert_info(cert_file, ssh_client_folder=None): @@ -118,11 +116,10 @@ def get_ssh_cert_info(cert_file, ssh_client_folder=None): try: return subprocess.check_output(command).decode().splitlines() except Exception as e: + recommendation = ("Ensure OpenSSH is installed, and that the PATH Environment Variable is set correctly.\n" + "Alternatively, use --ssh-client-folder to provide folder path with OpenSSH executables.") raise azclierror.BadRequestError(f"Failed to get certificate info with error: {str(e)}.", - "Ensure OpenSSH is installed, and that the " - "Path Environment Variable is set correctly. " - "Alternatively, use --ssh-client-folder to provide folder " - "path with OpenSSH executables.") + recommendation) def _get_ssh_cert_validity(cert_file, ssh_client_folder=None): From f63ec2c77c49d6310e35fec496d5b1a471754004 Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Fri, 8 Apr 2022 16:38:54 -0400 Subject: [PATCH 5/9] Fix help and add localuser name to ip entry --- src/ssh/azext_ssh/_help.py | 22 ++++++++-------------- src/ssh/azext_ssh/ssh_info.py | 10 +++++++--- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/ssh/azext_ssh/_help.py b/src/ssh/azext_ssh/_help.py index 0dce0f943f3..7fb1ccd977f 100644 --- a/src/ssh/azext_ssh/_help.py +++ b/src/ssh/azext_ssh/_help.py @@ -63,24 +63,18 @@ az ssh config --resource-group myResourceGroup --name myVm --file ./sshconfig ssh -F ./sshconfig myResourceGroup-myVM - - name: Give a resource group, resource name, and local user to create a config using local user credentials, save in local file, and then ssh into that resource + - name: Give the public IP (or hostname) of an Azure VM for which to create a config and then ssh into that VM + text: | + az ssh config --ip 1.2.3.4 --file ./sshconfig + ssh -F ./sshconfig 1.2.3.4 + + - name: Give a local user to create a config using local user credentials, save in local file, and then ssh into that resource text: | - # password based authentication az ssh config --resource-group myResourceGroup --name myVM --local-user username1 --file ./sshconfig ssh -F ./sshconfig MyResourceGroup-myVM-username1 - # key based authentication - az ssh config --resource-group myResourceGroup --name myVM --local-user username2 --private-key-file key --file ./sshconfig - ssh -F ./sshconfig MyResourceGroup-myVM-username2 - - # certificate based authentication - az ssh config --resource-group myResourceGroup --name myVM --local-user username3 --certificate-file cert.pub --private-key-file key --file ./sshconfig - ssh -F ./sshconfig MyResourceGroup-myVM-username3 - - - name: Give the public IP (or hostname) of a VM for which to create a config and then ssh - text: | - az ssh config --ip 1.2.3.4 --file ./sshconfig - ssh -F ./sshconfig 1.2.3.4 + az ssh config -ip 1.2.3.4 --local-user username2 --private-key-file key --file ./sshconfig + ssh -F ./sshconfig 1.2.3.4-username2 - name: Give Keys Destination Folder to store the generated keys and certificates. If not provided, SSH keys are stored in new folder "az_ssh_config" next to the config file. text: | diff --git a/src/ssh/azext_ssh/ssh_info.py b/src/ssh/azext_ssh/ssh_info.py index 6bccf8dd7fe..713bf83dc03 100644 --- a/src/ssh/azext_ssh/ssh_info.py +++ b/src/ssh/azext_ssh/ssh_info.py @@ -116,7 +116,7 @@ def get_config_text(self, is_aad): # default to all hosts for config if not self.ip: self.ip = "*" - lines = lines + self._get_ip_entry() + lines = lines + self._get_ip_entry(is_aad) return lines def _get_arc_entry(self, is_aad): @@ -154,9 +154,13 @@ def _get_rg_and_vm_entry(self, is_aad): lines.append("\tPort " + self.port) return lines - def _get_ip_entry(self): + def _get_ip_entry(self, is_aad): lines = [] - lines.append("Host " + self.ip) + if is_aad: + lines.append("Host " + self.ip) + else: + lines.append("Host " + self.ip + "-" + self.local_user) + lines.append("\tHostname " + self.ip) lines.append("\tUser " + self.local_user) if self.cert_file: lines.append("\tCertificateFile \"" + self.cert_file + "\"") From abd4a2828250bd749795150af3f2def542a605a7 Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Fri, 8 Apr 2022 16:47:36 -0400 Subject: [PATCH 6/9] help improvements --- src/ssh/azext_ssh/_help.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/ssh/azext_ssh/_help.py b/src/ssh/azext_ssh/_help.py index 7fb1ccd977f..31ed92aa319 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 (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 attempt to use pre-installed OpenSSH client (in that case, ensure OpenSSH client is installed and the Path environment variable is set correctly). text: | az ssh vm --resource-group myResourceGroup --name myVM --ssh-client-folder "C:\\Program Files\\OpenSSH" """ @@ -58,7 +58,7 @@ short-summary: Create an SSH config for resources (Azure VMs, Arc Servers, etc) which can then be used by clients that support OpenSSH configs and certificates long-summary: Other software (git/rsync/etc) that support setting an SSH command can be set to use the config file by setting the command to 'ssh -F /path/to/config' e.g. rsync -e 'ssh -F /path/to/config'. Users can create ssh config files that use AAD issued certificates or local user credentials. examples: - - name: Give a resource group and resource name for which to create a config using AAD issued certificates, save in a local file, and then ssh into that resource + - name: Give the resource group and machine name for which to create a config using AAD issued certificates, save in a local file, and then ssh into that resource text: | az ssh config --resource-group myResourceGroup --name myVm --file ./sshconfig ssh -F ./sshconfig myResourceGroup-myVM @@ -70,11 +70,8 @@ - name: Give a local user to create a config using local user credentials, save in local file, and then ssh into that resource text: | - az ssh config --resource-group myResourceGroup --name myVM --local-user username1 --file ./sshconfig - ssh -F ./sshconfig MyResourceGroup-myVM-username1 - - az ssh config -ip 1.2.3.4 --local-user username2 --private-key-file key --file ./sshconfig - ssh -F ./sshconfig 1.2.3.4-username2 + az ssh config --resource-group myResourceGroup --name myMachine --local-user username --certificate-file cert --private-key-file key --file ./sshconfig + ssh -F ./sshconfig MyResourceGroup-myMachine-username - name: Give Keys Destination Folder to store the generated keys and certificates. If not provided, SSH keys are stored in new folder "az_ssh_config" next to the config file. text: | @@ -94,7 +91,7 @@ rsync -e 'ssh -F ./sshconfig' -avP directory/ myvm:~/directory GIT_SSH_COMMAND="ssh -F ./sshconfig" git clone myvm:~/gitrepo - - name: Give a SSH Client Folder to use the ssh executables in that folder. 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 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). text: | az ssh vm --resource-group myResourceGroup --name myMachine --ssh-client-folder "C:\\Program Files\\OpenSSH" """ From fb61c7985480bebeac0e532b97c247ed95c97408 Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Fri, 8 Apr 2022 17:50:28 -0400 Subject: [PATCH 7/9] Add ssh not found error recommendation to constants --- src/ssh/azext_ssh/constants.py | 3 +++ src/ssh/azext_ssh/ssh_utils.py | 12 +++--------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/ssh/azext_ssh/constants.py b/src/ssh/azext_ssh/constants.py index 17c7e94824e..92c56bddfdf 100644 --- a/src/ssh/azext_ssh/constants.py +++ b/src/ssh/azext_ssh/constants.py @@ -11,3 +11,6 @@ CLEANUP_AWAIT_TERMINATION_IN_SECONDS = 30 RELAY_INFO_MAXIMUM_DURATION_IN_SECONDS = 3600 WINDOWS_INVALID_FOLDERNAME_CHARS = "\\/*:<>?\"|" +RECOMMENDATION_SSH_CLIENT_NOT_FOUND = ("Ensure OpenSSH is installed and the PATH Environment " + "Variable is set correctly.\nAlternatively, use " + "--ssh-client-folder to provide OpenSSH folder path.") diff --git a/src/ssh/azext_ssh/ssh_utils.py b/src/ssh/azext_ssh/ssh_utils.py index a2f091ecea1..4d646aef2d3 100644 --- a/src/ssh/azext_ssh/ssh_utils.py +++ b/src/ssh/azext_ssh/ssh_utils.py @@ -64,10 +64,8 @@ def start_ssh_connection(op_info, delete_keys, delete_cert): # 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: - recommendation = ("Ensure OpenSSH is installed, and that the PATH Environment Variable is set correctly.\n" - "Alternatively, use --ssh-client-folder to provide folder path with OpenSSH executables.") raise azclierror.BadRequestError(f"Failed to run ssh command with error: {str(e)}.", - recommendation) + const.RECOMMENDATION_SSH_CLIENT_NOT_FOUND) connection_duration = (time.time() - connection_duration) / 60 ssh_connection_data = {'Context.Default.AzureCLI.SSHConnectionDurationInMinutes': connection_duration} @@ -103,10 +101,8 @@ def create_ssh_keyfile(private_key_file, ssh_client_folder=None): try: subprocess.call(command) except Exception as e: - recommendation = ("Ensure OpenSSH is installed, and that the PATH Environment Variable is set correctly.\n" - "Alternatively, use --ssh-client-folder to provide folder path with OpenSSH executables.") raise azclierror.BadRequestError(f"Failed to create ssh key file with error: {str(e)}.", - recommendation) + const.RECOMMENDATION_SSH_CLIENT_NOT_FOUND) def get_ssh_cert_info(cert_file, ssh_client_folder=None): @@ -116,10 +112,8 @@ def get_ssh_cert_info(cert_file, ssh_client_folder=None): try: return subprocess.check_output(command).decode().splitlines() except Exception as e: - recommendation = ("Ensure OpenSSH is installed, and that the PATH Environment Variable is set correctly.\n" - "Alternatively, use --ssh-client-folder to provide folder path with OpenSSH executables.") raise azclierror.BadRequestError(f"Failed to get certificate info with error: {str(e)}.", - recommendation) + const.RECOMMENDATION_SSH_CLIENT_NOT_FOUND) def _get_ssh_cert_validity(cert_file, ssh_client_folder=None): From 24bdef4db251ac09e927e4671764691ce17bc76a Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Fri, 8 Apr 2022 18:26:00 -0400 Subject: [PATCH 8/9] Make sure we capture stderr only when log is not supposed to be displyed --- src/ssh/azext_ssh/ssh_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ssh/azext_ssh/ssh_utils.py b/src/ssh/azext_ssh/ssh_utils.py index 4d646aef2d3..a123df04dc0 100644 --- a/src/ssh/azext_ssh/ssh_utils.py +++ b/src/ssh/azext_ssh/ssh_utils.py @@ -58,7 +58,7 @@ 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): + if set(['-v', '-vv', '-vvv']).isdisjoint(ssh_arg_list) or log_file: connection_status = subprocess.run(command, env=env, stderr=subprocess.PIPE, text=True) else: # Logs are sent to stderr. In that case, we shouldn't capture stderr. From 39be94ff1ada22ba74de5cd6e83775e4843154a9 Mon Sep 17 00:00:00 2001 From: Vivian Thiebaut Date: Mon, 11 Apr 2022 12:51:56 -0400 Subject: [PATCH 9/9] update tests, help, and style --- src/ssh/azext_ssh/_help.py | 4 + src/ssh/azext_ssh/ssh_info.py | 2 +- .../azext_ssh/tests/latest/test_ssh_info.py | 15 ++- .../azext_ssh/tests/latest/test_ssh_utils.py | 102 +++--------------- 4 files changed, 33 insertions(+), 90 deletions(-) diff --git a/src/ssh/azext_ssh/_help.py b/src/ssh/azext_ssh/_help.py index 31ed92aa319..9f4dac63242 100644 --- a/src/ssh/azext_ssh/_help.py +++ b/src/ssh/azext_ssh/_help.py @@ -94,6 +94,10 @@ - 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). text: | az ssh vm --resource-group myResourceGroup --name myMachine --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: | + az ssh config --resource-type [Microsoft.Compute|Microsoft.HybridCompute] --resource-group myResourceGroup --name myVM --file ./myconfig """ helps['ssh cert'] = """ diff --git a/src/ssh/azext_ssh/ssh_info.py b/src/ssh/azext_ssh/ssh_info.py index 713bf83dc03..5e99459e924 100644 --- a/src/ssh/azext_ssh/ssh_info.py +++ b/src/ssh/azext_ssh/ssh_info.py @@ -160,7 +160,7 @@ def _get_ip_entry(self, is_aad): lines.append("Host " + self.ip) else: lines.append("Host " + self.ip + "-" + self.local_user) - lines.append("\tHostname " + self.ip) + lines.append("\tHostName " + self.ip) lines.append("\tUser " + self.local_user) if self.cert_file: lines.append("\tCertificateFile \"" + self.cert_file + "\"") diff --git a/src/ssh/azext_ssh/tests/latest/test_ssh_info.py b/src/ssh/azext_ssh/tests/latest/test_ssh_info.py index 4523672a627..40a231d5088 100644 --- a/src/ssh/azext_ssh/tests/latest/test_ssh_info.py +++ b/src/ssh/azext_ssh/tests/latest/test_ssh_info.py @@ -121,17 +121,25 @@ def test_get_rg_and_vm_entry(self, mock_abspath): @mock.patch('os.path.abspath') def test_get_ip_entry(self, mock_abspath): - expected_lines = [ + expected_lines_aad = [ "Host ip", "\tUser user", "\tCertificateFile \"cert_path\"", "\tIdentityFile \"priv_path\"" ] + expected_lines_local_user = [ + "Host ip-user", + "\tHostName ip", + "\tUser user", + "\tCertificateFile \"cert_path\"", + "\tIdentityFile \"priv_path\"" + ] mock_abspath.side_effect = ["config_path", "pub_path", "priv_path", "cert_path", "client_path", "cred_folder"] session = ssh_info.ConfigSession("config", "rg", "vm", "ip", "pub", "priv", False, False, "user", "cert", None, "compute", "cred", None, "client/folder") - self.assertEqual(session._get_ip_entry(), expected_lines) + self.assertEqual(session._get_ip_entry(True), expected_lines_aad) + self.assertEqual(session._get_ip_entry(False), expected_lines_local_user) @mock.patch('os.path.abspath') def test_get_arc_entry(self, mock_abspath): @@ -185,7 +193,8 @@ def test_get_config_text_compute(self, mock_abspath): "\tCertificateFile \"cert_path\"", "\tIdentityFile \"priv_path\"", "\tPort port", - "Host ip", + "Host ip-user", + "\tHostName ip", "\tUser user", "\tCertificateFile \"cert_path\"", "\tIdentityFile \"priv_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 3bd0d3b6261..4d004a0b69d 100644 --- a/src/ssh/azext_ssh/tests/latest/test_ssh_utils.py +++ b/src/ssh/azext_ssh/tests/latest/test_ssh_utils.py @@ -16,7 +16,7 @@ class SSHUtilsTests(unittest.TestCase): @mock.patch.object(ssh_utils, '_start_cleanup') @mock.patch.object(ssh_utils, '_terminate_cleanup') @mock.patch.object(ssh_utils, '_get_ssh_client_path') - @mock.patch('subprocess.call') + @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): @@ -37,13 +37,13 @@ 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, shell=platform.system() == 'Windows', env=expected_env) + mock_call.assert_called_once_with(expected_command, env=expected_env, stderr=mock.ANY, text=True) mock_terminatecleanup.assert_called_once_with(True, True, False, 'cleanup process', 'cert', 'priv', 'pub', 'log', 0) @mock.patch.object(ssh_utils, '_terminate_cleanup') @mock.patch('os.environ.copy') @mock.patch.object(ssh_utils, '_get_ssh_client_path') - @mock.patch('subprocess.call') + @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): @@ -66,7 +66,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, shell=platform.system() == 'Windows', env=expected_env) + mock_call.assert_called_once_with(expected_command, env=expected_env, stderr=mock.ANY, text=True) mock_terminatecleanup.assert_called_once_with(False, False, False, None, 'cert', 'priv', 'pub', None, 0) @@ -139,93 +139,23 @@ 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_with_client_folder_non_windows(self, mock_isfile, mock_system, mock_join): - mock_join.return_value = "ssh_path" - 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_with_client_folder_no_file(self, mock_isfile, mock_system, mock_join): + def test_get_ssh_client_path_not_found(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') + path = ssh_utils._get_ssh_client_path("ssh", "folder") + self.assertEqual(path, "ssh") - 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('platform.system') @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): + def test_get_ssh_client_path_found(self, mock_isfile, mock_system, mock_join): + mock_join.return_value = "ssh_path" 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 - - 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) + 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