-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Fix SSH hostname parsing to be case sensitive and refactor remoting methods to static class #20968
base: master
Are you sure you want to change the base?
Fix SSH hostname parsing to be case sensitive and refactor remoting methods to static class #20968
Conversation
Will add tests for this |
src/System.Management.Automation/engine/remoting/commands/PSRemotingCmdlet.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/PSRemotingCmdlet.cs
Outdated
Show resolved
Hide resolved
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 think this is safe to include, I'm unsure if we need to worry about non-ASCII characters in hostnames and whether a lower cased length might be different than an upper case one. I honestly can't think of a situation where that might be the case and whether it is valid for a hostname but still leaving it here in case others can think of it.
Thanks @jborean93 @iSazonov for review. I didn't get a chance to add tests for this. It seems hard to do with I can also just do pester test if you guys see an easy way to test a change like this 🙂. |
I don't know if the test environment allows such tests. /cc @SteveL-MSFT Who's replacing Paul? |
If doing a test I would say this is perfect for a unit test and to make this a static method somewhere. |
src/System.Management.Automation/engine/remoting/common/RemotingUtils.cs
Outdated
Show resolved
Hide resolved
Thanks @jborean93. I've created a static |
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) |
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.
Some suggestions to make the tests a bit more robust if changes occur in this in the future
Thanks @SteveL-MSFT. I have made the changes to those tests. |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
PR Summary
Fixes #20954
Fix SSH hostname parsing to be case sensitive. This is to have
Enter-PSSession
preseve case when resolving host names from~/.ssh/config
.E.g. if you had the following
~/.ssh/config
:Current behaviour for
Enter-PSSession -Hostname HOST1
would lowercase the hostname and use10.0.1.1
hostname.The new behaviour in PR makes sure that host name case is preserved and
Enter-PSSession -Hostname HOST1
would use10.0.1.2
hostname.PR Context
Given
System.Uri
canonicalizes URI and makesSystem.Uri.Host
lowercase, it is not possible to disable this to allow case sensitive hostname parsing for SSH.A workaround I added in this PR was to inspect
System.Uri.OriginalString
, and extract the preserved case hostname substring.More than happy to use a more easier/efficient way, but given the limitations with
System.Uri
API for customizing canonicalization this seemed like the easiest workaround.Also moved the following methods from
PSRemotingBaseCmdlet.cs
to staticRemotingUtils.cs
class so they can be tested easier:ParseSshHostName
ResolveComputerName
ValidateComputerName
And added XUnit tests for
ParseSshHostname
to cover different test cases so current behaviour doesn't break anything.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.(which runs in a different PS Host).