GitHub 2FA support

[julian45]

I have 2FA enabled on my GitHub account. When I ran git repo config, this happened:

Do you want to configure the github service?
    [Yn]> y
Please enter your credentials to connect to the service:
username> tcby45
password>
Fatal error: 401 Must specify two-factor authentication OTP code.

Could you please either include a 2FA check on the account (then a prompt for the code), or possibly ask whether to authenticate by personal access token or user/pass?

[guyzmo]

well, the idea is that when you enter login/password with github, it's actually creating a personal token for git-repo, it's NOT keeping your credentials.

I agree it's a bit too cryptic and needs more explanation, though. But if you don't want to share your credentials, just can edit ~/.gitconfig manually!

[guyzmo]

About github 2FA per se, it's definitely a good idea, but it's definitely low priority for me, way after implementing proper OAuth for bitbucket.

[jandrusk]

Just ran into the same thing and would like 2FA support as well.

[guyzmo]

how would you see the 2FA happening? Just for the first time authentication? Or each time you're using the CLI tool?

[guyzmo]

I'd be happy to see a proper usage use case for accepting the 2FA as a feature.

[julian45]

With what you've described above, here's an ideal situation:

1. User runs git repo config for the first time and elects to use GitHub.
2. Wizard prompts user for username and password.
3. Two options:

a. User with 2FA enabled sends username and password. Wizard receives X-GitHub-OTP: required; :2fa-type header (see here). User is prompted for OTP password, which is sent, in the X-GitHub-OTP header, along with another request containing the username and password. GitHub approves, and the wizard continues as normal.
b. User without 2FA enabled sends username and password. GitHub approves, and the wizard continues as normal.

This way, anyone with 2FA enabled (i.e. those who have any sense of security) can take advantage of the wizard and git-repo as a whole.

[guyzmo]

oooh alright… we'll have to check with @sigmavirus24 how the sigmavirus24/github3.py library would handle 2FA, and I might make a patch for that.

[guyzmo]

looks like it's in the lib sigmavirus24/github3.py#167 so it's definitely possible to add support in the client.

[guyzmo]

I've just pushed a new branch that supports 2FA auth:

https://github.com/guyzmo/git-repo/tree/features/2FA_github

the only issue is that it crashes with a 422 Validation Failed when trying to create a token for the tool.

Please test and hack based on that commit 0c29743

[julian45]

Downloaded and installed from that branch, got the following error: Fatal error: 'GitHub' object has no attribute 'token'

[guyzmo]

oh, I'm sorry, I forgot a bit of code when I made the commit ☺

Though it's still failing with the 422 error.

I guess that we need to patch the github3.py code so that it supports the
two_factor_auth_callback setting as shown in the login() method.

The main difference in auth methods being that authorize() uses self.session.temporary_basic_auth(), whilst login() uses self.session.basic_auth(). Looking more closely at the code, the former is actually a wrapper around the second, implemented as a context manager.

As you can see, on the git-repo side, the code implementing auth is pretty minimalistic…

[sigmavirus24]

self.session.basic_auth() does nothing with the 2FA callback work. You'll need to tell us you're going to use the 2FA callback (at the moment just doing gh.login(two_factor_callback=...) and then using authorize. It will still catch it for you

[guyzmo]

hi @sigmavirus24 🖖, very nice of you to join the discussion!

well, that's what I've actually implemented there, and for some reason it's failing with a 422.

So let's dig into that…

[guyzmo]

…ok I went too fast when I first tried.

The issue is that when there's already a key with the same name on github, it's failing with a 422 Unprocessable Entity, making some confusion.

So basically, the 2FA code is working! I'm not sure how I can make a meaningful regression test for that, though… ☹

[guyzmo]

ok, it's merged in devel. I'll roll it out with the next release.