feat: opt-in v2 Argon2+HKDF encryption for multisig and MPC flows#8592
feat: opt-in v2 Argon2+HKDF encryption for multisig and MPC flows#8592pranavjain97 wants to merge 10 commits intomasterfrom
Conversation
|
Wallet sharing to be done as a separate ticket. |
pranavjain97
changed the base branch from
pranavjain/wcn-31-phase-1-hkdf-caching-layer-for-multi-call-operations
to
master
pranavjain97
force-pushed
the
pranavjain/wcn-32-v2-encryption-call-sites
branch
from
48cfa0a to
36c78cd
Compare
zahin-mohammad
left a comment
zahin-mohammad
left a comment
There was a problem hiding this comment.
Review Summary
After multiple validation rounds with parallel reviewers (architect, crypto, call-site tracer, test-gap, devil's-advocate, consensus-checker), REQUEST CHANGES with two blockers and a focused set of inline comments.
Blockers
B1. No breaking-change marker on commits. None of the 7 commits on this PR carry feat!:, fix!:, or a BREAKING CHANGE: footer. lerna publish --conventional-commits (Angular preset) requires one of those for a major bump. Result: @bitgo/sdk-core will publish a minor bump despite IWallet.getUserPrv and decryptKeychainPrivateKey changing from sync string returns to Promise<string>. Downstream JS / non-strict-TS consumers will silently get a Promise where they expect a string. The repo's Check breaking changes CI job only diffs OpenAPI specs from modules/express — it does not guard SDK type signatures, so this is a code-review responsibility entirely. Fix: add a BREAKING CHANGE: footer to commit 36c78cd (or a follow-up commit) describing the async return-type change so lerna bumps sdk-core to a major version on publish.
B2. Sync v1 decrypt fan-out callers not converted, no follow-up tickets linked. A v2-encrypted wallet that flows into any of these sites hard-throws with a confusing SJCL tag-mismatch error. Blast radius is bounded today (v2 is opt-in), but every client that opts into v2 will trip at least one of these in normal usage:
modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts:647—assertIsValidKeycallssjcl.decryptdirectlymodules/abstract-utxo/src/recovery/crossChainRecovery.ts:316— syncdecrypt(passphrase, encryptedPrv)modules/sdk-core/src/bitgo/recovery/initiate.ts:112—bitgo.decrypt(affects EOS/TRX/STX/XRP/UTXO backup-key recovery)modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts:567, 601— non-MPCv2 ECDSA offline signing roundsmodules/sdk-core/src/bitgo/keychain/keychains.ts:170—updateSingleKeychainPasswordmodules/sdk-core/src/bitgo/keychain/keychains.ts:354-364—recreateMpc(three sync decrypts)modules/sdk-core/src/bitgo/trading/tradingAccount.ts:72— OFC payload signingmodules/sdk-core/src/bitgo/wallet/wallets.ts(acceptShare / reshareWalletWithSpenders) — explicitly deferred per the PR comment
Fix options (any one): (a) convert each path to decryptAsync in this PR; (b) link follow-up tickets in the PR description for each path; (c) add a v2-opt-in guard at wallet creation that fails fast on these callers with a clear "v2 wallet operations require WCN-XX" error rather than letting SJCL surface a misleading tag-mismatch.
Major findings
See inline comments below.
Test coverage gap (not blocking, but please address)
The useV2 dispatch branches in createOfflineRound{1,2,3}Share (MPCv2) and getUserToBitgoCommitment / decryptRShare (EdDSA) currently have zero unit-test coverage. CI is green on the v2 dispatch only because no test exercises the branch — the live testnet runs in the PR description don't substitute for unit-level branch coverage. Please add at least one v2 signing-path test per coin family.
Separately: modules/bitgo/test/unit/decryptKeychain.ts (5 sync call sites) was not updated for the new async signature. The test runner uses tsx (no type-check), so this is not a CI break — but the assertions now run against a Promise object. Either the file silently fails to load and is skipped, or it asserts against a Promise and that's masked by the cdn=true reporter option. Either way the file no longer validates the function. Please add await and make the test functions async.
What was retracted from earlier review rounds
For the record:
- "CI will fail to compile" — wrong;
tsxstrips types. The decryptKeychain test file is a silent no-op, not a build break. - "MPCv2 signing creates 3× Argon2id per signing" — wrong; each round is a separate HTTP request lifecycle, so cross-round session sharing is structurally impossible without server-side session storage.
- The v2
adatadrop is documented design intent per commit22bab61c(AES-GCM is self-authenticating). The narrower remaining concern (cross-round replay binding) is captured in the inline comment onecdsaMPCv2.tsbelow.
Nits
isV2Envelopeis duplicated verbatim inecdsaMPCv2.tsandeddsa.ts— extract to a shared util.decryptAsyncfalls through to v1 SJCL on unknown future versions (v: 3+). Should fail closed withError('unknown envelope version').- No JSDoc on
EncryptOptions.adatawarning that it's silently dropped whenencryptionVersion: 2.
Great findings. Addressed all review comments across 3 commits:
|
…ase/BitGoAPI WCN-32: Adds async encryption dispatch (v1/v2 based on encryptionVersion param) and session-based encryption to the BitGoBase interface and BitGoAPI implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hain types WCN-32: Thread encryptionVersion?: 2 through GenerateWalletOptions, GenerateMpcWalletOptions, CreateMpcOptions, CreateBackupOptions, and both Lightning/GoAccount codecs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion WCN-32: Convert sync encrypt() to async encryptAsync() in wallet generation and keychain creation paths. Thread encryptionVersion from GenerateWalletOptions through Lightning, GoAccount, TSS, and onchain multisig flows. Default remains v1. Only opt-in encryptionVersion: 2 triggers v2 encryption. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n and signing WCN-32: DKLS keygen uses encryption session when encryptionVersion: 2, signing rounds auto-detect v2 from envelope and use decryptAsync/session. validateAdata skipped for v2 envelopes. All v1 paths unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… signing WCN-32: EdDSA keygen uses encryption session when encryptionVersion: 2, signing auto-detects v2 from envelope and uses decryptAsync/session. All v1 paths unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WCN-32: Verify that createKeychains with encryptionVersion: 2 produces v2 envelopes for encryptedPrv/reducedEncryptedPrv and that they are decryptable via decryptAsync. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…etection WCN-32: Convert decryptKeychainPrivateKey to use decryptAsync internally so signing flows work with both v1 and v2 encrypted keychains. Make getUserPrv async and update all callers across sdk-core, abstract-utxo, abstract-eth, and bitgo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- add EncryptionVersion type alias and IEncryptionSession interface - restore sync getUserPrv/decryptKeychainPrivateKey, add async variants - share encryption session in EdDSA keygen (saves one Argon2id derivation) - extract isV2Envelope to shared util in baseTypes - fail closed on unknown envelope version in decryptAsync - remove encryptionVersion from GG18 ECDSA (not in scope, tracked in WCN-283) - fix decryptKeychain tests and add async test suite WCN-32
…ntext binding - add optional adata param to aesGcmEncrypt/aesGcmDecrypt (GCM additionalData) - store adata in v2 envelope, use as GCM AAD on encrypt and decrypt - add adata param to EncryptionSession.encrypt and IEncryptionSession - forward adata from encryptAsync to encryptV2 for v2 path - DKLS signing: pass adata to session.encrypt, call validateAdata on v2 decrypt - fix typesEddsaMPCv2 imports (renamed EddsaMPCv2KeyGen* to MPCv2KeyGen*) - add AAD round-trip, tamper detection, and session adata tests WCN-32
pranavjain97
force-pushed
the
pranavjain/wcn-32-v2-encryption-call-sites
branch
from
178ec63 to
3d25190
Compare
… with adata - e2e 3-round offline signing with v2 encrypted keys and adata context binding - validates v2 envelopes with adata at each round boundary (round 1->2->3) - tests validateAdata rejects mismatched adata on v2 envelopes - fix typesEddsaMPCv2 imports (EddsaMPCv2KeyGen* -> MPCv2KeyGen*) WCN-32
pranavjain97
force-pushed
the
pranavjain/wcn-32-v2-encryption-call-sites
branch
from
3d25190 to
4ae4ec3
Compare
zahin-mohammad
left a comment
zahin-mohammad
left a comment
There was a problem hiding this comment.
Re-review summary
Most of the prior round's blockers and majors are resolved -- thanks for the careful refactor. Three items remain before merge:
Resolved ✅
- B1 (breaking-change marker on
getUserPrv/decryptKeychainPrivateKey): solved differently and better -- sync versions kept,*Asyncvariants added. No public API break. - AAD threading:
aesGcmEncrypt/Decrypt,EncryptionSession.encrypt(plaintext, adata?), andencryptV2({adata})now bind AAD via the GCM tag, withvalidateAdataenforcing context binding on v2 round-session decryption (ecdsaMPCv2.ts:1267, 1366). Round domain separators applied. EncryptionVersion = 1 | 2type alias andIEncryptionSessioninterface extracted tosdk-core/src/api/types.ts.isV2Envelopeextracted to sharedbaseTypes.ts.decryptAsyncfails closed on unknown envelope versions.- EdDSA single shared session in
createKeychains-- saves one Argon2id derivation per keygen. decryptKeychain.tstest file now has both sync and async test suites.- MPCv2 e2e v2 signing test exercises
createOfflineRound{1,2,3}with adata round-trip.
Remaining
B3 (blocker, must fix). Three call sites still await the sync getUserPrv (left over from the async-revert in 62d00bcd33). On v1 wallets these are no-ops; on v2 wallets they hard-throw with a misleading SJCL tag-mismatch. See inline comments on:
modules/abstract-eth/src/abstractEthLikeNewCoins.ts:2557modules/abstract-utxo/src/impl/btc/inscriptionBuilder.ts:305(line 265 in the same file is a pre-existing version of the same issue and should also be flipped)
Fix is one token per site: getUserPrv → getUserPrvAsync.
M1 (acknowledgement required). The original B2 sync-decrypt fan-out is unaddressed and there are no follow-up tickets linked in the PR description. v2-encrypted wallets that flow through any of these will surface the same misleading SJCL tag-mismatch error:
sdk-core/src/bitgo/baseCoin/baseCoin.ts:647--assertIsValidKey(sjcl.decryptdirect)sdk-core/src/bitgo/keychain/keychains.ts:170--updateSingleKeychainPasswordsdk-core/src/bitgo/keychain/keychains.ts:361, 366, 371--recreateMpc(3× sync decrypt)sdk-core/src/bitgo/wallet/wallets.ts:972, 1016, 1027, 1092, 1127, 1256, 1376, 1440-- share/reshare flowssdk-core/src/bitgo/recovery/initiate.ts:112-- recovery pathsdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts:567, 601-- non-MPCv2 ECDSA offline signing roundssdk-core/src/bitgo/trading/tradingAccount.ts:72-- OFC payload signingabstract-utxo/src/recovery/crossChainRecovery.ts:316-- syncdecrypt(passphrase, encryptedPrv)
Given v2 is opt-in and not yet flipped to default, I'm OK with this not being fixed in this PR -- but please file follow-up tickets and link them in the PR description so we don't lose track. Could you confirm the plan here?
M2 (must add). EdDSA v2 signing dispatch has no unit-test coverage. The MPCv2 e2e test is great, but the equivalent v2 branches in EdDSA are uncovered. See inline on eddsa.ts:466.
Minors (nice-to-have)
- Test arity bug on
validateAdata-- inline. - Codec/type alignment on
encryptionVersion-- inline.
Security and back-compat summary
- Crypto design is sound after the AAD-threading commit. v2 envelope adata is bound by GCM tag;
validateAdataprovides context binding; cross-round/cross-tx replay prevented to v1 parity. Argon2 parameters bounded -- DoS via crafted envelopes capped. - Public API is additive only (no breaking changes). Conventional
feat:is correct.
Will re-approve once B3 is fixed, M1 is acknowledged with linked tickets, and M2 has at least one test.
|
|
||
| const userKeychain = await this.keychains().get({ id: wallet.keyIds()[0] }); | ||
| const userPrv = wallet.getUserPrv({ keychain: userKeychain, walletPassphrase }); | ||
| const userPrv = await wallet.getUserPrv({ keychain: userKeychain, walletPassphrase }); |
There was a problem hiding this comment.
Blocker -- regression. getUserPrv was reverted to sync in 62d00bcd33, but the await added here in 083ebcf3 was not reverted. This still calls the sync method, which on a v2-encrypted wallet routes through SJCL and hard-throws with password error - ccm: tag doesn't match. The await makes this look correct but the runtime call is sync.
Fix:
- const userPrv = await wallet.getUserPrv({ keychain: userKeychain, walletPassphrase });
+ const userPrv = await wallet.getUserPrvAsync({ keychain: userKeychain, walletPassphrase });This is createHopTransactionParams -- every EVM hop transaction on a v2 wallet would fail.
| ): Promise<SubmitTransactionResponse> { | ||
| const userKeychain = await this.wallet.baseCoin.keychains().get({ id: this.wallet.keyIds()[KeyIndices.USER] }); | ||
| const prv = this.wallet.getUserPrv({ keychain: userKeychain, walletPassphrase }); | ||
| const prv = await this.wallet.getUserPrv({ keychain: userKeychain, walletPassphrase }); |
There was a problem hiding this comment.
Blocker -- same regression as abstractEthLikeNewCoins.ts:2557. This now calls sync getUserPrv with a leftover await; v2 wallets hard-throw at SJCL.
- const prv = await this.wallet.getUserPrv({ keychain: userKeychain, walletPassphrase });
+ const prv = await this.wallet.getUserPrvAsync({ keychain: userKeychain, walletPassphrase });While you're here, line 265 (signAndSendCommit) has the same pattern (pre-existing -- the await was already there before this PR, but the function was always sync). Same getUserPrv → getUserPrvAsync fix applies. Both inscription flows would fail on v2 wallets without it.
| const stringifiedRShare = JSON.stringify(userSignShare); | ||
| const encryptedRShare = this.bitgo.encrypt({ input: stringifiedRShare, password: params.walletPassphrase }); | ||
| let encryptedRShare: string; | ||
| if (params.encryptedPrv && isV2Envelope(params.encryptedPrv)) { |
There was a problem hiding this comment.
Major -- no unit test for the EdDSA v2 signing dispatch. The v2 dispatch branches here (createCommitmentShareFromTxRequest line 466 and createRShareFromTxRequest line 489) are gated by isV2Envelope(...) and never exercised by any test. The only EdDSA v2 test in tssUtils/eddsa.ts:581 covers keygen.
MPCv2 has an e2e v2 round-trip test (signTxRequest.ts:270-372) -- please add the equivalent for EdDSA. At minimum:
- Encrypt a prv via
encryptAsync({encryptionVersion: 2}). - Call
createCommitmentShareFromTxRequestwith the v2encryptedPrv-- assertencryptedUserToBitgoRShare.shareis a v2 envelope. - Round-trip through
createRShareFromTxRequest-- assert it decrypts correctly.
Without this, a regression in either dispatch branch will pass CI.
| adata: 'context-A', | ||
| }); | ||
|
|
||
| (() => (tssUtils as any).validateAdata('context-B', ct)).should.throw(/Adata does not match/); |
There was a problem hiding this comment.
Minor -- test calls validateAdata with 2 args, signature requires 3. Per the round-domain-separator commit e7dfa8e865, the signature is validateAdata(adata, cyphertext, roundDomainSeparator). Here roundDomainSeparator is undefined.
The test passes by accident: when roundDomainSeparator is undefined, the function compares envelope.adata ("context-A") against "undefined:context-B" and "context-B" -- both fail, so it throws as expected. But this means the test wouldn't catch a regression in the round-domain binding logic.
Pass an explicit roundDomainSeparator so the test exercises the intended check:
(() => (tssUtils as any).validateAdata('context-B', ct, EcdsaMPCv2Utils.DKLS23_SIGNING_ROUND1_STATE)).should.throw(/Adata does not match/);Ideally also add a positive case asserting validateAdata accepts a correctly-bound envelope.
| }), | ||
| t.partial({ | ||
| lightningProvider: t.union([t.literal('amboss'), t.literal('voltage')]), | ||
| encryptionVersion: t.literal(2), |
There was a problem hiding this comment.
Minor -- codec narrower than declared TS type. EncryptionVersion = 1 | 2 (api/types.ts:20), but this codec uses t.literal(2). A caller forwarding a generic cfg.version: EncryptionVersion will pass type-check but fail codec validation when version === 1.
Either widen the codec to allow both:
encryptionVersion: t.union([t.literal(1), t.literal(2)])or add a comment explaining that the codec deliberately rejects 1 (since v1 is the implicit default and explicitly passing it through the wire is unexpected). Same applies to line 127.
2 participants
Summary
Wire v2 encryption (Argon2id + AES-256-GCM + HKDF session caching) into wallet creation and signing call sites across multisig, DKLS MPCv2, and EdDSA flows.
encryptionVersion: 2on wallet/key creationdecryptKeychainPrivateKeymade async to support v1/v2 auto-detection in signing pathsLive Node.JS Testing (testnet)
All flows tested end-to-end on testnet with real transactions:
v2 wallet creation is consistently faster due to HKDF session caching. Signing is roughly equivalent -- network round trips dominate.
Test plan
TICKET: WCN-32