Secp256k1 in parity crypto and alternative wasm32 implementation #80

cheme · 2018-11-15T17:17:02Z

Removal of rust-crypto

This PR superseed #14 , also probably nice to have for openethereum/parity-ethereum#7915

RustCrypto hashing crate are already used Parity-ethereum (deps from new hyper version).

AES, on the other hand is new, possible change:

adopt a conservative approach and restore old unmaintained AES rust-crypto implementation and put RustCrypto in an alt implementation crate.

Also note that we can enable aes-ni while building for a nice bench boost (requires RUSTFLAGS="-C target-feature=+aes,+sse2,+ssse3" cargo bench) :

aes_ctr/10000           time:   [3.5767 us 3.5778 us 3.5789 us]                           
                        change: [-98.174% -98.174% -98.173%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild
aes_ctr/100000          time:   [36.707 us 36.740 us 36.778 us]                            
                        change: [-98.091% -98.089% -98.087%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 14 outliers among 100 measurements (14.00%)

Alternate implementation

This PR exposes alternate implementation (mainly for wasm32 usecase), the choice of crates is less secure but it is an alternate implementation (wasm32 does not give the same garantie as native code).

Possible change:

add feature switch to make them default implementation (for custom build for instance)

secp256k1

basic integration

A quick and dirty basic integration was running correctly at this point (really conservative) : https://github.com/cheme/parity-common/blob/wasm-test/parity-crypto/src/secp256k1.rs and https://github.com/cheme/parity-common/blob/wasm-test/parity-crypto/src/secp256k1_alt.rs .

This can still be an alternative but is not really fit for a common crate.

trait

Using the previous basic integration reveals some small discrepancy between both crates.
At the end it seems obvious that using a trait to reduce exposed functionality makes senses to ensure compatibility in the long term.

A trait was define for secp in module traits.

Some alternative:

At a some point I made PublicKey and SecretKey implements AsRef<u8>, but I revert back (if we want to maintain a cache of serialize value it should be at parity-ethereum/ethkey instead).
If reverting back to being AsRef<u8> use fixed-hash for inner cache, using H512 to get efficient equality, and derive ordering, probably not it should be done at ethkey level.
define Signature as a trait, I see signature as a 'use once' operation so I keept things light with bytes here.
Public key size for secp to 65 (currently 64 without uncompressed tag) could be more correct.
Add default value to PublicKey : this was done at some point but feels wrong.
error changes the way debug trait behave, originally the native error where displayed in debug mode from json rpc therefore parity ethereum rpc changes a bit. I only identified one message, so I do not think a compatibility layer is needed, but that is an open question.

Secret Store

On my parity branch (https://github.com/cheme/parity-ethereum/tree/crypto-only), secret store switch to the secp trait did not work well due to a few design point:

default public key use in test (and asymption on its first position in btree), the value is an invalid publickey that cannot be spawn.

Currently on branch there is a mix of NodeId aka H512 and actual full PublicKey, with a lot of conversion between each. Moreover there is
a artificial unsafe Default implementation in secp256k1.rs.

Possibility:

do not switch secret-store : keep dependancy on secp crate for secret-store only
revert to a more conservative approach : use NodeId (H512) everywhere and Public only for spawning and maths
Integrate Public everywhere (except Serialize objects) by very involved refactoring
keep as is on the branch, even if tests are passing I am not to confident with that, and there is already a lot of byte copy involved.

bench

I initiated some bench, but there is not much.
bench --feature alt to get both secp implementation benched (alt tests are run without this feature it is related to an issue with bench not running with deps embedding test code).

clear on drop

crate clear_on_drop is used and exposed, MemZero from parity-ethereum has been copied to parity common and can both use clean_on_drop or previous implementation.

Possibility:

I realize while creating this pr that I use internally ClearOnDrop instead of MemZero, it could make sense to use MemZero

Next steps

I did fail at keeping the PR small (especially when considering https://github.com/cheme/parity-ethereum/tree/crypto-only).
Next step is integration into parity-ethereum (after pushing crate version of course), then others PR to use trait for other crypto things. Doing things in a iterative way.

Another important thing is to make the crate no_std compatible (at least the traits and some implementations).

Also a TODO in secp alt impl for clearing memory (we need an unsafe method to access inner field of secret key).

Some alternative:

wait for better math design (to replace ethstore and ethkey secret store impls)
wait for other traits impl.

Note that we use RustCrypto aes root crates : so if we build with RUSTFLAGS='-C target-feature=+aes,+ssse3,+sse2' on x86, AES native instruction will be used. Also added two Aes encryptor struct in preparation of switching (only two use case remaining at the time in 'network-devp2p' crate.

…into RustCrypto

Remove unused scrypt features.

soon).

something in a trait to factor tests) secp256k1, derived from parity ethereum usage. Add an alternate implementation that compiles to wasm32 (from sorpass secp crate).

needed (some details of call from parity-ethereum are not understandable otherwhise).

First compat is OsRng, which would probably soon be useless (0.6 of rand?). Make it works half 0.4/0.5 , it would be a great idea to move all crates to 0.5.

condition with single thread threadpool (see ethkey command)).

- hook print to redirect globally to console. - memap to fix compilation errors : do not use - time - fs : very incomplete (would need path support, still metadata could be quick and simple to add).

Clear_on_drop usage.

…AsRef<u8>`).

struct).

Impl alt secp as asym trait. Add missing files from previous commits.

parity-crypto/src/secp256k1_alt.rs

sorpaas · 2018-11-20T15:01:50Z

Published libsecp256k1 0.2 so that we can test this without affecting existing users (if any!).

dvdplm · 2018-11-22T13:02:11Z

mem/Cargo.toml

+[package]
+name = "mem"
+version = "0.1.0"
+authors = ["Parity Technologies <admin@parity.io>"]


Need description, license and repository info here.

The crate was copied from https://github.com/paritytech/parity-ethereum/tree/master/util/mem so staying with gpl 3 license.

Sure. Just make sure you add the required fields to the [package] section here (or else it can't be published).

mem/Cargo.toml

mem/src/lib.rs

dvdplm · 2018-11-22T13:04:03Z

mem/Cargo.toml

+clear_on_drop = "0.2.3"
+
+[features]
+# when activated mem is removed through volatile primitive instead of clear_on_drop crate


This should probably go into the README/module docs

I am not sure about 'README/module', just added a README file to mem.

That's fine, I just meant "make sure this info is in either the README or in the module-level docs (or both)".

parity-crypto/Cargo.toml

parity-crypto/src/hmac_alt.rs

parity-crypto/src/secp256k1.rs

parity-crypto/src/secp256k1_alt.rs

dvdplm · 2018-11-22T13:59:57Z

parity-crypto/src/traits/asym.rs

+// You should have received a copy of the GNU General Public License
+// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
+
+//! asymetric trait


Need more docs here.

twittner

I probably lack some context but it is my understanding that this PR attempts to achieve three objectives:

Remove rust-crypto.
Enable cross-compilation to wasm32 target.
Add support for secp256k1.

Item 3 in particular seems complicated given that it depends on eth-secp256k1 and libsecp256k1. As for item 2, the APIs of the "alt" implementations differ enough (at least in some types) to make seamless cross-compilation not quite possible yet.

Instead of having this single PR, would it be possible to move from 1 to 3 one PR at a time?

twittner · 2018-11-22T14:39:57Z

mem/src/lib.rs

+	}
+}
+
+#[cfg(not(feature = "volatile-erase"))]


When should volatile-erase not be used and why offering clear_on_drop and write_volatile?

I did use 'clear_on_drop' in my implementation (it is a bit more flexible api-wise) in other contexts and feel like using a single erasure mechanism was simpler.
The crate in itself got its own advantages (cesarb/clear_on_drop#2), yet for me it was just a matter of giving the choice (under current configuration it is imported with 'volatile' feature activated in parity-crypto).

I think in this case "less is more". I think we should have a single way of doing memory zeroing.

twittner · 2018-11-22T15:16:31Z

parity-crypto/src/hmac.rs

+		signer1.update(&input[..]);
+		for i in 0 .. big_input.len() / 33 {
+			signer2.update(&big_input[i*33..(i+1)*33]);
+		}


Would you mind explaining what this is about?

Just running the update on slices that are not 64 byte multiple, a best test would use different sizes.

twittner · 2018-11-22T15:58:36Z

parity-crypto/src/aes.rs

+impl AesEcb256 {
+
+	/// New encoder/decoder, no iv for ecb
+	#[inline]


Is LLVM not smart enough to figure out when to inline and when not to? Could bin crates not turn on LTO to achieve the same effect as having #[inline] everywhere?

Yes it is a bad habit I have to add those, I will rem them in next commit.

twittner · 2018-11-22T16:02:44Z

parity-crypto/src/aes.rs

+	let salt = include_bytes!("../test/salt1");
+	let content = include_bytes!("../test/content");
+	let ctr_enc = include_bytes!("../test/result_128_ctr");
+	let cbc_enc = include_bytes!("../test/result_128_cbc");


Given that all those files contain only small amounts of data, why not inlining those test values?

cheme · 2018-11-23T14:05:37Z

@twittner

Instead of having this single PR, would it be possible to move from 1 to 3 one PR at a time?

Could do, it will be probably in 1-3-2 order. Only thing will be that my current parity-ethereum branch testing this pr will be hard to align, but it do not really have to if I maintain a sync of those three branch with this branch.

dvdplm · 2019-05-17T06:09:43Z

@cheme what is the status of this work? Should we close this in favour of the three split PRs you mention above?

cheme · 2019-05-17T07:03:37Z

I do not really know, the main point of this pr is to be able to compile parity-ethereum to webassembly.
It also go in a different way than recent wasm compatibility pr: it uses feature a lot when recent pr focused on removing non compatible eth implementation. eg aes-gcm
Here we use multiple implementation behind a trait, that is a totally different direction for the crate (this pr see this crate as a unified place for parity related crypto implementation with trait to allow easy inter-operabillity, cross implementation testing, auditability and such but it seems it was not really a goal).
I will still rebase, because whereas switching from ring to dalek seems good, switching to other secp for wasm may be another question.
The PR could probably be close afterward, close pr still have the visibility so if we need to get secp to wasm this could be revive easily.

Warn: mem drop is totally deprecated (should stop using clear on drop : I think it can break wasm).

cheme · 2019-05-17T08:19:16Z

also merging this requires a pr on ethereum size that is pretty impacting and maybe not worth it if we do not want to compile to wasm; therefore I am closing the PR.

cheme added 30 commits August 6, 2018 14:34

Merge branch 'master' of https://github.com/paritytech/parity-common …

07b3afc

…into RustCrypto

Merge branch 'master' into RustCrypto

e6a4d8a

Remove unused scrypt features.

Build to wasm32 (hmac and pbkdf2 will probably need to be implemented

0cc2661

soon).

temp use of previous ring version

c20f544

Alternate hmac impl

c19caa5

hmac verify in test and is_equal method

2da14e4

In progress of moving secp256k1 in crypto

41d4c78

Add basic (type and proto are currently bad and should probably move to

0083acf

something in a trait to factor tests) secp256k1, derived from parity ethereum usage. Add an alternate implementation that compiles to wasm32 (from sorpass secp crate).

Some last munute change for wasm secp, usage of a trait is definitely

a502a11

needed (some details of call from parity-ethereum are not understandable otherwhise).

Added some compat wasm crate.

ccb7f0a

First compat is OsRng, which would probably soon be useless (0.6 of rand?). Make it works half 0.4/0.5 , it would be a great idea to move all crates to 0.5.

Single threadpool compat.

f606daa

Add single direction single thread mpsc (can be use under certain

5b2aef6

condition with single thread threadpool (see ethkey command)).

ring to 0.13

c57fb09

Merge branch 'master' into wasm-test

0ec49ac

home_dir to '/home' for browser.

85d2f80

dummy memmap

58c9e72

Allow heapsize on wasm32.

41481dd

Pbkdf2 RustCrypto alternative.

4b72742

tabs

1c5b4b1

Missing file for snappy compat.

3ff4e30

Some wasmp compat.

37a38dc

- hook print to redirect globally to console. - memap to fix compilation errors : do not use - time - fs : very incomplete (would need path support, still metadata could be quick and simple to add).

Keep only crypto changes.

85707f9

Merge branch 'crypto-tmp' into crypto-compat

4aed419

moving mem from parity-ethereum to parity-common and generalizing

6ab9fb9

Clear_on_drop usage.

Check point commit.

a976b47

Switching to more aligned to parity-eth code base refacto (make key `…

e41feab

…AsRef<u8>`).

Switch arithmetic trait to simple mut ref (better for using with type

a24dbd0

struct).

Test async trait and bench it too.

9445702

Impl alt secp as asym trait. Add missing files from previous commits.

No as_ref buffer usage.

6b5e6fa

sorpaas reviewed Nov 20, 2018

View reviewed changes

parity-crypto/src/secp256k1_alt.rs Show resolved Hide resolved

cheme added 2 commits November 20, 2018 17:45

Switch libsecp version.

0117a5f

Remove last maths from secp256k1_alt

ac0eb93

dvdplm mentioned this pull request Nov 21, 2018

Use libsecp256k1 for secio handshakes libp2p/rust-libp2p#668

Closed

dvdplm reviewed Nov 22, 2018

View reviewed changes

dvdplm requested a review from twittner November 22, 2018 14:04

twittner reviewed Nov 23, 2018

View reviewed changes

Use rust-secp256k1 fork aligned to current master.

aa0bdca

Fix import, remove inlines.

ed370cc

cheme added A1-onice and removed A0-pleasereview labels Nov 30, 2018

cheme added 4 commits November 30, 2018 12:06

Switch to RustCrypto.

6a6a111

Test values directly define in code.

7a3d75d

Merge branch 'RustCrypto2' into crypto-compat

abb0134

Missing from merge

862533f

This was referenced Nov 30, 2018

[parity-crypto] Allow easier implementation switch and specific targets #84

Closed

rust-crypto dependency removal #85

Merged

cheme added 3 commits December 3, 2018 16:50

Move tests in their own file and remove macros

351b964

Merge branch 'RustCrypto2' into crypto-compat

82b04ac

remove part of macro (TODO remove all and move alt to module)

3c36377

sorpaas mentioned this pull request Feb 25, 2019

Added Drop to SharedSecret and SecretKey paritytech/libsecp256k1#9

Merged

cheme added 2 commits May 17, 2019 09:58

Merge branch 'master' into crypto-compat

bb81eb3

Warn: mem drop is totally deprecated (should stop using clear on drop : I think it can break wasm).

rename of package is now stable

ec212b0

cheme closed this May 17, 2019

ordian mentioned this pull request Sep 3, 2019

Move ethkey crypto utils to parity crypto crate #210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secp256k1 in parity crypto and alternative wasm32 implementation #80

Secp256k1 in parity crypto and alternative wasm32 implementation #80

cheme commented Nov 15, 2018 •

edited

Loading

sorpaas commented Nov 20, 2018

dvdplm Nov 22, 2018

cheme Nov 23, 2018

dvdplm Nov 27, 2018

dvdplm Nov 22, 2018

cheme Nov 23, 2018

dvdplm Nov 27, 2018

dvdplm Nov 22, 2018

twittner left a comment

twittner Nov 22, 2018

cheme Nov 23, 2018

dvdplm Nov 27, 2018

twittner Nov 22, 2018

cheme Nov 23, 2018

twittner Nov 22, 2018

cheme Nov 23, 2018

twittner Nov 22, 2018

cheme Nov 23, 2018

cheme commented Nov 23, 2018

dvdplm commented May 17, 2019

cheme commented May 17, 2019 •

edited

Loading

cheme commented May 17, 2019

Secp256k1 in parity crypto and alternative wasm32 implementation #80

Secp256k1 in parity crypto and alternative wasm32 implementation #80

Conversation

cheme commented Nov 15, 2018 • edited Loading

Removal of rust-crypto

Alternate implementation

secp256k1

basic integration

trait

Secret Store

bench

clear on drop

Next steps

sorpaas commented Nov 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twittner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheme commented Nov 23, 2018

dvdplm commented May 17, 2019

cheme commented May 17, 2019 • edited Loading

cheme commented May 17, 2019

cheme commented Nov 15, 2018 •

edited

Loading

cheme commented May 17, 2019 •

edited

Loading