From 918ead80ec42dc1b10f7fc0d3fc21bffad8b0677 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Tue, 9 Jul 2024 17:08:55 +0200 Subject: [PATCH] review feedback --- .../playwright-core/bin/socks-certs/README.md | 12 +++++ .../playwright-core/src/server/browser.ts | 12 ++--- .../src/server/browserContext.ts | 8 ++-- .../socksClientCertificatesInterceptor.ts | 46 ++++++++----------- 4 files changed, 42 insertions(+), 36 deletions(-) create mode 100644 packages/playwright-core/bin/socks-certs/README.md diff --git a/packages/playwright-core/bin/socks-certs/README.md b/packages/playwright-core/bin/socks-certs/README.md new file mode 100644 index 0000000000..458d849d02 --- /dev/null +++ b/packages/playwright-core/bin/socks-certs/README.md @@ -0,0 +1,12 @@ +# Certfificates for Socks Proxy + +These certificates are used when client certificates are used with +Playwright. Playwright then creates a Socks proxy, which sits between +the browser and the actual target server. The Socks proxy then does the +TLS upgrade using the certficiates in this directory (locally) while +maintaining the client certificates to the target server. +The certificates are generated via: + +```bash +openssl req -new -newkey rsa:2048 -days 3650 -nodes -x509 -keyout key.pem -out cert.pem -subj "/CN=localhost" +``` diff --git a/packages/playwright-core/src/server/browser.ts b/packages/playwright-core/src/server/browser.ts index e5390889bf..1693c6eae4 100644 --- a/packages/playwright-core/src/server/browser.ts +++ b/packages/playwright-core/src/server/browser.ts @@ -25,7 +25,7 @@ import type { RecentLogsCollector } from '../utils/debugLogger'; import type { CallMetadata } from './instrumentation'; import { SdkObject } from './instrumentation'; import { Artifact } from './artifact'; -import { ClientCertificatesProxy, shouldUseMitmSocksProxy } from './socksClientCertificatesInterceptor'; +import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor'; export interface BrowserProcess { onclose?: ((exitCode: number | null, signal: string | null) => void); @@ -85,13 +85,13 @@ export abstract class Browser extends SdkObject { async newContext(metadata: CallMetadata, options: channels.BrowserNewContextParams): Promise { validateBrowserContextOptions(options, this.options); - let socksServer: ClientCertificatesProxy | undefined; - if (shouldUseMitmSocksProxy(options)) { - socksServer = new ClientCertificatesProxy(options); - options.proxy = { server: await socksServer.listen() }; + let clientCertificatesProxy: ClientCertificatesProxy | undefined; + if (options.clientCertificates?.length) { + clientCertificatesProxy = new ClientCertificatesProxy(options); + options.proxy = { server: await clientCertificatesProxy.listen() }; } const context = await this.doCreateNewContext(options); - context._socksServer = socksServer; + context._clientCertificatesProxy = clientCertificatesProxy; if (options.storageState) await context.setStorageState(metadata, options.storageState); return context; diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index 90512dda5d..09bfa484a1 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -43,7 +43,7 @@ import * as consoleApiSource from '../generated/consoleApiSource'; import { BrowserContextAPIRequestContext } from './fetch'; import type { Artifact } from './artifact'; import { Clock } from './clock'; -import { shouldUseMitmSocksProxy, type ClientCertificatesProxy } from './socksClientCertificatesInterceptor'; +import { type ClientCertificatesProxy } from './socksClientCertificatesInterceptor'; export abstract class BrowserContext extends SdkObject { static Events = { @@ -91,7 +91,7 @@ export abstract class BrowserContext extends SdkObject { private _debugger!: Debugger; _closeReason: string | undefined; readonly clock: Clock; - _socksServer: ClientCertificatesProxy | undefined; + _clientCertificatesProxy: ClientCertificatesProxy | undefined; constructor(browser: Browser, options: channels.BrowserNewContextParams, browserContextId: string | undefined) { super(browser, 'browser-context'); @@ -449,7 +449,7 @@ export abstract class BrowserContext extends SdkObject { await harRecorder.flush(); await this.tracing.flush(); - await this._socksServer?.close(); + await this._clientCertificatesProxy?.close(); // Cleanup. const promises: Promise[] = []; @@ -691,7 +691,7 @@ export function validateBrowserContextOptions(options: channels.BrowserNewContex throw new Error(`Browser needs to be launched with the global proxy. If all contexts override the proxy, global proxy will be never used and can be any string, for example "launch({ proxy: { server: 'http://per-context' } })"`); options.proxy = normalizeProxySettings(options.proxy); } - if (shouldUseMitmSocksProxy(options)) + if (options.clientCertificates?.length) options.ignoreHTTPSErrors = true; verifyGeolocation(options.geolocation); } diff --git a/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts b/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts index 7f1f0a8457..8bc7df60bb 100644 --- a/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts +++ b/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts @@ -21,7 +21,7 @@ import fs from 'fs'; import tls from 'tls'; import stream from 'stream'; import { createSocket } from '../utils/happy-eyeballs'; -import { assert, globToRegex, isUnderTest } from '../utils'; +import { globToRegex, isUnderTest } from '../utils'; import type { SocksSocketClosedPayload, SocksSocketDataPayload, SocksSocketRequestedPayload } from '../common/socksProxy'; import { SocksProxy } from '../common/socksProxy'; import type * as channels from '@protocol/channels'; @@ -38,20 +38,24 @@ class SocksConnectionDuplex extends stream.Duplex { } class SocksProxyConnection { + private readonly socksProxy: ClientCertificatesProxy; + private readonly uid: string; + private readonly host: string; + private readonly port: number; firstPackageReceived: boolean = false; - isTLS: boolean = false; - target!: net.Socket; - // In case of http, we just pipe data to the target socket and they are |undefined|. internal: stream.Duplex | undefined; internalTLS: tls.TLSSocket | undefined; - constructor(private readonly socksProxy: ClientCertificatesProxy, private readonly uid: string, private readonly host: string, private readonly port: number) {} + constructor(socksProxy: ClientCertificatesProxy, uid: string, host: string, port: number) { + this.socksProxy = socksProxy; + this.uid = uid; + this.host = host; + this.port = port; + } async connect() { - // WebKit on Darwin doesn't proxy localhost requests through the given proxy. - // Workaround: Rewrite to localhost during tests. this.target = await createSocket(isUnderTest() ? 'localhost' : this.host, this.port); this.target.on('close', () => this.socksProxy._socksProxy.sendSocketEnd({ uid: this.uid })); this.target.on('error', error => this.socksProxy._socksProxy.sendSocketError({ uid: this.uid, error: error.message })); @@ -73,21 +77,18 @@ class SocksProxyConnection { if (!this.firstPackageReceived) { this.firstPackageReceived = true; // 0x16 is SSLv3/TLS "handshake" content type: https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_record - this.isTLS = data[0] === 0x16; - if (this.isTLS) + if (data[0] === 0x16) this._attachTLSListeners(); else this.target.on('data', data => this.socksProxy._socksProxy.sendSocketData({ uid: this.uid, data })); } - if (this.isTLS) - this.internal!.push(data); + if (this.internal) + this.internal.push(data); else this.target.write(data); } private _attachTLSListeners() { - assert(this.isTLS); - this.internal = new SocksConnectionDuplex(data => this.socksProxy._socksProxy.sendSocketData({ uid: this.uid, data })); this.internalTLS = new tls.TLSSocket(this.internal, { isServer: true, @@ -160,7 +161,8 @@ export class ClientCertificatesProxy { } public async listen(): Promise { - return `socks5://127.0.0.1:${await this._socksProxy.listen(0)}`; + const port = await this._socksProxy.listen(0, '127.0.0.1'); + return `socks5://127.0.0.1:${port}`; } public async close() { @@ -193,20 +195,12 @@ export function clientCertificatesToTLSOptions( for (const { certs } of matchingCerts) { for (const cert of certs) { if (cert.cert) - requestOptions.cert.push(cert.cert); + tlsOptions.cert.push(cert.cert); if (cert.key) - requestOptions.key.push({ pem: cert.key, passphrase: cert.passphrase }); + tlsOptions.key.push({ pem: cert.key, passphrase: cert.passphrase }); if (cert.pfx) - requestOptions.pfx.push({ buf: cert.pfx, passphrase: cert.passphrase }); + tlsOptions.pfx.push({ buf: cert.pfx, passphrase: cert.passphrase }); } } - return requestOptions; -} - -export function shouldUseMitmSocksProxy(options: { - clientCertificates?: channels.BrowserNewContextOptions['clientCertificates']; -}) { - if (options.clientCertificates && options.clientCertificates.length > 0) - return true; - return false; + return tlsOptions; }