Skip to content

Fix for SSH remoting when banner is enabled on SSHD endpoint#16205

Merged
daxian-dbw merged 5 commits intoPowerShell:masterfrom
PaulHigin:fix-ssh-banner
Oct 28, 2021
Merged

Fix for SSH remoting when banner is enabled on SSHD endpoint#16205
daxian-dbw merged 5 commits intoPowerShell:masterfrom
PaulHigin:fix-ssh-banner

Conversation

@PaulHigin
Copy link
Copy Markdown
Contributor

@PaulHigin PaulHigin commented Oct 6, 2021 •
edited
Loading

PR Summary

This is a fix for Issue #15759.

PR Context

The SSH transport currently terminates a connection attempt if SSH writes any error to stdErr stream, essentially making any SSH error a terminating error. However, data stdErr is unreliable for this since it can contain warnings, the endpoint banner text, and probably other non-error related information, and this can cause an SSH connection to fail unnecessarily.

Fix is to just remove this code and only write the stdErr data to the terminal. A user can terminate an SSH connection in one of two ways;

  • Typing Ctrl+C in an interactive session
  • Using the -ConnectingTimeout parameter in the New-PSSession, Invoke-Command, Enter-PSSession cmdlets

PR Checklist

@ghost ghost assigned daxian-dbw Oct 6, 2021
iSazonov
iSazonov reviewed Oct 7, 2021
View reviewed changes
Comment thread src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs Outdated
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 7, 2021
@iSazonov iSazonov marked this pull request as draft October 7, 2021 05:01
@PaulHigin PaulHigin changed the title [WIP] Fix for SSH remoting when banner is enabled on SSHD endpoint Fix for SSH remoting when banner is enabled on SSHD endpoint Oct 7, 2021
@PaulHigin PaulHigin added this to the 7.2.0-preview.10 milestone Oct 7, 2021
@ghost ghost removed this from the 7.2.0-preview.10 milestone Oct 7, 2021
@ghost
Copy link
Copy Markdown

ghost commented Oct 7, 2021

Open PRs should not be assigned to milestone, so they are not assigned to the wrong milestone after they are merged. For backport consideration, use a backport label.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 11, 2021
@PaulHigin
Copy link
Copy Markdown
Contributor Author

PaulHigin commented Oct 26, 2021

@daxian-dbw Is there anything preventing this from being merged into vNext branch?

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 26, 2021
@PaulHigin PaulHigin added this to the 7.3-Consider milestone Oct 26, 2021
@ghost ghost removed this from the 7.3-Consider milestone Oct 26, 2021
@ghost
Copy link
Copy Markdown

ghost commented Oct 26, 2021

Open PRs should not be assigned to milestone, so they are not assigned to the wrong milestone after they are merged. For backport consideration, use a backport label.

@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Oct 27, 2021

@PaulHigin You forgot to move this to "Ready for review", so I didn't know this is ready to be merged.
I will mark it as ready and merge it.

@daxian-dbw daxian-dbw marked this pull request as ready for review October 27, 2021 00:54
@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Oct 27, 2021

Oh, there are conflicts. Can you please rebase your changes to resolve the conflicts?

@PaulHigin
Copy link
Copy Markdown
Contributor Author

PaulHigin commented Oct 28, 2021

I checked the This PR is ready to merge and is not Work in Progress.' in the check list. I didn't see a Ready for Review. Also I disagree with adding a comma to the end of the experimental list.

@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Oct 28, 2021

I didn't see a Ready for Review.

Ilya marked your PR as draft when he left his comment:
image

I marked it "ready for review" 2 days ago when saw your comment:
image

"Ready for review" button is at the right corner of the CI part of a draft PR:
image

Also I disagree with adding a comma to the end of the experimental list.

It has been this way for previous changes to this list, for example, PSNativeCommandErrorActionPreferenceFeatureName, PSAnsiRenderingFileInfo, and PSLoadAssemblyFromNativeCode. It adds a little convenience when adding a new item at the end of the list.

@daxian-dbw daxian-dbw merged commit bd4e559 into PowerShell:master Oct 28, 2021
@PaulHigin
Copy link
Copy Markdown
Contributor Author

PaulHigin commented Oct 28, 2021

Just because it has been that way before doesn't mean I have to agree to it :)

@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Oct 28, 2021

Of course :) Just wanted to clarify why I made that minor change.

@daxian-dbw daxian-dbw mentioned this pull request Nov 12, 2021
Closed
5 tasks
@ghost
Copy link
Copy Markdown

ghost commented Dec 16, 2021

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
@PaulHigin @TrapGodBrim
Fix for SSH remoting when banner is enabled on SSHD endpoint (PowerSh…
77530b8 
…ell#16205)
@daxian-dbw daxian-dbw mentioned this pull request Sep 26, 2022
Merged
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

Assignees

@daxian-dbw daxian-dbw

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PaulHigin @daxian-dbw @iSazonov