[Rust]: Implement Private and Public keys #3107

satoshiotomakan · 2023-04-21T09:25:09Z

Description

Transaction signing is the key responsibility of wallet-core. We need to implement the PrivateKey and PublicKey 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

Add and implement TWPrivateKey and TWPublicKey structures in Rust.
Export Private and Public keys FFI
Implement the secp256k1 curve.

Note

Found an issue in k256 crate - RustCrypto/elliptic-curves#886
Fixed by disabling the precomputed-tables feature.

Checklist

Explore the Rust crates that provides the secp256k1 implementation.
Implement TWPrivateKey and TWPublicKey high-level structures in Rust
Export TWPrivateKey and TWPublicKey FFI types
Add multiple unit and integration tests (Rust->C++, Rust)
Implementing Shared Key hashing
Implement and well test secp256k1 signing with a canonical checking: https://github.com/trustwallet/wallet-core/blob/master/src/PrivateKey.cpp#L253 - PoC
Implement and well test secp256k1 getting shared key hash: https://github.com/trustwallet/wallet-core/blob/master/src/PrivateKey.cpp#L175-L191 - PoC
Integrate Starkex curve
Run Rust tests in wasm32-unknown-emscripten as a linux-rust-ci step

* 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`

lamafab

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.

rust/tw_keypair/src/tw/public.rs

rust/tw_starknet/src/ffi.rs

rust/tw_keypair/src/tw/private.rs

rust/tw_keypair/tests/private_key_ffi_tests.rs

rust/tw_keypair/src/tw/private.rs

rust/tw_keypair/tests/private_key_ffi_tests.rs

rust/tw_keypair/src/secp256k1/private.rs

Milerius

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.

Milerius

first review iteration, globally excellent job, let's ensure for top clarity here

rust/tw_keypair/src/secp256k1/signature.rs

rust/tw_keypair/src/starkex/mod.rs

* CI Run tests in WASM

hewigovens

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.

rust/Cargo.toml

rust/tw_hash/src/hash_array.rs

tests/chains/Ethereum/SignerTests.cpp

rust/tw_keypair/src/tw/mod.rs

hewigovens · 2023-04-25T10:29:32Z

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?

⚠️ Security Warning
The secp256k1 elliptic curve arithmetic contained in this crate has never been independently audited!

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!

satoshiotomakan · 2023-04-25T12:28:14Z

@hewigovens @lamafab thank you for noticing. We've scheduled a call with the security team.

do we have a checklist for the auditor to take more eyes on?

Milerius · 2023-04-25T12:53:48Z

I think we don't need to use TW prefix like TWPrivateKey, it made me confused with the C interface.

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>`

satoshiotomakan · 2023-04-26T08:53:43Z

@hewigovens @Milerius @lamafab thank you for the advices!
The PR is ready for the next review.

hewigovens

lgtm

satoshiotomakan added 8 commits April 18, 2023 17:20

Save dev state

e5e8836

* Add tests to check if `k256` suits our requirements

[wip] Save dev state

821d95a

* Add `tw_hash::Hash<N>` * Implement `tw_keeper::secp256k1` Private, Public keys, signing and verifying

[wip] Save dev state

4fe5649

* Export FFI interface

[wip] Integrate 'Rust::tw_private_key_sign' into C++ code

6efe740

[wip] Rename tw_keeper crate to tw_keypair

15406f5

* Add `secp256k1::KeyPair` structure

[wip] Make TWPrivateKey and TWPublicKey available for end-user

4ea61be

* Move `TWPrivateKey` and `TWPublicKey` to the `tw_keypair::tw` module * Refactor `TWPublicKey` by making it enum * TODO add `TWKeyPair`

[wip] Integrate Rust public key verifying into C++

44d7f67

* Add a documentation * Comment out `secp256k1::PrivateKey::shared_key_hash` as it's not finished yet

Merge branch 'master' into s/rust-keeper

c888285

satoshiotomakan self-assigned this Apr 21, 2023

satoshiotomakan linked an issue Apr 21, 2023 that may be closed by this pull request

[Rust]: Implement Private and Public keys #3093

Closed

6 tasks

satoshiotomakan added 7 commits April 21, 2023 13:17

[wip] Split secp256k1 module into submodules

675880b

* Add a PoC canonical signing

[wip] Fix secp256k1 getting shared key

6cbbe98

[wip] Move starkex curve from tw_starnet to tw_keypair crate

cbb85db

* Remove `tw_starknet`

[wip] Document FFIs

359ff33

Merge branch 'master' into s/rust-keeper

8ef0662

[wip] Update starknet-crypto and starknet-ff crate

36e16ef

Merge branch 'dev' into s/rust-keeper

c33ce5b

satoshiotomakan changed the base branch from master to dev April 24, 2023 08:34

[r2r] Minor refactoring

720b364

satoshiotomakan marked this pull request as ready for review April 24, 2023 09:54

satoshiotomakan requested review from Milerius and lamafab as code owners April 24, 2023 09:54

[r2r] Update code coverage

0119618

lamafab reviewed Apr 24, 2023

View reviewed changes

satoshiotomakan added 2 commits April 24, 2023 17:12

[r2r] Disable k256 default features to fix WASM

0f5be1f

[r2r] Document tw_keypair structures and methods

dafd136

Milerius requested changes Apr 25, 2023

View reviewed changes

Milerius requested a review from hewigovens April 25, 2023 07:36

Milerius requested changes Apr 25, 2023

View reviewed changes

rust/tw_keypair/src/secp256k1/signature.rs Outdated Show resolved Hide resolved

rust/tw_keypair/src/starkex/mod.rs Outdated Show resolved Hide resolved

[r2r] Zeroize secrets

9017ed2

* CI Run tests in WASM

[r2r] Fix Rust CI

dbefeb3

hewigovens reviewed Apr 25, 2023

View reviewed changes

rust/Cargo.toml Outdated Show resolved Hide resolved

rust/tw_hash/src/hash_array.rs Show resolved Hide resolved

tests/chains/Ethereum/SignerTests.cpp Show resolved Hide resolved

rust/tw_keypair/src/tw/mod.rs Outdated Show resolved Hide resolved

satoshiotomakan added 3 commits April 25, 2023 15:27

[r2r] Add secp256k1 tests

977fcad

* Implement `Serialize` and `Deserialize` for `Hash<N>`

[r2r] Remove TW prefixes

d80eee3

[r2r] Rename tw_utils to tw_misc

1b2695f

Milerius approved these changes Apr 26, 2023

View reviewed changes

hewigovens approved these changes Apr 26, 2023

View reviewed changes

lamafab approved these changes Apr 26, 2023

View reviewed changes

Milerius merged commit cb5be68 into dev Apr 26, 2023

Milerius deleted the s/rust-keeper branch April 26, 2023 10:26

SuperGNUS pushed a commit to GeniusVentures/wallet-core that referenced this pull request May 11, 2023

[Rust]: Implement Private and Public keys (trustwallet#3107)

e7d3cef

satoshiotomakan added a commit that referenced this pull request Jul 17, 2023

[Rust]: Implement Private and Public keys (#3107)

7be9f0e

satoshiotomakan added a commit that referenced this pull request Jul 17, 2023

[Rust]: Implement Private and Public keys (#3107)

ac0fe38

satoshiotomakan added a commit that referenced this pull request Oct 6, 2023

[Rust]: Implement Private and Public keys (#3107)

a565082

satoshiotomakan mentioned this pull request Oct 9, 2023

[Dev/UnderAudit]: Dev<->Master #3120

Merged

12 tasks

EduMenges pushed a commit to GeniusVentures/wallet-core that referenced this pull request Jun 17, 2024

[Rust]: Implement Private and Public keys (trustwallet#3107)

cd29232

EduMenges pushed a commit to GeniusVentures/wallet-core that referenced this pull request Jun 18, 2024

[Rust]: Implement Private and Public keys (trustwallet#3107)

542ace6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rust]: Implement Private and Public keys #3107

[Rust]: Implement Private and Public keys #3107

satoshiotomakan commented Apr 21, 2023 •

edited

Loading

lamafab left a comment

Milerius left a comment

Milerius left a comment

hewigovens left a comment

hewigovens commented Apr 25, 2023

satoshiotomakan commented Apr 25, 2023

Milerius commented Apr 25, 2023

satoshiotomakan commented Apr 26, 2023

hewigovens left a comment

[Rust]: Implement Private and Public keys #3107

[Rust]: Implement Private and Public keys #3107

Conversation

satoshiotomakan commented Apr 21, 2023 • edited Loading

Description

How to test

Types of changes

Note

Checklist

lamafab left a comment

Choose a reason for hiding this comment

Milerius left a comment

Choose a reason for hiding this comment

Milerius left a comment

Choose a reason for hiding this comment

hewigovens left a comment

Choose a reason for hiding this comment

hewigovens commented Apr 25, 2023

satoshiotomakan commented Apr 25, 2023

Milerius commented Apr 25, 2023

satoshiotomakan commented Apr 26, 2023

hewigovens left a comment

Choose a reason for hiding this comment

satoshiotomakan commented Apr 21, 2023 •

edited

Loading