WAL-1868: Express endpoint to verify a TSS EdDSA key belongs to a wallet - #9873
yashaanand wants to merge 1 commit into
Conversation
|
|
|
@claude review this PR? are there any critical blockers? |
|
Claude finished @yashaanand's task —— View job PR Review AnalysisReviewing PR for critical blockers and security issues... Todo List:
✅ No Critical Blockers FoundThis PR implements a secure TSS EdDSA key verification feature with excellent security practices. Here's my analysis: 🔒 Security Review - EXCELLENTKey Material Protection:
Error Handling:
🏗️ Architecture Review - SOLIDClean Separation of Concerns:
Extensible Design:
🧪 Test Coverage - COMPREHENSIVESecurity-First Testing:
Edge Cases Covered:
🚀 Implementation Quality - HIGHDefensive Programming:
Performance:
📝 Documentation & Standards - GOOD
Minor Observations (Non-blocking):
🎯 Recommendation: APPROVEThis PR demonstrates exemplary security practices for handling cryptographic material. The implementation is robust, well-tested, and follows all security best practices. No critical issues identified. |
7380885 to
30b6909
Compare
|
@claude have the previous suggestions been implemented? is the PR ready? |
|
Claude finished @yashaanand's task —— View job PR Readiness Assessment ✅I've reviewed the current state of the PR against the previous reviewer suggestions. Here's my analysis: Todo List:
🎯 Assessment Result: SUGGESTIONS HAVE BEEN IMPLEMENTED ✅The PR author has successfully addressed all 5 review suggestions from @mohammadalfaiyazbitgo: ✅ 1. Better Sensitive Fields ValidationSuggestion: "can't we do some better validation and only filter the sensitive fields?" ✅ IMPLEMENTED: The code now uses a dedicated ✅ 2. MethodNotImplemented Error UsageSuggestion: "I think ✅ IMPLEMENTED: The base class now properly uses ✅ 3-5. IO-TS Codec ImplementationSuggestions: Multiple comments about using ✅ IMPLEMENTED: The key verification module (
🔒 Security Improvements Maintained
🏗️ Code Quality Enhancements
📋 Recommendation: READY TO MERGE ✅The PR has successfully implemented all reviewer feedback while maintaining the excellent security posture. The implementation is now more robust with proper type validation and better error handling semantics. |
pranavjain97
left a comment
pranavjain97
left a comment
There was a problem hiding this comment.
clean otherwise. blocked on verifyKey returning match: true for a wrong seed
| const MPC = await Eddsa.initialize(); | ||
| let combinedKey: KeyCombine; | ||
| try { | ||
| combinedKey = MPC.keyCombine(signingMaterial.uShare, [signingMaterial.bitgoYShare, signingMaterial.backupYShare]); |
There was a problem hiding this comment.
a wrong uShare.seed with correct public fields returns match: true. keyCombine only uses the seed to derive u, which is never compared (repro'd locally). regenerate y from the seed with keyShare and compare it to uShare.y. same gap for bitgoYShare.u when v is absent.
Adds a boolean key-verification path so a caller that holds its own TSS user signing material can learn up front whether it belongs to a wallet, instead of discovering a mismatch only when a signature fails, and exposes it as an Express route so REST callers get the same answer. - sdk-core: pure helpers in utils/tss/keyVerification parse and shape-check user signing material with constant, non-leaking errors (the shape check keeps malformed input away from the combine routine, whose raw errors interpolate submitter-controlled fields) and recombine it to compare against a commonKeychain - sdk-core: ITssUtils gains verifyKey and supportsVerifyKey; the base implementation reports unsupported and EddsaUtils implements the EdDSA MPCv1 check, so ECDSA/MPCv2/on-chain wallets reject without a keychain request; TypeScript consumers implementing ITssUtils may need to add the new members; Wallet.verifyKey fetches only the user keychain and returns the result object verbatim for the Express response body; walletPassphrase is deliberately not accepted - express: typed route and handler for POST /api/v2/:coin/wallet/:id/verifyKey, with 400s for unsupported wallet types and malformed material; errors with an HTTP status or a transport-level code keep their semantics instead of being masked as a 400; the decode-error formatter now redacts structured values so key material sent with the wrong field type cannot be echoed back; the corresponding server-side mirror route is a follow-up in the server repo Ticket: WAL-1868
30b6909 to
081c90d
Compare
|
@claude was pranav's change request implemented? any blockers? |
dpkjnr
left a comment
dpkjnr
left a comment
There was a problem hiding this comment.
Nice work overall. The leak-avoidance (constant errors, no JSON.parse echo, redaction of sensitive paths) is thorough, and the tests cover it well. On 081c90d, Pranav's wrong-seed case looks fixed by uShareSeedMatchesY plus the "v must be present" rule.
Three inline comments:
- Medium: genuine material without a VSS commitment
vreturnsmatch: falseinstead of "can't verify" (L149). - Low: the docs describe
match: trueas proof of possession, which it isn't (L129). - Nit: a combine failure gives a 400, while a wrong seed gives
false(L155).
Generated by Claude Code
| if (signingMaterial.bitgoYShare.v === undefined || signingMaterial.backupYShare.v === undefined) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Medium: genuine keys could come back as match: false here.
For user signing material, bitgoYShare.v is copied from the server's bitGoToUserShare.vssProof at keychain creation (utils/tss/eddsa/eddsa.ts:167), and vssProof is optional on the key share type (keychain/iKeychains.ts:216). If any MPCv1 wallet was created while BitGo wasn't returning vssProof, its own valid material would be reported as "not this wallet's key". That's the most misleading answer this endpoint can give, because the user may go looking for a different key.
Two suggestions:
- Throw a distinct error such as
Unable to verify key - signing material has no VSS commitmentinstead of returningfalse, so "can't verify" stays separate from "doesn't match". - Confirm with the server team whether
vssProofhas always been returned for MPCv1 EdDSA wallets. If it has, a comment here saying so would help.
I inferred this from the types; I haven't found a wallet that is actually missing v.
Generated by Claude Code
| * Recombination alone is not proof of possession, so two further checks run first. The seed is | ||
| * bound to `uShare.y`, and the Y shares must carry their VSS commitment `v` — `keyCombine` | ||
| * verifies a Y share's secret `u` only when `v` is present, so omitting it skips the check that | ||
| * the share is consistent with the public `y` it claims. Genuine shares produced by `keyShare` | ||
| * always carry `v`. |
There was a problem hiding this comment.
Low (docs): match: true is a consistency check, not proof of possession.
The seed check closes the wrong-seed case. But someone who knows only the public commonKeychain can still get true:
- Generate their own
uShare; its seed matches itsy. - Forge both Y shares. VSS only checks
u·G == y + i·v, so they can pickufreely, solve forv, and choose theyvalues so all three sum to the wallet's common key.
This is harmless for the intended use (a user checking their own file). However, this comment and the route docs read as though a true result proves the caller holds the wallet's key. Could we say plainly that the result must never be used as an authorization signal? The PR description already lists binding the user share as an open item, so this is only about the wording.
I reasoned this from the Feldman VSS check in keyCombine and did not run a forgery.
Generated by Claude Code
| } catch (e) { | ||
| throw new Error('Invalid user key - could not combine signing material'); | ||
| } |
There was a problem hiding this comment.
Nit: two different answers for "this key doesn't work for this wallet".
A wrong seed returns match: false. A tampered Y-share secret with v present makes keyCombine throw, and the caller gets a 400 "could not combine signing material". Both mean the material can't sign for this wallet, so every caller has to handle both. Could we either map this combine failure to false, or document both outcomes on the route?
Generated by Claude Code
This branch has not been deployed
4 participants
Description
Adds
POST /api/v2/{coin}/wallet/{id}/verifyKeyto BitGo Express, backed by a new SDKWallet.verifyKeymethod: a caller that holds its own TSS EdDSA (MPCv1) user signing material can learn up front whether it belongs to a given wallet, instead of discovering a mismatch only when a real transaction fails to sign.The caller passes the same
prvstring it would use to sign (e.g. on sendmany). Express recombines the shares locally and compares the result against the wallet'scommonKeychain, returning{ "match": true | false }. The key material never leaves the Express process, is never sent to the BitGo server, and is never echoed in any error message or response body.Implementation layers:
utils/tss/keyVerification(constant, non-leaking error messages); a per-algorithm hook onITssUtils(verifyKey+supportsVerifyKey— the base default reports unsupported andEddsaUtilsimplements the EdDSA MPCv1 check), so later algorithm support is a one-class change;Wallet.verifyKeyrejects unsupported wallet types before any network request and fetches only the user keychain.express.v2.wallet.verifyKey; errors with a meaningful HTTP status (e.g. the keychain fetch) surface with their real status instead of being masked as 400; transport-level failures surface as infrastructure errors. The shared decode-error formatter now redacts structured values, so key material sent with the wrong field type can never be echoed back (this also hardens existing routes with sensitive fields).Release note
ITssUtils(sdk-core) gains two required members,verifyKey()andsupportsVerifyKey(). TypeScript consumers that implement this interface may need to add them; in-repo implementers inherit the base defaults.Issue Number
Ticket: WAL-1868
Type of change
How Has This Been Tested?
yarn lerna run build --include-dependencies --scope @bitgo/sdk-core --scope @bitgo/express— clean, on a branch rebased onto latest mastersdk-core test/unit/bitgo/utils/tss/keyVerification.ts— 14 cases: parse/shape validation (missing/malformed/wrong-length shares), genuine material with the optionalvabsent, recombine true/false, constant-error boundary, and leak-rule assertions (no error message ever echoes the input)sdk-core test/unit/bitgo/wallet/walletVerifyKey.ts— 7 cases: match true/false, rejection without any keychain request for on-chain / EdDSA MPCv2 / ECDSA wallets, request-tracer passthrough, missing-commonKeychainkeychainexpress test/unit/typedRoutes/verifyKey.ts— 9 cases: 200 true/false, 400 with the SDK message, missing-prv400, upstream-status passthrough (404 wallet lookup, 429 from inside verifyKey), transport failure surfacing as 500, non-Error message, and non-stringprvrejected without echoing the materialexpress test/unit/typedRoutes/formatValidationErrors.ts— redaction contract for structured valuesyarn lintclean in both modules;yarn check-commitscleanChecklist:
Follow-ups (not blocking this PR)
express/src/clientRoutes.ts, every new v2 Express route must also be registered in the BitGo server'swww/config/routesV2.jsso the monolith returns a "call BitGo Express" error. That file lives in the private server repo and needs a corresponding change there; tracked on WAL-1868.commonKeychain; cryptographically binding the submitted user share to its seed (and making VSS verification mandatory) is an open review discussion tracked internally on WAL-1868.walletPassphrasesupport, on-chain multisig, TSS ECDSA, and TSS EdDSA MPCv2/Redpallas.