Merge pull request #23641 from JuliaLang/cv/libgit2-ssh-agent

Move SSH agent state out of SSHCredentials
JuliaLang · Sep 11, 2017 · 49776dd · 49776dd
2 parents b3ec03f + be167aa
commit 49776dd
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 31 deletions.
diff --git a/base/libgit2/callbacks.jl b/base/libgit2/callbacks.jl
@@ -56,22 +56,16 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload,
         creds.pass = ""
     end
 
-    # Note: The same SSHCredentials can be used to authenticate separate requests using the
-    # same credential cache. e.g. using Pkg.update when there are two private packages.
-    errcls, errmsg = Error.last_error()
-    if errcls != Error.None
-        # Check if we used ssh-agent
-        if creds.usesshagent == "U"
-            creds.usesshagent = "E" # reported ssh-agent error, disables ssh agent use for the future
-        end
-    end
-
     # first try ssh-agent if credentials support its usage
-    if creds.usesshagent == "Y" || creds.usesshagent == "U"
+    if p.use_ssh_agent == 'Y' && username_ptr != Cstring(C_NULL)
         err = ccall((:git_cred_ssh_key_from_agent, :libgit2), Cint,
-                     (Ptr{Ptr{Void}}, Cstring), libgit2credptr, username_ptr)
-        creds.usesshagent = "U" # used ssh-agent only one time
-        err == 0 && return Cint(0)
+                    (Ptr{Ptr{Void}}, Cstring), libgit2credptr, username_ptr)
+        if err == 0
+            p.use_ssh_agent = 'U'  # used ssh-agent only one time
+            return Cint(0)
+        else
+            p.use_ssh_agent = 'E'
+        end
     end
 
     if creds.prompt_if_incorrect
@@ -221,7 +215,7 @@ For `LibGit2.Consts.CREDTYPE_SSH_KEY` type, if the payload contains fields:
 Typing `^D` (control key together with the `d` key) will abort the credential prompt.
 
 Credentials are checked in the following order (if supported):
-- ssh key pair (`ssh-agent` if specified in payload's `usesshagent` field)
+- ssh key pair (`ssh-agent` if specified in payload's `use_ssh_agent` field)
 - plain text
 
 **Note**: Due to the specifics of the `libgit2` authentication procedure, when

diff --git a/base/libgit2/types.jl b/base/libgit2/types.jl
@@ -1095,10 +1095,9 @@ mutable struct SSHCredentials <: AbstractCredentials
     pass::String
     prvkey::String
     pubkey::String
-    usesshagent::String  # used for ssh-agent authentication
     prompt_if_incorrect::Bool    # Whether to allow interactive prompting if the credentials are incorrect
     function SSHCredentials(u::AbstractString,p::AbstractString,prvkey::AbstractString,pubkey::AbstractString,prompt_if_incorrect::Bool=false)
-        c = new(u,p,prvkey,pubkey,"Y",prompt_if_incorrect)
+        c = new(u,p,prvkey,pubkey,prompt_if_incorrect)
         finalizer(c, securezero!)
         return c
     end
@@ -1143,13 +1142,14 @@ mutable struct CredentialPayload <: Payload
     credential::Nullable{AbstractCredentials}
     cache::Nullable{CachedCredentials}
     first_pass::Bool
+    use_ssh_agent::Char
     scheme::String
     username::String
     host::String
     path::String
 
     function CredentialPayload(credential::Nullable{<:AbstractCredentials}, cache::Nullable{CachedCredentials})
-        new(credential, cache, true, "", "", "", "")
+        new(credential, cache, true, 'Y', "", "", "", "")
     end
 end
 

diff --git a/test/libgit2-helpers.jl b/test/libgit2-helpers.jl
@@ -65,15 +65,7 @@ function credential_loop(
         use_ssh_agent::Bool=false)
 
     if !use_ssh_agent
-        if isnull(payload.cache)
-            payload.cache = Nullable(CachedCredentials())
-        end
-        cache = get(payload.cache)
-
-        m = match(LibGit2.URL_REGEX, url)
-        default_cred = SSHCredentials(true)
-        default_cred.usesshagent = "N"
-        LibGit2.get_creds!(cache, "ssh://$(m[:host])", default_cred)
+        payload.use_ssh_agent = 'N'
     end
 
     credential_loop(valid_credential, url, user, 0x000046, payload)

diff --git a/test/libgit2.jl b/test/libgit2.jl
@@ -1644,10 +1644,11 @@ mktempdir() do dir
             end
 
             # SSH requires username
+            url_no_username = "github.com:test/package.jl"
             ssh_u_ex = quote
                 include($LIBGIT2_HELPER_PATH)
                 valid_cred = SSHCredentials($username, "", $valid_key, $(valid_key * ".pub"))
-                credential_loop(valid_cred, $url)
+                credential_loop(valid_cred, $url_no_username)
             end
 
             # Note: We cannot use the default ~/.ssh/id_rsa for tests since we cannot be
@@ -1748,7 +1749,7 @@ mktempdir() do dir
                 ssh_user_empty_ex = quote
                     include($LIBGIT2_HELPER_PATH)
                     valid_cred = LibGit2.SSHCredentials($username, "", $valid_key, $(valid_key * ".pub"))
-                    credential_loop(valid_cred, $url, "")
+                    credential_loop(valid_cred, $url_no_username, "")
                 end
 
                 challenges = [
@@ -1901,6 +1902,39 @@ mktempdir() do dir
             @test auth_attempts == 2
         end
 
+        @testset "SSH agent username" begin
+            url = "github.com:test/package.jl"
+
+            valid_key = joinpath(KEY_DIR, "valid")
+            username = "git"
+
+            ssh_empty_ex = quote
+                include($LIBGIT2_HELPER_PATH)
+                valid_cred = LibGit2.SSHCredentials($username, "", $valid_key, $(valid_key * ".pub"))
+                credential_loop(valid_cred, $url, Nullable(""), use_ssh_agent=true)
+            end
+
+            ssh_null_ex = quote
+                include($LIBGIT2_HELPER_PATH)
+                valid_cred = LibGit2.SSHCredentials($username, "", $valid_key, $(valid_key * ".pub"))
+                credential_loop(valid_cred, $url, Nullable(), use_ssh_agent=true)
+            end
+
+            challenges = [
+                "Username for 'github.com':" => "\x04",
+            ]
+
+            err, auth_attempts = challenge_prompt(ssh_empty_ex, challenges)
+            @test err == abort_prompt  # TODO: `eauth_error` when we can disable prompting
+            @test auth_attempts == 2
+
+            # A null username_ptr passed into `git_cred_ssh_key_from_agent` can cause a
+            # segfault.
+            err, auth_attempts = challenge_prompt(ssh_null_ex, challenges)
+            @test err == abort_prompt  # TODO: `eauth_error` when we can disable prompting
+            @test auth_attempts == 1
+        end
+
         @testset "SSH explicit credentials" begin
             url = "[email protected]:test/package.jl"
 
@@ -1920,9 +1954,8 @@ mktempdir() do dir
                 include($LIBGIT2_HELPER_PATH)
                 valid_cred = LibGit2.SSHCredentials($username, $passphrase, $valid_p_key, $(valid_p_key * ".pub"))
                 invalid_cred = LibGit2.SSHCredentials($username, "", $invalid_key, $(invalid_key * ".pub"))
-                invalid_cred.usesshagent = "N"  # Disable SSH agent use
                 payload = CredentialPayload(Nullable(invalid_cred))
-                credential_loop(valid_cred, $url, $username, payload)
+                credential_loop(valid_cred, $url, $username, payload, use_ssh_agent=false)
             end
 
             # Explicitly provided credential is correct