Skip to content

Constrained Runspace Support #1507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

dkattan
Copy link
Contributor

@dkattan dkattan commented Jun 23, 2021

@andschwa I've got this boiled down with as few changes as possible, could I have you take a look?

@dkattan dkattan requested a review from rjmholt as a code owner June 23, 2021 16:18
@dkattan dkattan marked this pull request as draft June 23, 2021 18:03
@dkattan dkattan marked this pull request as ready for review June 23, 2021 22:07
@dkattan dkattan marked this pull request as draft June 24, 2021 10:21
@dkattan
Copy link
Contributor Author

dkattan commented Jun 24, 2021

I'm working on the test failures

@andyleejordan
Copy link
Member

Hey @dkattan, sorry not to have read through this yet, it's on my radar. Since I've taken over maintainership of PSES from @TylerLeonhardt, and this work was started before I joined, I need more context. I'd also be happy to have a call with you.

I've read through #1282, but could you summarize/explain the full context of what drove this work, what it solves, how you've gone about solving it, as well as the potential risks of these changes with respect to the extension and other users of editor services? I would really like to see this get in, as I know you've been working hard on it and so there are obviously good reasons for the work, I just want to understand those reasons myself.

@dkattan
Copy link
Contributor Author

dkattan commented Jun 29, 2021

Hey @dkattan, sorry not to have read through this yet, it's on my radar. Since I've taken over maintainership of PSES from @TylerLeonhardt, and this work was started before I joined, I need more context. I'd also be happy to have a call with you.

I've read through #1282, but could you summarize/explain the full context of what drove this work, what it solves, how you've gone about solving it, as well as the potential risks of these changes with respect to the extension and other users of editor services? I would really like to see this get in, as I know you've been working hard on it and so there are obviously good reasons for the work, I just want to understand those reasons myself.

Absolutely! I'll start by defining the problem.

I have a .net core web application that allows users to write PowerShell that executes in the backend. Our backend exposes various cmdlets that are implemented in C# that have complex parameter sets. To protect against users unintentionally, or intentionally breaking the backend by overwriting files, we run the PowerShell in a Constrained Runspace, in Constrained Language Mode.

A Constrained Runspace is a runspace created with the InitialSessionState.Create() method (versus CreateDefault() or CreateDefault2()). The .Create() method creates an initialSessionState with nothing preloaded at all. From there I explicitly add the cmdlets, aliases, modules, functions, providers, etc that I want the user to have access to. CreateDefault() and CreateDefault2() will automatically load the FileSystem provider, so I am forced to create the runspace in this way if I don't want that provider loaded automatically.

Where this gets tricky is ensuring that the PSES has access to the cmdlets and modules it requires, which is actually a pretty manageable list:

Cmdlets

  • Get-Command
  • Get-Help
  • Get-Module
  • tabexpansion2
  • prompt

Modules

  • PSReadLine
  • PSScriptAnalyzer
  • and of course PowerShellEditorServices

The Cmdlets were easy enough, the only complication is that we reference them by their fully qualified name which I worked around by adding an alias to the shortname that I loaded into the constrained runspace.

The modules were slightly trickier since they were being loaded by Import-Module -Name $Path after the runspace was already open. Even if I added Import-Module to the runspace, this approach assumes a FileSystem provider will be present. @TylerLeonhardt had commented that he preferred doing it like this as the logging is better, which is true. The only way to load file based modules into a ConstrainedRunspace is via InitialSessionState.ImportPSModulesFromPath which returns void and throws no exceptions on failure. To work around that, I am now checking if the module path exists and verify the module is present in the initialSessionState object after calling ImportPSModulesFromPath, and logging as appropriate.

A recent complication I had to overcome was the structure of the PowerShellEditorServices.Commands sub-module. Traditionally .psd1 files should have the same filename as their parent folder, and ImportPSModulesFromPath only takes folder paths, and will only load modules who have a .psd1 that match the parent folder name.

However, our module violates this since the path is module\PowerShellEditorServices\Commands\PowerShellEditorServices.Commands.psd1

So as of yesterday I am making a copy of PowerShellEditorServices.Commands.psd1 and naming it Commands.psd1 and giving ImportPSModulesFromPath(".module\PowerShellEditorServices\Commands") and it loads successfully.

The complications with Serilog are really a side-effect of me running Start-EditorServices.ps1 from a process that has overlapping dependencies. I'm able to work around that now with no code changes. I simply delete the extra .dll files that you guys are opportunistically loading. I could submit a merge request to check if it already exists in the loaded assemblies, but baby steps.

The final and probably most risky change is ensuring that the other runspaces PSES may create, are created from the same initialSessionState object as the original runspace. To accomplish this, I simply pass the entire initialSessionState object in place of the LanguageMode property that was previously being passed. The initialSessionState object is where the language mode was coming from in the first place, and was down the line being used to create, you guessed it, a new initialSessionState object which was then being used to create out of process runspaces. Since these new runspaces were created using CreateDefault() This was causing PSES to leak FileSystem details to my ConstrainedRunspace. Passing along the initialSessionState from the calling runspace solved this problem.

The meat of my change is only triggered when the FileSystem provider is absent, so I don't anticipate it affecting much outside of that circumstance.

Now I just need to figure out what I broke that is causing the tests to fail in the pipeline. Those tests pass when run locally.

@dkattan
Copy link
Contributor Author

dkattan commented Jun 29, 2021

Also, @andschwa I'd love to jump on a call. I don't know if you've got time today, but I emailed you a Teams link just in case.

@@ -349,6 +349,8 @@ private EditorServicesConfig CreateConfigObject()
var profile = (PSObject)GetVariableValue("profile");

var hostInfo = new HostInfo(HostName, HostProfileId, HostVersion);
var iss = Runspace.DefaultRunspace.InitialSessionState;
iss.LanguageMode = Runspace.DefaultRunspace.SessionStateProxy.LanguageMode;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixed the broken tests @rjmholt @andschwa
Presumably the tests were changing the languagemode after the runspace was created so I set the LanguageMode to the LanguageMode on the SessionStateProxy (which is where it was coming from originally) and normal functionality was restored.

@dkattan dkattan force-pushed the feature/constrained-runspace-support-2021 branch from 2685a37 to 37d1fe9 Compare August 5, 2021 17:12
@dkattan dkattan force-pushed the feature/constrained-runspace-support-2021 branch from 37d1fe9 to f353d72 Compare August 5, 2021 17:31
@dkattan
Copy link
Contributor Author

dkattan commented Nov 5, 2021

I was able to get everything working without changing the codebase with the changes in 3.0 closing this PR.

@dkattan dkattan closed this Nov 5, 2021
@dkattan dkattan deleted the feature/constrained-runspace-support-2021 branch November 5, 2021 02:06
@andyleejordan
Copy link
Member

@dkattan Does that mean PSES is fully working in constrained runspaces now?

@dkattan
Copy link
Contributor Author

dkattan commented Nov 5, 2021

@dkattan Does that mean PSES is fully working in constrained runspaces now?

There is a lot I have to do to my constrained runspace to make PSES work, it doesn't just work out of the box.

If I'm being honest, now that I'm looking at it with fresh eyes from the time I originally submitted this, I think I could have made it work all along, but the idea was to put the onus on PSES to do that instead of the calling code.

@andyleejordan
Copy link
Member

Would love to know what all you're still having to do, as I want to support constrained language mode at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants