Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new FxA DB migration level (195) introducing a passkeys_wraps table intended to store per-passkey key-wrapping “envelopes” (PRF-wrapped recovery keypair material and HPKE-sealed kB) and wiring it to passkeys via a composite ON DELETE CASCADE FK so passkey deletion also deletes its wrap.
Changes:
- Add forward migration
patch-194-195.sqlto createpasskeys_wrapswith(uid, credentialId)primary key and a composite cascading FK topasskeys(uid, credentialId). - Add rollback migration
patch-195-194.sql(commented-out rollback steps, consistent with existing rollback patch style). - Bump DB schema target level from 194 → 195.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/db-migrations/databases/fxa/patches/patch-194-195.sql | Creates the new wraps table and advances schema patch level to 195. |
| packages/db-migrations/databases/fxa/patches/patch-195-194.sql | Provides the rollback patch steps (commented), returning schema patch level to 194. |
| packages/db-migrations/databases/fxa/target-patch.json | Updates the target schema patch level to 195. |
| pkR VARBINARY(512) NOT NULL, -- v1: 133B, uncompressed P-521 point | ||
| prfWrappedSkR VARBINARY(512) NOT NULL, -- v1: 257B, AES-256-GCM(pkcs8 skR) + tag | ||
| keyWrapIv BINARY(12) NOT NULL, -- AES-GCM nonce | ||
| hpkeEncapsulatedSecret VARBINARY(512) NOT NULL, -- v1: 133B, RFC 9180 Nenc for DHKEM(P-521) | ||
| hpkeSealedKb VARBINARY(512) NOT NULL, -- v1: 48B, 32B kB + tag |
There was a problem hiding this comment.
My thinking was that we give a little bit of wiggle room so we're less likely to hit issues where the write is rejected because the size changes, but I'm unsure if that's even possible for the size to change. If it's not, or it shouldn't change, than I can make it more strict
There was a problem hiding this comment.
IUC, those sizes are fixed and based on the crypto libraries. If this is infact true, and we potentially have a bunch of records here, we should make them the exact size. Also use binary instead of varbinary. This is consistent with how columns that hold wrapped kbs work.
There was a problem hiding this comment.
I wasn't sure obviously, but thanks for confirming! I'll get that updated
c0f8db3 to
bb72fb9
Compare
nshirley
changed the title
804e7a8 to
3e293be
Compare
| -- opaque. Matches the passkeys table. | ||
| credentialId VARBINARY(1023) NOT NULL, | ||
| pkR BINARY(133) NOT NULL, -- uncompressed P-521 point: 0x04 || x(66) || y(66) | ||
| prfWrappedSkR BINARY(257) NOT NULL, -- AES-256-GCM(pkcs8 skR, 241B) + 16B tag |
There was a problem hiding this comment.
an alternative to consider here - The panva hpke library we're considering has a DeserializePrivateKey helper we may want to use, and it would make prfWrappedSkR 82 bytes instead of 257 (raw 66-byte scalar). Two things favour it for a format we can't change later: 66 is a spec constant while 241 rests on an optional DER field, and it's the only option with a post-quantum path. It also derives the public point itself, so the pkR slicing risk doesn't apply.
There was a problem hiding this comment.
Thanks for the suggestion - I did a bit of digging and would agree it makes sense; dropping the extras out of the PKCS container saves space and enforces a stronger contract between front and back end. I've made the adjustment
Because: * Passkey-based sync sign-in needs somewhere to store the PRF-wrapped recovery keypair and the HPKE-sealed kB, one envelope per passkey. This commit: * Adds patch-194-195.sql creating passkeyWraps, keyed on (uid, credentialId) with an ON DELETE CASCADE FK to passkeys so wraps are removed with their passkey. deleteAccount_24 needs no change. * Sizes each crypto column as fixed-width BINARY holding exactly what the v1 ciphersuite emits. skR is stored as the raw 66-byte scalar (Nsk) rather than a pkcs8 export, so every length is a ciphersuite constant: pkcs8's 241 bytes rest on Web Crypto emitting an OPTIONAL public-key field, and a conforming implementation that omitted it would be zero-padded into the fixed column and never open again. * Keeps credentialId as VARBINARY(1023), matching passkeys: WebAuthn credential IDs are authenticator-chosen and genuinely variable. * Adds the patch-195-194.sql rollback and bumps target-patch to 195.
Because
keypair and the HPKE-sealed
kB, one envelope per passkey. This is the storagemodel from the technical spec §2.2.5.
builds on this table, so the schema needs to land first.
This pull request
packages/db-migrations/databases/fxa/patches/patch-194-195.sql, creatingpasskeyWrapskeyed on(uid, credentialId). Every payload column isfixed-length and sized from the v1 ciphersuite
(
DHKEM(P-521, HKDF-SHA512)/HKDF-SHA512/AES-256-GCM).ON DELETE CASCADEforeign key topasskeys(uid, credentialId),so deleting a passkey drops its envelope. Account deletion needs no procedure
change:
deleteAccount_24already deletes frompasskeysbeforeaccounts, andwraps cascade with that delete.
patch-195-194.sqlrollback and bumpstarget-patch.jsonto 195.Issue that this pull request solves
Closes: FXA-13138
Checklist
Put an
xin the boxes that applyHow to review (Optional)
patch-194-195.sql— specifically thatuid,credentialId, and the timestamp types matchpasskeysexactly, since thecomposite FK cannot be created otherwise.
lets this land without touching
deleteAccount_24.Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Verified against a local DB.
packages/db-migrationshas no automated schemaassertions, so these checks are manual:
table and returns the level to 194; forward then re-applies cleanly.
wrap intact.
CALL deleteAccount_24(uid, reason)succeeded with a wrap present — account,passkeys, and wraps all removed,
deletionReasonrecorded — with no change to theprocedure.
Purely additive, and no application code reads the table yet, so this must deploy ahead
of the data model that consumes it.