Add WebAuthn 2FA in UI #2108

grzuy · 2019-08-21T18:14:05Z

First stab at #1948 in collaboration with @padulafacundo.

Prototype adding experimental support for WebAuthn as a 2nd factor.

Goal of this first prototype is to have a working solution that can be tested by rubygems.org and community developers to get early feedback and start the discussion.

What it does

Let's a user who already has 2FA (OTP) configure register WebAuthn Security Keys
On UI Sign In, user can 2FA with WebAuthn Security or fallback to OTP

Pending:

Follow up items:

Support WebAuthn 2FA at the API level

app/models/user.rb

app/views/layouts/application.html.erb

app/views/sessions/webauthn_prompt.html.erb

db/migrate/20190814131051_create_credentials.rb

config/locales/en.yml

app/controllers/webauthn_credentials_controller.rb

db/migrate/20190814131051_create_credentials.rb

app/controllers/sessions_controller.rb

app/models/user.rb

Also, use `disable_with` in the 'Register a new credential' button

db/migrate/20190822144428_change_credentials_to_webauthn_credentials.rb

vendor/assets/javascripts/webauthn-json.js

app/controllers/webauthn_credentials_controller.rb

eliotsykes · 2019-08-27T08:43:33Z

config/routes.rb

+
+  resources :webauthn_credentials, only: %i[index create destroy] do
+    collection do
+      get :create_options


get :create_options route is not protected against CSRF. Consider if this should use post instead to gain CSRF protection.

This action is not a state-changing action.

We renamed it, create_options was a bit confusing.

eliotsykes · 2019-08-27T08:47:02Z

config/routes.rb

@@ -169,6 +176,10 @@

    resource :session, only: %i[create destroy] do
      post 'mfa_create', to: 'sessions#mfa_create', as: :mfa_create
+      collection do
+        get :webauthn_authentication_options, constraints: { format: :json }, defaults: { format: :json }


Consider altering the :webauthn_authentication_options route to use post instead of get to take advantage of Rails' CSRF protection.

Was coded as a GET because not a state-changing action.

WebAuthn code that isn't realted to the UI is now on two controllers under 'internal'. This fixes two failing tests regarding UI routes.

eliotsykes · 2019-08-29T08:31:07Z

Would any of the new WebAuthn routes need throttling to mitigate brute force attacks?

Use 'security key' instead of 'credential'. Also, add missing translations on languages other than english (fixes failing test)

app/controllers/internal/webauthn_registrations_controller.rb

Avoid getting mixed with top-level default params

vendor/assets/javascripts/webauthn-json.js

sonalkr132 · 2020-10-09T05:38:19Z

app/views/sessions/mfa_prompt.html.erb

+  <%= render "sessions/webauthn_prompt" %>
+<% end %>
+
+<%= render "sessions/otp_prompt" %>


Can we please this a horizontal split? vertical looks clunky. l-half--l and l-half--r may be helpful.

sonalkr132 · 2020-10-09T05:39:18Z

config/locales/de.yml

@@ -16,6 +16,8 @@ de:
  please_sign_in:
  otp_incorrect:
  otp_missing:
+  use_key: Sign in with Security Key


Just add keys (empty value) for non-english locales.

sonalkr132 · 2020-10-09T05:43:26Z

app/views/profiles/edit.html.erb

@@ -102,6 +102,8 @@
      </div>
      <%= submit_tag t('.mfa.update'), :class => 'form__submit' %>
    <% end %>
+
+    <h2><%= link_to t('webauthn_credentials.index.title'), webauthn_credentials_path %></h2>


As this stands, the user can only add webauth security keys after they have registered a TOTP authenticator device. I am not sure I understand this requirement. Shouldn't the user be able to enable mfa and only register webauth keys?

sonalkr132 · 2020-10-09T05:48:24Z

Please add a section in CONTRIBUTING.md (or a new doc page) for steps to tests this locally. I was able to use https://github.com/google/virtual-authenticators-tab but it would nice to have this or perhaps an alternate method documented where we may not have to purchase a yubikey.

We will also need guides (for users) similar to https://guides.rubygems.org/setting-up-multifactor-authentication/
perhaps clarifying the downside that this method doesn't support gem cli.

sonalkr132 · 2020-10-09T05:51:15Z

app/controllers/webauthn_credentials_controller.rb

+    @user = current_user
+  end
+
+  def destroy


Is it acceptable to allow users to delete the key without verifying that they have access to the key?

sonalkr132 · 2020-10-09T05:57:24Z

app/models/user.rb

@@ -218,6 +224,26 @@ def otp_verified?(otp)
    save!(validate: false)
  end

+  def webauthn_verified?(current_challenge, webauthn_credential)


I am not sure I understand why this method is in User model. Would it be better if we asked WebauthnCredential object if it was valid/verified? Do we want to ensure that webauthn_credential belongs to the user?

sonalkr132 · 2022-06-25T04:47:38Z

Closing this in favour of #2865

Add WebAuthn 2FA support

4ae5350

grzuy mentioned this pull request Aug 21, 2019

2FA: FIDO / U2F / yubikey support #1948

Closed

grzuy commented Aug 21, 2019

View reviewed changes

app/models/user.rb Outdated Show resolved Hide resolved

app/views/layouts/application.html.erb Outdated Show resolved Hide resolved

app/views/sessions/webauthn_prompt.html.erb Outdated Show resolved Hide resolved

ecnelises self-requested a review August 22, 2019 02:15

bdewater reviewed Aug 22, 2019

View reviewed changes

refactor: collapse js code into one file

a46d073

eliotsykes reviewed Aug 22, 2019

View reviewed changes

app/controllers/sessions_controller.rb Outdated Show resolved Hide resolved

app/controllers/sessions_controller.rb Outdated Show resolved Hide resolved

app/models/user.rb Outdated Show resolved Hide resolved

Facundo Padula and others added 4 commits August 22, 2019 11:03

refactor: use button_to instead of form_tag in webauthn sign in prompt

66c3c64

Also, use `disable_with` in the 'Register a new credential' button

refactor: modifiy name and columns of credentials table

506f643

feat: add support for multiple webauthn credentials

5a55035

Host webauthn-json in vendor/assets/javascript

dbdbe2a

bdewater reviewed Aug 27, 2019

View reviewed changes

db/migrate/20190822144428_change_credentials_to_webauthn_credentials.rb Show resolved Hide resolved

eliotsykes reviewed Aug 27, 2019

View reviewed changes

grzuy and others added 5 commits August 27, 2019 10:06

Prefer non-minified version of webauthn-json

f5d9a8c

Check if the user agent supports WebAuthn

f299e83

Better handle sign in/credential registration failures

d1a046c

Randomize user handle

83b7129

Refactor routes and controllers

aaa3ec1

WebAuthn code that isn't realted to the UI is now on two controllers under 'internal'. This fixes two failing tests regarding UI routes.

Facundo Padula and others added 10 commits August 29, 2019 14:14

Pollish translations

b090654

Use 'security key' instead of 'credential'. Also, add missing translations on languages other than english (fixes failing test)

Don't allow repeated credentials

9d4db17

Pollish UX

9065ef2

Add 'last_used_on' field to webauthn credentials

8b9d9fc

Integrate webauthn in the 'Edit profile' page

7a43041

Allow max sign count value

30047e2

Consider 'destroy' might fail silently

a7ae3ca

Throttle webauthn routes

d82f0c2

Merge branch 'master' into add_webauthn

e36e0e9

Update to webauthn gem edge master

a8cfa5c

grzuy commented Sep 4, 2019

View reviewed changes

app/controllers/internal/webauthn_registrations_controller.rb Outdated Show resolved Hide resolved

grzuy added 2 commits September 18, 2019 13:37

Avoid second lookup of the user credential

7a1b547

Avoid unnecessary session access before delete

9ffc99a

grzuy force-pushed the add_webauthn branch from 1b72308 to 9ffc99a Compare September 18, 2019 16:38

grzuy added 9 commits September 18, 2019 15:47

Fix indentation

c0ed0c2

Generalize disabling of webauthn buttons

501085e

Remove unnecessary presence check

77dd527

Simplify webauthn authentication event handler plugging

9d63848

Attempt to make form reference more consistent

c00319f

Remove unneeded method option

ca33802

Remove unnecessary function naming

c059002

Namespace WebAuthn API provided credential in params

14c0fb5

Avoid getting mixed with top-level default params

Make local variable name consistent with the rest of the file

058fc3e

grzuy changed the title ~~Add WebAuthn 2FA support~~ Add WebAuthn 2FA support in UI Sep 20, 2019

grzuy changed the title ~~Add WebAuthn 2FA support in UI~~ Add WebAuthn 2FA in UI Sep 20, 2019

grzuy marked this pull request as ready for review September 20, 2019 14:25

Update @github/webauthn-json to v0.3.6

d99cbf8

grzuy commented Sep 20, 2019

View reviewed changes

vendor/assets/javascripts/webauthn-json.js Show resolved Hide resolved

grzuy mentioned this pull request Apr 19, 2020

Use webpacker for JS dependencies #2126

Closed

grzuy added 2 commits May 10, 2020 13:13

Update webauthn gem to v2.2

b879f36

Merge branch 'master' into add_webauthn

1b192e0

sonalkr132 reviewed Oct 9, 2020

View reviewed changes

colby-swandale mentioned this pull request Nov 15, 2021

Webauthn credentials #2865

Merged

4 tasks

paulschreiber mentioned this pull request Jun 14, 2022

Add WebAuth / FIDO2 / security key support #3097

Closed

sonalkr132 closed this Jun 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WebAuthn 2FA in UI #2108

Add WebAuthn 2FA in UI #2108

grzuy commented Aug 21, 2019 •

edited

eliotsykes Aug 27, 2019

grzuy Sep 17, 2019

eliotsykes Aug 27, 2019

grzuy Sep 17, 2019

eliotsykes commented Aug 29, 2019

sonalkr132 Oct 9, 2020

sonalkr132 Oct 9, 2020

sonalkr132 Oct 9, 2020

sonalkr132 commented Oct 9, 2020

sonalkr132 Oct 9, 2020

sonalkr132 Oct 9, 2020 •

edited

sonalkr132 commented Jun 25, 2022

Add WebAuthn 2FA in UI #2108

Add WebAuthn 2FA in UI #2108

Conversation

grzuy commented Aug 21, 2019 • edited

What it does

Pending:

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

eliotsykes commented Aug 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonalkr132 commented Oct 9, 2020

Choose a reason for hiding this comment

sonalkr132 Oct 9, 2020 • edited

Choose a reason for hiding this comment

sonalkr132 commented Jun 25, 2022

grzuy commented Aug 21, 2019 •

edited

sonalkr132 Oct 9, 2020 •

edited