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

2FA (Webauthn) #5795

Merged
merged 84 commits into from
Jun 17, 2019
Merged

2FA (Webauthn) #5795

merged 84 commits into from
Jun 17, 2019

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented May 6, 2019

Begins work on Webauthn as a supported 2FA method.

TODO:

  • Fill out the utils.webauthn helper methods
  • Link the helpers up to the Webauthn model via IUserService
  • Frontend integration (navigator.credentials)
  • Provisioning, removing, authentication with security keys
  • Allow multiple security keys
  • Unit tests

cc @brainwane @nlhkabu @di @ewdurbin @dstufft

@woodruffw woodruffw mentioned this pull request May 6, 2019
@woodruffw
Copy link
Member Author

Hey @brainwane @di @dstufft @ewdurbin -- this should be good for a review!

We're still waiting on duo-labs/py_webauthn#41 for multiple WebAuthn credential support; we can either push forwards with this PR and begin a new one for multiple credentials, or wait on that (and begin another task) while they cut a release. Thoughts?

@di
Copy link
Member

di commented Jun 3, 2019

I think I'd prefer to merge this for support w/ multiple credentials if the Duo team's timeline is compatible with ours.

@mschwager, is there anything you can do to help us resolve duo-labs/py_webauthn#41?

@ewdurbin
Copy link
Member

ewdurbin commented Jun 3, 2019

This seems functionally excellent.

Looks like we still need to sort out:

  • Two skipped tests Edit: looks like I was viewing outdated comments
  • Design concern on the TOTP vs WebAuthn forms/routes
  • UX/UI @nlhkabu will you have time to review?
  • Multiple registered devices, blocked on webauthn release

@nlhkabu
Copy link
Contributor

nlhkabu commented Jun 13, 2019

@woodruffw I've now updated the UI:

No 2fa

Screenshot from 2019-06-13 06-33-12

2fa by TOTP only

Screenshot from 2019-06-13 06-34-06

Single security key

Screenshot from 2019-06-13 06-35-12

Multiple security keys

Screenshot from 2019-06-13 06-56-46

Add key page

Screenshot from 2019-06-13 06-44-38

Remove key modal

Screenshot from 2019-06-13 06-38-01

@nlhkabu
Copy link
Contributor

nlhkabu commented Jun 13, 2019

@woodruffw it seems I've broken the remove modal - when trying to remove a key, I am getting a 404 error. I can't work out how to fix it - could you please look into this?

@woodruffw
Copy link
Member Author

Sure, I'll take a look in a moment.

Copy link
Member

@ewdurbin ewdurbin left a comment

Choose a reason for hiding this comment

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

Once we get the signing counter issues sorted, this looks 💯 to me.

@@ -83,6 +84,7 @@ <h2><a href="#my-account">My Account</a></h2>
<li><a href="#compromised-password">{{ compromised_password() }}</a></li>
<li><a href="#2fa">{{ twoFA() }}</a></li>
<li><a href="#totp">{{ totp() }}</a></li>
<li><a href="#utfkey">{{ utfkey() }}</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

I did a double-take at utf 🙂
IIUC this is about U2F keys, not Unicode’s UTF, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is about U2F keys and not Unicode's UTF; the T stands for "two". I'm ok with this since it's just an anchor tag, but if you want to change it I figure a PR would be pretty easy!

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.

9 participants