feat: prove new SIWE identifiers with recovery mutations

packages/mfa-recovery-controller/ARCHITECTURE.md

@@ -118,8 +118,8 @@ Anything that fails is rejected rather than acted on:
   an identifier.
 - `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
+  `identifiers_hash` to match `approvedIdentifiers`, and every non-SIWE
+  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.
@@ -147,9 +147,12 @@ sequenceDiagram
   M->>M: skip escrows with a stored receipt, re-verify stored receipts
   M->>E: isAvailable (remaining escrows)
   M->>M: reuse token, or re-issue it if exp <= now()
-  alt not register
-    M->>I: getKeyBoundIdentifierToken
-    M->>E: generateChallenge + PoP per remaining escrow (all required)
+  opt updateIdentifiers with SIWE identifiers
+    M->>E: getIdentifiers (enrolled set at expectedVersion)
+  end
+  opt update identifier, or SIWE identifiers not yet enrolled
+    M->>I: getKeyBoundIdentifierToken per identifier
+    M->>E: generateChallenge + PoPs per remaining escrow (all required)
   end
   M->>M: wrap secret per escrow, persist writing
   par write every remaining replica
@@ -178,8 +181,12 @@ Auth approvals:
 - 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.
+  `identifiers_hash` and rejects non-SIWE payload identifiers outside them.
+  Hashing sorts object keys, not arrays.
+- Auth does not vouch for SIWE wallets. Each SIWE identifier an escrow has not
+  enrolled needs its own proof in `identifierAuthorizations`; updates add one
+  proof for the `identifier` argument. Escrows past `expectedVersion` replay
+  their receipt, so they get no SIWE proofs.
 - Missing or stale MFA rejects with `AuthenticationController`'s
   `StepUpRequiredError`.
 - Escrows verify signatures. The controller decodes claims only for pending-state
@@ -211,8 +218,8 @@ Versioning:
 
 `resume()` is the same write path without creating a new mutation:
 authorizing pending gets a fresh token then replicates; writing pending
-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
+retries only escrows that do not already have a stored receipt. Updates and
+mutations adding SIWE wallets 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 integration
@@ -301,13 +308,13 @@ the highest one. The next mutation still requires `resume()` first.
 flowchart TD
   Start[identifier + requestHash] --> Mode{trusted type?}
   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]
+  KB --> Chal[each escrow generateChallenge, one per escrow]
+  Chal --> PoP[each proof key signs hash of token + challengeId + requestHash]
   Mode -->|emailOtp / smsOtp| Todo[TODO MFA-605 / MFA-606 / MFA-568]
   Mode -->|anything else| Reject[unknown_identifier_type]
 ```
 
 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`).
+challenge fails are skipped. A `register` without SIWE identifiers sends no
+identifier authorizations (`[]`).

packages/mfa-recovery-controller/CHANGELOG.md

@@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - 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))
   - Adds `getIdentifiers`, which returns the enrolled identifiers at the highest escrow version as `epoch` along with Auth's identifiers, for building `updateIdentifiers` calls ([#11149](https://github.com/MetaMask/core/pull/11149))
   - Passes the operation's AuthController token to `RecoveryEscrowProvider.generateChallenge`, and requires `RecoveryEscrowProvider.getIdentifiers` ([#11149](https://github.com/MetaMask/core/pull/11149))
+  - Proves each SIWE identifier an escrow has not enrolled through `identifierAuthProvider`, including at `register`, instead of requiring it in `approvedIdentifiers`, and passes `RecoveryEscrowProvider.applyMutation` a list of identifier authorizations sharing one challenge ([#11151](https://github.com/MetaMask/core/pull/11151))
+  - Rejects SIWE identifiers whose `namespace` is not their verifier's `domain:chainId` with `invalid_identifier` ([#11151](https://github.com/MetaMask/core/pull/11151))
   - 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.

packages/mfa-recovery-controller/CUBIST_TEST_SEQUENCE.md

@@ -71,22 +71,30 @@ sequenceDiagram
   Auth-->>C: JWT bound to requestHash
   C->>Cubist: isAvailable
 
-  opt updates only (not register)
+  opt updateIdentifiers adding SIWE identifiers
+    C->>Cubist: getIdentifiers(JWT)
+    Cubist-->>C: enrolled identifiers, version
+  end
+  opt updates, or SIWE identifiers not yet enrolled
     Note over C,Cubist: Proof of possession for requestHash (see below)
   end
 
   Note over C: Encrypt the secret to the Cubist wrap key
-  C->>Cubist: applyMutation(mutation, JWT, { token, proof } or null, payload, approvedIdentifiers)
+  C->>Cubist: applyMutation(mutation, JWT, [{ token, proof }, ...], payload, approvedIdentifiers)
   Cubist-->>C: signed receipt
   Note over C: Verify the receipt with the pinned receipt key
   C-->>Mobile: done
 ```
 
-| Operation              | Identifier proof        | Payload              |
-| ---------------------- | ----------------------- | -------------------- |
-| `register`             | none                    | identifiers + secret |
-| `updateRecoverySecret` | the identifier argument | secret               |
-| `updateIdentifiers`    | the identifier argument | identifiers          |
+| Operation              | Identifier proofs                                      | Payload              |
+| ---------------------- | ------------------------------------------------------ | -------------------- |
+| `register`             | each SIWE identifier                                   | identifiers + secret |
+| `updateRecoverySecret` | the identifier argument                                | secret               |
+| `updateIdentifiers`    | the identifier argument + each SIWE identifier it adds | identifiers          |
+
+Auth does not vouch for SIWE wallets, so `approvedIdentifiers` need not
+include them; each new wallet proves control instead. In step 1 the primary
+account signs a SIWE proof for `register`.
 
 If an escrow refuses the mutation (`MutationRejectedError`) and every replica
 does the same, the controller clears the pending mutation. A transport failure
@@ -95,18 +103,21 @@ to finish that write.
 
 ## Proof of possession
 
-Updates and reads prove control of a registered identifier:
-
-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 proof private key never leaves the controller/caller; read sessions keep it
-in memory. `register` sends `null` instead of an identifier proof. Implementation:
+Updates and reads prove control of a registered identifier; mutations also prove
+each SIWE identifier the escrow has not enrolled:
+
+1. The controller generates an Ed25519 proof key per identifier and requests an
+   identifier token binding `hash([proofPublicKey, requestHash])`: the SIWE nonce
+   or WebAuthn challenge.
+2. Each escrow issues one challenge, keyed to the approval's profile. Every proof
+   key signs `hash([token, challenge.id, requestHash])` for that challenge. The
+   controller sends the approval and the `{ kind: 'key-bound', token, proof }`
+   list with `applyMutation`, or the single proof with `getSecret`.
+3. The escrow C2F verifies the provider assertions, identifiers, request
+   binding, challenge, and proof signatures, then consumes the challenge once.
+
+The proof private keys never leave the controller/caller; read sessions keep
+theirs in memory. A `register` without SIWE identifiers sends no proofs. Implementation:
 `requestKeyBoundIdentifierToken` and `authorizeEscrowsWithToken` in `src/identifier-auth.ts`.
 
 ## Read: `authenticateIdentifier`, `getRecoverySecret`

packages/mfa-recovery-controller/README.md

@@ -16,10 +16,12 @@ 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
+escrow as `approvedIdentifiers`; the escrow rejects non-SIWE payload
+identifiers outside that list. Build those from Auth's entries unchanged
+(verifier included): the controller rejects any other before writing, and the
+mutation can then be aborted. Each SIWE wallet a mutation adds is proven
+through `identifierAuthProvider` instead, including at `register`. Set
+`CubistEscrowProviderOptions.authAudience` to the escrow's
 `AUTH_CONTROLLER_AUDIENCE`.
 
 Before `updateIdentifiers`, call `getIdentifiers()` (AAL1) for the enrolled

packages/mfa-recovery-controller/src/MfaRecoveryController.test.ts

@@ -51,6 +51,13 @@ const SOCIAL: Identifier = {
 
 const OTHER_PASSKEY: Identifier = { ...PASSKEY, value: 'cred-2' };
 
+const WALLET: Identifier = {
+  type: 'siwe',
+  namespace: 'example.com:1',
+  value: '0xAbC0000000000000000000000000000000000001',
+  verifier: { domain: 'example.com', chainId: 1 },
+};
+
 const IDENTIFIERS = [PASSKEY, SOCIAL];
 
 const SECRET = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]);
@@ -901,6 +908,73 @@ describe('MfaRecoveryController', () => {
     );
   });
 
+  describe('SIWE identifiers', () => {
+    it('proves each SIWE identifier at register', async () => {
+      await withController(async ({ controller, escrowA }) => {
+        const apply = vi.spyOn(escrowA, 'applyMutation');
+        await controller.register(SECRET, [PASSKEY, WALLET]);
+        expect(
+          apply.mock.calls[0][2].map(({ token }) => token.identifier),
+        ).toStrictEqual([WALLET]);
+      });
+    });
+
+    it('proves an added SIWE identifier with the enrolled identifier on one challenge', async () => {
+      await withController(async ({ controller, escrowA }) => {
+        await controller.register(SECRET, IDENTIFIERS);
+        const apply = vi.spyOn(escrowA, 'applyMutation');
+        await controller.updateIdentifiers(
+          PASSKEY,
+          [SOCIAL, WALLET],
+          REGISTERED_EPOCH,
+        );
+        const authorizations = apply.mock.calls[0][2];
+        expect(
+          authorizations.map(({ token }) => token.identifier),
+        ).toStrictEqual([PASSKEY, WALLET]);
+        expect(
+          new Set(authorizations.map(({ proof }) => proof.challengeId)).size,
+        ).toBe(1);
+      });
+    });
+
+    it('does not prove an enrolled SIWE identifier in another address case', async () => {
+      await withController(async ({ controller, escrowA }) => {
+        await controller.register(SECRET, [PASSKEY, WALLET]);
+        const apply = vi.spyOn(escrowA, 'applyMutation');
+        await controller.updateIdentifiers(
+          PASSKEY,
+          [SOCIAL, { ...WALLET, value: WALLET.value.toLowerCase() }],
+          REGISTERED_EPOCH,
+        );
+        expect(
+          apply.mock.calls[0][2].map(({ token }) => token.identifier),
+        ).toStrictEqual([PASSKEY]);
+      });
+    });
+
+    it('does not prove SIWE identifiers to an escrow that already applied the mutation', async () => {
+      await withController(async ({ controller, escrowA }) => {
+        await controller.register(SECRET, IDENTIFIERS);
+        escrowA.invalidReceipts = true;
+        await expect(
+          controller.updateIdentifiers(
+            PASSKEY,
+            [SOCIAL, WALLET],
+            REGISTERED_EPOCH,
+          ),
+        ).rejects.toThrow('Invalid mutation receipt');
+        escrowA.invalidReceipts = false;
+        const apply = vi.spyOn(escrowA, 'applyMutation');
+        await controller.resume();
+        expect(
+          apply.mock.calls[0][2].map(({ token }) => token.identifier),
+        ).toStrictEqual([PASSKEY]);
+        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 }) => {
@@ -1783,6 +1857,20 @@ describe('MfaRecoveryController', () => {
         });
       },
     );
+
+    it.each([
+      { ...WALLET, namespace: 'example.com:2' },
+      { ...WALLET, verifier: undefined },
+    ])(
+      'rejects a SIWE namespace that does not match its verifier',
+      async (wallet) => {
+        await withController(async ({ controller }) => {
+          await expect(
+            controller.register(SECRET, [PASSKEY, wallet]),
+          ).rejects.toMatchObject({ code: 'invalid_identifier' });
+        });
+      },
+    );
   });
 });