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.
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 - Add --signerauthsecret / BITGO_SIGNER_AUTH_SECRET config, required when signerMode or externalSignerUrl is set, and --sslca / BITGO_SSL_CA for TLS client authentication. - Add requireSignerAuth middleware in front of prepareBitGo on all three signer routes: dedicated X-Auth-Signer header, constant-time SHA-256 digest comparison, generic 401 before any key material is accessed. - Attach the secret on the online client side at every forward to the signer: custom signing function, OFC forward, and all TSS round generators. - checkPreconditions: refuse to start a signer without a secret of at least 16 characters, and refuse signerMode on non-loopback binds without TLS client authentication (sslCa together with server TLS) regardless of env; createHttpsServer enables requestCert, rejectUnauthorized and ca from sslCa. - Route all signer-mode wallet access through getSignerWalletCredentials: one uniform 400 error for unknown wallet, missing passphrase, unreadable key file and failed decryption, env-only passphrase on the OFC route, and a per-walletId lockout after 5 consecutive failures for 15 minutes. - Document the new requirements in EXTERNAL_SIGNER.md and README.md. verification - modules/express: tsc -p . --noEmit clean; prettier and eslint clean on all touched files. - Full express unit suite with BITGOJS_TEST_PASSWORD set: 2006 passing, 0 failing (pristine master baseline: 1991 passing), including new middleware, gate, uniform-error, lockout and client-forward tests. 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
83c3f51 to
7a0fe6f
Compare
|
@claude review |
| // authentication so the secret cannot be intercepted on the network. | ||
| if ( | ||
| signerMode !== undefined && | ||
| (signerAuthSecret === undefined || signerAuthSecret.length < MIN_SIGNER_AUTH_SECRET_LENGTH) | ||
| ) { | ||
| throw new ExternalSignerConfigError( | ||
| `signerMode requires a signer auth secret of at least ${MIN_SIGNER_AUTH_SECRET_LENGTH} characters ` + | ||
| '(--signerauthsecret / BITGO_SIGNER_AUTH_SECRET).' | ||
| ); | ||
| } | ||
|
|
||
| // An instance using an external signer must be able to authenticate to it. | ||
| if ( | ||
| externalSignerUrl !== undefined && | ||
| (signerAuthSecret === undefined || signerAuthSecret.length < MIN_SIGNER_AUTH_SECRET_LENGTH) | ||
| ) { | ||
| throw new ExternalSignerConfigError( | ||
| `externalSignerUrl requires a signer auth secret of at least ${MIN_SIGNER_AUTH_SECRET_LENGTH} characters ` + | ||
| '(--signerauthsecret / BITGO_SIGNER_AUTH_SECRET) so requests to the signer can be authenticated.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Operators whose signer secret has a trailing newline, surrounding spaces or non-Latin-1 characters get two instances that start fine, but every signing forward fails. checkPreconditions only checks signerAuthSecret.length (expressApp.ts:292, :303). The secret then has to travel as the x-auth-signer header (clientRoutes.ts:162). Fix: at startup, on both the signer and the online instance, reject (or trim) a secret that is not a valid header value, e.g. require /^[\x21-\x7e]+$/. Without that, a newline makes Node refuse to send the header, and surrounding whitespace is stripped by the signer's parser, so every request gets a 401.
Why this was flagged
An operator loads BITGO_SIGNER_AUTH_SECRET from a Kubernetes secret built with --from-file, so the value ends in \n. readEnvVar (config.ts:7-10) does not trim it, and the length check at expressApp.ts:292/303 passes, so both instances start. On every forward, withSignerAuth calls .set(SIGNER_AUTH_HEADER, options.signerAuthSecret) (clientRoutes.ts:162). Node's setHeader rejects the CR/LF with ERR_INVALID_CHAR, so every external-signer sign, TSS share and OFC signPayload request fails. A secret with leading or trailing spaces or tabs is sent, but the signer's HTTP parser strips the outer whitespace. The SHA-256 compare in requireSignerAuth (clientRoutes.ts:2032-2034) then fails, and every request gets { error: 'unauthorized' } with no hint why. Characters above U+00FF also fail at setHeader. On the base branch no secret existed, so these deployments signed normally. Nothing at startup catches it, because only the length is validated.
Verification: readEnvVar (config.ts:7-10) returns process.env[name] as-is, with no trim. The startup checks (expressApp.ts:290-308) only test signerAuthSecret.length, so both instances start. withSignerAuth sets the secret as a header (clientRoutes.ts:161-162). A trailing \n makes Node throw ERR_INVALID_CHAR on every forward. With surrounding spaces, requireSignerAuth (clientRoutes.ts:2036-2038) returns 401.
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