Repository navigation
fix(express): require authentication for signer-mode routes (WAL-2458) - #10150
Charan-Ratlan wants to merge 1 commit into
Conversation
|
@claude review |
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| assertWalletNotLocked(walletId); | ||
| try { | ||
| const walletPassphrase = getWalletPwFromEnv(walletId); | ||
| const encryptedPrivKey = await getEncryptedPrivKey(signerFileSystemPath, walletId); | ||
| const privKey = await decryptPrivKey(bitgo, encryptedPrivKey, walletPassphrase); | ||
| walletSignFailures.delete(walletId); | ||
| return { privKey, walletPassphrase }; | ||
| } catch (e) { | ||
| recordWalletSignFailure(walletId); | ||
| throw new ApiResponseError(WALLET_SIGN_FAILURE_MESSAGE, 400); | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Operators who fix a broken key file can still get 400s for up to 15 minutes, and nothing is logged. The lockout also blocks no passphrase guessing, since passphrases now come only from env. getSignerWalletCredentials counts every failure toward lockout, including a missing key-file entry, a JSON read/parse error and EMFILE. Fix: count only failures driven by caller input, or drop the lockout, since no route takes a caller passphrase now. A key-file fix must take effect on the next request, as on the base branch.
Why this was flagged
After this change no signer route takes a passphrase from the caller. The OFC body passphrase was removed, and getWalletPwFromEnv is the only source (clientRoutes.ts:517). So every failure counted by recordWalletSignFailure (clientRoutes.ts:523) comes from server-side state, not from a guess. The key file is re-read on every request (getEncryptedPrivKey, clientRoutes.ts:465). On the base branch, an operator who corrects encryptedPrivKeys.json (for example, re-running the fetch script for a missing or wrong walletId entry) restores signing at once, without a restart. Now five failed attempts for that walletId set lockedUntil 15 minutes ahead (WALLET_SIGN_FAILURE_LOCKOUT_MS, clientRoutes.ts:56). That is one or two user sends, since each TSS signing makes 3-4 signer calls. assertWalletNotLocked (clientRoutes.ts:487-491) then rejects even after the file is fixed. A transient read error or a partly written file during an update locks every wallet hit in that window the same way. The 400 looks the same as the original error, and nothing is logged.
Verification: In clientRoutes.ts, the catch in getSignerWalletCredentials calls recordWalletSignFailure(walletId) for every exception. After 5 failures it sets a 15-minute lockedUntil, and assertWalletNotLocked then throws the 400 before anything is read. Nothing is logged. On the base branch, fixing encryptedPrivKeys.json worked on the next request.
83c3f51 to
7a0fe6f
Compare
|
@claude review |
…AL-2458) problem/goal The signer auth secret was only length-checked, but it must travel as the X-Auth-Signer header value: a secret with a trailing newline makes Node refuse to send the header, and surrounding whitespace is stripped by header parsing on the signer, so both instances start fine while every signing forward fails with a 401 at runtime. implementation details - Both startup gates (signerMode and externalSignerUrl) now require the secret to match /^[\x21-\x7e]+$/ in addition to the 16-character minimum: printable, non-whitespace ASCII only, rejected on the signer and the online instance alike. verification - modules/express: tsc, prettier and eslint clean; full express unit suite with BITGOJS_TEST_PASSWORD set: 2012 passing, 0 failing across consecutive runs, including new cases for trailing newline, surrounding and interior whitespace, and non-ASCII secret characters. Ticket: WAL-2458
7a0fe6f to
9e2d6d6
Compare
|
@claude review |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
rishikeshdadam136
left a comment
rishikeshdadam136
left a comment
There was a problem hiding this comment.
Might be a breaking change as discussed and merge conflicts also
problem/goal
Signer-mode Express instances registered their key-using routes (/api/v2/:coin/sign, /tssshare/*, /api/v2/ofc/signPayload) with only the prepareBitGo middleware, which checks no credential, so anyone who could reach the signer's port could have the stored wallet keys applied to attacker-chosen transactions and payloads. The OFC route additionally accepted walletPassphrase in the request body (an unthrottled passphrase oracle), and distinct wallet-lookup error messages allowed walletId enumeration and disclosed the key-file path.
implementation details
verification
BREAKING CHANGE: signer-mode deployments must set BITGO_SIGNER_AUTH_SECRET (--signerauthsecret) and listen on loopback/IPC or configure TLS client authentication (--sslca together with --keypath/--crtpath or --sslkey/--sslcert). Instances using --externalSignerUrl must also set the secret. The OFC extSignPayload route no longer accepts walletPassphrase in the request body (environment variable only). Upgrade the online express instance before the signer; older signers ignore the new header.
Ticket: WAL-2458