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
13 changes: 12 additions & 1 deletion modules/express/src/clientRoutes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,15 @@ function getWalletPwFromEnv(walletId: string): string {
return walletPw;
}

/**
* Returns the wallet passphrase from the environment, or undefined if not set.
* Unlike getWalletPwFromEnv, this does not throw when the env variable is absent.
Comment thread
mrdanish26 marked this conversation as resolved.
* Use this when the passphrase is optional (e.g. KMS-backed wallets).
*/
function findWalletPwFromEnv(walletId: string): string | undefined {
return process.env[`WALLET_${walletId}_PASSPHRASE`];
}

async function getEncryptedPrivKey(path: string, walletId: string): Promise<string> {
const privKeyFile = await fs.readFile(path, { encoding: 'utf8' });
const encryptedPrivKey = JSON.parse(privKeyFile);
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.

+1 to zahin's thread: collapse findWalletPwFromEnv and getWalletPwFromEnv into one function

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.

Will update it in a follow up PR

Expand Down Expand Up @@ -629,7 +638,9 @@ export async function handleV2OFCSignPayload(
throw new ApiResponseError(`Could not find OFC wallet ${walletId}`, 404);
}

const walletPassphrase = bodyWalletPassphrase || getWalletPwFromEnv(wallet.id());
// Prefer the passphrase from the request body; fall back to the env var.
// If neither is present, pass undefined — signPayload() routes to KMS internally.
const walletPassphrase = bodyWalletPassphrase ?? findWalletPwFromEnv(wallet.id());
const tradingAccount = wallet.toTradingAccount();
const stringifiedPayload = typeof payload === 'string' ? payload : JSON.stringify(payload);
const signature = await tradingAccount.signPayload({
Expand Down
93 changes: 93 additions & 0 deletions modules/express/test/unit/typedRoutes/ofcSignPayload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,103 @@ describe('OfcSignPayload codec tests', function () {
const decodedResponse = assertDecode(OfcSignPayloadResponse200, result.body);
assert.strictEqual(decodedResponse.signature, mockSignPayloadResponse.signature);

// Verify env passphrase was forwarded to signPayload
const signCall = mockTradingAccount.signPayload.getCall(0);
assert.ok(signCall, 'tradingAccount.signPayload should have been called');
assert.strictEqual(signCall.args[0].walletPassphrase, 'env_passphrase', 'env passphrase should be forwarded');

// Cleanup environment variable
delete process.env['WALLET_ofc-wallet-id-123_PASSPHRASE'];
});

it('should pass undefined walletPassphrase to signPayload when no passphrase in body or env (KMS path)', async function () {
const requestBody = {
walletId: 'ofc-wallet-id-no-passphrase',
payload: { amount: '1000000', currency: 'USD' },
// no walletPassphrase
};

// Ensure no env var is set for this wallet
delete process.env['WALLET_ofc-wallet-id-no-passphrase_PASSPHRASE'];

const mockTradingAccount = {
signPayload: sinon.stub().resolves(mockSignPayloadResponse.signature),
};

const mockWallet = {
id: () => requestBody.walletId,
toTradingAccount: sinon.stub().returns(mockTradingAccount),
};

const walletsGetStub = sinon.stub().resolves(mockWallet);
const mockWallets = { get: walletsGetStub };
const mockCoin = { wallets: sinon.stub().returns(mockWallets) };
sinon.stub(BitGo.prototype, 'coin').returns(mockCoin as any);

const result = await agent
.post('/api/v2/ofc/signPayload')
.set('Authorization', 'Bearer test_access_token_12345')
.set('Content-Type', 'application/json')
.send(requestBody);

assert.strictEqual(result.status, 200);
const decodedResponse = assertDecode(OfcSignPayloadResponse200, result.body);
assert.strictEqual(decodedResponse.signature, mockSignPayloadResponse.signature);

// signPayload must be called with walletPassphrase=undefined so the SDK routes to KMS
const signCall = mockTradingAccount.signPayload.getCall(0);
assert.ok(signCall, 'tradingAccount.signPayload should have been called');
assert.strictEqual(
signCall.args[0].walletPassphrase,
undefined,
'walletPassphrase should be undefined to trigger KMS signing'
);
});

it('should prefer body walletPassphrase over env passphrase', async function () {
const requestBody = {
walletId: 'ofc-wallet-id-123',
payload: { amount: '500' },
walletPassphrase: 'body_passphrase',
};

// Set a different env passphrase — body should win
process.env['WALLET_ofc-wallet-id-123_PASSPHRASE'] = 'env_passphrase';

const mockTradingAccount = {
signPayload: sinon.stub().resolves(mockSignPayloadResponse.signature),
};

const mockWallet = {
id: () => requestBody.walletId,
toTradingAccount: sinon.stub().returns(mockTradingAccount),
};

const walletsGetStub = sinon.stub().resolves(mockWallet);
const mockWallets = { get: walletsGetStub };
const mockCoin = { wallets: sinon.stub().returns(mockWallets) };
sinon.stub(BitGo.prototype, 'coin').returns(mockCoin as any);

const result = await agent
.post('/api/v2/ofc/signPayload')
.set('Authorization', 'Bearer test_access_token_12345')
.set('Content-Type', 'application/json')
.send(requestBody);

assert.strictEqual(result.status, 200);

// body passphrase should take precedence
const signCall = mockTradingAccount.signPayload.getCall(0);
assert.ok(signCall, 'tradingAccount.signPayload should have been called');
assert.strictEqual(
signCall.args[0].walletPassphrase,
'body_passphrase',
'body passphrase should take precedence over env'
);

delete process.env['WALLET_ofc-wallet-id-123_PASSPHRASE'];
});

it('should successfully sign complex nested JSON payload', async function () {
const requestBody = {
walletId: 'ofc-wallet-id-123',
Expand Down
7 changes: 6 additions & 1 deletion modules/sdk-core/src/bitgo/trading/iTradingAccount.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { ITradingNetwork } from './network';

/**
* Parameters for the signing a payload from the trading account
* @param payload - The payload to sign
* @param walletPassphrase - The passphrase of the wallet that will be used to decrypt the user key and sign the payload. If not provided, the BitGo key will be used.
*/
export interface SignPayloadParameters {
payload: string | Record<string, unknown>;
walletPassphrase: string;
walletPassphrase?: string;
Comment thread
alextse-bg marked this conversation as resolved.
}

export interface ITradingAccount {
Expand Down
7 changes: 2 additions & 5 deletions modules/sdk-core/src/bitgo/trading/network/network.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export class TradingNetwork implements ITradingNetwork {

/**
* Prepare an allocation for submission
* @param {string} walletPassphrase ofc wallet passphrase
* @param {string} walletPassphrase ofc wallet passphrase - required only when signing via user key
* @param {string} connectionId connection to whom to make the allocation or deallocation
* @param {string=} clientExternalId one time generated uuid v4
* @param {string} currency currency for which the allocation should be made. e.g. btc / tbtc
Expand All @@ -130,10 +130,7 @@ export class TradingNetwork implements ITradingNetwork {
}

const payload = JSON.stringify(body);

const prv = await this.wallet.getPrv({ walletPassphrase });
const signedBuffer: Buffer = await this.wallet.baseCoin.signMessage({ prv }, payload);
const signature = signedBuffer.toString('hex');
const signature = await this.wallet.toTradingAccount().signPayload({ payload, walletPassphrase });

return {
...body,
Expand Down
2 changes: 1 addition & 1 deletion modules/sdk-core/src/bitgo/trading/network/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ export type GetNetworkAllocationByIdResponse = {
};

export type PrepareNetworkAllocationParams = Omit<CreateNetworkAllocationParams, 'payload' | 'signature'> & {
walletPassphrase: string;
walletPassphrase?: string;
clientExternalId?: string;
nonce?: string;
};
Expand Down
40 changes: 39 additions & 1 deletion modules/sdk-core/src/bitgo/trading/tradingAccount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,51 @@ export class TradingAccount implements ITradingAccount {
}

/**
* Signs an arbitrary payload with the user key on this trading account
* Signs an arbitrary payload. Use the user key if passphrase is provided, or the BitGo key if not.
* @param params
* @param params.payload arbitrary payload object (string | Record<string, unknown>)
* @param params.walletPassphrase passphrase on this trading account, used to unlock the account user key
* @returns hex-encoded signature of the payload
*/
async signPayload(params: SignPayloadParameters): Promise<string> {
// if no passphrase is provided, attempt to sign using the wallet's bitgo key remotely
if (!params.walletPassphrase) {
return this.signPayloadByBitGoKey(params);
}
Comment thread
alextse-bg marked this conversation as resolved.
// if a passphrase is provided, we must be trying to sign using the user private key - decrypt and sign locally
return this.signPayloadByUserKey(params);
}

/**
* Signs the payload of a trading account via the trading account BitGo key
* @param params
* @private
*/
private async signPayloadByBitGoKey(params: Omit<SignPayloadParameters, 'walletPassphrase'>): Promise<string> {
const walletData = this.wallet.toJSON();
if (walletData.userKeySigningRequired) {
throw new Error(
'Wallet must use user key to sign ofc transaction, please provide the wallet passphrase or visit your wallet settings page to configure one.'
);
}
if (walletData.keys.length < 2) {
throw new Error(
'Wallet does not support BitGo signing. Please reach out to support@bitgo.com to resolve this issue.'
);
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.

when params.payload is a string, .send(string) sends a plain text body, not JSON. the user-key path stringifies first, this should match

}
Comment thread
alextse-bg marked this conversation as resolved.

const url = this.wallet.url('/tx/sign');
const { signature } = await this.wallet.bitgo.post(url).send(params.payload).result();

return signature;
}

/**
* Signs the payload of a trading account locally by fetching the user's encrypted private key and decrypt using passphrase
* @param params
* @private
*/
private async signPayloadByUserKey(params: SignPayloadParameters): Promise<string> {
const key = (await this.wallet.baseCoin.keychains().get({ id: this.wallet.keyIds()[0] })) as any;
const prv = this.wallet.bitgo.decrypt({
input: key.encryptedPrv,
Expand Down
1 change: 1 addition & 0 deletions modules/sdk-core/src/bitgo/wallet/iWallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,7 @@ export interface WalletData {
evmKeyRingReferenceWalletId?: string;
isParent?: boolean;
enabledChildChains?: string[];
userKeySigningRequired?: string;
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.

userKeySigningRequired is typed string but the guard treats it as boolean. "false" from the API would incorrectly block BitGo signing. should be boolean

}

export interface RecoverTokenOptions {
Expand Down
21 changes: 21 additions & 0 deletions modules/sdk-core/src/coins/ofc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
SignTransactionOptions,
VerifyAddressOptions,
VerifyTransactionOptions,
Wallet,
} from '../';

export class Ofc extends BaseCoin {
Expand Down Expand Up @@ -104,6 +105,26 @@ export class Ofc extends BaseCoin {
throw new MethodNotImplementedError();
}

/**
* Signs a message using a trading wallet's BitGo Key
* @param wallet - uses the BitGo key of this trading wallet to sign the message remotely in a KMS
* @param message
*/
async signMessage(wallet: Wallet, message: string): Promise<Buffer>;
/**
* Signs a message using the private key
* @param key - uses the private key to sign the message
* @param message
*/
async signMessage(key: { prv: string }, message: string): Promise<Buffer>;
async signMessage(keyOrWallet: { prv: string } | Wallet, message: string): Promise<Buffer> {
if (!(keyOrWallet instanceof Wallet)) {
return super.signMessage(keyOrWallet, message);
}
const signatureHexString = await keyOrWallet.toTradingAccount().signPayload({ payload: message });
return Buffer.from(signatureHexString, 'hex');
}

/** @inheritDoc */
auditDecryptedKey(params: AuditDecryptedKeyParams) {
throw new MethodNotImplementedError();
Expand Down
20 changes: 16 additions & 4 deletions modules/sdk-core/src/coins/ofcToken.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
SignTransactionOptions as BaseSignTransactionOptions,
SignedTransaction,
ITransactionRecipient,
Wallet,
} from '../';
import { isBolt11Invoice } from '../lightning';

Expand All @@ -18,7 +19,8 @@ export interface SignTransactionOptions extends BaseSignTransactionOptions {
txPrebuild: {
payload: string;
};
prv: string;
prv?: string;
wallet?: Wallet;
}

export { OfcTokenConfig };
Expand Down Expand Up @@ -107,15 +109,25 @@ export class OfcToken extends Ofc {
}

/**
* Assemble keychain and half-sign prebuilt transaction
* Signs a half-signed OFC transaction.
* Signs the transaction remotely using the BitGo key if prv is not provided.
* @param params
* @returns {Promise<SignedTransaction>}
*/
async signTransaction(params: SignTransactionOptions): Promise<SignedTransaction> {
const txPrebuild = params.txPrebuild;
const payload = txPrebuild.payload;
const signatureBuffer = (await this.signMessage(params, payload)) as any;
const signature: string = signatureBuffer.toString('hex');

let signature: string;
if (params.wallet) {
signature = await params.wallet.toTradingAccount().signPayload({ payload, walletPassphrase: params.prv });
} else if (params.prv) {
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.

params.prv arriving here via wallet.signTransaction is already a decrypted key (from getUserPrv), not a passphrase. passing it as walletPassphrase to signPayload will cause signPayloadByUserKey to use it as a decryption password against encryptedPrv, which won't work

const signatureBuffer = (await this.signMessage({ prv: params.prv }, payload)) as any;
signature = signatureBuffer.toString('hex');
} else {
throw new Error('You must pass in either one of wallet or prv');
}

return { halfSigned: { payload, signature } } as any;
}

Expand Down
Loading
Loading