From 7c15ec9341e35c124751ce224322120f66fa7ca1 Mon Sep 17 00:00:00 2001 From: Trevor Johnston Date: Fri, 22 Jan 2016 12:11:41 -0500 Subject: [PATCH 1/5] always reject with objects --- src/cloud/social/provider.ts | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/src/cloud/social/provider.ts b/src/cloud/social/provider.ts index 56e57cc..df319a4 100644 --- a/src/cloud/social/provider.ts +++ b/src/cloud/social/provider.ts @@ -367,11 +367,15 @@ export class CloudSocialProvider { public inviteUser = (clientId: string): Promise => { log.debug('inviteUser'); if (!(clientId in this.savedContacts_)) { - return Promise.reject(new Error('unknown cloud instance ' + clientId)); + return Promise.reject({ + message: 'unknown cloud instance ' + clientId + }); } if (this.savedContacts_[clientId].invite.user !== ADMIN_USERNAME) { - return Promise.reject(new Error('user is logged in as non-admin user ' + - this.savedContacts_[clientId].invite.user)); + return Promise.reject({ + message: 'user is logged in as non-admin user ' + + this.savedContacts_[clientId].invite.user + }); } return this.reconnect_(this.savedContacts_[clientId].invite).then( (connection: Connection) => { @@ -436,7 +440,9 @@ class Connection { // TODO: timeout public connect = (): Promise => { if (this.state_ !== ConnectionState.NEW) { - return Promise.reject(new Error('can only connect in NEW state')); + return Promise.reject({ + message: 'can only connect in NEW state' + }); } this.state_ = ConnectionState.CONNECTING; @@ -465,7 +471,9 @@ class Connection { ZORK_HOST, ZORK_PORT, (e: Error, stream: ssh2.Channel) => { if (e) { this.close(); - R(new Error('error establishing tunnel: ' + e.toString())); + R({ + message: 'error establishing tunnel: ' + e.message + }); return; } this.setState_(ConnectionState.WAITING_FOR_PING); @@ -482,8 +490,9 @@ class Connection { F(); } else { this.close(); - R(new Error('did not receive ping from server on login: ' + - reply)); + R({ + message: 'did not receive ping from server on login: ' + reply + }); } break; case ConnectionState.ESTABLISHED: @@ -510,7 +519,9 @@ class Connection { // TODO: does this occur outside of startup, i.e. should it always reject? log.warn('%1: tunnel error: %2', this.name_, e); this.close(); - R(new Error('could not establish tunnel: ' + e.toString())); + R({ + message: 'could not establish tunnel: ' + e.message + }); }).on('end', () => { // Occurs when the stream is "over" for any reason, including // failed connection. @@ -531,7 +542,9 @@ class Connection { // TODO: does this occur outside of startup, i.e. should it always reject? log.warn('%1: connection error: %2', this.name_, e); this.close(); - R(new Error('could not login: ' + e.toString())); + R({ + message: 'could not login: ' + e.message + }); }).on('end', () => { // Occurs when the connection is "over" for any reason, including // failed connection. From 90e9b4419cd1d312fa659c5a55a81ff5342c66e8 Mon Sep 17 00:00:00 2001 From: Trevor Johnston Date: Fri, 22 Jan 2016 12:14:25 -0500 Subject: [PATCH 2/5] add a timeout to the cloud social provider SSH connection --- src/cloud/social/provider.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/cloud/social/provider.ts b/src/cloud/social/provider.ts index df319a4..a60666c 100644 --- a/src/cloud/social/provider.ts +++ b/src/cloud/social/provider.ts @@ -26,6 +26,9 @@ const STORAGE_KEY = 'cloud-social-contacts'; const ADMIN_USERNAME = 'giver'; const REGULAR_USERNAME = 'getter'; +// Timeout for establishing an SSH connection. +const CONNECT_TIMEOUT_MS = 10000; + // Credentials for accessing a cloud instance. // The serialised, base64 form is distributed amongst users. // TODO: add (private) keys, for key-based auth @@ -450,6 +453,7 @@ class Connection { host: this.invite_.host, port: SSH_SERVER_PORT, username: this.invite_.user, + readyTimeout: CONNECT_TIMEOUT_MS, // Remaining fields only for type-correctness. tryKeyboard: false, debug: undefined @@ -464,6 +468,7 @@ class Connection { return new Promise((F, R) => { this.client_.on('ready', () => { + // TODO: set a timeout here, too this.setState_(ConnectionState.ESTABLISHING_TUNNEL); this.client_.forwardOut( // TODO: since we communicate using the stream, what does this mean? From df26c23f221d36d07308bbda0d4b058e27e45c99 Mon Sep 17 00:00:00 2001 From: Trevor Johnston Date: Fri, 22 Jan 2016 15:17:06 -0500 Subject: [PATCH 3/5] numerous fixes to ssh2 use, plus some renaming --- src/cloud/social/provider.ts | 92 +++++++++++++++--------------------- 1 file changed, 39 insertions(+), 53 deletions(-) diff --git a/src/cloud/social/provider.ts b/src/cloud/social/provider.ts index a60666c..2bed2eb 100644 --- a/src/cloud/social/provider.ts +++ b/src/cloud/social/provider.ts @@ -203,7 +203,7 @@ export class CloudSocialProvider { if (invite.host in this.clients_) { log.debug('closing old connection to %1', invite.host); this.clients_[invite.host].then((connection: Connection) => { - connection.close(); + connection.end(); }); } @@ -353,7 +353,7 @@ export class CloudSocialProvider { log.debug('logout'); for (let address in this.clients_) { this.clients_[address].then((connection: Connection) => { - connection.close(); + connection.end(); }); } return Promise.resolve(); @@ -430,10 +430,10 @@ class Connection { private state_ = ConnectionState.NEW; - private client_ = new Client(); + private connection_ = new Client(); // The tunneled connection, i.e. secure link to Zork. - private stream_ :ssh2.Channel; + private tunnel_ :ssh2.Channel; constructor( private invite_: Invite, @@ -467,23 +467,22 @@ class Connection { } return new Promise((F, R) => { - this.client_.on('ready', () => { + this.connection_.on('ready', () => { // TODO: set a timeout here, too this.setState_(ConnectionState.ESTABLISHING_TUNNEL); - this.client_.forwardOut( + this.connection_.forwardOut( // TODO: since we communicate using the stream, what does this mean? '127.0.0.1', 0, - ZORK_HOST, ZORK_PORT, (e: Error, stream: ssh2.Channel) => { + ZORK_HOST, ZORK_PORT, (e: Error, tunnel: ssh2.Channel) => { if (e) { - this.close(); + this.end(); R({ message: 'error establishing tunnel: ' + e.message }); return; } - this.setState_(ConnectionState.WAITING_FOR_PING); - this.stream_ = stream; + this.setState_(ConnectionState.WAITING_FOR_PING); var bufferQueue = new queue.Queue(); new linefeeder.LineFeeder(bufferQueue).setSyncHandler((reply: string) => { @@ -494,7 +493,7 @@ class Connection { this.setState_(ConnectionState.ESTABLISHED); F(); } else { - this.close(); + this.end(); R({ message: 'did not receive ping from server on login: ' + reply }); @@ -511,54 +510,36 @@ class Connection { default: log.warn('%1: did not expect message in state %2: %3', this.name_, ConnectionState[this.state_], reply); - this.close(); + this.end(); } }); - // TODO: add error handler for stream - stream.on('data', (buffer: Buffer) => { + this.tunnel_ = tunnel; + tunnel.on('data', (buffer: Buffer) => { bufferQueue.handle(arraybuffers.bufferToArrayBuffer(buffer)); - }).on('error', (e: Error) => { - // This occurs when: - // - host cannot be reached, e.g. non-existant hostname - // TODO: does this occur outside of startup, i.e. should it always reject? - log.warn('%1: tunnel error: %2', this.name_, e); - this.close(); - R({ - message: 'could not establish tunnel: ' + e.message - }); }).on('end', () => { - // Occurs when the stream is "over" for any reason, including - // failed connection. log.debug('%1: tunnel end', this.name_); - this.close(); }).on('close', (hadError: boolean) => { - // TODO: when does this occur? don't see it on normal close or failure - log.debug('%1: tunnel close: %2', this.name_, hadError); - this.close(); + log.debug('%1: tunnel close, with%2 error', this.name_, (hadError ? '' : 'out')); }); - stream.write('ping\n'); + tunnel.write('ping\n'); }); }).on('error', (e: Error) => { // This occurs when: // - user supplies the wrong username or password // - host cannot be reached, e.g. non-existant hostname - // TODO: does this occur outside of startup, i.e. should it always reject? log.warn('%1: connection error: %2', this.name_, e); - this.close(); + this.setState_(ConnectionState.TERMINATED); R({ message: 'could not login: ' + e.message }); }).on('end', () => { - // Occurs when the connection is "over" for any reason, including - // failed connection. - log.debug('%1: connection ended', this.name_); - this.close(); + log.debug('%1: connection end', this.name_); + this.setState_(ConnectionState.TERMINATED); }).on('close', (hadError: boolean) => { - // TODO: when does this occur? don't see it on normal close or failure - log.debug('%1: connection close: %2', this.name_, hadError); - this.close(); + log.debug('%1: connection close, with%1 error', this.name_, (hadError ? '' : 'out')); + this.setState_(ConnectionState.TERMINATED); }).connect(connectConfig); }); } @@ -567,17 +548,14 @@ class Connection { if (this.state_ !== ConnectionState.ESTABLISHED) { throw new Error('can only connect in ESTABLISHED state'); } - this.stream_.write(s + '\n'); + this.tunnel_.write(s + '\n'); } - public close = (): void => { + public end = (): void => { log.debug('%1: close', this.name_); - if (this.state_ === ConnectionState.TERMINATED) { - log.debug('%1: already closed', this.name_); - } else { + if (this.state_ !== ConnectionState.TERMINATED) { this.setState_(ConnectionState.TERMINATED); - this.client_.end(); - // TODO: what about the stream? + this.connection_.end(); } } @@ -596,21 +574,29 @@ class Connection { // Executes a command, fulfilling with the command's stdout // or rejecting if output is received on stderr. private exec_ = (command:string): Promise => { + log.debug('%1: execute command: %2', this.name_, command); if (this.state_ !== ConnectionState.ESTABLISHED) { return Promise.reject(new Error('can only execute commands in ESTABLISHED state')); } return new Promise((F, R) => { - this.client_.exec(command, (e: Error, stream: ssh2.Channel) => { + this.connection_.exec(command, (e: Error, stream: ssh2.Channel) => { if (e) { - R(e); + R({ + message: 'failed to execute command: ' + e.message + }); return; } - stream.on('data', function(data: Buffer) { + + // TODO: There is a close event with a return code which + // is probably a better indication of success. + stream.on('data', (data: Buffer) => { F(data.toString()); - }).stderr.on('data', function(data: Buffer) { - R(new Error(data.toString())); - }).on('close', function(code: any, signal: any) { - log.debug('exec stream closed'); + }).stderr.on('data', (data: Buffer) => { + R({ + message: 'command output to STDERR: ' + data.toString() + }); + }).on('end', (code: any, signal: any) => { + log.debug('%1: exec stream end', this.name_); }); }); }); From fbbd4f62ca3f7f8114955aac368fe3cd96b4083f Mon Sep 17 00:00:00 2001 From: Trevor Johnston Date: Fri, 22 Jan 2016 15:34:56 -0500 Subject: [PATCH 4/5] right args for stream end --- src/cloud/social/provider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cloud/social/provider.ts b/src/cloud/social/provider.ts index 2bed2eb..78cc5d2 100644 --- a/src/cloud/social/provider.ts +++ b/src/cloud/social/provider.ts @@ -595,7 +595,7 @@ class Connection { R({ message: 'command output to STDERR: ' + data.toString() }); - }).on('end', (code: any, signal: any) => { + }).on('end', () => { log.debug('%1: exec stream end', this.name_); }); }); From c944fb93ab3b9c210cf43d5a43656f6355d12224 Mon Sep 17 00:00:00 2001 From: Trevor Johnston Date: Wed, 27 Jan 2016 12:29:06 -0500 Subject: [PATCH 5/5] fix log line --- src/cloud/social/provider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cloud/social/provider.ts b/src/cloud/social/provider.ts index 78cc5d2..596d6fc 100644 --- a/src/cloud/social/provider.ts +++ b/src/cloud/social/provider.ts @@ -538,7 +538,7 @@ class Connection { log.debug('%1: connection end', this.name_); this.setState_(ConnectionState.TERMINATED); }).on('close', (hadError: boolean) => { - log.debug('%1: connection close, with%1 error', this.name_, (hadError ? '' : 'out')); + log.debug('%1: connection close, with%2 error', this.name_, (hadError ? '' : 'out')); this.setState_(ConnectionState.TERMINATED); }).connect(connectConfig); });