Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/express/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ BitGo Express currently supports the following proxy protocols:

BitGo Express can be run in an external signing mode, where the signing of transactions is performed in a separate instance of BitGo Express that has access to the private keys. This may be preferable for users who would like to apply their signature to their transactions using a more secure environment than BitGo SDK or BitGo Express, such as a signer with restricted access or network connectivity.

To set up BitGo Express with an external signer, a url to the external signer instance of BitGo Express must be provided using the `externalSignerUrl` configuration option. The corresponding external signer instance of BitGo Express must have `signerMode` set, and `signerFileSystemPath` set to the path of a json containing the private key.
To set up BitGo Express with an external signer, a url to the external signer instance of BitGo Express must be provided using the `externalSignerUrl` configuration option. The corresponding external signer instance of BitGo Express must have `signerMode` set, and `signerFileSystemPath` set to the path of a json containing the private key. Note that if BitGo Express encounters an `ECONNREFUSED` error when requesting the external signer for a signature, it will retry the request for up to 15 seconds.

The JSON file containing the unencrypted private key(s) must be in the format `"<walletId>": "<privateKey>"` wallet ID and private key. Here is an example json file containing two wallet IDs and their corresponding private keys.

Expand Down
4 changes: 2 additions & 2 deletions modules/express/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"@types/morgan": "^1.7.35",
"@types/nock": "^9.3.1",
"@types/node": "^11.11.4",
"@types/sinon": "^7.0.6",
"@types/sinon": "^10.0.11",
"@types/supertest": "^2.0.11",
"bignumber.js": "^8.0.1",
"lint-staged": "^9.2.0",
Expand All @@ -68,7 +68,7 @@
"should": "^13.2.3",
"should-http": "^0.1.1",
"should-sinon": "^0.0.6",
"sinon": "^6.3.5",
"sinon": "^13.0.1",
"supertest": "^4.0.2",
"supertest-as-promised": "https://github.com/BitGo/supertest-as-promised/archive/a7f4b612b9fa090ae33a9616c41862aec2b25c7e.tar.gz"
},
Expand Down
16 changes: 11 additions & 5 deletions modules/express/src/clientRoutes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { Config } from './config';
import { ApiResponseError } from './errors';
import { promises as fs } from 'fs';
import * as assert from 'assert';
import { retryPromise } from './retryPromise';

const { version } = require('bitgo/package.json');
const pjson = require('../package.json');
Expand Down Expand Up @@ -1025,11 +1026,16 @@ export function createCustomSigningFunction(externalSignerUrl: string): CustomSi
txPrebuild: TransactionPrebuild;
pubs?: string[];
}): Promise<SignedTransaction> {
const { body: signedTx } = await superagent
.post(`${externalSignerUrl}/api/v2/${params.coin.getChain()}/sign`)
.type('json')
.send({ txPrebuild: params.txPrebuild, pubs: params.pubs });

const { body: signedTx } = await retryPromise(
() =>
superagent
.post(`${externalSignerUrl}/api/v2/${params.coin.getChain()}/sign`)
.type('json')
.send({ txPrebuild: params.txPrebuild, pubs: params.pubs }),
(err, tryCount) => {
console.error(`attempt number ${tryCount}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make this a debug instead and make it a little more informative

Suggested change
console.error(`attempt number ${tryCount}`);
debug(`failed to connect to external signer (attempt ${tryCount}, error: ${err.message})`);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to #1989

}
);
return signedTx;
};
}
48 changes: 48 additions & 0 deletions modules/express/src/retryPromise.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* Thrown in `retryPromise()`
*
* @prettier
*/

export class ErrorMaxRetriesExceededError extends Error {
constructor(maxTries: number) {
super(`giving up after reaching max retry limit of ${maxTries}`);
}
}

/**
* Retries a promise (like a request) if it returns with 'ECONNREFUSED'. Retries are delayed with an exponential backoff.
* @param {Function} func - Promise to execute. When it throws an error, it is called and passed to onError
* @param {Function} onError - Error handler. Called with error as argument.
* If an error should not be retried, the handler should re-throw the passed error.
* @param params
* @param {Number} params.retryLimit - the maximum number of retries to attempt before giving up.
*/
export async function retryPromise<T>(
func: () => Promise<T>,
onError: (err: Error, tryCount: number) => void = () => ({}),
params: { retryLimit: number } = { retryLimit: 3 }
): Promise<T> {
let tryCount = 0;

while (tryCount < params.retryLimit) {
tryCount += 1;
try {
return await func();
} catch (err) {
if (err.code === 'ECONNREFUSED') {
onError(err, tryCount);
} else {
throw new Error(err);
}
}

// if we are going to make another attempt, delay first with exponential backoff
if (tryCount < params.retryLimit) {
const secondsToWait = 2 ** (tryCount - 1) + Math.random();
await new Promise((res) => setTimeout(res, Math.round(secondsToWait * 1000)));
}
}

throw new ErrorMaxRetriesExceededError(params.retryLimit);
}
110 changes: 110 additions & 0 deletions modules/express/test/integration/externalSigner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ describe('Custom signing function', () => {
debug: true,
env: 'test',
externalSignerUrl,
timeout: 60000,
};

const app = expressApp(args);
Expand Down Expand Up @@ -85,4 +86,113 @@ describe('Custom signing function', () => {
postProcessPrebuildStub.restore();
verifyTransactionStub.restore();
});

it('should retry requests to external signer on an "ECONNREFUSED" error', async function () {
const bgUrl = Environments.test.uri;
// setup persistent nock to external signer
const signernock = nock(externalSignerUrl)
.post('/api/v2/btc/sign')
.times(3)
.replyWithError({ code: 'ECONNREFUSED' });

// setup nock to wallet platform GET /wallet/fakeid
const wpWalletnock = nock(bgUrl)
.get('/api/v2/btc/wallet/fakeid')
.reply(200, { id: 'fakeid', keys: ['abc', 'def', 'ghi'], coinSpecific: {} });
const wpKeychainNocks = [
nock(bgUrl).get('/api/v2/btc/key/abc').reply(200, { pub: 'xpubabc' }),
nock(bgUrl).get('/api/v2/btc/key/def').reply(200, { pub: 'xpubdef' }),
nock(bgUrl).get('/api/v2/btc/key/ghi').reply(200, { pub: 'xpubghi' }),
];
const wpLatestBlockNock = nock(bgUrl).get('/api/v2/btc/public/block/latest').reply(200);

const wpBuildnock = nock(bgUrl)
.post('/api/v2/btc/wallet/fakeid/tx/build')
.reply(200, { wpBuild: 'WP build response' });

const postProcessPrebuildStub = sinon.stub(Btc.prototype, 'postProcessPrebuild').resolvesArg(0);
const verifyTransactionStub = sinon.stub(Btc.prototype, 'verifyTransaction').resolves(true);

const clock = sinon.useFakeTimers();

// make request to express application to initiate send
const resultPromise = agent
.post('/api/v2/btc/wallet/fakeid/sendcoins')
.type('json')
.send({ address: 'abc', amount: 123 });

// every 10 "fake" ms, push the clock forward another 10 "fake" ms
const interval = clock.setInterval(async () => {
await clock.tickAsync(10);
}, 10);

// start running the setInterval loop above to push the clock forward
clock.next();

const result = await resultPromise;

result.ok.should.be.false();

result.should.have.property('text');
result.text.should.match(/giving up after reaching max retry limit of/);

signernock.done();
wpKeychainNocks.forEach((s) => s.done());
wpLatestBlockNock.done();
wpWalletnock.done();
wpBuildnock.done();

clock.clearInterval(interval as any);
clock.restore();
postProcessPrebuildStub.restore();
verifyTransactionStub.restore();
});

it('should not retry requests to external signer for an error other than "ECONNREFUSED"', async function () {
const bgUrl = Environments.test.uri;
// setup nocks to external signer
const signernock = nock(externalSignerUrl).post('/api/v2/btc/sign').replyWithError({ code: 'not ECONNREFUSED' });

// setup nock to wallet platform GET /wallet/fakeid
const wpWalletnock = nock(bgUrl)
.get('/api/v2/btc/wallet/fakeid')
.reply(200, { id: 'fakeid', keys: ['abc', 'def', 'ghi'], coinSpecific: {} });
const wpKeychainNocks = [
nock(bgUrl).get('/api/v2/btc/key/abc').reply(200, { pub: 'xpubabc' }),
nock(bgUrl).get('/api/v2/btc/key/def').reply(200, { pub: 'xpubdef' }),
nock(bgUrl).get('/api/v2/btc/key/ghi').reply(200, { pub: 'xpubghi' }),
];
const wpLatestBlockNock = nock(bgUrl).get('/api/v2/btc/public/block/latest').reply(200);

const wpBuildnock = nock(bgUrl)
.post('/api/v2/btc/wallet/fakeid/tx/build')
.reply(200, { wpBuild: 'WP build response' });

const postProcessPrebuildStub = sinon.stub(Btc.prototype, 'postProcessPrebuild').resolvesArg(0);
const verifyTransactionStub = sinon.stub(Btc.prototype, 'verifyTransaction').resolves(true);

// check to make sure request to external signer is not attempted a second time
nock.emitter.on('no match', (req) => {
if (req.path === '/api/v2/btc/sign') {
throw new Error(`Unexpected retry request was sent to ${req.path}`);
}
});

// make request to express application to initiate send
const result = await agent
.post('/api/v2/btc/wallet/fakeid/sendcoins')
.type('json')
.send({ address: 'abc', amount: 123 });

result.ok.should.be.false();

signernock.done();
wpKeychainNocks.forEach((s) => s.done());
wpLatestBlockNock.done();
wpWalletnock.done();
wpBuildnock.done();

postProcessPrebuildStub.restore();
verifyTransactionStub.restore();
});
});
55 changes: 53 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2283,13 +2283,20 @@
resolved "https://registry.yarnpkg.com/@sindresorhus/is/-/is-0.14.0.tgz#9fb3a3cf3132328151f353de4632e01e52102bea"
integrity sha512-9NET910DNaIPngYnLLPeg+Ogzqsi9uM4mSboU5y6p8S5DzMTVEsJZrawi+BoDNUVBa2DhJqQYUFvMDfgU062LQ==

"@sinonjs/commons@^1", "@sinonjs/commons@^1.0.2", "@sinonjs/commons@^1.3.0", "@sinonjs/commons@^1.4.0", "@sinonjs/commons@^1.7.0":
"@sinonjs/commons@^1", "@sinonjs/commons@^1.0.2", "@sinonjs/commons@^1.3.0", "@sinonjs/commons@^1.4.0", "@sinonjs/commons@^1.6.0", "@sinonjs/commons@^1.7.0", "@sinonjs/commons@^1.8.3":
version "1.8.3"
resolved "https://registry.yarnpkg.com/@sinonjs/commons/-/commons-1.8.3.tgz#3802ddd21a50a949b6721ddd72da36e67e7f1b2d"
integrity sha512-xkNcLAn/wZaX14RPlwizcKicDk9G3F8m2nU3L7Ukm5zBgTwiT0wsoFAHx9Jq56fJA1z/7uKGtCRu16sOUCLIHQ==
dependencies:
type-detect "4.0.8"

"@sinonjs/fake-timers@>=5", "@sinonjs/fake-timers@^9.0.0":
version "9.1.0"
resolved "https://registry.yarnpkg.com/@sinonjs/fake-timers/-/fake-timers-9.1.0.tgz#8c92c56f195e0bed4c893ba59c8e3d55831ca0df"
integrity sha512-M8vapsv9qQupMdzrVzkn5rb9jG7aUTEPAZdMtME2PuBaefksFZVE2C1g4LBRTkF/k3nRDNbDc5tp5NFC1PEYxA==
dependencies:
"@sinonjs/commons" "^1.7.0"

"@sinonjs/formatio@^3.0.0", "@sinonjs/formatio@^3.2.1":
version "3.2.2"
resolved "https://registry.yarnpkg.com/@sinonjs/formatio/-/formatio-3.2.2.tgz#771c60dfa75ea7f2d68e3b94c7e888a78781372c"
Expand All @@ -2312,6 +2319,15 @@
array-from "^2.1.1"
lodash "^4.17.15"

"@sinonjs/samsam@^6.1.1":
version "6.1.1"
resolved "https://registry.yarnpkg.com/@sinonjs/samsam/-/samsam-6.1.1.tgz#627f7f4cbdb56e6419fa2c1a3e4751ce4f6a00b1"
integrity sha512-cZ7rKJTLiE7u7Wi/v9Hc2fs3Ucc3jrWeMgPHbbTCeVAB2S0wOBbYlkJVeNSL04i7fdhT8wIbDq1zhC/PXTD2SA==
dependencies:
"@sinonjs/commons" "^1.6.0"
lodash.get "^4.4.2"
type-detect "^4.0.8"

"@sinonjs/text-encoding@^0.7.1":
version "0.7.1"
resolved "https://registry.yarnpkg.com/@sinonjs/text-encoding/-/text-encoding-0.7.1.tgz#8da5c6530915653f3a1f38fd5f101d8c3f8079c5"
Expand Down Expand Up @@ -2959,11 +2975,23 @@
dependencies:
"@types/node" "*"

"@types/sinon@^10.0.11":
version "10.0.11"
resolved "https://registry.yarnpkg.com/@types/sinon/-/sinon-10.0.11.tgz#8245827b05d3fc57a6601bd35aee1f7ad330fc42"
integrity sha512-dmZsHlBsKUtBpHriNjlK0ndlvEh8dcb9uV9Afsbt89QIyydpC7NcR+nWlAhASfy3GHnxTl4FX/aKE7XZUt/B4g==
dependencies:
"@types/sinonjs__fake-timers" "*"

"@types/sinon@^7.0.6", "@types/sinon@^7.5.0":
version "7.5.2"
resolved "https://registry.yarnpkg.com/@types/sinon/-/sinon-7.5.2.tgz#5e2f1d120f07b9cda07e5dedd4f3bf8888fccdb9"
integrity sha512-T+m89VdXj/eidZyejvmoP9jivXgBDdkOSBVQjU9kF349NEx10QdPNGxHeZUaj1IlJ32/ewdyXJjnJxyxJroYwg==

"@types/sinonjs__fake-timers@*":
version "8.1.1"
resolved "https://registry.yarnpkg.com/@types/sinonjs__fake-timers/-/sinonjs__fake-timers-8.1.1.tgz#b49c2c70150141a15e0fa7e79cf1f92a72934ce3"
integrity sha512-0kSuKjAS0TrGLJ0M/+8MaFkGsQhZpB6pxOmvS3K8FYI72K//YmdfoW9X2qPsAKh1mkwxGD5zib9s1FIFed6E8g==

"@types/superagent@*", "@types/superagent@^4.1.3":
version "4.1.15"
resolved "https://registry.yarnpkg.com/@types/superagent/-/superagent-4.1.15.tgz#63297de457eba5e2bc502a7609426c4cceab434a"
Expand Down Expand Up @@ -10571,6 +10599,17 @@ nise@^1.4.5, nise@^1.5.2:
lolex "^5.0.1"
path-to-regexp "^1.7.0"

nise@^5.1.1:
version "5.1.1"
resolved "https://registry.yarnpkg.com/nise/-/nise-5.1.1.tgz#ac4237e0d785ecfcb83e20f389185975da5c31f3"
integrity sha512-yr5kW2THW1AkxVmCnKEh4nbYkJdB3I7LUkiUgOvEkOp414mc2UMaHMA7pjq1nYowhdoJZGwEKGaQVbxfpWj10A==
dependencies:
"@sinonjs/commons" "^1.8.3"
"@sinonjs/fake-timers" ">=5"
"@sinonjs/text-encoding" "^0.7.1"
just-extend "^4.0.2"
path-to-regexp "^1.7.0"

no-case@^3.0.4:
version "3.0.4"
resolved "https://registry.yarnpkg.com/no-case/-/no-case-3.0.4.tgz#d361fd5c9800f558551a8369fc0dcd4662b6124d"
Expand Down Expand Up @@ -12949,6 +12988,18 @@ simple-git@^1.85.0:
dependencies:
debug "^4.0.1"

sinon@^13.0.1:
version "13.0.1"
resolved "https://registry.yarnpkg.com/sinon/-/sinon-13.0.1.tgz#2a568beca2084c48985dd98e276e065c81738e3c"
integrity sha512-8yx2wIvkBjIq/MGY1D9h1LMraYW+z1X0mb648KZnKSdvLasvDu7maa0dFaNYdTDczFgbjNw2tOmWdTk9saVfwQ==
dependencies:
"@sinonjs/commons" "^1.8.3"
"@sinonjs/fake-timers" "^9.0.0"
"@sinonjs/samsam" "^6.1.1"
diff "^5.0.0"
nise "^5.1.1"
supports-color "^7.2.0"

sinon@^6.3.5:
version "6.3.5"
resolved "https://registry.yarnpkg.com/sinon/-/sinon-6.3.5.tgz#0f6d6a5b4ebaad1f6e8e019395542d1d02c144a0"
Expand Down Expand Up @@ -13731,7 +13782,7 @@ supports-color@^5.3.0, supports-color@^5.5.0:
dependencies:
has-flag "^3.0.0"

supports-color@^7.1.0:
supports-color@^7.1.0, supports-color@^7.2.0:
version "7.2.0"
resolved "https://registry.yarnpkg.com/supports-color/-/supports-color-7.2.0.tgz#1b7dcdcb32b8138801b3e478ba6a51caa89648da"
integrity sha512-qpCAvRl9stuOHveKsn7HncJRvv501qIacKzQlO/+Lwxc9+0q2wLyv4Dfvt80/DPn2pqOBsJdDiogXGR9+OvwRw==
Expand Down