diff --git a/packages/mfa-recovery-controller/ARCHITECTURE.md b/packages/mfa-recovery-controller/ARCHITECTURE.md index 94ea602e0e9..6d81e27de31 100644 --- a/packages/mfa-recovery-controller/ARCHITECTURE.md +++ b/packages/mfa-recovery-controller/ARCHITECTURE.md @@ -34,19 +34,18 @@ flowchart LR | --------------------------- | --------------------------------------------------------------------------- | | `pendingOperation` | Only persisted field: encrypted `authorizing` / `writing` blob, or `null` | | `escrows[]` | Build-time replica set; non-empty, unique `id`s; mutations require **all** | -| `authProvider` | Profile id + request-bound AuthController token (Hydra JWT) | +| `authProvider` | Profile id + request-bound Auth API approval | | `identifierAuthProvider` | Key-bound IdP assertion (passkey / OIDC / SIWE) | | `pendingOperationEncryptor` | Encrypts / decrypts the pending blob with wallet secure storage | | `now` | Clock (Unix seconds) for AuthController token expiry; defaults to `unixNow` | | `#withLock` | Serializes every public method except `authenticateIdentifier` | -Each escrow has two identities: `id` is the replica id used in mutation -`audiences` and receipts; `authAudience` is the audience the AuthController -token must carry for that escrow. They are independent (Cubist uses `"cubist"` -for both). +`id` identifies the replica in mutations and receipts; `authAudience` identifies +the approval audience. Cubist uses `id: "cubist"` and an `authAudience` matching +`AUTH_CONTROLLER_AUDIENCE`. Identifier types pick the auth mode from a trusted table, not a client flag: -`passkey` / `oidc` / `siwe` → key-bound. Any other type is rejected with +`passkey` / `social` / `siwe` → key-bound. Any other type is rejected with `unknown_identifier_type`. `emailOtp` / `smsOtp` escrow-challenge is not wired yet (MFA-605 / MFA-606 / MFA-568). @@ -113,10 +112,13 @@ Anything that fails is rejected rather than acted on: must match the mutation fields. - `register` has `expectedVersion` `0` and `identifier: null`; updates carry an identifier. -- `writing` only: the AuthController token's `sub` is the mutation - `profileId`, its `ext.request_hash` is the mutation `requestHash`, updates - have `aal >= 2`, `register` / `updateIdentifiers` carry `identifiers_hash`, - and receipts are well-formed with at most one per escrow. +- `writing` only: the approval binds `sub` to `profileId` and `ext.request_hash` + to `requestHash`, with `aal >= 2`. `register` / `updateIdentifiers` also need + `identifiers_hash` to match `approvedIdentifiers`, and every payload + identifier to equal one of them exactly. This runs before `writing` is + persisted: an escrow rejection is not a `Deny`, so a mismatch found there + would leave an unabortable write. Receipts must be well-formed with at most + one per escrow. ## Mutation sequence (`register` / updates) @@ -164,15 +166,20 @@ sequenceDiagram end ``` -AuthController token request: - -- `requestHash` is the mutation `requestHash`; `audiences` is every escrow's - `authAudience`. -- Updates set `requireTwoFactor`. `register` and `updateIdentifiers` pass the - payload `identifiers` so the token is bound to them. -- The token is a compact Hydra JWT. The controller decodes its claims without - verifying the signature (escrows verify it) and only uses them for - pending-state checks and the `exp` refresh. +Auth approvals: + +- The provider receives the operation's `requestHash` and each target escrow's + `authAudience`. All writes and reads set `requireTwoFactor` for AAL2. +- The issuer controls approval lifetime. AAL2 requires recent MFA. +- The approval's `identifiers` are persisted with its token as + `approvedIdentifiers` and refreshed with it. `register` and + `updateIdentifiers` send them to every escrow, which checks them against + `identifiers_hash` and rejects payload identifiers outside them. Hashing + sorts object keys, not arrays. +- Missing or stale MFA rejects with `AuthenticationController`'s + `StepUpRequiredError`. +- Escrows verify signatures. The controller decodes claims only for pending-state + validation and expiry-based refresh. Receipts: @@ -204,11 +211,11 @@ retries only escrows that do not already have a stored receipt. Updates re-run identifier auth (a new key-bound token and PoP) for those escrows on every attempt, so the identifier provider must be able to authenticate again. -## Cubist store sequence +## Cubist integration The controller does not call CubeSigner, the registration API, or -`KeyringController`. A Cubist deploy injects one `CubistEscrowProvider` -(`id` and `authAudience`: `"cubist"`). The caller proves OIDC, calls +`KeyringController`. A Cubist deploy injects one `CubistEscrowProvider`. +The caller proves OIDC, calls `ensure-user`, opens the session, and constructs the provider with the authenticated `CubeSignerClient` and the pinned wrap and receipt public keys before `register`. @@ -223,53 +230,7 @@ the mutation id, request hash, escrow id, `version === newVersion`, `receiptKeyId` (SHA-256 of the pinned receipt key), and the Ed25519 signature locally. -`register(recoverySecret, identifiers)` takes an identifier **array** of at -least two distinct key-bound identifiers. One `siwe` identifier is rejected. -`applyMutation` is the store: the controller wraps the secret to the escrow -wrap key, and the Cubist C2F persists that payload. - -`authenticateIdentifier` asks the injected identifier provider for a key-bound -token. A SIWE provider signs with `KeyringController.signPersonalMessage`. -The controller does not. `getRecoverySecret` then challenges the escrow, signs -the proof of possession locally, calls `getSecret`, and unwraps. `epoch` is -the replica `version`. - -```mermaid -sequenceDiagram - participant Caller as Caller - participant Auth as RecoveryAuthProvider - participant IdP as SIWE identifier provider - participant Ctrl as MfaRecoveryController - participant Escrow as Cubist escrow - - Note over Caller,Escrow: proveOidcIdentity, ensure-user, createOidcSession, then new CubistEscrowProvider - - Caller->>Ctrl: register(secret, identifiers) - Ctrl->>Escrow: isAvailable (ping) - Ctrl->>Auth: getAuthenticatedProfileId - Ctrl->>Ctrl: persist authorizing - Ctrl->>Auth: authorizeRecoveryRequest - Auth-->>Ctrl: AuthController token (Hydra JWT) - Ctrl->>Ctrl: wrap secret, persist writing - Ctrl->>Escrow: applyMutation - Escrow-->>Ctrl: receipt - Ctrl->>Ctrl: verify receipt, clear pending - - Caller->>Ctrl: authenticateIdentifier(siwe) - Ctrl->>IdP: getKeyBoundIdentifierToken - Note over IdP: signPersonalMessage - IdP-->>Ctrl: token - Ctrl-->>Caller: identifierSession - - Caller->>Ctrl: getRecoverySecret(identifierSession) - Ctrl->>Escrow: isAvailable (ping) - Ctrl->>Escrow: generateChallenge - Ctrl->>Ctrl: sign proof of possession - Ctrl->>Escrow: getSecret - Escrow-->>Ctrl: wrapped secret + version + wrapKeyId - Ctrl->>Ctrl: check wrapKeyId, unwrap - Ctrl-->>Caller: secret bytes + epoch -``` +For a single-escrow walkthrough, see [Cubist test sequence](CUBIST_TEST_SEQUENCE.md). ## Read sequence (`getRecoverySecret`) @@ -283,6 +244,7 @@ sequenceDiagram participant C as Client participant M as MfaRecoveryController participant I as IdentifierAuthProvider + participant A as RecoveryAuthProvider participant E as Available escrows C->>M: authenticateIdentifier(identifier) @@ -296,9 +258,11 @@ sequenceDiagram alt none available M-->>C: no_available_escrow end + M->>A: authorizeRecoveryRequest (read hash, AAL2) + A-->>M: request-bound approval M->>E: generateChallenge + PoP (partial OK) par read - M->>E: getSecret + M->>E: getSecret(approval, authorization, requestId, pkE) M->>M: check wrapKeyId, unwrap with ephemeral key end M->>M: selectHighestConsistentVersion @@ -330,7 +294,7 @@ the highest one. The next mutation still requires `resume()` first. ```mermaid flowchart TD Start[identifier + requestHash] --> Mode{trusted type?} - Mode -->|passkey / oidc / siwe| KB[fresh proof key, IdP key-bound token] + Mode -->|passkey / social / siwe| KB[fresh proof key, IdP key-bound token] KB --> Chal[each escrow generateChallenge] Chal --> PoP[sign PoP over hash of token + challengeId + requestHash] Mode -->|emailOtp / smsOtp| Todo[TODO MFA-605 / MFA-606 / MFA-568] @@ -341,22 +305,3 @@ On **mutation**, every remaining target escrow must authorize or the write does not start (`identifier_auth_failed`). On **read**, escrows whose challenge fails are skipped. `register` sends no identifier authorization (`null`). - -## Crash / repair - -```mermaid -flowchart TD - Crash[crash or partial apply] --> Phase{pending phase?} - Phase -->|none| Idle[idle] - Phase -->|authorizing| AbortOrResume{client choice} - AbortOrResume -->|abort| Idle - AbortOrResume -->|resume| Auth[re-issue AuthController token] - Auth --> Write[replicate] - Phase -->|writing| ResumeOnly[resume only] - ResumeOnly --> Skip[skip escrows that already have verified receipts] - Skip --> Refresh[re-issue token only if expired] - Refresh --> Write - Write --> All{receipts for every configured escrow?} - All -->|yes| Clear[clear pending] - All -->|no| PendingErr[IncompleteMutationError] -``` diff --git a/packages/mfa-recovery-controller/CHANGELOG.md b/packages/mfa-recovery-controller/CHANGELOG.md index 4899431a3a3..deff0452d99 100644 --- a/packages/mfa-recovery-controller/CHANGELOG.md +++ b/packages/mfa-recovery-controller/CHANGELOG.md @@ -10,7 +10,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Initial release of `MfaRecoveryController`, which manages MFA recovery flows and persists in-flight operations in its state ([#10022](https://github.com/MetaMask/core/pull/10022)) + - Requires recent, request-bound AAL2 approvals for writes and reads. + - Sends the approval's identifiers to escrows as `approvedIdentifiers` with `register` and `updateIdentifiers` mutations, and rejects payload identifiers that are not exactly among them before writing ([#10883](https://github.com/MetaMask/core/pull/10883)) - Exports `MfaRecoveryController`, `getDefaultMfaRecoveryControllerState`, and the controller's action, event, messenger, state, and options types. + - Exports `AuthApiRecoveryAuthProvider`, `AuthApiRecoveryAuthProviderOptions` and `AuthApiRecoveryAuthProviderMessenger` for requesting approvals through `AuthenticationController:requestMfaApproval`, using the profile id from `AuthenticationController:refreshCanonicalProfileId` ([#10883](https://github.com/MetaMask/core/pull/10883)) - Exports `CubistEscrowProvider` and `CubistEscrowProviderOptions` for storing recovery secrets with CubeSigner. - Exports `MfaRecoveryError`, `IncompleteMutationError`, and `MutationRejectedError`. - Exports `MIN_IDENTIFIERS`, `IDENTIFIER_AUTH_MODES`, and `getIdentifierAuthMode` for identifier authentication. diff --git a/packages/mfa-recovery-controller/CUBIST_TEST_SEQUENCE.md b/packages/mfa-recovery-controller/CUBIST_TEST_SEQUENCE.md index 1e45cfaa617..57c2a324207 100644 --- a/packages/mfa-recovery-controller/CUBIST_TEST_SEQUENCE.md +++ b/packages/mfa-recovery-controller/CUBIST_TEST_SEQUENCE.md @@ -7,47 +7,14 @@ each one. Error branches, pending-state repair, and multi-escrow behavior are in [ARCHITECTURE.md](./ARCHITECTURE.md); treat the TypeScript in `src/` as the source of truth if a diagram drifts. -| Step | Client call | Version | -| ---- | ------------------------------------------------------------ | ------- | -| 1 | `register(secret, [siwe, passkey])` | `0 → 1` | -| 2 | `updateRecoverySecret(passkey, newSecret, epoch)` | `1 → 2` | -| 3 | `updateIdentifiers(siwe, [siwe, replacementPasskey], epoch)` | `2 → 3` | +| Step | Client call | Read with | Version | +| ---- | ------------------------------------------------------------ | -------------------- | ------- | +| 1 | `register(secret, [siwe, passkey])` | `siwe` | `0 → 1` | +| 2 | `updateRecoverySecret(passkey, newSecret, epoch)` | `passkey` | `1 → 2` | +| 3 | `updateIdentifiers(siwe, [siwe, replacementPasskey], epoch)` | `replacementPasskey` | `2 → 3` | Each update passes the `epoch` returned by the previous read. -## Overview - -```mermaid -sequenceDiagram - autonumber - participant Mobile as Mobile client - participant C as MfaRecoveryController - participant Cubist as Cubist escrow - - Note over Mobile,Cubist: Setup: sign in, open Cubist session, build controller - - Mobile->>C: register(secret, [siwe, passkey]) - C->>Cubist: applyMutation (version 0 → 1) - Cubist-->>C: signed receipt - Mobile->>C: read with siwe - C->>Cubist: getSecret - C-->>Mobile: secret, epoch 1 - - Mobile->>C: updateRecoverySecret(passkey, newSecret, 1) - C->>Cubist: applyMutation (version 1 → 2) - Cubist-->>C: signed receipt - Mobile->>C: read with passkey - C->>Cubist: getSecret - C-->>Mobile: newSecret, epoch 2 - - Mobile->>C: updateIdentifiers(siwe, [siwe, replacementPasskey], 2) - C->>Cubist: applyMutation (version 2 → 3) - Cubist-->>C: signed receipt - Mobile->>C: read with replacementPasskey - C->>Cubist: getSecret - C-->>Mobile: same secret, epoch 3 -``` - ## Setup The mobile client does this before it creates the controller. The controller @@ -83,7 +50,9 @@ sequenceDiagram ## Write: `register`, `updateRecoverySecret`, `updateIdentifiers` -All three mutations share one path. +All mutations require a request-bound AAL2 approval. `register` and +`updateIdentifiers` also send the approval's identifiers as +`approvedIdentifiers`, bound by `identifiers_hash`. ```mermaid sequenceDiagram @@ -98,7 +67,7 @@ sequenceDiagram C->>Auth: getAuthenticatedProfileId() Auth-->>C: profileId Note over C: Build the mutation (version epoch → epoch + 1)
and its requestHash - C->>Auth: authorizeRecoveryRequest(requestHash, ...) + C->>Auth: authorizeRecoveryRequest(requestHash, requireTwoFactor: true, ...) Auth-->>C: JWT bound to requestHash C->>Cubist: isAvailable @@ -107,17 +76,17 @@ sequenceDiagram end Note over C: Encrypt the secret to the Cubist wrap key - C->>Cubist: applyMutation(mutation, JWT, { token, proof } or null, payload) + C->>Cubist: applyMutation(mutation, JWT, { token, proof } or null, payload, approvedIdentifiers) Cubist-->>C: signed receipt Note over C: Verify the receipt with the pinned receipt key C-->>Mobile: done ``` -| Operation | Identifier proof | JWT requires | Payload | -| ---------------------- | ----------------------- | ---------------- | -------------------- | -| `register` | none | identifiers | identifiers + secret | -| `updateRecoverySecret` | the identifier argument | 2FA | secret | -| `updateIdentifiers` | the identifier argument | 2FA, identifiers | identifiers | +| Operation | Identifier proof | Payload | +| ---------------------- | ----------------------- | -------------------- | +| `register` | none | identifiers + secret | +| `updateRecoverySecret` | the identifier argument | secret | +| `updateIdentifiers` | the identifier argument | identifiers | If an escrow refuses the mutation (`MutationRejectedError`) and every replica does the same, the controller clears the pending mutation. A transport failure @@ -126,47 +95,19 @@ to finish that write. ## Proof of possession -Updates and reads prove control of a registered identifier in two parts. The -identifier provider signs a token that names a fresh proof key and the -`requestHash`. Then the controller signs a Cubist challenge with that proof -key. Code: `requestKeyBoundIdentifierToken` and `authorizeEscrowsWithToken` in -`src/identifier-auth.ts`. - -```mermaid -sequenceDiagram - autonumber - participant C as MfaRecoveryController - participant Id as identifierAuthProvider - participant Cubist as Cubist escrow - - rect rgba(128, 128, 128, 0.1) - Note over C,Id: Part 1: proof key + identifier token - Note over C: generateSigningKey() → Ed25519 proof key pair - C->>Id: getKeyBoundIdentifierToken(identifier, proofPublicKey, requestHash) - Note over Id: binding = hash([proofPublicKey, requestHash])
SIWE: personal_sign a SIWE message, Nonce = binding
Passkey: WebAuthn assertion, challenge = binding - Id-->>C: token { identifier, proofPublicKey, requestHash, providerAssertion } - end +Updates and reads prove control of a registered identifier: - rect rgba(128, 128, 128, 0.1) - Note over C,Cubist: Part 2: challenge + signature - C->>Cubist: generateChallenge() - Cubist-->>C: challenge { id, escrowId, expiresAt } - Note over C: signature = sign(proofPrivateKey,
hash([token, challenge.id, requestHash])) - Note over C: authorization = { kind: key-bound, token,
proof: { challengeId, requestHash, signature } } - C->>Cubist: applyMutation or getSecret(authorization, ...) - Note over Cubist: providerAssertion is valid for the identifier
and binds proofPublicKey + requestHash
identifier is registered, challenge id is its own
signature verifies under proofPublicKey - end -``` +1. The controller generates an Ed25519 proof key and requests an identifier token + binding `hash([proofPublicKey, requestHash])`: the SIWE nonce or WebAuthn challenge. +2. The proof key signs `hash([token, challenge.id, requestHash])` for each escrow. + The controller sends the approval and `{ kind: 'key-bound', token, proof }` + with `applyMutation` or `getSecret`. +3. The escrow C2F verifies the provider assertion, registered identifier, request + binding, challenge, and proof signature. -- The identifier provider signs once, over the proof key and `requestHash`. - The proof key then signs each escrow challenge, so the token works only for - this request. -- The proof private key is never sent to the identifier provider or Cubist. - For reads it lives only in the session returned by `authenticateIdentifier`. -- The Cubist-side checks happen in the escrow C2F (`cubist_secret_escrow`), - not in this package. -- `register` sends no identifier proof (`null`); the JWT from `authProvider` - is the only authorization. +The proof private key never leaves the controller/caller; read sessions keep it +in memory. `register` sends `null` instead of an identifier proof. Implementation: +`requestKeyBoundIdentifierToken` and `authorizeEscrowsWithToken` in `src/identifier-auth.ts`. ## Read: `authenticateIdentifier`, `getRecoverySecret` @@ -175,6 +116,7 @@ sequenceDiagram autonumber participant Mobile as Mobile client participant C as MfaRecoveryController + participant Auth as authProvider participant Id as identifierAuthProvider participant Cubist as Cubist escrow @@ -186,8 +128,10 @@ sequenceDiagram Mobile->>C: getRecoverySecret(session) Note over C: Recompute requestHash, must equal token.requestHash C->>Cubist: isAvailable + C->>Auth: authorizeRecoveryRequest(requestHash, requireTwoFactor: true, ...) + Auth-->>C: request-bound AAL2 approval Note over C,Cubist: Proof of possession, part 2: challenge + signature - C->>Cubist: getSecret({ token, proof }, requestId, pkE) + C->>Cubist: getSecret(approval, { token, proof }, requestId, pkE) Cubist-->>C: secret encrypted to the read key, version Note over C: Decrypt the secret C-->>Mobile: { recoverySecret, epoch: version } diff --git a/packages/mfa-recovery-controller/README.md b/packages/mfa-recovery-controller/README.md index bbd5ca03500..d1b3e5dd761 100644 --- a/packages/mfa-recovery-controller/README.md +++ b/packages/mfa-recovery-controller/README.md @@ -1,13 +1,26 @@ # `@metamask/mfa-recovery-controller` -Manages MFA recovery flows across MetaMask clients. Auth and escrow storage are -injected so the same controller can run against different identity providers and -escrow backends. - -Pending mutations are persisted as encoded encrypted `authorizing` / `writing` -state so a crash can `resume()` the same mutation. `abort()` drops `authorizing` -only; `writing` must `resume()`. The encoded ciphertext is stored as a string so -BaseController state serialization preserves the encrypted value. +Coordinates MFA recovery through injected auth and escrow providers. + +Pending mutations are encrypted in controller state. `resume()` completes them; +`abort()` is allowed only before writing. See [Architecture](ARCHITECTURE.md). + +## Auth API Integration + +Configure `AuthApiRecoveryAuthProvider` with a messenger that can call +`AuthenticationController:refreshCanonicalProfileId` and +`AuthenticationController:requestMfaApproval`. It resolves the current canonical +profile id for each mutation and requests approvals from `/api/v2/mfa/approvals`. Writes and reads require a +request-bound AAL2 approval; updates and reads also require an identifier proof. +Without a recent MFA verification, approvals reject with `StepUpRequiredError`: +verify, then call `resume()` or retry the read. + +`register` and `updateIdentifiers` send the approval's `identifiers` to each +escrow as `approvedIdentifiers`; the escrow rejects payload identifiers outside +that list. Build payload identifiers from those entries unchanged (verifier +included): the controller rejects any other identifier before writing, and the +mutation can then be aborted. Set `CubistEscrowProviderOptions.authAudience` to the escrow's +`AUTH_CONTROLLER_AUDIENCE`. ## Installation diff --git a/packages/mfa-recovery-controller/package.json b/packages/mfa-recovery-controller/package.json index 2a0a073d956..68c31709efe 100644 --- a/packages/mfa-recovery-controller/package.json +++ b/packages/mfa-recovery-controller/package.json @@ -56,6 +56,7 @@ "@metamask/base-controller": "^10.1.0", "@metamask/cryptography": "^1.1.2", "@metamask/messenger": "^3.0.0", + "@metamask/profile-sync-controller": "^36.0.0", "@metamask/superstruct": "^3.4.1", "@metamask/utils": "^12.0.1", "json-stable-stringify": "^1.3.0" diff --git a/packages/mfa-recovery-controller/scripts/register-with-controller.ts b/packages/mfa-recovery-controller/scripts/register-with-controller.ts index d559a793e42..11f39f1fbf2 100644 --- a/packages/mfa-recovery-controller/scripts/register-with-controller.ts +++ b/packages/mfa-recovery-controller/scripts/register-with-controller.ts @@ -26,13 +26,13 @@ import { * Usage: * yarn workspace @metamask/mfa-recovery-controller run cubesigner:register -- \ * --wrap-key \ - * --receipt-key \ + * --receipt-key --auth-audience \ * [--function-id ] [--version latest|v0] * * Keys are raw 32-byte public keys, hex-encoded. */ const USAGE = - 'Usage: cubesigner:register --wrap-key --receipt-key [--function-id ] [--version latest|v0]'; + 'Usage: cubesigner:register --wrap-key --receipt-key --auth-audience [--function-id ] [--version latest|v0]'; const PASSKEY: Identifier = { type: 'passkey', @@ -41,11 +41,10 @@ const PASSKEY: Identifier = { verifier: { publicKey: 'mock-passkey' }, }; -const OIDC: Identifier = { - type: 'oidc', +const SOCIAL: Identifier = { + type: 'social', namespace: 'https://accounts.google.com', - value: 'sub-1', - verifier: { auds: ['mock-client'] }, + value: 'hashed-email', }; type RootMessenger = Messenger< @@ -109,7 +108,12 @@ function getControllerMessenger(): MfaRecoveryControllerMessenger { async function main(flags: Record): Promise { const wrapPublicKey = flags['wrap-key']; const receiptPublicKey = flags['receipt-key']; - if (wrapPublicKey === undefined || receiptPublicKey === undefined) { + const authAudience = flags['auth-audience']; + if ( + wrapPublicKey === undefined || + receiptPublicKey === undefined || + authAudience === undefined + ) { console.error(USAGE); process.exitCode = 1; return; @@ -118,6 +122,7 @@ async function main(flags: Record): Promise { const client = await CubeSignerClient.create(defaultUserSessionManager()); const escrow = new CubistEscrowProvider({ client, + authAudience, wrapPublicKey: hexToBytes(wrapPublicKey), receiptPublicKey: hexToBytes(receiptPublicKey), ...(flags['function-id'] === undefined @@ -127,14 +132,14 @@ async function main(flags: Record): Promise { }); const controller = new MfaRecoveryController({ messenger: getControllerMessenger(), - authProvider: new StubAuthProvider(), + authProvider: new StubAuthProvider([PASSKEY, SOCIAL]), identifierAuthProvider: new StubIdentifierAuthProvider(), escrows: [escrow], pendingOperationEncryptor: passthroughEncryptor, }); const recoverySecret = new Uint8Array(randomBytes(32)); - await controller.register(recoverySecret, [PASSKEY, OIDC]); + await controller.register(recoverySecret, [PASSKEY, SOCIAL]); const identifierSession = await controller.authenticateIdentifier(PASSKEY); const recovered = await controller.getRecoverySecret(identifierSession); diff --git a/packages/mfa-recovery-controller/src/MfaRecoveryController.test.ts b/packages/mfa-recovery-controller/src/MfaRecoveryController.test.ts index 00767af8802..dfeff2f26e6 100644 --- a/packages/mfa-recovery-controller/src/MfaRecoveryController.test.ts +++ b/packages/mfa-recovery-controller/src/MfaRecoveryController.test.ts @@ -16,7 +16,6 @@ import { passthroughEncryptor, } from '../tests/stubs.js'; import { - canonicalizeIdentifiers, generateSigningKey, generateWrapKey, hash, @@ -44,14 +43,15 @@ const PASSKEY: Identifier = { verifier: { publicKey: 'pk' }, }; -const OIDC: Identifier = { - type: 'oidc', +const SOCIAL: Identifier = { + type: 'social', namespace: 'https://accounts.google.com', - value: 'sub-1', - verifier: { auds: ['client'] }, + value: 'hashed-email', }; -const IDENTIFIERS = [PASSKEY, OIDC]; +const OTHER_PASSKEY: Identifier = { ...PASSKEY, value: 'cred-2' }; + +const IDENTIFIERS = [PASSKEY, SOCIAL]; const SECRET = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]); const SECRET_2 = new Uint8Array([8, 7, 6, 5, 4, 3, 2, 1]); @@ -187,6 +187,31 @@ describe('MfaRecoveryController', () => { }); }); + it.each([ + ['an AAL1 approval', { requireTwoFactor: false }, {}], + ['identifiers the approval does not bind', {}, { identifiers: [SOCIAL] }], + ])( + 'rejects %s before writing', + async (_name, paramsPatch, approvalPatch) => { + await withController(async ({ controller, authProvider, escrowA }) => { + const approve = + authProvider.authorizeRecoveryRequest.bind(authProvider); + vi.spyOn(authProvider, 'authorizeRecoveryRequest').mockImplementation( + async (params) => ({ + ...(await approve({ ...params, ...paramsPatch })), + ...approvalPatch, + }), + ); + const apply = vi.spyOn(escrowA, 'applyMutation'); + + await expect( + controller.register(SECRET, IDENTIFIERS), + ).rejects.toMatchObject({ code: 'invalid_pending_operation' }); + expect(apply).not.toHaveBeenCalled(); + }); + }, + ); + it('publishes state changes through the stateChanged event', async () => { await withController(async ({ controller, rootMessenger }) => { const listener = vi.fn(); @@ -367,6 +392,20 @@ describe('MfaRecoveryController', () => { }); describe('getRecoverySecret', () => { + it('requests a request-bound AAL2 approval for each read', async () => { + await withController(async ({ controller, authProvider }) => { + await controller.register(SECRET, IDENTIFIERS); + const authorize = vi.spyOn(authProvider, 'authorizeRecoveryRequest'); + const session = await controller.authenticateIdentifier(PASSKEY); + await controller.getRecoverySecret(session); + expect(authorize).toHaveBeenCalledExactlyOnceWith({ + requestHash: session.token.requestHash, + audiences: ['stub-audience', 'stub-audience'], + requireTwoFactor: true, + }); + }); + }); + it('reads the highest version while a mutation is writing', async () => { await withController(async ({ controller, escrowA }) => { await controller.register(SECRET, IDENTIFIERS); @@ -726,10 +765,10 @@ describe('MfaRecoveryController', () => { await controller.register(SECRET, IDENTIFIERS); await controller.updateIdentifiers( PASSKEY, - [OIDC, PASSKEY], + [SOCIAL, PASSKEY], REGISTERED_EPOCH, ); - const recovered = await recover(controller, OIDC); + const recovered = await recover(controller, SOCIAL); expect(bytesToHex(recovered.recoverySecret)).toBe(bytesToHex(SECRET)); }); }); @@ -744,6 +783,49 @@ describe('MfaRecoveryController', () => { }); }); + describe('Auth identifiers', () => { + it('sends the caller identifiers with all Auth-approved identifiers', async () => { + await withController(async ({ controller, authProvider, escrowA }) => { + authProvider.identifiers = [SOCIAL, OTHER_PASSKEY, PASSKEY]; + const authorize = vi.spyOn(authProvider, 'authorizeRecoveryRequest'); + const apply = vi.spyOn(escrowA, 'applyMutation'); + await controller.register(SECRET, IDENTIFIERS); + expect(authorize).toHaveBeenCalledExactlyOnceWith({ + requestHash: apply.mock.calls[0][0].requestHash, + audiences: ['stub-audience', 'stub-audience'], + requireTwoFactor: true, + }); + expect(apply.mock.calls[0][3]).toMatchObject({ + identifiers: IDENTIFIERS, + }); + expect(apply.mock.calls[0][4]).toStrictEqual(authProvider.identifiers); + }); + }); + + it.each([ + ['an identifier Auth did not approve', [SOCIAL, OTHER_PASSKEY]], + [ + 'a stale passkey verifier', + [{ ...PASSKEY, verifier: { publicKey: 'rotated' } }, SOCIAL], + ], + ])( + 'rejects %s before writing, leaving the mutation abortable', + async (_name, approved) => { + await withController(async ({ controller, authProvider, escrowA }) => { + authProvider.identifiers = approved; + const apply = vi.spyOn(escrowA, 'applyMutation'); + await expect( + controller.register(SECRET, IDENTIFIERS), + ).rejects.toMatchObject({ code: 'invalid_pending_operation' }); + expect(apply).not.toHaveBeenCalled(); + expect(await controller.getPhase()).toBe('authorizing'); + await controller.abort(); + expect(await controller.getPhase()).toBe('idle'); + }); + }, + ); + }); + describe('resume and abort', () => { it('resumes a persisted writing mutation after a replica failure', async () => { await withController(async ({ controller, escrowA }) => { @@ -1029,6 +1111,7 @@ describe('MfaRecoveryController', () => { requestHash: mutation.requestHash, aal: 2, }), + approvedIdentifiers: IDENTIFIERS, payload, identifier: PASSKEY, receipts, @@ -1094,6 +1177,7 @@ describe('MfaRecoveryController', () => { exp: unixNow() + 60, requestHash: '0x', }), + approvedIdentifiers: IDENTIFIERS, payload: { recoverySecret: bytesToHex(SECRET), identifiers: IDENTIFIERS, @@ -1145,10 +1229,10 @@ describe('MfaRecoveryController', () => { sub: 'profile-1', exp: unixNow() + 60, requestHash: mutation.requestHash, - identifiersHash: await hash( - canonicalizeIdentifiers(payload.identifiers), - ), + aal: 2, + identifiersHash: await hash(payload.identifiers), }), + approvedIdentifiers: IDENTIFIERS, payload, identifier: null, receipts: [ @@ -1207,10 +1291,10 @@ describe('MfaRecoveryController', () => { sub: 'profile-1', exp: unixNow() + 60, requestHash: mutation.requestHash, - identifiersHash: await hash( - canonicalizeIdentifiers(payload.identifiers), - ), + aal: 2, + identifiersHash: await hash(payload.identifiers), }), + approvedIdentifiers: IDENTIFIERS, payload, identifier: null, receipts: [ @@ -1318,7 +1402,12 @@ describe('MfaRecoveryController', () => { messenger: getMessenger(getRootMessenger()), state: { pendingOperation: JSON.stringify(pending) }, }); + options.authProvider.identifiers = [...IDENTIFIERS, OTHER_PASSKEY]; + const apply = vi.spyOn(escrowA, 'applyMutation'); await resumed.resume(); + expect(apply.mock.calls[0][4]).toStrictEqual( + options.authProvider.identifiers, + ); expect(await resumed.getPhase()).toBe('idle'); }); }); @@ -1527,6 +1616,12 @@ describe('MfaRecoveryController', () => { }); }, }, + { + name: 'malformed approved identifiers', + mutate: async (pending: Record): Promise => { + pending.approvedIdentifiers = [{}]; + }, + }, { name: 'an invalid receipt array', mutate: async (pending: Record): Promise => { @@ -1624,11 +1719,12 @@ type RootMessenger = Messenger< type WithControllerCallback = (payload: { controller: MfaRecoveryController; + authProvider: StubAuthProvider; rootMessenger: RootMessenger; controllerMessenger: MfaRecoveryControllerMessenger; escrowA: StubEscrowProvider; escrowB: StubEscrowProvider; - options: MfaRecoveryControllerOptions; + options: ReturnType; }) => Promise | ReturnValue; function getRootMessenger(): RootMessenger { @@ -1647,10 +1743,10 @@ function getMessenger( function getOptions( escrowA: StubEscrowProvider, escrowB: StubEscrowProvider, -): MfaRecoveryControllerOptions { +): MfaRecoveryControllerOptions & { authProvider: StubAuthProvider } { return { messenger: getMessenger(getRootMessenger()), - authProvider: new StubAuthProvider(), + authProvider: new StubAuthProvider(IDENTIFIERS), identifierAuthProvider: new StubIdentifierAuthProvider(), escrows: [escrowA, escrowB], pendingOperationEncryptor: passthroughEncryptor, @@ -1695,6 +1791,7 @@ async function getValidWritingPending( requestHash: await hash(fields), aal: 2, }), + approvedIdentifiers: IDENTIFIERS, payload, identifier: PASSKEY, receipts: [], @@ -1726,6 +1823,7 @@ async function withController( const controller = new MfaRecoveryController(options); return await testFunction({ controller, + authProvider: options.authProvider, rootMessenger, controllerMessenger, escrowA, diff --git a/packages/mfa-recovery-controller/src/MfaRecoveryController.ts b/packages/mfa-recovery-controller/src/MfaRecoveryController.ts index 407507b8bee..7701c5e2903 100644 --- a/packages/mfa-recovery-controller/src/MfaRecoveryController.ts +++ b/packages/mfa-recovery-controller/src/MfaRecoveryController.ts @@ -44,13 +44,11 @@ import { } from './pending-operation-validation.js'; import { assertAbortAllowed, getRecoveryPhase } from './state-machine.js'; import type { - AuthControllerToken, Identifier, IdentifierSession, Mutation, MutationPayload, MutationReceipt, - PendingMutationPayload, PendingOperation, PendingOperationEncryptor, PendingRegisterPayload, @@ -385,6 +383,11 @@ export class MfaRecoveryController extends BaseController< 'no_available_escrow', ); } + const { approval } = await this.#authProvider.authorizeRecoveryRequest({ + requestHash, + audiences: available.map((escrow) => escrow.authAudience), + requireTwoFactor: true, + }); const authorizedEscrows = await authorizeEscrowsWithToken({ escrows: available, credential: session, @@ -392,6 +395,7 @@ export class MfaRecoveryController extends BaseController< const results = await Promise.allSettled( authorizedEscrows.map(async ({ escrow, authorization }) => { const response = await escrow.getSecret( + approval, authorization, requestId, pkE, @@ -492,14 +496,10 @@ export class MfaRecoveryController extends BaseController< }; await this.#persistPending(pending); - const authControllerToken = await this.#authorizeMutation( - mutation, - payload, - ); await this.#replicateMutation({ ...pending, phase: 'writing', - authControllerToken, + ...(await this.#authorizeMutation(mutation)), receipts: [], }); }); @@ -507,16 +507,16 @@ export class MfaRecoveryController extends BaseController< async #authorizeMutation( mutation: Mutation, - payload: PendingMutationPayload, - ): Promise { - return await this.#authProvider.authorizeRecoveryRequest({ - requestHash: mutation.requestHash, - audiences: this.#escrows.map((escrow) => escrow.authAudience), - ...(mutation.operation === 'register' ? {} : { requireTwoFactor: true }), - ...(pendingPayloadHasIdentifiers(payload) - ? { identifiers: payload.identifiers } - : {}), - }); + ): Promise< + Pick + > { + const { approval, identifiers } = + await this.#authProvider.authorizeRecoveryRequest({ + requestHash: mutation.requestHash, + audiences: this.#escrows.map((escrow) => escrow.authAudience), + requireTwoFactor: true, + }); + return { authControllerToken: approval, approvedIdentifiers: identifiers }; } async #repairPendingMutation(): Promise { @@ -530,10 +530,7 @@ export class MfaRecoveryController extends BaseController< await this.#replicateMutation({ ...pending, phase: 'writing', - authControllerToken: await this.#authorizeMutation( - pending.mutation, - pending.payload, - ), + ...(await this.#authorizeMutation(pending.mutation)), receipts: [], }); return; @@ -544,7 +541,7 @@ export class MfaRecoveryController extends BaseController< } async #replicateMutation(pending: WritingPendingOperation): Promise { - const { mutation, payload, identifier } = pending; + const { mutation, identifier } = pending; const remainingEscrows = await this.#getRemainingEscrows(pending); if (remainingEscrows.length === 0) { await this.#clearPending(); @@ -554,11 +551,11 @@ export class MfaRecoveryController extends BaseController< await this.#assertEscrowsAvailable(remainingEscrows); const claims = decodeAuthControllerToken(pending.authControllerToken); - const authControllerToken = + const writing = claims && claims.exp > this.#now() - ? pending.authControllerToken - : await this.#authorizeMutation(mutation, payload); - const writing = { ...pending, authControllerToken }; + ? pending + : { ...pending, ...(await this.#authorizeMutation(mutation)) }; + await this.#assertPending(writing); const authorizedEscrows = mutation.operation === 'register' ? null @@ -590,6 +587,7 @@ export class MfaRecoveryController extends BaseController< writing.authControllerToken, authorizedEscrows?.[index]?.authorization ?? null, payloads[index], + writing.approvedIdentifiers, ), ), ); diff --git a/packages/mfa-recovery-controller/src/auth-api-recovery-provider.test.ts b/packages/mfa-recovery-controller/src/auth-api-recovery-provider.test.ts new file mode 100644 index 00000000000..f73e630214c --- /dev/null +++ b/packages/mfa-recovery-controller/src/auth-api-recovery-provider.test.ts @@ -0,0 +1,129 @@ +import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger'; +import type { + MessengerActions, + MessengerEvents, + MockAnyNamespace, +} from '@metamask/messenger'; +import { describe, expect, it, vi } from 'vitest'; + +import type { AuthApiRecoveryAuthProviderMessenger } from './auth-api-recovery-provider.js'; +import { AuthApiRecoveryAuthProvider } from './auth-api-recovery-provider.js'; + +type RootMessenger = Messenger< + MockAnyNamespace, + MessengerActions, + MessengerEvents +>; + +type Approval = { + approval: string; + expiresIn: number; + identifiers?: { type: string; namespace: string; value: string }[]; +}; + +const APPROVAL = { + approval: 'signed-approval', + expiresIn: 300, + identifiers: [ + { + type: 'social', + namespace: 'https://accounts.google.com', + value: 'hash', + }, + { + type: 'passkey', + namespace: 'metamask.io', + value: 'AQID', + verifier: { + publicKey: { cose: 'AQID' }, + rpId: 'metamask.io', + origins: ['https://metamask.io'], + }, + }, + ], +}; + +/** + * Builds a provider whose messenger is backed by mock AuthenticationController + * actions. + * + * @param approval - The approval `requestMfaApproval` resolves with. + * @returns The provider and the `requestMfaApproval` mock. + */ +function setup(approval: Approval = APPROVAL): { + provider: AuthApiRecoveryAuthProvider; + requestMfaApproval: ReturnType; +} { + const rootMessenger: RootMessenger = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); + const messenger: AuthApiRecoveryAuthProviderMessenger = new Messenger({ + namespace: 'AuthApiRecoveryAuthProvider', + parent: rootMessenger, + }); + rootMessenger.delegate({ + actions: [ + 'AuthenticationController:refreshCanonicalProfileId', + 'AuthenticationController:requestMfaApproval', + ], + events: [], + messenger, + }); + rootMessenger.registerActionHandler( + 'AuthenticationController:refreshCanonicalProfileId', + async () => 'profile-1', + ); + const requestMfaApproval = vi.fn().mockResolvedValue(approval); + rootMessenger.registerActionHandler( + 'AuthenticationController:requestMfaApproval', + requestMfaApproval, + ); + return { + provider: new AuthApiRecoveryAuthProvider({ messenger }), + requestMfaApproval, + }; +} + +describe('AuthApiRecoveryAuthProvider', () => { + it('returns a freshly resolved canonical profile id', async () => { + expect(await setup().provider.getAuthenticatedProfileId()).toBe( + 'profile-1', + ); + }); + + it.each([ + [undefined, false], + [true, true], + ])( + 'returns the approval and identifiers (requireTwoFactor: %s)', + async (requireTwoFactor, requireVerification) => { + const { provider, requestMfaApproval } = setup(); + expect( + await provider.authorizeRecoveryRequest({ + requestHash: '0xrequest', + audiences: ['cubist-audience'], + requireTwoFactor, + }), + ).toStrictEqual({ + approval: APPROVAL.approval, + identifiers: APPROVAL.identifiers, + }); + expect(requestMfaApproval).toHaveBeenCalledWith({ + requestHash: '0xrequest', + audiences: ['cubist-audience'], + requireVerification, + reason: { operation: 'mfa_recovery' }, + }); + }, + ); + + it('returns no identifiers when Auth omits them', async () => { + const { provider } = setup({ approval: 'signed-approval', expiresIn: 300 }); + expect( + await provider.authorizeRecoveryRequest({ + requestHash: '0xrequest', + audiences: ['cubist-audience'], + }), + ).toStrictEqual({ approval: 'signed-approval', identifiers: [] }); + }); +}); diff --git a/packages/mfa-recovery-controller/src/auth-api-recovery-provider.ts b/packages/mfa-recovery-controller/src/auth-api-recovery-provider.ts new file mode 100644 index 00000000000..93b3a1f0310 --- /dev/null +++ b/packages/mfa-recovery-controller/src/auth-api-recovery-provider.ts @@ -0,0 +1,65 @@ +import type { Messenger } from '@metamask/messenger'; +import type { + AuthenticationControllerRefreshCanonicalProfileIdAction, + AuthenticationControllerRequestMfaApprovalAction, +} from '@metamask/profile-sync-controller/auth'; + +import type { RecoveryApproval, RecoveryAuthProvider } from './types.js'; + +export type AuthApiRecoveryAuthProviderMessenger = Messenger< + 'AuthApiRecoveryAuthProvider', + | AuthenticationControllerRefreshCanonicalProfileIdAction + | AuthenticationControllerRequestMfaApprovalAction +>; + +export type AuthApiRecoveryAuthProviderOptions = { + messenger: AuthApiRecoveryAuthProviderMessenger; +}; + +/** + * Requests Auth API approvals through `AuthenticationController`. A missing or + * stale MFA verification rejects with its `StepUpRequiredError`. + */ +export class AuthApiRecoveryAuthProvider implements RecoveryAuthProvider { + readonly #messenger: AuthApiRecoveryAuthProviderMessenger; + + /** + * @param options - Provider options. + * @param options.messenger - Messenger with the AuthenticationController + * actions. + */ + constructor({ messenger }: AuthApiRecoveryAuthProviderOptions) { + this.#messenger = messenger; + } + + /** + * Resolves the canonical profile id from the server, since the cached one + * can be stale after pairing on another device. + * + * @returns The canonical profile id, which approvals use as `sub`. + */ + async getAuthenticatedProfileId(): Promise { + return await this.#messenger.call( + 'AuthenticationController:refreshCanonicalProfileId', + ); + } + + /** + * @param params - Request hash, audiences, and MFA requirement. + * @returns The approval and current recovery identifiers. + */ + async authorizeRecoveryRequest( + params: Parameters[0], + ): Promise { + const { approval, identifiers = [] } = await this.#messenger.call( + 'AuthenticationController:requestMfaApproval', + { + requestHash: params.requestHash, + audiences: params.audiences, + requireVerification: params.requireTwoFactor === true, + reason: { operation: 'mfa_recovery' }, + }, + ); + return { approval, identifiers }; + } +} diff --git a/packages/mfa-recovery-controller/src/auth-controller-token.test.ts b/packages/mfa-recovery-controller/src/auth-controller-token.test.ts index 61def9373ae..eb8f35f4e9b 100644 --- a/packages/mfa-recovery-controller/src/auth-controller-token.test.ts +++ b/packages/mfa-recovery-controller/src/auth-controller-token.test.ts @@ -28,7 +28,7 @@ const VALID_CLAIMS = { }; describe('decodeAuthControllerToken', () => { - it('decodes all claims when optional step-up bindings are present', () => { + it('decodes approval claims', () => { expect( decodeAuthControllerToken(tokenWithPayload(JSON.stringify(VALID_CLAIMS))), ).toStrictEqual({ @@ -42,14 +42,14 @@ describe('decodeAuthControllerToken', () => { }); }); - it('decodes a token without optional aal or identifiers hash', () => { + it('decodes an approval without an identifiers hash', () => { expect( decodeAuthControllerToken( tokenWithPayload( JSON.stringify({ sub: 'profile-1', exp: 1, - ext: { request_hash: '0xrequest' }, + ext: { request_hash: '0xrequest', aal: 1 }, }), ), ), @@ -58,7 +58,7 @@ describe('decodeAuthControllerToken', () => { exp: 1, ext: { requestHash: '0xrequest', - aal: undefined, + aal: 1, identifiersHash: undefined, }, }); @@ -105,32 +105,23 @@ describe('decodeAuthControllerToken', () => { }); it.each([ - ['sub is missing', { exp: 1, ext: { request_hash: '0x' } }], - ['sub is not a string', { sub: 1, exp: 1, ext: { request_hash: '0x' } }], - [ - 'exp is not a number', - { sub: 'profile-1', exp: '1', ext: { request_hash: '0x' } }, - ], - ['request_hash is missing', { sub: 'profile-1', exp: 1, ext: {} }], - [ - 'aal is not a number', - { - sub: 'profile-1', - exp: 1, - ext: { request_hash: '0x', aal: '2' }, - }, - ], + ['sub is missing', { sub: undefined }], + ['sub is not a string', { sub: 1 }], + ['exp is not a number', { exp: '1' }], + ['request_hash is missing', { ext: { aal: 1 } }], + ['aal is missing', { ext: { request_hash: '0x' } }], + ['aal is not a number', { ext: { request_hash: '0x', aal: '2' } }], [ 'identifiers_hash is not a string', { - sub: 'profile-1', - exp: 1, - ext: { request_hash: '0x', identifiers_hash: 1 }, + ext: { request_hash: '0x', aal: 1, identifiers_hash: 1 }, }, ], ])('returns undefined when %s', (_name, claims) => { expect( - decodeAuthControllerToken(tokenWithPayload(JSON.stringify(claims))), + decodeAuthControllerToken( + tokenWithPayload(JSON.stringify({ ...VALID_CLAIMS, ...claims })), + ), ).toBeUndefined(); }); }); diff --git a/packages/mfa-recovery-controller/src/auth-controller-token.ts b/packages/mfa-recovery-controller/src/auth-controller-token.ts index 64adc1d5a90..206380d2d34 100644 --- a/packages/mfa-recovery-controller/src/auth-controller-token.ts +++ b/packages/mfa-recovery-controller/src/auth-controller-token.ts @@ -3,9 +3,7 @@ import { bytesToString, isObject } from '@metamask/utils'; import { fromBase64Url } from './crypto.js'; /** - * The claims the controller reads from an AuthController (Hydra) access token, - * in the token's shape with camelCase names. The Auth API token hook adds the - * step-up bindings under `ext`; a token that did not step up has none of them. + * Claims read from an Auth API approval, with camelCase field names. */ export type AuthControllerTokenClaims = { /** @@ -21,9 +19,9 @@ export type AuthControllerTokenClaims = { /** * Authentication assurance level. */ - aal?: number; + aal: number; /** - * Only present when the step-up was bound to a payload's identifiers. + * Hash of the Auth API's recovery identifiers. */ identifiersHash?: string; }; @@ -64,7 +62,7 @@ export function decodeAuthControllerToken( typeof sub !== 'string' || typeof exp !== 'number' || typeof requestHash !== 'string' || - (aal !== undefined && typeof aal !== 'number') || + typeof aal !== 'number' || (identifiersHash !== undefined && typeof identifiersHash !== 'string') ) { return undefined; diff --git a/packages/mfa-recovery-controller/src/crypto.test.ts b/packages/mfa-recovery-controller/src/crypto.test.ts index 987d8244506..df836703485 100644 --- a/packages/mfa-recovery-controller/src/crypto.test.ts +++ b/packages/mfa-recovery-controller/src/crypto.test.ts @@ -3,8 +3,6 @@ import { describe, expect, it } from 'vitest'; import { canonicalize, - canonicalizeIdentifiers, - compareUtf8, decryptFromPublic, encryptToPublic, generateSigningKey, @@ -88,71 +86,31 @@ describe('crypto', () => { ); }); - it('sorts identifiers for ownership hashes', async () => { - const first: Identifier = { - type: 'passkey', - namespace: 'b.com', - value: '2', - verifier: null, - }; - const second: Identifier = { - type: 'passkey', - namespace: 'a.com', - value: '1', - verifier: null, - }; - expect(canonicalize(canonicalizeIdentifiers([first, second]))).toBe( - canonicalize(canonicalizeIdentifiers([second, first])), + it('matches the Auth API and escrow identifiers hash', async () => { + const identifiers: Identifier[] = [ + { + type: 'social', + namespace: 'https://accounts.google.com', + value: 'abc', + }, + { + type: 'passkey', + namespace: 'metamask.io', + value: 'AQID', + verifier: { + credentialId: 'AQID', + publicKey: { cose: 'AQID' }, + rpId: 'metamask.io', + origins: ['https://metamask.io', 'android:apk-key-hash:example'], + }, + }, + ]; + expect(await hash(identifiers)).toBe( + '0x7fbf077f78e6cc4c420eb9b1885ba911aca3ec9810422ab5f26f767f13622090', + ); + expect(await hash([...identifiers].reverse())).not.toBe( + await hash(identifiers), ); - }); - - describe('identifier sort order', () => { - // The escrow sorts by UTF-8 byte order (Rust `String` ordering). Any other - // order changes `identifiersHash` and the escrow rejects the mutation. - const identifier = (value: string): Identifier => ({ - type: 'passkey', - namespace: 'metamask.io', - value, - verifier: null, - }); - const sortedValues = (values: string[]): string[] => - ( - canonicalizeIdentifiers(values.map(identifier)) as { value: string }[] - ).map((item) => item.value); - - it('puts uppercase before lowercase, unlike locale order', () => { - // base64url credential ids are mixed case; localeCompare would put - // `a2V5LTE` first. - expect(sortedValues(['a2V5LTE', 'Zm9vLTI'])).toStrictEqual([ - 'Zm9vLTI', - 'a2V5LTE', - ]); - expect(sortedValues(['Zm9vLTI', 'a2V5LTE'])).toStrictEqual([ - 'Zm9vLTI', - 'a2V5LTE', - ]); - }); - - it('orders by UTF-8 bytes rather than UTF-16 code units', () => { - // U+FF21 is EF BC A1 in UTF-8 and U+1F600 is F0 9F 98 80, so U+FF21 - // sorts first. In UTF-16 U+1F600 (D83D DE00) would sort first. - expect(sortedValues(['\u{1F600}', '\uFF21'])).toStrictEqual([ - '\uFF21', - '\u{1F600}', - ]); - }); - - it('orders a shorter identifier before its extension', () => { - expect(sortedValues(['abc', 'ab'])).toStrictEqual(['ab', 'abc']); - }); - - it('compares UTF-8 bytes, treating a shared prefix as shorter', () => { - expect(compareUtf8('ab', 'abc')).toBeLessThan(0); - expect(compareUtf8('abc', 'ab')).toBeGreaterThan(0); - expect(compareUtf8('abc', 'abc')).toBe(0); - expect(compareUtf8('a', 'b')).toBeLessThan(0); - expect(compareUtf8('b', 'a')).toBeGreaterThan(0); - }); }); describe('wire-format vectors', () => { diff --git a/packages/mfa-recovery-controller/src/crypto.ts b/packages/mfa-recovery-controller/src/crypto.ts index 8ff2ee60fcf..b42d5d4e90c 100644 --- a/packages/mfa-recovery-controller/src/crypto.ts +++ b/packages/mfa-recovery-controller/src/crypto.ts @@ -22,8 +22,6 @@ import { import type { Hex } from '@metamask/utils'; import stableStringify from 'json-stable-stringify'; -import type { Identifier } from './types.js'; - const WRAP_INFO = new TextEncoder().encode('escrow-wrap-v1'); const WRAP_KEY_LEN = 32; @@ -50,47 +48,6 @@ export async function hash(value: unknown): Promise { return bytesToHex(await sha256(stringToBytes(canonicalize(value)))); } -/** - * Orders two strings by their UTF-8 bytes. The escrow sorts identifiers the same - * way (Rust `String` ordering), so this must not be locale or UTF-16 aware. - * - * @param left - First string. - * @param right - Second string. - * @returns Negative, zero, or positive like `Array.prototype.sort` expects. - */ -export function compareUtf8(left: string, right: string): number { - const leftBytes = stringToBytes(left); - const rightBytes = stringToBytes(right); - const shared = Math.min(leftBytes.length, rightBytes.length); - for (let index = 0; index < shared; index += 1) { - const difference = leftBytes[index] - rightBytes[index]; - if (difference !== 0) { - return difference; - } - } - return leftBytes.length - rightBytes.length; -} - -/** - * Canonical identifier list bound into AuthController `identifiersHash`. - * - * @param identifiers - Identifier set. - * @returns Identifier records including verifier material, sorted by the UTF-8 - * bytes of their canonical JSON. - */ -export function canonicalizeIdentifiers(identifiers: Identifier[]): unknown { - return [...identifiers] - .map((identifier) => ({ - namespace: identifier.namespace, - type: identifier.type, - value: identifier.value, - verifier: identifier.verifier, - })) - .sort((left, right) => - compareUtf8(canonicalize(left), canonicalize(right)), - ); -} - /** * @returns A random 0x-prefixed id. */ diff --git a/packages/mfa-recovery-controller/src/escrow-providers/cubist-escrow-provider.test.ts b/packages/mfa-recovery-controller/src/escrow-providers/cubist-escrow-provider.test.ts index ce5c083fe31..6f7fc9352d2 100644 --- a/packages/mfa-recovery-controller/src/escrow-providers/cubist-escrow-provider.test.ts +++ b/packages/mfa-recovery-controller/src/escrow-providers/cubist-escrow-provider.test.ts @@ -13,6 +13,7 @@ import { import { MfaRecoveryError, MutationRejectedError } from '../errors.js'; import type { AuthControllerToken, + Identifier, IdentifierAuthorization, Mutation, MutationPayload, @@ -55,6 +56,14 @@ const WIRE_PAYLOAD = { }, }; +const APPROVED: Identifier[] = [ + { + type: 'social', + namespace: 'https://accounts.google.com', + value: 'hashed-email', + }, +]; + const AUTHORIZATION: IdentifierAuthorization = { kind: 'key-bound', token: { @@ -85,6 +94,7 @@ describe('CubistEscrowProvider', () => { it('uses the cubist escrow id and the pinned wrap key', () => { const { provider } = createProvider(); expect(provider.id).toBe('cubist'); + expect(provider.authAudience).toBe('gamma.signer.cubist.dev'); expect(provider.wrapPublicKey).toStrictEqual(wrapKey.publicKey); }); @@ -167,7 +177,7 @@ describe('CubistEscrowProvider', () => { const receipt = await signedReceipt(); const { provider, invoke } = createProvider({ stdout: receipt }); expect( - await provider.applyMutation(MUTATION, TOKEN, null, PAYLOAD), + await provider.applyMutation(MUTATION, TOKEN, null, PAYLOAD, APPROVED), ).toStrictEqual(receipt); expect(invoke).toHaveBeenCalledWith('cubist_secret_escrow', 'latest', { request: { @@ -176,6 +186,7 @@ describe('CubistEscrowProvider', () => { authControllerToken: TOKEN, identifierAuthorization: null, payload: WIRE_PAYLOAD, + approvedIdentifiers: APPROVED, }, }); }); @@ -184,7 +195,13 @@ describe('CubistEscrowProvider', () => { const { provider, invoke } = createProvider({ stdout: await signedReceipt(), }); - await provider.applyMutation(MUTATION, TOKEN, AUTHORIZATION, PAYLOAD); + await provider.applyMutation( + MUTATION, + TOKEN, + AUTHORIZATION, + PAYLOAD, + APPROVED, + ); const [, , { request }] = invoke.mock.calls[0] as [ string, string, @@ -201,7 +218,13 @@ describe('CubistEscrowProvider', () => { it('rejects a response that is not a receipt', async () => { const { provider } = createProvider({ stdout: { nope: true } }); await expect( - provider.applyMutation(MUTATION, TOKEN, AUTHORIZATION, PAYLOAD), + provider.applyMutation( + MUTATION, + TOKEN, + AUTHORIZATION, + PAYLOAD, + APPROVED, + ), ).rejects.toMatchObject({ code: 'escrow_response_invalid' }); }); @@ -215,7 +238,13 @@ describe('CubistEscrowProvider', () => { const receipt = { ...(await signedReceipt()), ...patch }; const { provider } = createProvider({ stdout: receipt }); await expect( - provider.applyMutation(MUTATION, TOKEN, AUTHORIZATION, PAYLOAD), + provider.applyMutation( + MUTATION, + TOKEN, + AUTHORIZATION, + PAYLOAD, + APPROVED, + ), ).rejects.toMatchObject({ code: 'escrow_response_invalid' }); }); @@ -224,7 +253,7 @@ describe('CubistEscrowProvider', () => { policy: { response: 'Deny', reason: 'version mismatch' }, }); await expect( - provider.applyMutation(MUTATION, TOKEN, null, PAYLOAD), + provider.applyMutation(MUTATION, TOKEN, null, PAYLOAD, APPROVED), ).rejects.toBeInstanceOf(MutationRejectedError); }); }); @@ -240,7 +269,7 @@ describe('CubistEscrowProvider', () => { it('invokes getSecret and returns the wrapped secret as bytes', async () => { const { provider, invoke } = createProvider({ stdout: secretResponse }); expect( - await provider.getSecret(AUTHORIZATION, 'req-1', PK_E), + await provider.getSecret(TOKEN, AUTHORIZATION, 'req-1', PK_E), ).toStrictEqual({ ...secretResponse, recoverySecret: { ciphertext: new Uint8Array([0xcc]) }, @@ -248,6 +277,7 @@ describe('CubistEscrowProvider', () => { expect(invoke).toHaveBeenCalledWith('cubist_secret_escrow', 'latest', { request: { cmd: 'getSecret', + authControllerToken: TOKEN, identifierAuthorization: { ...AUTHORIZATION, token: { @@ -282,7 +312,7 @@ describe('CubistEscrowProvider', () => { ])('rejects a secret response that is %s', async (_label, body) => { const { provider } = createProvider({ stdout: body }); await expect( - provider.getSecret(AUTHORIZATION, 'req-1', PK_E), + provider.getSecret(TOKEN, AUTHORIZATION, 'req-1', PK_E), ).rejects.toMatchObject({ code: 'escrow_response_invalid' }); }); }); @@ -455,6 +485,7 @@ function createProvider( return { provider: new CubistEscrowProvider({ client, + authAudience: 'gamma.signer.cubist.dev', wrapPublicKey: wrapKey.publicKey, receiptPublicKey: receiptKey.publicKey, functionId: options.functionId, diff --git a/packages/mfa-recovery-controller/src/escrow-providers/cubist-escrow-provider.ts b/packages/mfa-recovery-controller/src/escrow-providers/cubist-escrow-provider.ts index f07993ebe51..97b34ef9319 100644 --- a/packages/mfa-recovery-controller/src/escrow-providers/cubist-escrow-provider.ts +++ b/packages/mfa-recovery-controller/src/escrow-providers/cubist-escrow-provider.ts @@ -21,6 +21,7 @@ import { isMutationReceipt } from '../escrow-utils.js'; import type { AuthControllerToken, GetRecoverySecretResponse, + Identifier, IdentifierAuthorization, Mutation, MutationPayload, @@ -30,7 +31,6 @@ import type { } from '../types.js'; const ESCROW_ID = 'cubist'; -const AUTH_AUDIENCE = 'cubist'; const DEFAULT_FUNCTION_ID = 'cubist_secret_escrow'; const DEFAULT_VERSION: Version = 'latest'; @@ -47,6 +47,10 @@ export type CubistEscrowProviderOptions = { * Authenticated CubeSigner client used to invoke the escrow C2F. */ client: CubeSignerClient; + /** + * Must match the escrow's AUTH_CONTROLLER_AUDIENCE configuration. + */ + authAudience: string; /** * C2F name or named-policy id. Defaults to `cubist_secret_escrow`. */ @@ -68,7 +72,7 @@ export type CubistEscrowProviderOptions = { export class CubistEscrowProvider implements RecoveryEscrowProvider { readonly id = ESCROW_ID; - readonly authAudience = AUTH_AUDIENCE; + readonly authAudience: string; readonly wrapPublicKey: Uint8Array; @@ -84,6 +88,7 @@ export class CubistEscrowProvider implements RecoveryEscrowProvider { * @param options - CubeSigner client, pinned keys, and optional C2F id/version. */ constructor(options: CubistEscrowProviderOptions) { + this.authAudience = options.authAudience; this.wrapPublicKey = options.wrapPublicKey; this.#receiptPublicKey = options.receiptPublicKey; this.#client = options.client; @@ -111,12 +116,14 @@ export class CubistEscrowProvider implements RecoveryEscrowProvider { } /** + * @param authControllerToken - Request-bound approval for this read. * @param authorization - Identifier proof for this read. * @param requestId - Client request id bound into the read hash. * @param pkE - Ephemeral wrap public key for the response. * @returns The wrapped recovery secret and its version. */ async getSecret( + authControllerToken: AuthControllerToken, authorization: IdentifierAuthorization, requestId: string, pkE: Uint8Array, @@ -127,6 +134,7 @@ export class CubistEscrowProvider implements RecoveryEscrowProvider { identifierAuthorization: authorization, requestId, pkE, + authControllerToken, }, parseGetSecretResponse, ); @@ -137,6 +145,8 @@ export class CubistEscrowProvider implements RecoveryEscrowProvider { * @param authControllerToken - Request-bound AuthController token. * @param identifierAuthorization - Identifier proof. `null` for register. * @param payload - Per-escrow mutation payload. + * @param approvedIdentifiers - The approval's identifiers, which payload + * identifiers must be drawn from. * @returns The escrow's mutation receipt. */ async applyMutation( @@ -144,6 +154,7 @@ export class CubistEscrowProvider implements RecoveryEscrowProvider { authControllerToken: AuthControllerToken, identifierAuthorization: IdentifierAuthorization | null, payload: MutationPayload, + approvedIdentifiers: Identifier[], ): Promise { return await this.#invoke( 'applyMutation', @@ -152,6 +163,7 @@ export class CubistEscrowProvider implements RecoveryEscrowProvider { authControllerToken, identifierAuthorization, payload, + approvedIdentifiers, }, parseReceipt, true, // a policy Deny did not apply the mutation diff --git a/packages/mfa-recovery-controller/src/identifier-auth.ts b/packages/mfa-recovery-controller/src/identifier-auth.ts index 6e8b936d260..7d367cbdc70 100644 --- a/packages/mfa-recovery-controller/src/identifier-auth.ts +++ b/packages/mfa-recovery-controller/src/identifier-auth.ts @@ -37,7 +37,7 @@ export function countDistinctIdentifiers(identifiers: Identifier[]): number { * `escrow-challenge` once escrow OTP commands exist. */ export const IDENTIFIER_AUTH_MODES: Record = { - oidc: 'key-bound', + social: 'key-bound', passkey: 'key-bound', siwe: 'key-bound', }; diff --git a/packages/mfa-recovery-controller/src/index.ts b/packages/mfa-recovery-controller/src/index.ts index 118126dbfb1..daea6d7bac1 100644 --- a/packages/mfa-recovery-controller/src/index.ts +++ b/packages/mfa-recovery-controller/src/index.ts @@ -23,6 +23,11 @@ export type { } from './MfaRecoveryController-method-action-types.js'; export { CubistEscrowProvider } from './escrow-providers/cubist-escrow-provider.js'; export type { CubistEscrowProviderOptions } from './escrow-providers/cubist-escrow-provider.js'; +export { AuthApiRecoveryAuthProvider } from './auth-api-recovery-provider.js'; +export type { + AuthApiRecoveryAuthProviderMessenger, + AuthApiRecoveryAuthProviderOptions, +} from './auth-api-recovery-provider.js'; export { IncompleteMutationError, MfaRecoveryError, @@ -49,6 +54,7 @@ export type { PendingRegisterPayload, PendingUpdateIdentifiersPayload, PendingUpdateRecoverySecretPayload, + RecoveryApproval, RecoveryAuthProvider, RecoveryEscrowProvider, RecoveryIdentifierAuthProvider, diff --git a/packages/mfa-recovery-controller/src/pending-operation-validation.ts b/packages/mfa-recovery-controller/src/pending-operation-validation.ts index 8755c79c22f..f6cd76225cd 100644 --- a/packages/mfa-recovery-controller/src/pending-operation-validation.ts +++ b/packages/mfa-recovery-controller/src/pending-operation-validation.ts @@ -111,19 +111,39 @@ export async function assertValidPendingOperation( return; } const token = decodeAuthControllerToken(pending.authControllerToken); + const { approvedIdentifiers } = pending; if ( token?.sub !== mutation.profileId || token.ext.requestHash !== mutation.requestHash || - (mutation.operation !== 'register' && (token.ext.aal ?? 0) < 2) || - ((mutation.operation === 'register' || - mutation.operation === 'updateIdentifiers') && - token.ext.identifiersHash === undefined) || + token.ext.aal < 2 || + !Array.isArray(approvedIdentifiers) || + !approvedIdentifiers.every(isIdentifier) || + (pendingPayloadHasIdentifiers(payload) && + (token.ext.identifiersHash !== (await hash(approvedIdentifiers)) || + !(await isApprovedSubset(payload.identifiers, approvedIdentifiers)))) || !isMutationReceiptArray(pending.receipts) ) { throwInvalidPendingOperation(); } } +/** + * Escrows reject payload identifiers that are not exactly (verifier included) + * in the approval's list, and the rejection leaves the write unabortable. + * + * @param identifiers - Payload identifiers. + * @param approvedIdentifiers - The approval's identifiers. + * @returns Whether every payload identifier is in the approval. + */ +async function isApprovedSubset( + identifiers: Identifier[], + approvedIdentifiers: Identifier[], +): Promise { + const approved = new Set(await Promise.all(approvedIdentifiers.map(hash))); + const hashes = await Promise.all(identifiers.map(hash)); + return hashes.every((identifierHash) => approved.has(identifierHash)); +} + function isRecord(value: unknown): value is Record { return value !== null && typeof value === 'object' && !Array.isArray(value); } @@ -195,8 +215,7 @@ function isIdentifier(value: unknown): value is Identifier { isRecord(value) && typeof value.type === 'string' && typeof value.namespace === 'string' && - typeof value.value === 'string' && - Object.prototype.hasOwnProperty.call(value, 'verifier') + typeof value.value === 'string' ); } diff --git a/packages/mfa-recovery-controller/src/types.ts b/packages/mfa-recovery-controller/src/types.ts index fd3cb49212f..f1acc6e027b 100644 --- a/packages/mfa-recovery-controller/src/types.ts +++ b/packages/mfa-recovery-controller/src/types.ts @@ -6,7 +6,7 @@ export type Identifier = { type: string; namespace: string; value: string; - verifier: unknown; + verifier?: unknown; }; export type PoPChallenge = { @@ -63,11 +63,16 @@ export type Mutation = { }; /** - * Hydra access token (compact JWT). Its `ext` claims bind it to one mutation: + * Auth API approval (compact JWT). Its `ext` claims bind it to one mutation: * `aal`, `request_hash`, and `identifiers_hash`. */ export type AuthControllerToken = string; +export type RecoveryApproval = { + approval: AuthControllerToken; + identifiers: Identifier[]; +}; + export type MutationReceipt = { mutationId: string; requestHash: string; @@ -206,6 +211,11 @@ export type AuthorizingPendingOperation = PendingOperationBase & { export type WritingPendingOperation = PendingOperationBase & { phase: 'writing'; authControllerToken: AuthControllerToken; + /** + * The approval's identifiers, in Auth API order, bound by its + * `identifiers_hash`. + */ + approvedIdentifiers: Identifier[]; receipts: MutationReceipt[]; }; @@ -246,12 +256,15 @@ export type RecoveredSecret = { */ export type RecoveryAuthProvider = { getAuthenticatedProfileId: () => Promise; + /** + * Returns the Auth API's approval and current recovery identifiers. + * `requireTwoFactor` requests recent AAL2 authentication. + */ authorizeRecoveryRequest: (params: { requestHash: string; audiences: string[]; requireTwoFactor?: boolean; - identifiers?: Identifier[]; - }) => Promise; + }) => Promise; }; /** @@ -287,6 +300,7 @@ export type RecoveryEscrowProvider = { isAvailable: () => Promise; generateChallenge: () => Promise; getSecret: ( + authControllerToken: AuthControllerToken, authorization: IdentifierAuthorization, requestId: string, pkE: Uint8Array, @@ -296,6 +310,7 @@ export type RecoveryEscrowProvider = { authControllerToken: AuthControllerToken, identifierAuthorization: IdentifierAuthorization | null, payload: MutationPayload, + approvedIdentifiers: Identifier[], ) => Promise; /** * Verifies a receipt cryptographically using the build-pinned receipt key diff --git a/packages/mfa-recovery-controller/tests/stubs.ts b/packages/mfa-recovery-controller/tests/stubs.ts index 119e5ac6741..b398a40e6e3 100644 --- a/packages/mfa-recovery-controller/tests/stubs.ts +++ b/packages/mfa-recovery-controller/tests/stubs.ts @@ -8,7 +8,7 @@ import { import { decodeAuthControllerToken } from '../src/auth-controller-token.js'; import { - canonicalizeIdentifiers, + canonicalize, decryptFromPublic, encryptToPublic, hash, @@ -35,6 +35,7 @@ import type { PendingOperation, PendingOperationEncryptor, PoPChallenge, + RecoveryApproval, RecoveryAuthProvider, RecoveryEscrowProvider, RecoveryIdentifierAuthProvider, @@ -60,7 +61,7 @@ const CHALLENGE_TTL_SECS = 5 * 60; /** * Builds an unsigned compact JWT carrying the claims the controller reads from - * an AuthController (Hydra) access token. + * an Auth API approval. * * @param claims - Token claims. * @param claims.sub - The profile id. @@ -74,7 +75,7 @@ export function createAuthControllerToken({ sub, exp, requestHash, - aal, + aal = 1, identifiersHash, }: { sub: string; @@ -104,6 +105,12 @@ export class StubAuthProvider implements RecoveryAuthProvider { now: () => number = unixNow; + identifiers: Identifier[]; + + constructor(identifiers: Identifier[] = []) { + this.identifiers = identifiers; + } + async getAuthenticatedProfileId(): Promise { return this.profileId; } @@ -111,21 +118,17 @@ export class StubAuthProvider implements RecoveryAuthProvider { async authorizeRecoveryRequest(params: { requestHash: string; requireTwoFactor?: boolean; - identifiers?: Identifier[]; - }): Promise { - return createAuthControllerToken({ - sub: this.profileId, - exp: this.now() + 60 * 60, - requestHash: params.requestHash, - aal: params.requireTwoFactor ? 2 : 1, - ...(params.identifiers === undefined - ? {} - : { - identifiersHash: await hash( - canonicalizeIdentifiers(params.identifiers), - ), - }), - }); + }): Promise { + return { + approval: createAuthControllerToken({ + sub: this.profileId, + exp: this.now() + 5 * 60, + requestHash: params.requestHash, + aal: params.requireTwoFactor ? 2 : 1, + identifiersHash: await hash(this.identifiers), + }), + identifiers: this.identifiers, + }; } } @@ -242,6 +245,7 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { } async getSecret( + authControllerToken: AuthControllerToken, authorization: IdentifierAuthorization, requestId: string, pkE: Uint8Array, @@ -261,6 +265,15 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { authorization, requestHash, ); + const claims = decodeAuthControllerToken(authControllerToken); + if ( + claims?.sub !== profileId || + claims.ext.requestHash !== requestHash || + claims.ext.aal < 2 || + claims.exp <= this.now() + ) { + throw new MfaRecoveryError('Invalid read approval', 'invalid_read_auth'); + } const record = this.#records.get(profileId); if (record === undefined) { throw new MfaRecoveryError('No recovery record', 'not_registered'); @@ -284,6 +297,7 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { authControllerToken: AuthControllerToken, identifierAuthorization: IdentifierAuthorization | null, payload: MutationPayload, + approvedIdentifiers: Identifier[], ): Promise { if (this.failNextApplyCount > 0) { this.failNextApplyCount -= 1; @@ -297,6 +311,7 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { authControllerToken, identifierAuthorization, payload, + approvedIdentifiers, ); } catch (error) { if (!this.#applyCommitted && error instanceof MfaRecoveryError) { @@ -311,6 +326,7 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { authControllerToken: AuthControllerToken, identifierAuthorization: IdentifierAuthorization | null, payload: MutationPayload, + approvedIdentifiers: Identifier[], ): Promise { await this.#verifyMutationAuthorization(mutation, authControllerToken); @@ -354,6 +370,7 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { await this.#assertIdentifierOwnership( authControllerToken, registerPayload.identifiers, + approvedIdentifiers, ); this.#assertIdentifiersAvailable( mutation.profileId, @@ -382,8 +399,7 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { return await this.#signReceipt(mutation); } - const claims = decodeAuthControllerToken(authControllerToken); - if (record === undefined || (claims?.ext.aal ?? 0) < 2) { + if (record === undefined) { throw new MfaRecoveryError( 'Update not authorized', 'update_unauthorized', @@ -415,7 +431,11 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { } else { const { identifiers } = payload as UpdateIdentifiersPayload; await this.#assertPayloadHash(mutation, payload); - await this.#assertIdentifierOwnership(authControllerToken, identifiers); + await this.#assertIdentifierOwnership( + authControllerToken, + identifiers, + approvedIdentifiers, + ); this.#assertIdentifiersAvailable(mutation.profileId, identifiers); this.#applyCommitted = true; this.#replaceIdentifierIndex( @@ -538,6 +558,7 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { requestHash !== mutation.requestHash || claims?.ext.requestHash !== mutation.requestHash || claims.sub !== mutation.profileId || + claims.ext.aal < 2 || claims.exp <= this.now() || !this.#hasExactAudiences(mutation.audiences) ) { @@ -558,10 +579,13 @@ export class StubEscrowProvider implements RecoveryEscrowProvider { async #assertIdentifierOwnership( token: AuthControllerToken, identifiers: Identifier[], + approvedIdentifiers: Identifier[], ): Promise { + const approved = new Set(approvedIdentifiers.map(canonicalize)); if ( decodeAuthControllerToken(token)?.ext.identifiersHash !== - (await hash(canonicalizeIdentifiers(identifiers))) + (await hash(approvedIdentifiers)) || + !identifiers.every((identifier) => approved.has(canonicalize(identifier))) ) { throw new MfaRecoveryError( 'Identifier ownership not approved', diff --git a/packages/mfa-recovery-controller/tsconfig.build.json b/packages/mfa-recovery-controller/tsconfig.build.json index cf795055652..b5bb89ae7be 100644 --- a/packages/mfa-recovery-controller/tsconfig.build.json +++ b/packages/mfa-recovery-controller/tsconfig.build.json @@ -14,6 +14,9 @@ { "path": "../messenger/tsconfig.build.json" }, + { + "path": "../profile-sync-controller/tsconfig.build.json" + }, { "path": "../utils/tsconfig.build.json" } diff --git a/packages/mfa-recovery-controller/tsconfig.json b/packages/mfa-recovery-controller/tsconfig.json index fdeaa07f238..6806c189e55 100644 --- a/packages/mfa-recovery-controller/tsconfig.json +++ b/packages/mfa-recovery-controller/tsconfig.json @@ -10,6 +10,9 @@ { "path": "../messenger" }, + { + "path": "../profile-sync-controller" + }, { "path": "../utils" } diff --git a/packages/mfa-recovery-controller/tsconfig.lint.json b/packages/mfa-recovery-controller/tsconfig.lint.json index 829917a673f..9b68878626e 100644 --- a/packages/mfa-recovery-controller/tsconfig.lint.json +++ b/packages/mfa-recovery-controller/tsconfig.lint.json @@ -16,6 +16,9 @@ }, { "path": "../utils/tsconfig.lint.json" + }, + { + "path": "../profile-sync-controller/tsconfig.lint.json" } ] } diff --git a/yarn.lock b/yarn.lock index 24a5e4a8aa1..8f377f73751 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7298,6 +7298,7 @@ __metadata: "@metamask/base-controller": "npm:^10.1.0" "@metamask/cryptography": "npm:^1.1.2" "@metamask/messenger": "npm:^3.0.0" + "@metamask/profile-sync-controller": "npm:^36.0.0" "@metamask/superstruct": "npm:^3.4.1" "@metamask/utils": "npm:^12.0.1" "@typescript/native": "npm:typescript@^7.0.2"