From 6b114eb53762315d4eaeb2fcc37e690663059569 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Tue, 9 Jan 2024 09:34:48 -0700 Subject: [PATCH] specify ssh2 as dependency (#197) * specify ssh2 as dependency * reject when local signing returns non-zero exit code * remove unnecessary promsie * remove dependency on signing-utils * remove from package-lock * fix check in signing-utils --- package.json | 2 +- .../local-signing-client.spec.ts | 38 +++++++++++++ .../signing-clients/local-signing-client.ts | 33 ++++++++---- .../remote-signing-client.spec.ts | 53 +++---------------- .../signing-clients/remote-signing-client.ts | 45 ++-------------- packages/signing-utils/src/ssh-client.ts | 23 +++++++- 6 files changed, 96 insertions(+), 98 deletions(-) diff --git a/package.json b/package.json index 336d3428..d41a8a4c 100644 --- a/package.json +++ b/package.json @@ -51,4 +51,4 @@ "husky": "^8.0.3", "lerna": "^7.1.1" } -} \ No newline at end of file +} diff --git a/packages/signing-utils/src/signing-clients/local-signing-client.spec.ts b/packages/signing-utils/src/signing-clients/local-signing-client.spec.ts index 0f9c7b97..bcd13a71 100644 --- a/packages/signing-utils/src/signing-clients/local-signing-client.spec.ts +++ b/packages/signing-utils/src/signing-clients/local-signing-client.spec.ts @@ -41,4 +41,42 @@ describe('LocalSigningClient', function () { ).trim(); expect(signedFile).to.equal('signed content'); }); + + context('when the script returns a non-zero exit code', function () { + beforeEach(function () { + writeFileSync( + signingScript, + ` + #!/bin/bash + echo "Signing script called with arguments: $@" + >&2 echo "error - something went wrong" + exit 1 + ` + ); + }); + + it('sign() rejects', async function () { + const localSigningClient = new LocalSigningClient({ + signingScript: signingScript, + signingMethod: 'gpg', + }); + + const error = await localSigningClient.sign(fileToSign).catch((e) => e); + expect(error).to.be.instanceOf(Error); + }); + + it('includes the stdout and stderr of the failed script in the error', async function () { + const localSigningClient = new LocalSigningClient({ + signingScript: signingScript, + signingMethod: 'gpg', + }); + + const error: Error = await localSigningClient + .sign(fileToSign) + .catch((e) => e); + const { stdout, stderr } = JSON.parse(error.message); + expect(stdout).to.contain('Signing script called with arguments: '); + expect(stderr).to.equal('error - something went wrong\n'); + }); + }); }); diff --git a/packages/signing-utils/src/signing-clients/local-signing-client.ts b/packages/signing-utils/src/signing-clients/local-signing-client.ts index d007cf76..f62c5d7b 100644 --- a/packages/signing-utils/src/signing-clients/local-signing-client.ts +++ b/packages/signing-utils/src/signing-clients/local-signing-client.ts @@ -16,7 +16,10 @@ export class LocalSigningClient implements SigningClient { private options: Omit ) {} - sign(file: string): Promise { + // we want to wrap any errors in promise rejections, so even though there is no + // await statement, we use an `async` function + // eslint-disable-next-line @typescript-eslint/require-await + async sign(file: string): Promise { localClientDebug(`Signing ${file}`); const directoryOfFileToSign = path.dirname(file); @@ -27,15 +30,27 @@ export class LocalSigningClient implements SigningClient { method: this.options.signingMethod, }; - spawnSync('bash', [this.options.signingScript, path.basename(file)], { - cwd: directoryOfFileToSign, - env, - encoding: 'utf-8', - }); - + const { stdout, stderr, status } = spawnSync( + 'bash', + [this.options.signingScript, path.basename(file)], + { + cwd: directoryOfFileToSign, + env, + encoding: 'utf-8', + } + ); + + localClientDebug({ stdout, stderr }); + + if (status !== 0) { + throw new Error( + JSON.stringify({ + stdout, + stderr, + }) + ); + } localClientDebug(`Signed file ${file}`); - - return Promise.resolve(); } catch (error) { localClientDebug({ error }); throw error; diff --git a/packages/signing-utils/src/signing-clients/remote-signing-client.spec.ts b/packages/signing-utils/src/signing-clients/remote-signing-client.spec.ts index b8fd39f8..80d91d71 100644 --- a/packages/signing-utils/src/signing-clients/remote-signing-client.spec.ts +++ b/packages/signing-utils/src/signing-clients/remote-signing-client.spec.ts @@ -3,55 +3,16 @@ import { exec } from 'child_process'; import { RemoteSigningClient } from './remote-signing-client'; import { expect } from 'chai'; import type { SSHClient } from '../ssh-client'; +import { promisify } from 'util'; const getMockedSSHClient = () => { return { - getSftpConnection: () => { - return { - fastPut: async ( - localFile: string, - remoteFile: string, - cb: (err?: Error) => void - ) => { - try { - await fs.copyFile(localFile, remoteFile); - cb(); - } catch (err) { - cb(err as Error); - } - }, - fastGet: async ( - remoteFile: string, - localFile: string, - cb: (err?: Error) => void - ) => { - try { - await fs.copyFile(remoteFile, localFile); - cb(); - } catch (err) { - cb(err as Error); - } - }, - unlink: async (remoteFile: string, cb: (err?: Error) => void) => { - try { - await fs.unlink(remoteFile); - cb(); - } catch (err) { - cb(err as Error); - } - }, - }; - }, - exec: (command: string) => { - return new Promise((resolve, reject) => { - exec(command, { shell: 'bash' }, (err) => { - if (err) { - return reject(err); - } - return resolve('Ok'); - }); - }); - }, + // The mocked ssh client + copyFile: (from: string, to: string) => fs.copyFile(from, to), + downloadFile: (remote: string, local: string) => fs.copyFile(remote, local), + removeFile: fs.unlink.bind(fs.unlink), + exec: (command: string) => + promisify(exec)(command, { shell: 'bash' }).then(() => 'Ok'), disconnect: () => {}, } as unknown as SSHClient; }; diff --git a/packages/signing-utils/src/signing-clients/remote-signing-client.ts b/packages/signing-utils/src/signing-clients/remote-signing-client.ts index 02832434..99b3769a 100644 --- a/packages/signing-utils/src/signing-clients/remote-signing-client.ts +++ b/packages/signing-utils/src/signing-clients/remote-signing-client.ts @@ -1,12 +1,9 @@ import path from 'path'; -import type { SFTPWrapper } from 'ssh2'; import type { SSHClient } from '../ssh-client'; import { debug, getEnv } from '../utils'; import type { SigningClient, SigningClientOptions } from '.'; export class RemoteSigningClient implements SigningClient { - private sftpConnection!: SFTPWrapper; - constructor( private sshClient: SSHClient, private options: SigningClientOptions @@ -19,13 +16,12 @@ export class RemoteSigningClient implements SigningClient { * - Copy the signing script to the remote machine */ private async init() { - this.sftpConnection = await this.sshClient.getSftpConnection(); await this.sshClient.exec(`mkdir -p ${this.options.workingDirectory}`); // Copy the signing script to the remote machine { const remoteScript = `${this.options.workingDirectory}/garasign.sh`; - await this.copyFile(this.options.signingScript, remoteScript); + await this.sshClient.copyFile(this.options.signingScript, remoteScript); await this.sshClient.exec(`chmod +x ${remoteScript}`); } } @@ -36,39 +32,6 @@ export class RemoteSigningClient implements SigningClient { )}`; } - private async copyFile(file: string, remotePath: string): Promise { - return new Promise((resolve, reject) => { - this.sftpConnection.fastPut(file, remotePath, (err) => { - if (err) { - return reject(err); - } - return resolve(); - }); - }); - } - - private async downloadFile(remotePath: string, file: string): Promise { - return new Promise((resolve, reject) => { - this.sftpConnection.fastGet(remotePath, file, (err) => { - if (err) { - return reject(err); - } - return resolve(); - }); - }); - } - - private async removeFile(remotePath: string): Promise { - return new Promise((resolve, reject) => { - this.sftpConnection.unlink(remotePath, (err) => { - if (err) { - return reject(err); - } - return resolve(); - }); - }); - } - private async signRemoteFile(file: string) { const env = getEnv(); /** @@ -100,18 +63,18 @@ export class RemoteSigningClient implements SigningClient { // establish connection await this.init(); - await this.copyFile(file, remotePath); + await this.sshClient.copyFile(file, remotePath); debug(`SFTP: Copied file ${file} to ${remotePath}`); await this.signRemoteFile(path.basename(remotePath)); debug(`SFTP: Signed file ${file}`); - await this.downloadFile(remotePath, file); + await this.sshClient.downloadFile(remotePath, file); debug(`SFTP: Downloaded signed file to ${file}`); } catch (error) { debug({ error }); } finally { - await this.removeFile(remotePath); + await this.sshClient.removeFile(remotePath); debug(`SFTP: Removed remote file ${remotePath}`); this.sshClient.disconnect(); } diff --git a/packages/signing-utils/src/ssh-client.ts b/packages/signing-utils/src/ssh-client.ts index 1b3aeb5c..51b3bde6 100644 --- a/packages/signing-utils/src/ssh-client.ts +++ b/packages/signing-utils/src/ssh-client.ts @@ -79,7 +79,7 @@ export class SSHClient { return data; } - async getSftpConnection(): Promise { + private async getSftpConnection(): Promise { if (!this.connected) { throw new Error('Not connected to ssh server'); } @@ -89,4 +89,25 @@ export class SSHClient { (await promisify(this.sshConnection.sftp.bind(this.sshConnection))()); return this.sftpConnection; } + + async copyFile(file: string, remotePath: string): Promise { + const sftpConnection = await this.getSftpConnection(); + return promisify(sftpConnection.fastPut.bind(sftpConnection))( + file, + remotePath + ); + } + + async downloadFile(remotePath: string, file: string): Promise { + const sftpConnection = await this.getSftpConnection(); + return promisify(sftpConnection.fastGet.bind(sftpConnection))( + remotePath, + file + ); + } + + async removeFile(remotePath: string): Promise { + const sftpConnection = await this.getSftpConnection(); + return promisify(sftpConnection.unlink.bind(sftpConnection))(remotePath); + } }