Skip to content

Conversation

@PaulHigin
Copy link
Contributor

@PaulHigin PaulHigin commented Apr 6, 2021

PR Summary

This fixes the SSH remoting hang issue: #15174

PR Context

If an SSH remoting endpoint configuration (sshd_config file) is misconfigured, the ssh.exe client process used to make the ssh connection fails silently. No error is emitted. As a result, the PowerShell remoting layer waits indefinitely for protocol messages.

I also noticed that the base transport manager message queue threads were not being cleaned up in this case, because a connection is never established and the normal protocol close processing never occurs. So I have modified the code to ensure this clean up is done in this SSH connection failed case.

After some discussion, the proposed fix is to add a -ConnectingTimeout parameter to SSH remoting that abandons a connection attempt after the timeout period. The default value will be infinite so as not to introduce regressions.

This change will affect:

Enter-PSSession SSH parameter set
Invoke-PSSession SSH parameter set
New-PSSession SSH parameter set
SSHConnectionInfo class

PR Checklist

@PaulHigin PaulHigin requested a review from JamesWTruher April 6, 2021 22:21
@PaulHigin PaulHigin changed the title Fix SSH remoting connection hang with misconfigured endpoint [WIP] Fix SSH remoting connection hang with misconfigured endpoint Apr 7, 2021
@PaulHigin PaulHigin added WG-Remoting PSRP issues with any transport layer Needs-Triage The issue is new and needs to be triaged by a work group. labels Apr 7, 2021
@PaulHigin
Copy link
Contributor Author

Remoting working group: Please see solution proposal in PR Context section.

@TravisEz13
Copy link
Member

Remoting working group: The proposed fix to add a -ConnectingTimeout parameter to SSH remoting that abandons a connection attempt after the timeout period. The default value will be infinite so it is not a breaking change.

This seems like a good fix.

@PaulHigin PaulHigin changed the title [WIP] Fix SSH remoting connection hang with misconfigured endpoint Fix SSH remoting connection hang with misconfigured endpoint Apr 12, 2021
@PaulHigin
Copy link
Contributor Author

After some experimentation, it appears the random hang is during the connection close process, if the target is no longer responding. The base transport manager already has code to handle this with a try/catch around the text writer object that sends the PSRP message. However, the catch didn't include 'ObjectDisposedException' and after adding this in the handler, the hang appears to have gone away.

Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic! :-)

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 13, 2021
@daxian-dbw daxian-dbw merged commit 046e0fe into PowerShell:master Apr 13, 2021
@daxian-dbw daxian-dbw removed the Needs-Triage The issue is new and needs to be triaged by a work group. label Apr 13, 2021
@daxian-dbw daxian-dbw added this to the 7.2.0-preview.5 milestone Apr 13, 2021
@ghost
Copy link

ghost commented Apr 14, 2021

🎉v7.2.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

TravisEz13 added a commit that referenced this pull request Apr 16, 2021
[7.2.0-preview.5] - 2021-04-14

* Breaking Changes

- Make PowerShell Linux deb and RPM packages universal (#15109)
- Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035)
- Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!)

* Experimental Features

- `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692)

* Engine Updates and Fixes

- Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!)

* General Cmdlet Updates and Fixes

- Fix SSH remoting connection never finishing with misconfigured endpoint (#15175)
- Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969)
- Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048)
- Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!)
- Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!)
- Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!)
- Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!)
- Allow `Set-Clipboard` to accept empty string (#14579)
- Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943)
- Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077)

* Code Cleanup

<details>

<summary>

<p>We thank the following contributors!</p>
<p>@xtqqczze, @iSazonov, @ZhiZe-ZG</p>

</summary>

<ul>
<li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li>
<li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li>
<li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li>
<li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li>
<li>Remove unnecessary <code>Array</code> -&gt; <code>List</code> -&gt; <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li>
<li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li>
<li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li>
<li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li>
</ul>

</details>

* Tools

- Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!)

* Tests

- Add the missing tag in Host Utilities tests (#14983)
- Update `copy-props` version in `package.json` (#15124)

* Build and Packaging Improvements

<details>

<summary>

<p>We thank the following contributors!</p>
<p>@JustinGrote</p>

</summary>

<ul>
<li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li>
<li>Make package validation regex accept universal Linux packages (#15226)</li>
<li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li>
<li>Make MSI and EXE signing always copy to fix daily build (#15191)</li>
<li>Sign internals of EXE package so that it works correctly when signed (#15132)</li>
<li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li>
<li>Update daily release tag format to  work with new Microsoft Update work (#15164)</li>
<li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li>
<li>Treat rebuild branches like release branches (#15099)</li>
<li>Update WiX to 3.11.2 (#15097)</li>
<li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li>
<li>Allow patching of preview releases (#15074)</li>
<li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li>
<li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li>
<li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li>
</ul>

</details>

* Documentation and Help Content

- Merge `7.2.0-preview.4` changes to master (#15056)
- Update `README` and `metadata.json` (#15046)
- Fix broken links for `dotnet` CLI (#14937)

[7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log WG-Remoting PSRP issues with any transport layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants