Integrate Git credential helper support #23824

omus · 2017-09-22T18:19:05Z

Integrate Git credential helper support which allows HTTPS credentials to be cached similarly to SSH agent. The integration is optional and only is enabled if a user has specified helpers to use in their git configuration. Note that with this integration credential information entered in the Julia prompts can be saved and used by any tools which integration git credential helpers (e.g. git).

Additionally there exists a "osxkeychain" helper which would allow Julia to pull credentials from the OS X keychain.

Replaces #20725

omus · 2017-09-22T18:20:12Z

base/libgit2/gitcredential.jl

+    end
+
+    return Nullable{String}()
+end


I need to think of a better name for this function.

What about just helpers and username since you aren't exporting these methods and they make sense within the LibGit2 module?

My only issue is that those are good variable names.

gethelpers and getusername?

I renamed these to credential_helpers and default_username.

omus · 2017-09-22T18:20:37Z

base/libgit2/types.jl

+    value = unsafe_string(ce.value)
+
+    return (section, subsection, name, value)
+end


split is probably not the right name for this function. Suggestions welcome.

I think split makes sense. I'm imagining this working similar to urlsplit in python's urllib.

omus · 2017-09-22T18:22:14Z

test/libgit2.jl

+            success(`git --version`)
+        catch
+            false
+        end


I could revise these tests not to use git. That would involve making a custom helper just for these tests.

Hmmm, I don't think that's necessary as I'm guessing git is already required for building and testing julia. I'm guessing it should probably be explicitly listed in the README though?

git isn't a requirement for building Julia. I think you could just the GitHub "Download Zip" link to build Julia without git.

rofinn

This all seems pretty reasonable. I'd feel more comfortable approving this if julia had a SecureString type that handled the securezero! and finalization step for credential strings.

rofinn · 2017-09-22T20:21:49Z

base/libgit2/gitcredential.jl

+    end
+
+    return Nullable{String}()
+end


What about just helpers and username since you aren't exporting these methods and they make sense within the LibGit2 module?

rofinn · 2017-09-22T20:29:55Z

base/libgit2/types.jl

+    value = unsafe_string(ce.value)
+
+    return (section, subsection, name, value)
+end


I think split makes sense. I'm imagining this working similar to urlsplit in python's urllib.

rofinn · 2017-09-22T20:36:39Z

test/libgit2.jl

+            success(`git --version`)
+        catch
+            false
+        end


Hmmm, I don't think that's necessary as I'm guessing git is already required for building and testing julia. I'm guessing it should probably be explicitly listed in the README though?

rofinn · 2017-09-22T20:37:13Z

test/libgit2.jl

@@ -2001,7 +2194,8 @@ mktempdir() do dir
            function gen_ex(; username="git")
                quote
                    include($LIBGIT2_HELPER_PATH)
-                    payload = CredentialPayload(allow_ssh_agent=true, allow_prompt=false)
+                    payload = CredentialPayload(allow_prompt=false, allow_ssh_agent=true,
+                        allow_git_helpers=false)


Indentation.

rofinn · 2017-09-22T20:37:29Z

test/libgit2.jl

-                    payload = CredentialPayload($cred, allow_ssh_agent=$allow_ssh_agent,
-                        allow_prompt=$allow_prompt)
+                    payload = CredentialPayload($cred, allow_prompt=$allow_prompt,
+                        allow_ssh_agent=$allow_ssh_agent, allow_git_helpers=false)


Indentation.

omus · 2017-09-29T15:03:55Z

I'll be on holidays for the next two weeks. This PR should be ready to merge (besides some minor naming changes) and I'll add the 1.0 milestone to ensure this doesn't get forgotten about.

vtjnash · 2017-10-05T19:00:46Z

base/libgit2/gitcredential.jl

+    # Provide the helper with the credential information we know
+    write(input, cred)
+    write(input, "\n")
+    close(input)


t = @async close(input), then wait(t) after close(output)

vtjnash

need to avoid deadlock during process readwrite

StefanKarpinski · 2017-10-05T19:05:06Z

Should this be squashed?

omus · 2017-10-05T21:19:33Z

Definitely can be squashed. I can take care of the changes after I’m back from holidays.

Allows HTTPS credentials to be cached similarly to SSH agent. The integration is optional and only is enabled if a user has specified helpers to use in their git configuration. Note that with this integration credential information entered in the Julia prompts can be saved and used by any tools which integration git credential helpers (e.g. git). Additionally there exists a "osxkeychain" helper which would allow Julia to pull credentials from the OS X keychain. --- Track GitConfig in CredentialPayload In cases such as `fetch` or `push` this allows users of the payload to use the repo level configuration instead of just the global configuration. Add split for LibGit2.ConfigEntry Add GitCredential struct Fill GitCredential instances via helpers Switch to Nullable The Git credential protocol treats empty strings as valid. All fields of the GitCredential are Nullable to be able to distinguish between missing and empty. Integrate Git credential helper support Needed to disable git credential helpers for most tests as this could cause test credentials to get written to the user's credential store. More GitCredentialHelper tests Empty test

omus · 2017-10-23T16:40:00Z

base/libgit2/gitcredential.jl

+            Base.warn_once("Resetting the helper list is currently unsupported: " *
+                 "ignoring all git credential helpers.")
+            return GitCredentialHelper[]
+        end


Due to a bug with libgit2 the configuration iterator may not return results in the order in which they were written in the configuration file. For credential helpers this is only an issue when trying to use the special empty string helper which indicates that all previous helpers should be ignored.

See: libgit2/libgit2#4361

Note that this warning does show up when running the tests.

omus · 2017-10-23T16:41:09Z

Addressed comments, rebased, and squashed. @vtjnash can you verify that the deadlock has been addressed?

vtjnash · 2017-10-24T12:13:59Z

Yep, looks good.

omus · 2017-10-26T20:00:14Z

Will merge soon.

omus added the libgit2 The libgit2 library or the LibGit2 stdlib module label Sep 22, 2017

omus commented Sep 22, 2017

View reviewed changes

rofinn approved these changes Sep 22, 2017

View reviewed changes

omus added this to the 1.0 milestone Sep 29, 2017

JeffBezanson added the triage This should be discussed on a triage call label Sep 29, 2017

vtjnash reviewed Oct 5, 2017

View reviewed changes

vtjnash requested changes Oct 5, 2017

View reviewed changes

JeffBezanson removed the triage This should be discussed on a triage call label Oct 5, 2017

omus force-pushed the cv/git-credential-helpers branch from 111bc48 to 9b52df7 Compare October 23, 2017 16:36

omus changed the title ~~RFC: Integrate Git credential helper support~~ Integrate Git credential helper support Oct 23, 2017

omus commented Oct 23, 2017

View reviewed changes

omus merged commit 1486ba9 into master Oct 26, 2017

omus mentioned this pull request Oct 26, 2017

WIP: LibGit2 support for Git credential helpers #20725

Closed

5 tasks

StefanKarpinski deleted the cv/git-credential-helpers branch October 26, 2017 23:35

omus mentioned this pull request Oct 27, 2017

User experience improvements for Git credential helpers #24364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Git credential helper support #23824

Integrate Git credential helper support #23824

omus commented Sep 22, 2017

omus Sep 22, 2017

rofinn Sep 22, 2017

omus Sep 23, 2017

rofinn Sep 23, 2017

omus Oct 23, 2017

omus Sep 22, 2017

rofinn Sep 22, 2017

omus Sep 22, 2017

rofinn Sep 22, 2017

omus Sep 28, 2017

rofinn left a comment

rofinn Sep 22, 2017

rofinn Sep 22, 2017

rofinn Sep 22, 2017

rofinn Sep 22, 2017

rofinn Sep 22, 2017

omus commented Sep 29, 2017

vtjnash Oct 5, 2017

omus Oct 5, 2017

vtjnash left a comment

StefanKarpinski commented Oct 5, 2017

omus commented Oct 5, 2017

omus Oct 23, 2017

omus commented Oct 23, 2017

vtjnash commented Oct 24, 2017

omus commented Oct 26, 2017

Integrate Git credential helper support #23824

Integrate Git credential helper support #23824

Conversation

omus commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash left a comment

Choose a reason for hiding this comment

StefanKarpinski commented Oct 5, 2017

omus commented Oct 5, 2017

Choose a reason for hiding this comment

omus commented Oct 23, 2017

vtjnash commented Oct 24, 2017

omus commented Oct 26, 2017