SSH Remoting should handle multi-line error messages from SSH client #3612
Conversation
| var task = reader.ReadLineAsync(); | ||
| if (task.Wait(1000) && (task.Result != null)) | ||
| { | ||
| sb.Append("\r\n"); |
There was a problem hiding this comment.
Use Environment.NewLine instead? Are there cross-platform implications for system-specific newlines sent over the wire?
There was a problem hiding this comment.
Definitely. I'll make the change. Also I might be able to use ReadAsync instead of ReadLineAsync() in which case existing new lines are preserved. This just affects how the error message appears in the exception so it is not too critical.
| CloseConnection(); | ||
| } | ||
|
|
||
| private static string ReadError(StreamReader reader) |
There was a problem hiding this comment.
Would it be feasible to write a negative test that covers this scenario?
There was a problem hiding this comment.
At this time we don't have any functional tests for SSH based remoting because we don't currently have a way to automatically set it up. I thought I had an open issue about this (create an Enable-SSHRemoting cmdlet that downloads and sets up SSH for different platforms) but I don't see it. I'll add one but I do see that I created an RFC for it (https://github.com/PowerShell/PowerShell-RFC/blob/master/1-Draft/RFC0012-Enable-SSH-Remoting.md)
In any case adding negative tests (tests with malformed error stream) would require test hooks and mock ups for the transport. I am not sure it would be worthwhile at this point given that we don't yet have functional tests.
| try | ||
| { | ||
| var task = reader.ReadLineAsync(); | ||
| if (task.Wait(1000) && (task.Result != null)) |
There was a problem hiding this comment.
I'm curious about the reasoning behind the choice for a 1 second timeout over an await or a straight wait(). Did you encounter stalls in the stream reader when working on the fix or scenarios where the error lines were written with a long time gap between them?
There was a problem hiding this comment.
Good question. The reason is that the error stream reader is based on a pipe stream which is open ended and never ends. So the read operation blocks until data is available. Previously I assumed that a single stream text line constituted a single error message and when received was converted into a transport error message and the connection terminated. But SSH will return multiple line error messages (which normally just appear in the console) and I need to capture them as well as I can before terminating the connection. There is no way to know how many error lines will be streamed and so it is impossible not to hang the application by blocking on the read. My solution is to use the line async read with a timeout. I chose 1 second through ad hoc experimentation.
There was a problem hiding this comment.
@PaulHigin thanks for the clarification! Could you please add summarize this and add some comments to this line of code? I'm sure that will be very helpful to people reading the code 😄
While using SSH as a PowerShell remoting transport, any error text from the SSH client is turned into a remoting transport error and the session is ended. However, only one line of the message is saved before the session is ended. This is a problem for SSH client error messages that are multi-line, only the first line is recorded in the transport exception and the user cannot know what the error is about.
See Issue #3546
This fix modifies the SSH transport code to read multiple lines from the error stream. Since it is impossible to know how many error lines will be sent by the SSH client the stream reader runs asynchronously with a 1 second timeout per line.