Remove authentication failure protection counter

State between multiple credential_callback calls is now being maintained within the `CredentialPayload`. This change allows credential types to just contain credential information.
JuliaLang · Sep 7, 2017 · 26014f9 · 26014f9
1 parent 4c8e693
commit 26014f9
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 76 deletions.
diff --git a/base/libgit2/callbacks.jl b/base/libgit2/callbacks.jl
@@ -50,7 +50,11 @@ end
 
 function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload, username_ptr)
     creds = Base.get(p.credential)::SSHCredentials
-    isusedcreds = checkused!(creds)
+
+    # Reset password on sucessive calls
+    if !p.first_pass
+        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.
@@ -74,15 +78,10 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload,
         # if username is not provided or empty, then prompt for it
         username = username_ptr != Cstring(C_NULL) ? unsafe_string(username_ptr) : ""
         if isempty(username)
-            uname = creds.user # check if credentials were already used
             prompt_url = git_url(scheme=p.scheme, host=p.host)
-            if !isusedcreds
-                username = uname
-            else
-                response = Base.prompt("Username for '$prompt_url'", default=uname)
-                isnull(response) && return user_abort()
-                username = unsafe_get(response)
-            end
+            response = Base.prompt("Username for '$prompt_url'", default=creds.user)
+            isnull(response) && return user_abort()
+            username = unsafe_get(response)
         end
 
         prompt_url = git_url(scheme=p.scheme, host=p.host, username=username)
@@ -92,7 +91,7 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload,
             ENV["SSH_KEY_PATH"]
         else
             keydefpath = creds.prvkey # check if credentials were already used
-            if isempty(keydefpath) || isusedcreds
+            if isempty(keydefpath)
                 defaultkeydefpath = joinpath(homedir(),".ssh","id_rsa")
                 if isempty(keydefpath) && isfile(defaultkeydefpath)
                     keydefpath = defaultkeydefpath
@@ -117,7 +116,7 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload,
             ENV["SSH_PUB_KEY_PATH"]
         else
             keydefpath = creds.pubkey # check if credentials were already used
-            if isempty(keydefpath) || isusedcreds
+            if isempty(keydefpath)
                 if isempty(keydefpath)
                     keydefpath = privatekey*".pub"
                 end
@@ -135,7 +134,7 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload,
             ENV["SSH_KEY_PASS"]
         else
             passdef = creds.pass # check if credentials were already used
-            if (isempty(passdef) || isusedcreds) && is_passphrase_required(privatekey)
+            if isempty(passdef) && is_passphrase_required(privatekey)
                 if Sys.iswindows()
                     response = Base.winprompt(
                         "Your SSH Key requires a password, please enter it now:",
@@ -151,15 +150,13 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload,
             end
             passdef
         end
-        ((creds.user != username) || (creds.pass != passphrase) ||
-            (creds.prvkey != privatekey) || (creds.pubkey != publickey)) && reset!(creds)
 
         creds.user = username # save credentials
         creds.prvkey = privatekey # save credentials
         creds.pubkey = publickey # save credentials
         creds.pass = passphrase
-    else
-        isusedcreds && return Cint(Error.EAUTH)
+    elseif !p.first_pass
+        return Cint(Error.EAUTH)
     end
 
     return ccall((:git_cred_ssh_key_new, :libgit2), Cint,
@@ -169,37 +166,39 @@ end
 
 function authenticate_userpass(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload)
     creds = Base.get(p.credential)::UserPasswordCredentials
-    isusedcreds = checkused!(creds)
+
+    # Reset password on sucessive calls
+    if !p.first_pass
+        creds.pass = ""
+    end
 
     if creds.prompt_if_incorrect
         username = creds.user
         userpass = creds.pass
-        prompt_url = git_url(scheme=p.scheme, host=p.host)
-        if Sys.iswindows()
-            if isempty(username) || isempty(userpass) || isusedcreds
+        if isempty(username) || isempty(userpass)
+            prompt_url = git_url(scheme=p.scheme, host=p.host)
+            if Sys.iswindows()
                 response = Base.winprompt("Please enter your credentials for '$prompt_url'", "Credentials required",
                     isempty(username) ? p.username : username; prompt_username = true)
                 isnull(response) && return user_abort()
                 username, userpass = unsafe_get(response)
-            end
-        elseif isusedcreds
-            response = Base.prompt("Username for '$prompt_url'",
-                default=isempty(username) ? p.username : username)
-            isnull(response) && return user_abort()
-            username = unsafe_get(response)
+            else
+                response = Base.prompt("Username for '$prompt_url'",
+                    default=isempty(username) ? p.username : username)
+                isnull(response) && return user_abort()
+                username = unsafe_get(response)
 
-            prompt_url = git_url(scheme=p.scheme, host=p.host, username=username)
-            response = Base.prompt("Password for '$prompt_url'", password=true)
-            isnull(response) && return user_abort()
-            userpass = unsafe_get(response)
-            isempty(userpass) && return user_abort()  # Ambiguous if EOF or newline
+                prompt_url = git_url(scheme=p.scheme, host=p.host, username=username)
+                response = Base.prompt("Password for '$prompt_url'", password=true)
+                isnull(response) && return user_abort()
+                userpass = unsafe_get(response)
+                isempty(userpass) && return user_abort()  # Ambiguous if EOF or newline
+            end
         end
-
-        ((creds.user != username) || (creds.pass != userpass)) && reset!(creds)
         creds.user = username # save credentials
         creds.pass = userpass # save credentials
-    else
-        isusedcreds && return Cint(Error.EAUTH)
+    elseif !p.first_pass
+        return Cint(Error.EAUTH)
     end
 
     return ccall((:git_cred_userpass_plaintext_new, :libgit2), Cint,
@@ -228,11 +227,7 @@ Credentials are checked in the following order (if supported):
 **Note**: Due to the specifics of the `libgit2` authentication procedure, when
 authentication fails, this function is called again without any indication whether
 authentication was successful or not. To avoid an infinite loop from repeatedly
-using the same faulty credentials, the `checkused!` function can be called. This
-function returns `true` if the credentials were used.
-Using credentials triggers a user prompt for (re)entering required information.
-`UserPasswordCredentials` and `CachedCredentials` are implemented using a call
-counting strategy that prevents repeated usage of faulty credentials.
+using the same faulty credentials, we will keep track of state using the payload.
 """
 function credentials_callback(libgit2credptr::Ptr{Ptr{Void}}, url_ptr::Cstring,
                               username_ptr::Cstring,
@@ -269,12 +264,16 @@ function credentials_callback(libgit2credptr::Ptr{Ptr{Void}}, url_ptr::Cstring,
                 allowed_types &= Cuint(0)  # Unhandled credential type
             end
         end
+
+        p.first_pass = true
+    else
+        p.first_pass = false
     end
 
     # use ssh key or ssh-agent
     if isset(allowed_types, Cuint(Consts.CREDTYPE_SSH_KEY))
         if isnull(p.credential) || !isa(unsafe_get(p.credential), SSHCredentials)
-            creds = reset!(SSHCredentials(p.username, "", true), -1)
+            creds = SSHCredentials(p.username, "", true)
             if !isnull(p.cache)
                 credid = "ssh://$(p.host)"
                 creds = get_creds!(unsafe_get(p.cache), credid, creds)
@@ -287,7 +286,7 @@ function credentials_callback(libgit2credptr::Ptr{Ptr{Void}}, url_ptr::Cstring,
 
     if isset(allowed_types, Cuint(Consts.CREDTYPE_USERPASS_PLAINTEXT))
         if isnull(p.credential) || !isa(unsafe_get(p.credential), UserPasswordCredentials)
-            creds = reset!(UserPasswordCredentials(p.username, "", true), -1)
+            creds = UserPasswordCredentials(p.username, "", true)
             if !isnull(p.cache)
                 credid = "$(isempty(p.scheme) ? "ssh" : p.scheme)://$(p.host)"
                 creds = get_creds!(unsafe_get(p.cache), credid, creds)

diff --git a/base/libgit2/types.jl b/base/libgit2/types.jl
@@ -1076,9 +1076,8 @@ mutable struct UserPasswordCredentials <: AbstractCredentials
     user::String
     pass::String
     prompt_if_incorrect::Bool    # Whether to allow interactive prompting if the credentials are incorrect
-    count::Int                   # authentication failure protection count
     function UserPasswordCredentials(u::AbstractString,p::AbstractString,prompt_if_incorrect::Bool=false)
-        c = new(u,p,prompt_if_incorrect,3)
+        c = new(u,p,prompt_if_incorrect)
         finalizer(c, securezero!)
         return c
     end
@@ -1088,7 +1087,6 @@ end
 function securezero!(cred::UserPasswordCredentials)
     securezero!(cred.user)
     securezero!(cred.pass)
-    cred.count = 0
     return cred
 end
 
@@ -1104,9 +1102,8 @@ mutable struct SSHCredentials <: AbstractCredentials
     pubkey::String
     usesshagent::String  # used for ssh-agent authentication
     prompt_if_incorrect::Bool    # Whether to allow interactive prompting if the credentials are incorrect
-    count::Int
     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,3)
+        c = new(u,p,prvkey,pubkey,"Y",prompt_if_incorrect)
         finalizer(c, securezero!)
         return c
     end
@@ -1119,7 +1116,6 @@ function securezero!(cred::SSHCredentials)
     securezero!(cred.pass)
     securezero!(cred.prvkey)
     securezero!(cred.pubkey)
-    cred.count = 0
     return cred
 end
 
@@ -1128,21 +1124,11 @@ function Base.:(==)(a::SSHCredentials, b::SSHCredentials)
 end
 
 "Credentials that support caching"
-mutable struct CachedCredentials <: AbstractCredentials
+struct CachedCredentials <: AbstractCredentials
     cred::Dict{String,AbstractCredentials}
-    count::Int            # authentication failure protection count
-    CachedCredentials() = new(Dict{String,AbstractCredentials}(),3)
+    CachedCredentials() = new(Dict{String,AbstractCredentials}())
 end
 
-"Checks if credentials were used or failed authentication, see `LibGit2.credentials_callback`"
-function checkused!(p::Union{UserPasswordCredentials, SSHCredentials})
-    p.count <= 0 && return true
-    p.count -= 1
-    return false
-end
-reset!(p::Union{UserPasswordCredentials, SSHCredentials}, cnt::Int=3) = (p.count = cnt; p)
-reset!(p::CachedCredentials) = (foreach(reset!, values(p.cred)); p)
-
 "Obtain the cached credentials for the given host+protocol (credid), or return and store the default if not found"
 get_creds!(collection::CachedCredentials, credid, default) = get!(collection.cred, credid, default)
 
@@ -1161,13 +1147,14 @@ instances will be used when the URL has changed.
 mutable struct CredentialPayload <: Payload
     credential::Nullable{AbstractCredentials}
     cache::Nullable{CachedCredentials}
+    first_pass::Bool
     scheme::String
     username::String
     host::String
     path::String
 
     function CredentialPayload(credential::Nullable{<:AbstractCredentials}, cache::Nullable{CachedCredentials})
-        new(credential, cache, "", "", "", "")
+        new(credential, cache, true, "", "", "", "")
     end
 end
 

diff --git a/base/pkg/entry.jl b/base/pkg/entry.jl
@@ -427,7 +427,6 @@ function update(branch::AbstractString, upkgs::Set{String})
                         success = true
                         try
                             LibGit2.fetch(repo, payload = Nullable(creds))
-                            LibGit2.reset!(creds)
                             LibGit2.merge!(repo, fastforward=true)
                         catch err
                             cex = CapturedException(err, catch_backtrace())

diff --git a/test/libgit2-helpers.jl b/test/libgit2-helpers.jl
@@ -71,7 +71,7 @@ function credential_loop(
         cache = get(payload.cache)
 
         m = match(LibGit2.URL_REGEX, url)
-        default_cred = LibGit2.reset!(SSHCredentials(true), -1)
+        default_cred = SSHCredentials(true)
         default_cred.usesshagent = "N"
         LibGit2.get_creds!(cache, "ssh://$(m[:host])", default_cred)
     end

diff --git a/test/libgit2.jl b/test/libgit2.jl
@@ -1588,16 +1588,10 @@ mktempdir() do dir
         creds_user = "USER"
         creds_pass = "PASS"
         creds = LibGit2.UserPasswordCredentials(creds_user, creds_pass)
-        @test !LibGit2.checkused!(creds)
-        @test !LibGit2.checkused!(creds)
-        @test !LibGit2.checkused!(creds)
-        @test LibGit2.checkused!(creds)
         @test creds.user == creds_user
         @test creds.pass == creds_pass
         creds2 = LibGit2.UserPasswordCredentials(creds_user, creds_pass)
         @test creds == creds2
-        LibGit2.reset!(creds)
-        @test !LibGit2.checkused!(creds)
         sshcreds = LibGit2.SSHCredentials(creds_user, creds_pass)
         @test sshcreds.user == creds_user
         @test sshcreds.pass == creds_pass
@@ -1687,7 +1681,7 @@ mktempdir() do dir
                 ]
                 err, auth_attempts = challenge_prompt(ssh_p_ex, challenges)
                 @test err == git_ok
-                @test auth_attempts == 5
+                @test auth_attempts == 2
 
                 # User sends EOF in passphrase prompt which aborts the credential request
                 challenges = [
@@ -1737,7 +1731,7 @@ mktempdir() do dir
                 ]
                 err, auth_attempts = challenge_prompt(ssh_u_ex, challenges)
                 @test err == abort_prompt
-                @test auth_attempts == 5  # Should ideally be <= 2
+                @test auth_attempts == 2
 
                 # User repeatedly chooses an invalid username
                 challenges = [
@@ -1747,7 +1741,7 @@ mktempdir() do dir
                 ]
                 err, auth_attempts = challenge_prompt(ssh_u_ex, challenges)
                 @test err == abort_prompt
-                @test auth_attempts == 6
+                @test auth_attempts == 3
 
                 # Credential callback is given an empty string in the `username_ptr`
                 # instead of the typical C_NULL.
@@ -1904,7 +1898,7 @@ mktempdir() do dir
             ]
             err, auth_attempts = challenge_prompt(https_ex, challenges)
             @test err == git_ok
-            @test auth_attempts == 5
+            @test auth_attempts == 2
         end
 
         @testset "SSH explicit credentials" begin
@@ -1940,7 +1934,7 @@ mktempdir() do dir
             # TODO: Unless the SSH agent is disabled we may get caught in an infinite loop
             err, auth_attempts = challenge_prompt(invalid_ex, [])
             @test err == eauth_error
-            @test auth_attempts == 4
+            @test auth_attempts == 2
         end
 
         @testset "HTTPS explicit credentials" begin
@@ -1974,7 +1968,7 @@ mktempdir() do dir
             # Explicitly provided credential is incorrect
             err, auth_attempts = challenge_prompt(invalid_ex, [])
             @test err == eauth_error
-            @test auth_attempts == 4
+            @test auth_attempts == 2
         end
 
         @testset "Cached credentials" begin
@@ -2040,7 +2034,7 @@ mktempdir() do dir
             expected_cred = LibGit2.UserPasswordCredentials(valid_username, valid_password)
             err, auth_attempts, cache = challenge_prompt(replace_ex, challenges)
             @test err == git_ok
-            @test auth_attempts == 4
+            @test auth_attempts == 2
             @test typeof(cache) == LibGit2.CachedCredentials
             @test cache.cred == Dict(cred_id => expected_cred)
         end