From 6d13114a845c3b7d4fb463bdf8f82ef1e182fcb8 Mon Sep 17 00:00:00 2001 From: Pranish Nepal Date: Fri, 23 Oct 2026 12:25:30 -0400 Subject: [PATCH] feat: migrate recover to ExternalEd25519Signer Migrate XLM and HBAR recover() from the pre-made externalBackupSignature hook to the shared lazy ExternalEd25519Signer, signing tx.hash() (XLM) and built bodyBytes (HBAR) via signWithExternalEd25519Signer. backupKey is now optional when an external signer is supplied. Also harden the shared helper to hand the callback a copy of the message so signed bytes cannot be mutated after the fact. Ticket: WCN-3754 --- modules/sdk-coin-xlm/src/getStellarKeys.ts | 18 ++- modules/sdk-coin-xlm/src/xlm.ts | 46 +++--- modules/sdk-coin-xlm/test/unit/xlm.ts | 139 +++++++++++++++++- .../unit/bitgo/safe/derivableEd25519Pub.ts | 15 ++ 4 files changed, 187 insertions(+), 31 deletions(-) diff --git a/modules/sdk-coin-xlm/src/getStellarKeys.ts b/modules/sdk-coin-xlm/src/getStellarKeys.ts index b1f1039fad5..e17208c50e6 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 1947c8ce8a3..9a92404a2f3 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 54845632c43..e8171128048 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 df98ad75a0c..92e2cdf4b9e 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}$/);