diff --git a/modules/sdk-coin-xlm/src/getStellarKeys.ts b/modules/sdk-coin-xlm/src/getStellarKeys.ts index b1f1039fad..e17208c50e 100644 --- a/modules/sdk-coin-xlm/src/getStellarKeys.ts +++ b/modules/sdk-coin-xlm/src/getStellarKeys.ts @@ -1,19 +1,20 @@ -import { BitGoBase, ExternalEd25519Signature, InitiateRecoveryOptions } from '@bitgo/sdk-core'; +import { BitGoBase, ExternalEd25519Signer, InitiateRecoveryOptions } from '@bitgo/sdk-core'; import * as stellar from 'stellar-sdk'; import { Utils } from './lib'; export async function getStellarKeys( bitgo: BitGoBase, - params: InitiateRecoveryOptions, - externalBackupSignature?: ExternalEd25519Signature + params: Omit & { backupKey?: string }, + externalBackupSigner?: ExternalEd25519Signer ): Promise { const keys: stellar.Keypair[] = []; let userKey = params.userKey; let backupKey = params.backupKey; // Stellar's Ed25519 public keys start with a G, while private keys start with an S - const isKrsRecovery = !externalBackupSignature && backupKey.startsWith('G') && !userKey.startsWith('G'); - const isUnsignedSweep = !externalBackupSignature && backupKey.startsWith('G') && userKey.startsWith('G'); + const hasPublicBackupKey = backupKey !== undefined && backupKey.startsWith('G'); + const isKrsRecovery = !externalBackupSigner && hasPublicBackupKey && !userKey.startsWith('G'); + const isUnsignedSweep = !externalBackupSigner && hasPublicBackupKey && userKey.startsWith('G'); /** * Get User Key @@ -42,13 +43,16 @@ export async function getStellarKeys( /** * Get Backup Key */ - if (externalBackupSignature) { + if (externalBackupSigner) { keys.push( stellar.Keypair.fromPublicKey( - stellar.StrKey.encodeEd25519PublicKey(Buffer.from(externalBackupSignature.publicKey, 'hex')) + stellar.StrKey.encodeEd25519PublicKey(Buffer.from(externalBackupSigner.publicKey, 'hex')) ) ); } else { + if (!backupKey) { + throw new Error('backupKey is required for non-bitgo recovery when no external backup signer is provided'); + } try { if (!backupKey.startsWith('S') && !isKrsRecovery && !isUnsignedSweep) { backupKey = await bitgo.decrypt({ diff --git a/modules/sdk-coin-xlm/src/xlm.ts b/modules/sdk-coin-xlm/src/xlm.ts index 1947c8ce8a..9a92404a2f 100644 --- a/modules/sdk-coin-xlm/src/xlm.ts +++ b/modules/sdk-coin-xlm/src/xlm.ts @@ -23,8 +23,9 @@ import { checkKrsProvider, common, ErrorNoInputToRecover, - assertValidExternalEd25519Signature, - ExternalEd25519Signature, + assertValidExternalEd25519Signer, + ExternalEd25519Signer, + signWithExternalEd25519Signer, ExtraPrebuildParamsOptions, InvalidAddressError, InvalidMemoIdError, @@ -66,7 +67,7 @@ interface Memo { interface InitiateRecoveryOptions { userKey: string; - backupKey: string; + backupKey?: string; recoveryDestination: string; krsProvider?: string; walletPassphrase?: string; @@ -74,8 +75,8 @@ interface InitiateRecoveryOptions { interface RecoveryOptions extends InitiateRecoveryOptions { rootAddress?: string; - /** Optional pre-made signature for the backup child, whose signature must be over the transaction hash (`tx.hash()`). */ - externalBackupSignature?: ExternalEd25519Signature; + /** Signs `tx.hash()` once the network-dependent transaction fields are fixed. */ + externalBackupSigner?: ExternalEd25519Signer; } interface RecoveryTransaction { @@ -740,19 +741,25 @@ export class Xlm extends BaseCoin { params.userKey = Utils.encodePublicKey(Buffer.from(params.userKey, 'hex')); } - if (Utils.isValidRootPrivateKey(params.backupKey)) { - params.backupKey = Utils.encodePrivateKey(Buffer.from(params.backupKey.slice(0, 64), 'hex')); - } else if (Utils.isValidRootPublicKey(params.backupKey)) { - params.backupKey = Utils.encodePublicKey(Buffer.from(params.backupKey, 'hex')); + if (params.backupKey) { + if (Utils.isValidRootPrivateKey(params.backupKey)) { + params.backupKey = Utils.encodePrivateKey(Buffer.from(params.backupKey.slice(0, 64), 'hex')); + } else if (Utils.isValidRootPublicKey(params.backupKey)) { + params.backupKey = Utils.encodePublicKey(Buffer.from(params.backupKey, 'hex')); + } } - const hasExternalBackupSig = Boolean(params.externalBackupSignature); - if (params.externalBackupSignature) { - assertValidExternalEd25519Signature(params.externalBackupSignature); + const externalBackupSigner = params.externalBackupSigner; + if (externalBackupSigner) { + assertValidExternalEd25519Signer(externalBackupSigner); + } + if (!externalBackupSigner && !params.backupKey) { + throw new Error('backupKey is required for non-bitgo recovery when no external backup signer is provided'); } // Stellar's Ed25519 public keys start with a G, while private keys start with an S - const isKrsRecovery = !hasExternalBackupSig && params.backupKey.startsWith('G') && !params.userKey.startsWith('G'); - const isUnsignedSweep = !hasExternalBackupSig && params.backupKey.startsWith('G') && params.userKey.startsWith('G'); + const hasPublicBackupKey = params.backupKey !== undefined && params.backupKey.startsWith('G'); + const isKrsRecovery = !externalBackupSigner && hasPublicBackupKey && !params.userKey.startsWith('G'); + const isUnsignedSweep = !externalBackupSigner && hasPublicBackupKey && params.userKey.startsWith('G'); if (isKrsRecovery) { checkKrsProvider(this, params.krsProvider); @@ -762,7 +769,7 @@ export class Xlm extends BaseCoin { throw new InvalidAddressError('Invalid destination address!'); } - const [userKey, backupKey] = await getStellarKeys(this.bitgo, params, params.externalBackupSignature); + const [userKey, backupKey] = await getStellarKeys(this.bitgo, params, externalBackupSigner); if (!params.rootAddress || !stellar.StrKey.isValidEd25519PublicKey(params.rootAddress)) { throw new Error(`Invalid wallet address: ${params.rootAddress}`); @@ -850,12 +857,9 @@ export class Xlm extends BaseCoin { } if (!isKrsRecovery && !isUnsignedSweep) { - const externalBackupSignature = params.externalBackupSignature; - if (externalBackupSignature) { - tx.addSignature( - backupKey.publicKey(), - Buffer.from(externalBackupSignature.signature, 'hex').toString('base64') - ); + if (externalBackupSigner) { + const signature = await signWithExternalEd25519Signer(externalBackupSigner, tx.hash()); + tx.addSignature(backupKey.publicKey(), Buffer.from(signature, 'hex').toString('base64')); } else { tx.sign(backupKey); } diff --git a/modules/sdk-coin-xlm/test/unit/xlm.ts b/modules/sdk-coin-xlm/test/unit/xlm.ts index 54845632c4..e817112804 100644 --- a/modules/sdk-coin-xlm/test/unit/xlm.ts +++ b/modules/sdk-coin-xlm/test/unit/xlm.ts @@ -558,7 +558,7 @@ describe('XLM:', function () { .should.be.rejectedWith(RecoveryProviderError); }); - describe('getStellarKeys with external backup signature', function () { + describe('getStellarKeys with external backup signer', function () { it('builds a pub-only backup Keypair from the external public key', async function () { const userKeypair = stellar.Keypair.random(); const externalBackupKeypair = stellar.Keypair.random(); @@ -566,17 +566,150 @@ describe('XLM:', function () { bitgo, { userKey: userKeypair.secret(), - backupKey: 'GC3D3ZNNK7GHLMSWJA54DQO6QJUJJF7K6J5JGCEW45ZT6QMKZ6PMUHUM', recoveryDestination: 'GDDHCKMYYYCVXOSAVMSEIYGYNX74LIAV3ACXYQ6WPMDUF7W3KZNWTHTH', }, { publicKey: externalBackupKeypair.rawPublicKey().toString('hex'), - signature: 'ab'.repeat(64), + sign: async () => ({ signature: 'ab'.repeat(64) }), } ); userKey.publicKey().should.equal(userKeypair.publicKey()); backupKey.publicKey().should.equal(externalBackupKeypair.publicKey()); }); + + it('rejects when neither a backupKey nor an external backup signer is provided', async function () { + const userKeypair = stellar.Keypair.random(); + await assert.rejects( + () => + getStellarKeys(bitgo, { + userKey: userKeypair.secret(), + recoveryDestination: 'GDDHCKMYYYCVXOSAVMSEIYGYNX74LIAV3ACXYQ6WPMDUF7W3KZNWTHTH', + }), + /backupKey is required/ + ); + }); + }); + + describe('recover with an external backup signer', function () { + const userKeypair = stellar.Keypair.fromSecret(userKeychain.prv); + const backupKeypair = stellar.Keypair.fromSecret(backupKeychain.prv); + const publicKey = backupKeypair.rawPublicKey().toString('hex'); + const params = { + userKey: userKeychain.prv, + rootAddress: userKeychain.pub, + recoveryDestination: 'GDDHCKMYYYCVXOSAVMSEIYGYNX74LIAV3ACXYQ6WPMDUF7W3KZNWTHTH', + }; + + beforeEach(function () { + nock.disableNetConnect(); + nock('https://horizon-testnet.stellar.org/accounts') + .get('/' + params.rootAddress) + .reply(200, { + sequence: '35995558267060226', + balances: [{ asset_type: 'native', balance: '6500000000' }], + signers: [ + { key: userKeypair.publicKey(), weight: 1, type: 'ed25519_public_key' }, + { key: backupKeypair.publicKey(), weight: 1, type: 'ed25519_public_key' }, + ], + }); + + nock('https://horizon-testnet.stellar.org/accounts') + .get('/' + params.recoveryDestination) + .reply(200, { sequence: '35995558267060213', balances: 13131313 }); + + nock('https://horizon-testnet.stellar.org') + .get('/ledgers') + .query({ order: 'desc', limit: 1 }) + .reply(200, { + records: [{ base_reserve_in_stroops: '5000000', base_fee_in_stroops: 100 }], + }) + .persist(); + }); + + afterEach(function () { + nock.cleanAll(); + nock.enableNetConnect(); + }); + + it('should attach verifiable user and external backup signatures over the transaction hash', async function () { + const signedMessages: Buffer[] = []; + const recovery = await basecoin.recover({ + ...params, + externalBackupSigner: { + publicKey, + sign: async (message: Uint8Array) => { + signedMessages.push(Buffer.from(message)); + return { signature: backupKeypair.sign(Buffer.from(message)).toString('hex') }; + }, + }, + }); + + const tx = new stellar.Transaction(recovery.txBase64, basecoin.getStellarNetwork()); + assert.strictEqual(tx.signatures.length, 2); + assert.ok(userKeypair.verify(tx.hash(), tx.signatures[0].signature())); + assert.ok(backupKeypair.verify(tx.hash(), tx.signatures[1].signature())); + assert.deepStrictEqual(signedMessages, [tx.hash()]); + }); + + it('should preserve recovery signed with the local backup seed', async function () { + const recovery = await basecoin.recover({ ...params, backupKey: backupKeychain.prv }); + const tx = new stellar.Transaction(recovery.txBase64, basecoin.getStellarNetwork()); + + assert.strictEqual(tx.signatures.length, 2); + assert.ok(userKeypair.verify(tx.hash(), tx.signatures[0].signature())); + assert.ok(backupKeypair.verify(tx.hash(), tx.signatures[1].signature())); + }); + + for (const { description, signature, error } of [ + { description: 'does not verify', signature: 'ab'.repeat(64), error: /does not verify against its public key/ }, + { description: 'is malformed', signature: 'ab', error: /expected 64 bytes/ }, + ]) { + it(`should reject an external backup signer whose signature ${description}`, async function () { + await assert.rejects( + () => + basecoin.recover({ + ...params, + externalBackupSigner: { publicKey, sign: async () => ({ signature }) }, + }), + error + ); + }); + } + + it('should reject an external backup signer with a malformed public key', async function () { + await assert.rejects( + () => + basecoin.recover({ + ...params, + externalBackupSigner: { + publicKey: 'not-hex', + sign: async () => ({ signature: 'ab'.repeat(64) }), + }, + }), + /expected 32 bytes/ + ); + }); + + it('should propagate an external backup signer failure', async function () { + const signingError = new Error('external signer unavailable'); + await assert.rejects( + () => + basecoin.recover({ + ...params, + externalBackupSigner: { + publicKey, + sign: async () => { + throw signingError; + }, + }, + }), + (error) => error === signingError + ); + }); + + it('should require a backup key when no external signer is provided', async function () { + await assert.rejects(() => basecoin.recover(params), /backupKey is required/); + }); }); it('should throw RecoveryKeyMismatchError when the user key is not an account signer', async function () { diff --git a/modules/sdk-core/test/unit/bitgo/safe/derivableEd25519Pub.ts b/modules/sdk-core/test/unit/bitgo/safe/derivableEd25519Pub.ts index df98ad75a0..92e2cdf4b9 100644 --- a/modules/sdk-core/test/unit/bitgo/safe/derivableEd25519Pub.ts +++ b/modules/sdk-core/test/unit/bitgo/safe/derivableEd25519Pub.ts @@ -275,6 +275,21 @@ describe('derivableEd25519Pub', function () { ).should.be.rejectedWith(/does not verify against its public key/); }); + it('rejects a signature over mutated bytes without changing the original message', async function () { + const input = Uint8Array.from(message); + await signWithExternalEd25519Signer( + { + publicKey, + sign: async (m: Uint8Array) => { + m[0] ^= 0xff; + return { signature: Buffer.from(nacl.sign.detached(m, keyPair.secretKey)).toString('hex') }; + }, + }, + input + ).should.be.rejectedWith(/does not verify against its public key/); + Buffer.from(input).equals(message).should.equal(true); + }); + it('signs and returns a verified 64-byte signature', async function () { const signature = await signWithExternalEd25519Signer(validSigner, message); signature.should.match(/^[0-9a-f]{128}$/);