From bede4623b9102fea60a3f9a57a240ad498ed8bc8 Mon Sep 17 00:00:00 2001 From: Clifton Barnes Date: Tue, 21 Dec 2021 14:47:23 -0500 Subject: [PATCH 1/7] Properly close connection on EOF --- lib/server.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/server.js b/lib/server.js index 4f6fd1f6..4af46a9a 100644 --- a/lib/server.js +++ b/lib/server.js @@ -207,6 +207,11 @@ class Session extends EventEmitter { state: 'open' } }; + + this.on('eof', () => { + if (this._channel) + this._channel.end(); + }); } } From 3f179a870f61e20f9d5ab94558e0b3f971b2f1cc Mon Sep 17 00:00:00 2001 From: Clifton Barnes Date: Wed, 5 Jan 2022 11:23:46 -0500 Subject: [PATCH 2/7] Restrict channel close to SFTP instances --- lib/server.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/server.js b/lib/server.js index 4af46a9a..069a9d57 100644 --- a/lib/server.js +++ b/lib/server.js @@ -209,7 +209,7 @@ class Session extends EventEmitter { }; this.on('eof', () => { - if (this._channel) + if (this._channel instanceof SFTP) this._channel.end(); }); } From de31b0059f591979128e8bcb868698fb1615085f Mon Sep 17 00:00:00 2001 From: Clifton Barnes Date: Mon, 24 Jan 2022 12:00:50 -0500 Subject: [PATCH 3/7] Add OpenSSH sftp test --- test/test-integration-openssh-sftp.js | 247 ++++++++++++++++++++++++++ 1 file changed, 247 insertions(+) create mode 100644 test/test-integration-openssh-sftp.js diff --git a/test/test-integration-openssh-sftp.js b/test/test-integration-openssh-sftp.js new file mode 100644 index 00000000..957543ee --- /dev/null +++ b/test/test-integration-openssh-sftp.js @@ -0,0 +1,247 @@ +'use strict'; + +const assert = require('assert'); +const { spawn, spawnSync } = require('child_process'); +const readline = require('readline'); + +const Server = require('../lib/server.js'); + +const { + fixture, + fixtureKey, + mustCall, + mustCallAtLeast, +} = require('./common.js'); + +const SPAWN_OPTS = { windowsHide: true }; +const CLIENT_TIMEOUT = 5000; + +const debug = false; +const opensshPath = 'ssh'; +const sftpPath = 'sftp'; +let opensshVer; + +{ + // Get OpenSSH client version first + const { + error, stderr, stdout + } = spawnSync(opensshPath, ['-V'], SPAWN_OPTS); + + if (error) { + console.error('OpenSSH client is required for these tests'); + process.exitCode = 5; + return; + } + + const re = /^OpenSSH_([\d.]+)/; + let m = re.exec(stdout.toString()); + if (!m || !m[1]) { + m = re.exec(stderr.toString()); + if (!m || !m[1]) { + console.error('OpenSSH client is required for these tests'); + process.exitCode = 5; + return; + } + } + + opensshVer = m[1]; + console.log(`Testing with OpenSSH version: ${opensshVer}`); +} + +{ + const clientKey = fixtureKey('openssh_new_rsa'); + const username = 'KeyUser'; + const { server } = setup( + 'Client closes the connection', + { + client: { + username, + privateKeyPath: clientKey.fullPath, + }, + server: { hostKeys: [ fixture('ssh_host_rsa_key') ] }, + debug, + } + ); + + server.on('_child', mustCall((childProc) => { + childProc.stdin.write('bye\r'); + childProc.stdin.end(); + })).on('connection', mustCall((conn) => { + let authAttempt = 0; + conn.on('authentication', mustCallAtLeast((ctx) => { + assert(ctx.username === username, + `Wrong username: ${ctx.username}`); + switch (++authAttempt) { + case 1: + assert(ctx.method === 'none', + `Wrong auth method: ${ctx.method}`); + return ctx.reject(); + case 2: + case 3: + if (authAttempt === 3) + assert(ctx.signature, 'Missing publickey signature'); + assert(ctx.method === 'publickey', + `Wrong auth method: ${ctx.method}`); + assert(ctx.key.algo === clientKey.key.type, + `Wrong key algo: ${ctx.key.algo}`); + assert.deepStrictEqual(clientKey.key.getPublicSSH(), + ctx.key.data, + 'Public key mismatch'); + break; + default: + assert(false, 'Unexpected number of auth attempts'); + } + if (ctx.signature) { + assert(clientKey.key.verify(ctx.blob, ctx.signature) === true, + 'Could not verify publickey signature'); + // We should not expect any further auth attempts after we verify a + // signature + authAttempt = Infinity; + } + ctx.accept(); + }, 2)).on('ready', mustCall(() => { + conn.on('session', mustCall((accept, reject) => { + accept().on('sftp', mustCall((accept, reject) => { + const sftp = accept(); + sftp.on('REALPATH', mustCall((id, path) => { + assert(path === '.'); + sftp.name(id, { filename: '/' }); + })).on('end', mustCall(() => { + })); + })); + })); + })); + })); +} + +function setup(title, configs) { + const { + client: clientCfg, + server: serverCfg, + allReady: allReady_, + timeout: timeout_, + debug, + noForceServerReady, + } = configs; + let clientClose = false; + let serverClose = false; + let serverReady = false; + let client; + const msg = (text) => { + return `${title}: ${text}`; + }; + + const timeout = (typeof timeout_ === 'number' + ? timeout_ + : CLIENT_TIMEOUT); + + const allReady = (typeof allReady_ === 'function' ? allReady_ : undefined); + + if (debug) { + serverCfg.debug = (...args) => { + console.log(`[${title}][SERVER]`, ...args); + }; + } + + const serverReadyFn = (noForceServerReady ? onReady : mustCall(onReady)); + const server = new Server(serverCfg); + + server.on('error', onError) + .on('connection', mustCall((conn) => { + conn.on('error', onError) + .on('ready', serverReadyFn); + server.close(); + })) + .on('close', mustCall(onClose)); + + function onError(err) { + const which = (arguments.length >= 3 ? 'client' : 'server'); + assert(false, msg(`Unexpected ${which} error: ${err}`)); + } + + function onReady() { + assert(!serverReady, msg('Received multiple ready events for server')); + serverReady = true; + allReady && allReady(); + } + + function onClose() { + if (arguments.length >= 3) { + assert(!clientClose, msg('Received multiple close events for client')); + clientClose = true; + } else { + assert(!serverClose, msg('Received multiple close events for server')); + serverClose = true; + } + } + + process.nextTick(mustCall(() => { + server.listen(0, 'localhost', mustCall(() => { + const args = [ + '-o', 'UserKnownHostsFile=/dev/null', + '-o', 'StrictHostKeyChecking=no', + '-o', 'CheckHostIP=no', + '-o', 'ConnectTimeout=3', + '-o', 'GlobalKnownHostsFile=/dev/null', + '-o', 'GSSAPIAuthentication=no', + '-o', 'IdentitiesOnly=yes', + '-o', 'BatchMode=yes', + '-o', 'VerifyHostKeyDNS=no', + + '-vvvvv', + '-o', 'KbdInteractiveAuthentication=no', + '-o', 'HostbasedAuthentication=no', + '-o', 'PasswordAuthentication=no', + '-o', 'PubkeyAuthentication=yes', + '-o', 'PreferredAuthentications=publickey' + ]; + + if (clientCfg.privateKeyPath) + args.push('-o', `IdentityFile=${clientCfg.privateKeyPath}`); + + if (!/^[0-6]\./.test(opensshVer)) { + // OpenSSH 7.0+ disables DSS/DSA host (and user) key support by + // default, so we explicitly enable it here + args.push('-o', 'HostKeyAlgorithms=+ssh-dss'); + args.push('-o', 'PubkeyAcceptedKeyTypes=+ssh-dss'); + } + + args.push('-P', server.address().port.toString(), + `${clientCfg.username}@localhost`); + + client = spawn(sftpPath, args, SPAWN_OPTS); + server.emit('_child', client); + + if (debug) { + readline.createInterface({ + input: client.stdout + }).on('line', (line) => { + console.log(`[${title}][CLIENT][STDOUT]`, line); + }); + readline.createInterface({ + input: client.stderr + }).on('line', (line) => { + console.error(`[${title}][CLIENT][STDERR]`, line); + }); + } else { + client.stdout.resume(); + client.stderr.resume(); + } + + client.on('error', (err) => { + onError(err, null, null); + }).on('exit', (code) => { + clearTimeout(client.timer); + if (code !== 0) + return onError(new Error(`Non-zero exit code ${code}`), null, null); + onClose(null, null, null); + }); + + client.timer = setTimeout(() => { + assert(false, msg('Client timeout')); + }, timeout); + })); + })); + + return { server }; +} From 3cb7eb4ea0ac4221f1cbc4779c24276b03a2a6d5 Mon Sep 17 00:00:00 2001 From: Clifton Barnes Date: Fri, 28 Jan 2022 09:04:43 -0500 Subject: [PATCH 4/7] Skip integration tests on Windows --- test/test-integration-openssh-sftp.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/test-integration-openssh-sftp.js b/test/test-integration-openssh-sftp.js index 957543ee..2de1b09e 100644 --- a/test/test-integration-openssh-sftp.js +++ b/test/test-integration-openssh-sftp.js @@ -21,6 +21,12 @@ const opensshPath = 'ssh'; const sftpPath = 'sftp'; let opensshVer; +// TODO: figure out why this test is failing on Windows +if (process.platform === 'win32') { + console.log('Skipping OpenSSH integration tests on Windows'); + process.exit(0); +} + { // Get OpenSSH client version first const { From ef839124715208a0b2c27f5eb0a52a171c7fee9d Mon Sep 17 00:00:00 2001 From: Clifton Barnes Date: Mon, 31 Jan 2022 09:45:40 -0500 Subject: [PATCH 5/7] Remove sftp integration test --- test/test-integration-openssh-sftp.js | 253 -------------------------- 1 file changed, 253 deletions(-) delete mode 100644 test/test-integration-openssh-sftp.js diff --git a/test/test-integration-openssh-sftp.js b/test/test-integration-openssh-sftp.js deleted file mode 100644 index 2de1b09e..00000000 --- a/test/test-integration-openssh-sftp.js +++ /dev/null @@ -1,253 +0,0 @@ -'use strict'; - -const assert = require('assert'); -const { spawn, spawnSync } = require('child_process'); -const readline = require('readline'); - -const Server = require('../lib/server.js'); - -const { - fixture, - fixtureKey, - mustCall, - mustCallAtLeast, -} = require('./common.js'); - -const SPAWN_OPTS = { windowsHide: true }; -const CLIENT_TIMEOUT = 5000; - -const debug = false; -const opensshPath = 'ssh'; -const sftpPath = 'sftp'; -let opensshVer; - -// TODO: figure out why this test is failing on Windows -if (process.platform === 'win32') { - console.log('Skipping OpenSSH integration tests on Windows'); - process.exit(0); -} - -{ - // Get OpenSSH client version first - const { - error, stderr, stdout - } = spawnSync(opensshPath, ['-V'], SPAWN_OPTS); - - if (error) { - console.error('OpenSSH client is required for these tests'); - process.exitCode = 5; - return; - } - - const re = /^OpenSSH_([\d.]+)/; - let m = re.exec(stdout.toString()); - if (!m || !m[1]) { - m = re.exec(stderr.toString()); - if (!m || !m[1]) { - console.error('OpenSSH client is required for these tests'); - process.exitCode = 5; - return; - } - } - - opensshVer = m[1]; - console.log(`Testing with OpenSSH version: ${opensshVer}`); -} - -{ - const clientKey = fixtureKey('openssh_new_rsa'); - const username = 'KeyUser'; - const { server } = setup( - 'Client closes the connection', - { - client: { - username, - privateKeyPath: clientKey.fullPath, - }, - server: { hostKeys: [ fixture('ssh_host_rsa_key') ] }, - debug, - } - ); - - server.on('_child', mustCall((childProc) => { - childProc.stdin.write('bye\r'); - childProc.stdin.end(); - })).on('connection', mustCall((conn) => { - let authAttempt = 0; - conn.on('authentication', mustCallAtLeast((ctx) => { - assert(ctx.username === username, - `Wrong username: ${ctx.username}`); - switch (++authAttempt) { - case 1: - assert(ctx.method === 'none', - `Wrong auth method: ${ctx.method}`); - return ctx.reject(); - case 2: - case 3: - if (authAttempt === 3) - assert(ctx.signature, 'Missing publickey signature'); - assert(ctx.method === 'publickey', - `Wrong auth method: ${ctx.method}`); - assert(ctx.key.algo === clientKey.key.type, - `Wrong key algo: ${ctx.key.algo}`); - assert.deepStrictEqual(clientKey.key.getPublicSSH(), - ctx.key.data, - 'Public key mismatch'); - break; - default: - assert(false, 'Unexpected number of auth attempts'); - } - if (ctx.signature) { - assert(clientKey.key.verify(ctx.blob, ctx.signature) === true, - 'Could not verify publickey signature'); - // We should not expect any further auth attempts after we verify a - // signature - authAttempt = Infinity; - } - ctx.accept(); - }, 2)).on('ready', mustCall(() => { - conn.on('session', mustCall((accept, reject) => { - accept().on('sftp', mustCall((accept, reject) => { - const sftp = accept(); - sftp.on('REALPATH', mustCall((id, path) => { - assert(path === '.'); - sftp.name(id, { filename: '/' }); - })).on('end', mustCall(() => { - })); - })); - })); - })); - })); -} - -function setup(title, configs) { - const { - client: clientCfg, - server: serverCfg, - allReady: allReady_, - timeout: timeout_, - debug, - noForceServerReady, - } = configs; - let clientClose = false; - let serverClose = false; - let serverReady = false; - let client; - const msg = (text) => { - return `${title}: ${text}`; - }; - - const timeout = (typeof timeout_ === 'number' - ? timeout_ - : CLIENT_TIMEOUT); - - const allReady = (typeof allReady_ === 'function' ? allReady_ : undefined); - - if (debug) { - serverCfg.debug = (...args) => { - console.log(`[${title}][SERVER]`, ...args); - }; - } - - const serverReadyFn = (noForceServerReady ? onReady : mustCall(onReady)); - const server = new Server(serverCfg); - - server.on('error', onError) - .on('connection', mustCall((conn) => { - conn.on('error', onError) - .on('ready', serverReadyFn); - server.close(); - })) - .on('close', mustCall(onClose)); - - function onError(err) { - const which = (arguments.length >= 3 ? 'client' : 'server'); - assert(false, msg(`Unexpected ${which} error: ${err}`)); - } - - function onReady() { - assert(!serverReady, msg('Received multiple ready events for server')); - serverReady = true; - allReady && allReady(); - } - - function onClose() { - if (arguments.length >= 3) { - assert(!clientClose, msg('Received multiple close events for client')); - clientClose = true; - } else { - assert(!serverClose, msg('Received multiple close events for server')); - serverClose = true; - } - } - - process.nextTick(mustCall(() => { - server.listen(0, 'localhost', mustCall(() => { - const args = [ - '-o', 'UserKnownHostsFile=/dev/null', - '-o', 'StrictHostKeyChecking=no', - '-o', 'CheckHostIP=no', - '-o', 'ConnectTimeout=3', - '-o', 'GlobalKnownHostsFile=/dev/null', - '-o', 'GSSAPIAuthentication=no', - '-o', 'IdentitiesOnly=yes', - '-o', 'BatchMode=yes', - '-o', 'VerifyHostKeyDNS=no', - - '-vvvvv', - '-o', 'KbdInteractiveAuthentication=no', - '-o', 'HostbasedAuthentication=no', - '-o', 'PasswordAuthentication=no', - '-o', 'PubkeyAuthentication=yes', - '-o', 'PreferredAuthentications=publickey' - ]; - - if (clientCfg.privateKeyPath) - args.push('-o', `IdentityFile=${clientCfg.privateKeyPath}`); - - if (!/^[0-6]\./.test(opensshVer)) { - // OpenSSH 7.0+ disables DSS/DSA host (and user) key support by - // default, so we explicitly enable it here - args.push('-o', 'HostKeyAlgorithms=+ssh-dss'); - args.push('-o', 'PubkeyAcceptedKeyTypes=+ssh-dss'); - } - - args.push('-P', server.address().port.toString(), - `${clientCfg.username}@localhost`); - - client = spawn(sftpPath, args, SPAWN_OPTS); - server.emit('_child', client); - - if (debug) { - readline.createInterface({ - input: client.stdout - }).on('line', (line) => { - console.log(`[${title}][CLIENT][STDOUT]`, line); - }); - readline.createInterface({ - input: client.stderr - }).on('line', (line) => { - console.error(`[${title}][CLIENT][STDERR]`, line); - }); - } else { - client.stdout.resume(); - client.stderr.resume(); - } - - client.on('error', (err) => { - onError(err, null, null); - }).on('exit', (code) => { - clearTimeout(client.timer); - if (code !== 0) - return onError(new Error(`Non-zero exit code ${code}`), null, null); - onClose(null, null, null); - }); - - client.timer = setTimeout(() => { - assert(false, msg('Client timeout')); - }, timeout); - })); - })); - - return { server }; -} From a4a4058de675c74990a5327a5ca20ea5983d7405 Mon Sep 17 00:00:00 2001 From: Clifton Barnes Date: Mon, 31 Jan 2022 09:51:46 -0500 Subject: [PATCH 6/7] Add explanation for EOF handling --- lib/server.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/server.js b/lib/server.js index 069a9d57..21b08f7b 100644 --- a/lib/server.js +++ b/lib/server.js @@ -208,6 +208,9 @@ class Session extends EventEmitter { } }; + // This is necessary to properly terminate the connection for some + // clients (ex: OpenSSH sftp) that send EOF when requesting to close the + // connection. this.on('eof', () => { if (this._channel instanceof SFTP) this._channel.end(); From 56299f5943c7f9bcfa82926cc5e7279b6b64f796 Mon Sep 17 00:00:00 2001 From: Clifton Barnes Date: Mon, 31 Jan 2022 11:06:31 -0500 Subject: [PATCH 7/7] Add test for client sending EOF --- test/test-sftp.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/test-sftp.js b/test/test-sftp.js index 50e56f5d..72ba8511 100644 --- a/test/test-sftp.js +++ b/test/test-sftp.js @@ -46,6 +46,11 @@ setup('close', mustCall((client, server) => { })); })); +setup('eof', mustCall((client, server) => { + client.on('close', mustCall(() => {})); + client._protocol.channelEOF(client.outgoing.id); +})); + setup('read', mustCall((client, server) => { const expected = Buffer.from('node.jsnode.jsnode.jsnode.jsnode.jsnode.js'); const handle_ = Buffer.from('node.js');