Final LibGit2 credential callback changes #23770

omus · 2017-09-19T17:03:51Z

Should be the last round of bugfixes and minor features to the LibGit2 credential callback system before I move onto integrating git credential helpers. There are quite a few small changes in this PR including:

CredentialPayload's credential field is now used a temporary storage. We can safely do this without accidentally modifying cached credentials because of LibGit2 credential approval/rejection #23711
Load credential information from ENV only once. Allows future iterations to use user input as defaults
Created new isfilled function which verifies a credential is ready to be used in authentication
Introduced modified check which allows us to abort when all authentication methods have failed
Re-prompting for an SSH key will always ask for the private key to allow the user to change it
Introduced a way to prompt for the public key when set to an invalid file
All previously broken/commented out callback tests are now enabled
Limit number of user prompts to 3
Support ~ in SSH prompts for private key and public key

Additionally, at this point I think the credential_loop tests cover our testing of the SSH code paths. The ancient "SSH" testset has been removed which fixes #18403, #18306.

Part of #20725.

omus · 2017-09-19T19:35:13Z

Interesting, this code is no longer broken on Windows:

if Sys.iswindows() && LibGit2.version() >= v"0.26.0"
    # see #22681 and https://github.com/libgit2/libgit2/pull/4055
    @test_broken ex.code == LibGit2.Error.EAUTH
    @test ex.code == LibGit2.Error.ERROR
else
    @test ex.code == LibGit2.Error.EAUTH
end

I believe this may have been caused by the changes in "Set error message when auth fails" which now always sets the error message. It's possible that the LibGit2 v0.26.0 update would modify the error code if no new error message was set (working theory).

rofinn

I don't see any obvious logical issues with the code, just some style and organizational suggestions.

rofinn · 2017-09-19T19:52:42Z

base/libgit2/callbacks.jl

            response = Base.prompt("Private key location for '$prompt_url'",
-                default=privatekey)
+                default=creds.prvkey)


Same indentation issues as previous PR.

I've addressed this by renaming prompt_url to just url which means I can fit this keyword on a single line.

rofinn · 2017-09-19T20:30:47Z

base/libgit2/callbacks.jl

+        p.remaining_prompts <= 0 && return prompt_limit()
+    end
+
+    if !modified


Couldn't you just move this into the if-block above (e.g., elseif !modified)? That might make the code easier to follow.

I purposefully did this as I can see the if above turning into a while loop. Originally this is what was done in #20725.

rofinn · 2017-09-19T22:17:13Z

base/libgit2/callbacks.jl

 function authenticate_ssh(libgit2credptr::Ptr{Ptr{Void}}, p::CredentialPayload, username_ptr)
    creds = Base.get(p.credential)::SSHCredentials
+    modified = false


Some of these modified changes seem like they could simplified (e.g., You could probably just have modified = p.first_pass && isfilled(creds) at the beginning).

It may be better if I drop modified and just do a comparison between the initial credentials and the current credentials. I'll do some checks.

My idea of doing the comparison won't work. If a user were to enter the same invalid credentials twice in a row they would get an "All authentication methods have failed" error. The name modified isn't quite the right term as this just verifying that a new authentication method has been attempted.

I reworked this a little bit. I've ended up changing modified to revised. I think this term is better suited for the current behaviour. As for revised = p.first_pass && isfilled(creds) I've decided to keep the if block as it matches the way revised is used in the reset of the code.

rofinn · 2017-09-19T22:27:17Z

base/libgit2/callbacks.jl

+        if Sys.iswindows()
+            response = Base.winprompt(
+                "Please enter your credentials for '$prompt_url'", "Credentials required",
+                isempty(creds.user) ? p.username : creds.user; prompt_username=true)


I'd be inclined to move the ternary out of the if-block and put the remaining code into an user_auth_prompt function.

rofinn · 2017-09-19T22:30:11Z

base/libgit2/callbacks.jl

        end

-        if isempty(passphrase) && is_passphrase_required(privatekey)
+        # Ask for a passphrase when the private key exists and requires a passphrase
+        if isempty(creds.pass) && is_passphrase_required(creds.prvkey)
            if Sys.iswindows()
                response = Base.winprompt(


Move prompt logic into ssh_auth_prompt?

Separating these into new functions hasn't turned out well. I've decided to stick with the status quo.

omus · 2017-09-20T19:49:12Z

Ok, I worked my way through the AppVeyor issue. As it turns out the commit "Error when credentials are not modified" corrects the broken test. In this commit we throw a EAUTH error from the callback when the credentials are not filled. I'll update the libgit2-online tests to reflect this but I also think it could be reasonable to remove the "with empty credentials" test.

The temporary credential object is independent from any cached or stored credential until it is explicitly approved.

Credentials should be used when they are filled in and alternatives should be tried when they are not. These changes allow use to enable the commented out tests which use invalid credentials. Note when prompting is disabled we now attempt to authenticate using the default ~/.ssh/id_rsa key.

Previously if the public key file exists and the private key was correct we could get into a scenario where we would never prompt for the public key to be changed.

Note the prompt limit is an EAUTH error since hitting the prompt limit should cause saved credential to be rejected.

omus · 2017-09-21T18:47:33Z

During the investigation on changing how modified worked I discovered a corner case where if prompting was disabled, ssh agent had failed, and no ENV variables were set we would fail to try the default ~/.ssh/id_rsa key. I've added a test to ensure that the default SSH key will be used without prompting.

omus · 2017-09-21T18:47:45Z

The PR should be ready to go now. I think this should be merged rather than squashed to keep the history understandable. I've run the LibGit2 tests locally on each commit.

omus · 2017-09-21T21:58:20Z

Found a minor issue with the LibGit2 library I'll patch yet.

Without setting an error message the message will be whatever error message came before.

The test has been superseded by all of the new credential_loop tests.

According to the LibGit2 authentication guide (https://libgit2.github.com/docs/guides/authentication/) the callback is suppose to be retried if the server doesn't accept the credentials. In the case where we use SSH and the passphrase is invalid LibGit2 would just give a generic: GitError(Code:ERROR, Class:SSH, Failed to authenticate SSH session: Callback returned error)

Allows the user to confirm they want to use the default private key. During development it was noticed that the username was only being filled in if prompting was allowed.

omus · 2017-09-22T04:53:05Z

After digging into the LibGit2 issue the problem turned out to bug where the credential callback loop is aborted if the SSH passphrase is incorrect. Addressing this now makes Pkg.update much better as now you can accidentally type your SSH passphrase wrong and get re-prompted right away.

Additionally, after doing more experimenting with Pkg.update there was an issue where cached credentials were being ignored when the SSH agent was enabled. Now that these are all addressed Pkg.update authentication finally works the way I always wanted it to.

omus · 2017-09-22T13:45:57Z

PR for the LibGit2 patch: libgit2/libgit2#4357

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

omus mentioned this pull request Sep 19, 2017

Experiment for #23770 (Final LibGit2 credential callback changes) #23776

Closed

rofinn approved these changes Sep 19, 2017

View reviewed changes

omus added 9 commits September 20, 2017 22:30

Rename internal variable creds to cred

bc7b824

Save modifications directory to temp credential

3894ef1

The temporary credential object is independent from any cached or stored credential until it is explicitly approved.

Create use_env check

5d10ce4

Create isfilled function

822b45e

Prompt for public key when invalid

d6e0c5b

Previously if the public key file exists and the private key was correct we could get into a scenario where we would never prompt for the public key to be changed.

Add prompt limit

3f5b1f6

Note the prompt limit is an EAUTH error since hitting the prompt limit should cause saved credential to be rejected.

Remove path from CredentialPayload

bfda7d9

Incorporate expanduser

3b63e61

omus force-pushed the cv/libgit2-one-more-thing branch from 062b293 to 597ce0b Compare September 21, 2017 18:47

omus added 9 commits September 21, 2017 22:39

Set error message when auth fails

e9940f8

Without setting an error message the message will be whatever error message came before.

Remove old unused SSH testset

b26c2ca

The test has been superseded by all of the new credential_loop tests.

Add additional docstrings

22ed0ae

Formatting fix

d1084ec

Add SSH id_rsa test

e55b9a6

Additional comments

3f97c24

Skip SSH agent when using cached credentials

773d226

Prompt for private if SSH_KEY_PATH unset

23d2fd2

Allows the user to confirm they want to use the default private key. During development it was noticed that the username was only being filled in if prompting was allowed.

omus force-pushed the cv/libgit2-one-more-thing branch from 597ce0b to 23d2fd2 Compare September 22, 2017 04:47

omus merged commit def987e into master Sep 22, 2017

StefanKarpinski deleted the cv/libgit2-one-more-thing branch September 22, 2017 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Final LibGit2 credential callback changes #23770

Final LibGit2 credential callback changes #23770

omus commented Sep 19, 2017

omus commented Sep 19, 2017 •

edited

Loading

rofinn left a comment

rofinn Sep 19, 2017

omus Sep 21, 2017

rofinn Sep 19, 2017

omus Sep 20, 2017

rofinn Sep 19, 2017

omus Sep 20, 2017

omus Sep 21, 2017

omus Sep 21, 2017

rofinn Sep 19, 2017

rofinn Sep 19, 2017

omus Sep 21, 2017

omus commented Sep 20, 2017

omus commented Sep 21, 2017

omus commented Sep 21, 2017

omus commented Sep 21, 2017

omus commented Sep 22, 2017

omus commented Sep 22, 2017

Final LibGit2 credential callback changes #23770

Final LibGit2 credential callback changes #23770

Conversation

omus commented Sep 19, 2017

omus commented Sep 19, 2017 • edited Loading

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

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 20, 2017

omus commented Sep 21, 2017

omus commented Sep 21, 2017

omus commented Sep 21, 2017

omus commented Sep 22, 2017

omus commented Sep 22, 2017

omus commented Sep 19, 2017 •

edited

Loading