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
Support OpenSSH options for PSRP over SSH commands #12802
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me, but can you also update the Enter-PSSession command? SSH remoting is supported by Invoke-Command, New-PSSession, and Enter-PSSession.
https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/remoting/commands/PushRunspaceCommand.cs#L51
f5402fe
to
07d61eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@BrannenGH Are you ready to remove WIP? |
@PaulHigin @iSazonov Thanks for reviewing this so quickly! WIP is removed. I'll take a look at the docs later this week. Let me know if you need anything else. |
{ | ||
foreach (DictionaryEntry pair in this.Options) | ||
{ | ||
startInfo.ArgumentList.Add(string.Format(CultureInfo.InvariantCulture, @"-o {0}={1}", pair.Key, pair.Value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we get this through public API should we make additional validations?
I mean that Key
and Value
could contain composite values.
/cc @PaulHigin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, right now the Format method will just try to convert whatever is passed in to a string, would we want to handle composite values any other way?
I also could try to add some validation to ensure that we're only passing options that OpenSSH would consider valid, but that would be a lot to maintain as well. Please let me know your thoughts.
It "Verifies explicit Options parameter" { | ||
$options = @{"Port"="22"} | ||
$script:session = New-PSSession -HostName localhost -Options $options -ErrorVariable err | ||
$err | Should -HaveCount 0 | ||
VerifySession $script:session | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test checks that the parameter is only recognized, but does not verify that it is being applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can definitely add more integration tests, it will take me a few days to get those done though.
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
* Options parameter added to Invoke-Command * Options parameter added to Start-PSSession * Options parameter added to SSHConnection Hashtable
07d61eb
to
2a2d149
Compare
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
/rebase |
@PaulHigin Is it ready to merge? |
@iSazonov Yes, I feel this is ready to merge. |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
/rebase |
@BrannenGH Thanks for your contribution! |
🎉 Handy links: |
PR Summary
Fixes #12762
Add an
-Options
parameter toInvoke-Command
andStart-PSSession
to pass SSH optionsPR Context
PSRP over SSH uses OpenSSH to connect to remote machines. It would be helpful if we could pass OpenSSH options to cmdlets that support PSRP over SSH.
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.