[WIP] FEATURE: Webauthn authenticator management with 2FA login (Security Keys) (PR #8099)

Users can now add Security Keys as second factor authentication method via the Webauthn API. RFC on Meta: Webauthn support - feature - Discourse Meta.

Users can register security keys from the Second Factor Authentication preferences screen. They can give each key a name and edit the name & disable a key if required. When they register the key they are prompted to insert and use their physical key:

image image

When the user logs in and they have active security keys, they are shown a prompt to authenticate using a security key. When they click “Authenticate With Security Key” they are prompted to use their physical key. In the case of having multiple keys registered, we just tell the webauthn API all of the allowed credential IDs for the user, and the first key matching any of them is used when logging in.

image

If a user does not have their security key with them they can choose to fall back to TOTP based 2FA.

image

The missing case I’ve found is “What happens when a user has only security keys registered and loses all of them?”. This seems to happen as well when I only set up one TOTP code and no backup codes, I cannot proceed in login if I have “lost” my code. What happens normally here?:

image

Implementation Assumptions

  • I have used Discourse.current_hostname for the Relaying Party ID (Web Authentication: An API for accessing Public Key Credentials - Level 2) which should be the “effective domain” of the relaying party. In localhost this is just localhost and I assume in production sites it would be meta.discourse.org, forums.envato.com etc.
  • I’ve left out certain validation steps that are either optional or unnecessary for an initial implementation, e.g. clientExtensionResults, fully verifying the attestation statement using trust anchors (using attestation format of none for now), checking tokenBinding.status. This does not reduce the security of the implementation.
  • I’ve just used a single supported public key algorithm for now – -7 which is ECDSA w/ SHA-256. CBOR Object Signing and Encryption (COSE)
  • I have only tested the implementation with Yubikeys; I do not have an Android phone / macOS / any other physical hardware authentication methods

Dependencies Added

  • base64js.min.js - Used for converting base64 to and from bytes, for use with webauthn APIs
  • cbor (gem) - webauthn credential attestation data is CBOR encoded, and we need to decode it to inspect the attestation
  • cose (gem) - the public key for the webauthn credential is encoded in the COSE key format

Schema Changes

  • Added a UserSecurityKey model and database table. The information stored for security keys differs enough from the UserSecondFactor table that they need to be stored on their own. Also, at some point security keys will be used for first + multi factor auth.
  • Added secure_identifier column to User which is only written when it is needed. This was done to satisfy webauthn.credentials.create() by providing a random string to identify a Discourse user. Web Authentication: An API for accessing Public Key Credentials - Level 2. Using the user ID integer worked OK on an older Yubikey, but did not work at all on the YubiKey 5 NFC, so I think it’s safer to go with a random string for this.

WIP TODO

  • The code currently is feature complete, however controller tests for the session controller + user controller as well as a QUnit integration spec is still missing.

GitHub

You’ve signed the CLA, mjrbrennan. Thank you! This pull request is ready for review.

base64js.min.js

IE9 is not a release target for us, can we just use the built in implementation? https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/btoa

1 Like

can we require: false the new gems?

1 Like

Is there a reason you made an action to delegate to another action like this? You could probably pass action in the template instead and not have this code.

Shouldn’t this be scheduleOnce? I believe schedule runs over and over and scheduleOnce will only execute once.

This likely will fail the linter. Can you confirm that you’ve run our linting on this PR? I see failures in Travis.

I tend to prefer the catch syntax for promises.

You don’t need to close modal if it’s in finally

I’m curious why you couldn’t pass credential here rather than converting it into a FormData?

Trivial but it’s preferred to indent within handlebars blocks.

This is a bit odd - why is there an empty link in the translation? It would be better to keep such markup in the handlebars code.

There is a scope for enabled above - thoughts on adding that as part of an index?

No time like the present :slight_smile:

P.S. Overall this is looking excellent! A ton of good work in here.

Argh thought I got rid of this one! Removing, as this fabricator is no longer relevant

Thank you both for your feedback so far, I’m making changes now!

The above second_factor translation has it as well, I thought that it must have been a convention followed. I will change it and the other translation to not contain an empty link

This schedule code was already there. I’m not overly familiar with Ember but what you’re saying sounds right + correlates with the docs, will change

base64js.min.js

IE9 is not a release target for us, can we just use the built in implementation? WindowOrWorkerGlobalScope.btoa() - Web APIs | MDN

@SamSaffron hmm you’re right, but a bit of extra transformation needs to happen before btoa because the webauthn API provides ArrayBuffers. Will implement the one-liner here https://stackoverflow.com/a/11562550 instead and get rid of the base64.min.js dependency