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 7 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
…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) <[email protected]>
…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) <[email protected]>
…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) <[email protected]>
…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) <[email protected]>
… 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) <[email protected]>
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) <[email protected]>
…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) <[email protected]>
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.
| input: string; | ||
| password?: string; | ||
| adata?: string; | ||
| encryptionVersion?: 2; |
There was a problem hiding this comment.
Major — use a named alias. A bare literal 2 makes encryptionVersion: 1 a type error, even though v1 is the current default — so callers forwarding from a generic config ({ encryptionVersion: cfg.version }) need a cast. Suggest:
export type EncryptionVersion = 1 | 2;
export interface EncryptOptions {
input: string;
password?: string;
adata?: string;
encryptionVersion?: EncryptionVersion;
}This prepares for v3 without a hunt-and-replace across the 8+ option types where the field appears (GenerateWalletOptions, GenerateMpcWalletOptions, CreateMpcOptions, CreateBackupOptions, CreateKeychainParamsBase, etc.). Also: adata should get a JSDoc note that it is silently dropped when encryptionVersion === 2 — see the inline comment on BitGoAPI.encryptAsync.
| createEncryptionSession(password: string): Promise<{ | ||
| encrypt(plaintext: string): Promise<string>; | ||
| decrypt(ciphertext: string): Promise<string>; | ||
| destroy(): void; |
There was a problem hiding this comment.
Major — extract a named IEncryptionSession. The anonymous inline return type is repeated 4+ times across the PR (ecdsaMPCv2.ts addUserKeychain/addBackupKeychain parameter types, eddsa.ts session usage, etc.). EncryptionSession already exists as a class in modules/sdk-api/src/encryptionSession.ts. The structural problem is that sdk-core can't import from sdk-api (circular). Fix: declare an IEncryptionSession interface in modules/sdk-core/src/api/types.ts, export it from sdk-core/src/index.ts, and have both this interface and the EncryptionSession class in sdk-api reference it.
Without this, no external SDK consumer can write let session: ??? with a proper type import.
| throw new Error('cannot encrypt without password'); | ||
| } | ||
| if (params.encryptionVersion === 2) { | ||
| return encryptV2(params.password, params.input); |
There was a problem hiding this comment.
Major — params.adata is silently dropped on the v2 path. v1 forwards adata to sjcl.encrypt; v2 calls encryptV2(password, input) with no AAD. Two consequences:
- Callers passing
adataget false confidence that AAD is bound — it is not. - In
ecdsaMPCv2.ts, the v1 path usesadata = ${hashBuffer}:${derivationPath}to bind the encrypted round-session to the transaction context (validateAdataenforces the binding). The v2 path has no equivalent (see inline comment onecdsaMPCv2.tsline 1242).
v2's AES-GCM is self-authenticating for the ciphertext — but doesn't replicate the transaction-context binding unless AAD is threaded through encryptV2 envelopes. Fix: add an additionalData?: string option to encryptV2/aesGcmEncrypt, store it in the v2 envelope, and verify in decryptV2. Or, at minimum, throw if params.adata is set and encryptionVersion === 2 so the silent drop becomes a loud error.
| passphrase: string; | ||
| enterprise?: string | undefined; | ||
| originalPasscodeEncryptionCode?: string | undefined; | ||
| encryptionVersion?: 2; |
There was a problem hiding this comment.
Major — accepted but never read. EcdsaUtils.createKeychains (non-MPCv2 ECDSA) declares encryptionVersion?: 2 in its param type, but the function body never references it — createParticipantKeychain (line 299) still calls this.bitgo.encrypt(...) synchronously. Result: a caller that passes encryptionVersion: 2 to a non-MPCv2 ECDSA wallet gets v1 silently.
Fix: either thread encryptionVersion through createParticipantKeychain (mirroring the MPCv2 pattern), or remove the field from the param type if non-MPCv2 ECDSA is not in scope for v2 in this PR.
| password: passphrase, | ||
| }); | ||
| if (encryptionVersion === 2) { | ||
| const session = await this.bitgo.createEncryptionSession(passphrase); |
There was a problem hiding this comment.
Major — separate Argon2id session per keychain. createUserKeychain (line 200) and createBackupKeychain (line 295) each create their own createEncryptionSession for one encryption then immediately destroy(). Since both run in parallel via Promise.all (line 397/407 of createKeychains), Argon2id is computed twice unnecessarily.
EcdsaMPCv2Utils.createKeychains does this correctly: a single shared session created in the parent function and threaded into both addUserKeychain and addBackupKeychain. Please mirror that pattern here — create one session in createKeychains, pass it into both children, destroy() it in finally. Saves one full Argon2id derivation per keygen.
| round1Session = await this.bitgo.decryptAsync({ input: encryptedRound1Session, password: walletPassphrase }); | ||
| } else { | ||
| round1Session = this.bitgo.decrypt({ input: encryptedRound1Session, password: walletPassphrase }); | ||
| this.validateAdata(adata, encryptedRound1Session); |
There was a problem hiding this comment.
Major / defense-in-depth — v2 path skips transaction-context binding. The v1 path (this.validateAdata(adata, encryptedRound1Session) on line 1246) binds the encrypted round-session to ${hashBuffer}:${derivationPath}, preventing replay across different transaction signing contexts. The v2 branch above (line 1242) has no equivalent.
GCM's authentication tag covers the ciphertext, but does not bind the session to a specific transaction — that property was provided by adata in v1. AAD support needs to be threaded through encryptV2 / EncryptionSession envelopes (see inline on BitGoAPI.encryptAsync), or the v2 round-session needs a different binding mechanism, or the relaxation needs to be documented as accepted security trade-off (with rationale for why round-session replay is not a concern for ephemeral signing keys).
This is also relevant for line 1337 (round-3 path).
Note: Zhongxi's HSM-1513 work (commits e3bc07cbf6 / d531ed2802 on a separate branch) adds round domain separators to v1 adata. v2 should get the equivalent before this lands.
| prebuildTransaction(params?: PrebuildTransactionOptions): Promise<PrebuildTransactionResult>; | ||
| signTransaction(params?: WalletSignTransactionOptions): Promise<SignedTransaction>; | ||
| getUserPrv(params?: GetUserPrvOptions): string; | ||
| getUserPrv(params?: GetUserPrvOptions): Promise<string>; |
There was a problem hiding this comment.
Blocker — breaking-change commit marker missing. This signature change (string → Promise<string>) is a public-API break for @bitgo/sdk-core consumers. IWallet is exported from the package barrel; downstream JS / non-strict-TS callers who do const prv = wallet.getUserPrv(opts); signTransaction({ prv }) will silently pass a Promise<string> to the signer.
lerna publish --conventional-commits (Angular preset) only triggers a major bump on feat!: / fix!: / BREAKING CHANGE: footer. None of the 7 commits on this PR carry that marker, so sdk-core will publish under a minor bump. The repo's Check breaking changes CI job is OpenAPI-only and does not catch SDK type-signature changes.
Fix: add a follow-up commit (or rewrite 36c78cd) with body:
BREAKING CHANGE: IWallet.getUserPrv now returns Promise<string> instead of string.
Callers must await the result.
Same applies to the decryptKeychainPrivateKey export — see inline on decryptKeychain.ts.
| export async function decryptKeychainPrivateKey( | ||
| bitgo: BitGoBase, | ||
| keychain: OptionalKeychainEncryptedKey, | ||
| password: string |
There was a problem hiding this comment.
Blocker — same as IWallet.getUserPrv. This is a named public export of @bitgo/sdk-core. The async signature change requires a BREAKING CHANGE: footer so lerna bumps the package to a major version on publish. Without it the published package will be a minor bump and downstream callers in untyped JS / non-strict TS will silently get a Promise where they expect a string | undefined.
Fix: roll into the same BREAKING CHANGE: footer as getUserPrv (or split into a separate footer). Suggest:
BREAKING CHANGE:
- IWallet.getUserPrv now returns Promise<string> (was string)
- decryptKeychainPrivateKey now returns Promise<string | undefined> (was string | undefined)
All callers must await the result.
Also note: modules/bitgo/test/unit/decryptKeychain.ts (5 sync decryptKeychainPrivateKey calls at lines 18, 36, 55, 74, 78) was not updated — see the summary for details. Please add await and make the test functions async so the file actually validates the new behavior.
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