diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ce4c17..e355a41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,37 @@ ### Fixed +- The WebAuthn user handle is 64 random bytes instead of the UTF-8 of whatever + the caller passes as `userId`. An authenticator keeps one discoverable + credential per `(rp.id, user.id)` and **replaces** the previous one when both + match — silently, with no prompt and nothing to undo. Deriving the handle from + a typed name therefore meant two people registering as "anna" on a shared + device destroyed each other's passkey, and with it the DID and every entry + signed under it. The same happened to one person re-entering their usual name + after clearing storage. WebAuthn L2 §5.4.3 also forbids putting personally + identifying information in the handle, which a name or e-mail plainly is, and + the handle is stored in the authenticator indefinitely. + + `userId` keeps its rightful place as `user.name`, the label the credential + picker shows. It is a label only: it identifies nothing, two credentials may + carry the same one, and nothing in this package looks a credential up by it. + Closes #45. + + Nothing needs migrating. Recovery goes through discoverable credentials + (`readLargeBlobMetadata` calls `get()` with no `allowCredentials`), or through + an explicit credential ID where those are switched off — no path resolves a + credential by handle, so credentials registered under the old scheme keep + working untouched. The derived `did:key` comes from the credential's public + key and does not change. + + Consumers should expect a behavioural difference: re-registering under a name + that was used before now **adds** a passkey instead of replacing one. That is + the point — a replaced passkey is data loss, a second entry is a choice — but + it means the picker can show several, so `user.name` and `displayName` should + be distinguishing enough to pick from. The new random handle is returned as + `credential.userHandle` (base64url); the authenticator keeps its own copy, so + storing it is optional. + - Actually write the secret key into `largeBlob`. The keystore record carried `secretKey: sk, // Will be moved to largeBlob` — and nothing ever moved it. That was the only occurrence of the field in `src/`, and diff --git a/playwright.node.config.js b/playwright.node.config.js index f1e7eba..4b0e972 100644 --- a/playwright.node.config.js +++ b/playwright.node.config.js @@ -21,6 +21,7 @@ export default defineConfig({ 'webauthn-extension-support.test.js', 'webauthn-varsig-challenge.test.js', 'webauthn-attestation-parsing.test.js', + 'webauthn-user-handle.test.js', 'webauthn-two-peer-replication.test.js', ], fullyParallel: false, diff --git a/src/webauthn/provider.js b/src/webauthn/provider.js index a70fa13..916fb8f 100644 --- a/src/webauthn/provider.js +++ b/src/webauthn/provider.js @@ -82,7 +82,11 @@ export class WebAuthnDIDProvider { * Create a WebAuthn credential for OrbitDB identity * This triggers biometric authentication (Face ID, Touch ID, Windows Hello, etc.) * @param {Object} options - Credential options - * @param {string} options.userId - User ID + * @param {string} options.userId - Account label shown in the credential + * picker (`user.name`). A label only: it does not identify the credential, + * two credentials may carry the same one, and it is never used to look one + * up. The handle the authenticator files the credential under is generated + * here and returned as `userHandle`. * @param {string} options.displayName - Display name * @param {string} options.domain - Domain/RP ID * @param {boolean} options.encryptKeystore - Enable keystore encryption @@ -117,7 +121,19 @@ export class WebAuthnDIDProvider { // Generate challenge for credential creation const challenge = crypto.getRandomValues(new Uint8Array(32)); - const userIdBytes = new TextEncoder().encode(userId); + + // The user handle is an opaque key, not a label. An authenticator stores + // one discoverable credential per (rp.id, user.id) pair and *replaces* the + // previous one when both match — silently, with no prompt and nothing to + // undo. Deriving the handle from a typed name therefore meant two people + // registering as "anna" on a shared device destroyed each other's passkey, + // and with it the DID and everything written under it (#45). + // + // 64 random bytes, as WebAuthn L2 §5.4.3 recommends. The same section + // forbids putting personally identifying information here, which a typed + // name or e-mail address plainly is. The name keeps its rightful place in + // `user.name` below, where the credential picker shows it. + const userHandle = crypto.getRandomValues(new Uint8Array(64)); webauthnLog('Calling navigator.credentials.create() for user: %s', userId); @@ -130,7 +146,7 @@ export class WebAuthnDIDProvider { id: domain, }, user: { - id: userIdBytes, + id: userHandle, name: userId, displayName, }, @@ -247,6 +263,13 @@ export class WebAuthnDIDProvider { publicKey, userId, displayName, + // Nothing in this package looks a credential up by handle — recovery + // goes through discoverable credentials, or through an explicit + // credential ID when those are switched off. It is surfaced anyway + // because it is the only copy the caller will ever see, and a flow + // that one day passes `allowCredentials` needs it. Decode with + // `WebAuthnDIDProvider.base64urlToArrayBuffer()`. + userHandle: WebAuthnDIDProvider.arrayBufferToBase64url(userHandle), attestationObject: new Uint8Array( credential.response.attestationObject ), diff --git a/tests/helpers/mock-authenticator.js b/tests/helpers/mock-authenticator.js index e809043..7fc20a5 100644 --- a/tests/helpers/mock-authenticator.js +++ b/tests/helpers/mock-authenticator.js @@ -75,12 +75,23 @@ export async function createMockAuthenticator({ await crypto.subtle.exportKey('spki', keypair.publicKey) ); - const credentialId = crypto.getRandomValues( + // One resident credential per (rpId, user.id), keyed by the handle — the rule + // that makes a name-derived handle destructive (#45). A registration whose + // handle is already present replaces what was there; a new handle adds to it. + const residentByHandle = new Map(); + let activeCredentialId = crypto.getRandomValues( new Uint8Array(credentialIdLength) ); const rpIdHash = await sha256(new TextEncoder().encode(rpId)); - const state = { signCount: 0, assertions: 0, creations: 0, prfEvals: 0 }; + const state = { + signCount: 0, + assertions: 0, + creations: 0, + prfEvals: 0, + /** Every `user` dictionary passed to create(), in order. */ + createdUsers: [], + }; // Per-credential secret the PRF output is derived from. const prfKey = prfSecret ?? crypto.getRandomValues(new Uint8Array(32)); @@ -107,7 +118,10 @@ export async function createMockAuthenticator({ ) ); - const buildAuthData = ({ includeAttestedCredential }) => { + const buildAuthData = ({ + includeAttestedCredential, + credentialId = activeCredentialId, + }) => { const flagByte = includeAttestedCredential ? 0x01 | 0x04 | 0x40 // UP | UV | AT : 0x01 | 0x04; // UP | UV @@ -152,8 +166,23 @@ export async function createMockAuthenticator({ state.creations += 1; state.signCount += 1; + const user = options?.publicKey?.user ?? {}; + state.createdUsers.push(user); + + const handleKey = bytesToBase64url(new Uint8Array(user.id ?? [])); + const credentialId = crypto.getRandomValues( + new Uint8Array(credentialIdLength) + ); + // set(), not a guard: a repeat handle silently drops the credential + // that was stored under it, exactly as an authenticator does. + residentByHandle.set(handleKey, credentialId); + activeCredentialId = credentialId; + const challenge = options?.publicKey?.challenge ?? new Uint8Array(32); - const authData = buildAuthData({ includeAttestedCredential: true }); + const authData = buildAuthData({ + includeAttestedCredential: true, + credentialId, + }); const attestationObject = new Uint8Array( encode( new Map([ @@ -222,10 +251,10 @@ export async function createMockAuthenticator({ ); return { - id: bytesToBase64url(credentialId), - rawId: credentialId.buffer.slice( - credentialId.byteOffset, - credentialId.byteOffset + credentialId.byteLength + id: bytesToBase64url(activeCredentialId), + rawId: activeCredentialId.buffer.slice( + activeCredentialId.byteOffset, + activeCredentialId.byteOffset + activeCredentialId.byteLength ), type: 'public-key', response: { @@ -246,7 +275,16 @@ export async function createMockAuthenticator({ return { navigator: navigatorShim, publicKey: { algorithm: -7, x, y, keyType: 2, curve: 1 }, - credentialId, + /** The credential an assertion would use: the most recently created one. */ + get credentialId() { + return activeCredentialId; + }, + /** What the authenticator still holds, one entry per distinct handle. */ + residentCredentials: () => + [...residentByHandle.entries()].map(([handle, id]) => ({ + handle, + credentialId: bytesToBase64url(id), + })), state, }; } diff --git a/tests/webauthn-user-handle.test.js b/tests/webauthn-user-handle.test.js new file mode 100644 index 0000000..cdde70e --- /dev/null +++ b/tests/webauthn-user-handle.test.js @@ -0,0 +1,124 @@ +/** + * The WebAuthn user handle must not be derived from anything a person types. + * + * An authenticator keeps one discoverable credential per (rp.id, user.id) and + * replaces the previous one when both match — no prompt, no undo. While the + * handle was `TextEncoder().encode(userId)`, two people registering under the + * same name on one device destroyed each other's passkey, and with it the DID + * and every entry signed under it (#45). + * + * The first test is the load-bearing one: it asserts what the library *sends*, + * so it holds regardless of how faithfully the mock models an authenticator. + * The rest describe the consequence. + */ +import { test, expect } from '@playwright/test'; +import { + createMockAuthenticator, + installMockAuthenticator, +} from './helpers/mock-authenticator.js'; +import { silenceWebAuthnDebugLogging } from './helpers/two-peer.js'; + +const NAME = 'anna'; + +test.describe('WebAuthn user handle', () => { + let restoreAuthenticator; + let restoreLogging; + let authenticator; + let WebAuthnDIDProvider; + + test.beforeEach(async () => { + restoreLogging = silenceWebAuthnDebugLogging(); + authenticator = await createMockAuthenticator(); + restoreAuthenticator = installMockAuthenticator(authenticator); + ({ WebAuthnDIDProvider } = await import('../src/webauthn/provider.js')); + }); + + test.afterEach(() => { + restoreAuthenticator?.(); + restoreLogging?.(); + }); + + test('is 64 random bytes, and carries nothing the user typed', async () => { + await WebAuthnDIDProvider.createCredential({ + userId: NAME, + displayName: 'Anna', + }); + await WebAuthnDIDProvider.createCredential({ + userId: NAME, + displayName: 'Anna', + }); + + const [first, second] = authenticator.state.createdUsers; + const firstHandle = new Uint8Array(first.id); + const secondHandle = new Uint8Array(second.id); + + expect(firstHandle.length).toBe(64); + expect(secondHandle.length).toBe(64); + + // Same input, different handle — the property the whole fix rests on. + expect(Buffer.from(firstHandle).equals(Buffer.from(secondHandle))).toBe( + false + ); + + // And nothing recognisable in it: not the typed bytes, not the name as text. + const typed = new TextEncoder().encode(NAME); + expect( + Buffer.from(firstHandle.subarray(0, typed.length)).equals( + Buffer.from(typed) + ) + ).toBe(false); + expect(new TextDecoder().decode(firstHandle)).not.toContain(NAME); + }); + + test('keeps the typed value as the label the picker shows', async () => { + await WebAuthnDIDProvider.createCredential({ + userId: NAME, + displayName: 'Anna at the front desk', + }); + + const [user] = authenticator.state.createdUsers; + expect(user.name).toBe(NAME); + expect(user.displayName).toBe('Anna at the front desk'); + }); + + test('two people with the same name no longer overwrite each other', async () => { + const first = await WebAuthnDIDProvider.createCredential({ + userId: NAME, + displayName: 'Anna', + }); + const second = await WebAuthnDIDProvider.createCredential({ + userId: NAME, + displayName: 'Anna', + }); + + expect(second.credentialId).not.toBe(first.credentialId); + + // Both survive. Under a name-derived handle the second registration would + // have landed in the first one's slot and left a single credential here. + const resident = authenticator.residentCredentials(); + expect(resident.length).toBe(2); + expect(resident.map((entry) => entry.credentialId).sort()).toEqual( + [first.credentialId, second.credentialId].sort() + ); + }); + + test('returns the handle, base64url of 64 bytes', async () => { + const credential = await WebAuthnDIDProvider.createCredential({ + userId: NAME, + displayName: 'Anna', + }); + + expect(credential.userHandle).toBeTruthy(); + expect(credential.userHandle).toMatch(/^[A-Za-z0-9_-]+$/); + + const decoded = new Uint8Array( + WebAuthnDIDProvider.base64urlToArrayBuffer(credential.userHandle) + ); + expect(decoded.length).toBe(64); + expect( + Buffer.from(decoded).equals( + Buffer.from(new Uint8Array(authenticator.state.createdUsers[0].id)) + ) + ).toBe(true); + }); +}); diff --git a/types/index.d.ts b/types/index.d.ts index 2564db5..23d3fc0 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -91,6 +91,8 @@ export interface WebAuthnCredentialInfo { publicKey: WebAuthnPublicKey | Uint8Array; userId?: string; displayName?: string; + /** Random 64-byte WebAuthn user handle, base64url. Not derived from userId. */ + userHandle?: string; attestationObject?: Uint8Array; prfInput?: Uint8Array; did?: string; @@ -99,6 +101,7 @@ export interface WebAuthnCredentialInfo { } export interface CreateCredentialOptions { + /** Account label for the credential picker. Not an identifier. */ userId?: string; displayName?: string; domain?: string;