Skip to content

Commit 609391a

Browse files
fix: two end-to-end bugs blocking filterRequest + ephemeral CA (#260)
* fix(network): updateConfig() throws DataCloneError when filterRequest is set structuredClone cannot clone functions. When the consumer passes a network.filterRequest callback (introduced in #258), the deep-clone in updateConfig() throws DataCloneError. Callers that catch the throw end up with config never set, so initialize() runs with no config and the proxy falls through to opaque tunnelling instead of TLS termination. Pull filterRequest out before cloning and restore the reference after — a function reference is immutable in the sense the clone is protecting against. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(terminating-tls): leaf AKI mismatched ephemeral CA's SKI → chain verify failed node-forge's getExtension('subjectKeyIdentifier').subjectKeyIdentifier returns a hex string for in-memory forge-created certs (e.g. the ephemeral CA from #259) but raw bytes for certs parsed from PEM (e.g. a user-supplied CA). Passing the hex string as authorityKeyIdentifier's keyIdentifier caused the leaf's AKI to be the ASCII bytes of the hex string, which doesn't match the CA's SKI — chain verification fails with 'unable to get local issuer certificate'. AKI is optional; issuer/subject DN match is sufficient for chain building. Drop the extension. Regression test: `openssl verify` against both the fixture CA and an ephemeral CA. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(terminating-tls): end-to-end curl through proxy with ephemeral CA Regression for the AKI fix: same curl-through-proxy round-trip as the fixture-CA describe, but with createMitmCA({}). curl --cacert points at the ephemeral CA's certPath; would fail 'unable to get local issuer' without the AKI fix. Also: agent:false on the proxy's outbound https.request. A proxy's outbound leg shouldn't share the process-global connection pool; this also documents (without fixing — it's process-wide) a Bun quirk where the first request's `ca:` value sticks. The test sidesteps that by using the fixture CA for the upstream leg in both describes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 462c889 commit 609391a

6 files changed

Lines changed: 122 additions & 31 deletions

File tree

src/sandbox/mitm-leaf.ts

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,12 @@ export function mintLeafCert(ca: MitmCA, hostname: string): LeafCert {
4848
},
4949
{ name: 'extKeyUsage', serverAuth: true },
5050
{ name: 'subjectAltName', altNames: [sanFor(hostname)] },
51-
{
52-
name: 'authorityKeyIdentifier',
53-
keyIdentifier:
54-
(
55-
ca.cert.getExtension('subjectKeyIdentifier') as {
56-
subjectKeyIdentifier?: string
57-
} | null
58-
)?.subjectKeyIdentifier ?? true,
59-
},
51+
// No authorityKeyIdentifier: node-forge stores SKI as a hex string for
52+
// in-memory certs (e.g. the ephemeral CA) but as raw bytes for certs
53+
// parsed from PEM (e.g. a user-supplied CA). Passing the hex string as
54+
// keyIdentifier produces an AKI that doesn't match the CA's SKI and
55+
// chain verification fails. AKI is optional — issuer/subject DN match
56+
// is sufficient — so omit it.
6057
])
6158
cert.sign(ca.key, md.sha256.create())
6259

src/sandbox/sandbox-manager.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -765,8 +765,12 @@ function getConfig(): SandboxRuntimeConfig | undefined {
765765
* @param newConfig - The new configuration to use
766766
*/
767767
function updateConfig(newConfig: SandboxRuntimeConfig): void {
768-
// Deep clone the config to avoid mutations
769-
config = structuredClone(newConfig)
768+
// Deep clone the config to avoid mutations. structuredClone cannot clone
769+
// functions, so pull filterRequest out, clone the rest, and put it back —
770+
// a function reference is immutable in the sense that matters here.
771+
const { filterRequest, ...rest } = newConfig.network
772+
config = structuredClone({ ...newConfig, network: rest })
773+
config.network.filterRequest = filterRequest
770774
// Re-resolve parent proxy so hot-reload picks up changes. Note: the proxy
771775
// servers capture `parentProxy` by value at creation, so changes here take
772776
// effect only on re-initialize. This keeps the state consistent for the

src/sandbox/tls-terminate-proxy.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,11 @@ async function forwardUpstream(
186186
// omitting the key, so spread conditionally.
187187
...(isIP(target.hostname) ? {} : { servername: target.hostname }),
188188
...(target.upstreamCA ? { ca: target.upstreamCA } : {}),
189+
// No global agent: a proxy's outbound leg shouldn't share a connection
190+
// pool keyed on the proxy process. Also works around a Bun quirk where
191+
// the first request's `ca:` value is cached on the global agent and
192+
// subsequent calls with a different `ca:` are silently ignored.
193+
agent: false,
189194
},
190195
upRes => {
191196
res.writeHead(upRes.statusCode ?? 502, stripHopByHop(upRes.headers))

test/sandbox/mitm-leaf.test.ts

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { tmpdir } from 'node:os'
55
import { join } from 'node:path'
66
import { X509Certificate } from 'node:crypto'
77
import { createSecureContext } from 'node:tls'
8-
import { createMitmCA } from '../../src/sandbox/mitm-ca.js'
8+
import { createMitmCA, disposeMitmCA } from '../../src/sandbox/mitm-ca.js'
99
import { mintLeafCert, secureContextFor } from '../../src/sandbox/mitm-leaf.js'
1010
import { whichSync } from '../../src/utils/which.js'
1111

@@ -72,22 +72,32 @@ describe('mitm-leaf: mintLeafCert', () => {
7272
// Chain verification via openssl — the strongest correctness signal.
7373
// Skipped only if openssl is unavailable.
7474
const verifyTest = whichSync('openssl') !== null ? test : test.skip
75-
verifyTest('leaf verifies against the CA via `openssl verify`', () => {
76-
const leaf = mintLeafCert(ca, 'verify.example')
77-
const dir = mkdtempSync(join(tmpdir(), 'srt-mitm-leaf-'))
78-
try {
79-
const leafPath = join(dir, 'leaf.crt')
80-
writeFileSync(leafPath, firstPemBlock(leaf.certPem))
81-
const out = execFileSync(
82-
'openssl',
83-
['verify', '-CAfile', CA_CERT, leafPath],
84-
{ encoding: 'utf8' },
85-
)
86-
expect(out.trim()).toMatch(/: OK$/)
87-
} finally {
88-
rmSync(dir, { recursive: true, force: true })
89-
}
90-
})
75+
verifyTest(
76+
'leaf verifies against the CA via `openssl verify` (fixture and ephemeral)',
77+
async () => {
78+
// Regression: with an ephemeral (forge-generated) CA, node-forge stores
79+
// the SKI as a hex string, which the leaf's authorityKeyIdentifier was
80+
// copying verbatim → AKI ≠ SKI → chain verification failed.
81+
const ephemeral = createMitmCA({})
82+
const dir = mkdtempSync(join(tmpdir(), 'srt-mitm-leaf-'))
83+
try {
84+
for (const c of [ca, ephemeral]) {
85+
const leaf = mintLeafCert(c, 'verify.example')
86+
const leafPath = join(dir, 'leaf.crt')
87+
writeFileSync(leafPath, firstPemBlock(leaf.certPem))
88+
const out = execFileSync(
89+
'openssl',
90+
['verify', '-CAfile', c.certPath, leafPath],
91+
{ encoding: 'utf8' },
92+
)
93+
expect(out.trim()).toMatch(/: OK$/)
94+
}
95+
} finally {
96+
rmSync(dir, { recursive: true, force: true })
97+
await disposeMitmCA(ephemeral)
98+
}
99+
},
100+
)
91101
})
92102

93103
function firstPemBlock(pem: string): string {

test/sandbox/tls-terminate-proxy.test.ts

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { spawn } from 'node:child_process'
55
import { readFileSync } from 'node:fs'
66
import { join } from 'node:path'
77
import { createHttpProxyServer } from '../../src/sandbox/http-proxy.js'
8-
import { createMitmCA } from '../../src/sandbox/mitm-ca.js'
8+
import { createMitmCA, disposeMitmCA } from '../../src/sandbox/mitm-ca.js'
99
import { mintLeafCert } from '../../src/sandbox/mitm-leaf.js'
1010

1111
// Committed test-only CA — see test/fixtures/tls-terminate/README.md.
@@ -145,6 +145,67 @@ describe('tls-terminate-proxy: end-to-end through createHttpProxyServer', () =>
145145
})
146146
})
147147

148+
// Regression: same end-to-end path with an SRT-generated ephemeral CA
149+
// (createMitmCA({})). #259 introduced ephemeral CAs; the leaf-minting AKI
150+
// extension turned out to encode the SKI as a hex string (rather than raw
151+
// bytes) for forge-created CAs, breaking chain verification — caught only
152+
// when testing against a non-fixture CA.
153+
describe('tls-terminate-proxy: end-to-end with ephemeral CA', () => {
154+
test('curl trusts the ephemeral-CA-signed leaf and round-trips', async () => {
155+
const ca = createMitmCA({})
156+
// Upstream uses the FIXTURE CA (same as the other describe) so the
157+
// proxy's outbound `ca:` value is identical across the file — Bun's
158+
// https.request caches the first `ca:` process-wide. The regression
159+
// under test is the client-facing leaf (ephemeral CA → curl), which is
160+
// covered by mitmCA below + curl --cacert pointing at the ephemeral CA.
161+
const fixtureCA = createMitmCA({ caCertPath: CA_CERT, caKeyPath: CA_KEY })
162+
const upCert = mintLeafCert(fixtureCA, '127.0.0.1')
163+
const upLeafOnly = upCert.certPem.match(
164+
/-----BEGIN CERTIFICATE-----[\s\S]*?-----END CERTIFICATE-----\r?\n?/,
165+
)![0]
166+
const upstream = createHttpsServer(
167+
{ cert: upLeafOnly, key: upCert.keyPem },
168+
(req, res) => {
169+
let body = ''
170+
req.on('data', c => (body += c))
171+
req.on('end', () => {
172+
res.writeHead(200, { 'x-upstream': 'ok' })
173+
res.end(JSON.stringify({ echoed: body, path: req.url }))
174+
})
175+
},
176+
)
177+
await new Promise<void>(r => upstream.listen(0, '127.0.0.1', r))
178+
const upstreamPort = (upstream.address() as AddressInfo).port
179+
180+
const proxy = createHttpProxyServer({
181+
filter: () => true,
182+
mitmCA: ca,
183+
tlsTerminateUpstreamCA: CA_PEM,
184+
})
185+
await new Promise<void>(r => proxy.listen(0, '127.0.0.1', () => r()))
186+
const proxyPort = (proxy.address() as AddressInfo).port
187+
188+
try {
189+
const r = await curlViaProxy(
190+
proxyPort,
191+
`https://127.0.0.1:${upstreamPort}/hello?a=1`,
192+
{ method: 'POST', body: 'from-ephemeral', cacert: ca.certPath },
193+
)
194+
expect(r.exit).toBe(0)
195+
expect(r.status).toBe(200)
196+
expect(r.headers['x-upstream']).toBe('ok')
197+
const parsed = JSON.parse(r.body)
198+
expect(parsed.echoed).toBe('from-ephemeral')
199+
expect(parsed.path).toBe('/hello?a=1')
200+
expect(r.stderr).toMatch(/issuer:.*sandbox-runtime ephemeral CA/)
201+
} finally {
202+
await new Promise<void>(r => proxy.close(() => r()))
203+
await new Promise<void>(r => upstream.close(() => r()))
204+
await disposeMitmCA(ca)
205+
}
206+
})
207+
})
208+
148209
type CurlResult = {
149210
exit: number
150211
status: number
@@ -156,15 +217,15 @@ type CurlResult = {
156217
async function curlViaProxy(
157218
proxyPort: number,
158219
url: string,
159-
opts: { method?: string; body?: string } = {},
220+
opts: { method?: string; body?: string; cacert?: string } = {},
160221
): Promise<CurlResult> {
161222
const args = [
162223
'-sS',
163224
'-v', // TLS issuer line goes to stderr
164225
'--proxy',
165226
`http://127.0.0.1:${proxyPort}`,
166227
'--cacert',
167-
CA_CERT,
228+
opts.cacert ?? CA_CERT,
168229
'--max-time',
169230
'10',
170231
'-D',

test/sandbox/update-config.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,20 @@ describe('SandboxManager.updateConfig', () => {
162162
expect(fullConfig).toBeDefined()
163163
expect(fullConfig?.network.allowedDomains).toEqual([])
164164
})
165+
166+
it('preserves filterRequest across updateConfig (structuredClone cannot clone functions)', () => {
167+
const filterRequest = async () => ({ action: 'allow' as const })
168+
// Must not throw: structuredClone(fn) throws DataCloneError; the
169+
// function is pulled out, the rest is cloned, then the reference is
170+
// restored.
171+
SandboxManager.updateConfig({
172+
network: { allowedDomains: [], deniedDomains: [], filterRequest },
173+
filesystem: { denyRead: [], allowWrite: [], denyWrite: [] },
174+
})
175+
expect(SandboxManager.getConfig()?.network.filterRequest).toBe(
176+
filterRequest,
177+
)
178+
})
165179
})
166180

167181
describe('SandboxManager.updateConfig proxy filtering', () => {

0 commit comments

Comments
 (0)