Skip to content
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

Jump to bottom

PowerShell transcripts should include the configuration name in the transcript header #2890

Merged
merged 2 commits into from
Mar 16, 2017
Merged

PowerShell transcripts should include the configuration name in the transcript header #2890

merged 2 commits into from
Mar 16, 2017

Conversation

chunqingchen
Copy link
Contributor

@chunqingchen chunqingchen commented Dec 15, 2016 •
edited by joeyaiello
Loading

Steps to reproduce

New-PSSessionConfigurationFile -Path .\myJeaConfig.pssc -TranscriptDirectory 'C:\temp\transcripts' -SessionType RestrictedRemoteServer
Register-PSSessionConfiguration -Name JEA -Path .\myJeaconfig.pssc
Enter-PSSession -ComputerName Localhost -ConfigurationName JEA

exit

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

Name Value

PSVersion 5.1.14993.1000
PSEdition Desktop
PSCompatibleVersions {1.0, 2.0, 3.0, 4.0...}
BuildVersion 10.0.14993.1000
CLRVersion 4.0.30319.42000
WSManStackVersion 3.0
PSRemotingProtocolVersion 2.3
SerializationVersion 1.1.0.1

@chunqingchen chunqingchen changed the title PowerShell transcripts should include the configuration name in the ranscript header #2888 PowerShell transcripts should include the configuration name in the ranscript header Dec 15, 2016
@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Dec 15, 2016
PaulHigin
PaulHigin previously requested changes Dec 15, 2016
View reviewed changes
src/System.Management.Automation/engine/hostifaces/MshHostUserInterface.cs Outdated
string[] configurationArray =
senderInfo.ConfigurationName.Split(new char[] {System.IO.Path.DirectorySeparatorChar},
StringSplitOptions.RemoveEmptyEntries);
psConfigurationName = configurationArray[configurationArray.Length - 1];
Copy link
Contributor

@PaulHigin PaulHigin Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

src/System.Management.Automation/engine/remoting/server/serverremotesession.cs Outdated
@@ -206,7 +206,7 @@ internal class ServerRemoteSession : RemoteSession
{
throw PSTraceSource.NewInvalidOperationException("RemotingErrorIdStrings.NonExistentInitialSessionStateProvider", configurationProviderId);
}

senderInfo.ConfigurationName = configurationProviderId;
Copy link
Contributor

@PaulHigin PaulHigin Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

lzybkr
lzybkr reviewed Dec 15, 2016
View reviewed changes
src/System.Management.Automation/engine/remoting/fanin/PSPrincipal.cs Outdated
/// <summary>
/// Contains the configurationName from the sever remote session
/// </summary>
public string ConfigurationName
Copy link
Member

@lzybkr lzybkr Dec 15, 2016 •
edited by PaulHigin
Loading

Choose a reason for hiding this comment

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

You should prefer auto-properties where possible. #Resolved

@lzybkr lzybkr changed the title PowerShell transcripts should include the configuration name in the ranscript header PowerShell transcripts should include the configuration name in the transcript header Dec 15, 2016
@chunqingchen
Copy link
Contributor Author

chunqingchen commented Dec 17, 2016 •
edited by PaulHigin
Loading

Thanks @lzybkr, @PaulHigin ! I have incorporated the suggested change. #Resolved

PaulHigin
PaulHigin reviewed Jan 3, 2017
View reviewed changes
src/System.Management.Automation/engine/hostifaces/MshHostUserInterface.cs
@@ -463,6 +467,7 @@ private void LogTranscriptHeader(System.Management.Automation.Remoting.PSSenderI
DateTime.Now,
username,
runAsUser,
psConfigurationName,
Copy link
Contributor

Choose a reason for hiding this comment

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

psConfigurationName [](start = 20, length = 19)

I believe that this cannot be null. Please make it default to empty string.

Copy link
Member

Choose a reason for hiding this comment

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

@PaulHigin , I belive this is resolved.

@chunqingchen
Copy link
Contributor Author

@PaulHigin the request has been addressed.

@mirichmo
Copy link
Member

@chunqingchen Please include tests in this PR

SteveL-MSFT
SteveL-MSFT previously requested changes Jan 25, 2017
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

test needs to be added

@mirichmo
Copy link
Member

@chunqingchen You need to add yourself to Microsoft on GitHub

@mirichmo mirichmo added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Feb 17, 2017
@joeyaiello
Copy link
Contributor

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

@TravisEz13 TravisEz13 closed this Feb 23, 2017
@TravisEz13 TravisEz13 reopened this Feb 23, 2017
@TravisEz13 TravisEz13 closed this Feb 23, 2017
@TravisEz13 TravisEz13 reopened this Feb 23, 2017
@TravisEz13
Copy link
Member

There are open comments that still need to be addressed. See: #2890 (comment)

@TravisEz13
Copy link
Member

TravisEz13 commented Mar 6, 2017 •
edited
Loading

We have been waiting on author response to PR comments for over a week.

There are open comments that still need to be addressed. See: #2890 (comment)

@chunqingchen
Copy link
Contributor Author

working on the test now

@chunqingchen chunqingchen dismissed stale reviews from PaulHigin and SteveL-MSFT March 9, 2017 22:30

resolved

@TravisEz13
Copy link
Member

@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
TravisEz13 previously approved these changes Mar 13, 2017
View reviewed changes
@chunqingchen
Copy link
Contributor Author

@TravisEz13 I meant the reviews are resolved. So dismiss is not the way to say it has been resolved. Got it.

@chunqingchen
Copy link
Contributor Author

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

iSazonov
iSazonov reviewed Mar 15, 2017
View reviewed changes
test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
@@ -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') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use more informative title.
Sample - "Transcript header".

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please replace bug fix test with something more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
{
Unregister-PSSessionConfiguration -Name JEA -force -ErrorAction SilentlyContinue
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add new line.

Copy link
Member

Choose a reason for hiding this comment

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

This is recommended, but not required.

Copy link
Member

Choose a reason for hiding this comment

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

This was not fixed

test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
try
{
New-PSSessionConfigurationFile -Path $PSSessionConfigFile -TranscriptDirectory $RoleCapDirectory -SessionType RestrictedRemoteServer
Register-PSSessionConfiguration -Name JEA -Path $PSSessionConfigFile -force -ErrorAction SilentlyContinue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace -force with -Force.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace aliases with full cmdlet names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
& $scriptBlock
$headerFile = Get-ChildItem $transScriptFile | sort LastWriteTime | select -Last 1
$header = Get-Content $headerFile | Out-String
$header.Contains("Configuration Name: JEA") | Should Be $true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use Should BeLike or Should BeLikeExactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
}
finally
{
Unregister-PSSessionConfiguration -Name JEA -force -ErrorAction SilentlyContinue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace -force with -Force.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

@PaulHigin
Copy link
Contributor

I agree with @iSazonov comments. Otherwise LGTM.

@TravisEz13 TravisEz13 dismissed their stale review March 15, 2017 19:05

I agree with @iSazonov comments. Otherwise LGTM.

TravisEz13
TravisEz13 requested changes Mar 15, 2017
View reviewed changes
Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

I agree with @iSazonov comments. Otherwise LGTM.

@chunqingchen
Copy link
Contributor Author

@iSazonov @TravisEz13 comments are all resolved

@iSazonov
Copy link
Collaborator

Thanks!
Tests LGTM.

@TravisEz13 TravisEz13 merged commit 3f274aa into PowerShell:master Mar 16, 2017
@jpsnover
Copy link
Contributor

jpsnover commented Apr 9, 2017

Great change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzybkr lzybkr lzybkr left review comments

@PaulHigin PaulHigin PaulHigin left review comments

@iSazonov iSazonov iSazonov left review comments

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@SteveL-MSFT SteveL-MSFT SteveL-MSFT left review comments

Assignees

@TravisEz13 TravisEz13

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.