chore: Use win apis to run process

[matta-aws]

What was the problem/requirement? (What/Why)

The Windows RunAs (aka Impersonation) and Notify features were being managed via powershell scripts.
a) this involved passing a username and password via command line.
b) it was specifcally using powershell 7 (pwsh.exe), which is not default on Windows.
c) the notify performance was slower

What was the solution? (How)

Remove this powershell layer and use Win apis to start and notify the process.

We use python subprocess.Popen for the non-impersonation case. For impersonation, we derive from Popen and run CreateProcessWithLogonW for our use case. Notify, for impersonation, is now a python script running win apis.

We still use one powershell script to set the env vars and run the command. Here we switch it back to running with powershell 5 rather than 7.

What is the impact of this change?

Performance of running and notifying commands on windows is improved. Also, Powershell 7 is no longer a requirement.

How was this change tested?

Unit tests. These were manually forced to run as another user to test the impersonation behavior.

Was this change documented?

No

Is this a breaking change?

No

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[Honglichenn]

the Non-Impersonation case is implemented here. We just use subprocess.Popen here.

May I ask if we still plan to use the CreateProcessWithLogonW for the impersonation, which we did the POC before? If so, I think may be CreateProcessA for the non-impersonation is a better choice here to keep the consistent.

In the official doc of CreateProcessA, it mentions:

If the calling process is impersonating another user, the new process uses the token for the calling process, not the impersonation token. To run the new process in the security context of the user represented by the impersonation token, use the CreateProcessAsUser or CreateProcessWithLogonW function.

So I believe that their implementations are similar. Once we can do non-impersonation by using the CreateProcessA, I think this will be the similar in impersonation, which will help us save times investigating how to fetch the return code, logging, signal and exceptions.

[roblutt]

Can we add the lpCurrentDirectory arg here and pass a working directory parameter from the Session module? Otherwise, the subprocess will fail to run because the impersonated user won't have permission on the current directory.

[matta-aws]

Updated this section. cwd is a Popen arg, and it is now passed along to the CreateProcessWithLogon call. We can look to use that.

[matta-aws]

the Non-Impersonation case is implemented here. We just use subprocess.Popen here.

May I ask if we still plan to use the CreateProcessWithLogonW for the impersonation, which we did the POC before? If so, I think may be CreateProcessA for the non-impersonation is a better choice here to keep the consistent.

In the official doc of CreateProcessA, it mentions:

If the calling process is impersonating another user, the new process uses the token for the calling process, not the impersonation token. To run the new process in the security context of the user represented by the impersonation token, use the CreateProcessAsUser or CreateProcessWithLogonW function.

So I believe that their implementations are similar. Once we can do non-impersonation by using the CreateProcessA, I think this will be the similar in impersonation, which will help us save times investigating how to fetch the return code, logging, signal and exceptions.

Offline, it was brought up that relying on the subprocess.Popen library to run the non-impersonation case was preferable to writing out own. And, as I ended up deriving from that class to run CreateProcessWithLogon for the impersonation case, it keeps the implementations similar and leverages existing Popen functionality.

[Honglichenn]

It looks good to me in general. Leave some minor comments.

[ddneilson]

Can you also modify the requirements in the README for the required powershell version -- https://github.com/OpenJobDescription/openjd-sessions-for-python?tab=readme-ov-file#compatibility

[ddneilson]

What's going on with the tests?! 8 (!!) retries to get the CI to pass?

Looks like two tests in particular:

FAILED test/openjd/sessions/test_runner_base.py::TestScriptRunnerBase::test_run_as_windows_user - RuntimeError: Could not create temp directory within C:\ProgramData\Amazon\OpenJD: [WinError 3] The system cannot find the path specified: 'C:\\ProgramData\\Amazon\\OpenJD\\tmp2vumgeza'
FAILED test/openjd/sessions/test_runner_base.py::TestScriptRunnerBase::test_does_not_inherit_env_vars_windows - RuntimeError: Could not create temp directory within C:\ProgramData\Amazon\OpenJD: [WinError 3] The system cannot find the path specified: 'C:\\ProgramData\\Amazon\\OpenJD\\tmpt6ad3c1_'

Is there a race condition there? It kind of looks like the TempDir() that gets created isn't available on the system right away for some reason. Any thoughts?

[ddneilson]

What's going on with the tests?! 8 (!!) retries to get the CI to pass?

Looks like two tests in particular:

FAILED test/openjd/sessions/test_runner_base.py::TestScriptRunnerBase::test_run_as_windows_user - RuntimeError: Could not create temp directory within C:\ProgramData\Amazon\OpenJD: [WinError 3] The system cannot find the path specified: 'C:\\ProgramData\\Amazon\\OpenJD\\tmp2vumgeza'
FAILED test/openjd/sessions/test_runner_base.py::TestScriptRunnerBase::test_does_not_inherit_env_vars_windows - RuntimeError: Could not create temp directory within C:\ProgramData\Amazon\OpenJD: [WinError 3] The system cannot find the path specified: 'C:\\ProgramData\\Amazon\\OpenJD\\tmpt6ad3c1_'

Is there a race condition there? It kind of looks like the TempDir() that gets created isn't available on the system right away for some reason. Any thoughts?

Interesting -- actions/runner-images#8755

It seems that the C: drive in GitHub actions is super bad performance. Maybe we should put in an env-var for tests that allows us to override where the Session directory is created, and create that on the D: drive in GitHub.

[matta-aws]

What's going on with the tests?! 8 (!!) retries to get the CI to pass?

Looks like two tests in particular:

FAILED test/openjd/sessions/test_runner_base.py::TestScriptRunnerBase::test_run_as_windows_user - RuntimeError: Could not create temp directory within C:\ProgramData\Amazon\OpenJD: [WinError 3] The system cannot find the path specified: 'C:\\ProgramData\\Amazon\\OpenJD\\tmp2vumgeza'
FAILED test/openjd/sessions/test_runner_base.py::TestScriptRunnerBase::test_does_not_inherit_env_vars_windows - RuntimeError: Could not create temp directory within C:\ProgramData\Amazon\OpenJD: [WinError 3] The system cannot find the path specified: 'C:\\ProgramData\\Amazon\\OpenJD\\tmpt6ad3c1_'

Is there a race condition there? It kind of looks like the TempDir() that gets created isn't available on the system right away for some reason. Any thoughts?

There is a race condition. "One of the tests creates the openjd directory and doesn’t clean it up. Other test will only succeeded if that test has already run first or if the directory already exists". We will follow up on this separately.

[matta-aws]

Interesting -- actions/runner-images#8755

It seems that the C: drive in GitHub actions is super bad performance. Maybe we should put in an env-var for tests that allows us to override where the Session directory is created, and create that on the D: drive in GitHub.

that could explain the longer test runs

[ddneilson]

There is a race condition. "One of the tests creates the openjd directory and doesn’t clean it up. Other test will only succeeded if that test has already run first or if the directory already exists". We will follow up on this separately.

Ahh. Okay, yeah, please fix this in a follow-on PR. Re-running the CIs is mildly blood-boiling :-)