Skip to content

WIP: Add -SSHTransportPath Parameter - #14462

Closed
Justin Grote (JustinGrote) wants to merge 5 commits into
PowerShell:masterfrom
JustinGrote:feature/AllowCustomSSHCommand
Closed

Justin Grote (JustinGrote) wants to merge 5 commits into
PowerShell:masterfrom
JustinGrote:feature/AllowCustomSSHCommand

Conversation

@JustinGrote

@JustinGrote Justin Grote (JustinGrote) commented Dec 18, 2020 •

Copy link
Copy Markdown
Contributor

PR Summary

Implementation of #13282

PR Context

This PR adds the -SSHTransportPath parameter to allow specification of a custom ssh executable. While it can be used to use a custom version of OpenSSH for compatibility or troubleshooting reasons, the primary impetus is to enable transport authors to substitute their own executables in order to provide alternative transports (such as websockets, rendevous, Azure Bastion-like services, phone-home debug sessions, etc.), since all Powershell does is spawn ssh and then pass stdin/stdout PSRemoting XML to/from the program.

This PR is intended to be as low impact and non-breaking as possible, and as such the Arguments passed to the command do not change and are not customizable, however they do provide all the information required to establish a session so a custom wrapper can translate the SSH arguments into whatever working options it requires.

This is intended as a low-impact stop gap until the PSRemoting engine is decoupled into a subsystem and a proper CustomTransportHandler interface is provided.

WIP to complete

  • Paul Higinbotham (@PaulHigin) in the case the executable is not found, it currently gets wrapped and throws a generic eventing error, is it OK to adjust the eventing error with the file not found exception?
  • Tests

PR Checklist

@awakecoding

Copy link
Copy Markdown
Contributor

It was indeed a very minor change, now if all we could do was to add command-line parameters to be passed to the executable instead of the OpenSSH command-line, it would be fully usable! Great work. Again, I am surprised how little changes were required in the end, great work!

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 22, 2020
@ghost ghost added the Stale label Jan 6, 2021
@ghost

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

@JustinGrote

Copy link
Copy Markdown
Contributor Author

Paul Higinbotham (@PaulHigin) awaiting your feedback on the WIP items, thanks.

@ghost ghost removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept Stale labels Jan 6, 2021
@PaulHigin Paul Higinbotham (PaulHigin) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jan 6, 2021
@SteveL-MSFT

Copy link
Copy Markdown
Member

@PowerShell/powershell-committee reviewed this. We believe the right abstraction to allow for custom transports is planned work for 7.2 to enable separate PS subsystems for transports. This will be more work on the transport author side, but will ensure adherence to expectations for transports and allow more flexibility on the remoting protocol used and installable as a module. For this reason, we would not accept this PR.

@SteveL-MSFT Steve Lee (SteveL-MSFT) added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 6, 2021
@JustinGrote

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) Understood, the goal of this PR was to provide a stop-gap to that eventual work with the assumption it would be an involved process at least a year out, however if it is going to be scheduled for 7.2 then that's a close enough timeframe. Thank you for your consideration.

@JustinGrote

Justin Grote (JustinGrote) commented Jan 19, 2021 •

Copy link
Copy Markdown
Contributor Author

Marc-André Moreau (@awakecoding) FYI, it was rejected

@awakecoding

Copy link
Copy Markdown
Contributor

Marc-André Moreau (@awakecoding) FYI, it was rejected

I know, it's a bummer. I understand that a different design approach was decided and potentially planned for 7.2, but I still think it is overkill and complementary to this first approach. I am just concerned that we may end up without a solution before the feature freeze for 7.2 given the much larger scope of a plugin system and all the stuff that goes with it.

@JustinGrote

Justin Grote (JustinGrote) commented Jan 21, 2021 •

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) Joey Aiello (@joeyaiello) Last case I'll make for this and I'll drop it since the community call wasn't a great forum and the Q&A didn't post my full explanation.

I absolutely 100% hear your concerns about supportability and the fact you have to live with the changes, and I agree with Steve Lee (@SteveL-MSFT) that the original solution was the "hack" to begin with as the hardcoded .exe. I absolutely also think that the remoting transport infrastructure should unlock the seal/private on the transporthandler class or reimplemented as an interface so that it can be reimplemented as required which is clearly Paul Higinbotham (@PaulHigin)'s original design, the problem is that it's difficult to do plugins in a "powershelly" way for that as its architected and it's not worth heavily reimplementing if it's moving out to a subsystem.

I also totally agree that moving it into a separate subsystem is the best long term solution and I look forward to that, however given the strained priorities of the already burdened team I'm very dubious this will make 7.2 once the real intricacies, edge cases, etc. get involved with moving it out to a subsystem.

As to the comment from Joey Aiello (@joeyaiello) on "totally fine using a fork", the problem here is that it is a client operation, so without wide adoption, asking everyone to install a custom powershell just to use a custom transport is antithetical to the purpose, which is to make powershell remoting more easily accessible via phone home techniques, Azure Relay, websockets, etc.

So, that being said,

From a support standpoint this should be seen as fixing the hack and not introducing a new one. ssh.exe should have never been hardcoded, at the very least it should have been made available as a settings variable, which is basically what this does. it does not require the Powershell team to endorse custom transports, to support their implementation or use, or to support users who use them, any less than the Powershell team would be expected to support custom Extension Vaults made for Secret Management. So I don't think the support argument is a valid argument unless there's something I'm missing, because I didn't hear a specific support scenario that you are concerned about "living with" for the next 3-5 years.

In terms of "user whiplash", I don't imagine you plan to rip out the entire remoting system, instead have the subsystem run side-by-side with a toggleable option. If you plan to rip out parameters like -SSHTransport as a breaking change then that's going to have a lot more impact to users by far. So this wouldn't create whiplash, it would be one method of implementation and then the subsystem would become a new one.

So, I ask to reconsider from that standpoint, at least as an experimental feature which I'm happy to adjust it to be, but I won't press any more on it. Thanks for your time and all you do, the gravity of decisions that have to be made for a product that has a massive installation base and a reputation for high quality and stability are not lost on me, I just think the standard arguments being made in that regard do not apply here.

@PaulHigin

Copy link
Copy Markdown
Contributor

As the implementer of the SSH transport feature, I can tell you that we thought long and hard about shelling out to SSH. The implementation is less than ideal and fraught with problems that were difficult to deal with. But we decided to go with it anyway because SSH is ubiquitous and the value of using it was great.

But I feel the case for extending it in a general way is much less compelling. If we are going to provide for pluggable remoting transports, I strongly feel we should do it in a much more robust way.

@awakecoding

Copy link
Copy Markdown
Contributor

Paul Higinbotham (@PaulHigin) the current choice may be seen as less than ideal, but it works, and from what has been discussed in the community calls, using something other than OpenSSH is very unlikely to be supported officially due to the strict security review requirements.

The way it currently works is likely to remain the only way it works for a long time, and it's not necessarily a bad thing: by launching ssh.exe as a subprocess and interacting with it using nothing more than standard input/output streams, you have effectively decoupled the OpenSSH client from PowerShell in a manner very similar to how the OpenSSH server is very decoupled from the PowerShell server through the OpenSSH subsystem interface.

Let me explain: on the OpenSSH server, one needs to configure a "powershell" subsystem with a command to execute. When the OpenSSH server sees the "powershell" subsystem it just calls that command (PowerShell in SSH server mode) and interacts with it using nothing more than standard input/output streams. Sounds familiar? That's exactly the same as on the client, except with roles reversed (OpenSSH calls PowerShell on the server, PowerShell calls OpenSSH on the client).

What we're asking for only makes the current SSH implementation more flexible, such that any executable with any parameters can be launched as a replacement of the OpenSSH client. Maybe you feel the current approach is not satisfactory, which is why you'd rather not improve it to make it possible to override those two, but that's really all we're asking for. It would open a world of possibilities, and I can confirm that it works perfectly well in our replacement transport that now ships as part of our product.

In other words, we can transform "unsatisfactory" OpenSSH integration into pure gold with a minor tweak allowing anybody to pass explicitly the following:

  • SSH client executable (other than ssh.exe)
  • SSH client parameters (array of arguments)

If we can do that, we've just made the client-side integration on par with the server-side integration. The OpenSSH server lets you specify any command and arguments for a given subsystem name, so I don't see why the PowerShell client wouldn't be able to accept any command and arguments, especially if they are to be passed explicitly to PowerShell, requiring no specific registration or plugin system.

Last but not least, I would consider this minor improvement/fix a complement to any future custom transport extensibility. Even with a full custom transport plugin system, the current OpenSSH client integration will likely always remain, so it wouldn't bring additional technical debt into the core project. As long as PowerShell supports calling "ssh.exe", this would remain supported with very limited code to "maintain".

To conclude, I should stress the fact that this already works extremely well today, it is just a real pain to override. I have to place an executable named "ssh.exe" in a directory that I prepend to the PATH, and then I have to modify my replacement executable to accept the same command-line arguments as ssh.exe, but read its real command line parameters through an environment variable instead. The only "hack" here is what has to be done today to override the parameters that currently cannot be passed explicitly. Don't make us beg, but please reconsider this, it is extremely valuable for so little.

@JustinGrote
Justin Grote (JustinGrote) deleted the feature/AllowCustomSSHCommand branch July 14, 2022 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants