[BC/CWC] Wallet private key updates [1]#4068
[BC/CWC] Wallet private key updates [1]#4068MichaelAJay wants to merge 36 commits intobitpay:masterfrom
Conversation
…sses, & expose method with DeriverProxy
…sses and add passthrough on DeriverProxy
…t to expected form
kajoseph
added
CWC
MichaelAJay
changed the title
kajoseph
left a comment
kajoseph
left a comment
There was a problem hiding this comment.
I think a better approach may be to auto migrate wallets with version < 2 to v2. That way we can avoid carve-outs while actively push better security.
loadWallet() {
const wallet = read wallet file;
if (wallet.version < 2) {
convert to v2 wallet;
backup wallet file to .bak;
overwrite wallet file with v2;
}
}
…loadWallet with 'raw' param
…t-privatekey-ref
…t-privatekey-ref
| const decrypted = decipher.update(encHex, 'hex'); | ||
| const final = decipher.final(); | ||
| try { | ||
| return Buffer.concat([decrypted, final]); |
There was a problem hiding this comment.
Include the decipher.final() in the try.
try {
return Buffer.concat([decrypted, decipher.final()]);
| const final = decipher.final(); | ||
| const output = Buffer.concat([payload, final]); | ||
| try { | ||
| return toBuffer ? output : output.toString('hex'); |
There was a problem hiding this comment.
include decipher.final() in the try.
let final;
try {
final = decipher.final();
const output = Buffer.concat([payload, final]);
return toBuffer ? output : output.toString('hex');
} finally {
payload.fill(0);
final?.fill(0);
}
Not sure if defining final outside the try is necessary. If this code is going to error, it's highly likely that it's due to an invalid checksum/tag in the decipher.final() call, so it may be fine to just do Buffer.concat([payload, decipher.final()]);. Whatever is decided also applies to the other encrypt/decrypt buffer methods.
kajoseph
left a comment
kajoseph
left a comment
There was a problem hiding this comment.
Functionally, this seems to work well. Nice job!
In addition to the minor comments below, we should add test cases for the migration as well as tests for critical functions with the new version wallets (i.e. deriving, sending, sign message)
| } | ||
|
|
||
| async migrateWallet(encryptionKey: Buffer): Promise<Wallet> { | ||
| /** |
There was a problem hiding this comment.
We should log that a migration is taking place. E.g. "Migrating wallet from version x to version y"
| throw new Error('Migration failed - wallet not found'); | ||
| } | ||
|
|
||
| await writeFile(`${this.name}.bak`, rawWallet, 'utf8') |
There was a problem hiding this comment.
Should write to an expected directory instead of just dumping in the cwd. e.g. ~/.bitcore/bitcoreWallets/backup/ (next to where the text wallet files are saved)
| throw new Error('Migration failure: wallet not successfully saved. Use backups to restore prior wallet and keys'); | ||
| }); | ||
|
|
||
| return this; |
There was a problem hiding this comment.
Log migration successful
| } | ||
|
|
||
| /** | ||
| * 1: Wallet to .bak |
There was a problem hiding this comment.
Log that a pre-migration wallet backup is being written to <file>
2 participants
bitcore-client
Encryption
Storage
addKeysSafe- incoming keys' private key is encrypted, so not immediately available to be used to retrieve pubkey, which should be available anyway. Throws if missing a pubkeyWallet
createencrypts HDPrivateKey xprivkey and privateKey so they can be decrypted as buffers instead of serializing the whole masterKey and THEN encryptingimportKeysdoes the same for signing keyssignTxdecrypts to buffers then uses deriver for chain-aware decoding. In the next phase, this decoding should be made unnecessary by passing the buffer all the way through to use, if possibleTests
crypto-wallet-core
Derivation
privateKeyToBufferandprivateKeyBufferToNativePrivateKeyto IDeriver & implement for each Deriver - no need to override for any of the extending classes (UTXOs to AbstractBitcoreLibDeriver & EVM to EthDeriver)