-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
#13680 git support private key password #94407
#13680 git support private key password #94407
Conversation
Here is one way to test this without too much trouble :
|
Can it check if an agent is already running? (Which it should be since the Win32-OpenSSH sets the service to automatically start upon reboot and systemd starts all services including ssh on Linux). Basically the only time the ssh-agent might possibly NOT be running is if you are using the |
Please add |
I did not implement this feature in this first version, but I should be able to add it. |
I would add a comment or note somewhere (perhaps a warning message) that the ssh-agent for the official Win32 OpenSSH requires PowerShell/openssh-portable#362 for PKCS11 Support. |
b56d0df
to
127bb59
Compare
Unfortunately it seems that interacting with OpenSSH's With I tried to locate the issue by building OpenSSH from source: it seems that the issue is that data written by
As I have no experience with all of this, any help would be appreciated. |
We might have to leave off for this PR and add a TODO, file an issue on their repo and link it in the TODO |
I might have a solution : I am currently looking into node-pty, which seems to do just what we need. |
It now works with Windows OpenSSH. The current behaviour is:
To find the
To communicate with the
With the OpenSSH version on Windows, if an agent is already running, it will be used (it is the default setting for this command). For the other versions, a new agent is started. |
Awesome job. Linux should default to an already running ssh agent as well. I'm not familiar enough with Mac to know if an agent is already started but I think it's safer to try re-using there too. A new agent should only be started if its not detected as running. For Linux and Mac, you can quickly check by using |
Thanks! I agree, however detecting if an agent is already running is not enough: in order to communicate with it, one must find the socket it is bound to. The man states that it defaults to |
In the event of custom, I would feel comfortable starting a new agent instead unless someone chimes in differently. We're only trying to check standard ssh configurations. So I would just stick to the man and search that file and be sure there are good tests in place to ensure this feature works as intended. If you think it's trivial to address the custom value, then include it by all means and will certainly warrant its own test. |
Another issue with the search for a running agent would be to find which one to use if several are found. For example, a machine I recently installed running Ubuntu 18.04 has 2 agents that start on boot: one seems to come from
Maybe the safest way would be to check if the |
3a6507c
to
67c3e18
Compare
I added this feature: if the |
I suggest rebasing off master again. It appears Linux CI is broken so I guess just run tests locally for Linux. |
67c3e18
to
cb1b353
Compare
Can't wait for this to be merged! Is there anything keeping this PR from being merged? |
It needs to be reviewed first, then I'll have to address the comments made in the review. |
extensions/git/package.nls.json
Outdated
@@ -143,6 +143,8 @@ | |||
"config.untrackedChanges.mixed": "All changes, tracked and untracked, appear together and behave equally.", | |||
"config.untrackedChanges.separate": "Untracked changes appear separately in the Source Control view. They are also excluded from several actions.", | |||
"config.untrackedChanges.hidden": "Untracked changes are hidden and excluded from several actions.", | |||
"config.sshPrivateKeyPath": "Path of the SSH private key associated with your remote.", |
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.
Is this needed even if the ssh config file includes proper Host entries?
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.
Also, what happens if the ssh-agent already has some keys added?
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.
Also see #13680 (comment). It might be relevant.
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.
Is this needed even if the ssh config file includes proper Host entries?
It can be avoided, but it would cost more substantial changes to the extension code, so I am waiting for reviews before doing it. I think the best way to use the SSH config file would not be to parse it (that would add complexity to the extension) but to read the output of the git commands and to handle any password prompt there. However, that would mean changing the exec
function since it uses child_process
which is unable to read such prompts, but replacing it with node-pty
as I did here would not be enough since we would lose the stdout / stderr distinction.
Also, what happens if the ssh-agent already has some keys added?
If the ssh-agent already has some keys added, nothing changes : you can add the same key twice without side effects.
Also see #13680 (comment). It might be relevant.
I agree that this could be an interesting feature. Git Bash's agents store their SSH_AUTH_SOCK
in %TEMP%\ssh-*
folders, so we could check them before starting the agent. I chose not to do it at first because I read that OpenSSH was now installed by default on Windows 10, and as you noted in your comment its agent is available to all processes. However I guess there is still a number of people who do not use it and would therefore be interested in this.
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.
Thanks for clarifying. Let's wait for some maintainers to get around to reviewing this.
I think it's better to get something in first and then iterate on feedback rather than holding things up to achieve perfection.
Thanks for picking this up. I attempted this 4 years ago and got to where you were stuck - interaction with the ssh-agent. Thanks a lot for persisting with this and bridging that gap. It's surprising really how difficult /impossible interaction with ssh is. |
cb1b353
to
cc1925c
Compare
cc1925c
to
142f1c5
Compare
1d22a32
to
722e4ed
Compare
These last commits make this work on Windows 10 (until recently I could only test it on Windows 7). One part of the fix involved a better handling of ANSI/VT sequences. I made a custom function in the git extension to remove them, but I think it would be better to put it elsewhere. I wanted to edit and use the |
Closing this PR as this capability has been added through #159573 leveraging some of the existing ASKPASS infrastructure. |
This PR fixes #13680.
The git extension starts an
ssh-agent
right after the Git object instantiation. Code looks for this command in the directory given by thegit.sshAgentDirectory
setting or inC:\Program Files\Git\usr\bin
on Windows or/usr/bin
on other platforms, before tyring to call the raw command.It then stores the associated
SSH_AUTH_SOCK
andSSH_AGENT_PID
environment variables and uses them for every subsequentgit
call.When opening a git repository, Code checks for a
git.sshPrivateKeyPath
. If it is set, it adds it to the running ssh-agent.