GeezerVison

With this change, the Windows PowerShell GPO settings don't apply to PSCore6, correct? This seems correct to me, but is a breaking change. We should label this PR as such. cc Joey Aiello (@joeyaiello)

Yes, GPO settings in Registry won't apply to PowerShell Core. I have added the breaking change label. Thanks for catching that.

This does not seem correct to me. Indhu Sivaramakrishnan (@Indhukrishna) DSC has had feedback that seems to indicate that they expect GPO to apply.

I will start to work on enabling GPO as well on windows. The preference order of the settings should be config file --> GPO, meaning if a policy setting is defined in the config file, then use it; if it's not defined in the config file, query registry. This way, you can have control whether you want the GPO settings to take effect.

Common rule is that Computer GPO is high priority, then User GPO, then computer wide config, then user config. It is critical rule and security related.

Update.
Software\Policies\Microsoft\Windows\PowerShell - here Policies assume that it is high priority and should overlap other configs - users can not change its.
Software\Microsoft\Windows\PowerShell - it is a normal config corresponding to a text config file. This setting can be set manually by user or by using Group Policy Preferences (GPP). I think it should be low priority over a text config file (looks as changing defaults or set recommended defaults that the user can change by a config file).

For the policy related settings, we are having a debate on which one should take preference. I will talk to Travis Plunk (@TravisEz13) later today about it.
For non-policy related settings, we shouldn't use registry settings anymore but only refer to the config file.

The conclusion is: on windows, we query group policies from registry first, and if the requested policy is not defined in GP, then we query from the configuration file.
New commit has been pushed to enable this.

For non-policy related settings, we shouldn't use registry settings anymore but only refer to the config file.

I don't agree - GPO is great Enterprise feature and Domain Admins should have full and flexible control on PowerShell. We could discuss this in a Issue (as I mention below too).

I mean "non-policy related settings", like the settings in "SOFTWARE\Microsoft\PowerShell" today. They are not group policies.

If we're moving GPO support, we shouldn't mention "original Group Policies", just call it "PowerShell Policies"

Since we now re-enabled the GPO support on Windows, I will leave this comment unchanged.

Correct

this path should be a const somewhere

This code is in a #if !CORECLR ... #endif block. I leave the registry key path here only for reference, so that when we clean up this !CORECLR block, we know what the key is.

ok

Why isn't the IsProductCode check in the outer if statement since it's never logged when set.

Theoretically, this can be re-written as

if ((force && logSetting?.EnableScriptBlockLogging != false) || logSetting?.EnableScriptBlockLogging == true)
{
    if ((!scriptBlock.HasLogged || InternalTestHooks.ForceScriptBlockLogging) && !scriptBlock.ScriptBlockData.IsProductCode)
    {
        ....
    }
}

Or even more aggressively, these two if blocks can be merged into one. But this doesn't look very readable, so I think I'm fine with the original code.

This behavior change may be confusing without some documentation.
We've essentially now have 3 logging states, on, off, and warnings. When the setting is not defined, scripts that have suspicious content are logged.

I suggest a doc change to clarify this.

There is no behavior change, the check logSetting?.EnableScriptBlockLogging == false is to make sure that the EnableScriptBlockLogging is explicitly set to false in the config file. If you look at the original implementation of ScriptBlockLoggingExplicitlyDisabled(), it returns true only if the registry key EnableScriptBlockLogging exists and its value is explicitly 0.

I agree to have docs to clarify how logging works in PowerShell. Not sure if we already have some docs about it.

Dongbo Wang (@daxian-dbw) Could you please add this in PR description for future docs?

Yes, a documentation section is added to PR description.

Do we really need multiple return paths? Why not if (... == false) else if (... == true) and set status in both cases.

I didn't change the original code, will update it to set status in both cases.

This lock is not used consistently with other usages. if the intent is to return the updated or original value, the lock should also encapsulate both the "if (transcription != null)" block and the implied else block.

As written, there is a possible race between this path and StopAllTranscribing

I didn't change the original code. I guess you are right, this method could return a TranscriptionOption object or null depending on whether StopAllTranscribing set systemTranscript to null before return systemTranscript; in this method. But I'm not familiar enough with the transcribing code to know if that could be a problem.

Can you open an issue tracking this comment? We can look at the transcribing code more closely when addressing that issue.

#5799

Not sure I understand the value of having this nullable. EnableScripts == null is same as EnableScripts == true, correct? Same question for the nullable properties below where they should have a default value and being null is the same as the default value.

EnableScripts == null means this setting is not defined in the config file, so it's up to powershell to define the default behavior. EnableScripts == true means it's defined in the config file and the value is true. Likewise, EnableScripts == false means it's defined in the config file and the value is false.

It's the same in registry -- a key is not defined, which means the application needs to use the pre-defined behavior; when the key is defined, the value will be either 1 or 0.

An example is the use of EnableScriptBlockLogging in LogScriptBlockCreation(ScriptBlock scriptBlock, bool force)

• when it's explicitly set to 'true' (1 in registry), we always do script block logging;
• when it's not defined, we do script block logging when force == true
• when it's explicitly set to 'false' (0 in registry), we always disable script block logging, even if force == true.

Ok, the force case makes sense that there is a difference with null. Thanks.

I don't like the nullable too. I see no reason to emulate registry keys. If now we use static classes it is more readable and maintainable to set defaults explicitly. Force works with this too.

I don't think it's mimicking registry keys, but a requirement to represent three states: enabled, disabled and undefined. Without bool?, if EnableScriptBlockLogging is not specified in the config file while EnableScriptBlockInvocationLogging is specified, then the deserialized ScriptBlockLogging object will have EnableScriptBlockLogging set to false (the default value), which will be interpreted as "script block logging" is explicitly disabled.

The EnableScriptBlockLogging example is from existing code, I didn't change the logic. If it turns out we don't need the bool? type in future, we can always change it since it's internal.

We can install PowerShell Core side-by-side with other PowerShell Core-s. So we should have configs for "default" system wide installation and for "local" for every user installation.
Also it seems we should have GPO option - "Force GPO for Side-By-Side PowerShell installations" - by default we should apply GPO only for system wide PowerShell installation.

Based on internal discussion today, the current plan is to have PSCore6 GPO settings separate from Windows PowerShell. For now, we don't plan to have side-by-side GPO settings per version of PSCore6 until there is customer need for it. New admx will be published later for the PSCore6 settings (which will be the same as Windows PowerShell, but reside in a different location in registry).

GPO is Enterprise feature. GPO should allow Domain Admins get full control on PowerShell. I full trust MSFT experts but I'd prefer discuss GPO in a Issue. Also we forget about AppLocker.
I'd as Domain Admin prefer don't separate PowerShell Core GPO - it is better by default use Windows PowerShell setting until PowerShell Core GPOs is set explicitly otherwise users can ignore corporate policies.

Ilya (@iSazonov) since PSCore6 is not inbox in Windows and doesn't replace Windows PowerShell, we shouldn't use the same settings. PSCore6 in this case is no different from any 3rd party app.

We have lock (s_cachedPolicies) in Utils.cs - maybe that's enough?

There are other settings except for the policy related ones and they may query the config file simultaneously, so this lock is needed.

I think we could find a way to use only one global lock - if we want to have consistency, it doesn't matter how and what settings change.

The querying for GP setting is a hot code path, especially for the script block logging, every script block execution will query for it. The lock s_cachedPolicies is to make sure that when multiple threads are querying for GP settings at the same time, only one thread will actually hit and read the configuration file.

#5799

Shouldn't this be private?

Yes, it should. Will update.

This should be private, right?

Yes, it should. Will update.

about_Logging.md will need to be updated.
See https://github.com/PowerShell/PowerShell-Docs/blob/staging/reference/6/Microsoft.PowerShell.Core/About/about_Logging.md

Opened MicrosoftDocs/PowerShell-Docs#2017 to track the documentation update.

Note that about_logging.md needs to be updated regardless since it refers to 'PowerShellProperties.json'.

Really, we should register for GP change notifications and clear the cache. Not a regression, so, can you add a comment or file an issue: https://msdn.microsoft.com/en-us/library/windows/desktop/aa374404(v=vs.85).aspx

Issue #5804 is opened to track this.

Needs to be updated as we're putting it under ...\Microsoft\PowerShellCore now

Will do. Done.

Perhaps have the same assert message as line 656 below?

Done.