Skip to content

Commit 3dacebb

Browse files
huggingbotcursoragent
huggingbot
and
cursoragent
authored
feat: prove new SIWE identifiers with recovery mutations (#11151)
## Explanation Stacked on #11149. SIWE wallets prove control themselves (Auth doesn't vouch for them). - `applyMutation` takes `IdentifierAuthorization[]`, sharing one challenge per escrow. - Each SIWE identifier an escrow hasn't enrolled gets a proof, including at `register`. SIWE is exempt from the `approvedIdentifiers` check. ## References - Escrow: consensys-vertical-apps/cubist-secret-escrow#7 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by updating changelogs for packages I've changed - [ ] I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them --------- Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 238fd01 commit 3dacebb

14 files changed

Lines changed: 746 additions & 206 deletions

‎packages/mfa-recovery-controller/ARCHITECTURE.md‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,9 @@ stateDiagram-v2
9292

9393
Rules:
9494

95-
- **`abort()`** drops `authorizing` only. `writing` must `resume()`
95+
- **`abort()`** drops `authorizing`, or `writing` once its approval has expired
96+
and every escrow is still below `newVersion`; `resume()` then does the same
97+
and throws `MutationRejectedError`. Otherwise `writing` must `resume()`
9698
(`abort_not_allowed`).
9799
- `writing` is persisted before the **first escrow apply**, after the
98100
AuthController token is issued, every target escrow has accepted identifier
@@ -118,8 +120,8 @@ Anything that fails is rejected rather than acted on:
118120
an identifier.
119121
- `writing` only: the approval binds `sub` to `profileId` and `ext.request_hash`
120122
to `requestHash`, with `aal >= 2`. `register` / `updateIdentifiers` also need
121-
`identifiers_hash` to match `approvedIdentifiers`, and every payload
122-
identifier to equal one of them exactly. This runs before `writing` is
123+
`identifiers_hash` to match `approvedIdentifiers`, and every non-SIWE
124+
payload identifier to equal one of them exactly. This runs before `writing` is
123125
persisted: an escrow rejection is not a `Deny`, so a mismatch found there
124126
would leave an unabortable write. Receipts must be well-formed with at most
125127
one per escrow.
@@ -147,9 +149,12 @@ sequenceDiagram
147149
M->>M: skip escrows with a stored receipt, re-verify stored receipts
148150
M->>E: isAvailable (remaining escrows)
149151
M->>M: reuse token, or re-issue it if exp <= now()
150-
alt not register
151-
M->>I: getKeyBoundIdentifierToken
152-
M->>E: generateChallenge + PoP per remaining escrow (all required)
152+
opt updateIdentifiers with SIWE identifiers
153+
M->>E: getIdentifiers (enrolled set at expectedVersion)
154+
end
155+
opt update identifier, or SIWE identifiers not yet enrolled
156+
M->>I: getKeyBoundIdentifierToken per identifier
157+
M->>E: generateChallenge + PoPs per remaining escrow (all required)
153158
end
154159
M->>M: wrap secret per escrow, persist writing
155160
par write every remaining replica
@@ -178,8 +183,12 @@ Auth approvals:
178183
- The approval's `identifiers` are persisted with its token as
179184
`approvedIdentifiers` and refreshed with it. `register` and
180185
`updateIdentifiers` send them to every escrow, which checks them against
181-
`identifiers_hash` and rejects payload identifiers outside them. Hashing
182-
sorts object keys, not arrays.
186+
`identifiers_hash` and rejects non-SIWE payload identifiers outside them.
187+
Hashing sorts object keys, not arrays.
188+
- Auth does not vouch for SIWE wallets. Each SIWE identifier an escrow has not
189+
enrolled needs its own proof in `identifierAuthorizations`; updates add one
190+
proof for the `identifier` argument. Escrows past `expectedVersion` replay
191+
their receipt, so they get no SIWE proofs.
183192
- Missing or stale MFA rejects with `AuthenticationController`'s
184193
`StepUpRequiredError`.
185194
- Escrows verify signatures. The controller decodes claims only for pending-state
@@ -211,8 +220,8 @@ Versioning:
211220

212221
`resume()` is the same write path without creating a new mutation:
213222
authorizing pending gets a fresh token then replicates; writing pending
214-
retries only escrows that do not already have a stored receipt. Updates
215-
re-run identifier auth (a new key-bound token and PoP) for those escrows on
223+
retries only escrows that do not already have a stored receipt. Updates and
224+
mutations adding SIWE wallets re-run identifier auth (a new key-bound token and PoP) for those escrows on
216225
every attempt, so the identifier provider must be able to authenticate again.
217226

218227
## Cubist integration
@@ -301,13 +310,13 @@ the highest one. The next mutation still requires `resume()` first.
301310
flowchart TD
302311
Start[identifier + requestHash] --> Mode{trusted type?}
303312
Mode -->|passkey / social / siwe| KB[fresh proof key, IdP key-bound token]
304-
KB --> Chal[each escrow generateChallenge]
305-
Chal --> PoP[sign PoP over hash of token + challengeId + requestHash]
313+
KB --> Chal[each escrow generateChallenge, one per escrow]
314+
Chal --> PoP[each proof key signs hash of token + challengeId + requestHash]
306315
Mode -->|emailOtp / smsOtp| Todo[TODO MFA-605 / MFA-606 / MFA-568]
307316
Mode -->|anything else| Reject[unknown_identifier_type]
308317
```
309318

310319
On **mutation**, every remaining target escrow must authorize or the write
311320
does not start (`identifier_auth_failed`). On **read**, escrows whose
312-
challenge fails are skipped. `register` sends no identifier authorization
313-
(`null`).
321+
challenge fails are skipped. A `register` without SIWE identifiers sends no
322+
identifier authorizations (`[]`).

‎packages/mfa-recovery-controller/CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1414
- 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))
1515
- 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))
1616
- Passes the operation's AuthController token to `RecoveryEscrowProvider.generateChallenge`, and requires `RecoveryEscrowProvider.getIdentifiers` ([#11149](https://github.com/MetaMask/core/pull/11149))
17+
- 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))
18+
- Rejects SIWE identifiers with malformed verifier fields, an invalid address or domain, or a `namespace` that is not their verifier's `domain:chainId` with `invalid_identifier` ([#11151](https://github.com/MetaMask/core/pull/11151))
19+
- Rejects duplicate identifiers with `duplicate_identifier` and more than 16 identifiers with `too_many_identifiers` ([#11151](https://github.com/MetaMask/core/pull/11151))
20+
- Clears a `writing` mutation in `abort()` and `resume()` once its approval has expired and no escrow applied it; `resume()` then throws `MutationRejectedError` ([#11151](https://github.com/MetaMask/core/pull/11151))
1721
- Exports `MfaRecoveryController`, `getDefaultMfaRecoveryControllerState`, and the controller's action, event, messenger, state, and options types.
1822
- 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))
1923
- Exports `CubistEscrowProvider` and `CubistEscrowProviderOptions` for storing recovery secrets with CubeSigner.

‎packages/mfa-recovery-controller/CUBIST_TEST_SEQUENCE.md‎

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -71,22 +71,30 @@ sequenceDiagram
7171
Auth-->>C: JWT bound to requestHash
7272
C->>Cubist: isAvailable
7373
74-
opt updates only (not register)
74+
opt updateIdentifiers adding SIWE identifiers
75+
C->>Cubist: getIdentifiers(JWT)
76+
Cubist-->>C: enrolled identifiers, version
77+
end
78+
opt updates, or SIWE identifiers not yet enrolled
7579
Note over C,Cubist: Proof of possession for requestHash (see below)
7680
end
7781
7882
Note over C: Encrypt the secret to the Cubist wrap key
79-
C->>Cubist: applyMutation(mutation, JWT, { token, proof } or null, payload, approvedIdentifiers)
83+
C->>Cubist: applyMutation(mutation, JWT, [{ token, proof }, ...], payload, approvedIdentifiers)
8084
Cubist-->>C: signed receipt
8185
Note over C: Verify the receipt with the pinned receipt key
8286
C-->>Mobile: done
8387
```
8488

85-
| Operation | Identifier proof | Payload |
86-
| ---------------------- | ----------------------- | -------------------- |
87-
| `register` | none | identifiers + secret |
88-
| `updateRecoverySecret` | the identifier argument | secret |
89-
| `updateIdentifiers` | the identifier argument | identifiers |
89+
| Operation | Identifier proofs | Payload |
90+
| ---------------------- | ------------------------------------------------------ | -------------------- |
91+
| `register` | each SIWE identifier | identifiers + secret |
92+
| `updateRecoverySecret` | the identifier argument | secret |
93+
| `updateIdentifiers` | the identifier argument + each SIWE identifier it adds | identifiers |
94+
95+
Auth does not vouch for SIWE wallets, so `approvedIdentifiers` need not
96+
include them; each new wallet proves control instead. In step 1 the primary
97+
account signs a SIWE proof for `register`.
9098

9199
If an escrow refuses the mutation (`MutationRejectedError`) and every replica
92100
does the same, the controller clears the pending mutation. A transport failure
@@ -95,18 +103,21 @@ to finish that write.
95103

96104
## Proof of possession
97105

98-
Updates and reads prove control of a registered identifier:
99-
100-
1. The controller generates an Ed25519 proof key and requests an identifier token
101-
binding `hash([proofPublicKey, requestHash])`: the SIWE nonce or WebAuthn challenge.
102-
2. The proof key signs `hash([token, challenge.id, requestHash])` for each escrow.
103-
The controller sends the approval and `{ kind: 'key-bound', token, proof }`
104-
with `applyMutation` or `getSecret`.
105-
3. The escrow C2F verifies the provider assertion, registered identifier, request
106-
binding, challenge, and proof signature.
107-
108-
The proof private key never leaves the controller/caller; read sessions keep it
109-
in memory. `register` sends `null` instead of an identifier proof. Implementation:
106+
Updates and reads prove control of a registered identifier; mutations also prove
107+
each SIWE identifier the escrow has not enrolled:
108+
109+
1. The controller generates an Ed25519 proof key per identifier and requests an
110+
identifier token binding `hash([proofPublicKey, requestHash])`: the SIWE nonce
111+
or WebAuthn challenge.
112+
2. Each escrow issues one challenge, keyed to the approval's profile. Every proof
113+
key signs `hash([token, challenge.id, requestHash])` for that challenge. The
114+
controller sends the approval and the `{ kind: 'key-bound', token, proof }`
115+
list with `applyMutation`, or the single proof with `getSecret`.
116+
3. The escrow C2F verifies the provider assertions, identifiers, request
117+
binding, challenge, and proof signatures, then consumes the challenge once.
118+
119+
The proof private keys never leave the controller/caller; read sessions keep
120+
theirs in memory. A `register` without SIWE identifiers sends no proofs. Implementation:
110121
`requestKeyBoundIdentifierToken` and `authorizeEscrowsWithToken` in `src/identifier-auth.ts`.
111122

112123
## Read: `authenticateIdentifier`, `getRecoverySecret`

‎packages/mfa-recovery-controller/README.md‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
Coordinates MFA recovery through injected auth and escrow providers.
44

55
Pending mutations are encrypted in controller state. `resume()` completes them;
6-
`abort()` is allowed only before writing. See [Architecture](ARCHITECTURE.md).
6+
`abort()` is allowed before writing, or once the write approval has expired and
7+
no escrow applied the mutation. See [Architecture](ARCHITECTURE.md).
78

89
## Auth API Integration
910

@@ -16,10 +17,12 @@ Without a recent MFA verification, approvals reject with `StepUpRequiredError`:
1617
verify, then call `resume()` or retry the read.
1718

1819
`register` and `updateIdentifiers` send the approval's `identifiers` to each
19-
escrow as `approvedIdentifiers`; the escrow rejects payload identifiers outside
20-
that list. Build payload identifiers from those entries unchanged (verifier
21-
included): the controller rejects any other identifier before writing, and the
22-
mutation can then be aborted. Set `CubistEscrowProviderOptions.authAudience` to the escrow's
20+
escrow as `approvedIdentifiers`; the escrow rejects non-SIWE payload
21+
identifiers outside that list. Build those from Auth's entries unchanged
22+
(verifier included): the controller rejects any other before writing, and the
23+
mutation can then be aborted. Each SIWE wallet a mutation adds is proven
24+
through `identifierAuthProvider` instead, including at `register`. Set
25+
`CubistEscrowProviderOptions.authAudience` to the escrow's
2326
`AUTH_CONTROLLER_AUDIENCE`.
2427

2528
Before `updateIdentifiers`, call `getIdentifiers()` (AAL1) for the enrolled

‎packages/mfa-recovery-controller/src/MfaRecoveryController-method-action-types.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,19 @@ export type MfaRecoveryControllerGetIdentifiersAction = {
8080
};
8181

8282
/**
83-
* Completes a persisted pending mutation, if any.
83+
* Completes a persisted pending mutation, if any. Clears a `writing` one,
84+
* throwing `MutationRejectedError`, once its approval has expired and no
85+
* escrow applied it.
8486
*/
8587
export type MfaRecoveryControllerResumeAction = {
8688
type: `MfaRecoveryController:resume`;
8789
handler: MfaRecoveryController['resume'];
8890
};
8991

9092
/**
91-
* Drops `authorizing` pending. `writing` must be finished with {@link resume}.
93+
* Drops `authorizing` pending. Drops `writing` pending only once its approval
94+
* has expired and no escrow applied it; otherwise finish it with
95+
* {@link resume}.
9296
*/
9397
export type MfaRecoveryControllerAbortAction = {
9498
type: `MfaRecoveryController:abort`;

0 commit comments

Comments
 (0)