-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Rust]: Implement Private and Public keys #3107
Conversation
* Add tests to check if `k256` suits our requirements
* Add `tw_hash::Hash<N>` * Implement `tw_keeper::secp256k1` Private, Public keys, signing and verifying
* Export FFI interface
* Add `secp256k1::KeyPair` structure
* Move `TWPrivateKey` and `TWPublicKey` to the `tw_keypair::tw` module * Refactor `TWPublicKey` by making it enum * TODO add `TWKeyPair`
* Add a documentation * Comment out `secp256k1::PrivateKey::shared_key_hash` as it's not finished yet
* Add a PoC canonical signing
* Remove `tw_starknet`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nicely designed structures and impls. The types should have //!
documentation, however. Also, I'd like to note that the RustCrypto project explicitly states:
The secp256k1 elliptic curve arithmetic contained in this crate has never been independently audited!
Just wanted to mention that, but I assume you already discussed this with @Milerius. Regarding the testing custom cryptography implementations, I'd generally recommend testing this against well-established tools such as openssl
(e.g. generating tons of random data, signing the data with both tools and then comparing the output), but if you copied the tests from C++ then this is probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add support for this crate: https://crates.io/crates/zeroize to zeroize memory after usage and signing in general?
Probably ok to delete C++ tests that are replicated/moved to rust then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first review iteration, globally excellent job, let's ensure for top clarity here
* CI Run tests in WASM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear in the pr description that we introduce k256
to implement ecdsa signature; I think we don't need to use TW
prefix like TWPrivateKey
, it made me confused with the C interface.
Also this note from https://github.com/RustCrypto/elliptic-curves/tree/master/k256 should be highlighted, do we have a checklist for the auditor to take more eyes on?
This crate has been designed with the goal of ensuring that secret-dependent secp256k1 operations are performed in constant time (using the subtle crate and constant-time formulas). However, it has not been thoroughly assessed to ensure that generated assembly is constant time on common CPU architectures. USE AT YOUR OWN RISK! |
@hewigovens @lamafab thank you for noticing. We've scheduled a call with the security team.
|
It's actually C Interfaces generated Rust<->C through cbindgen, at some point the idea is to generate the TW bindings this way too. |
* Implement `Serialize` and `Deserialize` for `Hash<N>`
@hewigovens @Milerius @lamafab thank you for the advices! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Description
Transaction signing is the key responsibility of wallet-core. We need to implement the
PrivateKey
andPublicKey
structures to be able to move one of the blockchains from C++ to Rust.How to test
Run Rust and C++ tests
Types of changes
TWPrivateKey
andTWPublicKey
structures in Rust.secp256k1
curve.Note
Found an issue in k256 crate - RustCrypto/elliptic-curves#886
Fixed by disabling the
precomputed-tables
feature.Checklist
TWPrivateKey
andTWPublicKey
high-level structures in RustTWPrivateKey
andTWPublicKey
FFI typessecp256k1
signing with a canonical checking: https://github.com/trustwallet/wallet-core/blob/master/src/PrivateKey.cpp#L253 - PoCsecp256k1
getting shared key hash: https://github.com/trustwallet/wallet-core/blob/master/src/PrivateKey.cpp#L175-L191 - PoClinux-rust-ci
step