Support using EOF char (^D) to abort credential prompt #23092

omus · 2017-08-02T16:46:58Z

Part of #20725.

Supports using EOF to abort the credential prompt. This is important as newlines can be used to select the default value.

Note that for password prompts it is currently not possible to tell the difference between an EOF and a newline. The PR behaviour is to assume an EOF was given.

Additional changes include:

Deprecate LibGit2.prompt(::AbstractString) -> String with Base.prompt(::AbstractString) -> Nullable{String}
Lots of tests cases relating to using newline / EOF in the credential prompts (possible now that we have a reliable way of aborting)
Introduces LibGit2.user_abort function which is only meant for internal use.
An empty SSH username no longer aborts (replaced by EOF abort)
An empty SSH username_ptr string now triggers a prompt (previously would abort)
Test function credential_loop can now distinguish between "" and C_NULL.

omus · 2017-08-02T16:52:54Z

Note the additional tests produce more of the warnings:

WARNING: readuntil(IO,AbstractString) will perform poorly with a long string

The warnings will go away once #20621 is merged.

tkelman · 2017-08-03T17:57:25Z

base/libgit2/utils.jl

+        isempty(uinput) && return Nullable{String}()  # Encountered EOF
+        uinput = chomp(uinput)
+    end
+    Nullable{String}(isempty(uinput) ? default : uinput)


is anything externally (PkgDev maybe) using this function that will notice the API change to returning a Nullable?

Good catch. This is used by PkgDev.

What's the recommended procedure for deprecating a function's return type? Should I deprecate prompt(::AbstractString) and introduce the following:

prompt(::Type{Nullable{String}}, msg::AbstractString; default::AbstractString="", password::Bool=false)

I suppose that'll work, it's not the prettiest or most concise but it would do.

So we would deprecate prompt(::AbstractString)::AbstractString for Julia 0.7 and introduce prompt(::Type{Nullable{String}}, ::AbstractString).

For Julia 1.0 we would deprecate prompt(::Type{Nullable{String}}, ::AbstractString) in favor of prompt(::AbstractString)::Nullable{String}? I guess we could do this after 1.0.

I came up with a better plan. Since this function isn't LibGit2 specific I'll moved this function into Base. That way we can have Base.prompt and the deprecated LibGit2.prompt without having to change the method parameters.

tkelman · 2017-08-03T17:59:09Z

base/libgit2/callbacks.jl

+                    response = prompt("Passphrase for $privatekey", password=true)
+                    isnull(response) && return user_abort()
+                    passdef = unsafe_get(response)
+                    isempty(passdef) && return user_abort()  # Ambiguous if EOF or newline


is there still a way to specify / use passwordless keys? (likewise for the other "Ambiguous" comment)

When using ssh-keygen a passwordless key wouldn't pass the passphrase_required check.

For username/password I don't think LibGit2 will hit this callback unless it requires a password. I'll try to come up with an example for this.

I did some digging into this.

For SSH having a empty passphrase is considered as not having any passphrase at all. Since we can tell the difference between SSH keys that do and do not require passphrases (encrypted private keys have second line as "Proc-Type: 4,ENCRYPTED") and only prompt when a private key is encrypted it should be safe to abort if a empty passphrase is entered at the prompt.

For HTTP I ended up doing some experimentation with httpbin and using empty passwords for both HTTP Basic and Digest authentication. Both seem to consider an empty password invalid and will re-prompt for credentials.

tkelman · 2017-08-03T18:09:05Z

test/libgit2.jl

+                @test err == 0
+                @test auth_attempts == 1
+            end
+
            # TODO: Tests are currently broken. Credential callback prompts for:


are you fixing this entire TODO or only part of it?

This TODO is done and should be removed.

tkelman · 2017-08-03T18:11:15Z

test/libgit2.jl

+            withenv("SSH_KEY_PATH" => nothing,
+                    "SSH_PUB_KEY_PATH" => nothing,
+                    "SSH_KEY_PASS" => nothing,
+                    (Sys.iswindows() ? "USERPROFILE" : "HOME") => tempdir()) do


is this relying on tempdir happening to not have any key files in it? probably unlikely, but never know

I specifically need to set the HOME to be a directory that doesn't contain the file ".ssh/id_rsa". I'll add a comment and a test ensuring that this is the case.

I added this check and the comment.

As part of the supporting EOF update (#23092) we needed to deprecate the original `LibGit2.prompt` since the return type changed. Since packages like PkgDev make use of the `prompt` function and the function itself isn't LibGit2 specific it seemed best to move it into Base. This allows us to deprecate the original `prompt` without having to change the function name or modify the parameters.

omus · 2017-08-04T13:54:05Z

CI failure looks unrelated. I updated the PR description to mention all changes.

As part of the supporting EOF update (#23092) we needed to deprecate the original `LibGit2.prompt` since the return type changed. Since packages like PkgDev make use of the `prompt` function and the function itself isn't LibGit2 specific it seemed best to move it into Base. This allows us to deprecate the original `prompt` without having to change the function name or modify the parameters.

omus · 2017-08-06T01:52:21Z

Rebased changes to fix conflicts. Needed to update the LibGit2 test where the user provides an empty private key filename at the prompt (#23108).

diff --git a/test/libgit2.jl b/test/libgit2.jl
--- a/test/libgit2.jl
+++ b/test/libgit2.jl
@@ -1676,16 +1676,15 @@ mktempdir() do dir
                 @test err == abort_prompt
                 @test auth_attempts == 1

-                # User provides an empty private key
-                # TODO: Should trigger a re-prompt.
+                # User provides an empty private key which triggers a re-prompt
                 challenges = [
                     "Private key location for '[email protected]':" => "\n",
                     "Public key location for '[email protected]' [.pub]:" => "\n",
-                    "Passphrase for :" => "\n",
+                    "Private key location for '[email protected]':" => "\x04",
                 ]
                 err, auth_attempts = challenge_prompt(ssh_cmd, challenges)
                 @test err == abort_prompt
-                @test auth_attempts == 1
+                @test auth_attempts == 2
             end

Without this an empty string in the `username_ptr` will never prompt the user for a new username. Has not been seen in the wild.

As part of the supporting EOF update (#23092) we needed to deprecate the original `LibGit2.prompt` since the return type changed. Since packages like PkgDev make use of the `prompt` function and the function itself isn't LibGit2 specific it seemed best to move it into Base. This allows us to deprecate the original `prompt` without having to change the function name or modify the parameters.

omus · 2017-08-08T15:19:08Z

Rebased again to deal with conflicts from #23040. Planning to merge this tomorrow.

omus added the libgit2 The libgit2 library or the LibGit2 stdlib module label Aug 2, 2017

tkelman reviewed Aug 3, 2017

View reviewed changes

omus force-pushed the cv/prompt-eof branch from 7a5c3ec to 254dd20 Compare August 4, 2017 04:10

omus force-pushed the cv/prompt-eof branch from 254dd20 to a534388 Compare August 6, 2017 01:51

omus added 4 commits August 8, 2017 09:57

Support using of EOF char (^D) to abort prompt

e3e9b94

Revise variable name

9abef10

Prompt when username pointer contains empty string

ec010ad

Without this an empty string in the `username_ptr` will never prompt the user for a new username. Has not been seen in the wild.

omus force-pushed the cv/prompt-eof branch from a534388 to f26ee56 Compare August 8, 2017 15:18

omus merged commit 3678816 into master Aug 9, 2017

omus deleted the cv/prompt-eof branch August 9, 2017 10:26

DilumAluthge mentioned this pull request Jan 2, 2022

[docs] Add an example of doing str = readline() to read a single line in from stdin #43635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support using EOF char (^D) to abort credential prompt #23092

Support using EOF char (^D) to abort credential prompt #23092

omus commented Aug 2, 2017 •

edited

Loading

omus commented Aug 2, 2017

tkelman Aug 3, 2017

omus Aug 3, 2017

tkelman Aug 3, 2017

omus Aug 3, 2017

omus Aug 4, 2017

tkelman Aug 3, 2017 •

edited

Loading

omus Aug 3, 2017

omus Aug 4, 2017

tkelman Aug 3, 2017

omus Aug 3, 2017

tkelman Aug 3, 2017

omus Aug 3, 2017

omus Aug 4, 2017

omus commented Aug 4, 2017

omus commented Aug 6, 2017

omus commented Aug 8, 2017

Support using EOF char (^D) to abort credential prompt #23092

Support using EOF char (^D) to abort credential prompt #23092

Conversation

omus commented Aug 2, 2017 • edited Loading

omus commented Aug 2, 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

tkelman Aug 3, 2017 • edited Loading

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 Aug 4, 2017

omus commented Aug 6, 2017

omus commented Aug 8, 2017

omus commented Aug 2, 2017 •

edited

Loading

tkelman Aug 3, 2017 •

edited

Loading