Minor improvements to SSH remoting cmdlets (#2249)

* Fix for relative paths for KeyFilePath cmdlet parameter, renamed cmdlet KeyPath parameter to KeyFilePath parameter, and added tests.

* Code review comment update

* Added missing KeyFilePath check

* Test fix from review comment

src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs

@@ -693,11 +693,11 @@ namespace Microsoft.PowerShell.Commands
         [Parameter(ParameterSetName = InvokeCommandCommand.SSHHostParameterSet)]
         [Parameter(ParameterSetName = InvokeCommandCommand.FilePathSSHHostParameterSet)]
         [ValidateNotNullOrEmpty()]
-        public override string KeyPath
+        public override string KeyFilePath
         {
-            get { return base.KeyPath; }
+            get { return base.KeyFilePath; }
 
-            set { base.KeyPath = value; }
+            set { base.KeyFilePath = value; }
         }
 
         #endregion

src/System.Management.Automation/engine/remoting/commands/PSRemotingCmdlet.cs

@@ -683,7 +683,7 @@ namespace Microsoft.PowerShell.Commands
         /// <summary>
         /// SSH Target Host Name
         /// </summary>
-        [Parameter(ParameterSetName = PSRemotingBaseCmdlet.SSHHostParameterSet)]
+        [Parameter(ParameterSetName = PSRemotingBaseCmdlet.SSHHostParameterSet, Mandatory = true)]
         [ValidateNotNullOrEmpty()]
         public virtual string HostName
         {
@@ -694,7 +694,7 @@ namespace Microsoft.PowerShell.Commands
         /// <summary>
         /// SSH User Name
         /// </summary>
-        [Parameter(ParameterSetName = PSRemotingBaseCmdlet.SSHHostParameterSet)]
+        [Parameter(ParameterSetName = PSRemotingBaseCmdlet.SSHHostParameterSet, Mandatory = true)]
         [ValidateNotNullOrEmpty()]
         public virtual string UserName
         {
@@ -703,11 +703,11 @@ namespace Microsoft.PowerShell.Commands
         }
 
         /// <summary>
-        /// SSH Key Path
+        /// SSH Key File Path
         /// </summary>
         [Parameter(ParameterSetName = PSRemotingBaseCmdlet.SSHHostParameterSet)]
         [ValidateNotNullOrEmpty()]
-        public virtual string KeyPath
+        public virtual string KeyFilePath
         {
             get;
             set;
@@ -863,6 +863,14 @@ namespace Microsoft.PowerShell.Commands
         {
             base.BeginProcessing();
 
+            // Validate KeyFilePath parameter.
+            if ((ParameterSetName == PSRemotingBaseCmdlet.SSHHostParameterSet) &&
+                (this.KeyFilePath != null))
+            {
+                // Resolve the key file path when set.
+                this.KeyFilePath = PathResolver.ResolveProviderAndPath(this.KeyFilePath, true, this, false, RemotingErrorIdStrings.FilePathNotFromFileSystemProvider);
+            }
+
             // Validate IdleTimeout parameter.
             int idleTimeout = (int)SessionOption.IdleTimeout.TotalMilliseconds;
             if (idleTimeout != BaseTransportManager.UseServerDefaultIdleTimeout &&
@@ -1166,7 +1174,7 @@ namespace Microsoft.PowerShell.Commands
         /// </summary>
         protected void CreateHelpersForSpecifiedHostNames()
         {
-            var sshConnectionInfo = new SSHConnectionInfo(this.UserName, this.HostName, this.KeyPath);
+            var sshConnectionInfo = new SSHConnectionInfo(this.UserName, this.HostName, this.KeyFilePath);
             var typeTable = TypeTable.LoadDefaultTypeFiles();
             var remoteRunspace = RunspaceFactory.CreateRunspace(sshConnectionInfo, this.Host, typeTable) as RemoteRunspace;
             var pipeline = CreatePipeline(remoteRunspace);
@@ -1724,8 +1732,7 @@ namespace Microsoft.PowerShell.Commands
             }
 
             //Resolve file path
-            PathResolver resolver = new PathResolver();
-            string resolvedPath = resolver.ResolveProviderAndPath(filePath, isLiteralPath, this, false, RemotingErrorIdStrings.FilePathNotFromFileSystemProvider);
+            string resolvedPath = PathResolver.ResolveProviderAndPath(filePath, isLiteralPath, this, false, RemotingErrorIdStrings.FilePathNotFromFileSystemProvider);
 
             //read content of file
             ExternalScriptInfo scriptInfo = new ExternalScriptInfo(filePath, resolvedPath, this.Context);
@@ -3320,10 +3327,12 @@ namespace Microsoft.PowerShell.Commands
         } // RaiseOperationCompleteEvent
     } // ExecutionCmdletHelperComputerName
 
+    #region Path Resolver
+
     /// <summary>
     /// A helper class to resolve the path
     /// </summary>
-    internal class PathResolver
+    internal static class PathResolver
     {
         /// <summary>
         /// Resolves the specified path and verifies the path belongs to
@@ -3336,7 +3345,7 @@ namespace Microsoft.PowerShell.Commands
         /// <param name="allowNonexistingPaths"></param>
         /// <param name="resourceString">resource string for error when path is not from filesystem provider</param>
         /// <returns>A fully qualified string representing filename.</returns>
-        internal string ResolveProviderAndPath(string path, bool isLiteralPath, PSCmdlet cmdlet, bool allowNonexistingPaths, string resourceString)
+        internal static string ResolveProviderAndPath(string path, bool isLiteralPath, PSCmdlet cmdlet, bool allowNonexistingPaths, string resourceString)
         {
             // First resolve path
             PathInfo resolvedPath = ResolvePath(path, isLiteralPath, allowNonexistingPaths, cmdlet);
@@ -3373,7 +3382,7 @@ namespace Microsoft.PowerShell.Commands
         /// A string representing the resolved path.
         /// </returns>
         /// 
-        private PathInfo ResolvePath(
+        private static PathInfo ResolvePath(
             string pathToResolve,
             bool isLiteralPath,
             bool allowNonexistingPaths,
@@ -3466,5 +3475,7 @@ namespace Microsoft.PowerShell.Commands
         } // ResolvePath
     }
 
+    #endregion
+
     #endregion Helper Classes
 }

src/System.Management.Automation/engine/remoting/commands/PushRunspaceCommand.cs

@@ -1248,7 +1248,7 @@ namespace Microsoft.PowerShell.Commands
         /// </summary>
         private RemoteRunspace GetRunspaceForSSHSession()
         {
-            var sshConnectionInfo = new SSHConnectionInfo(this.UserName, this.HostName, this.KeyPath);
+            var sshConnectionInfo = new SSHConnectionInfo(this.UserName, this.HostName, this.KeyFilePath);
             var typeTable = TypeTable.LoadDefaultTypeFiles();
             var remoteRunspace = RunspaceFactory.CreateRunspace(sshConnectionInfo, this.Host, typeTable) as RemoteRunspace;
             remoteRunspace.Open();

src/System.Management.Automation/engine/remoting/commands/newrunspacecommand.cs

@@ -1065,7 +1065,7 @@ namespace Microsoft.PowerShell.Commands
             var sshConnectionInfo = new SSHConnectionInfo(
                 this.UserName,
                 this.HostName,
-                this.KeyPath);
+                this.KeyFilePath);
             var typeTable = TypeTable.LoadDefaultTypeFiles();
             remoteRunspaces.Add(RunspaceFactory.CreateRunspace(sshConnectionInfo, this.Host, typeTable) as RemoteRunspace);
 

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

@@ -1844,9 +1844,9 @@ namespace System.Management.Automation.Runspaces
         }
 
         /// <summary>
-        /// Key Path
+        /// Key File Path
         /// </summary>
-        private string KeyPath
+        private string KeyFilePath
         {
             get;
             set;
@@ -1867,18 +1867,18 @@ namespace System.Management.Automation.Runspaces
         /// </summary>
         /// <param name="userName">User Name</param>
         /// <param name="computerName">Computer Name</param>
-        /// <param name="keyPath">Key Path</param>
+        /// <param name="keyFilePath">Key File Path</param>
         public SSHConnectionInfo(
             string userName,
             string computerName,
-            string keyPath)
+            string keyFilePath)
         {
             if (userName == null) { throw new PSArgumentNullException("userName"); }
             if (computerName == null) { throw new PSArgumentNullException("computerName"); }
 
             this.UserName = userName;
             this.ComputerName = computerName;
-            this.KeyPath = keyPath;
+            this.KeyFilePath = keyFilePath;
         }
 
         #endregion
@@ -1930,7 +1930,7 @@ namespace System.Management.Automation.Runspaces
             SSHConnectionInfo newCopy = new SSHConnectionInfo();
             newCopy.ComputerName = this.ComputerName;
             newCopy.UserName = this.UserName;
-            newCopy.KeyPath = this.KeyPath;
+            newCopy.KeyFilePath = this.KeyFilePath;
 
             return newCopy;
         }
@@ -1996,11 +1996,17 @@ namespace System.Management.Automation.Runspaces
             //   See sshd_configuration file, subsystems section and it will have this entry:
             //     Subsystem       powershell C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe -Version 5.1 -sshs -NoLogo -NoProfile
             string arguments;
-            if (!string.IsNullOrEmpty(this.KeyPath))
+            if (!string.IsNullOrEmpty(this.KeyFilePath))
             {
+                if (!System.IO.File.Exists(this.KeyFilePath))
+                {
+                    throw new FileNotFoundException(
+                        StringUtil.Format(RemotingErrorIdStrings.KeyFileNotFound, this.KeyFilePath));
+                }
+
                 arguments = (string.IsNullOrEmpty(domainName)) ?
-                    string.Format(CultureInfo.InvariantCulture, @"-i ""{0}"" {1}@{2} -s powershell", this.KeyPath, userName, this.ComputerName) :
-                    string.Format(CultureInfo.InvariantCulture, @"-i ""{0}"" -l {1}@{2} {3} -s powershell", this.KeyPath, userName, domainName, this.ComputerName);
+                    string.Format(CultureInfo.InvariantCulture, @"-i ""{0}"" {1}@{2} -s powershell", this.KeyFilePath, userName, this.ComputerName) :
+                    string.Format(CultureInfo.InvariantCulture, @"-i ""{0}"" -l {1}@{2} {3} -s powershell", this.KeyFilePath, userName, domainName, this.ComputerName);
             }
             else
             {
@@ -2094,7 +2100,9 @@ namespace System.Management.Automation.Runspaces
                 (sshProcess == null) ||
                 (sshProcess.HasExited == true))
             {
-                throw new InvalidOperationException(RemotingErrorIdStrings.CannotStartSSHClient, ex);
+                throw new InvalidOperationException(
+                    StringUtil.Format(RemotingErrorIdStrings.CannotStartSSHClient, (ex != null) ? ex.Message : string.Empty), 
+                    ex);
             }
 
             // Create the std in writer/readers needed for communication with ssh.exe.

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

@@ -1524,13 +1524,20 @@ namespace System.Management.Automation.Remoting.Client
                 while (true)
                 {
                     string error = reader.ReadLine();
-                    if (!string.IsNullOrEmpty(error) && (error.IndexOf("WARNING:", StringComparison.OrdinalIgnoreCase) < 0))
+                    if (!string.IsNullOrEmpty(error) && (error.IndexOf("WARNING:", StringComparison.OrdinalIgnoreCase) > -1))
+                    {
+                        // Handle as interactive warning message.
+                        Console.WriteLine(error);
+                    }
+                    else
                     {
                         // Any SSH client error results in a broken session.
                         PSRemotingTransportException psrte = new PSRemotingTransportException(
                             PSRemotingErrorId.IPCServerProcessReportedError,
                             RemotingErrorIdStrings.IPCServerProcessReportedError,
-                            error);
+                            string.IsNullOrEmpty(error) ?
+                                RemotingErrorIdStrings.SSHClientEndNoErrorMessage
+                                : StringUtil.Format(RemotingErrorIdStrings.SSHClientEndWithErrorMessage, error));
                         RaiseErrorHandler(new TransportErrorOccuredEventArgs(psrte, TransportMethodEnum.CloseShellOperationEx));
                         CloseConnection();
                     }

src/System.Management.Automation/resources/RemotingErrorIdStrings.resx

@@ -1610,6 +1610,15 @@ All WinRM sessions connected to Windows PowerShell session configurations, such
   -The domain or computer name was not included with the specified credential, for example: DOMAIN\UserName or COMPUTER\UserName.</value>
   </data>
   <data name="CannotStartSSHClient" xml:space="preserve">
-    <value>An error occurred when starting the SSH.exe client needed for the remoting connection.</value>
+    <value>Failed to start the SSH client process needed for the remoting connection with error: {0}.</value>
+  </data>
+  <data name="KeyFileNotFound" xml:space="preserve">
+    <value>The specified key file {0} was not found.</value>
+  </data>
+  <data name="SSHClientEndWithErrorMessage" xml:space="preserve">
+    <value>The SSH client session has ended with error message: {0}</value>
+  </data>
+  <data name="SSHClientEndNoErrorMessage" xml:space="preserve">
+    <value>The SSH client session has ended with no error message.</value>
   </data>
 </root>

test/powershell/Modules/Microsoft.PowerShell.Core/RemotingCmdlets.Tests.ps1

@@ -0,0 +1,41 @@
+Describe "SSH Remoting Cmdlet Tests" -Tags "Feature" {
+
+    It "Enter-PSSession HostName parameter set should throw error for invalid key path" {
+
+        try
+        {
+            Enter-PSSession -HostName localhost -UserName User -KeyFilePath NoKeyFile
+            throw "Enter-PSSession did not throw expected PathNotFound exception."
+        }
+        catch
+        {
+            $_.FullyQualifiedErrorId | Should Be "PathNotFound,Microsoft.PowerShell.Commands.EnterPSSessionCommand"
+        }
+    }
+
+    It "New-PSSession HostName parameter set should throw error for invalid key path" {
+
+        try
+        {
+            New-PSSession -HostName localhost -UserName User -KeyFilePath NoKeyFile
+            throw "New-PSSession did not throw expected PathNotFound exception."
+        }
+        catch
+        {
+            $_.FullyQualifiedErrorId | Should Be "PathNotFound,Microsoft.PowerShell.Commands.NewPSSessionCommand"
+        }
+    }
+
+    It "Invoke-Command HostName parameter set should throw error for invalid key path" {
+
+        try
+        {
+            Invoke-Command -HostName localhost -UserName User -KeyFilePath NoKeyFile -ScriptBlock {1}
+            throw "Invoke-Command did not throw expected PathNotFound exception."
+        }
+        catch
+        {
+            $_.FullyQualifiedErrorId | Should Be "PathNotFound,Microsoft.PowerShell.Commands.InvokeCommandCommand"
+        }
+    }
+}

test/powershell/engine/Remoting/SSHRemotingAPI.Tests.ps1

@@ -0,0 +1,65 @@
+Describe "SSH Remoting API Tests" -Tags "Feature" {
+
+    Context "SSHConnectionInfo Class Tests" {
+
+        It "SSHConnectionInfo constructor should throw null argument exception for null UserName parameter" {
+
+            try
+            {
+                [System.Management.Automation.Runspaces.SSHConnectionInfo]::new(
+                    [System.Management.Automation.Internal.AutomationNull]::Value,
+                    "localhost",
+                    [System.Management.Automation.Internal.AutomationNull]::Value)
+
+                throw "SSHConnectionInfo constructor did not throw expected PSArgumentNullException exception"
+            }
+            catch
+            {
+                $_.FullyQualifiedErrorId | Should Match "PSArgumentNullException"
+            }
+        }
+
+        It "SSHConnectionInfo constructor should throw null argument exception for null HostName parameter" {
+
+            try
+            {
+                [System.Management.Automation.Runspaces.SSHConnectionInfo]::new(
+                    "UserName",
+                    [System.Management.Automation.Internal.AutomationNull]::Value,
+                    [System.Management.Automation.Internal.AutomationNull]::Value)
+
+                throw "SSHConnectionInfo constructor did not throw expected PSArgumentNullException exception"
+            }
+            catch
+            {
+                $_.FullyQualifiedErrorId | Should Match "PSArgumentNullException"
+            }
+        }
+
+        It "SSHConnectionInfo should throw file not found exception for invalid key file path" {
+
+            try
+            {
+                $sshConnectionInfo = [System.Management.Automation.Runspaces.SSHConnectionInfo]::new(
+                    "UserName",
+                    "localhost",
+                    "NoValidKeyFilePath")
+
+                $rs = [runspacefactory]::CreateRunspace($sshConnectionInfo)
+                $rs.Open()
+
+                throw "SSHConnectionInfo did not throw expected FileNotFoundException exception"
+            }
+            catch
+            {
+                $expectedFileNotFoundExecption = $null
+                if (($_.Exception -ne $null) -and ($_.Exception.InnerException -ne $null))
+                {
+                    $expectedFileNotFoundExecption = $_.Exception.InnerException.InnerException
+                }
+
+                ($expectedFileNotFoundExecption.GetType().FullName) | Should Be "System.IO.FileNotFoundException"
+            }
+        }
+    }
+}