Skip to content

Commit

Permalink
Merge pull request #23711 from JuliaLang/cv/libgit2-cred-approval
Browse files Browse the repository at this point in the history
LibGit2 credential approval/rejection
  • Loading branch information
omus authored Sep 18, 2017
2 parents bb1671f + 86c6a17 commit d668a95
Show file tree
Hide file tree
Showing 9 changed files with 141 additions and 32 deletions.
5 changes: 5 additions & 0 deletions base/deprecated.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1810,6 +1810,11 @@ end
# `SSHCredentials` and `UserPasswordCredentials` constructors using `prompt_if_incorrect`
# are deprecated in base/libgit2/types.jl.

# PR #23711
@eval LibGit2 begin
@deprecate get_creds!(cache::CachedCredentials, credid, default) get!(cache, credid, default)
end

# END 0.7 deprecations

# BEGIN 1.0 deprecations
Expand Down
42 changes: 22 additions & 20 deletions base/libgit2/callbacks.jl
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function user_abort()
(Cint, Cstring),
Cint(Error.Callback), "Aborting, user cancelled credential request.")

return Cint(Error.EAUTH)
return Cint(Error.EUSER)
end

function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload, username_ptr)
Expand Down Expand Up @@ -231,28 +231,40 @@ function credentials_callback(libgit2credptr::Ptr{Ptr{Void}}, url_ptr::Cstring,

# Parse URL only during the first call to this function. Future calls will use the
# information cached inside the payload.
if isempty(p.host)
url = match(URL_REGEX, unsafe_string(url_ptr))
if isempty(p.url)
p.url = unsafe_string(url_ptr)
m = match(URL_REGEX, p.url)

p.scheme = url[:scheme] === nothing ? "" : url[:scheme]
p.username = url[:user] === nothing ? "" : url[:user]
p.host = url[:host]
p.path = url[:path]
p.scheme = m[:scheme] === nothing ? "" : m[:scheme]
p.username = m[:user] === nothing ? "" : m[:user]
p.host = m[:host]
p.path = m[:path]

# When an explicit credential is supplied we will make sure to use the given
# credential during the first callback by modifying the allowed types. The
# modification only is in effect for the first callback since `allowed_types` cannot
# be mutated.
if !isnull(p.explicit)
p.credential = p.explicit
cred = unsafe_get(p.explicit)

# Copy explicit credentials to avoid mutating approved credentials.
p.credential = Nullable(deepcopy(cred))

if isa(cred, SSHCredentials)
allowed_types &= Cuint(Consts.CREDTYPE_SSH_KEY)
elseif isa(cred, UserPasswordCredentials)
allowed_types &= Cuint(Consts.CREDTYPE_USERPASS_PLAINTEXT)
else
allowed_types &= Cuint(0) # Unhandled credential type
end
elseif !isnull(p.cache)
cache = unsafe_get(p.cache)
cred_id = credential_identifier(p.scheme, p.host)

# Perform a deepcopy as we do not want to mutate approved cached credentials
if haskey(cache, cred_id)
p.credential = Nullable(deepcopy(cache[cred_id]))
end
end

p.first_pass = true
Expand All @@ -263,25 +275,15 @@ function credentials_callback(libgit2credptr::Ptr{Ptr{Void}}, url_ptr::Cstring,
# 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 = SSHCredentials(p.username)
if !isnull(p.cache)
credid = "ssh://$(p.host)"
creds = get_creds!(unsafe_get(p.cache), credid, creds)
end
p.credential = Nullable(creds)
p.credential = Nullable(SSHCredentials(p.username))
end
err = authenticate_ssh(libgit2credptr, p, username_ptr)
err == 0 && return err
end

if isset(allowed_types, Cuint(Consts.CREDTYPE_USERPASS_PLAINTEXT))
if isnull(p.credential) || !isa(unsafe_get(p.credential), UserPasswordCredentials)
creds = UserPasswordCredentials(p.username)
if !isnull(p.cache)
credid = "$(isempty(p.scheme) ? "ssh" : p.scheme)://$(p.host)"
creds = get_creds!(unsafe_get(p.cache), credid, creds)
end
p.credential = Nullable(creds)
p.credential = Nullable(UserPasswordCredentials(p.username))
end
err = authenticate_userpass(libgit2credptr, p)
err == 0 && return err
Expand Down
29 changes: 26 additions & 3 deletions base/libgit2/libgit2.jl
Original file line number Diff line number Diff line change
Expand Up @@ -265,12 +265,19 @@ function fetch(repo::GitRepo; remote::AbstractString="origin",
else
GitRemoteAnon(repo, remoteurl)
end
try
result = try
fo = FetchOptions(callbacks=RemoteCallbacks(credentials_cb(), p))
fetch(rmt, refspecs, msg="from $(url(rmt))", options = fo)
catch err
if isa(err, GitError) && err.code == Error.EAUTH
reject(payload)
end
rethrow()
finally
close(rmt)
end
approve(payload)
return result
end

"""
Expand Down Expand Up @@ -300,12 +307,19 @@ function push(repo::GitRepo; remote::AbstractString="origin",
else
GitRemoteAnon(repo, remoteurl)
end
try
result = try
push_opts = PushOptions(callbacks=RemoteCallbacks(credentials_cb(), p))
push(rmt, refspecs, force=force, options=push_opts)
catch err
if isa(err, GitError) && err.code == Error.EAUTH
reject(payload)
end
rethrow()
finally
close(rmt)
end
approve(payload)
return result
end

"""
Expand Down Expand Up @@ -513,7 +527,16 @@ function clone(repo_url::AbstractString, repo_path::AbstractString;
fetch_opts = fetch_opts,
remote_cb = remote_cb
)
return clone(repo_url, repo_path, clone_opts)
repo = try
clone(repo_url, repo_path, clone_opts)
catch err
if isa(err, GitError) && err.code == Error.EAUTH
reject(payload)
end
rethrow()
end
approve(payload)
return repo
end

""" git reset [<committish>] [--] <pathspecs>... """
Expand Down
40 changes: 38 additions & 2 deletions base/libgit2/types.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1136,14 +1136,30 @@ struct CachedCredentials <: AbstractCredentials
CachedCredentials() = new(Dict{String,AbstractCredentials}())
end

"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)
Base.haskey(cache::CachedCredentials, cred_id) = Base.haskey(cache.cred, cred_id)
Base.getindex(cache::CachedCredentials, cred_id) = Base.getindex(cache.cred, cred_id)
Base.get!(cache::CachedCredentials, cred_id, default) = Base.get!(cache.cred, cred_id, default)

function securezero!(p::CachedCredentials)
foreach(securezero!, values(p.cred))
return p
end

function approve(cache::CachedCredentials, cred::AbstractCredentials, url::AbstractString)
cred_id = credential_identifier(url)
cache.cred[cred_id] = cred
nothing
end

function reject(cache::CachedCredentials, cred::AbstractCredentials, url::AbstractString)
cred_id = credential_identifier(url)
if haskey(cache.cred, cred_id)
securezero!(cache.cred[cred_id]) # Wipe out invalid credentials
delete!(cache.cred, cred_id)
end
nothing
end

"""
LibGit2.CredentialPayload
Expand All @@ -1161,6 +1177,7 @@ mutable struct CredentialPayload <: Payload
credential::Nullable{AbstractCredentials}
first_pass::Bool
use_ssh_agent::Bool
url::String
scheme::String
username::String
host::String
Expand Down Expand Up @@ -1194,10 +1211,29 @@ function reset!(p::CredentialPayload)
p.credential = Nullable{AbstractCredentials}()
p.first_pass = true
p.use_ssh_agent = p.allow_ssh_agent
p.url = ""
p.scheme = ""
p.username = ""
p.host = ""
p.path = ""

return p
end

function approve(p::CredentialPayload)
isnull(p.credential) && return # No credentials were used
cred = unsafe_get(p.credential)

if !isnull(p.cache)
approve(unsafe_get(p.cache), cred, p.url)
end
end

function reject(p::CredentialPayload)
isnull(p.credential) && return # No credentials were used
cred = unsafe_get(p.credential)

if !isnull(p.cache)
reject(unsafe_get(p.cache), cred, p.url)
end
end
11 changes: 11 additions & 0 deletions base/libgit2/utils.jl
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,14 @@ function git_url(;

return String(take!(io))
end

function credential_identifier(scheme::AbstractString, host::AbstractString)
string(isempty(scheme) ? "ssh" : scheme, "://", host)
end

function credential_identifier(url::AbstractString)
m = match(URL_REGEX, url)
scheme = m[:scheme] === nothing ? "" : m[:scheme]
host = m[:host]
credential_identifier(scheme, host)
end
1 change: 0 additions & 1 deletion doc/src/devdocs/libgit2.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ Base.LibGit2.fullname
Base.LibGit2.features
Base.LibGit2.filename
Base.LibGit2.filemode
Base.LibGit2.get_creds!
Base.LibGit2.gitdir
Base.LibGit2.@githash_str
Base.LibGit2.head
Expand Down
6 changes: 6 additions & 0 deletions test/libgit2-helpers.jl
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ function credential_loop(

# Check if the callback provided us with valid credentials
if !isnull(payload.credential) && get(payload.credential) == valid_credential
LibGit2.approve(payload)
break
end

Expand All @@ -46,6 +47,11 @@ function credential_loop(
LibGit2.GitError(err)
end

# Reject the credential when an authentication error occurs
if git_error.code == LibGit2.Error.EAUTH
LibGit2.reject(payload)
end

return git_error, num_authentications
end

Expand Down
31 changes: 29 additions & 2 deletions test/libgit2.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1634,6 +1634,33 @@ mktempdir() do dir
@test sshcreds == sshcreds2
end

@testset "CachedCredentials" begin
cache = LibGit2.CachedCredentials()

url = "https://github.com/JuliaLang/Example.jl"
cred_id = LibGit2.credential_identifier(url)
cred = LibGit2.UserPasswordCredentials(deepcopy("julia"), deepcopy("password"))

@test !haskey(cache, cred_id)

# Reject a credential which wasn't stored
LibGit2.reject(cache, cred, url)
@test !haskey(cache, cred_id)
@test cred.user == "julia"
@test cred.pass == "password"

# Approve a credential which causes it to be stored
LibGit2.approve(cache, cred, url)
@test haskey(cache, cred_id)
@test cache[cred_id] === cred

# Reject an approved should cause it to be removed and erased
LibGit2.reject(cache, cred, url)
@test !haskey(cache, cred_id)
@test cred.user != "julia"
@test cred.pass != "password"
end

# The following tests require that we can fake a TTY so that we can provide passwords
# which use the `getpass` function. At the moment we can only fake this on UNIX based
# systems.
Expand All @@ -1643,7 +1670,7 @@ mktempdir() do dir
"No errors")

abort_prompt = LibGit2.GitError(
LibGit2.Error.Callback, LibGit2.Error.EAUTH,
LibGit2.Error.Callback, LibGit2.Error.EUSER,
"Aborting, user cancelled credential request.")

incompatible_error = LibGit2.GitError(
Expand Down Expand Up @@ -2030,7 +2057,7 @@ mktempdir() do dir
quote
include($LIBGIT2_HELPER_PATH)
cache = CachedCredentials()
$(cached_cred !== nothing && :(LibGit2.get_creds!(cache, $cred_id, $cached_cred)))
$(cached_cred !== nothing && :(LibGit2.approve(cache, $cached_cred, $url)))
payload = CredentialPayload(cache)
err, auth_attempts = credential_loop($valid_cred, $url, "", payload)
(err, auth_attempts, cache)
Expand Down
8 changes: 4 additions & 4 deletions test/misc.jl
Original file line number Diff line number Diff line change
Expand Up @@ -562,10 +562,10 @@ let a = [1,2,3]
@test unsafe_securezero!(Ptr{Void}(pointer(a)), sizeof(a)) == Ptr{Void}(pointer(a))
@test a == [0,0,0]
end
let creds = Base.LibGit2.CachedCredentials()
LibGit2.get_creds!(creds, "foo", LibGit2.SSHCredentials()).pass = "bar"
securezero!(creds)
@test LibGit2.get_creds!(creds, "foo", nothing).pass == "\0\0\0"
let cache = Base.LibGit2.CachedCredentials()
get!(cache, "foo", LibGit2.SSHCredentials("", "bar"))
securezero!(cache)
@test cache["foo"].pass == "\0\0\0"
end

# Test that we can VirtualProtect jitted code to writable
Expand Down

2 comments on commit d668a95

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Please sign in to comment.