ADD: Support SSH to Windows Machine #477
Conversation
There was a problem hiding this comment.
Hi @cs7-shrey thanks a lot for this, it's a very clean implementation. I don't have many suggestions, I think this is probably good to go in its current form.
The main issue here will be testing, to test over a few different machines / windows versions and check everything is working okay, and then automate these tests. This is related to #208 and #100, which cover extending our test cases (some public, but some internal for our institute). Out of interest, how are you testing at the moment, could you expand the PR message to include a step-by-step of the testing approach?
I'll look into this further and get back to you ASAP! We will definitely move forward with this PR, I'm just not certain on the timeline as mentioned on the zulip chat. But I will try and test it over the next couple of weeks and get back to you here!
Thanks again! this is a very nice addition.
| if "windows" in output: | ||
| return True | ||
|
|
||
| stdin, stdout, stderr = client.exec_command( |
There was a problem hiding this comment.
This looks nice, could you add a small docstring to explain the approach taken (I guess "ver" will not work in powershell terminal, only cmd so that it checked after "ver")?
| f">> %USERPROFILE%\\.ssh\\authorized_keys" | ||
| ) | ||
| client.exec_command( | ||
| "icacls %USERPROFILE%\\.ssh /inheritance:r /grant %USERNAME%:(OI)(CI)F" |
There was a problem hiding this comment.
Could you add a couple of comments here explaining what these steps do?
|
@JoeZiminski I have made changes as you suggested. I wanted to ask one more thing. The current approach does not change file/directory permissions if the windows user is an admin as I assumed that admin accounts have the already required permissions and restrictions. Do you suggest explicitly setting up permissions for admin users too? |
| # double >> for concatenate | ||
| f"echo {key.get_name()} {key.get_base64()}" | ||
| f">> C:\\ProgramData\\ssh\\administrators_authorized_keys" | ||
| ) |
There was a problem hiding this comment.
is it suggested to add commands to change file permissions for admin users?
There was a problem hiding this comment.
Good point @cs7-shrey, I think it makes sense to assume file permissions are already granted, as you say. If it turns out that its causing problems in testing, we can add this later if required.
Description
What is this PR
Why is this PR needed?
Fixes Issue #450 : Support SSH to Windows Machine
What does this PR do?
Allows storing public key on the server during a SSH to a Windows Machine.
References
Issue #450
How has this PR been tested?
Minor testing with a windows machine.
Please explain how any new code has been tested, and how you have ensured that no existing functionality has changed
Connection and data transfer from a windows machine was tested.
Testing Approach
(use
ssh -i <path_to_private_key>) (you may need to remove your own public key from the authorized keys file on windows since SSH will try to use all keys availabe, or you can try SSH withssh -vto inspect the logs and see if your key was accepted.Is this a breaking change?
No
Does this PR require an update to the documentation?
No
Checklist: