Support `Get-PSHostProcessInfo` and `Enter-PSHostProcess` on non-Windows #8232

SteveL-MSFT · 2018-11-11T22:19:44Z

PR Summary

There a some differences in support of named pipes for Windows and non-Windows. Named pipes on Unix have a 104 character path limit. On macOS, the $env:TMPDIR (on my system) is already 49 characters; corefx adds 12 more. Since AppDomainName isn't really used, changed it from DefaultAppDomain to None to shorten the name. Need to keep it since Windows PowerShell expects it. Changed starttime part of pipe name to 8 hex characters which is to provide uniqueness to the pipe name.

Fix #5110
Possibly also fix 6435, but need verification. #6435

PR Checklist

cleanup existing Windows specific code and update tests

src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs

src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs

use same model as AMSI uninitialize to cleanup named pipes on exit when enumerating pshostprocessinfo, check if they are valid, otherwise exclude and try to remove

PaulHigin · 2018-11-12T18:01:58Z

@SteveL-MSFT

Regarding the server named pipe dispose, this is done in WindowsPowerShell via the CLR AppDomain unload event, which is not supported on CORECLR. However, you can use the AppDomain.CurrentDomain.ProcessExit event that we use for AMSI un-initializing. Unfortunately, these CLR events are not reliable, but they are better than nothing.

The start time added to the filename is intended to make the named pipe instance unique, to prevent a DOS or man-in-the-middle attack. We will want to keep something like this, even if it means using something shorter.

src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs

address Paul's feedback

TylerLeonhardt

LGTM 👍

src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs

src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs

src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs

TylerLeonhardt · 2018-11-12T19:15:03Z

I love when a PR has more deletions than insertions yet more functionality

src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs

src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs

src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs

src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs

daxian-dbw · 2018-11-13T22:58:21Z

As for the domain name, do we really need it for the pipe name? Is multiple domain is going to be supported in .NET Core 3.0?

I have no problem keeping it for now until we get an accurate answer for the app domain support.

SteveL-MSFT · 2018-11-13T23:21:02Z

@daxian-dbw best I can tell, AppDomains are deprecated in .Net Core and not in the roadmap I can see for 3.0. So it may be fine to remove it for non-Windows. We'll have to keep it for Windows so that Windows PowerShell and PowerShell Core interop still works however.

PaulHigin · 2018-11-13T23:27:02Z

Yes, I have proposed dropping the Domain name. If it gets supported again in dotNet Core then we can add it back.

SteveL-MSFT · 2018-11-14T02:13:28Z

To retain interoperability between Windows PowerShell and keep the code similar between Windows and non-Windows, I'll shorten the DefaultAppDomainName to 'none' but will keep it in the pipe name.

address Paul and Dongbo's feedback

daxian-dbw

We'll have to keep it for Windows so that Windows PowerShell and PowerShell Core interop still works however.

Then we should keep the AppDomain field on all platforms. none is short enough and it's better to keep the same schema.

PaulHigin · 2018-11-14T19:29:51Z

@SteveL-MSFT I forgot about maintaining compatibility with Windows PowerShell. We definitely need to ensure we can still list/parse/connect to those hosts.

SteveL-MSFT · 2018-11-14T20:57:09Z

Ugh, with the changes I've made since, I can't connect to/from Windows PowerShell and PowerShell Core. Debugging.

revert defaultappdomainname and starttime format on Windows as it's needed to interop with Windows PowerShell

SteveL-MSFT · 2018-11-14T21:05:50Z

Looks like DefaultAppDomain is necessary on Windows as there is a check for that string and the starttime needs to stay as filetime integers on Windows to support interop between Windows PowerShell and PowerShell Core. I believe everything else is addressed. @PaulHigin can you take another look?

PaulHigin · 2018-11-14T22:09:39Z

@SteveL-MSFT Yeah, for Windows we need to keep the DomainName to be able to connect to WindowsPowerShell host, and that means keeping the default domain name for Windows platform pipe names.

SteveL-MSFT · 2018-11-14T22:12:02Z

@PaulHigin yes, the recent commit has the same DefaultAppDomainName and starttime format on Windows and I verified manually it works from and to Windows PowerShell

PaulHigin

LGTM

I wonder if we should add a test to ensure WindowsPowerShell is listed by Get-PSHostProcessInfo? WindowsPowerShell is always installed on Windows so it shouldn't be too hard to run powershell.exe and check.

SteveL-MSFT · 2018-11-15T01:32:49Z

@PaulHigin good suggestion. Let me add a test for Windows PowerShell interop.

added tests to validate interop with Windows PowerShell

daxian-dbw · 2018-11-15T20:17:02Z

@SteveL-MSFT, there are tests about Enter-PSHostProcess failing in AppVeyor, can you please take a look?

SteveL-MSFT · 2018-11-15T20:30:44Z

@daxian-dbw it appears to be a problem with redirected input with PSReadLine and only on AppVeyor. We're moving off AppVeyor (eventually), so I think we can ignore this otherwise we'll have to special case this by perhaps removing PSReadLine for this test

daxian-dbw · 2018-11-15T20:33:40Z

Given @SteveL-MSFT's clarification on the AppVeyor test failure, I'm merging this PR.

…tforms (PowerShell#8232) There a some differences in support of named pipes for Windows and non-Windows. Named pipes on Unix have a 104 character path limit. On macOS, the `$env:TMPDIR` (on my system) is already 49 characters; corefx adds 12 more. Since AppDomainName isn't really used, changed it from `DefaultAppDomain` to `None` to shorten the name. Need to keep it since Windows PowerShell expects it. Changed `starttime` part of pipe name to 8 hex characters which is to provide uniqueness to the pipe name.

initial code to add UNIX path to named pipes

6b917a0

SteveL-MSFT requested a review from PaulHigin November 11, 2018 22:19

SteveL-MSFT requested review from BrucePay, TravisEz13, adityapatwardhan, anmenaga, daxian-dbw and mirichmo as code owners November 11, 2018 22:19

fix codefactor and test failure

9b54fd2

SteveL-MSFT force-pushed the named-pipes branch from af2e3bb to 9b54fd2 Compare November 12, 2018 00:00

remove unnecessary legacy Windows specific doe

589c644

SteveL-MSFT changed the title ~~WIP: Support Get-PSHostProcessInfo and Enter-PSHostProcess on non-Windows~~ Support Get-PSHostProcessInfo and Enter-PSHostProcess on non-Windows Nov 12, 2018

SteveL-MSFT force-pushed the named-pipes branch from a79d490 to c65c187 Compare November 12, 2018 00:25

SteveL-MSFT changed the title ~~Support Get-PSHostProcessInfo and Enter-PSHostProcess on non-Windows~~ WIP: Support Get-PSHostProcessInfo and Enter-PSHostProcess on non-Windows Nov 12, 2018

SteveL-MSFT force-pushed the named-pipes branch 2 times, most recently from 3669b26 to a6649fd Compare November 12, 2018 01:08

SteveL-MSFT assigned daxian-dbw Nov 12, 2018

SteveL-MSFT force-pushed the named-pipes branch 2 times, most recently from be57216 to 4a96f68 Compare November 12, 2018 01:38

[feature]

2504dd9

cleanup existing Windows specific code and update tests

SteveL-MSFT force-pushed the named-pipes branch from 4a96f68 to 2504dd9 Compare November 12, 2018 01:58

iSazonov reviewed Nov 12, 2018

View reviewed changes

SteveL-MSFT added 2 commits November 11, 2018 22:41

address Ilya's feedback, some minor cleanup

33f6254

[feature]

60f2f7d

use same model as AMSI uninitialize to cleanup named pipes on exit when enumerating pshostprocessinfo, check if they are valid, otherwise exclude and try to remove

SteveL-MSFT changed the title ~~WIP: Support Get-PSHostProcessInfo and Enter-PSHostProcess on non-Windows~~ Support Get-PSHostProcessInfo and Enter-PSHostProcess on non-Windows Nov 12, 2018

PaulHigin suggested changes Nov 12, 2018

View reviewed changes

src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Show resolved Hide resolved

src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated Show resolved Hide resolved

[feature]

f2a9c04

address Paul's feedback

TylerLeonhardt approved these changes Nov 12, 2018

View reviewed changes

PaulHigin suggested changes Nov 13, 2018

View reviewed changes

TravisEz13 added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Nov 14, 2018

[feature]

6cbc498

address Paul and Dongbo's feedback

SteveL-MSFT force-pushed the named-pipes branch from 010e0a4 to 6cbc498 Compare November 14, 2018 06:50

SteveL-MSFT removed Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log labels Nov 14, 2018

Remove extra new lines

6d46bad

daxian-dbw approved these changes Nov 14, 2018

View reviewed changes

[feature]

f08bf2a

revert defaultappdomainname and starttime format on Windows as it's needed to interop with Windows PowerShell

PaulHigin approved these changes Nov 14, 2018

View reviewed changes

[feature]

1f26306

added tests to validate interop with Windows PowerShell

daxian-dbw added the Documentation Needed in this repo Documentation is needed in this repo label Nov 15, 2018

daxian-dbw merged commit c5dd3bd into PowerShell:master Nov 15, 2018

adityapatwardhan added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Dec 3, 2018

JustinGrote mentioned this pull request Jan 3, 2019

Invoke-Command : Windows Principal functionality is not supported on this platform. #6435

Closed

SteveL-MSFT deleted the named-pipes branch March 10, 2020 21:02

Support Get-PSHostProcessInfo and Enter-PSHostProcess on non-Windows #8232

Support Get-PSHostProcessInfo and Enter-PSHostProcess on non-Windows #8232

Uh oh!

Conversation

SteveL-MSFT commented Nov 11, 2018 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PaulHigin commented Nov 12, 2018

Uh oh!

Uh oh!

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TylerLeonhardt commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daxian-dbw commented Nov 13, 2018

Uh oh!

SteveL-MSFT commented Nov 13, 2018

Uh oh!

PaulHigin commented Nov 13, 2018

Uh oh!

SteveL-MSFT commented Nov 14, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

PaulHigin commented Nov 14, 2018

Uh oh!

SteveL-MSFT commented Nov 14, 2018

Uh oh!

SteveL-MSFT commented Nov 14, 2018

Uh oh!

PaulHigin commented Nov 14, 2018

Uh oh!

SteveL-MSFT commented Nov 14, 2018

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Nov 15, 2018

Uh oh!

daxian-dbw commented Nov 15, 2018

Uh oh!

SteveL-MSFT commented Nov 15, 2018

Uh oh!

daxian-dbw commented Nov 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Support `Get-PSHostProcessInfo` and `Enter-PSHostProcess` on non-Windows #8232

Support `Get-PSHostProcessInfo` and `Enter-PSHostProcess` on non-Windows #8232

SteveL-MSFT commented Nov 11, 2018 •

edited by daxian-dbw

Loading

TylerLeonhardt commented Nov 12, 2018 •

edited

Loading