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

RFC: Enable github two-factor authentication; fixes #5252 #6668

Merged
merged 2 commits into from Sep 4, 2014

Conversation

kmsquire
Copy link
Member

I was getting annoyed at having to disable two-factor authentication to run Pkg.publish(). Here's an attempt at fixing that.

julia> Pkg.publish()
INFO: Validating METADATA
INFO: No new package versions to publish
INFO: Submitting METADATA changes
INFO: Forking JuliaLang/METADATA.jl to kmsquire
Enter host password for user 'kmsquire':
INFO: Two-factor authentication in use.  Enter auth code.  (You may need to re-enter your password.)
Authentication code: 420442
Enter host password for user 'kmsquire':

INFO: Pushing changes as branch pull-request/5068ad74
INFO: To create a pull-request open:

  https://github.com/kmsquire/METADATA.jl/compare/pull-request/5068ad74

Remaining issues:

  1. Because there are two calls to curl, we have to enter our password twice. (minor)
    2. For the second password prompt, enter has to be pressed twice(!) (major)
  2. Adding more than one auth token does not seem to be possible. For an existing token and a new machine, I get:
    ERROR: 422: Validation Failed -- https://developer.github.com/v3/oauth_authorizations/#create-a-new-authorization
       resource:   OauthAccess
       field:      description
       error code: already_exists

Regarding 2, which is the main annoyance, I'm stumped. I'm guessing some interaction between the repl and the spawned process is not letting curl see one of the return characters. Or maybe running the same process twice is causing issues.

Edit: I just had to reauthenticate, and found that on latest master, this is no longer a problem (or at least it worked this time without having to press enter twice.

Since authenticating twice seems like a minor price to pay for having two-factor auth enabled, I think this can be merged (although I would appreciate someone else testing).

Cc: @StefanKarpinski, @aviks

@kmsquire kmsquire changed the title WIP: Enable github two-factor authentication; fixes #5252 RFC: Enable github two-factor authentication; fixes #5252 Aug 18, 2014
@kmsquire
Copy link
Member Author

@aviks, when you have the chance, would you be able to test this PR?

@aviks
Copy link
Member

aviks commented Aug 18, 2014

Ah, yes, will do imminently.

@Keno
Copy link
Member

Keno commented Aug 19, 2014

Can we include the host name in the description of the token, so we can have it on different machines? That must work somehow since hub does it.

@StefanKarpinski
Copy link
Sponsor Member

That would be amazing, but I have no idea how to do it. Where did you find documentation on this?

@Keno
Copy link
Member

Keno commented Aug 19, 2014

Actually you can just share the token (haven't tested yet, but will submit as PR after I have). I did verify using curl that it can work though. Sorry @kmsquire if this adds more work to this PR.

diff --git a/base/pkg/github.jl b/base/pkg/github.jl
index 5617e51..dffbe79 100644
--- a/base/pkg/github.jl
+++ b/base/pkg/github.jl
@@ -2,9 +2,10 @@ module GitHub

 import Main, ..Git, ..Dir

+const AUTH_NOTE = "Julia Package Manager"
 const AUTH_DATA = {
     "scopes" => ["repo"],
-    "note" => "Julia Package Manager",
+    "note" => AUTH_NOTE,
     "note_url" => "http://docs.julialang.org/en/latest/manual/packages/",
 }

@@ -45,12 +46,30 @@ curl(url::String, data::Nothing, opts::Cmd=``) = curl(url,opts)
 curl(url::String, data, opts::Cmd=``) =
     curl(url,`--data $(sprint(io->json().print(io,data))) $opts`)

+function update_token(args)
+    tokfile = Dir.path(".github","token")
+    Base.rm(tokfile)
+    token()
+end
+
 function token(user::String=user())
     tokfile = Dir.path(".github","token")
     isfile(tokfile) && return strip(readchomp(tokfile))
     status, content = curl("https://api.github.com/authorizations",AUTH_DATA,`-u $user`)
-    (status != 401 && status != 403) || error("$status: $(json().parse(content)["message"])")
-    tok = json().parse(content)["token"]
+    if status == 422
+        if json().parse(content)["errors"][1]["code"] == "already_exists"
+            info("Retrieving existing GitHub token (you may have to enter you password again)")
+            status,content = curl("https://api.github.com/authorizations",`-u $user`)
+            (status >= 400) && error("$status: $(json().parse(content)["message"])")
+            for entry in json().parse(content)
+                if entry["note"] == AUTH_NOTE
+                    tok = entry["token"]
+                end
+            end
+        end
+    else
+        (status != 401 && status != 403) || error("$status: $(json().parse(content)["message"])")
+        tok = json().parse(content)["token"]
+    end
     mkpath(dirname(tokfile))
     open(io->println(io,tok),tokfile,"w")
     return tok
@@ -85,6 +104,7 @@ end

 function fork(owner::String, repo::String)
     status, response = POST("repos/$owner/$repo/forks")
+    status == 401 && update_token() && POST("repos/$owner/$repo/forks")
     status == 202 || error("forking $owner/$repo failed: $(response["message"])")
     return response
 end

@kmsquire
Copy link
Member Author

No worries. I'll update this one once your PR is merged.

@StefanKarpinski
Copy link
Sponsor Member

Bump – this would be awesome to get merged.

@porterjamesj
Copy link
Contributor

+:100: would love to see this merged! current behavior is very annoying for those of us required by our employers to have 2FA on :disappointed:

@kmsquire
Copy link
Member Author

kmsquire commented Sep 3, 2014

Well, two-factor authentication works now, but it's kind of ugly. If all we care about is a working solution, this can be merged. If someone wants to improve it, please do!

The current situation:

  • If there is no two-factor authentication (TFA) and no existing token, it prompts once for a password, and just works. (OK)
  • If there is no TFA, but there is an existing token, it prompts twice for a password, gets the token on the second attemp, and just works (OK)
  • If there is TFA in effect
    1. the first request determines that TFA is active (one password requested) (OK)

    2. the second request attempts to create a new token (token + second password) (OK)

      If the token already exists

    3. a third request is required to have another token sent (third password entry) (!!)

    4. the fourth request gets all tokens, and extracts the one for julia (token + fourth password entry) (!!!!!)

It's possible to shift the burden around slightly, but not much, AFAICT. Mostly this is a consequence of calling curl.

Possible solutions:

  1. enter a password securely in Julia, store it, and pass it to curl

    Pros: total of one password entry
    Cons: how can we give curl the password so that it isn't clear text on the command line?

  2. use libCURL, HTTPClient, or something similar

    Pros: (slightly) simpler interface for making requests, less code in base (but the package is required)
    Cons: still have the password entry issue

@StefanKarpinski
Copy link
Sponsor Member

I would argue that an ugly solution is better than no solution. We can merge now and improve later.

@StefanKarpinski
Copy link
Sponsor Member

Can't you feed curl the password on stdin?

@kmsquire
Copy link
Member Author

kmsquire commented Sep 3, 2014

I'm not sure. I have the impression that it's a little more complicated than that, since curl (and similar utilities, e.g.ssh, etc) do a bit of work to prompt you for your password. And there's also no easy way to actually prompt for a password and not have it echo to the screen on all platforms. (Or at least I couldn't find one.). The standard library functions (getpasswd and company are not available on all platforms, and in some cases have been deprecated on the ones they exist on.

I'm wondering how anyone solves this in a cross platform manner!

@kmsquire
Copy link
Member Author

kmsquire commented Sep 3, 2014

Anyway, I'll merge a little later if no one else does first.

kmsquire added a commit that referenced this pull request Sep 4, 2014
RFC: Enable github two-factor authentication; fixes #5252
@kmsquire kmsquire merged commit 0bd25a1 into master Sep 4, 2014
@kmsquire kmsquire deleted the kms/two-factor-auth branch September 4, 2014 06:36
@kmsquire
Copy link
Member Author

kmsquire commented Sep 4, 2014

@StefanKarpinski, yes, you can actually send the password to curl via STDIN, if you use curl -K - (which reads a config file from STDIN).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants