Support GitHub two factor authentication #536

fabianlupa · 2017-01-15T19:00:59Z

References #494
Fixes #253

This should work for now. General approach:

Hook into authentication process before the login dialog appears
If the repository URL is supported by one registered two factor authenticator an additional option is added to the selection screen to enter a 2FA token
If a 2FA token is entered before the usual authentication, requests to the repository hoster's API are made to generate an access token using username, password and 2FA token
If that succeeds the access token is used instead of the user's password for basic authentication. The token is also cached in the user settings.

And when leaving and reentering abapGit:

Login required -> is two factor authentication supported and is there an access key cached for the hoster of the repository?
Ask the user to unlock the access token
Use the unlocked access token as the password again

The token can also be deleted from the login popup.

One thing that is still todo is encrypting the access token. Currently it is stored in abapGit's persistence layer in plain text which is not ideal on multi user development systems where anyone can access database tables directly. My idea was to symmetrically encrypt the access token using the user's password he entered for generating the token and decrypt it using the same password. abapGit does not need to validate the decryption because if the password is wrong and a wrong access key is decrypted authentication will fail anyways. But I could not find any standard function modules or classes that support symmetric encryption using a key. Any tips would be appreciated there. The two spots in code are marked with TODO.

Another thing is that it sometimes takes multiple tries for me to generate a new access token. Might be caused by my authenticator being out of sync or an API rate limit but also could be a programming error on my end.

I think I removed all the uses of >702 stuff but I am not sure how to test for that (my system is 750).

larshp · 2017-01-16T06:57:25Z

Hi,
Thanks!

I'm concerned that this will give the user a false sense of security. Perhaps add a popup explaining the user what actually happens, as this is not true 2FA as might be expected when looking at the GUI.

On the other hand, the current approach in abapGit does not work for GitHub users using 2FA, hmm

For encrypting the token I suggest using https://github.com/Sumu-Ning/AES just make it a dynamic binding so it is not required statically, and show a popup if the user needs to install it.

What 2FA setup have you used for testing? Something like Google Authenticator? I have a Yubikey, which sends a SMS code after entering the password if the Yubikey is not supported.

Do you know if there is a feature in the github API to check if the user has 2FA enabled? Perhaps it will be easier to guide the user to create the access token and enter it manually in abapGit settings?

fabianlupa · 2017-01-16T17:32:08Z

Hi Lars,

true 2FA would be quite cumbersome to implement. Git over http(s) on GitHub does not support authentication using 2FA while GitHub's own API does, so one would have to use GitHub's API for all git requests if it is a GitHub repository which I doubt is worth it. I agree with you though, currently it is not apparent for what the token is used. An info button with a documentation popup next to the token input field could solve this I think.

I personally use the Google Authenticator app for several services supporting two factor authentication including GitHub. SMS should be solvable as well (or already works just you need two tries?). You can get the type of two factor auth the account uses by reading the X-GitHub-OTP: required; :2fa-type header you receive after trying to authenticate using only username and password in the 401 response (source).

Now that I think about it, a better work flow might be to try to authenticate just with the login data and if you get a 401 response with that specific header you reopen the popup with the entered data and additionally show the token field.

If you dislike the approach of creating an access token "in application": Using manually created access tokens as the password already works very well without this PR. The problem is just that you need to reenter it every time. If there was a password safe in general in abapGit independent of 2FA this problem would be solved. The current repository based login data would need to be moved up one level though to git hosting services, because otherwise you create access tokens / save login data for each repository even though they all use the same logins.

So which direction do you want to go?

larshp · 2017-01-16T18:06:29Z

hmm, not sure, need some time to consider.

Which direction do you prefer/recommend?

larshp · 2017-01-17T15:17:10Z

New idea: how about deleting the token immediately after use? That way it will not have to be stored in abapGit.

If abapGit dumps after having created the token, it will not delete it, but that might be okay?

fabianlupa · 2017-01-17T16:54:18Z

Good idea, that seems to work best with the existing concept. We might even be able to delete the last used token when requesting a new one instead of immediately after use.

I also did some research on other git hosters. Gitlab and TFS support access tokens but I did not find an API to create one programmatically. Bitbucket does have one but it might require a browser based approach. So for these ones letting the user create a token on the respective website on his own and saving it in abapGit's not yet existing password storage would be the best solution in my opinion.

However, I think GitHub is by far the most used option of all these hosters so it does make sense to support its two factor authentication "natively". Also your idea is more in scope of the current approach of this PR while a password storage could/should be a separate one if it is still needed then.

So I would refactor this PR to achieve the following:

After returning from the password dialog with the users credentials check if the repository url is one of GitHub's
If that is the case send a login request using the credentials and check if two factor authentication is required using the X-GitHub-OTP: required; response header
If it is not required return from acquire_login_details and let normal authentication continue as before
If it is required show a new prompt for a token which can be obtained either from an authenticator app or by sms (which should have arrived by now because of the first login request)
Create a new access token using username, password and 2FA token (and maybe delete existing ones)
Return from acquire_login_details with the created access token as the password

Is that in line with your idea?

larshp · 2017-01-18T14:34:48Z

Yep, looks good to me 😄

larshp · 2017-01-21T18:32:30Z

src/zabapgit_http.prog.abap

@@ -494,6 +499,51 @@ CLASS lcl_http IMPLEMENTATION.
                                           iv_username = lv_user ).
    ENDIF.

+    " Is the repository hoster supported for using two factor authentication?
+    TRY.
+        IF lcl_2fa_authenticator_registry=>is_url_supported( iv_url ) = abap_true.


can we take most of this part and move to a method in the 2fa include? and have it raise lcx_exception

Sure, I'll get on that

larshp · 2017-01-22T09:33:55Z

sorry, merging #559 and changing all files in abapGit so you might need to rebase/fix some conflicts

cl_sxml_writer/reader do not support json on AS ABAP 7.02

fabianlupa · 2017-01-22T10:30:35Z

No problem, rebase worked out fine I think. I'll comment again once I got the token deletion working.

fabianlupa · 2017-02-04T20:16:06Z

Sorry for the delay!

I moved the entry logic to 2FA into a static method.
When using two factor authentication any previously generated access tokens are deleted before creating a new one. Actually only a single access token by abapGit can exist at any time in a GitHub account because the fingerprint attribute must be unique.

One thing I am mildly concerned about is the parsing logic for the list of all access tokens to filter out the one by abapGit (code). Since on 7.02 the new fancy JSON and REST apis do not exist yet (or I do not know the older ones?) I try to parse the correct token ID using a regular expression which is not ideal and might not handle all edge cases if the response structure is any different.

larshp · 2017-02-05T07:51:08Z

travis errors: fixed, I've released a new version of abaplint

parsing logic: worst case we have to adjust it, lets just ship it and see if it works out 😄

larshp

see comments

larshp · 2017-02-05T08:00:00Z

src/zabapgit_2fa.prog.abap

+
+CLASS lcl_2fa_github_authenticator IMPLEMENTATION.
+  METHOD constructor.
+    super->constructor( 'https?:\/\/(www\.)?github.com.*$' ).


should be "^https?://(www.)?github.com.*$" ? but I guess it does not matter much

larshp · 2017-02-05T08:15:28Z

src/zabapgit_2fa.prog.abap

+    " Try to authenticate, if 2FA is required there will be a specific response header
+    li_client->authenticate( username = iv_username password = iv_password ).
+    li_client->send( ).
+    li_client->receive( ).


dumps here, HTTP_COMMUNICATION_FAILURE, see comment for CREATE_BY_URL

larshp · 2017-02-05T08:37:08Z

src/zabapgit_2fa.prog.abap

+    cl_http_client=>create_by_url(
+      EXPORTING
+        url    = gc_github_api_url
+      IMPORTING


add following, I get a dump due to untrusted SSL certificates, as it currently does not use the same PSE(?) as the other abapGit requests

ssl_id = 'ANONYM' proxy_host = lo_settings->get_proxy_url( ) proxy_service = lo_settings->get_proxy_port( )

see example in include ZABAPGIT_HTTP

larshp · 2017-02-05T08:38:51Z

src/zabapgit_2fa.prog.abap

+    " Try to login to GitHub API with username, password and 2fa token
+    cl_http_client=>create_by_url(
+      EXPORTING
+        url                = gc_github_api_url


also add SSL_ID and proxy settings here

larshp · 2017-02-05T09:05:25Z

Using tokens as before still works

However I dont receive a SMS code, will look into why.

larshp · 2017-02-05T09:41:39Z

SMS codes are currently not triggered, it requires a real request to be sent, I've changed the code in method is_2fa_required and added below, it triggers an SMS(url changed, and http payload added)

    cl_http_client=>create_by_url(
      EXPORTING
        url    = `https://api.github.com/authorizations`
        ssl_id        = 'ANONYM'
      IMPORTING
        client = li_client ).

    li_client->request->set_data( cl_abap_codepage=>convert_to( '{"scopes": ["repo", "user"], "note": "getting-started"}' ) ).

also see https://gist.github.com/caspyin/2288960

fabianlupa · 2017-02-05T11:53:42Z

Thanks for the review!
SMS should work again. Regarding SSL certificates: api.github.com has its own certificate (*.github.com) which I added in STRUST a while ago (and totally forgot about...). I don't think there is a way around having the user add it there manually, or is there?

larshp · 2017-02-05T11:57:54Z

STRUST: nope, no known workaround, will do some testing, if possible would like to not break stuff for existing users. Perhaps we need to update the wiki installation guide

larshp · 2017-02-05T12:03:31Z

src/zabapgit_2fa.prog.abap

+  METHOD set_new_token_request.
+    DATA: lv_json_string TYPE string.
+
+    lv_json_string = |\{"scopes":["repo"],"note":"abapGit","fingerprint":"abapGit2FA"\}|.


Can we change the description of the token to "Generated by abapGit" or "Generated by ZABAPGIT"?

larshp · 2017-02-05T12:13:41Z

SMS: I get the SMS code and it works. But the 2nd time does not work, as it has to delete the old token, guess the SMS codes are real one time codes.

Do you know if the li_http_client in delete_access_tokens is reused in li_authenticator->authenticate, then perhaps it does not need the code again, or the same code can be reused as it is in the same HTTP session, currently it is 2 different sessions

larshp · 2017-02-05T12:21:46Z

SSL certificates: looks like api.github.com works with the main github.com certificate, so everything okay :)

fabianlupa · 2017-02-05T12:43:35Z

Can you try again now if the one SMS code is used for both the deletion and the creation request?
And sorry for breaking your definitions-at-top-check twice...

fabianlupa · 2017-02-05T13:11:02Z

Also I had to add the *.github.com certificate in addition to the normal one in STRUST for it to work. I guess we have configured the normal certificate differently? If anyone has the same configuration as me this would be a breaking change because is_2fa_required will perform the check for any repository on GitHub and raise LCX_2FA_COMMUNICATION_ERROR if the connection is not trusted.

I just read Installing abapGit and therefore assume you imported the root certificate in STRUST while I just imported the two "end of the chain" github certificates. Not sure how many others did it the way I did. Might want to have is_2fa_required fail silently.

larshp · 2017-02-05T13:57:51Z

STRUST: yeah, the top certificate is enough

Now it works when I have a breakpoint, and occasionally when pushing. I think we need to add a WAIT UP TO 1 SECONDS. after creating the token, as it most of the time does not seem to work immediately after it is created, GitHub needs time to process the request

definitions-at-top: it is a but in abaplint, will fix it in a bit. However I'm also a bit afraid of using STATICS here, perhaps make it a member attribute and clear it whenever method use_2fa_if_required is called?

larshp

also see comment above

larshp · 2017-02-05T15:17:24Z

src/zabapgit_2fa.prog.abap

+      RAISE EXCEPTION TYPE lcx_2fa_token_gen_failed
+        EXPORTING
+          iv_error_text = 'Token generation failed: parser error' ##NO_TEXT.
+    ENDIF.


think we need to add a WAIT UP TO 1 SECONDS. here, as Github needs to finish saving the token before it is used

Interesting that GitHub returns the token before it is fully persisted and ready to use. I also don't have that problem on my end.
I'd prefer adding RZL_SLEEP instead of WAIT to avoid the implicit commit.

yeah, I guess the token has to be distributed to the other servers in the cluster. But it seems like this is the issue,

RZL_SLEEP will also do the trick

larshp · 2017-02-05T15:50:47Z

tested ok, now it seems to work every time, merging 😄

thanks @flaiker

larshp · 2017-02-05T15:53:47Z

@flaiker are you on twitter?

fabianlupa · 2017-02-05T16:02:51Z

@larshp My pleasure! And no I am not, some bot took my name a while ago and since then I am averted :P

fabianlupa force-pushed the github_2fa_v2 branch from 047cee1 to 3b01baf Compare January 21, 2017 17:47

larshp reviewed Jan 21, 2017

View reviewed changes

fabianlupa added 7 commits January 22, 2017 11:17

Support GitHub two factor authentication

36b817b

Remove usages of REST API for compatibility

cb0161d

Fix code style issues and documentation

dd516fb

Replace usages of cl_sxml* for compatibility

7715bdc

cl_sxml_writer/reader do not support json on AS ABAP 7.02

Re-enable enter key in authentication dialog

f8cf870

Improve exception handling

f93bb68

Refactor 2FA to not require access token storage

0026e74

fabianlupa force-pushed the github_2fa_v2 branch from 3b01baf to 0026e74 Compare January 22, 2017 10:20

Update 2FA PR for v1.26.0

afd1c4e

Delete previously created 2FA access tokens

8a8df05

larshp requested changes Feb 5, 2017

View reviewed changes

Apply requested code review changes

d7fcf8a

larshp approved these changes Feb 5, 2017

View reviewed changes

larshp requested changes Feb 5, 2017

View reviewed changes

fabianlupa added 2 commits February 5, 2017 13:34

Cache authenticated session for one time SMS token

b063e35

Change generated access token description

991ff4c

larshp approved these changes Feb 5, 2017

View reviewed changes

larshp requested changes Feb 5, 2017

View reviewed changes

fabianlupa added 2 commits February 5, 2017 16:36

Refactor 2FA internal http session caching

6d63019

Wait 1 second after access token generation

e316546

larshp approved these changes Feb 5, 2017

View reviewed changes

larshp merged commit fc84127 into abapGit:master Feb 5, 2017

larshp mentioned this pull request Feb 5, 2017

2 factor authentication #253

Closed

fabianlupa deleted the github_2fa_v2 branch February 5, 2017 16:00

fabianlupa mentioned this pull request Aug 21, 2019

fix URL formatting, add note on **api**.github.com certificates #2886

Merged

elsewhat mentioned this pull request Jun 22, 2020

Deprecation of GitHub API password based authentication #3150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support GitHub two factor authentication #536

Support GitHub two factor authentication #536

fabianlupa commented Jan 15, 2017

larshp commented Jan 16, 2017

fabianlupa commented Jan 16, 2017

larshp commented Jan 16, 2017

larshp commented Jan 17, 2017

fabianlupa commented Jan 17, 2017

larshp commented Jan 18, 2017

larshp Jan 21, 2017

fabianlupa Jan 22, 2017

larshp commented Jan 22, 2017

fabianlupa commented Jan 22, 2017

fabianlupa commented Feb 4, 2017

larshp commented Feb 5, 2017

larshp left a comment

larshp Feb 5, 2017

larshp Feb 5, 2017

larshp Feb 5, 2017

larshp Feb 5, 2017

larshp commented Feb 5, 2017

larshp commented Feb 5, 2017 •

edited

fabianlupa commented Feb 5, 2017

larshp commented Feb 5, 2017

larshp Feb 5, 2017

larshp commented Feb 5, 2017

larshp commented Feb 5, 2017

fabianlupa commented Feb 5, 2017

fabianlupa commented Feb 5, 2017

larshp commented Feb 5, 2017

larshp left a comment

larshp Feb 5, 2017

fabianlupa Feb 5, 2017

larshp Feb 5, 2017

larshp commented Feb 5, 2017

larshp commented Feb 5, 2017

fabianlupa commented Feb 5, 2017

Support GitHub two factor authentication #536

Support GitHub two factor authentication #536

Conversation

fabianlupa commented Jan 15, 2017

larshp commented Jan 16, 2017

fabianlupa commented Jan 16, 2017

larshp commented Jan 16, 2017

larshp commented Jan 17, 2017

fabianlupa commented Jan 17, 2017

larshp commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larshp commented Jan 22, 2017

fabianlupa commented Jan 22, 2017

fabianlupa commented Feb 4, 2017

larshp commented Feb 5, 2017

larshp 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

larshp commented Feb 5, 2017

larshp commented Feb 5, 2017 • edited

fabianlupa commented Feb 5, 2017

larshp commented Feb 5, 2017

Choose a reason for hiding this comment

larshp commented Feb 5, 2017

larshp commented Feb 5, 2017

fabianlupa commented Feb 5, 2017

fabianlupa commented Feb 5, 2017

larshp commented Feb 5, 2017

larshp 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

larshp commented Feb 5, 2017

larshp commented Feb 5, 2017

fabianlupa commented Feb 5, 2017

larshp commented Feb 5, 2017 •

edited