Skip to content

Commit ab5b0cf

Browse files
ayoub-el-kajji-vayoub-el-kajji-v
committed
fix after reviews
1 parent c8754da commit ab5b0cf

2 files changed

Lines changed: 83 additions & 70 deletions

File tree

‎@vates/nbd-client/NbdTcpClient.mjs‎

Lines changed: 59 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import { once } from 'node:events'
2-
import { isIP, isIPv6, Socket } from 'node:net'
2+
import { request as httpRequest } from 'node:http'
3+
import { request as httpsRequest } from 'node:https'
4+
import { isIPv6, Socket } from 'node:net'
35
import { connect } from 'node:tls'
46
import { spawn } from 'node:child_process'
57
import AbstractNbdClient from './AbstractNbdClient.mjs'
@@ -14,6 +16,7 @@ import { NBD_DEFAULT_PORT, NBD_OPT_STARTTLS } from './constants.mjs'
1416
export default class NbdTcpClient extends AbstractNbdClient {
1517
#httpProxy
1618
#proxyRejectUnauthorized
19+
#proxyTimeout
1720
#serverAddress
1821
#serverCert
1922
#serverPort
@@ -26,18 +29,37 @@ export default class NbdTcpClient extends AbstractNbdClient {
2629
* @param {string} [settings.cert] - PEM certificate, enables TLS when set
2730
* @param {string} [settings.httpProxy] - URL of an HTTP(S) proxy, the connection is tunneled through it with `CONNECT`
2831
* @param {boolean} [settings.proxyRejectUnauthorized=true] - whether to check the certificate of an HTTPS proxy
32+
* @param {number} [settings.proxyTimeout=6e4] - delay in ms after which an unanswered `CONNECT` is aborted
2933
* @param {object} [options] - see {@link AbstractNbdClient}
3034
*/
3135
constructor(
32-
{ address, port = NBD_DEFAULT_PORT, exportname, cert, httpProxy, proxyRejectUnauthorized = true },
36+
{
37+
address,
38+
port = NBD_DEFAULT_PORT,
39+
exportname,
40+
cert,
41+
httpProxy,
42+
proxyRejectUnauthorized = true,
43+
proxyTimeout = 6e4,
44+
},
3345
options
3446
) {
3547
super({ exportname }, options)
48+
// other proxy protocols (e.g. SOCKS) are not supported for now
49+
if (httpProxy !== undefined) {
50+
const { protocol } = new URL(httpProxy)
51+
if (protocol !== 'http:' && protocol !== 'https:') {
52+
const error = new Error(`unsupported proxy protocol ${protocol}, only http: and https: are supported`)
53+
error.code = 'NBD_PROXY_UNSUPPORTED_PROTOCOL'
54+
throw error
55+
}
56+
}
3657
this.#serverAddress = address
3758
this.#serverPort = port
3859
this.#serverCert = cert
3960
this.#httpProxy = httpProxy
4061
this.#proxyRejectUnauthorized = proxyRejectUnauthorized
62+
this.#proxyTimeout = proxyTimeout
4163
}
4264

4365
/**
@@ -50,81 +72,48 @@ export default class NbdTcpClient extends AbstractNbdClient {
5072
async #connectThroughHttpProxy() {
5173
const proxy = new URL(this.#httpProxy)
5274
const isHttps = proxy.protocol === 'https:'
53-
// URL keeps the brackets around IPv6 addresses
54-
const proxyHost = proxy.hostname.replace(/^\[(.*)\]$/, '$1')
55-
const proxyPort = Number(proxy.port) || (isHttps ? 443 : 80)
5675

57-
let socket
58-
if (isHttps) {
59-
socket = connect({
60-
host: proxyHost,
61-
port: proxyPort,
62-
rejectUnauthorized: this.#proxyRejectUnauthorized,
63-
// SNI does not support IP addresses
64-
servername: isIP(proxyHost) === 0 ? proxyHost : undefined,
65-
})
66-
await once(socket, 'secureConnect')
67-
} else {
68-
socket = new Socket()
69-
socket.connect(proxyPort, proxyHost)
70-
await once(socket, 'connect')
76+
const address = this.#serverAddress
77+
const target = `${isIPv6(address) ? `[${address}]` : address}:${this.#serverPort}`
78+
const headers = { host: target }
79+
if (proxy.username !== '' || proxy.password !== '') {
80+
const credentials = `${decodeURIComponent(proxy.username)}:${decodeURIComponent(proxy.password)}`
81+
headers['proxy-authorization'] = `Basic ${Buffer.from(credentials).toString('base64')}`
7182
}
7283

73-
try {
74-
const address = this.#serverAddress
75-
const target = `${isIPv6(address) ? `[${address}]` : address}:${this.#serverPort}`
76-
let request = `CONNECT ${target} HTTP/1.1\r\nHost: ${target}\r\n`
77-
if (proxy.username !== '' || proxy.password !== '') {
78-
const credentials = `${decodeURIComponent(proxy.username)}:${decodeURIComponent(proxy.password)}`
79-
request += `Proxy-Authorization: Basic ${Buffer.from(credentials).toString('base64')}\r\n`
80-
}
81-
socket.write(request + '\r\n')
82-
83-
// read the proxy response headers, the NBD server may already have sent
84-
// some data after them
85-
const { head, rest } = await new Promise((resolve, reject) => {
86-
let buffer = Buffer.alloc(0)
87-
const cleanUp = () => {
88-
socket.removeListener('data', onData)
89-
socket.removeListener('end', onEnd)
90-
socket.removeListener('error', reject)
91-
}
92-
const onData = chunk => {
93-
buffer = Buffer.concat([buffer, chunk])
94-
const index = buffer.indexOf('\r\n\r\n')
95-
if (index !== -1) {
96-
cleanUp()
97-
socket.pause()
98-
resolve({ head: buffer.subarray(0, index).toString(), rest: buffer.subarray(index + 4) })
99-
} else if (buffer.length > 16 * 1024) {
100-
cleanUp()
101-
reject(new Error('HTTP proxy response headers too large'))
102-
}
103-
}
104-
const onEnd = () => {
105-
cleanUp()
106-
reject(new Error('HTTP proxy closed the connection before answering'))
107-
}
108-
socket.on('data', onData)
109-
socket.once('end', onEnd)
110-
socket.once('error', reject)
111-
})
84+
const req = (isHttps ? httpsRequest : httpRequest)({
85+
agent: false,
86+
headers,
87+
// URL keeps the brackets around IPv6 addresses
88+
hostname: proxy.hostname.replace(/^\[(.*)\]$/, '$1'),
89+
method: 'CONNECT',
90+
path: target,
91+
port: proxy.port,
92+
rejectUnauthorized: this.#proxyRejectUnauthorized,
93+
timeout: this.#proxyTimeout,
94+
})
95+
// `timeout` only emits an event, the request must be aborted explicitly
96+
req.on('timeout', () => {
97+
const error = new Error(`HTTP proxy did not answer the CONNECT to ${target} in time`)
98+
error.code = 'NBD_PROXY_CONNECT_TIMEOUT'
99+
req.destroy(error)
100+
})
101+
req.end()
112102

113-
const statusLine = head.split('\r\n', 1)[0]
114-
const statusCode = Number(statusLine.split(' ')[1])
115-
if (statusCode !== 200) {
116-
const error = new Error(`HTTP proxy refused to connect to ${target}: ${statusLine}`)
117-
error.code = 'NBD_PROXY_CONNECT_FAILED'
118-
throw error
119-
}
120-
if (rest.length !== 0) {
121-
socket.unshift(rest)
122-
}
123-
return socket
124-
} catch (error) {
103+
// the NBD server may already have sent some data, it is in `head`
104+
const [res, socket, head] = await once(req, 'connect')
105+
// the tunnel is established, NBD has its own timeouts
106+
socket.setTimeout(0)
107+
if (res.statusCode !== 200) {
125108
socket.destroy()
109+
const error = new Error(`HTTP proxy refused to connect to ${target}: ${res.statusCode} ${res.statusMessage}`)
110+
error.code = 'NBD_PROXY_CONNECT_FAILED'
126111
throw error
127112
}
113+
if (head.length !== 0) {
114+
socket.unshift(head)
115+
}
116+
return socket
128117
}
129118

130119
// mandatory , at least to start the handshake: the connection always starts

‎@vates/nbd-client/NbdTcpClient.test.mjs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,4 +80,28 @@ describe('NbdTcpClient through an HTTP proxy', () => {
8080
await assert.rejects(client.connect(), { code: 'NBD_PROXY_CONNECT_FAILED' })
8181
await client.disconnect().catch(() => {})
8282
})
83+
84+
it('aborts the CONNECT when the proxy does not answer', async () => {
85+
// accepts the TCP connection but never answers
86+
const silentProxy = createTcpServer(() => {})
87+
silentProxy.listen(0, '127.0.0.1')
88+
await once(silentProxy, 'listening')
89+
try {
90+
const client = new NbdTcpClient({
91+
address: '127.0.0.1',
92+
port: nbdPort,
93+
httpProxy: `http://127.0.0.1:${silentProxy.address().port}`,
94+
proxyTimeout: 50,
95+
})
96+
await assert.rejects(client.connect(), { code: 'NBD_PROXY_CONNECT_TIMEOUT' })
97+
} finally {
98+
silentProxy.close()
99+
}
100+
})
101+
102+
it('rejects unsupported proxy protocols', () => {
103+
assert.throws(() => new NbdTcpClient({ address: '127.0.0.1', httpProxy: 'socks5://127.0.0.1:1080' }), {
104+
code: 'NBD_PROXY_UNSUPPORTED_PROTOCOL',
105+
})
106+
})
83107
})

0 commit comments

Comments
 (0)