Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement getpasswd, fix up github two-factor authentication #8228

Closed
kmsquire opened this issue Sep 4, 2014 · 30 comments
Closed

Implement getpasswd, fix up github two-factor authentication #8228

kmsquire opened this issue Sep 4, 2014 · 30 comments
Assignees
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request needs tests Unit tests are required for this change packages Package management and loading
Milestone

Comments

@kmsquire
Copy link
Member

kmsquire commented Sep 4, 2014

#5252 #6668 added support for github two-factor authentication. However, the implementation is a bit ugly: in the worst case, the user has to enter her password 4 times(!).

The nicest way to handle this would probably be to implement a getpasswd (or getpass) function, which reads in a password without echoing, and then passes that onto curl via curl -K - (which reads a config file from stdin).

@kmsquire kmsquire changed the title Implement getpasswd, fix up two-factor authentication Implement getpasswd, fix up github two-factor authentication Sep 4, 2014
@stevengj
Copy link
Member

stevengj commented Sep 4, 2014

The Python getpass function may be helpful. There is also getpass.c from glibc, and a thread on stackoverflow.

(We should certainly be able to put together a pure-Julia implementation, but it would be good to take a quick look at other implementations in case there are subtleties.)

@stevengj
Copy link
Member

stevengj commented Sep 4, 2014

Note that we shouldn't keep the password in memory longer than necessary; probably better to be paranoid and to overwrite the string buffer with zeros when we are done; one also needs to be a bit careful in the getpass implementation to avoid making extra copies of the data.

@StefanKarpinski
Copy link
Member

@Keno, what's the best way to do prompting with the REPL? I'm assuming we don't want a new REPL mode for this.

@Keno
Copy link
Member

Keno commented Sep 4, 2014

Well, we should implement a simple "prompt" function, but basically what you do is construct a Prompt object and then pass it to prompt! with the currently active terminal.

@StefanKarpinski
Copy link
Member

Hmm. I'm messing with this and my need to refactor is getting strong...

@quinnj
Copy link
Member

quinnj commented Sep 4, 2014

How does this differ from readline(STDIN)? Or is this on the repl side of things, I guess? In any case, does this make it easier to read from the prompt in IJulia?

@StefanKarpinski
Copy link
Member

It's quite possible that STDIN is coming from something other than the terminal. Ideally, this should be nicely integrated with our line editing and allow backspace to work, etc.

@stevengj
Copy link
Member

stevengj commented Sep 5, 2014

readline(STDIN) works in IJulia, but that is about it; IPython is fairly limited in how much we can emulate standard input.

However, password input requires a bit different interface, because you don't want the password to appear as it is typed. And sending passwords via IJulia requires even more thought about security; I wouldn't recommend it for now.

@tkelman
Copy link
Contributor

tkelman commented Oct 22, 2015

I think this should be reopened at PkgDev.jl. @wildart @jakebolewski how much of this is already functioning?

@stevengj
Copy link
Member

A getpasswd function might be more generally useful than just PkgDev.

@wildart
Copy link
Member

wildart commented Oct 22, 2015

I use libc getpass in libgit2/utils.jl.

@stevengj
Copy link
Member

The man page for getpass says "This function is obsolete. Do not use it." It also doesn't exist on Windows as far as I can tell. Python uses their own implementation, apparently.

@tkelman
Copy link
Contributor

tkelman commented Oct 23, 2015

Does newer libuv maybe have something for this?

@wildart
Copy link
Member

wildart commented Oct 23, 2015

I think it is easier to write pure julia implementation.

@tkelman
Copy link
Contributor

tkelman commented Oct 23, 2015

It also doesn't exist on Windows as far as I can tell.

It does not. @wildart please be more careful to not assume that a posix libc is present. Especially in interactive code that does not get tested on CI.

@davidanthoff
Copy link
Contributor

Can this please get a 0.5 label? Until this is fixed, there is a major regression for windows users, i.e. that they can't work with private package repos at all.

@Keno Keno added this to the 0.5.0 milestone Apr 26, 2016
@Keno Keno self-assigned this Apr 26, 2016
@tkelman tkelman added the needs tests Unit tests are required for this change label Apr 26, 2016
@Keno
Copy link
Member

Keno commented Apr 26, 2016

Somebody with access to a windows machine please test the following:

diff --git a/base/libgit2/utils.jl b/base/libgit2/utils.jl
index b0c841b..66a9ab7 100644
--- a/base/libgit2/utils.jl
+++ b/base/libgit2/utils.jl
@@ -11,13 +11,20 @@ end

 isset(val::Integer, flag::Integer) = (val & flag == flag)

+using Base.Terminals: TTYTerminal, raw!
 function prompt(msg::AbstractString; default::AbstractString="", password::Bool=false)
-    msg = !isempty(default) ? msg*" [$default]:" : msg*":"
-    uinput = if password
-        bytestring(ccall(:getpass, Cstring, (Cstring,), msg))
+    msg = !isempty(default) ? msg*" [$default]: " : msg*": "
+    print(msg)
+    if password
+        t = TTYTerminal("xterm", STDIN, STDOUT, STDERR)
+        raw!(t, true)
+        try
+            uinput = rstrip(readuntil(STDIN,'\r'),'\r')
+        finally
+            raw!(t, false)
+        end
     else
-        print(msg)
-        chomp(readline(STDIN))
+        uinput = chomp(readline(STDIN))
     end
     isempty(uinput) ? default : uinput
 end

Tests for this are nontrivial, since it depends on the exact state of the tty which we can't really emulate.

@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2016

Don't we want this not to echo what gets typed?

@Keno
Copy link
Member

Keno commented Apr 26, 2016

Yes, that's what happens in raw mode.

@Keno
Copy link
Member

Keno commented Apr 26, 2016

At least on unix.

@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2016

Raw mode isn't really working on Windows then. And one or both of the \r above would have to be a \n to work in mintty.

@stevengj
Copy link
Member

Can we just use _getch on Windows?

The more I look into this, the more I think we should just use getpass on Unix systems. It's been marked as "obsolete" for over a decade, but since there is no simple alternative function it is still widely used. If it ever disappears we can re-implement it, but why bother now?

@stevengj
Copy link
Member

stevengj commented Apr 26, 2016

The following works for me on Mac, GNU/Linux, and Windows, though it would probably be good to handle the backspace key and other niceties on Windows:

@windows_only function getpass(prompt::AbstractString)
    print(prompt)
    p = UInt8[]
    sizehint!(p, 128)
    while true
        c = ccall(:_getch, Cint, ())
        c < 16 && break
        push!(p, c)
    end
    s = bytestring(p)
    fill!(p, 0)
    return s
end
@unix_only getpass(prompt::AbstractString) = pointer_to_string(ccall(:getpass, Cstring, (Cstring,), prompt), true)

@tkelman
Copy link
Contributor

tkelman commented Apr 27, 2016

That sounds like it'll be good enough for now. Certainly better than trying to ccall something that doesn't exist.

@stevengj
Copy link
Member

stevengj commented Apr 27, 2016

Here's a somewhat improved version, that handles backspace but ignores other control characters as well as arrow and function keys:

@windows_only function getpass(prompt::AbstractString)
    print(prompt)
    flush(STDOUT)
    p = Array(UInt8, 128) # mimic Unix getpass in ignoring more than 128-char passwords
                          # (also avoids any potential memory copies arising from push!)
    plen = 0
    while true
        c = ccall(:_getch, UInt8, ())
        if c == 0xff || c == UInt8('\n') || c == UInt8('\r')
            break # EOF or return
        elseif c == 0x00 || c == 0xe0
            ccall(:_getch, UInt8, ()) # ignore function/arrow keys
        elseif c == UInt8('\b') && plen > 0
            plen -= 1 # delete last character on backspace
        elseif !iscntrl(Char(c)) && plen < 128
            p[plen += 1] = c
        end
    end
    s = bytestring(pointer(p), plen)
    fill!(p, 0) # don't leave password in memory
    return s
end

@unix_only getpass(prompt::AbstractString) = pointer_to_string(ccall(:getpass, Cstring, (Cstring,), prompt), true)

There is also a _getwch function that returns a wint_t, but it appears to be useless: I tried it out, and not only does it seem to ignore Unicode characters, but it mimics _getch in using 0xe0 to indicate an arrow key (making the latter indistinguishable from U+00e0). In consequence, I think we can only support ASCII passwords on Windows.

@vtjnash
Copy link
Member

vtjnash commented Apr 27, 2016

Raw mode isn't really working on Windows then

i find that hard to believe since we depend on it for the REPL to print correctly

@tkelman
Copy link
Contributor

tkelman commented Apr 27, 2016

Ah, apologies, it looks like Keno's code does work when built into the sysimg but not when called from the repl. My bad.

@wildart
Copy link
Member

wildart commented May 1, 2016

I wonder if xterm is installed on every platform or distro by default.

@nalimilan
Copy link
Member

I wonder if xterm is installed on every platform or distro by default.

Just tried it, it's not even installed on my Fedora 23 box (on which I installed all kinds of packages over the years). So no.

@stevengj
Copy link
Member

stevengj commented May 6, 2016

Note that one of the things that we have to be very careful of is accidentally leaving extra copies of the password in memory. e.g. @Keno's version calls readuntil (which calls jl_readuntil), and it looks like jl_readuntil can easily make extra copies.

getpass implementations presumably go to more care in this regard. I tried to be careful in my Windows implementation above, too, although we should probably wrap it in a try/finally block to ensure that fill! is called... we certainly need to manually manage any buffers here.

Also, getpass has the nice feature of displaying a snazzy "key" icon in the prompt, at least on MacOS:
image
😏

wildart added a commit to wildart/julia that referenced this issue May 19, 2016
- handle https & ssh connections
- new credential types

added caching of credentials in `update`
added cross-platform `getpass` implementation [JuliaLang#8228]
added comments and documentation
show last saved value for pub/priv key path instead of default key path
move `getpass` to `Base`
introduced SSH credential type for cached credentials
changed PK variable to `SSH_KEY_PATH`, added PbK guessing from PK
wildart added a commit to wildart/julia that referenced this issue May 20, 2016
- handle https & ssh connections
- new credential types

added caching of credentials in `update`
added cross-platform `getpass` implementation [JuliaLang#8228]
added comments and documentation
show last saved value for pub/priv key path instead of default key path
move `getpass` to `Base`
introduced SSH credential type for cached credentials
changed PK variable to `SSH_KEY_PATH`, added PbK guessing from PK
@vtjnash vtjnash closed this as completed May 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request needs tests Unit tests are required for this change packages Package management and loading
Projects
None yet
Development

No branches or pull requests