Fix legacy key recovery. #775
Conversation
37ng
changed the title
LegacyDelegatedSignature ::recover_signer & fix sign_with_legacy_keyrecover_signer & sign_with_legacy_key
37ng
changed the title
recover_signer & sign_with_legacy_key| let wallet: LocalWallet = hex::encode(legacy_private_key).parse::<LocalWallet>()?; | ||
| let signature = wallet.sign_message(signature_text.clone()).await?; | ||
|
|
There was a problem hiding this comment.
This is the old logic:
let legacy_private_key = secp256k1.bytes;
let (mut delegating_signature, recovery_id) = k256_helper::sign_sha256(
&legacy_private_key, // secret_key
// TODO: Verify this will create a verifiable signature
signature_text.as_bytes(), // message
)
.map_err(IdentityError::LegacySignature)?;
delegating_signature.push(recovery_id); // TODO: normalize recovery ID if necessary
Out of curiosity, what is the reason we need to change this to construct a LocalWallet out of the legacy_private_key here?
There was a problem hiding this comment.
Because LocalWallet wraps the signature_text with EIP-191 text while old logic doesn't. EIP-191 is implemented by evm wallet like Metamask. If EIP-191 is not included here, we won't be able to recover the signature to the signer address properly.
We can add the EIP-191 to the old logic, but I feel it's better to abstract away EIP-191 from our codebase.
There was a problem hiding this comment.
ethers crate source code to add EIP-191 during signing, note the hash_message is used here and also in recovery logic.
There was a problem hiding this comment.
Ahhh that explains a lot, thank you!
| @@ -183,7 +183,7 @@ mod tests { | |||
| async fn legacy_keys() { | |||
| // This test is supposed to be run with a fresh state where account_address has not been registered. | |||
| // Subsequent runs will use a different code path. | |||
| let account_address = "0x0bD00B21aF9a2D538103c3AAf95Cb507f8AF1B28".to_lowercase(); | |||
| let account_address = "0x0bd00b21af9a2d538103c3aaf95cb507f8af1b28"; | |||
There was a problem hiding this comment.
Will this test fail if we leave it with a mix of upper and lower case? It might almost be better to test it without normalizing the address
There was a problem hiding this comment.
Yes the current use of strings for account addresses is a bit hacky and I'm planning to replace all account addresses with ethers::types::Address which is a byte array with necessary type conversion logics. I think it'd be a better canonical type than string or lower case string.
Before we do that, I'm ok to leave these as is since they won't be used anyways. wdty?
There was a problem hiding this comment.
To answer your question, yes the test will fail because we have to_lowercase() all over the code base now.
I think we already use EIP-191 to sign & recover, LocalWallet is wrapped in InboxOwner. code here Also we are using a new |
Resolves this comment.
Derive address from legacy public key(instead of recovering the signature) and make sure it matches the address of the wallet that signed the legacy key.
Use ethers::LocalWallet to sign the message so that
EIP-191is abstracted away, which is the reason current signature cannot be recovered properly.Moved
sign_with_legacy_keyfrom xmtp_mls to xmtp_id so that we can use it in tests without introducing circular dependencies.