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

Add WebAuthn 2FA in UI #2108

Closed
wants to merge 51 commits into from
Closed
Show file tree
Hide file tree
Changes from 49 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
4ae5350
Add WebAuthn 2FA support
Aug 16, 2019
a46d073
refactor: collapse js code into one file
Aug 22, 2019
66c3c64
refactor: use button_to instead of form_tag in webauthn sign in prompt
Aug 22, 2019
506f643
refactor: modifiy name and columns of credentials table
Aug 23, 2019
5a55035
feat: add support for multiple webauthn credentials
Aug 23, 2019
dbdbe2a
Host webauthn-json in vendor/assets/javascript
grzuy Aug 26, 2019
f5d9a8c
Prefer non-minified version of webauthn-json
grzuy Aug 27, 2019
f299e83
Check if the user agent supports WebAuthn
Aug 26, 2019
d1a046c
Better handle sign in/credential registration failures
Aug 27, 2019
83b7129
Randomize user handle
Aug 27, 2019
aaa3ec1
Refactor routes and controllers
Aug 28, 2019
b090654
Pollish translations
Aug 29, 2019
9d4db17
Don't allow repeated credentials
Aug 29, 2019
9065ef2
Pollish UX
Sep 2, 2019
8b9d9fc
Add 'last_used_on' field to webauthn credentials
Sep 3, 2019
7a43041
Integrate webauthn in the 'Edit profile' page
Sep 3, 2019
30047e2
Allow max sign count value
Sep 3, 2019
a7ae3ca
Consider 'destroy' might fail silently
Sep 3, 2019
d82f0c2
Throttle webauthn routes
Sep 3, 2019
e36e0e9
Merge branch 'master' into add_webauthn
grzuy Sep 4, 2019
a8cfa5c
Update to webauthn gem edge master
grzuy Sep 4, 2019
f66f05c
Update to latest webauthn gem
Sep 6, 2019
08da7fe
Attempt to fix build_docker CI step
grzuy Sep 6, 2019
fd99d4a
Fix uglifier compilation
grzuy Sep 6, 2019
b152de8
Fix uglifier compilation of webauthn-json
grzuy Sep 6, 2019
e33578c
Remove unnecessary encoding
Sep 6, 2019
1a55716
Display error message if security key registration/authentication fails
Sep 9, 2019
5eb73cc
Push encoding/decoding to webauthn gem
Sep 9, 2019
92e4f5b
Update to latest webauthn
grzuy Sep 13, 2019
0c869da
Update to latest webauthn
Sep 13, 2019
ca71cda
Update webauthn gem to released v2.0.0.beta1
grzuy Sep 17, 2019
a34aadf
Remove no longer needed git in docker builds
grzuy Sep 17, 2019
6c6a457
Remove unnecessary local variable
grzuy Sep 17, 2019
89f9d7a
Attempt to improve local variable naming semantics
grzuy Sep 17, 2019
90c4e2e
WebAuthn possible only if OTP already enabled
grzuy Sep 18, 2019
adc2aa4
Remove unnecessary conditional on sign_count update
grzuy Sep 18, 2019
e8f4a42
Prefer Time.current shortcut
grzuy Sep 18, 2019
7a1b547
Avoid second lookup of the user credential
grzuy Sep 18, 2019
9ffc99a
Avoid unnecessary session access before delete
grzuy Sep 18, 2019
c0ed0c2
Fix indentation
grzuy Sep 18, 2019
501085e
Generalize disabling of webauthn buttons
grzuy Sep 18, 2019
77dd527
Remove unnecessary presence check
grzuy Sep 18, 2019
9d63848
Simplify webauthn authentication event handler plugging
grzuy Sep 18, 2019
c00319f
Attempt to make form reference more consistent
grzuy Sep 18, 2019
ca33802
Remove unneeded method option
grzuy Sep 18, 2019
c059002
Remove unnecessary function naming
grzuy Sep 18, 2019
14c0fb5
Namespace WebAuthn API provided credential in params
grzuy Sep 18, 2019
058fc3e
Make local variable name consistent with the rest of the file
grzuy Sep 18, 2019
d99cbf8
Update @github/webauthn-json to v0.3.6
grzuy Sep 20, 2019
b879f36
Update webauthn gem to v2.2
grzuy May 10, 2020
1b192e0
Merge branch 'master' into add_webauthn
grzuy May 10, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ gem "sprockets-rails"
gem "rack-attack"
gem "rqrcode"
gem "rotp"
gem "webauthn", "~> 2.0.0.a"

# Logging
gem "lograge"
Expand Down
20 changes: 19 additions & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ GEM
ast (2.4.0)
autoprefixer-rails (9.6.1)
execjs
awrence (1.1.0)
aws-eventstream (1.0.3)
aws-sdk (2.11.263)
aws-sdk-resources (= 2.11.263)
Expand All @@ -59,6 +60,7 @@ GEM
aws-sigv4 (1.1.0)
aws-eventstream (~> 1.0, >= 1.0.2)
bcrypt (3.1.12)
bindata (2.4.4)
bootsnap (1.4.3)
msgpack (~> 1.0)
builder (3.2.3)
Expand All @@ -70,6 +72,7 @@ GEM
rack (>= 1.0.0)
rack-test (>= 0.5.4)
xpath (>= 2.0, < 4.0)
cbor (0.5.9.6)
celluloid (0.17.3)
celluloid-essentials
celluloid-extras
Expand Down Expand Up @@ -103,6 +106,8 @@ GEM
colorize (0.8.1)
compact_index (0.11.0)
concurrent-ruby (1.1.5)
cose (0.8.0)
cbor (~> 0.5.9)
crass (1.0.4)
css_parser (1.7.0)
addressable
Expand Down Expand Up @@ -171,6 +176,7 @@ GEM
http_parser.rb (0.6.0)
i18n (1.6.0)
concurrent-ruby (~> 1.0)
ipaddr (1.2.2)
jaro_winkler (1.5.2)
jmespath (1.4.0)
jquery-rails (4.3.3)
Expand Down Expand Up @@ -251,6 +257,8 @@ GEM
nokogiri (1.10.4)
mini_portile2 (~> 2.4.0)
oj (3.7.12)
openssl (2.1.2)
ipaddr
optimist (3.0.0)
os (1.0.1)
parallel (1.17.0)
Expand Down Expand Up @@ -352,6 +360,7 @@ GEM
sass-listen (4.0.0)
rb-fsevent (~> 0.9, >= 0.9.4)
rb-inotify (~> 0.9, >= 0.9.7)
securecompare (1.0.0)
shoryuken (2.1.3)
aws-sdk-core (~> 2)
celluloid (~> 0.17)
Expand Down Expand Up @@ -382,7 +391,7 @@ GEM
toxiproxy (1.0.3)
tzinfo (1.2.5)
thread_safe (~> 0.1)
uglifier (3.2.0)
uglifier (4.1.20)
execjs (>= 0.3.0, < 3)
unf (0.1.4)
unf_ext
Expand All @@ -393,6 +402,14 @@ GEM
raindrops (~> 0.7)
validates_formatting_of (0.9.0)
activemodel
webauthn (2.0.0.beta1)
awrence (~> 1.1)
bindata (~> 2.4)
cbor (~> 0.5.9)
cose (~> 0.8.0)
jwt (>= 1.5, < 3.0)
openssl (~> 2.0)
securecompare (~> 1.0)
websocket-driver (0.7.0)
websocket-extensions (>= 0.1.0)
websocket-extensions (0.1.3)
Expand Down Expand Up @@ -463,6 +480,7 @@ DEPENDENCIES
uglifier (>= 1.0.3)
unicorn (~> 5.5.0.1.g6836)
validates_formatting_of
webauthn (~> 2.0.0.a)
xml-simple

BUNDLED WITH
Expand Down
1 change: 1 addition & 0 deletions app/assets/javascripts/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
//= require jquery_ujs
//= require clipboard
//= require github_buttons
//= require webauthn-json
//= require_tree .

function handleClick(event, nav, removeNavExpandedClass, addNavExpandedClass) {
Expand Down
76 changes: 76 additions & 0 deletions app/assets/javascripts/webauthn_credentials.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
$(function(){
if(!webauthnJSON.supported()) {
if($("#unsupported-browser-message").length) {
$("#unsupported-browser-message").show();

$(".js-webauthn-button").each(function() {
$(this).prop("disabled", true);
});
}
}
});

$(".js-webauthn-registration-form").submit(function(event) {
event.preventDefault();
$("#security-key-error-message").hide();
var $form = $(this);

$.get({
url: "/internal/webauthn_registration/options",
dataType: "json",
}).done(function(options) {
webauthnJSON.create({ "publicKey": options }).then(
function(credential) {
callback(
"/internal/webauthn_registration",
{
"credential": credential,
"nickname": $("#nickname").val()
}
);
},
function(reason) {
$("#security-key-error-message").show();
var registerButton = $form.find("input.form__submit");
registerButton.attr('value', registerButton.attr('data-enable-with'));
registerButton.prop('disabled', false);
});
}).fail(function(response) { console.log(response) })
});

$(".js-webauthn-authentication-form").submit(function(event) {
event.preventDefault();
$("#security-key-error-message").hide();
var $form = $(this);

$.get({
url: "/internal/webauthn_session/options",
dataType: "json"
}).done(function(options) {
webauthnJSON.get({ "publicKey": options }).then(
function(credential) {
callback("/internal/webauthn_session", { "credential": credential });
},
function(reason) {
$("#security-key-error-message").show();
signInButton = $form.find(".js-webauthn-button");
signInButton.attr('value', signInButton.attr('data-enable-with'));
signInButton.prop('disabled', false);
});
}).fail(function(response) { window.location.replace(response.responseJSON["redirect_path"]) })
});

function callback(url, body) {
$.post({
url: url,
data: JSON.stringify(body),
dataType: "json",
headers: {
"Content-Type": "application/json"
}
}).done(function(response) {
window.location.replace(response["redirect_path"]);
}).error(function(response) {
window.location.replace(response.responseJSON["redirect_path"]);
});
}
4 changes: 4 additions & 0 deletions app/assets/stylesheets/modules/button.css
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@
transition-property: opacity; }
.form__submit:hover {
opacity: .7; }
.form__submit:disabled {
opacity: .5;
cursor: default;
}

.download__format {
margin-bottom: 24px;
Expand Down
10 changes: 10 additions & 0 deletions app/assets/stylesheets/type.css
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,18 @@ label.t-hidden {
overflow: visible\0;
clip: auto\0; }

.t-red {
color: red; }

.t-gray {
color: #9da2ab; }

.t-uppercase {
text-transform: uppercase; }

.t-credentials-list {
margin-bottom: 70px; }

.t-list__heading {
font-weight: 800;
font-size: 12px;
Expand Down Expand Up @@ -212,3 +218,7 @@ a.t-list__item {
.t-item--hidden {
display: none !important;
}

.t--hidden {
display: none;
}
44 changes: 44 additions & 0 deletions app/controllers/internal/webauthn_registrations_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
class Internal::WebauthnRegistrationsController < ApplicationController
def options
current_user.update(webauthn_handle: WebAuthn.generate_user_id) unless current_user.webauthn_handle

options_for_create = WebAuthn::Credential.options_for_create(
user: {
name: current_user.handle,
display_name: current_user.handle,
id: current_user.webauthn_handle
},
exclude: current_user.webauthn_credentials.pluck(:external_id)
)

session[:webauthn_challenge] = options_for_create.challenge

render json: options_for_create, status: :ok
end

def create
webauthn_credential = WebAuthn::Credential.from_create(params[:credential])

if webauthn_credential.verify(session[:webauthn_challenge])
user_credential = current_user.webauthn_credentials.build(
external_id: webauthn_credential.id,
public_key: webauthn_credential.public_key,
nickname: params[:nickname],
sign_count: webauthn_credential.sign_count
)

if user_credential.save
flash[:success] = t(".success")
status = :ok
else
flash[:error] = t(".fail")
status = :internal_server_error
end
else
flash[:error] = t("internal.webauthn_sessions.incorrect_security_key")
status = :unauthorized
end

render json: { redirect_path: webauthn_credentials_path }, status: status
end
end
37 changes: 37 additions & 0 deletions app/controllers/internal/webauthn_sessions_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
class Internal::WebauthnSessionsController < Clearance::SessionsController
def options
user = User.find_by(handle: session[:mfa_user])

if user&.webauthn_enabled?
options_for_get = WebAuthn::Credential.options_for_get(
allow: user.webauthn_credentials.pluck(:external_id)
)

session[:webauthn_challenge] = options_for_get.challenge

render json: options_for_get, status: :ok
else
flash[:error] = t("webauthn_credentials.not_enabled")
render json: { redirect_path: sign_in_path }, status: :unauthorized
end
end

def create
user = User.find_by(handle: session.delete(:mfa_user))
webauthn_credential = WebAuthn::Credential.from_get(params[:credential])

if user&.webauthn_enabled? && user&.webauthn_verified?(session[:webauthn_challenge], webauthn_credential)
sign_in(user) do |status|
if status.success?
render json: { redirect_path: root_path }, status: :ok
else
flash[:notice] = status.failure_message
render json: { redirect_path: sign_in_path }
end
end
else
flash[:error] = t("internal.webauthn_sessions.incorrect_security_key")
render json: { redirect_path: sign_in_path }, status: :unauthorized
end
end
end
2 changes: 1 addition & 1 deletion app/controllers/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def create

if @user&.mfa_enabled?
session[:mfa_user] = @user.handle
render "sessions/otp_prompt"
render "sessions/mfa_prompt"
else
do_login
end
Expand Down
22 changes: 22 additions & 0 deletions app/controllers/webauthn_credentials_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
class WebauthnCredentialsController < ApplicationController
before_action :redirect_to_signin, unless: :signed_in?

def index
@user = current_user
end

def destroy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

credential = current_user.webauthn_credentials.find_by(id: params[:id])
if credential
credential.destroy
if credential.destroyed?
flash[:success] = t(".success")
else
flash[:error] = t(".fail")
end
else
flash[:error] = t(".not_found")
end
redirect_to webauthn_credentials_url
end
end
26 changes: 26 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class User < ApplicationRecord
has_many :deletions, dependent: :nullify
has_many :web_hooks, dependent: :destroy

has_many :webauthn_credentials, dependent: :destroy

after_validation :set_unconfirmed_email, if: :email_changed?, on: :update
before_create :generate_api_key, :generate_confirmation_token

Expand Down Expand Up @@ -172,6 +174,10 @@ def mfa_enabled?
!mfa_disabled?
end

def webauthn_enabled?
webauthn_credentials.any?
end

def disable_mfa!
mfa_disabled!
self.mfa_seed = ""
Expand Down Expand Up @@ -210,6 +216,26 @@ def otp_verified?(otp)
save!(validate: false)
end

def webauthn_verified?(current_challenge, webauthn_credential)
Copy link
Member

@sonalkr132 sonalkr132 Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

user_credential = webauthn_credentials.find_by!(external_id: webauthn_credential.id)

if webauthn_credential.user_handle.present?
return false unless webauthn_handle == webauthn_credential.user_handle
end

begin
webauthn_credential.verify(
current_challenge,
public_key: user_credential.public_key,
sign_count: user_credential.sign_count
)

user_credential.update!(sign_count: webauthn_credential.sign_count, last_used_on: Time.current)
rescue WebAuthn::Error
false
end
end

def block!
transaction do
update_attribute(:email, "security+locked-#{SecureRandom.hex(4)}-#{id}-#{handle}@rubygems.org")
Expand Down
4 changes: 4 additions & 0 deletions app/models/webauthn_credential.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class WebauthnCredential < ApplicationRecord
validates :external_id, :public_key, :nickname, presence: true
belongs_to :user
end
2 changes: 2 additions & 0 deletions app/views/profiles/edit.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

<% else %>
<p>
<%= t '.mfa.disabled' %>
Expand Down
Loading