Repository navigation
feat(express): add default domain-scoped CA trust for BitGo connections - #10073
pranishnepal wants to merge 1 commit into
Conversation
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
7fc7465 to
c8d0fe9
Compare
pranishnepal
changed the title
d2625bb to
21198bd
Compare
| "unprettied": "grep -R -L --include '*.ts' --include '*.js' --include '*.json' '@prettier' src test", | ||
| "fmt": "yarn prettier --write '{src,test}/**/*.{ts,js,json}'" | ||
| "fmt": "yarn prettier --write '{src,test}/**/*.{ts,js,json}'", | ||
| "test:data": "bash ../../scripts/verify-bitgo-policy-data.sh --live" |
There was a problem hiding this comment.
will run prior to the unit test and fail the CI if it needs updating ~30d prior to the actual expiration date
@bitgo/express: $ bash ../../scripts/verify-bitgo-policy-data.sh --live
@bitgo/express: ok: 18 self-signed CA certs valid (>30d), 13 BitGo hosts covered, http downgrades throw
@bitgo/express: live ok: https://app.bitgo.com/ (HTTP 302)
There was a problem hiding this comment.
If it is expired, lets log what the releaser needs to do
There was a problem hiding this comment.
added as log under with RELEASER_STEPS in scripts/verify-bitgo-policy-data.sh
|
@claude review always |
| @@ -0,0 +1,381 @@ | |||
| # GlobalSign Root CA (validTo: Jan 28 12:00:00 2028 GMT) | |||
There was a problem hiding this comment.
Document how this file was generated and how to re-generate it.
There was a problem hiding this comment.
Good point, added bash scripts/verify-bitgo-policy-data.sh --regenerate to do that!
|
Claude finished @zahin-mohammad's task in 54s —— View job Review of PR #10073: default domain-scoped CA trust for BitGo connectionsI read the changed source and the verification script. I did not run the tests or the build. The design is sound. The single funnel in Should address
Worth considering
Verified OK
|
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
modules/express/src/fetchEncryptedPrivKeys.ts— Running the express mocha suite with any CLI argument now kills the run with an argparse usage error, before any test executes. That includes a spec path,--grepor an IDE runner. The new module-levelconfig()call parses process.argv throughparser.parseArgs()when the file is imported.test/unit/clientRoutes/externalSign.tsimports this file, so mocha's own flags reach argparse and it exits with code 2. Fix: do not parse CLI args at import time. Build the policy fromEnvConfig()merged withDefaultConfig, or createbglazily insidefetchKeys/main, so importing the module has no argv side effects.Why this was flagged
modules/express/src/fetchEncryptedPrivKeys.ts:44 now calls
getBitGoTlsPolicy(config())at module scope.config()(config.ts:200) callsargs(), which isparser.parseArgs()over process.argv (args.ts, argparse ^1.0.10). That argparse version callsthis.error('Unrecognized arguments ...')and then process.exit(2) on any unknown token. modules/express/test/unit/clientRoutes/externalSign.ts:66 importsfetchKeysfrom this module, and mocha'sspec: ['test/unit/**/*.ts']loads that file in the mocha process. Soyarn mocha test/unit/clientRoutes/externalSign.ts,yarn mocha --grep foo, or an IDE mocha runner puts mocha's argv in front of argparse, and the process exits with BitGo-Express usage text. On the base branch the import has no argv side effect and these runs work. Plainyarn unit-testpasses no argv, so CI still works.Verification: Trigger: someone runs the express mocha suite with any extra CLI token, such as
yarn unit-test --grep foo. The module-scope call is at fetchEncryptedPrivKeys.ts:44.config()(config.ts:200-204) runsArgConfig(args()). In argparse 1.x, leftover tokens lead toprocess.exit(2). Before this change nothing in src calledargs()at import time. -
🔴
modules/sdk-api/src/tlsPolicy.ts— After this change, every Express call to BitGo opens a new TCP and TLS connection, because pinning is on by default and the pinned agent does not reuse connections.new https.Agent({ ca: normalizedCa })leaveskeepAliveat false. On Node >=20 the global agent superagent used before keeps connections alive. Fix: build the agent withkeepAlive: true, matching the global agent's reuse, so pinning does not add a full handshake to each BitGo request.Why this was flagged
Express turns the policy on by default (
disablePinCerts: falsein config.ts) and passes it to every per-request BitGo client in prepareBitGo (clientRoutes.ts:1839). getAgentRequest (bitgoAPI.ts:500) then callsreq.agent(this._tlsPolicy.agent)for every app.bitgo*.com URL. That agent is built at tlsPolicy.ts:136 with onlyca, sokeepAlivedefaults to false and each socket closes after its response. On the base branch superagent useshttps.globalAgent, which has had keepAlive on by default since Node 19, and the repo supports Node >=20. So every proxied wallet, sign or send call now pays an extra TCP and TLS handshake, which adds latency and CPU. Under load it can also cause TIME_WAIT and ephemeral port exhaustion. Nothing in the diff sets keepAlive or reuses a shared agent.Verification: The policy agent is built at modules/sdk-api/src/tlsPolicy.ts:136-138 as
new https.Agent({ ca: normalizedCa }). Nothing setskeepAlive, so it defaults to false. getAgentRequest (bitgoAPI.ts:497-500) applies it to every covered URL. On the base branch superagent fell back tohttps.globalAgent, which has keepAlive on since Node 19. Every call from Express to BitGo now opens a new TCP connection and does a full TLS handshake.
21198bd to
04ef34b
Compare
504ff59 to
b6ff4f8
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The latest push brings back an issue flagged in an earlier review; replied in that comment's thread.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
b6ff4f8 to
53d5386
Compare
53d5386 to
59e67e8
Compare
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.
|
@claude review always |
|
Claude finished @pranishnepal's task in 16s —— View job Review of PR #10073 (latest push)I read Addressed since earlier reviews
Still open from earlier threads (not re-verified here)
No new issues in the reviewed file. |
Restrict outbound HTTPS connections to official BitGo domains
(*.bitgo.com, *.bitgo-test.com, *.bitgo-staging.com, *.bitgo-dev.com,
and their apex domains) to a bundled multi-provider root CA store
(Google Trust Services, Amazon Trust Services, DigiCert, Let's
Encrypt/ISRG, and Sectigo) while keeping the system trust store for all
non-BitGo destinations:
- sdk-api: add createBitGoTlsPolicy({ ca, hosts }) and enforce it in
BitGoAPI.getAgentRequest (refusing redirects and plain http on covered
hosts)
- express: enable the bundled 18-root CA policy by default at startup,
with a single opt-out flag (--disablepincerts /
BITGO_DISABLE_PIN_CERTS)
and automatic proxy fallback when BITGO_USE_PROXY is set
- scripts/verify-bitgo-policy-data.sh: CI check verifying all bundled
roots are self-signed, valid (>30d), and cover all BitGo domains
(plus --live handshake verification)
TICKET: WCN-3731
59e67e8 to
47e5545
Compare
What
Restricts outbound HTTPS connections from BitGo Express (and
@bitgo/sdk-apiwhentlsPolicyis configured) to official BitGo domains (*.bitgo.com,*.bitgo-test.com,*.bitgo-staging.com,*.bitgo-dev.com, and their apex domains) to a bundled multi-provider Root CA store (modules/express/bitgo-ca.pem) by default, while keeping the standard Node.js / system trust store for all non-BitGo destinations (external signer, block explorers, webhooks, KMS).modules/express/bitgo-ca.pem): Pre-includes the self-signed Root CAs for the 5 tier-1 providers BitGo uses today or would fail over to (Google Trust Services, Amazon Trust Services, DigiCert, Let's Encrypt / ISRG, and Sectigo). This shrinks the trusted CA surface for BitGo domains from ~150 global CAs down to 5 providers (coveringapp.*,admin.*, andcdn.bitgo.comout of the box) without breaking across routine leaf renewals, intermediate rotations, or CA migrations.BitGoAPI.getAgentRequest): Every BitGo-bound request (requestPatch,fetchConstants,executeAssetRequest) goes throughgetAgentRequest, which attaches the restrictedhttps.Agent({ ca }), refuses redirects (.redirects(0)) on covered hosts so a redirect cannot escape the domain policy, and refuses plaintexthttp://downgrades on covered hosts.disablePinCerts: false). Operators can opt out with--disablepincertsorBITGO_DISABLE_PIN_CERTS=true. IfBITGO_USE_PROXYis set, Express logs a startup warning and skips the bundled CA policy so existing proxy deployments continue to work.Why this shape
NODE_EXTRA_CA_CERTScan only add trust; nothing in Node.js lets you remove a compromised CA from the global trust store, and replacing the global store would break non-BitGo outbound calls. Passingcaon a dedicatedhttps.Agentreplaces the trust store only for connections routed through that agent. PairingappliesTo(url)withagentinsideBitGoTlsPolicyguarantees that covered BitGo domains use the restricted bundle while all other destinations keep the system store.What's in here
@bitgo/sdk-api(modules/sdk-api/src/tlsPolicy.ts,bitgoAPI.ts,types.ts):createBitGoTlsPolicy({ ca, hosts })splits and validates every PEM certificate incaviacrypto.X509Certificate, normalizes and validates host patterns viaparsePolicyHost, and returns{ agent, appliesTo(url) }.appliesTo(url)matches official BitGo hostnames (including trailing-dot FQDNs) and throws if a covered host is contacted over plainhttp://or a schemeless URL.getCaExpiries(ca)inspects certificate expiry dates for startup monitoring.BitGoAPI.getAgentRequest(bitgoAPI.ts:497-501) and wired intofetchConstants(bitgoAPI.ts:780).@bitgo/express(modules/express/src/tlsPolicy.ts,bitgo-ca.pem,config.ts,args.ts,expressApp.ts,clientRoutes.ts,fetchEncryptedPrivKeys.ts):modules/express/bitgo-ca.pem(17 self-signed Root CAs across GTS, Amazon, DigiCert, ISRG, and Sectigo).BITGO_DEFAULT_TLS_HOSTS(bitgo.com,.bitgo.com,bitgo-test.com,.bitgo-test.com,bitgo-staging.com,.bitgo-staging.com,bitgo-dev.com,.bitgo-dev.com).--disablepincerts/BITGO_DISABLE_PIN_CERTS(with proper boolean parsing soBITGO_DISABLE_PIN_CERTS=falsestays enabled).checkPreconditions(expressApp.ts:273) and warns viaconsole.warnif any bundled CA expires within 30 days.scripts/verify-bitgo-policy-data.sh,modules/express/package.json):expressunit-test(yarn test:data), verifying that all 17 certificates inbitgo-ca.pemparse, are self-signed CA roots (issuer === subjectandcert.ca === true), are>30days from expiry, and cover all 13 BitGo apex/subdomain hosts.bash scripts/verify-bitgo-policy-data.sh --liveto verify real TLS handshakes against liveapp.*,admin.*, andcdn.bitgo.comendpoints using onlybitgo-ca.pem.How it was tested
modules/sdk-api/test/unit/tlsPolicy.ts(18/18),modules/express/test/unit/tlsPolicy.ts,config.ts,bitgoExpress.ts(55/55), and fullexpressunit suite.scripts/verify-bitgo-policy-data.sh --live): Verified live TLS handshakes againstapp.bitgo.com,app.bitgo-test.com,app.bitgo-staging.com,app.bitgo-dev.com,admin.bitgo*.com, andcdn.bitgo.comusing onlybitgo-ca.pem.app.bitgo-staging.com): Verified authenticated SDK/Express calls (fetchConstants,me(), wallet read, session unlock) with the default 17-root policy enabled, plus negative rejection when an unapproved CA bundle is used (Linear test evidence).TICKET: WCN-3731