PowerShell transcripts should include the configuration name in the transcript header #2890
Conversation
| string[] configurationArray = | ||
| senderInfo.ConfigurationName.Split(new char[] {System.IO.Path.DirectorySeparatorChar}, | ||
| StringSplitOptions.RemoveEmptyEntries); | ||
| psConfigurationName = configurationArray[configurationArray.Length - 1]; |
There was a problem hiding this comment.
This may be more efficient for extracting the configuration name from the Shell Uri since it only allocates one string instead of multiple string objects:
string shellPrefix = System.Management.Automation.Remoting.Client.WSManNativeApi.ResourceURIPrefix;
int index = shelluri.IndexOf(shellPrefix, StringComparison.OrdinalIgnoreCase);
return (index == 0) ? shelluri.Substring(shellPrefix.Length) : string.Empty;
``` #Resolved
| @@ -206,7 +206,7 @@ internal class ServerRemoteSession : RemoteSession | |||
| { | |||
| throw PSTraceSource.NewInvalidOperationException("RemotingErrorIdStrings.NonExistentInitialSessionStateProvider", configurationProviderId); | |||
| } | |||
|
|
|||
| senderInfo.ConfigurationName = configurationProviderId; | |||
There was a problem hiding this comment.
It would be better to parse the configuration name from the shellUri (configurationProcviderId) here rather than parse it each time later when a transcript header is written. #Resolved
| /// <summary> | ||
| /// Contains the configurationName from the sever remote session | ||
| /// </summary> | ||
| public string ConfigurationName |
There was a problem hiding this comment.
You should prefer auto-properties where possible. #Resolved
|
Thanks @lzybkr, @PaulHigin ! I have incorporated the suggested change. #Resolved |
| @@ -463,6 +467,7 @@ private void LogTranscriptHeader(System.Management.Automation.Remoting.PSSenderI | |||
| DateTime.Now, | |||
| username, | |||
| runAsUser, | |||
| psConfigurationName, | |||
There was a problem hiding this comment.
psConfigurationName [](start = 20, length = 19)
I believe that this cannot be null. Please make it default to empty string.
There was a problem hiding this comment.
@PaulHigin , I belive this is resolved.
|
@PaulHigin the request has been addressed. |
|
@chunqingchen Please include tests in this PR |
|
@chunqingchen You need to add yourself to Microsoft on GitHub |
|
@chunqingchen I suspect you're already in the orgs, you just need to do this (for both PowerShell and Microsoft, though doing Microsoft is what will fix the CLA bot): https://help.github.com/articles/publicizing-or-hiding-organization-membership/ |
|
There are open comments that still need to be addressed. See: #2890 (comment) |
|
We have been waiting on author response to PR comments for over a week.
|
|
working on the test now |
|
@chunqingchen Please don't dismiss (delete) other people reviews. If you believe someone review is bad enough to be dismissed, please talk to a maintainer. |
|
@TravisEz13 I meant the reviews are resolved. So dismiss is not the way to say it has been resolved. Got it. |
|
@PaulHigin Hi Paul, I talked with Travis and he agrees to merge the commit if you are fine. do you have any more comment about this change? |
| @@ -8,4 +8,26 @@ Describe "New-PSSession basic test" -Tag @("CI") { | |||
| $_.FullyQualifiedErrorId | Should Be "InvalidOperation,Microsoft.PowerShell.Commands.NewPSSessionCommand" | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| Describe "JEA session bug fix test" -Tag @("Feature", 'RequireAdminOnWindows') { | |||
There was a problem hiding this comment.
Please use more informative title.
Sample - "Transcript header".
There was a problem hiding this comment.
Yes, please replace bug fix test with something more descriptive.
| { | ||
| Unregister-PSSessionConfiguration -Name JEA -force -ErrorAction SilentlyContinue | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is recommended, but not required.
| try | ||
| { | ||
| New-PSSessionConfigurationFile -Path $PSSessionConfigFile -TranscriptDirectory $RoleCapDirectory -SessionType RestrictedRemoteServer | ||
| Register-PSSessionConfiguration -Name JEA -Path $PSSessionConfigFile -force -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
Please replace -force with -Force.
| Register-PSSessionConfiguration -Name JEA -Path $PSSessionConfigFile -force -ErrorAction SilentlyContinue | ||
| $scriptBlock = {Enter-PSSession -ComputerName Localhost -ConfigurationName JEA; Exit-PSSession} | ||
| & $scriptBlock | ||
| $headerFile = Get-ChildItem $transScriptFile | sort LastWriteTime | select -Last 1 |
There was a problem hiding this comment.
Please replace aliases with full cmdlet names.
| & $scriptBlock | ||
| $headerFile = Get-ChildItem $transScriptFile | sort LastWriteTime | select -Last 1 | ||
| $header = Get-Content $headerFile | Out-String | ||
| $header.Contains("Configuration Name: JEA") | Should Be $true |
There was a problem hiding this comment.
Please use Should BeLike or Should BeLikeExactly
| } | ||
| finally | ||
| { | ||
| Unregister-PSSessionConfiguration -Name JEA -force -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
Please replace -force with -Force.
|
I agree with @iSazonov comments. Otherwise LGTM. |
I agree with @iSazonov comments. Otherwise LGTM.
…uration name in the transcript header #2890
|
@iSazonov @TravisEz13 comments are all resolved |
|
Thanks! |
|
Great change! |
Steps to reproduce
Expected behavior
PowerShell transcripts currently do not log the configuration name the user used to connect to and manage the machine. For JEA scenarios, this means an auditor trying to understand how someone was able to do a certain command will not know through which endpoint the user entered and was assigned those privileges.
Suggestion is to add a new line to the transcript header similar to the following:
ConfigurationName: MyJEA
[…]
RunAs User: WinRM Virtual Users\WinRM VA_10_PRIV_priv.demo
Configuration Name: JEA
Machine: DC (Microsoft Windows NT 10.0.14393.0)
[…]
Actual behavior
[…]
RunAs User: WinRM Virtual Users\WinRM VA_10_PRIV_priv.demo
Machine: DC (Microsoft Windows NT 10.0.14393.0)
[…]
Environment data