Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(abstract-utxo): update the keySignature verification network #3697

Merged
merged 2 commits into from
Jun 30, 2023
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
37 changes: 29 additions & 8 deletions modules/abstract-utxo/src/abstractUtxoCoin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ const { getExternalChainCode, isChainCode, scriptTypeForChain, outputScripts } =
type Unspent<TNumber extends number | bigint = number> = bitgo.Unspent<TNumber>;

type RootWalletKeys = bitgo.RootWalletKeys;

export interface VerifyAddressOptions extends BaseVerifyAddressOptions {
chain: number;
index: number;
Expand Down Expand Up @@ -280,9 +281,9 @@ export interface RecoverFromWrongChainOptions {
}

export interface VerifyKeySignaturesOptions {
userKeychain?: Keychain;
keychainToVerify?: Keychain;
keySignature?: string;
userKeychain: { pub?: string };
keychainToVerify: { pub?: string };
keySignature: string;
}

export interface VerifyUserPublicKeyOptions {
Expand Down Expand Up @@ -677,7 +678,7 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
* @param {VerifyKeySignaturesOptions} params
* @return {{backup: boolean, bitgo: boolean}}
*/
protected verifyKeySignature(params: VerifyKeySignaturesOptions): boolean {
public verifyKeySignature(params: VerifyKeySignaturesOptions): boolean {
// first, let's verify the integrity of the user key, whose public key is used for subsequent verifications
const { userKeychain, keychainToVerify, keySignature } = params;
if (!userKeychain) {
Expand All @@ -695,10 +696,16 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
// verify the signature against the user public key
assert(userKeychain.pub);
const publicKey = bip32.fromBase58(userKeychain.pub).publicKey;
// Due to interface of `bitcoinMessage`, we need to convert the public key to an address.
// Note that this address has no relationship to on-chain transactions. We are
// only interested in the address as a representation of the public key.
const signingAddress = utxolib.address.toBase58Check(
utxolib.crypto.hash160(publicKey),
utxolib.networks.bitcoin.pubKeyHash,
this.network
// we do not pass `this.network` here because it would fail for zcash
// the bitcoinMessage library decodes the address and throws away the first byte
// because zcash has a two-byte prefix, verify() decodes zcash addresses to an invalid pubkey hash
utxolib.networks.bitcoin
rushilbg marked this conversation as resolved.
Show resolved Hide resolved
);

// BG-5703: use BTC mainnet prefix for all key signature operations
Expand Down Expand Up @@ -740,7 +747,13 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
if (!keySignature) {
throw new Error(`missing required custom change ${KeyIndices[keyIndex].toLowerCase()} keychain signature`);
}
if (!this.verifyKeySignature({ userKeychain, keychainToVerify, keySignature })) {
if (
!this.verifyKeySignature({
userKeychain: userKeychain as { pub: string },
keychainToVerify: keychainToVerify as { pub: string },
keySignature,
})
) {
debug('failed to verify custom change %s key signature!', KeyIndices[keyIndex].toLowerCase());
return false;
}
Expand Down Expand Up @@ -810,8 +823,16 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
// let's verify these keychains
const keySignatures = parsedTransaction.keySignatures;
if (!_.isEmpty(keySignatures)) {
const verify = (key, pub) =>
this.verifyKeySignature({ userKeychain: keychains.user, keychainToVerify: key, keySignature: pub });
const verify = (key, pub) => {
if (!keychains.user || !keychains.user.pub) {
throw new Error('missing user keychain');
}
return this.verifyKeySignature({
userKeychain: keychains.user as { pub: string },
keychainToVerify: key,
keySignature: pub,
});
};
const isBackupKeySignatureValid = verify(keychains.backup, keySignatures.backupPub);
const isBitgoKeySignatureValid = verify(keychains.bitgo, keySignatures.bitgoPub);
if (!isBackupKeySignatureValid || !isBitgoKeySignatureValid) {
Expand Down
31 changes: 31 additions & 0 deletions modules/bitgo/test/v2/unit/coins/utxo/keySignatures.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import * as assert from 'assert';
import { AbstractUtxoCoin } from '@bitgo/abstract-utxo';
import { Keychain } from '@bitgo/sdk-core';

import { utxoCoins } from './util';

function describeWithCoin(coin: AbstractUtxoCoin) {
describe(`verifyKeySignatures for ${coin.getChain()}`, function () {
it('should verify key signature of ZEC', async () => {
const userKeychain = await coin.keychains().create();
const backupKeychain = await coin.keychains().create();
const bitgoKeychain = await coin.keychains().create();
Comment on lines +10 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const userKeychain = await coin.keychains().create();
const backupKeychain = await coin.keychains().create();
const bitgoKeychain = await coin.keychains().create();
const [ userKeychain, backupKeychain, bitgoKeychain] = await Promise.all([
coin.keychains().create(),
coin.keychains().create(),
coin.keychains().create()
]);

NIT: we can use promise.all instead of serially creating keychains

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe we are making http requests here anyway though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are synchronous operations, Promise.all won't speed them up.


const signatures = await coin.createKeySignatures(
userKeychain.prv,
{ pub: backupKeychain.pub as string },
{ pub: bitgoKeychain.pub as string }
);

assert.ok(
await coin.verifyKeySignature({
userKeychain: userKeychain as unknown as Keychain,
keychainToVerify: backupKeychain as unknown as Keychain,
keySignature: signatures.backup,
})
);
});
});
}

utxoCoins.forEach((coin) => describeWithCoin(coin));
22 changes: 22 additions & 0 deletions modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,28 @@ export abstract class BaseCoin implements IBaseCoin {
return signMessage(message, bip32.fromBase58(key.prv), utxolib.networks.bitcoin);
}

/**
* Create signatures for the backup and bitgo keys using the user key.
* We can verify the signatures when fetching the keys from wallet-platform later.
* Currently only `AbstractUtxoCoin` implements and uses the complementary `verifyKeySignature` method.
* @param prv - the user private key
* @param backupKeychain - contains the backup public key
* @param bitgoKeychain - contains the bitgo public key
*/
public async createKeySignatures(
prv: string,
backupKeychain: { pub: string },
bitgoKeychain: { pub: string }
): Promise<{
backup: string;
bitgo: string;
}> {
return {
backup: (await this.signMessage({ prv }, backupKeychain.pub)).toString('hex'),
bitgo: (await this.signMessage({ prv }, bitgoKeychain.pub)).toString('hex'),
};
}

/**
* Decompose a raw transaction into useful information.
* @param options - coin-specific
Expand Down
8 changes: 8 additions & 0 deletions modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,14 @@ export interface IBaseCoin {
baseUnitsToBigUnits(baseUnits: string | number): string;
bigUnitsToBaseUnits(bigUnits: string | number): string;
signMessage(key: { prv: string }, message: string): Promise<Buffer>;
createKeySignatures(
prv: string,
backupKeychain: { pub: string },
bitgoKeychain: { pub: string }
): Promise<{
backup: string;
bitgo: string;
}>;
explainTransaction(options: Record<string, any>): Promise<ITransactionExplanation<any, string | number> | undefined>;
verifyTransaction(params: VerifyTransactionOptions): Promise<boolean>;
verifyAddress(params: VerifyAddressOptions): Promise<boolean>;
Expand Down