Passkeys (experimental)

[tonkku107]

Adds support for passkeys behind an experimental config option.

Includes:

• Adding, removing and renaming passkeys in the account management frontend
• Signing in with a passkey

Does not include (will probably have to be separate PRs later):

• Ability to remove the password to go completely passwordless
• Registering with a passkey instead of a password
• Creating a new passkey during recovery instead of a password

Can be reviewed commit per commit.
Frontend is hacked together from existing components pending designs.

[tonkku107]

I was expecting webauthn-rs's dependence on openssl (3) to be a problem, but I wasn't expecting there to be a CI test for it 😁. Maybe webauthn_rp is a better alternative in this case. Library has been swapped

When I'm running the frontend tests locally, they're only rendering error pages and I don't know why. Would appreciate some pointers as to what I'm missing.

Either way, in the meanwhile @sandhose could we request for the designs?

[zacknewman]

@tonkku107, I only grokked the code; but it appears you plan to be storing a 16-byte identifier for the challenges. I'm not aware of the memory requirements, but RegistrationServerState and AuthenticationServerState are "only" 48 bytes and 64 bytes¹ in size respectively when using passkeys which may still be small enough that you can avoid storing them in the database entirely. The challenge they contain is based on a randomly-generated u128 which means they have the same amount of "entropy" as the 16-byte ID you plan on using. I realize that multiplicatively that is still 3 or 4 times the size of a 16-byte ID, but it's something you may want to consider.

Footnotes

1. At least on x86_64-unknown-linux-gnu platforms when compiled with rustc 1.85.1 using the release profile. ↩

[CLAassistant]

All committers have signed the CLA.

[zacknewman]

Some quick comments pertaining to webauthn_rp:

• As stated privately, 0.3.0 adds Decode impls for CredentialId<&[u8]> and UserHandle<&[u8]>, so some of this will be able to be simplified.
• In the case you still need to convert from something like CredentialId<Vec<u8>> to CredentialId<&[u8]>, you may be able to write something like (&cred_id).into() pending type inference. Still not the most ergonomic, but much more concise than CredentialId::<&[u8]>::from(&cred_id).
• When serde_relaxed is enabled, RegistrationVerificationOptions::client_data_json_relaxed and AuthenticationVerificationOptions::client_data_json_relaxed are true when relying on the Default impl. The idea being since you already opted into "relaxed" JSON deserialization, then it makes sense for this to default to true.
• 0.3.0 adds associated functions like Registration::from_json_relaxed; thus serde_json::from_str::<RegistrationRelaxed>(json).map(|reg| reg.0) could be written as Registration::from_json_relaxed(json).
• AuthTransports implements Encode and Decode, but I'm guessing you know this and you're intentionally storing the JSON array of strings instead because you don't want to deal with u8 being converted to an i8 since many databases don't support unsigned integers. Obviously it's faster and more concise to use the u8. Don't be "fooled" by AuthTransports' API. While it "behaves" like a collection (e.g., Vec), it's actually super efficient and is internally based on a u8 "flag".

Longer comment:

Origin validation is "complex" in that it's not particularly possible to support all the possible cases "out of the box". To fully empower downstream code, AuthenticationServerState::verify and RegistrationServerState::verify are polymorphic so that downstream code can use its own custom type to perform this validation so long as the type implements PartialEq<Origin<'_>>. In fact the type that is used for origin validation need not be the same as the type used for top origin validation—admittedly, it's likely extremely rare for one to want to perform origin validation differently than top origin validation. For strict validation, one should just use &str/String since the slice of &str/Strings are checked to match exactly with the Origin<'_>. For more complex matching, DomainOrigin<'_, '_> may be "good enough". If it is not flexible enough for your needs though, then you will have to implement your own type that does what you want.

DomainOrigin<'_, '_> first parses Origin<'_> via DomainOrigin::try_from which among other things requires Origin<'_> to have no scheme or a non-empty scheme and either no TCP/UDP port or a "canonical" TCP/UDP port. Upon successful parsing, case-sensitive matching is performed (e.g., the host example.com is not the same as Example.com).

A few examples that will fail origin validation when DomainOrigin { scheme: Scheme::Any, host: "localhost", port: Port::Any, } is used:

• "://localhost": while scheme is allowed to not exist, if one exists, it must be non-empty; thus DomainOrigin::try_from will fail.
• "https://Localhost": "localhost" doesn't match exactly with "Localhost".
• "https://localhost:0443": 0443 has a leading 0; thus DomainOrigin::try_from will fail.

Examples that will succeed:

• "localhost": scheme and port are allowed to not exist.
• "<U+0000>://localhost:0" where <U+0000> is NUL: any non-empty sequence of Unicode scalar values is a valid scheme and any 16-bit unsigned integer in "canonical" form is allowed.

In summary if this is too relaxed, too strict, or both for your needs; then you have no choice but to define your own type that parses Origin<'_> in its PartialEq<Origin<'_>> impl and does whatever you want.

[tonkku107]

may still be small enough that you can avoid storing them in the database entirely

Storing in memory won't be an option since multiple instances can be run at once.

The challenge they contain is based on a randomly-generated u128 which means they have the same amount of "entropy" as the 16-byte ID you plan on using.

I've added an Ulid to be consistent with the rest of the stuff in this project. There's actually only 10 bytes of entropy within the ID as Ulids reserve 6 bytes for a timestamp.

you may be able to write something like (&cred_id).into()

Yeah I know and I wasn't a big fan of that and ultimately moved it out to be more explicit about the weird type stuff going on.

but I'm guessing you know this and you're intentionally storing the JSON array of strings instead

I used JSON or strings wherever I could, but if database efficiency is a concern to the maintainers (which I doubt given my 60GB synapse DB full of JSON strings), that could be changed.

A few examples that will fail origin validation

Can you even get any of those examples out of a browser? 😄
The point there is to just have more lax verification for development purposes anyway. The real concerns about too strict or not strict enough would come from the case where the host of public_base is fed into DomainOrigin::new. Imo, it's just right for most deployments but some additional config options might be necessary for some people's needs.

[zacknewman]

Can you even get any of those examples out of a browser? 😄The point there is to just have more lax verification for development purposes anyway. The real concerns about too strict or not strict enough would come from the case where the host of public_base is fed into DomainOrigin::new. Imo, it's just right for most deployments but some additional config options might be necessary for some people's needs.

The code is probably OK. My comments were isolated to webauthn_rp without any regard with the rest of the code nor clients. The pedant in me just wanted to point out these technicalities that for most "normal" developers could be ignored. I'm a "weirdo" that obsesses over stuff though. For example, maybe I have my own modified version of Firefox that does allow such an origin. That's probably not a concern for this code though.

Since we are here, there is one more point I do think is important to make and that is related to the CBOR-parsing that happens internally. I strictly adhere to CTAP2 canonical CBOR encoding form. For the most part this is fine and is technically required by the spec; however there have been real-world cases where implementations violate it. For example, there was a time Firefox was not correctly encoding the kty value. Some libraries allow such incorrect encodings and others don't (e.g., I believe webauthn_rs rejects it).

I think most browsers correctly do it now though, so I haven't experienced issues in my tests. It's something you may want to be aware of though. Perhaps the bigger problem though is the CBOR-decoding that is done in webauthn_rp intentionally only supports the extensions that it "knows about". For example, if you attempt to use the largeBlob extension, a failure is guaranteed to happen even if you implement your own JSON deserialization. This is because that extension is not only a client extension but also an authenticator extension which means when RegistrationServerState::verify or AuthenticationServerState::verify is called, the CBOR-parsing that takes place will fail; furthermore you can't just "scrub it out" either since signature verification will fail—although honestly "scrubbing the data" would be a PITA, and you may want to implement your own library instead at that point.

[zacknewman]

I used JSON or strings wherever I could, but if database efficiency is a concern to the maintainers (which I doubt given my 60GB synapse DB full of JSON strings), that could be changed.

Well that is not true since you do use CredentialId::encode and UserHandle::encode. Both implement Serialize and Deserialize; thus you could change the code so that you store JSON strings. Additionally, you don't seem to care about MetadataOwned::decode; thus you could also call Metadata::into_json instead of Metadata::encode.

[tonkku107]

Well that is not true since you do use CredentialId::encode and UserHandle::encode.

I use serde_json::to_string and serde_json::from_str for CredentialIds. User handles I constructed from the Ulid's bytes.

Additionally, you don't seem to care about MetadataOwned::decode; thus you could also call Metadata::into_json instead of Metadata::encode.

Did not notice that as I was looking for the serialization impls. The metadata is quite useless anyway.

[sandhose]

I haven't reviewed the frontend part in-depth yet, but most of this looks really solid! In terms of UI, for now my main request is that the webauthn flow starts on the login page; this is my very quick non-designer proposal:

[sandhose]

This is supposed to be a 'buffersource', so raw bytes? Shouldn't that be a Vec<u8> / BYTEA?

[tonkku107]

Whichever works, the webauthn api provides both a base64 string and an ArrayBuffer (though both get sent as base64 thanks to toJSON). credential_id needs to be queried so a string format probably works better and that reminds me that it probably needs an index as well...

[sandhose]

I think I would rather have that the exact type, even if that means making mas-data-model depend on webauthn-rp? This way, potential errors comes at the repository level, and there is no need for post-processing those later

[sandhose]

Learned this the hard way: foreign keys don't create indexes!

So for each FK, please also do an explicit index, e.g.

CREATE INDEX user_passkeys_user_id_fk ON user_passkeys (user_id);

It's fine to create indexes non-concurrently on tables you've just created, but for FKs you're creating on existing tables, you need to create those with CREATE INDEX CONCURRENTLY to avoid locking the table during migration.

The other caveat, is that CREATE INDEX CONCURRENTLY must run outside of a transaction, and by default, migrations are run in transactions. You can opt-out of this by having -- no-transaction at the top of the file… which also means you can have a single statement in the migration.

See #4385 for example migrations for FK indexes on existing tables

[sandhose]

Ideally can you put new dependencies at the workspace level?

[sandhose]

I think this should either default to None if there is no displayname, or to the MXID instead. We already use the username as the user entity name

[sandhose]

Suggested change

	let response = serde_json::from_str::<RegistrationRelaxed>(&response)?.0;
	let RegistrationRelaxed(response) = serde_json::from_str(&response).context("Invalid PublicKeyCredential sent by client")?;

[sandhose]

Suggested change

	let response = serde_json::from_str::<DiscoverableAuthenticationRelaxed16>(&response)?.0;
	let DiscoverableAuthenticationRelaxed16(response) = serde_json::from_str(&response)?;

[sandhose]

Ideally this shouldn't be its own page for the form, but on the login page.

The "sign in with passkey" should trigger the challenge directly; which also means we can unconditionally create a challenge on the login page, even if this means we get a bunch of "ghost" challenges.

This will allow us to do conditional mediation on the username form on the login page

[sandhose]

This could render a React component as well. This can feel overkill, but I'd rather do this than manual DOM manipulation

Something like:

• in the template, put in a root, which will not render anything if JS is disabled (which is fine)

  <div id="passkey-root"></div>

• then render a react component in it, like

  createRoot(document.getElementById("passkey-root" as HTMLElement).render(
    <Suspense fallback="Loading…">
      <I18nextProvider i18n={i18n}>
        <WebAuthnButton options={options} />
      </I18nextProvider>
    </Suspense>
  )

or something like that. Vite should properly code-split and this shouldn't result into a giant JS payload for this page

[sandhose]

Looks like this polyfill exists to do that: https://github.com/MasterKale/webauthn-polyfills

[tonkku107]

I did use that at first, but it turned out that it didn't follow the spec and was missing fields.