Refactor libgit2 credential callback #17691
Conversation
|
cc @wildart for review. Also, for anybody else testing locally would be appreciated, since this functionality is not yet tested. |
| @@ -24,6 +24,153 @@ function mirror_callback(remote::Ptr{Ptr{Void}}, repo_ptr::Ptr{Void}, | |||
| return Cint(0) | |||
| end | |||
|
|
|||
| function authenticate_ssh(creds::SSHCredentials, cred::Ptr{Ptr{Void}}, username_ptr, schema, host) | |||
There was a problem hiding this comment.
could we give creds and cred more clearly distinguishable names? this is going to be bug-prone otherwise
There was a problem hiding this comment.
yeah I think that would be better
|
Works for me! |
| p.count <= 0 && return true | ||
| p.count -= 1 | ||
| return false | ||
| end | ||
| "Resets authentication failure protection count" | ||
| reset!(p::CachedCredentials, cnt::Int=3) = (p.count = cnt) |
There was a problem hiding this comment.
these probably shouldn't be no-ops?
base/libgit2/types.jl:62:reset!(p::AbstractCredentials, cnt::Int=3) = nothing
base/pkg/entry.jl:427: LibGit2.reset!(creds)
Keno
force-pushed
the
kf/refactorlibgit2
branch
2 times, most recently
from
0b3cf2d to
5c2d153
Compare
Cleanly separate the SSHCredentials and UserPasswordCredentials to only use each type when appropriate (the former for SSH keys, the latter for HTTPS for SSH password/keyboard-interactive authentication). Previously the types would sometimes get confused and SSHCredentials would be used for userpass authentication, since the field names happen to be a subset (though they have different meaning, since the `pass` field in SSHCredentials is the SSH key passphrase while the `pass` in UserPasswordCredentials is the remote user's password). Fix a couple issues in the process: - Allow externally passed in credentials to reach the right place - Don't skip using the SSH agent if there are two private packages
| "Resets authentication failure protection count" | ||
| reset!(p::CachedCredentials, cnt::Int=3) = (p.count = cnt) | ||
| reset!(p::Union{UserPasswordCredentials, SSHCredentials}, cnt::Int=3) = (p.count = cnt) | ||
| reset!(p::CachedCredentials) = foreach(reset!, values(p.cred)) |
There was a problem hiding this comment.
no 2-arg method for CachedCredentials?
There was a problem hiding this comment.
I'm not really sure what the point of retrying 3 times is anyway. The protocol is generally realiable and transport errors won't call the authentication callback anyway. I kept it this way because that's what was there before, but this is probably something to get rid of anyway.
There was a problem hiding this comment.
the retrying 3 times was something @wildart needed to implement in order to work around the upstream API not being very flexible, IIRC
There was a problem hiding this comment.
without reporting an authentication result back to credential callback, it is hard to figure out whether credentials are faulty or used several times on various urls. That is why credentials are used 3 times before falling back to user prompt.
@Keno Have you fixed authentication error reporting to credential callback?
There was a problem hiding this comment.
I have not. The count still doesn't make sense to me. Either the credentials are incorrect, in which case retying them 3 times doesn't help, or they're not in which case there's no problem. I agree that we have to record whether we tried or not since we don't get the error back, but I don't know why trying three times would help.
There was a problem hiding this comment.
I'm not suggesting the count be removed entirely, I'm just suggesting setting it to 1 would be sufficient.
There was a problem hiding this comment.
Thanks. So what should we do right now? Merge this PR as is and leave the current strategy?
There was a problem hiding this comment.
x-ref libgit2/libgit2#3761 (comment) where someone using the ruby bindings had reported spurious errors with a limit of 1
There was a problem hiding this comment.
Thanks for pointing that out @tkelman, let's leave things as is for now and fix this mess upstream.
|
I'm gonna go ahead and merge this, we can make further changes later if we want. |
Cleanly separate the SSHCredentials and UserPasswordCredentials to only use each type
when appropriate (the former for SSH keys, the latter for HTTPS for SSH password/keyboard-interactive
authentication). Previously the types would sometimes get confused and SSHCredentials
would be used for userpass authentication, since the field names happen to be a subset
(though they have different meaning, since the
passfield in SSHCredentials is theSSH key passphrase while the
passin UserPasswordCredentials is the remote user'spassword).
Fix a couple issues in the process: