Skip to content

Make PowerShell able to enable logging of script block execution on Unix platforms #5791

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.

Sign up for GitHub

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

Merged
daxian-dbw merged 13 commits into from
Jan 8, 2018
Merged

Make PowerShell able to enable logging of script block execution on Unix platforms #5791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
39af334
Enable logging on Linux through MshLog
daxian-dbw Dec 19, 2017
3dd86f5
Refactor 'PropertyAccessor.cs'
daxian-dbw Dec 21, 2017
263ec24
Rename 'PropertyScope' to 'ConfigScope'
daxian-dbw Dec 21, 2017
6ab1ad7
Rename file PropertyAccessor.cs to PSConfiguration.cs
daxian-dbw Dec 21, 2017
41c66db
Refactor GroupPolicy setting related code to use the configuration file
daxian-dbw Jan 3, 2018
117e83a
Add xUnit tests for reading Policy settings from config file
daxian-dbw Jan 3, 2018
a6c62bf
[Feature] Minor changes to make ScriptBlock logging work with SysLog
daxian-dbw Jan 4, 2018
167bbad
Address one of Dan's comments
daxian-dbw Jan 5, 2018
b4b864f
Re-enable group policy settings from registry
daxian-dbw Jan 5, 2018
3b5badc
Rename 'PowerShellProperties.json' to 'powershell.config.json'
daxian-dbw Jan 6, 2018
3b94173
[Feature] Change two methods to use 'private' modifier.
daxian-dbw Jan 6, 2018
6ee608f
[Feature] Consider a policy is undefined if none of its properties ar…
daxian-dbw Jan 7, 2018
01ecf4b
[Feature] Address Steve's new comments
daxian-dbw Jan 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions build.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,7 +1364,8 @@ function Test-PSPesterResults

function Start-PSxUnit {
[CmdletBinding()]param(
[string] $TestResultsFile = "XUnitResults.xml"
[string] $SequentialTestResultsFile = "SequentialXUnitResults.xml",
[string] $ParallelTestResultsFile = "ParallelXUnitResults.xml"
)

# Add .NET CLI tools to PATH
Expand All @@ -1375,9 +1376,11 @@ function Start-PSxUnit {
throw "PowerShell must be built before running tests!"
}

if(Test-Path $TestResultsFile)
{
Remove-Item $TestResultsFile -Force -ErrorAction SilentlyContinue
if (Test-Path $SequentialTestResultsFile) {
Remove-Item $SequentialTestResultsFile -Force -ErrorAction SilentlyContinue
}
if (Test-Path $ParallelTestResultsFile) {
Remove-Item $ParallelTestResultsFile -Force -ErrorAction SilentlyContinue
}

try {
Expand All @@ -1386,12 +1389,7 @@ function Start-PSxUnit {
# Path manipulation to obtain test project output directory
dotnet restore

# --fx-version workaround required due to https://github.com/dotnet/cli/issues/7901#issuecomment-343323674
if($Environment.IsWindows)
{
dotnet xunit --fx-version 2.0.0 -xml $TestResultsFile
}
else
if(-not $Environment.IsWindows)
{
if($Environment.IsMacOS)
{
Expand Down Expand Up @@ -1419,9 +1417,12 @@ function Start-PSxUnit {
{
throw "Dependencies $requiredDependencies not met."
}

dotnet xunit --fx-version 2.0.0 -configuration $Options.configuration -xml $TestResultsFile
}

# '-fxversion' workaround required due to https://github.com/dotnet/cli/issues/7901#issuecomment-343323674
# Run sequential tests first, and then run the tests that can execute in parallel
dotnet xunit -fxversion 2.0.0 -configuration $Options.configuration -xml $SequentialTestResultsFile -namespace "PSTests.Sequential" -parallel none
dotnet xunit -fxversion 2.0.0 -configuration $Options.configuration -xml $ParallelTestResultsFile -namespace "PSTests.Parallel" -nobuild
}
finally {
Pop-Location
Expand Down
23 changes: 6 additions & 17 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.IO;
using System.Collections.ObjectModel;
using System.Management.Automation;
using System.Management.Automation.Configuration;
using System.Management.Automation.Runspaces;
using System.Management.Automation.Internal;
using System.Diagnostics;
Expand Down Expand Up @@ -459,29 +460,17 @@ internal void Parse(string[] args)
}
}

private static string s_groupPolicyBase = @"Software\Policies\Microsoft\Windows\PowerShell";
private static string s_consoleSessionConfigurationKey = "ConsoleSessionConfiguration";
private static string s_enableConsoleSessionConfiguration = "EnableConsoleSessionConfiguration";
private static string s_consoleSessionConfigurationName = "ConsoleSessionConfigurationName";
private static string GetConfigurationNameFromGroupPolicy()
{
// Current user policy takes precedence.
var groupPolicySettings = Utils.GetGroupPolicySetting(s_groupPolicyBase, s_consoleSessionConfigurationKey, Utils.RegCurrentUserThenLocalMachine);
if (groupPolicySettings != null)
var consoleSessionSetting = Utils.GetPolicySetting<ConsoleSessionConfiguration>(Utils.CurrentUserThenSystemWideConfig);
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jan 4, 2018

Choose a reason for hiding this comment

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

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 @joeyaiello

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 4, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@TravisEz13 TravisEz13 Jan 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@iSazonov iSazonov Jan 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 5, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 5, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@iSazonov iSazonov Jan 6, 2018

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (consoleSessionSetting != null)
{
object keyValue;
if (groupPolicySettings.TryGetValue(s_enableConsoleSessionConfiguration, out keyValue))
if (consoleSessionSetting.EnableConsoleSessionConfiguration == true)
{
if (String.Equals(keyValue.ToString(), "1", StringComparison.OrdinalIgnoreCase))
if (!string.IsNullOrEmpty(consoleSessionSetting.ConsoleSessionConfigurationName))
{
if (groupPolicySettings.TryGetValue(s_consoleSessionConfigurationName, out keyValue))
{
string consoleSessionConfigurationName = keyValue.ToString();
if (!string.IsNullOrEmpty(consoleSessionConfigurationName))
{
return consoleSessionConfigurationName;
}
}
return consoleSessionSetting.ConsoleSessionConfigurationName;
}
}
}
Expand Down
53 changes: 0 additions & 53 deletions src/System.Management.Automation/engine/CommandDiscovery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1643,59 +1643,6 @@ private int GetCmdletRemovalIndex(List<CmdletInfo> cacheEntry, string PSSnapin)

internal ExecutionContext Context { get; }

/// <summary>
/// Reads the path for the appropriate shellID from the registry.
Copy link
Collaborator

@iSazonov iSazonov Jan 5, 2018

Choose a reason for hiding this comment

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

Maybe leave this? How Windows user or Domain Admins can system-wide switch from Windows PowerShell to PowerShell Core?

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 5, 2018

Choose a reason for hiding this comment

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

I don't understand your question. This method is removed because it's not used anywhere.

Copy link
Collaborator

@iSazonov iSazonov Jan 6, 2018

Choose a reason for hiding this comment

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

My thoughts was about side-by-side scenario but it seems not help.
Closed.

/// </summary>
///
/// <param name="shellID">
/// The ID of the shell to retrieve the path for.
/// </param>
///
/// <returns>
/// The path to the shell represented by the shellID.
/// </returns>
///
/// <remarks>
/// The shellID must be registered in the Windows Registry in either
/// the HKEY_CURRENT_USER or HKEY_LOCAL_MACHINE hive under
/// Software/Microsoft/MSH/&lt;ShellID&gt; and are searched in that order.
/// </remarks>
///
internal static string GetShellPathFromRegistry(string shellID)
{
string result = null;

#if !UNIX
try
{
RegistryKey shellKey = Registry.LocalMachine.OpenSubKey(Utils.GetRegistryConfigurationPath(shellID));
if (shellKey != null)
{
// verify the value kind as a string
RegistryValueKind kind = shellKey.GetValueKind("path");

if (kind == RegistryValueKind.ExpandString ||
kind == RegistryValueKind.String)
{
result = shellKey.GetValue("path") as string;
}
}
}
// Ignore these exceptions and return an empty or null result
catch (SecurityException)
{
}
catch (IOException)
{
}
catch (ArgumentException)
{
}
#endif

return result;
}

internal static PSModuleAutoLoadingPreference GetCommandDiscoveryPreference(ExecutionContext context, VariablePath variablePath, string environmentVariable)
{
Dbg.Assert(context != null, "context cannot be Null");
Expand Down
29 changes: 10 additions & 19 deletions src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.IO;
using System.Linq;
using System.Management.Automation;
using System.Management.Automation.Configuration;
using System.Management.Automation.Internal;
using System.Management.Automation.Runspaces;
using System.Management.Automation.Security;
Expand Down Expand Up @@ -4749,28 +4750,18 @@ internal static ModuleLoggingGroupPolicyStatus GetModuleLoggingInformation(out I
{
moduleNames = null;
ModuleLoggingGroupPolicyStatus status = ModuleLoggingGroupPolicyStatus.Undefined;
Dictionary<string, object> groupPolicySettings = Utils.GetGroupPolicySetting("ModuleLogging", Utils.RegLocalMachineThenCurrentUser);

if (groupPolicySettings != null)
var moduleLogging = Utils.GetPolicySetting<ModuleLogging>(Utils.SystemWideThenCurrentUserConfig);
if (moduleLogging != null)
{
object enableModuleLoggingValue = null;
if (groupPolicySettings.TryGetValue("EnableModuleLogging", out enableModuleLoggingValue))
if (moduleLogging.EnableModuleLogging == false)
Copy link
Contributor

@dantraMSFT dantraMSFT Jan 4, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 5, 2018

Choose a reason for hiding this comment

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

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

{
if (String.Equals(enableModuleLoggingValue.ToString(), "0", StringComparison.OrdinalIgnoreCase))
{
return ModuleLoggingGroupPolicyStatus.Disabled;
}

if (String.Equals(enableModuleLoggingValue.ToString(), "1", StringComparison.OrdinalIgnoreCase))
{
status = ModuleLoggingGroupPolicyStatus.Enabled;

object moduleNamesValue = null;
if (groupPolicySettings.TryGetValue("ModuleNames", out moduleNamesValue))
{
moduleNames = new List<String>((string[])moduleNamesValue);
}
}
status = ModuleLoggingGroupPolicyStatus.Disabled;
}
else if (moduleLogging.EnableModuleLogging == true)
{
status = ModuleLoggingGroupPolicyStatus.Enabled;
moduleNames = moduleLogging.ModuleNames;
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Collections.Generic;
using System.IO;
using System.Management.Automation.Configuration;
using System.Management.Automation.Internal;
using System.Management.Automation.Language;
using Microsoft.PowerShell.Commands;
Expand Down Expand Up @@ -977,8 +978,8 @@ internal static string GetModulePath()
internal static string SetModulePath()
{
string currentModulePath = GetExpandedEnvironmentVariable(Constants.PSModulePathEnvVar, EnvironmentVariableTarget.Process);
string systemWideModulePath = ConfigPropertyAccessor.Instance.GetModulePath(ConfigPropertyAccessor.PropertyScope.SystemWide);
string personalModulePath = ConfigPropertyAccessor.Instance.GetModulePath(ConfigPropertyAccessor.PropertyScope.CurrentUser);
string systemWideModulePath = PowerShellConfig.Instance.GetModulePath(ConfigScope.SystemWide);
string personalModulePath = PowerShellConfig.Instance.GetModulePath(ConfigScope.CurrentUser);

string newModulePathString = GetModulePath(currentModulePath, systemWideModulePath, personalModulePath);

Expand Down
Loading