Webauthn support

feature
#14

Argh, I knew I was missing a route in review. Good catch :heart:

I think it’s been like this for a while, but yes these are good changes

Agree, good change.

Perhaps we can re-use the built-in chrome copy here? “You have already registered this security key. You don’t have to register it again.” is a nice clear copy.

9 Likes
#15

Thanks @Falco and @sam for your feedback. I didn’t realise there was a different route for mobile login either! I will start work on these fixes including the password labelling/button changes tonight, hopefully even open a new PR to fix!

7 Likes
#16

I’m really glad this worked on your Android as well (even though the mobile view is not working correctly) — I didn’t have an Android to test with.

6 Likes
#17

May I recommend the Xiaomi Mi 9?

3 Likes
#18

I’m not sure I’m ready to return to Android – I love my iPhone 8 too much :sweat_smile:

4 Likes
#19

Here is the PR to fix up the above :rocket:

5 Likes
#20

Who said return? That is old world thinking! Modern people own multiple devices :wink:

7 Likes
#21

This is very nice!

One minor niggle though: on Safari/Mac, Web Authentication is a developer-only feature, off by default. When enabled, it works well. But we should likely show a message or a warning when Web Authentication is not enabled. Currently, on default Safari, nothing in the UI indicates that the registration process won’t work (the console has an error):

image
image.png2848Ă—1542 461 KB

10 Likes
#22

We can feature detect using navigator.credentials I assume.

9 Likes
#23

Some people have a hard time justifying the extra expense even though it’s nice :smiley:

6 Likes
#24

Argh I did that feature check on navigator.credentials.get but not create sorry about that, will be a quick fix!

7 Likes
#25

The most recent commit seems to be working. I was able to use 2-factor (with fingerprint!) on my android phone just now.

5 Likes
#26

Hey Penar, I’ve got a fix up for this now, are you able to check it out on Safari? [FIX] Check webauthn support when registering security keys by mjrbrennan · Pull Request #8146 · discourse/discourse · GitHub . I’m just not sure if the method of feature detection I’m using will work with Safari having the feature turned off.

9 Likes
#27

Sam beat me to the merge but this looks good on Safari, thanks Martin!

9 Likes
#28

Right but in this case it is a normal part of work at Discourse so we would cover the cost. Sorry if that was not clear, but hopefully it is clear now?

8 Likes
#29

Congrats on WebAuthn support! Interesting to see you rolled your own solution instead of using the webauthn gem, if there’s any feedback for us there I’d love to hear it :smiley:

I noticed in your implementation only supports algorithm -7 (ES256), but Windows Hello platform authenticators (backed by TPM 2.0 hardware) require -257 (RS256) as per Microsoft’s documentation. TPM 2.0 is required since July 28, 2016 for new Windows 10 desktop models, so that’s not an insignificant amount of hardware.

One suggestion for the “Login Flow” mockup - WebAuthn has an official logo that could be used instead of a generic fingerprint image. Besides fingerprint, facial recognition, swipe pattern or PIN are common user verification options as well.

9 Likes
#30

@Martin_Brennan any thoughts on :arrow_double_up:

5 Likes
#31

Thank you for the feedback Rafe, we will need to add the additional algorithm here then. And thank you for the link to the official logo! My thought when I set only one algorithm was to just add the minimum number of supported algorithms to get things working for V1, as I wasn’t sure of nuances between all the algorithms, and ES256 was used in all the examples I could get my hands on.

As for why at the time I opted not to use the gem, I agonized over this quite a bit, and I knew I would be asked about this decision at some point. I certainly read through a lot of the code in the gem to get a better understanding of the implementation. The main reasons were:

  1. Not wanting to add an additional dependency to Discourse. Every gem dependency adds additional overhead, no matter how excellent the code in that gem may be.
  2. Wanting to have a good understanding of how such a critical security piece of Discourse operates. I thought that having the code within the core of Discourse would make things clearer and easier to extend, without having a “black box” so to speak. (Not an indictment of the gem itself or its complexity, I realize people could just go look at the code there, and I found it quite easy to follow what was going on).
  3. Not wanting to add more than the minimum required code to get Webauthn running, for example I left out attestation from our initial implementation. I didn’t think there was much point adding a whole toolshed when all we needed was a spanner.

That being said, this could have been misguided on my part, and if we get into V2, V3 etc. of webauthn support with attestations, more supported algorithms, and what have you, it may become too laborious for us to become “Webauthn experts” so to speak, and at that point I think we could re-evaluate use of the gem.

15 Likes
#34

Support for the additional RS256 algorithm is now merged FEATURE: Support RS256 algorithm for webauthn by martin-brennan · Pull Request #8385 · discourse/discourse · GitHub

13 Likes
#35

It seems, iOS 13.3 and macOS Catalina 10.15.2 are now supporting FIDO2-auth as well :partying_face:

This update also includes bug fixes and other improvements. This update:

  • Adds support for NFC, USB, and Lightning FIDO2-compliant security keys in Safari
8 Likes