Linux platform SSH process creation for remoting by PaulHigin · Pull Request #3901 · PowerShell/PowerShell

Paul Higinbotham (PaulHigin) · 2017-05-31T23:25:34Z

Update:

The technique is to now use sigaction to create an SSH client process that ignores SIGINT signals. It remains in the PowerShell process group and has access to the terminal, which means it can prompt on the command line as before. So SSH_ASKPASS dependency is not needed.

This change addresses Issue #2321. The original idea for this fix was to follow what I did in Windows and create the SSH process in its own process group. That way a Ctrl+C typed signal in PowerShell (the parent process) wouldn't be handled by the child SSH process (causing it to end), and instead would allow PowerShell to handle it through its protocol (PSRP).

But this ended up disabling the /dev/tty terminal for the SSH process so it could not prompt the user for a password, if that was how SSH was set up.

It turns out that SSH will detect if there is no tty available and fall back to using SSH_ASKPASS program if available. This will happen if the SSH child process is created in its own session. So this change does that, it creates the SSH process (used for SSH remoting) in its own session.

The downside to this is that now the SSH_ASKPASS program needs to be installed if we want to support any SSH prompting. We will need to make sure it gets installed with Enable-SSHRemoting (a separate issue/work item).

Creating a Linux process in its own session does not seem like a common need and so I felt it was not necessary to add this functionality to DotNet CoreFx. Instead I pulled some of the UNIX process creation code over to PowerShell and added the ability to create in a separate session. So this change has a fair amount of native and platform interop code.

I will update the license document to indicate that PowerShell is using this DotNet CoreFx code under the MIT license.

…o UnixSSH

…rs in the error stream

…w session

Dongbo Wang (daxian-dbw) · 2017-06-01T01:13:45Z

Paul Higinbotham (@PaulHigin) Please add reviewers for your PR and assign it to an appropriate maintainer.

Ilya (iSazonov) · 2017-06-02T04:34:44Z

I leave a comment in the original Issue

Paul Higinbotham (PaulHigin) · 2017-06-02T18:41:39Z

Ilya (@iSazonov) Please put comments here rather than providing a link. The SSH -t option to create a pseudo tty prevents using subsystems which is needed to host PowerShell for SSH remoting. So it won't work here as far as I can tell. If there was some way to create and use the child SSH process such that it is not part of the process group (so that it doesn't get Ctrl+C signals) but still can use a tty for user prompt, then that would be great. But I don't currently see a way to do this. However, I am definitely open to any ideas as this would be preferable to using SSH_ASKPASS!

Ilya (iSazonov) · 2017-06-02T19:20:10Z

Paul Higinbotham (@PaulHigin) I don't read in depth but it is interesting https://unix.stackexchange.com/questions/266866/how-to-prevent-ctrlc-to-break-ssh-connection

Ilya (iSazonov) · 2017-06-03T19:55:18Z

Oh, I think about a remote session all the time, but the problem is on the local side, isn't it? If so there is a simple test:

run Bash
run stty intr undef
run PowerShell and test Ctrl-C in remote connection

If this works well we can make the same internally in PowerShell - disable ISIG by means of TERMIOS(3) at startup time.

Paul Higinbotham (PaulHigin) · 2017-06-05T16:32:18Z

This looks interesting, but I don't understand what the fix would be. I don't want to disable the ISIG for the PowerShell /dev/tty but instead want to keep the child SSH process from getting it. I only want PowerShell to handle it. It is not clear to me that disabling ISIG via termios can be done per process. But if so that would certainly solve our problem!

BTW I did look at your previous link as a way to turn off ISIG processing on the server side, but I wasn't able to get it to work. "set -m" didn't seem to do anything.

Ilya (iSazonov) · 2017-06-07T14:41:48Z

Also std pipes must be routed

Fork inherits its.

I have no more thoughts and PG can make an conclusion.

Paul Higinbotham (PaulHigin) · 2017-06-07T14:56:27Z

Nevertheless my position remains "why do it when it is not necessary?". Its funny but I just watched some presentations about performance yesterday. A big theme from the presenter was that best practices and accepted patterns were many times poor choices performance wise. One of the top things the presenter looked for when investigating performance problems was unnecessary work such as memory allocations, thread creation, uses of locks, and other system resource usage. So after that the idea of creating an unneeded process when it is just as easy to create a single process seems undesirable.

Dongbo Wang (daxian-dbw) · 2017-06-08T00:30:12Z

Very interesting discussion here and I believe calling sigactions before execve is the best solution.

I changed to code in ForkAndExecProcess a bit on CoreFx to fix issue https://github.com/dotnet/corefx/issues/13447, I think you should use the latest code instead. I will leave a comment there.

Dongbo Wang (daxian-dbw) · 2017-06-08T00:37:26Z

+
+    // Open pipes for any requests to redirect stdin/stdout/stderr
+    if ((redirectStdin && pipe(stdinFds) != 0) || (redirectStdout && pipe(stdoutFds) != 0) ||
+        (redirectStderr && pipe(stderrFds) != 0))


The code in this function was changed a bit to address https://github.com/dotnet/corefx/issues/13447, you can see the latest code at pal_process.cpp. The change was basically to make all pipe file descriptors close-on-exec. If not doing this here, then it's possible that the ssh process will continue to block on waiting for input even if the redirected standard input has been closed in powershell.

I don't think this is applicable in this situation. The pipes are only used for PSRP messages for a single remoting session. PowerShell only closes the pipe at the end of the session at which time it also terminates the SSH process.

Without close-on-exec, the pipe file descriptors will be leaked to all subsequent processes created by powershell. The symptom I described above is one result this would cause. Although it's not applicable to the SSH remoting scenario, my worry is that there may be other symptoms resulted by the leak that would cause a problem we don't know yet which might be hard to diagnose.

Ok, the leak definitely sounds bad. I'll add the close-on-exec. So that I understand more clearly, can you provide an example where the Fd is leaked even when SSH process is terminated?

Take the write end of the redirected stdin pipe as an instance (similar to the redirected stdout/stderr pipes), a FD pointing to the write end of that pipe will be created in powershell process after creating the SSH process. Assuming powershell then creates some other processes before SSH process terminates (I'm thinking of the scenario where I create a SSH PSSession but no an interactive remote session, so it's possible to run native commands locally while the SSH process is still alive), then that FD will be inherited by all those processes, so they are all potentially able to write to that pipe or affect that pipe in possible ways.

In this case, the SSH process is the only reader of that pipe, so when SSH terminates, "the only reader dies, and then the writer gets notified about this by getting a SIGPIPE or at least an EPIPE error (depending on how signals are defined)." (quoted from here)

I'm not saying it will definitely cause a problem, but just that if the leaked FD cause trouble to us, it might be very hard to figure out the real cause.

Thanks, this definitely helps and could be an issue for the fan-out scenario. I'll have this change in the PR update.

Ilya (iSazonov) · 2017-06-08T07:22:10Z

Paul Higinbotham (@PaulHigin) Dongbo Wang (@daxian-dbw) I want to get clarify about sigactions vs sigprocmask. I still belive we can use sigprocmask after fork so OS will not trigger the sigactions empty SIGINT handler in SSH process.

Ilya (iSazonov) · 2017-06-08T09:08:24Z

+
+#if UNIX
+        private static readonly UTF8Encoding s_utf8NoBom =
+            new UTF8Encoding(encoderShouldEmitUTF8Identifier: false);


It seems we can use this everywhere. So maybe put this in "right place" as internal?

I am not sure what you mean, I only see this being used here in my branch.

I think Ilya (@iSazonov) means that s_utf8NoBom should be put at some common place so that a single instance can be used in all places in our code base.

Oh I see. Yes this does seem like something that would be useful. I'll move it to PsUtils.cs

Ilya (iSazonov) · 2017-06-08T09:09:53Z

+        /// <returns>The opened stream.</returns>
+        private static FileStream OpenStream(int fd, FileAccess access)
+        {
+            Debug.Assert(fd >= 0);


Should we add an usefull message?

Sure, but I do see the point of DotNet CoreFx developers who originated this code. The assert condition is pretty explanatory...

Ilya (iSazonov) · 2017-06-08T09:15:36Z

            if (context != null)
            {
-                var cmdInfo = context.CommandDiscovery.LookupCommandInfo("ssh.exe", CommandOrigin.Internal) as ApplicationInfo;
+                var cmdInfo = context.CommandDiscovery.LookupCommandInfo("ssh", CommandOrigin.Internal) as ApplicationInfo;


Why we remove ".exe" for Windows? We can catch .bat or .com file.

I was hoping to simply the code a bit between platforms. But I see your point and will fix.

Dongbo Wang (daxian-dbw) · 2017-06-08T18:29:14Z

Ilya (@iSazonov) I see this in the man page of SIGPROCMASK

The use of sigprocmask() is unspecified in a multithreaded process;

This makes me feel safer to use sigactions.

Dongbo Wang (daxian-dbw) · 2017-06-09T00:49:45Z

+        // Close the child's copy of the parent end of any open pipes
+        CloseIfOpen(stdinFds[WRITE_END_OF_PIPE]);
+        CloseIfOpen(stdoutFds[READ_END_OF_PIPE]);
+        CloseIfOpen(stderrFds[READ_END_OF_PIPE]);


There are some additional changes for the close-on-exec -- calls to CloseIfOpen are removed from the if (processId == 0) block, see the CoreFx code here.

I may be easier to see the differences in the PR change: dotnet/corefx@6d204a6

Dongbo Wang (daxian-dbw) · 2017-06-09T00:52:35Z

Paul Higinbotham (@PaulHigin) Could you please update the PR description? A great discussion happened in this PR that led to a better solution. So the original description is out-dated and we'd better capture the solution of using SIGPROCMASK in the PR description.

Ilya (iSazonov) · 2017-06-09T03:30:25Z

LGTM.

Sorry for the long discussion - my last code for Unix was exactly 20 years ago. I'm glad I can remember just something. 😄

Mike Richmond (mirichmo) · 2017-06-09T15:41:46Z

+        // process creation code.
+        //
+
+#if UNIX


This whole region of code should be moved to the #if UNIX region of SSHConnectionInfo.StartSSHProcessImpl. The CorePSPlatform class is designed for generic operations that use compile-time definitions in order to reduce the number of #if's throughout the code. These functions are specific to SSH remoting and are not used anywhere else in the code.

An example of a function that would work well here is SSHConnectionInfo.GetCurrentUserName(). It doesn't necessarily need to be moved right now, but it fits the pattern.

Mike Richmond (mirichmo) · 2017-06-09T15:42:22Z

                internal static extern bool IsSameFileSystemItem([MarshalAs(UnmanagedType.LPStr)]string filePathOne,
                                                                 [MarshalAs(UnmanagedType.LPStr)]string filePathTwo);
+
+                [DllImport(psLib, CharSet = CharSet.Ansi, SetLastError = true)]


This should move too

Paul Higinbotham (PaulHigin) · 2017-06-12T15:20:20Z

Ilya (@iSazonov) Thank you for your help on this.

Steve Lee (SteveL-MSFT) · 2017-06-12T17:12:15Z

Ilya (@iSazonov) can you formally approve if you're good with the changes? Thanks!

Ilya (iSazonov) · 2017-06-12T17:37:15Z

Steve Lee (@SteveL-MSFT) Approved.

Paul Higinbotham (PaulHigin) · 2017-06-14T20:11:05Z

Mike Richmond (@mirichmo) Is there any reason why this cannot be merged?

Mike Richmond (mirichmo) · 2017-06-14T20:54:03Z

 using Microsoft.Win32;
 using Microsoft.Win32.SafeHandles;
 using System.IO;
+using System.Diagnostics;


Please clean this up

Paul Higinbotham (PaulHigin) · 2017-06-14T21:53:41Z

Mike Richmond (@mirichmo) Thanks!

* Fix SSH process creation on Linux platforms * Fixed typos * Added UNIX managed code StartProcess * Fixed compile errors * Changed process create to return pid * Now resolve full SSH command file path for all platforms * Removed data reader error handling because it conflicts with SSH errors in the error stream * Clean up work * Fix for file line endings * More clean up * Change to Linux platform SSH process creation to create process in new session * Added third party license text for DotNet Core * Removed process creation in new session and added suppress SIGINT * Removed unneeded code for creating SSH process * Fixed Unix compile errors * Changes for code review * Response to more code review comments * Removed unneeded using statements

Paul Higinbotham (PaulHigin) added 12 commits May 25, 2017 14:00

Initial changes

7d6a948

Fixed typos

42dcd95

Added UNIX managed code StartProcess

009e9a3

Merge branch 'UnixSSH' of https://github.com/PaulHigin/PowerShell int…

fc4c296

…o UnixSSH

Fixed compile errors

a70d385

Changed process create to return pid

64184bb

Now resolve full SSH command file path for all platforms

ce7d9f0

Removed data reader error handling because it conflicts with SSH erro…

b77fc5b

…rs in the error stream

Clean up work

37ed69b

Fix for file line endings

ecfe4ad

More clean up

9d47665

Change to Linux platform SSH process creation to create process in ne…

45fe1b8

…w session

Paul Higinbotham (PaulHigin) added WG-Remoting PSRP issues with any transport layer Breaking-Change breaking change that may affect users OS-Linux OS-macOS labels May 31, 2017

Paul Higinbotham (PaulHigin) added this to the 6.0.0-Pending milestone May 31, 2017

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label May 31, 2017

Added third party license text for DotNet Core

10d1472

Paul Higinbotham (PaulHigin) assigned Dongbo Wang (daxian-dbw) Jun 1, 2017

Paul Higinbotham (PaulHigin) requested review from Steve Lee (SteveL-MSFT), Ilya (iSazonov), jeffbi and Mike Richmond (mirichmo) June 1, 2017 16:28

Paul Higinbotham (PaulHigin) added 2 commits June 7, 2017 16:06

Removed unneeded code for creating SSH process

5ea6a61

Fixed Unix compile errors

076f066

Dongbo Wang (daxian-dbw) reviewed Jun 8, 2017

View reviewed changes

Ilya (iSazonov) reviewed Jun 8, 2017

View reviewed changes

Changes for code review

84559c9

Dongbo Wang (daxian-dbw) reviewed Jun 9, 2017

View reviewed changes

Mike Richmond (mirichmo) suggested changes Jun 10, 2017

View reviewed changes

Response to more code review comments

fdbb844

Ilya (iSazonov) approved these changes Jun 12, 2017

View reviewed changes

Mike Richmond (mirichmo) suggested changes Jun 14, 2017

View reviewed changes

Removed unneeded using statements

4bcc0a4

Mike Richmond (mirichmo) approved these changes Jun 14, 2017

View reviewed changes

Mike Richmond (mirichmo) merged commit 578f9e5 into PowerShell:master Jun 14, 2017

Paul Higinbotham (PaulHigin) deleted the UnixSSH branch June 15, 2017 15:16

Joel Bennett (Jaykul) mentioned this pull request Jul 10, 2018

Import-Module should support nupkg files #7259

Closed

Justin Van Patten (justinvp) mentioned this pull request Apr 18, 2023

Pulumi Inline Automation API process exits immediately when hitting ctrl+c pulumi/pulumi-dotnet#124

Open

Conversation

Paul Higinbotham (PaulHigin) commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update:

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 1, 2017

Uh oh!

Ilya (iSazonov) commented Jun 2, 2017

Uh oh!

Paul Higinbotham (PaulHigin) commented Jun 2, 2017

Uh oh!

Ilya (iSazonov) commented Jun 2, 2017

Uh oh!

Ilya (iSazonov) commented Jun 3, 2017

Uh oh!

Paul Higinbotham (PaulHigin) commented Jun 5, 2017

Uh oh!

Ilya (iSazonov) commented Jun 7, 2017

Uh oh!

Paul Higinbotham (PaulHigin) commented Jun 7, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Jun 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 9, 2017

Uh oh!

Ilya (iSazonov) commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paul Higinbotham (PaulHigin) commented Jun 12, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Jun 12, 2017

Uh oh!

Ilya (iSazonov) commented Jun 12, 2017

Uh oh!

Paul Higinbotham (PaulHigin) commented Jun 14, 2017

Uh oh!

Paul Higinbotham (PaulHigin) commented May 31, 2017 •

edited

Loading

Dongbo Wang (daxian-dbw) commented Jun 8, 2017 •

edited

Loading