Fix ssh-agent use by libgit2 #17459
Fix ssh-agent use by libgit2 #17459
Conversation
| @@ -82,7 +82,8 @@ function credentials_callback(cred::Ptr{Ptr{Void}}, url_ptr::Cstring, | |||
| end | |||
|
|
|||
| # first try ssh-agent if credentials support its usage | |||
| if creds[:usesshagent, credid] == "Y" | |||
| if creds[:usesshagent, credid] === nothing || | |||
There was a problem hiding this comment.
shouldn't this have just happened a few lines up?
There was a problem hiding this comment.
That's what I thought, but if creds is EmpyCredentials, it ignores that setindex.
There was a problem hiding this comment.
that sounds like a bug maybe?
There was a problem hiding this comment.
Well, it looks to be by design, but I'm not sure, @wildart should take a look.
There was a problem hiding this comment.
Right, EmpyCredentials is a useless class. But this fix will work.
There was a problem hiding this comment.
why is it getting used or passed around then? if it doesn't store anything, then all the logic is going to be wrong for it and this is not going to be the only bug
There was a problem hiding this comment.
Yeah, having a setindex that is later not retrievable by setindex with the same index violates expectations. It would be good to redesign that API
There was a problem hiding this comment.
right. don't need to do so right this minute unless that turns out to be the best fix to the blocker bugs we're seeing, but it's going to be error prone until we do
|
You better disable ssh-agent usage on windows. |
|
how should windows users use ssh without it? cc @davidanthoff |
|
Not all git builds on windows come with ssh-agent. I believe recent windows build of Git integrates with Windows Credential Store. I'm not sure whether it is used for ssh. |
|
I don't understand what the harm is. If the agent can't be found, it should fall back to alternative methods. |
|
Here is my understanding of the landscape. I might well be off. Current git versions for windows bring ssh-agent along, i.e. ssh-agent.exe is in the usr\bin folder in the folder where you install git. posh-git (the powershell plugin that most people seem to use if the use powershell and git), automatically starts ssh-agent when you start a new powershell prompt. So when I start julia from a powershell prompt, ssh-agent is running. Once ssh-agent is started, you have to manually kill it in task manager, or log out, to end that process. So at least for me, ssh-agent is running 99% of the time. If I'm in a window session and had never started powershell (with posh-git) and had not somehow manually started ssh-agent, it would probably not be running. If I then started julia from the startmenu shortcut, I guess ssh-agent would not be running. This has more info that might be relevant. And this. Then there is also the Git credential manager for Windows. So, my sense is that a lot of Windows users will have a ssh-agent running. I think if it is running, julia should just use it, and not prompt for anything, right? |
|
if you check ssh-agent.exe in dependency walker though, it should be linking to msys-2.0.dll, or cygwin1.dll for cygwin's ssh-agent, so it's a posix executable rather than a win32 native one. we no longer bundle a posix layer with the windows julia install (now that cygwin is lgpl we could consider it), so any ssh-agent (or pageant) would have to be a bring your own deal. very few windows users know anything about powershell based tools in my experience, so you might be overestimating how common this will be. |
|
I think the following would be perfectly fine: if julia sees an ssh-agent running, use it, if not, don't. That is probably how it works on Linux and Mac already, right? On Windows that would essentially mean that those people that have git installed (which ships ssh-agent) and got the agent started somehow would see julia using it. Git itself seems the most prevalent vehicle for users to get the ssh-agent binaries on their machines, and then there are lots of ways on how it might be started: the github help tells you how to start it, posh-git starts it, if you install git via something like the github desktop (or whatever the name is), it will run automatically etc. |
|
looks like it happens inside libgit2, and is probably up to libssh2 to figure out how to find and talk to an ssh-agent? |
|
Libgit2 gives very little leeway in terms of managing user credentials. So, either we provide some sort of setup procedure for using SSH protocol, or presume some default parameters (e.g. location of the key), or let user set it up through environmental variables. One should not expect same level of UX functionality from |
|
@wildart Do you see this as the correct fix or is there something else we should do? |
|
The logic behind of credentials callback if following: I wanted to connect to ssh-agent firstly before falling back on manually specified ssh key. This PR would always request ssh-agent authentication in Currently, we avoid authentication issue by forcefully changing protocol to all installed package on I think it would be better to provide support of ssh-agent authentication for N.B. Failing to connect to ssh-agent should not be treaded as error. It only means that some setup is required before using SSH protocol in the package manager. |
|
Yes, if the ssh-agent doesn't work, it should fall back to something else. If this is not the correct fix, please suggest an appropriate one. |
|
There is one caveat: If ssh-agent authentication fails with error, whole operation is aborted. There is no way to fallback on other provider after that. |
|
What counts as an error? |
|
That's pretty stupid. Can we patch libgit2 to avoid this? |
|
Sure, given some time, we can do anything. Just for ref: libgit2/libgit2#3761 |
|
It just seems ridiculous to me that there's no way to retry a request if the ssh agent can't be found. |
|
A bit of topic, I looked at |
|
The proper solution would be to return a authentication error to callback and not fail until callback will specify so. |
|
Maybe re-open the issue with an explanation of the use case we have and why the alternate behavior would be desirable? |
|
I reopend libgit2/libgit2#3761. |
|
Let's just temporarily patch this loop to retry if there was an error with the agent. |
| end | ||
| function Base.setindex!(p::AbstractCredentials, val, key, _=nothing) | ||
| ks = Symbol(key) | ||
| @assert isdefined(p, ks) |
There was a problem hiding this comment.
It's an internal error, an assert is fine.
There was a problem hiding this comment.
if it ever happens, would want to be able to see what the inputs were
Also make setindex! fail if requested on a credential type that does not have the given field.
|
I've pushed an update to this, removing EmptyCredentials entirely. I'll test on windows to see what we can do about failure to connect to ssh agent. |
| @@ -7,7 +7,7 @@ $(eval $(call git-external,libgit2,LIBGIT2,CMakeLists.txt,build/libgit2.$(SHLIB_ | |||
| LIBGIT2_OBJ_SOURCE := $(BUILDDIR)/$(LIBGIT2_SRC_DIR)/libgit2.$(SHLIB_EXT) | |||
| LIBGIT2_OBJ_TARGET := $(build_shlibdir)/libgit2.$(SHLIB_EXT) | |||
|
|
|||
| LIBGIT2_OPTS := $(CMAKE_COMMON) -DCMAKE_BUILD_TYPE=Release -DTHREADSAFE=ON | |||
| LIBGIT2_OPTS := $(CMAKE_COMMON) -DCMAKE_BUILD_TYPE=Debug -DTHREADSAFE=ON | |||
There was a problem hiding this comment.
Yes, left over from debugging, sorry.
|
Ok, now with 100% fewer fatal failures due to ssh-agent. |
|
@Keno did this patch ever get upstreamed? If not, any reason it didn't? |
|
IIRC there was an issue opened (libgit2/libgit2#3866), but this was more of a workaround than a real fix. |
|
Oh right, thanks, I even bumped it a few months later. Maybe I'll ping them on their slack to see if I can get an opinion. |
cc @wildart. Works for me, but I'm not convinced this is the right solution.