POSIX client can't disconnect from version 1.x.x SFTP server

[sedenardi]

Description

When using a macOS or Linux client to connect to an SFTP server created using ssh2, the typical way to disconnect from the server is using the quit (or exit or bye) command. When the server uses the latest version of this library, 1.1.0, the client hangs after issuing the command (the program is never closed). Additionally, the end Connection event is never fired on the server.

When using versions of ssh2 prior to 1.x.x, issuing the exit command on the client exits the program and fires the end Connection event on the server.

Versions

Server: macOS 11.4, node v14.15.0
Tested clients: macOS 11.4, CentOS Linux 7
Tested library versions: 0.8.9 (pass), 1.0.0 (fail), 1.1.0 (fail)

Sample Code

These are arbitrary server implementations, based on the examples for each version's README, that support a posix client only connecting and then disconnecting. The client issues a cwd upon connecting, so REALPATH is the only server event implemented. For both, the following commands were run on both macOS and CentOS:

$ sftp -oPort=2222 foo@localhost
foo@localhost's password: ### enter 'bar' ###
Connected to localhost.
sftp> ### enter 'exit' ###

1.1.0 - Failing

const { timingSafeEqual } = require('crypto');
const { readFileSync } = require('fs');

const {
  Server,
  utils: {
    sftp: {
      STATUS_CODE
    }
  }
} = require('ssh2');

const allowedUser = Buffer.from('foo');
const allowedPassword = Buffer.from('bar');

function checkValue(input, allowed) {
  const autoReject = (input.length !== allowed.length);
  if (autoReject) {
    allowed = input;
  }
  const isMatch = timingSafeEqual(input, allowed);
  return (!autoReject && isMatch);
}

new Server({
  hostKeys: [readFileSync('host.key')]
}, (client) => {
  console.log('client connected');

  client.on('authentication', (ctx) => {
    let allowed = true;
    if (!checkValue(Buffer.from(ctx.username), allowedUser))
      allowed = false;

    switch (ctx.method) {
      case 'password':
        if (!checkValue(Buffer.from(ctx.password), allowedPassword))
          return ctx.reject();
        break;
      default:
        return ctx.reject();
    }

    if (allowed)
      ctx.accept();
    else
      ctx.reject();
  }).on('ready', () => {
    console.log('client ready');

    client.on('session', (accept) => {
      const session = accept();
      session.on('sftp', (accept) => {
        console.log('session sftp')
        const sftp = accept();
        sftp.on('REALPATH', (reqid, path) => {
          console.log('sftp REALPATH', path);
          sftp.name(reqid, [{
            filename: path,
            longname: 'drwxr-xr-x  56 foo foo      4096 Nov 10 01:05 .'
          }]);
        }).on('CLOSE', (reqid) => {
          console.log('sftp CLOSE');
          sftp.status(reqid, STATUS_CODE.OK);
        });
      });
    });
  }).on('close', () => {
    console.log('client close');
  }).on('error', (err) => {
    console.log('client error', err);
  });
}).listen(2222, 'localhost', function() {
  console.log('server listening on port ' + this.address().port);
});

Output:

server listening on port 2222
client connected
client ready
session sftp
sftp REALPATH .

0.8.9 - Passing

const fs = require('fs');
const crypto = require('crypto');

const ssh2 = require('ssh2');
const STATUS_CODE = ssh2.SFTP_STATUS_CODE;

const allowedUser = Buffer.from('foo');
const allowedPassword = Buffer.from('bar');

new ssh2.Server({
  hostKeys: [readFileSync('host.key')]
}, (client) => {
  console.log('client connected');

  client.on('authentication', (ctx) => {
    const user = Buffer.from(ctx.username);
    if (user.length !== allowedUser.length
        || !crypto.timingSafeEqual(user, allowedUser)) {
      return ctx.reject();
    }

    switch (ctx.method) {
      case 'password':
        const password = Buffer.from(ctx.password);
        if (password.length !== allowedPassword.length
            || !crypto.timingSafeEqual(password, allowedPassword)) {
          return ctx.reject();
        }
        break;
      default:
        return ctx.reject();
    }

    ctx.accept();
  }).on('ready', () => {
    console.log('client ready');

    client.on('session', (accept) => {
      const session = accept();
      session.on('sftp', (accept) => {
        console.log('session sftp')
        const sftpStream = accept();
        sftpStream.on('REALPATH', (reqid, path) => {
          console.log('sftp REALPATH', path);
          sftpStream.name(reqid, [{
            filename: path,
            longname: 'drwxr-xr-x  56 foo foo      4096 Nov 10 01:05 .'
          }]);
        }).on('CLOSE', (reqid) => {
          console.log('sftp CLOSE');
          sftpStream.status(reqid, STATUS_CODE.OK);
        });
      });
    });
  }).on('end', () => {
    console.log('client end');
  }).on('error', (err) => {
    console.log('client error', err);
  });
}).listen(2222, 'localhost', function() {
  console.log('server listening on port ' + this.address().port);
});

Output:

server listening on port 2222
client connected
client ready
session sftp
sftp REALPATH .
client end

[mscdex]

You're using 'close' on 1.x vs. 'end' on v0.8.9, does 'end' get emitted for you on 1.x?

[sedenardi]

You're using 'close' on 1.x vs. 'end' on v0.8.9, does 'end' get emitted for you on 1.x?

@mscdex No, 'end' does not get emitted on 1.x.

[sedenardi]

@mscdex Hiya, any idea what could be causing the issue? I'd be happy to look into it and even submit a PR, but I'm at a loss of where exactly to start looking.

[mscdex]

I don't know offhand, AFAICT from the code it should be at least emitting 'end'. I haven't had time to look into it.

[schantaraud]

Here's a workaround that seems to work for me:

session.on('close', () => {
  client.end();
});
session.on('end', () => {
  client.end();
});

Some clients trigger the first, some the second.

[mscdex]

@sedenardi Can you verify #1111 fixes the problem for you?

[trsvax]

had this problem on Mac sftp

session.on('close', () => {
client.end();
});
session.on('end', () => {
client.end();
});

fixed my problem

[sedenardi]

@sedenardi Can you verify #1111 fixes the problem for you?

Apologies for the long delay, I completely missed your reply.

Adding a listener for end on the session indeed allows the client to disconnect:

session.on('end', () => {
  client.end();
});

Adding a listener for close on the session doesn't do anything, as it never gets called.