Start-Process: Fix -Credential error as non-admin

[jborean93]

PR Summary

When using Start-Process -Credential in a non-administrator context with a different user's credentials the code currently throws an exception when attempting to get the new process' handle. This change treats this error as a warning only if -PassThru was specified as it only affects certain members on the returned object like ExitCode but others like WaitForExit() still work fine.

Fixes: #21073

PR Context

The code to retrieve the Handle property was added by #20749 which tries to set the internal state of the Process object. This works in most cases but can fail if the user is not running as admin and the credentials specified was for another user. This PR turns the exception it raises about being unable to retrieve the handle to now become a warning as some things can still be used with the returned Process object.

This isn't a change in the status quo as going back to .NET Framework the returned Process object in the above scenario still could not do things like get the ExitCode due to permissions problems. This just ensures that an error/exception is not raised and code that worked in the back will still continue to do so.

The ultimately fix here is to start the process through the .NET System.Diagnostics.Process class instead of using PInvoke with CreateProcess* but until they add an API to create a suspended process or build the Process object from a SafeHandle PowerShell cannot do so for all scenarios it deals with

• Add a way to create a suspended Process dotnet/runtime#15026
• Make it possible to start a process suspended, and later resume it. dotnet/runtime#67642
• [API Proposal]: ProcessStartInfo.CreationFlags (for Windows only) dotnet/runtime#71515
• [API Proposal]: ProcessStartInfo.CreateSuspended, Process.Resume dotnet/runtime#94127

The PR should be considered for a backport to 7.4.x as that was when the change was also introduced.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
      (which runs in a different PS Host).
      • Issue filed:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Issue filed:
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Issue filed:
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
      • Issue filed:

[jborean93]

Just as an FYI I did not create a test in this as it would require being able to create another user (requires admin) then run as non-admin. The test runner doesn't seem to have an easy way to really do this so I skipped it.

[iSazonov]

Just as an FYI I did not create a test in this as it would require being able to create another user (requires admin) then run as non-admin. The test runner doesn't seem to have an easy way to really do this so I skipped it.

We could try to create a test hook. Keyword is InternalTestHooks. I don't know how much complexity is warranted, but a regression defense would be desirable.

[jborean93]

We could try to create a test hook. Keyword is InternalTestHooks

Isn't that to just tell PowerShell to do something differently internally? I'm not sure how that would help here as the exception occurs in .NET code and not PowerShell. Unless I'm mistaken?

The problem that we need to test is doing Start-Process -Credential $cred where $cred is another user's credentials. The trouble with setting this up is the test needs to run as non-admin but we need admin rights to create the user.

[iSazonov]

Test hooks are definitely something we do internally. It's kind of like having our own test Web server. And that's not easy to implement for processes. This effort makes sense if we want to actively develop this cmdlet.

On the other hand, I suppose we could use the GiHub action config to create a regular user with a single-session password. Or try net user /add inside the test. That would be easier than the test hook.

[jborean93]

Yea the proper method would be to configure CI to create a test user for this (and other tests) as an admin before it starts pester as the non-admin user. I’ve got no experience with the CI setup in this repository and I probably don’t want to go fiddling around with it without someone from that side advising what they want to do there.

As for the test hook I can see if being useful if we want to test out specific code branches but in this case it’s an exception being raised in code that we are calling rather than in code that we control. It wouldn’t make sense to raise an exception ourselves here to just catch it IMO.

[iSazonov]

Interesting, .Net uses WindowsTestAccount to create a test user.
We could try to add this to our test common module. Then it would be easier for us to create tests for the Start-Process

[jborean93]

Creating the test user is the simple part with builtin cmdlets New-LocalUser. The hard part is getting the rights to do so in a non-admin context or running the test as admin an de-elevating for this particular scenario.

[iSazonov]

Creating the test user is the simple part with builtin cmdlets New-LocalUser.

I forget that we have Windows PowerShell. 😄

We run unelevated tests with
Start-UnelevatedProcess

PowerShell/build.psm1

Lines 1664 to 1682 in b23607e

	function script:Start-UnelevatedProcess
	{
	param(
	[string]$process,
	[string[]]$arguments
	)
	if (-not $environment.IsWindows)
	{
	throw "Start-UnelevatedProcess is currently not supported on non-Windows platforms"
	}
	if (-not $environment.OSArchitecture -eq 'arm64')
	{
	throw "Start-UnelevatedProcess is currently not supported on arm64 platforms"
	}
	runas.exe /trustlevel:0x20000 "$process $arguments"
	}

So it seems it is easy to create a test user account.

[jborean93]

So it seems it is easy to create a test user account.

The question is more when to integrate it. We would need to create the test user before running the Pester process and pass through the credentials somehow. It also would need to cleanup the user once the test is done. I don’t feel comfortable enough without the people who maintain the CI part of this repo to provide their input. I’m also somewhat concerned that the talk to add tests will bog down the fix especially since it should be backported to the next 7.4 release.

[iSazonov]

I think this PR will be reviewed quickly as it is a regression.

Also, I think we can easily add (and remove) a user to build.psm1 in the try-catch-finally block that is listed above. The only requirement is to generate a password dynamically. It will be in a pwsh variable for the duration of the test.
@TravisEz13 Is this acceptable?

[daxian-dbw]

Code changes look good but need Cmdlet WG to review the behavior.

[iSazonov]

For WG. It seems we never write "warnings" to avoid "noise". So we could remove the "warning" at all or replace it with "verbose" or "debug".

[SteveL-MSFT]

Should we check the HRESULT for the specific error rather than treating them all the same?

[jborean93]

As getting the Handle was an attempt to try and set the internal state and not something that is being used in the code I didn't think it was worth setting a specific error code. I can change it but I didn't think it warranted catching a specific error because we aren't doing anything to work around the error.

[iSazonov]

@SteveL-MSFT Here we should rather move in the other direction. Since the only operation we do is getting a handle, we rather need to catch and ignore all exceptions, especially since if there is a question about behavior, we can always display the debug message.
Although I think Win32Exception covers everything that is needed.

[SteveL-MSFT]

The @PowerShell/wg-powershell-cmdlets discussed this and compared the behavior to WinPS5.1. We recommend changing from Warning to a Debug message, then handle is null w/o any message so the behavior is consistent with 5.1.

[jborean93]

Updated to become WriteDebug.

[jborean93]

Thanks for the reviews and merge!

[adityapatwardhan]

We will wait on this change to be released in a preview before considering a backport to 7.4.

[jborean93]

Will that even make a difference. There are people already affected by this and the chances of someone actually using the preview for such use cases I see as rare.