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

Allow using webauthn in places that currently require a password #4271

Merged
merged 1 commit into from Dec 16, 2023

Conversation

segiddins
Copy link
Member

@segiddins segiddins commented Dec 7, 2023

i.e. allow using passkeys as a single auth factor

Demo: https://share.cleanshot.com/9cmkZf62

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fdbbcb5) 98.71% compared to head (20677a3) 98.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4271      +/-   ##
==========================================
+ Coverage   98.71%   98.73%   +0.01%     
==========================================
  Files         330      336       +6     
  Lines        7341     7414      +73     
==========================================
+ Hits         7247     7320      +73     
  Misses         94       94              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@segiddins segiddins force-pushed the segiddins/webauthn-instead-of-passwords branch 4 times, most recently from 141bf8e to 3811143 Compare December 8, 2023 20:24
@segiddins segiddins marked this pull request as ready for review December 8, 2023 20:25
@martinemde martinemde self-requested a review December 8, 2023 22:24
@segiddins segiddins force-pushed the segiddins/webauthn-instead-of-passwords branch from 3811143 to 4333a23 Compare December 9, 2023 00:19
Copy link
Member

@indirect indirect left a comment

Choose a reason for hiding this comment

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

Looks great! I'm happy to merge this now.

I'm also interested in collecting feedback about the idea of using a cookie or the email address (once it has been entered) to trigger a request for a passkey auth. The UX patterns around how to make it easy to use passkeys haven't really shaken out yet, and I'm interested in anything we can do to make it easier and less hassle to use passkeys rather than emails and passwords.

@martinemde
Copy link
Member

using a cookie or the email address (once it has been entered) to trigger a request for a passkey auth.

Agreed.

Copy link
Member

@martinemde martinemde left a comment

Choose a reason for hiding this comment

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

I read this thoroughly and it all looks good. I have some issues with the testing approach. It's fine for this PR since it's just copied, but I think we could be more exact with auth tests and rely less on vague textual proxies for being signed in or out.

test/system/sign_in_webauthn_test.rb Show resolved Hide resolved
test/test_helper.rb Show resolved Hide resolved
Comment on lines 241 to +245
post 'webauthn_create', to: 'sessions#webauthn_create', as: :webauthn_create
post 'webauthn_full_create', to: 'sessions#webauthn_full_create', as: :webauthn_full_create
Copy link
Member

Choose a reason for hiding this comment

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

Nit: could we either explain somewhere or update the names of these actions to better capture what exactly they are and what's different between them? Why isn't webauthn_create as full as webauthn_full_create?.

Copy link
Member

@jenshenny jenshenny left a comment

Choose a reason for hiding this comment

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

I haven't taken too close of a look at the code yet, but user tested it through and this is great! 🎉

Just want to double check... if we are making webauthn a single auth factor on signin, then we are deciding that users don't need to provide another factor even if they have MFA enabled? Do you think some users would want to sign in with a username/password and then webauthn as another factor? If so, do you think there be an opt-in / opt-out option for this feature?

@segiddins
Copy link
Member Author

Given webauthn is using pre-exchanged public key cryptography, I think using it as a "single factor" is just as safe as traditional 2FA. It is resistant to replay attacks, phishing (by tying the relying party to the origin). I think the standard is to allow using passkeys in place of passwords pretty much everywhere I've seen? But I am open to making this configurable if there's significant prior art for it

@indirect
Copy link
Member

I'm happy to hear more from folks more focused on security, but my understanding is that using webauthn/passkey login is designed to be the same level of security as password+OTP. For example, here is GitHub's documentation about passkeys:

Passkeys allow you to sign in securely to GitHub.com, without having to input your password. If you use two-factor authentication (2FA), passkeys satisfy both password and 2FA requirements, so you can complete your sign in with a single step. You can also use passkeys for sudo mode and resetting your password.

@jenshenny
Copy link
Member

Ok cool thanks 👍 that is the sentiment that I was getting in that webauthn is as safe as password+OTP, but it felt a bit odd that if you signin with a password, you still need to webauthn if you have it enabled (I guess that makes things extra secure? 😄 ).

I didn't realize that Github had something similar!

@indirect
Copy link
Member

I think I would describe it as "passkeys provide the security of 2FA plus a password", so if you only enter a password you still need 2FA (which can be provided by a passkey), but if you provide a passkey, you no longer also need to provide a password. :)

@segiddins segiddins force-pushed the segiddins/webauthn-instead-of-passwords branch from 4333a23 to 03593e3 Compare December 11, 2023 21:14
i.e. allow using passkeys as a single auth factor
@segiddins segiddins force-pushed the segiddins/webauthn-instead-of-passwords branch from 03593e3 to 20677a3 Compare December 11, 2023 21:28
@segiddins
Copy link
Member Author

@jenshenny does that answer your question?

Copy link
Member

@jenshenny jenshenny left a comment

Choose a reason for hiding this comment

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

It does!

@segiddins segiddins merged commit 81f537e into master Dec 16, 2023
16 checks passed
@segiddins segiddins deleted the segiddins/webauthn-instead-of-passwords branch December 16, 2023 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants