Two-factor authentication feature in API part. by ecnelises · Pull Request #1753 · rubygems/rubygems.org

ecnelises · 2018-07-09T13:24:54Z

This PR is for the multifactor auth of the API part. The changes are added into v1 version of rubygems.org API. Current design is:

A new interface will be added into ProfilesController (or maybe other), getting the mfa setting status of current user (needs authentication).
If the mfa on write option (mfa_login_and_write) is enabled, those actions needing mfa should check OTP field in request header, and verify it.
If verification on OTP fails, a 401 forbidden response with prompt text are returned. Otherwise, everything goes normally.
Actions needing mfa are:
- Pushing gems
- Change ownership of gems.
Maybe a new interface will be added for changing mfa status through RESTful API.

I'm not sure whether the approach is compatible for the rubygems versions (not supporting mfa). I will check how rubygems client handles the operation errors.

Below are tasks.

Add check for OTP field in header.
Check for rubygems push action.
Check for ownership change action.
Check for creating deletions (yank command).
MFA level change. (In UI part)
Check for API Key acquirement.
Integration testing.
(Maybe) Support for mfa state change (enabling, disabling, etc.) through API.

sonalkr132 · 2018-07-15T16:16:44Z

+  def verify_with_otp
+    return unless @api_user.mfa_login_and_write?
+    otp = request.headers["HTTP_OTP"] || ''
+    return if @api_user&.otp_verified?(otp)


is this & needed here? verify_authenticated_user ensures that user see 401 if @api_user is nil.

Right. At first, I'm considering a case that verify_with_otp is put before verify_authenticated_user. But since the method relies on existence of @api_user, it must be put after verify_authenticated_user.

sonalkr132 · 2018-07-22T09:06:30Z

This looks good so far 👍 Can we please make some headway with

MFA level change. (In UI part)

ecnelises · 2018-07-22T13:04:21Z

Something like this seems reasonable. @sonalkr132

sonalkr132 · 2018-07-26T15:53:29Z

Something like this seems reasonable

Its fine but can be better.
Check these out:
https://codepen.io/cssinate/pen/KVdYjz
https://codepen.io/tr13ze/pen/yEdxvN
https://codepen.io/tag/dropdown/

Public pens are MIT licensed, if you are going to copy any of them, please make sure it matches our color scheme.

ecnelises · 2018-07-26T17:29:35Z

@sonalkr132
Thanks, I'll check them out. I saw styles about select tags in our css file, but there's no select in the templates before.

sonalkr132 · 2018-07-31T05:34:38Z


+  def mfa_write_authorized?(otp)
+    return true unless mfa_login_and_write?
+    otp_verified?(otp || '')


please don't do otp || '', either add default argument to mfa_write_authorized? or otp_verified?.

I think I can replace otp || '' with otp.to_s

sonalkr132 · 2018-08-11T14:55:03Z

+        <%= select_tag :level, options_for_select([
+          ['Login and Write', 'mfa_login_and_write'],
+          ['Login Only', 'mfa_login_only'],
+          ['Disabled', 'no_mfa']], @user.mfa_level), :class => 'form__input form__select' %>


Login only and Login and write don't make much sense to me. At least to user, we should show something more appropriate like UI Authentication and UI and API Authentication.

We should also consider updating function names as well. I am not a stickler for naming it UI and API Authentication but ..login_and_write is just misleading. For example, api request to show key is not a write operation and by login_only we don't mean api login. Distinction has more to do with whether we are checking something for api or ui.
Sorry, I failed to point it out previously.

The name login and write or login only derives from the original RFC issue about multi-factor authentication. NPM uses these names to identify their different authentication level. These are basic elements in mfa implementation, so I just borrowed them at that time without too much hesitation.

Currently, fetching API key requires OTP only when auth level set to login and write. Truly, these would be confusing to both users and contributors who see this code. Some of other actions not wrapped with mfa now, like API key reset or reset password, but they are also critical. Should they require OTP if UI Authentication enabled? Except that, I think name around UI and API is good. Thanks.

I did it. Auth for UI Only and Auth for UI and API is okay.

sonalkr132 · 2018-08-11T14:56:50Z

    page.fill_in "otp", with: recoveries.sample
-    click_button "Disable"
+    page.select "Disabled"
+    find('#mfa-edit input[type=submit]').click


Please make a function out of above two line.

sonalkr132 · 2018-08-11T14:58:29Z

-          should redirect_to('the profile edit page') { edit_profile_path }
-          should 'disable mfa' do
-            refute @user.reload.mfa_enabled?
+          context 'when input recovery code' do


when otp is recovery code

sonalkr132 · 2018-08-11T15:11:44Z

  none: None
  not_found: Not Found
  please_sign_up: Access Denied. Please sign up for an account at https://rubygems.org
+  please_send_correct_otp: You have enabled multifactor authentication but your request doesn't have the correct OTP code. Please check it and retry.


your request doesn't have the correct OTP code.

Won't we be showing this when OPT is missing too? if yes, please update it to say you didn't provide otp or provided otp was incorrect.

sonalkr132 · 2018-08-11T18:15:20Z

Some of other actions not wrapped with mfa now, like API key reset or reset password, but they are also critical.

In my opinion, asking mfa once during login through UI is good enough. I wish we could do the same for api as well, as pointed out by @colby-swandale

Can we just ask the user to give their MFA token whenever they sign in to an account which has 2FA?

We are asking for otp for every critical cli components because as of now, there is no concept of session when it comes to our api. Unless we implement a way of api authentication where tokens expires after a given time, we can't really do the same for api requests.
aws cli seem to be using related workflow https://aws.amazon.com/premiumsupport/knowledge-center/authenticate-mfa-cli/

ecnelises · 2018-08-12T06:55:45Z

In my opinion, asking mfa once during login through UI is good enough.

Yes, but change mfa level still requires otp check. Actually we don’t have concept like _no mfa check after a duration_. In GitHub, if I’m going to delete one of my repo, the site would ask me to re-input my password. If I want to delete others, password is not needed in the short period. We don’t have this feature.

I wish we could do the same for api as well.

I think it’s feasible to use JWT-like approach to implement it - wrap API key and _session duration_ with sign in header. There was talk about this before. I can handle it as further work. Authentication of API just check header fields from every request. In ideal implementation, UI actions needing mfa can also be wrapped just with something like `before_action :mfa_ui_auth`, similar to how API does. User requests these actions, they will be redirected to a mfa prompt page, enter correct otp, finish actions, otherwise redirected back with a flash message.

…

Aditya Prakash ***@***.***> 於 2018年8月12日上午2:15 寫道： Some of other actions not wrapped with mfa now, like API key reset or reset password, but they are also critical. In my opinion, asking mfa once during login through UI is good enough. I wish we could do the same for api as well, as pointed out by @colby-swandale Can we just ask the user to give their MFA token whenever they sign in to an account which has 2FA? We are asking for otp for every critical cli components because as of now, there is no concept of session when it comes to our api. Unless we implement a way of api authentication where tokens expires after a given time, we can't really do the same for api requests. aws cli seem to be using related workflow https://aws.amazon.com/premiumsupport/knowledge-center/authenticate-mfa-cli/ — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

hsbt · 2018-09-12T02:08:47Z

ref #1725

sonalkr132 · 2018-10-20T18:01:50Z

@@ -0,0 +1,12 @@
+class Api::V1::MultifactorAuthsController < Api::BaseController


I don't think we are using this in client any more.

sonalkr132 · 2018-10-20T18:06:32Z

  none: None
  not_found: Not Found
  please_sign_up: Access Denied. Please sign up for an account at https://rubygems.org
+  otp_incorrect_or_missing: You have enabled multifactor authentication but your OTP code is missing or incorrect. Please check it and retry.


your OTP code is missing or incorrect.

this is unnecessarily ambiguous. Can we please add a check for otp being empty and show error message accordingly?

Right. But change to this may affect rubygems client, I'll check more of it.

sonalkr132 · 2018-10-20T18:06:53Z

    destroy:
      success: You have successfully disabled multifactor authentication.
+    update:
+      success: You have successfully updated your auth level.


aren't we updating mfa level and not auth level?

sonalkr132 · 2018-10-20T18:07:40Z

        go_settings: Register a new device
-        enabled: You have enabled multifactor authentication. Please input your OTP from your authenticator or one of your active recovery codes to disable it.
-        disable: Disable
+        enabled: You have enabled multifactor authentication. Please input your OTP from your authenticator or one of your active recovery codes to change your auth level or disable it.


sonalkr132 · 2018-10-20T18:11:51Z

+        @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api_mfa)
+      end
+
+      context "on POST to add other user as gem owner with email without OTP" do


please drop with email in all these context.

sonalkr132 · 2018-10-20T18:14:52Z

Rebase 💥 please.

sonalkr132 · 2018-10-23T16:01:02Z

One minor change. Looks good to merge now 👍
Have you tried this locally with ruby/rubygems#2369?

ecnelises · 2018-10-23T16:57:16Z

One minor change. Looks good to merge now 👍
Have you tried this locally with rubygems/rubygems#2369?

Local output is like this:
https://paste.ofcode.org/h473UpYsJGytsarQnxjbWC

One thing I need to do: I have to comment the code checking current RubyGems client version. Or I would get

You are using a beta release of RubyGems (3.0.0.beta1) which is not
allowed to push gems. Please downgrade or upgrade to a release version.

sonalkr132 · 2018-10-23T17:19:54Z

This command needs digit code for multifactor authentication.
Code: 757891
Signed in.
Pushing gem to http://localhost:3000...
This command needs digit code for multifactor authentication.
Code: 757891
Successfully registered gem: hello (0.0.9)

If we don't delete otp here, it would be nicer not to ask for otp again.

ecnelises · 2018-10-24T07:15:14Z

If we don't delete otp here, it would be nicer not to ask for otp again.

I remember we talked about it and I checked the chat history. It seems that I misunderstood your meanings. Currently, I need to do multi-factor authentication even seconds after signing in with correct OTP code. Security problem arises when such no-otp interval exists. But now we don't have it. Every execution is independent.

Only concern: If fetching newest version of gem is too slow, the OTP may expire. (Since we have tolerance for OTP in last interval, this should not be a big problem)

Thank you for notifying me about this. I'll delete first it until we found new approaches for authentication.

- Now 'OTP' header is needed for gem creation if user has enabled `mfa-login-and-write` option. - Related unit tests are also added.

- Add `before_action` to `create` and `destroy` action of owner's API. - Add respective tests for OTP check when doing owner change. - Remove unnecessary safe navigation operator of @api_user in OTP checking.

- `api/v1/multifactor_auths#show` gives MFA level of current user. - Related tests are added.

- 'Login Only' is now 'Auth for UI Only' - 'Login and Write' is not 'Auth for UI and API'

sonalkr132 · 2018-11-03T14:30:46Z

Thanks @ecnelises 🍷 ✨

2369: [GSoC] Multi-factor feature for RubyGems. r=hsbt a=ecnelises # Description: Hello. This is my GSoC project, dedicated to add multifactor authentication to both RubyGems command program and Gemcutter ([RubyGems.org](https://rubygems.org)). Work for the Gemcutter part has almost been finished (see [PR #1753](rubygems/rubygems.org#1753), [PR #1729](rubygems/rubygems.org#1729) and a series of [my progress reports](https://ecnelises.github.io/)). ## Content: This PR will contain my changes to RubyGems client, adding multifactor auth for `gem push`, `gem signin` and `gem owner` commands. Since no command for editing profile, adding command for changing multifactor auth settings seems unnecessary. ## Workflow: - User set up multifactor auth well in the site. (into `mfa_login_and_write` level) - When user does the actions requiring MFA, an OTP prompt is shown. Or user can add `--otp` option into command, like `gem push mygem-0.0.0.gem --otp 123456`. - If the OTP is incorrect, operation fails with failure text. ______________ # Tasks: - [x] Add OTP requirement to `push_command`. - [x] Add OTP requirement to `owner_command`. - [x] Add OTP prompt to `sign_in`. - [ ] Support for `yank_command`. - [x] Write related tests. I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: Qiu Chaofan <fwage73@gmail.com> Co-authored-by: SHIBATA Hiroshi <hsbt@ruby-lang.org>

sonalkr132 added the needs-review label Jul 9, 2018

ecnelises force-pushed the mfa-api branch from 0c24b2a to 401c105 Compare July 12, 2018 18:08

connorshea mentioned this pull request Jul 12, 2018

RFC: Add 2FA auth to RubyGems.org #1252

Closed

sonalkr132 reviewed Jul 15, 2018

View reviewed changes

sonalkr132 added the needs-more-work label Jul 15, 2018

ecnelises mentioned this pull request Jul 27, 2018

[GSoC] Multi-factor feature for RubyGems. ruby/rubygems#2369

Merged

5 tasks

ecnelises force-pushed the mfa-api branch from 2237d2d to 333d6b2 Compare July 29, 2018 16:58

sonalkr132 reviewed Jul 31, 2018

View reviewed changes

ecnelises force-pushed the mfa-api branch from 3320d58 to 083face Compare July 31, 2018 07:40

sonalkr132 reviewed Aug 11, 2018

View reviewed changes

sonalkr132 removed the needs-more-work label Aug 11, 2018

ecnelises force-pushed the mfa-api branch from bca59c7 to 7b1ceca Compare September 1, 2018 07:39

ecnelises force-pushed the mfa-api branch from 7b1ceca to 42a10bb Compare September 21, 2018 16:13

sonalkr132 reviewed Oct 20, 2018

View reviewed changes

sonalkr132 added the needs-more-work label Oct 21, 2018

ecnelises force-pushed the mfa-api branch from e0bb155 to 715205b Compare October 23, 2018 16:56

sonalkr132 removed the needs-more-work label Oct 24, 2018

ecnelises added 16 commits November 3, 2018 14:11

Add multifactor auth to rubygems#create api.

f80b38d

- Now 'OTP' header is needed for gem creation if user has enabled `mfa-login-and-write` option. - Related unit tests are also added.

Add OTP requirement for owner add/remove.

285f40f

- Add `before_action` to `create` and `destroy` action of owner's API. - Add respective tests for OTP check when doing owner change. - Remove unnecessary safe navigation operator of @api_user in OTP checking.

Add a new API showing user's MFA set level.

13575ab

- `api/v1/multifactor_auths#show` gives MFA level of current user. - Related tests are added.

Add feature to change multifactor auth level.

cf33062

Add OTP check to API Key acquirement.

33e7691

Convert otp to String before verification.

9aeb4fc

Prompt for OTP when loggin in through API.

ea2b1e4

Wrap auth level change to a single method in tests.

97857f1

Tweak test name about recovery codes.

4adf17a

Tweak prompt text when OTP is missing or incorrect.

8d95eca

Change name for MFA levels.

0125297

- 'Login Only' is now 'Auth for UI Only' - 'Login and Write' is not 'Auth for UI and API'

Change flash.now to flash in mfa tests to supress errors.

d9419df

Remove multifactor_auths controller in v1 api.

abb5383

Remove redundant text in mfa test & rename 'auth level' to 'mfa level'.

efd1ad6

Move otp incorrect and missing into different error prompts.

e56489e

Move MFA level text into locales.

b7b1a02

ecnelises force-pushed the mfa-api branch from 715205b to b7b1a02 Compare November 3, 2018 06:12

sonalkr132 merged commit b2113ff into rubygems:master Nov 3, 2018

rubygems-deployer temporarily deployed to staging November 3, 2018 14:40 Inactive

dwradcliffe temporarily deployed to production November 3, 2018 18:02 Inactive

sonalkr132 mentioned this pull request Aug 3, 2019

Add scope and hashing to api keys #1962

Merged

2 tasks

		@@ -0,0 +1,12 @@
		class Api::V1::MultifactorAuthsController < Api::BaseController

Uh oh!

Conversation

ecnelises commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonalkr132 commented Jul 22, 2018

Uh oh!

ecnelises commented Jul 22, 2018

Uh oh!

sonalkr132 commented Jul 26, 2018

Uh oh!

ecnelises commented Jul 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonalkr132 commented Aug 11, 2018

Uh oh!

ecnelises commented Aug 12, 2018 via email

Uh oh!

hsbt commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonalkr132 commented Oct 20, 2018

Uh oh!

sonalkr132 commented Oct 23, 2018

Uh oh!

ecnelises commented Oct 23, 2018

Uh oh!

sonalkr132 commented Oct 23, 2018

Uh oh!

ecnelises commented Oct 24, 2018

Uh oh!

sonalkr132 commented Nov 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ecnelises commented Jul 9, 2018 •

edited

Loading