Shred CredentialPayload upon approval/rejection (#24884)

* Zero LibGit2 credential payload in approve/reject

Previously running securezero! on credentials in the
reject(::CredentialCache, ...) results in the credentials being wiped
before reject(::GitConfig, ...) was run.

* Always return payload from credential_loop

* Add additional shred tests

Note I switched the credential_loop to have a shred keyword as it seems
best to default have the function use the same default as the standard
LibGit2 credential loop.

* challenge_prompt shows process output on ProcessError

* Display message about expected warning

* Code formatting fixes

base/libgit2/gitcredential.jl

@@ -297,7 +297,6 @@ function approve(cfg::GitConfig, cred::UserPasswordCredentials, url::AbstractStr
     end
 
     securezero!(git_cred)
-
     nothing
 end
 
@@ -310,7 +309,5 @@ function reject(cfg::GitConfig, cred::UserPasswordCredentials, url::AbstractStri
     end
 
     securezero!(git_cred)
-    securezero!(cred)
-
     nothing
 end

base/libgit2/types.jl

@@ -1239,7 +1239,6 @@ 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
@@ -1318,37 +1317,51 @@ function reset!(p::CredentialPayload, config::GitConfig=p.config)
 end
 
 """
-    approve(payload::CredentialPayload) -> Void
+    approve(payload::CredentialPayload; shred::Bool=true) -> Void
 
 Store the `payload` credential for re-use in a future authentication. Should only be called
 when authentication was successful.
+
+The `shred` keyword controls whether sensitive information in the payload credential field
+should be destroyed. Should only be set to `false` during testing.
 """
-function approve(p::CredentialPayload)
+function approve(p::CredentialPayload; shred::Bool=true)
     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)
+        shred = false  # Avoid wiping `cred` as this would also wipe the cached copy
     end
     if p.allow_git_helpers
         approve(p.config, cred, p.url)
     end
+
+    shred && securezero!(cred)
+    nothing
 end
 
 """
-    reject(payload::CredentialPayload) -> Void
+    reject(payload::CredentialPayload; shred::Bool=true) -> Void
 
 Discard the `payload` credential from begin re-used in future authentication. Should only be
 called when authentication was unsuccessful.
+
+The `shred` keyword controls whether sensitive information in the payload credential field
+should be destroyed. Should only be set to `false` during testing.
 """
-function reject(p::CredentialPayload)
+function reject(p::CredentialPayload; shred::Bool=true)
     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)
+        shred = false  # Avoid wiping `cred` as this would also wipe the cached copy
     end
     if p.allow_git_helpers
         reject(p.config, cred, p.url)
     end
+
+    shred && securezero!(cred)
+    nothing
 end

test/TestHelpers.jl

@@ -55,14 +55,14 @@ end
 
 function challenge_prompt(code::Expr, challenges; timeout::Integer=10, debug::Bool=true)
     output_file = tempname()
-    wrapped_code = """
-    result = let
-        $code
-    end
-    open("$output_file", "w") do fp
-        serialize(fp, result)
+    wrapped_code = quote
+        result = let
+            $code
+        end
+        open($output_file, "w") do fp
+            serialize(fp, result)
+        end
     end
-    """
     cmd = `$(Base.julia_cmd()) --startup-file=no -e $wrapped_code`
     try
         challenge_prompt(cmd, challenges, timeout=timeout, debug=debug)
@@ -116,11 +116,12 @@ function challenge_prompt(cmd::Cmd, challenges; timeout::Integer=10, debug::Bool
 
         # Process timed out or aborted
         if !success(p)
+            write(out, readavailable(master))
             if isready(done) && fetch(done) == :timed_out
                 error("Process timed out possibly waiting for a response. ",
                       format_output(out))
             else
-                Base.pipeline_error(p)
+                error("Failed process. ", format_output(out), "\n", p)
             end
         end
     end

test/libgit2-helpers.jl

@@ -14,7 +14,8 @@ function credential_loop(
         url::AbstractString,
         user::Nullable{<:AbstractString},
         allowed_types::UInt32,
-        payload::CredentialPayload)
+        payload::CredentialPayload;
+        shred::Bool=true)
     cb = Base.LibGit2.credentials_cb()
     libgitcred_ptr_ptr = Ref{Ptr{Void}}(C_NULL)
 
@@ -32,7 +33,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)
+            LibGit2.approve(payload, shred=shred)
             break
         end
 
@@ -48,34 +49,37 @@ function credential_loop(
         LibGit2.GitError(err)
     end
 
-    # Reject the credential when an authentication error occurs
+    # Reject and shred the credential when an authentication error occurs
     if git_error.code == LibGit2.Error.EAUTH
-        LibGit2.reject(payload)
+        LibGit2.reject(payload, shred=shred)
     end
 
-    return git_error, num_authentications
+    return git_error, num_authentications, payload
 end
 
 function credential_loop(
         valid_credential::UserPasswordCredentials,
         url::AbstractString,
         user::Nullable{<:AbstractString}=Nullable{String}(),
-        payload::CredentialPayload=DEFAULT_PAYLOAD)
-    credential_loop(valid_credential, url, user, 0x000001, payload)
+        payload::CredentialPayload=DEFAULT_PAYLOAD;
+        shred::Bool=true)
+    credential_loop(valid_credential, url, user, 0x000001, payload, shred=shred)
 end
 
 function credential_loop(
         valid_credential::SSHCredentials,
         url::AbstractString,
         user::Nullable{<:AbstractString}=Nullable{String}(),
-        payload::CredentialPayload=DEFAULT_PAYLOAD)
-    credential_loop(valid_credential, url, user, 0x000046, payload)
+        payload::CredentialPayload=DEFAULT_PAYLOAD;
+        shred::Bool=true)
+    credential_loop(valid_credential, url, user, 0x000046, payload, shred=shred)
 end
 
 function credential_loop(
         valid_credential::AbstractCredentials,
         url::AbstractString,
         user::AbstractString,
-        payload::CredentialPayload=DEFAULT_PAYLOAD)
-    credential_loop(valid_credential, url, Nullable(user), payload)
+        payload::CredentialPayload=DEFAULT_PAYLOAD;
+        shred::Bool=true)
+    credential_loop(valid_credential, url, Nullable(user), payload, shred=shred)
 end