-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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 as an alternative 2FA method #14466
Add WebAuthn as an alternative 2FA method #14466
Conversation
Can we avoid using name WebAuthn as seen in “Sign in using WebAuthn” GIF and instead provide helpful title and description to the user? (“Achtung! 2FA authorization”, “something something use your verified security key to sign in”)
If you are using internalization methods to display the strings on the pages, then that's the job for translators on Crowdin — when feature is merged, source translation files will be synced, and we'll get on it! :) |
Webauthn rules. Love to see it. |
Good catch! Sure I'll do it!
Great! Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very ambitious PR, happy to see tests included, not sure about some changes though.
Gemfile
Outdated
@@ -100,6 +100,7 @@ gem 'twitter-text', '~> 1.14' | |||
gem 'tzinfo-data', '~> 1.2020' | |||
gem 'webpacker', '~> 5.1' | |||
gem 'webpush' | |||
gem 'webauthn', '= 3.0.0.alpha1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there is a good reason this should use ~>
operator.
@@ -18,18 +18,21 @@ def new | |||
end | |||
|
|||
def create | |||
if current_user.validate_and_consume_otp!(confirmation_params[:otp_attempt]) | |||
if current_user.validate_and_consume_otp!(confirmation_params[:otp_attempt], otp_secret: session[:new_otp_secret]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this new_otp_secret
from session?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you take a look at how the OTP setup flow works, basically when you click on the Set Up button in the OTP setup page you are hitting TwoFactorAuthentication::OtpAuthenticationController.rb#create
which generates a new otp secret
that needs to be passed to this controller TwoFactorAuthentication::ConfirmationsController.rb
in order to generate the QR Code – in the action new
– and later to verify if the code 6-digit code introduced was valid – create
action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand it, isn't the otp_secret
from the current_user
enough? I don't understand how the flow changed from what it previously was, here.
end | ||
|
||
def create | ||
session[:new_otp_secret] = User.generate_otp_secret(32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this separated like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know exactly why it was implemented like this, we just renamed this controller from TwoFactorAuthenticationController
to TwoFactorAuthentication::OtpAuthenticationController
.
.fields-group | ||
= f.input :otp_attempt, type: :number, wrapper: :with_label, label: t('simple_form.labels.defaults.otp_attempt'), input_html: { 'aria-label' => t('simple_form.labels.defaults.otp_attempt'), :autocomplete => 'off' }, autofocus: true | ||
- if @webauthn_enabled | ||
= render partial: 'auth/sessions/two_factor/webauthn_form', locals: { hidden: @scheme_type != 'webauthn' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it important to render this if you're gonna hide it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that when you are attempting to log in and you have WebAuthn
enabled, you will land on a page to use your security keys to log in, but if you want to log in using an OTP code from your phone you have to be able to do so, and that's why we added that link below in the last GIF: Don't you have your security key? Enter a two-factor code from your phone or a recovery code. That link will render the page for using an OTP code as second factor authentication.
In the current implementation, when users hit create
from SessionsController
, if they have second factor enabled, the action will render this two_factor
view. So, suppose that instead of that view we have two: otp_2fa_form
and webauthn_2fa_form
, the problem is that if we wanted to change between them using a link, we would have to hit SessionsController#create
again with user's username and password and render, which is not possible.
So, answering your question, as we couldn't find another way of doing it, we decided of introduce the code of the two forms in the same view and use JavaScript to hide and show one or the other.
|
||
- if Setting.site_contact_email.present? | ||
%p.hint.subtle-hint= t('users.otp_lost_help_html', email: mail_to(Setting.site_contact_email, nil)) | ||
= render partial: 'auth/sessions/two_factor/otp_authentication_form', locals: { hidden: @scheme_type != 'totp' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, what's the point of always rendering this if it's going to be hidden?
config/initializers/webauthn.rb
Outdated
WebAuthn.configure do |config| | ||
# This value needs to match `window.location.origin` evaluated by | ||
# the User Agent during registration and authentication ceremonies. | ||
config.origin = ENV["WEBAUTHN_ORIGIN"] || "http://localhost:3000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New WEBAUTHN_ORIGIN
environment variable? Shouldn't this equal the "local (or web) domain" setting set elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this with LOCAL_DOMAIN
but tests are failing – which make sense if you take a look at the environment variables for tests: LOCAL_DOMAIN
should be http://test.host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admins aren't going to set a new environment variable just for this which must be identical with an existing one anyway.
Try using Rails.config.x.web_domain
for this, though you'll also need to use Rails.config.x.use_https
since the other variable does not contain the scheme part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admins aren't going to set a new environment variable just for this which must be identical with an existing one anyway.
Try using Rails.config.x.web_domain for this, though you'll also need to use Rails.config.x.use_https since the other variable does not contain the scheme part.
Got it! Then I will try to do it in that way 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I got it to work using Rails.configuration.x.web_domain
and Rails.configuration.x.use_https
.
I tried this with LOCAL_DOMAIN but tests are failing – which make sense if you take a look at the environment variables for tests: LOCAL_DOMAIN should be http://test.host
Don't pay attention to this, it works properly using the environment variables LOCAL_DOMAIN
and LOCAL_HTTPS
.
I just forgot to make changes in the tests that I was manually setting the domain of the FakeClient
to be http://test.host
.
See: 830000b
config/routes.rb
Outdated
resource :two_factor_authentication, only: [:show, :create, :destroy] | ||
resources :two_factor_authentication_methods, only: [:index] do | ||
collection do | ||
post 'disable' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code style consistency-wise, if there's no /
in the path we use symbols like post :disable
UserMailer.two_factor_disabled(@user).deliver_later! | ||
UserMailer.webauthn_disabled(@user).deliver_later! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that cause users to receive two mails whenever they disable 2FA, regardless of which authentication methods they had set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you're right 😅
I think we'll be good if we just remove the email that notifies that WebAuthn was disabled and leave just the email that states that 2FA is disabled, as the first one is redundant since the latter email includes it.
render json: options_for_get, status: :ok | ||
else | ||
flash[:error] = t('webauthn_credentials.not_enabled') | ||
render json: { redirect_path: sign_in_path }, status: :unauthorized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure setting a flash
together with a json render makes a lot of sense, but I may be missing context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the JSON will be used to redirect the page, then the flash will be displayed on that page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the JSON will be used to redirect the page, then the flash will be displayed on that page.
That was basically the idea, although we later changed our minds about how to show error messages to users, so now there's no need of setting the flash in this case.
What we are doing instead is adding the html of the error messages and showing/hiding them when needed.
flash.now[:alert] = t('webauthn_credentials.invalid_credential') | ||
render json: {}, status: :unauthorized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, the use of flash.now
with a json
reply seems suspicious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flash.now
will definitely not work with or without redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we were trying to do here was to set the flash message and then reload the page using javascript so it renders properly, but, as you correctly point out this will not work using flash.now
.
Anyway, as I mentioned in the other thread, we changed the way we displayed error messages so I will update this 🙂
This adds a basic UI for enabling WebAuthn 2FA. We did a little refactor to the Settings page for editing the 2FA methods – now it will list the methods that are available to the user (TOTP and WebAuthn) and from there they'll be able to add or remove any of them. Also, it's worth mentioning that for enabling WebAuthn it's required to have TOTP enabled, so the first time that you go to the 2FA Settings page, you'll be asked to set it up. This work was inspired by the one donde by Github in their platform, and despite it could be approached in different ways, we decided to go with this one given that we feel that this gives a great UX. Co-authored-by: Facundo Padula <[email protected]>
This commits adds the feature for using WebAuthn as a second factor for login when enabled. If users have WebAuthn enabled, now a page requesting for the use of a WebAuthn credential for log in will appear, although a link redirecting to the old page for logging in using a two-factor code will also be present. Co-authored-by: Facundo Padula <[email protected]>
Co-authored-by: Facundo Padula <[email protected]>
Co-authored-by: Facundo Padula <[email protected]>
Following examples form other platforms like Github, we decided to make Webauthn 2FA secondary to 2FA with TOTP, so that we removed the possibility of removing TOTP authentication only, leaving users with just WEbAuthn as 2FA. Instead, users will have to click on 'Disable 2FA' in order to remove second factor auth. The reason for WebAuthn being secondary to TOPT is that in that way, users will still be able to log in using their code from their phone's application if they don't have their security keys with them – or maybe even lost them. * We had to change a little the flow for setting up TOTP, given that now it's possible to setting up again if you already had TOTP, in order to let users modify their authenticator app – given that now it's not possible for them to disable TOTP and set it up again with another authenticator app. So, basically, now instead of storing the new `otp_secret` in the user, we store it in the session until the process of set up is finished. This was because, as it was before, when users clicked on 'Edit' in the new two-factor methods lists page, but then went back without finishing the flow, their `otp_secret` had been changed therefore invalidating their previous authenticator app, making them unable to log in again using TOTP. Co-authored-by: Facundo Padula <[email protected]>
The PR build was failing given that linting returning some errors. This commit attempts to fix them.
The build was failing given that i18n translations files were not normalized. This commits fixes that.
When an admins disable 2FA for users, we were sending two mails to them, one notifying that 2FA was disabled and the other to notify that WebAuthn was disabled. As the second one is redundant since the first email includes it, we can remove it and send just one email to users.
5edf459
to
0124ad1
Compare
0124ad1
to
d1a5b69
Compare
if (error.response.status === 422) { | ||
const errorMessage = document.getElementById('security-key-error-message'); | ||
errorMessage.classList.remove('hidden'); | ||
console.error(error.response.data.error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit ugly, but what I'm trying to do is to display an error message to users when an invalid key is used – where an unprocessable_entity
response is returned –, but handling the error differently if it's caused by another reason.
Obviously suggestions are welcomed and appreciated 🙂
d1a5b69
to
19131b7
Compare
This is a leftover for the work done in mastodon#14466.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to review after it got merged, but here is a partial review (I read most of it, but not most of the HTML/JS part yet).
If I understand correctly, WebAuthn can only be added after enabling TOTP, which cannot be disabled without disabling WebAuthn as well, is that right? That's not a dealbreaker to me, but it's a bit weird.
Also, as far as I can see, the “edit” flow for TOTP has a few issues:
- it's confusing
- it seems to allow generating a new TOTP secret without checking the previous one. This sounds like a security regression.
Furthermore, “disable 2FA” does not seem to check for TOTP nor WebAuthn. If you're logged in, you only get the password challenge, not the 2FA one. This is a security regression.
layout 'admin' | ||
|
||
before_action :authenticate_user! | ||
before_action :verify_otp_not_enabled, only: [:show] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be for :create
. Did anything change?
@@ -18,18 +18,21 @@ def new | |||
end | |||
|
|||
def create | |||
if current_user.validate_and_consume_otp!(confirmation_params[:otp_attempt]) | |||
if current_user.validate_and_consume_otp!(confirmation_params[:otp_attempt], otp_secret: session[:new_otp_secret]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand it, isn't the otp_secret
from the current_user
enough? I don't understand how the flow changed from what it previously was, here.
libssl-dev is provided with the stack image in build time and conflicts in building openssl Gem for webauthn Gem added with mastodon#14466.
libssl-dev is provided with the stack image in build time and conflicts in building openssl Gem for webauthn Gem added with #14466.
This is a leftover for the work done in #14466.
* feat: add possibility of adding WebAuthn security keys to use as 2FA This adds a basic UI for enabling WebAuthn 2FA. We did a little refactor to the Settings page for editing the 2FA methods – now it will list the methods that are available to the user (TOTP and WebAuthn) and from there they'll be able to add or remove any of them. Also, it's worth mentioning that for enabling WebAuthn it's required to have TOTP enabled, so the first time that you go to the 2FA Settings page, you'll be asked to set it up. This work was inspired by the one donde by Github in their platform, and despite it could be approached in different ways, we decided to go with this one given that we feel that this gives a great UX. Co-authored-by: Facundo Padula <[email protected]> * feat: add request for WebAuthn as second factor at login if enabled This commits adds the feature for using WebAuthn as a second factor for login when enabled. If users have WebAuthn enabled, now a page requesting for the use of a WebAuthn credential for log in will appear, although a link redirecting to the old page for logging in using a two-factor code will also be present. Co-authored-by: Facundo Padula <[email protected]> * feat: add possibility of deleting WebAuthn Credentials Co-authored-by: Facundo Padula <[email protected]> * feat: disable WebAuthn when an Admin disables 2FA for a user Co-authored-by: Facundo Padula <[email protected]> * feat: remove ability to disable TOTP leaving only WebAuthn as 2FA Following examples form other platforms like Github, we decided to make Webauthn 2FA secondary to 2FA with TOTP, so that we removed the possibility of removing TOTP authentication only, leaving users with just WEbAuthn as 2FA. Instead, users will have to click on 'Disable 2FA' in order to remove second factor auth. The reason for WebAuthn being secondary to TOPT is that in that way, users will still be able to log in using their code from their phone's application if they don't have their security keys with them – or maybe even lost them. * We had to change a little the flow for setting up TOTP, given that now it's possible to setting up again if you already had TOTP, in order to let users modify their authenticator app – given that now it's not possible for them to disable TOTP and set it up again with another authenticator app. So, basically, now instead of storing the new `otp_secret` in the user, we store it in the session until the process of set up is finished. This was because, as it was before, when users clicked on 'Edit' in the new two-factor methods lists page, but then went back without finishing the flow, their `otp_secret` had been changed therefore invalidating their previous authenticator app, making them unable to log in again using TOTP. Co-authored-by: Facundo Padula <[email protected]> * refactor: fix eslint errors The PR build was failing given that linting returning some errors. This commit attempts to fix them. * refactor: normalize i18n translations The build was failing given that i18n translations files were not normalized. This commits fixes that. * refactor: avoid having the webauthn gem locked to a specific version * refactor: use symbols for routes without '/' * refactor: avoid sending webauthn disabled email when 2FA is disabled When an admins disable 2FA for users, we were sending two mails to them, one notifying that 2FA was disabled and the other to notify that WebAuthn was disabled. As the second one is redundant since the first email includes it, we can remove it and send just one email to users. * refactor: avoid creating new env variable for webauthn_origin config * refactor: improve flash error messages for webauthn pages Co-authored-by: Facundo Padula <[email protected]>
libssl-dev is provided with the stack image in build time and conflicts in building openssl Gem for webauthn Gem added with mastodon#14466.
This is a leftover for the work done in mastodon#14466.
* feat: add possibility of adding WebAuthn security keys to use as 2FA This adds a basic UI for enabling WebAuthn 2FA. We did a little refactor to the Settings page for editing the 2FA methods – now it will list the methods that are available to the user (TOTP and WebAuthn) and from there they'll be able to add or remove any of them. Also, it's worth mentioning that for enabling WebAuthn it's required to have TOTP enabled, so the first time that you go to the 2FA Settings page, you'll be asked to set it up. This work was inspired by the one donde by Github in their platform, and despite it could be approached in different ways, we decided to go with this one given that we feel that this gives a great UX. Co-authored-by: Facundo Padula <[email protected]> * feat: add request for WebAuthn as second factor at login if enabled This commits adds the feature for using WebAuthn as a second factor for login when enabled. If users have WebAuthn enabled, now a page requesting for the use of a WebAuthn credential for log in will appear, although a link redirecting to the old page for logging in using a two-factor code will also be present. Co-authored-by: Facundo Padula <[email protected]> * feat: add possibility of deleting WebAuthn Credentials Co-authored-by: Facundo Padula <[email protected]> * feat: disable WebAuthn when an Admin disables 2FA for a user Co-authored-by: Facundo Padula <[email protected]> * feat: remove ability to disable TOTP leaving only WebAuthn as 2FA Following examples form other platforms like Github, we decided to make Webauthn 2FA secondary to 2FA with TOTP, so that we removed the possibility of removing TOTP authentication only, leaving users with just WEbAuthn as 2FA. Instead, users will have to click on 'Disable 2FA' in order to remove second factor auth. The reason for WebAuthn being secondary to TOPT is that in that way, users will still be able to log in using their code from their phone's application if they don't have their security keys with them – or maybe even lost them. * We had to change a little the flow for setting up TOTP, given that now it's possible to setting up again if you already had TOTP, in order to let users modify their authenticator app – given that now it's not possible for them to disable TOTP and set it up again with another authenticator app. So, basically, now instead of storing the new `otp_secret` in the user, we store it in the session until the process of set up is finished. This was because, as it was before, when users clicked on 'Edit' in the new two-factor methods lists page, but then went back without finishing the flow, their `otp_secret` had been changed therefore invalidating their previous authenticator app, making them unable to log in again using TOTP. Co-authored-by: Facundo Padula <[email protected]> * refactor: fix eslint errors The PR build was failing given that linting returning some errors. This commit attempts to fix them. * refactor: normalize i18n translations The build was failing given that i18n translations files were not normalized. This commits fixes that. * refactor: avoid having the webauthn gem locked to a specific version * refactor: use symbols for routes without '/' * refactor: avoid sending webauthn disabled email when 2FA is disabled When an admins disable 2FA for users, we were sending two mails to them, one notifying that 2FA was disabled and the other to notify that WebAuthn was disabled. As the second one is redundant since the first email includes it, we can remove it and send just one email to users. * refactor: avoid creating new env variable for webauthn_origin config * refactor: improve flash error messages for webauthn pages Co-authored-by: Facundo Padula <[email protected]>
libssl-dev is provided with the stack image in build time and conflicts in building openssl Gem for webauthn Gem added with mastodon#14466.
This is a leftover for the work done in mastodon#14466.
Yep, that's confusing. I wasn't aware that Mastodon supported WebAuthn until reading this PR. I opened a new issue for that: #16916 |
What
First stab at #562 in collaboration with @padulafacundo.
Prototype adding experimental support for WebAuthn as a 2nd factor, allowing users who already have 2FA (OTP) to register WebAuthn security keys and login using WebAuthn as 2FA or fallback to OTP
The goal of this first prototype is to have a working solution that can be tested to get early feedback and start the discussion.
Screenshots
Adding a new WebAuthn Credential
Deleting a WebAuthn Credential
Sign in using WebAuthn
How To Review